| Message ID | 1503907651-65296-2-git-send-email-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, 28 Aug 2017 11:11:15 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > On 08/28/2017 11:07 AM, Cornelia Huck wrote: > > On Mon, 28 Aug 2017 10:07:28 +0200 > > Christian Borntraeger <borntraeger@de.ibm.com> wrote: > > > >> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> > >> > >> kvm has always supported the concept of starting in z/Arch mode so let's > >> reflect the feature bit to the guest. > >> > >> Also, we change sigp set architecture to reject any request to change > >> architecture modes. > > > > What are the implications of this for guests? IIRC, czam is a z14 > > feature (the updated PoP is not yet out, is it?). We now reject any set > > arch sigp, even if we profess to be an older machine. Linux can > > probably deal with this, but is it correct? > > Linux can deal with it. Since KVM does not support IPL in ESA mode, we basically > always provided the czam functionality. We now do it like it was architected. > z/VM does it the same way when enabling czam on an older system like z13. OK, this implies any z operating system needs to be able to deal with czam on older machines. > In other words the old code was not compliant (and never was), the new code > is compliant. Since czam is kind of a "negative" facility, we can implement > it also on older systems. Looking at this, we should mirror this in qemu in tcg mode, as the corresponding qemu patch only handles the kvm path (I obviously did not look at the patch in enough detail...) Added to my list.
On Mon, 28 Aug 2017 10:07:28 +0200 Christian Borntraeger <borntraeger@de.ibm.com> wrote: > From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> > > kvm has always supported the concept of starting in z/Arch mode so let's > reflect the feature bit to the guest. > > Also, we change sigp set architecture to reject any request to change > architecture modes. > > Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/sigp.c | 36 +++++++++++++++++------------------- > arch/s390/tools/gen_facilities.c | 1 + > 2 files changed, 18 insertions(+), 19 deletions(-) Reviewed-by: Cornelia Huck <cohuck@redhat.com>
On 28.08.2017 10:07, Christian Borntraeger wrote: > From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> > > kvm has always supported the concept of starting in z/Arch mode so let's > reflect the feature bit to the guest. > > Also, we change sigp set architecture to reject any request to change > architecture modes. Hm ... this seems to imply that CZAM is always set, but what about running on old user space (possibly on old hw)? Old QEMU will not enable CZAM. And especially old user space will rely on SET ARCHITECTURE being handled in the kernel. I agree, that for QEMU we can always do that change (as we control which features are indicated to the guest). But I am wondering if we would have to stick to existing behavior unless test_kvm_facility(vcpu->kvm, 138). > > Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> > Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > arch/s390/kvm/sigp.c | 36 +++++++++++++++++------------------- > arch/s390/tools/gen_facilities.c | 1 + > 2 files changed, 18 insertions(+), 19 deletions(-) > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c > index 1a252f5..11d3845 100644 > --- a/arch/s390/kvm/sigp.c > +++ b/arch/s390/kvm/sigp.c > @@ -155,29 +155,26 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, > return rc; > } > [...] > } > - return rc; > + > + *status_reg &= 0xffffffff00000000ULL; s/ULL/UL/ > + > + /* Reject set arch order, with czam we're always in z/Arch mode. */ > + *status_reg |= (all_stopped ? SIGP_STATUS_INVALID_PARAMETER : > + SIGP_STATUS_INCORRECT_STATE); > + return SIGP_CC_STATUS_STORED; > } > > static int __sigp_set_prefix(struct kvm_vcpu *vcpu, struct kvm_vcpu *dst_vcpu, > @@ -446,7 +443,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu) > switch (order_code) { > case SIGP_SET_ARCHITECTURE: > vcpu->stat.instruction_sigp_arch++; > - rc = __sigp_set_arch(vcpu, parameter); > + rc = __sigp_set_arch(vcpu, parameter, > + &vcpu->run->s.regs.gprs[r1]); > break; > default: > rc = handle_sigp_dst(vcpu, order_code, cpu_addr, > diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c > index 025ea20..181db5b 100644 > --- a/arch/s390/tools/gen_facilities.c > +++ b/arch/s390/tools/gen_facilities.c > @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = { > 78, /* enhanced-DAT 2 */ > 130, /* instruction-execution-protection */ > 131, /* enhanced-SOP 2 and side-effect */ > + 138, /* configuration z/architecture mode (czam) */ > 146, /* msa extension 8 */ > -1 /* END */ > } >
On 08/28/2017 04:06 PM, David Hildenbrand wrote: > On 28.08.2017 10:07, Christian Borntraeger wrote: >> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> >> >> kvm has always supported the concept of starting in z/Arch mode so let's >> reflect the feature bit to the guest. >> >> Also, we change sigp set architecture to reject any request to change >> architecture modes. > > Hm ... this seems to imply that CZAM is always set, but what about > running on old user space (possibly on old hw)? Old QEMU will not enable > CZAM. 3 cases. 1. very old QEMU without user sigp 2. old QEMU with user sigp/without CPU model 3. new QEMU with user sigp/cpu model I think we agree that cases 2 and 3 should not matter at all for this kernel patch as the sigp is handled by QEMU. This is case 1: > And especially old user space will rely on SET ARCHITECTURE being > handled in the kernel. Yes, and it continues to be handled in the kernel. It is just that the guest will now see a different sigp return code. Before, our sigp implementation lied to the guest in a way that worked for Linux (we lied by saying "yes, we switched"). We now say "sorry, we are already in zarch mode, sigp ignored" which also works perfectly fine for Linux. And IMHO it is even the better choice even without STFLE.138 being set as it matches what an old hardware would do when in zarch mode. > > I agree, that for QEMU we can always do that change (as we control which > features are indicated to the guest). But I am wondering if we would > have to stick to existing behavior unless test_kvm_facility(vcpu->kvm, 138). > >> >> Signed-off-by: Jason J. Herne <jjherne@linux.vnet.ibm.com> >> Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> arch/s390/kvm/sigp.c | 36 +++++++++++++++++------------------- >> arch/s390/tools/gen_facilities.c | 1 + >> 2 files changed, 18 insertions(+), 19 deletions(-) >> >> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c >> index 1a252f5..11d3845 100644 >> --- a/arch/s390/kvm/sigp.c >> +++ b/arch/s390/kvm/sigp.c >> @@ -155,29 +155,26 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, >> return rc; >> } >> > [...] > >> } >> - return rc; >> + >> + *status_reg &= 0xffffffff00000000ULL; > > s/ULL/UL/ Right, will fix.
On 28.08.2017 16:24, Christian Borntraeger wrote: > > > On 08/28/2017 04:06 PM, David Hildenbrand wrote: >> On 28.08.2017 10:07, Christian Borntraeger wrote: >>> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> >>> >>> kvm has always supported the concept of starting in z/Arch mode so let's >>> reflect the feature bit to the guest. >>> >>> Also, we change sigp set architecture to reject any request to change >>> architecture modes. >> >> Hm ... this seems to imply that CZAM is always set, but what about >> running on old user space (possibly on old hw)? Old QEMU will not enable >> CZAM. > > 3 cases. > 1. very old QEMU without user sigp > 2. old QEMU with user sigp/without CPU model > 3. new QEMU with user sigp/cpu model > > I think we agree that cases 2 and 3 should not matter at all for this kernel patch > as the sigp is handled by QEMU. > > > > This is case 1: >> And especially old user space will rely on SET ARCHITECTURE being >> handled in the kernel. > > > Yes, and it continues to be handled in the kernel. It is just that the guest > will now see a different sigp return code. Before, our sigp implementation lied > to the guest in a way that worked for Linux (we lied by saying "yes, we switched"). > We now say "sorry, we are already in zarch mode, sigp ignored" which also works > perfectly fine for Linux. And IMHO it is even the better choice even without > STFLE.138 being set as it matches what an old hardware would do when in zarch mode. Ok, if it worked for relevant Linux versions, than it should indeed be fine. Thanks! >> [...] >> >>> } >>> - return rc; >>> + >>> + *status_reg &= 0xffffffff00000000ULL; >> >> s/ULL/UL/ > > > Right, will fix. >
On 08/28/2017 04:38 PM, David Hildenbrand wrote: > On 28.08.2017 16:24, Christian Borntraeger wrote: >> >> >> On 08/28/2017 04:06 PM, David Hildenbrand wrote: >>> On 28.08.2017 10:07, Christian Borntraeger wrote: >>>> From: "Jason J. Herne" <jjherne@linux.vnet.ibm.com> >>>> >>>> kvm has always supported the concept of starting in z/Arch mode so let's >>>> reflect the feature bit to the guest. >>>> >>>> Also, we change sigp set architecture to reject any request to change >>>> architecture modes. >>> >>> Hm ... this seems to imply that CZAM is always set, but what about >>> running on old user space (possibly on old hw)? Old QEMU will not enable >>> CZAM. >> >> 3 cases. >> 1. very old QEMU without user sigp >> 2. old QEMU with user sigp/without CPU model >> 3. new QEMU with user sigp/cpu model >> >> I think we agree that cases 2 and 3 should not matter at all for this kernel patch >> as the sigp is handled by QEMU. >> >> >> >> This is case 1: >>> And especially old user space will rely on SET ARCHITECTURE being >>> handled in the kernel. >> >> >> Yes, and it continues to be handled in the kernel. It is just that the guest >> will now see a different sigp return code. Before, our sigp implementation lied >> to the guest in a way that worked for Linux (we lied by saying "yes, we switched"). >> We now say "sorry, we are already in zarch mode, sigp ignored" which also works >> perfectly fine for Linux. And IMHO it is even the better choice even without >> STFLE.138 being set as it matches what an old hardware would do when in zarch mode. > > Ok, if it worked for relevant Linux versions, than it should indeed be fine. I checked back to 2006 and Linux never checked the return value for this sigp.
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c index 1a252f5..11d3845 100644 --- a/arch/s390/kvm/sigp.c +++ b/arch/s390/kvm/sigp.c @@ -155,29 +155,26 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, return rc; } -static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter) +static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, + u64 *status_reg) { - int rc; unsigned int i; struct kvm_vcpu *v; + bool all_stopped = true; - switch (parameter & 0xff) { - case 0: - rc = SIGP_CC_NOT_OPERATIONAL; - break; - case 1: - case 2: - kvm_for_each_vcpu(i, v, vcpu->kvm) { - v->arch.pfault_token = KVM_S390_PFAULT_TOKEN_INVALID; - kvm_clear_async_pf_completion_queue(v); - } - - rc = SIGP_CC_ORDER_CODE_ACCEPTED; - break; - default: - rc = -EOPNOTSUPP; + kvm_for_each_vcpu(i, v, vcpu->kvm) { + if (v == vcpu) + continue; + if (!is_vcpu_stopped(v)) + all_stopped = false; } - return rc; + + *status_reg &= 0xffffffff00000000ULL; + + /* Reject set arch order, with czam we're always in z/Arch mode. */ + *status_reg |= (all_stopped ? SIGP_STATUS_INVALID_PARAMETER : + SIGP_STATUS_INCORRECT_STATE); + return SIGP_CC_STATUS_STORED; } static int __sigp_set_prefix(struct kvm_vcpu *vcpu, struct kvm_vcpu *dst_vcpu, @@ -446,7 +443,8 @@ int kvm_s390_handle_sigp(struct kvm_vcpu *vcpu) switch (order_code) { case SIGP_SET_ARCHITECTURE: vcpu->stat.instruction_sigp_arch++; - rc = __sigp_set_arch(vcpu, parameter); + rc = __sigp_set_arch(vcpu, parameter, + &vcpu->run->s.regs.gprs[r1]); break; default: rc = handle_sigp_dst(vcpu, order_code, cpu_addr, diff --git a/arch/s390/tools/gen_facilities.c b/arch/s390/tools/gen_facilities.c index 025ea20..181db5b 100644 --- a/arch/s390/tools/gen_facilities.c +++ b/arch/s390/tools/gen_facilities.c @@ -80,6 +80,7 @@ static struct facility_def facility_defs[] = { 78, /* enhanced-DAT 2 */ 130, /* instruction-execution-protection */ 131, /* enhanced-SOP 2 and side-effect */ + 138, /* configuration z/architecture mode (czam) */ 146, /* msa extension 8 */ -1 /* END */ }