Message ID | 20180116200217.211897-3-borntraeger@de.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
> +struct kvm_s390_gisa { > + u32 next_alert; > + u8 ipm; > + u8 reserved01; > + u8:6; > + u8 g:1; > + u8 c:1; > + u8 iam; > + u8 reserved02[4]; > + u32 airq_count; > +}; > + > /* > - * sie_page2 has to be allocated as DMA because fac_list and crycb need > - * 31bit addresses in the sie control block. > + * sie_page2 has to be allocated as DMA because fac_list, crycb and > + * gisa need 31bit addresses in the sie control block. > */ > struct sie_page2 { > __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ > struct kvm_s390_crypto_cb crycb; /* 0x0800 */ > - u8 reserved900[0x1000 - 0x900]; /* 0x0900 */ > + struct kvm_s390_gisa gisa __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */ can we instead specify sie_page2 as packed if really needed? (I can see that we have a BUILD_BUG_ON below, which is nice) (alignment within well defined data structures looks strange) > + u8 reserved910[0x1000 - 0x910]; /* 0x0910 */ > }; > > struct kvm_s390_vsie { > @@ -757,6 +772,7 @@ struct kvm_arch{ > struct kvm_s390_migration_state *migration_state; > /* subset of available cpu features enabled by user space */ > DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS); > + struct kvm_s390_gisa *gisa; What's this little fellow doing in this patch? :)
On Tue, Jan 16, 2018 at 09:25:12PM +0100, David Hildenbrand wrote: > > > +struct kvm_s390_gisa { > > + u32 next_alert; > > + u8 ipm; > > + u8 reserved01; > > + u8:6; > > + u8 g:1; > > + u8 c:1; > > + u8 iam; > > + u8 reserved02[4]; > > + u32 airq_count; > > +}; > > + > > /* > > - * sie_page2 has to be allocated as DMA because fac_list and crycb need > > - * 31bit addresses in the sie control block. > > + * sie_page2 has to be allocated as DMA because fac_list, crycb and > > + * gisa need 31bit addresses in the sie control block. > > */ > > struct sie_page2 { > > __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ > > struct kvm_s390_crypto_cb crycb; /* 0x0800 */ > > - u8 reserved900[0x1000 - 0x900]; /* 0x0900 */ > > + struct kvm_s390_gisa gisa __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */ > > can we instead specify sie_page2 as packed if really needed? (I can see > that we have a BUILD_BUG_ON below, which is nice) > > (alignment within well defined data structures looks strange) The alignment, if needed, should go to the definition of struct kvm_s390_gisa above. The structure needs at least an alignment of eight bytes since bitops are used to modify ipm (in a later patch), by using a (long *) cast to this structure. But of course it is all naturally aligned anyway... ;)
On 17.01.18 08:57, Heiko Carstens wrote: > On Tue, Jan 16, 2018 at 09:25:12PM +0100, David Hildenbrand wrote: >> >>> +struct kvm_s390_gisa { >>> + u32 next_alert; >>> + u8 ipm; >>> + u8 reserved01; >>> + u8:6; >>> + u8 g:1; >>> + u8 c:1; >>> + u8 iam; >>> + u8 reserved02[4]; >>> + u32 airq_count; >>> +}; >>> + >>> /* >>> - * sie_page2 has to be allocated as DMA because fac_list and crycb need >>> - * 31bit addresses in the sie control block. >>> + * sie_page2 has to be allocated as DMA because fac_list, crycb and >>> + * gisa need 31bit addresses in the sie control block. >>> */ >>> struct sie_page2 { >>> __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ >>> struct kvm_s390_crypto_cb crycb; /* 0x0800 */ >>> - u8 reserved900[0x1000 - 0x900]; /* 0x0900 */ >>> + struct kvm_s390_gisa gisa __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */ >> >> can we instead specify sie_page2 as packed if really needed? (I can see >> that we have a BUILD_BUG_ON below, which is nice) >> >> (alignment within well defined data structures looks strange) I'm doing that because the gisa requires "integral boundary" alignment that changes with a later patch when I introduce format-1. > > The alignment, if needed, should go to the definition of struct > kvm_s390_gisa above. The structure needs at least an alignment of eight > bytes since bitops are used to modify ipm (in a later patch), by using a > (long *) cast to this structure. adding this to the definition does not allow to use sizeof() for the currently defined struct. Thus I would have to explicitly specify the size... :( > But of course it is all naturally aligned anyway... ;) > Thanks Michael
Two minor things > > +struct kvm_s390_gisa { > + u32 next_alert; > + u8 ipm; > + u8 reserved01; > + u8:6; Mind giving this also a reserved name > + u8 g:1; > + u8 c:1; Can you put spaces before/after the colon? > + u8 iam; > + u8 reserved02[4]; > + u32 airq_count; > +}; > + > /*
On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote: > Two minor things > > > > > +struct kvm_s390_gisa { > > + u32 next_alert; > > + u8 ipm; > > + u8 reserved01; > > + u8:6; > > Mind giving this also a reserved name And then all reserved fields have to be renamed as soon as one bit gets used? Please don't...
On 19.01.2018 11:12, Heiko Carstens wrote: > On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote: >> Two minor things >> >>> >>> +struct kvm_s390_gisa { >>> + u32 next_alert; >>> + u8 ipm; >>> + u8 reserved01; >>> + u8:6; >> >> Mind giving this also a reserved name > > And then all reserved fields have to be renamed as soon as one bit gets > used? Please don't... > Only if one keeps the order of the reserved field numbers ...
On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote: > On 19.01.2018 11:12, Heiko Carstens wrote: > > On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote: > >> Two minor things > >> > >>> > >>> +struct kvm_s390_gisa { > >>> + u32 next_alert; > >>> + u8 ipm; > >>> + u8 reserved01; > >>> + u8:6; > >> > >> Mind giving this also a reserved name > > > > And then all reserved fields have to be renamed as soon as one bit gets > > used? Please don't... > > Only if one keeps the order of the reserved field numbers ... And that's what people usually do. Therefore having unnamed bitfields is nice, since you don't have to care.
On Fri, 19 Jan 2018 11:20:39 +0100 Heiko Carstens <heiko.carstens@de.ibm.com> wrote: > On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote: > > On 19.01.2018 11:12, Heiko Carstens wrote: > > > On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote: > > >> Two minor things > > >> > > >>> > > >>> +struct kvm_s390_gisa { > > >>> + u32 next_alert; > > >>> + u8 ipm; > > >>> + u8 reserved01; > > >>> + u8:6; > > >> > > >> Mind giving this also a reserved name > > > > > > And then all reserved fields have to be renamed as soon as one bit gets > > > used? Please don't... > > > > Only if one keeps the order of the reserved field numbers ... > > And that's what people usually do. Therefore having unnamed bitfields is > nice, since you don't have to care. > +1 They make these kinds of definitions so much nicer...
On 19.01.2018 11:29, Cornelia Huck wrote: > On Fri, 19 Jan 2018 11:20:39 +0100 > Heiko Carstens <heiko.carstens@de.ibm.com> wrote: > >> On Fri, Jan 19, 2018 at 11:17:10AM +0100, David Hildenbrand wrote: >>> On 19.01.2018 11:12, Heiko Carstens wrote: >>>> On Thu, Jan 18, 2018 at 09:47:01PM +0100, David Hildenbrand wrote: >>>>> Two minor things >>>>> >>>>>> >>>>>> +struct kvm_s390_gisa { >>>>>> + u32 next_alert; >>>>>> + u8 ipm; >>>>>> + u8 reserved01; >>>>>> + u8:6; >>>>> >>>>> Mind giving this also a reserved name >>>> >>>> And then all reserved fields have to be renamed as soon as one bit gets >>>> used? Please don't... >>> >>> Only if one keeps the order of the reserved field numbers ... >> >> And that's what people usually do. Therefore having unnamed bitfields is >> nice, since you don't have to care. >> > > +1 > > They make these kinds of definitions so much nicer... > I'm convinced :)
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 9981721f258f..11f15a51bfc7 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -183,6 +183,7 @@ struct kvm_s390_sie_block { #define ECA_IB 0x40000000 #define ECA_SIGPI 0x10000000 #define ECA_MVPGI 0x01000000 +#define ECA_AIV 0x00200000 #define ECA_VX 0x00020000 #define ECA_PROTEXCI 0x00002000 #define ECA_SII 0x00000001 @@ -227,7 +228,8 @@ struct kvm_s390_sie_block { __u8 epdx; /* 0x0069 */ __u8 reserved6a[2]; /* 0x006a */ __u32 todpr; /* 0x006c */ - __u8 reserved70[16]; /* 0x0070 */ + __u32 gd; /* 0x0070 */ + __u8 reserved74[12]; /* 0x0074 */ __u64 mso; /* 0x0080 */ __u64 msl; /* 0x0088 */ psw_t gpsw; /* 0x0090 */ @@ -703,14 +705,27 @@ struct kvm_s390_crypto_cb { struct kvm_s390_apcb1 apcb1; /* 0x0080 */ }; +struct kvm_s390_gisa { + u32 next_alert; + u8 ipm; + u8 reserved01; + u8:6; + u8 g:1; + u8 c:1; + u8 iam; + u8 reserved02[4]; + u32 airq_count; +}; + /* - * sie_page2 has to be allocated as DMA because fac_list and crycb need - * 31bit addresses in the sie control block. + * sie_page2 has to be allocated as DMA because fac_list, crycb and + * gisa need 31bit addresses in the sie control block. */ struct sie_page2 { __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ struct kvm_s390_crypto_cb crycb; /* 0x0800 */ - u8 reserved900[0x1000 - 0x900]; /* 0x0900 */ + struct kvm_s390_gisa gisa __aligned(sizeof(struct kvm_s390_gisa)); /* 0x0900 */ + u8 reserved910[0x1000 - 0x910]; /* 0x0910 */ }; struct kvm_s390_vsie { @@ -757,6 +772,7 @@ struct kvm_arch{ struct kvm_s390_migration_state *migration_state; /* subset of available cpu features enabled by user space */ DECLARE_BITMAP(cpu_feat, KVM_S390_VM_CPU_FEAT_NR_BITS); + struct kvm_s390_gisa *gisa; }; #define KVM_HVA_ERR_BAD (-1UL) diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index de16c224319c..48f0099188df 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -1915,6 +1915,7 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type) if (!kvm->arch.dbf) goto out_err; + BUILD_BUG_ON(sizeof(struct sie_page2) != 4096); kvm->arch.sie_page2 = (struct sie_page2 *) get_zeroed_page(GFP_KERNEL | GFP_DMA); if (!kvm->arch.sie_page2)