| Message ID | 20180125132848.175942-13-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 25.01.2018 14:28, Christian Borntraeger wrote: > From: Michael Mueller <mimu@linux.vnet.ibm.com> > > The patch modifies the previously defined GISA data structure to be > able to store two GISA formats, format-0 and format-1. Additionally, > it verifies the availability of the GISA format facility and enables > the use of a format-1 GISA in the SIE control block accordingly. > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > Reviewed-by: Pierre Morel <pmorel@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 | 44 +++++++++++++++++++++++++++++++--------- > arch/s390/kvm/kvm-s390.c | 2 ++ > 2 files changed, 36 insertions(+), 10 deletions(-) > > diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h > index 6802d5d..287c99b 100644 > --- a/arch/s390/include/asm/kvm_host.h > +++ b/arch/s390/include/asm/kvm_host.h > @@ -228,6 +228,7 @@ struct kvm_s390_sie_block { > __u8 epdx; /* 0x0069 */ > __u8 reserved6a[2]; /* 0x006a */ > __u32 todpr; /* 0x006c */ > +#define GISA_FORMAT1 0x00000001 > __u32 gd; /* 0x0070 */ > __u8 reserved74[12]; /* 0x0074 */ > __u64 mso; /* 0x0080 */ > @@ -706,15 +707,38 @@ struct kvm_s390_crypto_cb { > }; > > 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; > + union { > + struct { /* common to all formats */ > + u32 next_alert; > + u8 ipm; > + u8 reserved01[2]; > + u8 iam; > + }; > + struct { /* format 0 */ > + u32 next_alert; > + u8 ipm; > + u8 reserved01; > + u8 : 6; > + u8 g : 1; > + u8 c : 1; > + u8 iam; > + u8 reserved02[4]; > + u32 airq_count; > + } g0; > + struct { /* format 1 */ > + u32 next_alert; > + u8 ipm; > + u8 simm; > + u8 nimm; > + u8 iam; > + u8 aism[8]; > + u8 : 6; > + u8 g : 1; > + u8 c : 1; > + u8 reserved03[11]; > + u32 airq_count; > + } g1; > + }; > }; > > /* > @@ -725,7 +749,7 @@ struct sie_page2 { > __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ > struct kvm_s390_crypto_cb crycb; /* 0x0800 */ > struct kvm_s390_gisa gisa; /* 0x0900 */ > - u8 reserved910[0x1000 - 0x910]; /* 0x0910 */ > + u8 reserved920[0x1000 - 0x920]; /* 0x0920 */ > }; > > struct kvm_s390_vsie { > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 68d7eef..efde264 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > vcpu->arch.sie_block->icpua = id; > spin_lock_init(&vcpu->arch.local_int.lock); > vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; > + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) > + vcpu->arch.sie_block->gd |= GISA_FORMAT1; > seqcount_init(&vcpu->arch.cputm_seqcount); > > rc = kvm_vcpu_init(vcpu, kvm, id); > So, what does this bring us? We don't seem to be using any new GISA-1 features.
On 01/25/2018 04:31 PM, David Hildenbrand wrote: [...] >> struct kvm_s390_vsie { >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >> index 68d7eef..efde264 100644 >> --- a/arch/s390/kvm/kvm-s390.c >> +++ b/arch/s390/kvm/kvm-s390.c >> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, >> vcpu->arch.sie_block->icpua = id; >> spin_lock_init(&vcpu->arch.local_int.lock); >> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; >> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) >> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; >> seqcount_init(&vcpu->arch.cputm_seqcount); >> >> rc = kvm_vcpu_init(vcpu, kvm, id); >> > > So, what does this bring us? We don't seem to be using any new GISA-1 > features. Preparation for device pass-through interrupt forwarding.
On Thu, 25 Jan 2018 16:43:27 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 01/25/2018 04:31 PM, David Hildenbrand wrote: > [...] > >> struct kvm_s390_vsie { > >> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >> index 68d7eef..efde264 100644 > >> --- a/arch/s390/kvm/kvm-s390.c > >> +++ b/arch/s390/kvm/kvm-s390.c > >> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > >> vcpu->arch.sie_block->icpua = id; > >> spin_lock_init(&vcpu->arch.local_int.lock); > >> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; > >> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) > >> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; > >> seqcount_init(&vcpu->arch.cputm_seqcount); > >> > >> rc = kvm_vcpu_init(vcpu, kvm, id); > >> > > > > So, what does this bring us? We don't seem to be using any new GISA-1 > > features. > > Preparation for device pass-through interrupt forwarding. > Should we start out with a dual format-0/format-1 gisa block, then? IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can do anything that gisa-0 can do?
On 01/25/2018 04:47 PM, Cornelia Huck wrote: > On Thu, 25 Jan 2018 16:43:27 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 01/25/2018 04:31 PM, David Hildenbrand wrote: >> [...] >>>> struct kvm_s390_vsie { >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index 68d7eef..efde264 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, >>>> vcpu->arch.sie_block->icpua = id; >>>> spin_lock_init(&vcpu->arch.local_int.lock); >>>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; >>>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) >>>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; >>>> seqcount_init(&vcpu->arch.cputm_seqcount); >>>> >>>> rc = kvm_vcpu_init(vcpu, kvm, id); >>>> >>> >>> So, what does this bring us? We don't seem to be using any new GISA-1 >>> features. >> >> Preparation for device pass-through interrupt forwarding. >> > > Should we start out with a dual format-0/format-1 gisa block, then? > IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can > do anything that gisa-0 can do? There might be systems that only have gisa-0, so I think having both makes sense.
On 25.01.2018 16:43, Christian Borntraeger wrote: > > > On 01/25/2018 04:31 PM, David Hildenbrand wrote: > [...] >>> struct kvm_s390_vsie { >>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>> index 68d7eef..efde264 100644 >>> --- a/arch/s390/kvm/kvm-s390.c >>> +++ b/arch/s390/kvm/kvm-s390.c >>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, >>> vcpu->arch.sie_block->icpua = id; >>> spin_lock_init(&vcpu->arch.local_int.lock); >>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; >>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) >>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; wonder if the would be nicer via if (kvm->arch.gisa) { vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; if (sclp.has_gisaf) cpu->arch.sie_block->gd |= GISA_FORMAT1; } >>> seqcount_init(&vcpu->arch.cputm_seqcount); >>> >>> rc = kvm_vcpu_init(vcpu, kvm, id); >>> >> >> So, what does this bring us? We don't seem to be using any new GISA-1 >> features. > > Preparation for device pass-through interrupt forwarding. > Can you add something like that to the patch description? Reviewed-by: David Hildenbrand <david@redhat.com>
On Thu, 25 Jan 2018 17:12:59 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 01/25/2018 04:47 PM, Cornelia Huck wrote: > > On Thu, 25 Jan 2018 16:43:27 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 01/25/2018 04:31 PM, David Hildenbrand wrote: > >> [...] > >>>> struct kvm_s390_vsie { > >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >>>> index 68d7eef..efde264 100644 > >>>> --- a/arch/s390/kvm/kvm-s390.c > >>>> +++ b/arch/s390/kvm/kvm-s390.c > >>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > >>>> vcpu->arch.sie_block->icpua = id; > >>>> spin_lock_init(&vcpu->arch.local_int.lock); > >>>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; > >>>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) > >>>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; > >>>> seqcount_init(&vcpu->arch.cputm_seqcount); > >>>> > >>>> rc = kvm_vcpu_init(vcpu, kvm, id); > >>>> > >>> > >>> So, what does this bring us? We don't seem to be using any new GISA-1 > >>> features. > >> > >> Preparation for device pass-through interrupt forwarding. > >> > > > > Should we start out with a dual format-0/format-1 gisa block, then? > > IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can > > do anything that gisa-0 can do? > > There might be systems that only have gisa-0, so I think having both makes > sense. > Yes, that's what I meant. Just do it earlier in the series - this patch feels like an afterthought with no real user.
On 01/25/2018 05:16 PM, David Hildenbrand wrote: > On 25.01.2018 16:43, Christian Borntraeger wrote: >> >> >> On 01/25/2018 04:31 PM, David Hildenbrand wrote: >> [...] >>>> struct kvm_s390_vsie { >>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>> index 68d7eef..efde264 100644 >>>> --- a/arch/s390/kvm/kvm-s390.c >>>> +++ b/arch/s390/kvm/kvm-s390.c >>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, >>>> vcpu->arch.sie_block->icpua = id; >>>> spin_lock_init(&vcpu->arch.local_int.lock); >>>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; >>>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) >>>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; > > wonder if the would be nicer via > > if (kvm->arch.gisa) { > vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; > if (sclp.has_gisaf) > cpu->arch.sie_block->gd |= GISA_FORMAT1; > } Dont know. I leave it as is. > >>>> seqcount_init(&vcpu->arch.cputm_seqcount); >>>> >>>> rc = kvm_vcpu_init(vcpu, kvm, id); >>>> >>> >>> So, what does this bring us? We don't seem to be using any new GISA-1 >>> features. >> >> Preparation for device pass-through interrupt forwarding. >> > > Can you add something like that to the patch description? done. > > Reviewed-by: David Hildenbrand <david@redhat.com> >
On 01/25/2018 05:17 PM, Cornelia Huck wrote: > On Thu, 25 Jan 2018 17:12:59 +0100 > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >> On 01/25/2018 04:47 PM, Cornelia Huck wrote: >>> On Thu, 25 Jan 2018 16:43:27 +0100 >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: >>> >>>> On 01/25/2018 04:31 PM, David Hildenbrand wrote: >>>> [...] >>>>>> struct kvm_s390_vsie { >>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c >>>>>> index 68d7eef..efde264 100644 >>>>>> --- a/arch/s390/kvm/kvm-s390.c >>>>>> +++ b/arch/s390/kvm/kvm-s390.c >>>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, >>>>>> vcpu->arch.sie_block->icpua = id; >>>>>> spin_lock_init(&vcpu->arch.local_int.lock); >>>>>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; >>>>>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) >>>>>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; >>>>>> seqcount_init(&vcpu->arch.cputm_seqcount); >>>>>> >>>>>> rc = kvm_vcpu_init(vcpu, kvm, id); >>>>>> >>>>> >>>>> So, what does this bring us? We don't seem to be using any new GISA-1 >>>>> features. >>>> >>>> Preparation for device pass-through interrupt forwarding. >>>> >>> >>> Should we start out with a dual format-0/format-1 gisa block, then? >>> IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can >>> do anything that gisa-0 can do? >> >> There might be systems that only have gisa-0, so I think having both makes >> sense. >> > > Yes, that's what I meant. Just do it earlier in the series - this patch > feels like an afterthought with no real user. I added A format-1 can do everything that format-0 can and we will need it for real HW passthrough. As there are systems with only format-0 we keep both variants. to the patch description. Maybe its now a bit less odd?
On Thu, 25 Jan 2018 17:51:24 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 01/25/2018 05:17 PM, Cornelia Huck wrote: > > On Thu, 25 Jan 2018 17:12:59 +0100 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> On 01/25/2018 04:47 PM, Cornelia Huck wrote: > >>> On Thu, 25 Jan 2018 16:43:27 +0100 > >>> Christian Borntraeger <borntraeger@de.ibm.com> wrote: > >>> > >>>> On 01/25/2018 04:31 PM, David Hildenbrand wrote: > >>>> [...] > >>>>>> struct kvm_s390_vsie { > >>>>>> diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > >>>>>> index 68d7eef..efde264 100644 > >>>>>> --- a/arch/s390/kvm/kvm-s390.c > >>>>>> +++ b/arch/s390/kvm/kvm-s390.c > >>>>>> @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, > >>>>>> vcpu->arch.sie_block->icpua = id; > >>>>>> spin_lock_init(&vcpu->arch.local_int.lock); > >>>>>> vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; > >>>>>> + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) > >>>>>> + vcpu->arch.sie_block->gd |= GISA_FORMAT1; > >>>>>> seqcount_init(&vcpu->arch.cputm_seqcount); > >>>>>> > >>>>>> rc = kvm_vcpu_init(vcpu, kvm, id); > >>>>>> > >>>>> > >>>>> So, what does this bring us? We don't seem to be using any new GISA-1 > >>>>> features. > >>>> > >>>> Preparation for device pass-through interrupt forwarding. > >>>> > >>> > >>> Should we start out with a dual format-0/format-1 gisa block, then? > >>> IIUC, you'll switch to gisa-1 if the facility is there and gisa-1 can > >>> do anything that gisa-0 can do? > >> > >> There might be systems that only have gisa-0, so I think having both makes > >> sense. > >> > > > > Yes, that's what I meant. Just do it earlier in the series - this patch > > feels like an afterthought with no real user. > > I added > > A format-1 can do everything that format-0 can and we will need it > for real HW passthrough. As there are systems with only format-0 > we keep both variants. > > to the patch description. Maybe its now a bit less odd? Also fine with me. (I'll do a proper review round through the patches later.)
On Thu, 25 Jan 2018 14:28:48 +0100 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: Michael Mueller <mimu@linux.vnet.ibm.com> > > The patch modifies the previously defined GISA data structure to be > able to store two GISA formats, format-0 and format-1. Additionally, > it verifies the availability of the GISA format facility and enables > the use of a format-1 GISA in the SIE control block accordingly. > > Signed-off-by: Michael Mueller <mimu@linux.vnet.ibm.com> > Reviewed-by: Pierre Morel <pmorel@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 | 44 +++++++++++++++++++++++++++++++--------- > arch/s390/kvm/kvm-s390.c | 2 ++ > 2 files changed, 36 insertions(+), 10 deletions(-) With the description changed as discussed previously, Acked-by: Cornelia Huck <cohuck@redhat.com>
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h index 6802d5d..287c99b 100644 --- a/arch/s390/include/asm/kvm_host.h +++ b/arch/s390/include/asm/kvm_host.h @@ -228,6 +228,7 @@ struct kvm_s390_sie_block { __u8 epdx; /* 0x0069 */ __u8 reserved6a[2]; /* 0x006a */ __u32 todpr; /* 0x006c */ +#define GISA_FORMAT1 0x00000001 __u32 gd; /* 0x0070 */ __u8 reserved74[12]; /* 0x0074 */ __u64 mso; /* 0x0080 */ @@ -706,15 +707,38 @@ struct kvm_s390_crypto_cb { }; 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; + union { + struct { /* common to all formats */ + u32 next_alert; + u8 ipm; + u8 reserved01[2]; + u8 iam; + }; + struct { /* format 0 */ + u32 next_alert; + u8 ipm; + u8 reserved01; + u8 : 6; + u8 g : 1; + u8 c : 1; + u8 iam; + u8 reserved02[4]; + u32 airq_count; + } g0; + struct { /* format 1 */ + u32 next_alert; + u8 ipm; + u8 simm; + u8 nimm; + u8 iam; + u8 aism[8]; + u8 : 6; + u8 g : 1; + u8 c : 1; + u8 reserved03[11]; + u32 airq_count; + } g1; + }; }; /* @@ -725,7 +749,7 @@ struct sie_page2 { __u64 fac_list[S390_ARCH_FAC_LIST_SIZE_U64]; /* 0x0000 */ struct kvm_s390_crypto_cb crycb; /* 0x0800 */ struct kvm_s390_gisa gisa; /* 0x0900 */ - u8 reserved910[0x1000 - 0x910]; /* 0x0910 */ + u8 reserved920[0x1000 - 0x920]; /* 0x0920 */ }; struct kvm_s390_vsie { diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c index 68d7eef..efde264 100644 --- a/arch/s390/kvm/kvm-s390.c +++ b/arch/s390/kvm/kvm-s390.c @@ -2518,6 +2518,8 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, vcpu->arch.sie_block->icpua = id; spin_lock_init(&vcpu->arch.local_int.lock); vcpu->arch.sie_block->gd = (u32)(u64)kvm->arch.gisa; + if (vcpu->arch.sie_block->gd && sclp.has_gisaf) + vcpu->arch.sie_block->gd |= GISA_FORMAT1; seqcount_init(&vcpu->arch.cputm_seqcount); rc = kvm_vcpu_init(vcpu, kvm, id);