Message ID | 20191024114059.102802-14-frankja@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | KVM: s390: Add support for protected VMs | expand |
On 24.10.19 13:40, Janosch Frank wrote: > From: Michael Mueller <mimu@linux.ibm.com> > > Define the interruption injection codes and the related fields in the > sie control block for PVM interruption injection. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 6cc3b73ca904..82443236d4cc 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -215,7 +215,15 @@ struct kvm_s390_sie_block { > __u8 icptcode; /* 0x0050 */ > __u8 icptstatus; /* 0x0051 */ > __u16 ihcpu; /* 0x0052 */ > - __u8 reserved54[2]; /* 0x0054 */ > + __u8 reserved54; /* 0x0054 */ > +#define IICTL_CODE_NONE 0x00 > +#define IICTL_CODE_MCHK 0x01 > +#define IICTL_CODE_EXT 0x02 > +#define IICTL_CODE_IO 0x03 > +#define IICTL_CODE_RESTART 0x04 > +#define IICTL_CODE_SPECIFICATION 0x10 > +#define IICTL_CODE_OPERAND 0x11 > + __u8 iictl; /* 0x0055 */ > __u16 ipa; /* 0x0056 */ > __u32 ipb; /* 0x0058 */ > __u32 scaoh; /* 0x005c */ > @@ -252,7 +260,8 @@ struct kvm_s390_sie_block { > #define HPID_KVM 0x4 > #define HPID_VSIE 0x5 > __u8 hpid; /* 0x00b8 */ > - __u8 reservedb9[11]; /* 0x00b9 */ > + __u8 reservedb9[7]; /* 0x00b9 */ > + __u32 eiparams; /* 0x00c0 */ > __u16 extcpuaddr; /* 0x00c4 */ > __u16 eic; /* 0x00c6 */ > __u32 reservedc8; /* 0x00c8 */ > @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { > __u8 oai; /* 0x00e2 */ > __u8 armid; /* 0x00e3 */ > __u8 reservede4[4]; /* 0x00e4 */ > - __u64 tecmc; /* 0x00e8 */ > - __u8 reservedf0[12]; /* 0x00f0 */ > + union { > + __u64 tecmc; /* 0x00e8 */ > + struct { > + __u16 subchannel_id; /* 0x00e8 */ > + __u16 subchannel_nr; /* 0x00ea */ > + __u32 io_int_parm; /* 0x00ec */ > + __u32 io_int_word; /* 0x00f0 */ > + }; I only wonder if we should give this member a fitting name, e.g., "ioparams" > + } __packed; > + __u8 reservedf4[8]; /* 0x00f4 */ > #define CRYCB_FORMAT_MASK 0x00000003 > #define CRYCB_FORMAT0 0x00000000 > #define CRYCB_FORMAT1 0x00000001 >
On 30.10.19 16:53, David Hildenbrand wrote: >> @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { >> __u8 oai; /* 0x00e2 */ >> __u8 armid; /* 0x00e3 */ >> __u8 reservede4[4]; /* 0x00e4 */ >> - __u64 tecmc; /* 0x00e8 */ >> - __u8 reservedf0[12]; /* 0x00f0 */ >> + union { >> + __u64 tecmc; /* 0x00e8 */ >> + struct { >> + __u16 subchannel_id; /* 0x00e8 */ >> + __u16 subchannel_nr; /* 0x00ea */ >> + __u32 io_int_parm; /* 0x00ec */ >> + __u32 io_int_word; /* 0x00f0 */ >> + }; > > I only wonder if we should give this member a fitting name, e.g., > "ioparams" Do you see a real gain for that? We have a lot of other unnamed structs defined here as well. > >> + } __packed; >> + __u8 reservedf4[8]; /* 0x00f4 */ >> #define CRYCB_FORMAT_MASK 0x00000003 >> #define CRYCB_FORMAT0 0x00000000 >> #define CRYCB_FORMAT1 0x00000001 >> Thanks, Michael
On 31.10.19 09:48, Michael Mueller wrote: > > > On 30.10.19 16:53, David Hildenbrand wrote: >>> @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { >>> __u8 oai; /* 0x00e2 */ >>> __u8 armid; /* 0x00e3 */ >>> __u8 reservede4[4]; /* 0x00e4 */ >>> - __u64 tecmc; /* 0x00e8 */ >>> - __u8 reservedf0[12]; /* 0x00f0 */ >>> + union { >>> + __u64 tecmc; /* 0x00e8 */ >>> + struct { >>> + __u16 subchannel_id; /* 0x00e8 */ >>> + __u16 subchannel_nr; /* 0x00ea */ >>> + __u32 io_int_parm; /* 0x00ec */ >>> + __u32 io_int_word; /* 0x00f0 */ >>> + }; >> >> I only wonder if we should give this member a fitting name, e.g., >> "ioparams" > > Do you see a real gain for that? We have a lot of other unnamed structs > defined here as well. I was wondering if we could just copy the whole struct when delivering the interrupt. You could even reuse "struct kvm_s390_io_info" here to make that more clear.
On 31.10.19 10:15, David Hildenbrand wrote: > On 31.10.19 09:48, Michael Mueller wrote: >> >> >> On 30.10.19 16:53, David Hildenbrand wrote: >>>> @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { >>>> __u8 oai; /* 0x00e2 */ >>>> __u8 armid; /* 0x00e3 */ >>>> __u8 reservede4[4]; /* 0x00e4 */ >>>> - __u64 tecmc; /* 0x00e8 */ >>>> - __u8 reservedf0[12]; /* 0x00f0 */ >>>> + union { >>>> + __u64 tecmc; /* 0x00e8 */ >>>> + struct { >>>> + __u16 subchannel_id; /* 0x00e8 */ >>>> + __u16 subchannel_nr; /* 0x00ea */ >>>> + __u32 io_int_parm; /* 0x00ec */ >>>> + __u32 io_int_word; /* 0x00f0 */ >>>> + }; >>> >>> I only wonder if we should give this member a fitting name, e.g., >>> "ioparams" >> >> Do you see a real gain for that? We have a lot of other unnamed structs >> defined here as well. > > I was wondering if we could just copy the whole struct when delivering > the interrupt. > > You could even reuse "struct kvm_s390_io_info" here to make that more > clear. I want to keep it the way it is to have the fields in the SCB declaration explicit. Thanks, Michael
On Thu, 24 Oct 2019 07:40:35 -0400 Janosch Frank <frankja@linux.ibm.com> wrote: > From: Michael Mueller <mimu@linux.ibm.com> > > Define the interruption injection codes and the related fields in the > sie control block for PVM interruption injection. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 6cc3b73ca904..82443236d4cc 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -215,7 +215,15 @@ struct kvm_s390_sie_block { > __u8 icptcode; /* 0x0050 */ > __u8 icptstatus; /* 0x0051 */ > __u16 ihcpu; /* 0x0052 */ > - __u8 reserved54[2]; /* 0x0054 */ > + __u8 reserved54; /* 0x0054 */ > +#define IICTL_CODE_NONE 0x00 > +#define IICTL_CODE_MCHK 0x01 > +#define IICTL_CODE_EXT 0x02 > +#define IICTL_CODE_IO 0x03 > +#define IICTL_CODE_RESTART 0x04 > +#define IICTL_CODE_SPECIFICATION 0x10 > +#define IICTL_CODE_OPERAND 0x11 > + __u8 iictl; /* 0x0055 */ > __u16 ipa; /* 0x0056 */ > __u32 ipb; /* 0x0058 */ > __u32 scaoh; /* 0x005c */ > @@ -252,7 +260,8 @@ struct kvm_s390_sie_block { > #define HPID_KVM 0x4 > #define HPID_VSIE 0x5 > __u8 hpid; /* 0x00b8 */ > - __u8 reservedb9[11]; /* 0x00b9 */ > + __u8 reservedb9[7]; /* 0x00b9 */ > + __u32 eiparams; /* 0x00c0 */ > __u16 extcpuaddr; /* 0x00c4 */ > __u16 eic; /* 0x00c6 */ > __u32 reservedc8; /* 0x00c8 */ > @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { > __u8 oai; /* 0x00e2 */ > __u8 armid; /* 0x00e3 */ > __u8 reservede4[4]; /* 0x00e4 */ > - __u64 tecmc; /* 0x00e8 */ > - __u8 reservedf0[12]; /* 0x00f0 */ > + union { > + __u64 tecmc; /* 0x00e8 */ > + struct { > + __u16 subchannel_id; /* 0x00e8 */ > + __u16 subchannel_nr; /* 0x00ea */ > + __u32 io_int_parm; /* 0x00ec */ > + __u32 io_int_word; /* 0x00f0 */ > + }; > + } __packed; > + __u8 reservedf4[8]; /* 0x00f4 */ IIUC, for protected guests, you won't get an interception for which tecmc would be valid anymore, but need to put the I/O interruption stuff at the same place, right? My main issue is that this makes the control block definition a bit ugly, since the f0 value that's unused in the non-protvirt case is not obvious anymore; but I don't know how to express this without making it even uglier :( > #define CRYCB_FORMAT_MASK 0x00000003 > #define CRYCB_FORMAT0 0x00000000 > #define CRYCB_FORMAT1 0x00000001
On 05.11.19 18:51, Cornelia Huck wrote: > On Thu, 24 Oct 2019 07:40:35 -0400 > Janosch Frank <frankja@linux.ibm.com> wrote: > >> From: Michael Mueller <mimu@linux.ibm.com> >> >> Define the interruption injection codes and the related fields in the >> sie control block for PVM interruption injection. >> >> Signed-off-by: Michael Mueller <mimu@linux.ibm.com> >> --- >> arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++---- >> 1 file changed, 21 insertions(+), 4 deletions(-) >> >> diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h >> index 6cc3b73ca904..82443236d4cc 100644 >> --- a/arch/s390/include/asm/kvm_host.h >> +++ b/arch/s390/include/asm/kvm_host.h >> @@ -215,7 +215,15 @@ struct kvm_s390_sie_block { >> __u8 icptcode; /* 0x0050 */ >> __u8 icptstatus; /* 0x0051 */ >> __u16 ihcpu; /* 0x0052 */ >> - __u8 reserved54[2]; /* 0x0054 */ >> + __u8 reserved54; /* 0x0054 */ >> +#define IICTL_CODE_NONE 0x00 >> +#define IICTL_CODE_MCHK 0x01 >> +#define IICTL_CODE_EXT 0x02 >> +#define IICTL_CODE_IO 0x03 >> +#define IICTL_CODE_RESTART 0x04 >> +#define IICTL_CODE_SPECIFICATION 0x10 >> +#define IICTL_CODE_OPERAND 0x11 >> + __u8 iictl; /* 0x0055 */ >> __u16 ipa; /* 0x0056 */ >> __u32 ipb; /* 0x0058 */ >> __u32 scaoh; /* 0x005c */ >> @@ -252,7 +260,8 @@ struct kvm_s390_sie_block { >> #define HPID_KVM 0x4 >> #define HPID_VSIE 0x5 >> __u8 hpid; /* 0x00b8 */ >> - __u8 reservedb9[11]; /* 0x00b9 */ >> + __u8 reservedb9[7]; /* 0x00b9 */ >> + __u32 eiparams; /* 0x00c0 */ >> __u16 extcpuaddr; /* 0x00c4 */ >> __u16 eic; /* 0x00c6 */ >> __u32 reservedc8; /* 0x00c8 */ >> @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { >> __u8 oai; /* 0x00e2 */ >> __u8 armid; /* 0x00e3 */ >> __u8 reservede4[4]; /* 0x00e4 */ >> - __u64 tecmc; /* 0x00e8 */ >> - __u8 reservedf0[12]; /* 0x00f0 */ >> + union { >> + __u64 tecmc; /* 0x00e8 */ >> + struct { >> + __u16 subchannel_id; /* 0x00e8 */ >> + __u16 subchannel_nr; /* 0x00ea */ >> + __u32 io_int_parm; /* 0x00ec */ >> + __u32 io_int_word; /* 0x00f0 */ >> + }; >> + } __packed; >> + __u8 reservedf4[8]; /* 0x00f4 */ > > IIUC, for protected guests, you won't get an interception for which > tecmc would be valid anymore, but need to put the I/O interruption > stuff at the same place, right? Yes, the format 4 architecture defines this. > > My main issue is that this makes the control block definition a bit > ugly, since the f0 value that's unused in the non-protvirt case is not > obvious anymore; but I don't know how to express this without making it > even uglier :( :) > >> #define CRYCB_FORMAT_MASK 0x00000003 >> #define CRYCB_FORMAT0 0x00000000 >> #define CRYCB_FORMAT1 0x00000001 > Thanks Michael
On 24/10/2019 13.40, Janosch Frank wrote: > From: Michael Mueller <mimu@linux.ibm.com> > > Define the interruption injection codes and the related fields in the > sie control block for PVM interruption injection. > > Signed-off-by: Michael Mueller <mimu@linux.ibm.com> > --- > arch/s390/include/asm/kvm_host.h | 25 +++++++++++++++++++++---- > 1 file changed, 21 insertions(+), 4 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 6cc3b73ca904..82443236d4cc 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -215,7 +215,15 @@ struct kvm_s390_sie_block { > __u8 icptcode; /* 0x0050 */ > __u8 icptstatus; /* 0x0051 */ > __u16 ihcpu; /* 0x0052 */ > - __u8 reserved54[2]; /* 0x0054 */ > + __u8 reserved54; /* 0x0054 */ > +#define IICTL_CODE_NONE 0x00 > +#define IICTL_CODE_MCHK 0x01 > +#define IICTL_CODE_EXT 0x02 > +#define IICTL_CODE_IO 0x03 > +#define IICTL_CODE_RESTART 0x04 > +#define IICTL_CODE_SPECIFICATION 0x10 > +#define IICTL_CODE_OPERAND 0x11 > + __u8 iictl; /* 0x0055 */ > __u16 ipa; /* 0x0056 */ > __u32 ipb; /* 0x0058 */ > __u32 scaoh; /* 0x005c */ > @@ -252,7 +260,8 @@ struct kvm_s390_sie_block { > #define HPID_KVM 0x4 > #define HPID_VSIE 0x5 > __u8 hpid; /* 0x00b8 */ > - __u8 reservedb9[11]; /* 0x00b9 */ > + __u8 reservedb9[7]; /* 0x00b9 */ > + __u32 eiparams; /* 0x00c0 */ > __u16 extcpuaddr; /* 0x00c4 */ > __u16 eic; /* 0x00c6 */ > __u32 reservedc8; /* 0x00c8 */ > @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { > __u8 oai; /* 0x00e2 */ > __u8 armid; /* 0x00e3 */ > __u8 reservede4[4]; /* 0x00e4 */ > - __u64 tecmc; /* 0x00e8 */ > - __u8 reservedf0[12]; /* 0x00f0 */ > + union { > + __u64 tecmc; /* 0x00e8 */ I have to admit that I always have to think twice where the compiler might put the padding in this case. Maybe you could do that manually to make it obvious and wrap it in a struct, too: struct { __u64 tecmc; /* 0x00e8 */ __u8 reservedf0[4]; /* 0x00f0 */ }; ? Just my 0.02 €, though. Thomas > + struct { > + __u16 subchannel_id; /* 0x00e8 */ > + __u16 subchannel_nr; /* 0x00ea */ > + __u32 io_int_parm; /* 0x00ec */ > + __u32 io_int_word; /* 0x00f0 */ > + }; > + } __packed; > + __u8 reservedf4[8]; /* 0x00f4 */ > #define CRYCB_FORMAT_MASK 0x00000003 > #define CRYCB_FORMAT0 0x00000000 > #define CRYCB_FORMAT1 0x00000001 >
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 6cc3b73ca904..82443236d4cc 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -215,7 +215,15 @@ struct kvm_s390_sie_block { __u8 icptcode; /* 0x0050 */ __u8 icptstatus; /* 0x0051 */ __u16 ihcpu; /* 0x0052 */ - __u8 reserved54[2]; /* 0x0054 */ + __u8 reserved54; /* 0x0054 */ +#define IICTL_CODE_NONE 0x00 +#define IICTL_CODE_MCHK 0x01 +#define IICTL_CODE_EXT 0x02 +#define IICTL_CODE_IO 0x03 +#define IICTL_CODE_RESTART 0x04 +#define IICTL_CODE_SPECIFICATION 0x10 +#define IICTL_CODE_OPERAND 0x11 + __u8 iictl; /* 0x0055 */ __u16 ipa; /* 0x0056 */ __u32 ipb; /* 0x0058 */ __u32 scaoh; /* 0x005c */ @@ -252,7 +260,8 @@ struct kvm_s390_sie_block { #define HPID_KVM 0x4 #define HPID_VSIE 0x5 __u8 hpid; /* 0x00b8 */ - __u8 reservedb9[11]; /* 0x00b9 */ + __u8 reservedb9[7]; /* 0x00b9 */ + __u32 eiparams; /* 0x00c0 */ __u16 extcpuaddr; /* 0x00c4 */ __u16 eic; /* 0x00c6 */ __u32 reservedc8; /* 0x00c8 */ @@ -268,8 +277,16 @@ struct kvm_s390_sie_block { __u8 oai; /* 0x00e2 */ __u8 armid; /* 0x00e3 */ __u8 reservede4[4]; /* 0x00e4 */ - __u64 tecmc; /* 0x00e8 */ - __u8 reservedf0[12]; /* 0x00f0 */ + union { + __u64 tecmc; /* 0x00e8 */ + struct { + __u16 subchannel_id; /* 0x00e8 */ + __u16 subchannel_nr; /* 0x00ea */ + __u32 io_int_parm; /* 0x00ec */ + __u32 io_int_word; /* 0x00f0 */ + }; + } __packed; + __u8 reservedf4[8]; /* 0x00f4 */ #define CRYCB_FORMAT_MASK 0x00000003 #define CRYCB_FORMAT0 0x00000000 #define CRYCB_FORMAT1 0x00000001