diff mbox

[02/12] KVM: s390: define GISA format-0 data structure

Message ID 20180116200217.211897-3-borntraeger@de.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Christian Borntraeger Jan. 16, 2018, 8:02 p.m. UTC 
From: Michael Mueller <mimu@linux.vnet.ibm.com>

In preperation to support pass-through adapter interrupts, the Guest
Interruption State Area (GISA) and the Adapter Interruption Virtualization
(AIV) features will be introduced here.

This patch introduces format-0 GISA (that is defines the struct describing
the GISA, allocates storage for it, and introduces fields for the
GISA address in kvm_s390_sie_block and kvm_s390_vsie).

As the GISA requires storage below 2GB, it is put in sie_page2, which is
already allocated in ZONE_DMA. In addition, The GISA requires alignment to
its integral boundary.

Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com>
Reviewed-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
Reviewed-by: Halil Pasic <pasic@linux.vnet.ibm.com>
Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com>
Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
---
 arch/s390/include/asm/kvm_host.h | 24 ++++++++++++++++++++----
 arch/s390/kvm/kvm-s390.c         |  1 +
 2 files changed, 21 insertions(+), 4 deletions(-)

Comments

David Hildenbrand Jan. 16, 2018, 8:25 p.m. UTC | #1 
> +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? :)
Heiko Carstens Jan. 17, 2018, 7:57 a.m. UTC | #2 
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... ;)
Michael Mueller Jan. 18, 2018, 3:49 p.m. UTC | #3 
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
David Hildenbrand Jan. 18, 2018, 8:47 p.m. UTC | #4 
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;
> +};
> +
>  /*
Heiko Carstens Jan. 19, 2018, 10:12 a.m. UTC | #5 
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...
David Hildenbrand Jan. 19, 2018, 10:17 a.m. UTC | #6 
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 ...
Heiko Carstens Jan. 19, 2018, 10:20 a.m. UTC | #7 
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.
Cornelia Huck Jan. 19, 2018, 10:29 a.m. UTC | #8 
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...
David Hildenbrand Jan. 19, 2018, 11:28 a.m. UTC | #9 
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 mbox

Patch

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)