| Message ID | 20211008203112.1979843-2-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improvements to SIGP handling [KVM] | expand |
On 08/10/2021 22.31, Eric Farman wrote: > The Principles of Operations describe the various reasons that > each individual SIGP orders might be rejected, and the status > bit that are set for each condition. > > For example, for the Set Architecture order, it states: > > "If it is not true that all other CPUs in the configu- > ration are in the stopped or check-stop state, ... > bit 54 (incorrect state) ... is set to one." > > However, it also states: > > "... if the CZAM facility is installed, ... > bit 55 (invalid parameter) ... is set to one." > > Since the Configuration-z/Architecture-Architectural Mode (CZAM) > facility is unconditionally presented, there is no need to examine > each VCPU to determine if it is started/stopped. It can simply be > rejected outright with the Invalid Parameter bit. > > Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > arch/s390/kvm/sigp.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c > index 683036c1c92a..cf4de80bd541 100644 > --- a/arch/s390/kvm/sigp.c > +++ b/arch/s390/kvm/sigp.c > @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, > static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, > u64 *status_reg) > { > - unsigned int i; > - struct kvm_vcpu *v; > - bool all_stopped = true; > - > - kvm_for_each_vcpu(i, v, vcpu->kvm) { > - if (v == vcpu) > - continue; > - if (!is_vcpu_stopped(v)) > - all_stopped = false; > - } > - > *status_reg &= 0xffffffff00000000UL; > > /* 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); > + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; > return SIGP_CC_STATUS_STORED; > } I was initially a little bit torn by this modification, since, as you already mentioned, it could theoretically be possible that a userspace (like an older version of QEMU) does not use CZAM bit yet. But then I read an older version of the PoP which does not feature CZAM yet, and it reads: "The set-architecture order is completed as follows: • If the code in the parameter register is not 0, 1, or 2, or if the CPU is already in the architectural mode specified by the code, the order is not accepted. Instead, bit 55 (invalid parameter) of the general register designated by the R 1 field of the SIGNAL PROCESSOR instruction is set to one, and condition code 1 is set. • If it is not true that all other CPUs in the configu- ration are in the stopped or check-stop state, the order is not accepted. Instead, bit 54 (incorrect state) of the general register designated by the R 1 field of the SIGNAL PROCESSOR instruction is set to one, and condition code 1 is set. • The architectural mode of all CPUs in the config- uration is set as specified by the code. ..." So to me this sounds like "invalid parameter" has a higher priority than "incorrect state" anyway, so we likely never should have reported here "incorrect state"...? Thus, I think it's the right way to go now: Reviewed-by: Thomas Huth <thuth@redhat.com>
Am 11.10.21 um 08:29 schrieb Thomas Huth: > On 08/10/2021 22.31, Eric Farman wrote: >> The Principles of Operations describe the various reasons that >> each individual SIGP orders might be rejected, and the status >> bit that are set for each condition. >> >> For example, for the Set Architecture order, it states: >> >> "If it is not true that all other CPUs in the configu- >> ration are in the stopped or check-stop state, ... >> bit 54 (incorrect state) ... is set to one." >> >> However, it also states: >> >> "... if the CZAM facility is installed, ... >> bit 55 (invalid parameter) ... is set to one." >> >> Since the Configuration-z/Architecture-Architectural Mode (CZAM) >> facility is unconditionally presented, there is no need to examine >> each VCPU to determine if it is started/stopped. It can simply be >> rejected outright with the Invalid Parameter bit. >> >> Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") >> Signed-off-by: Eric Farman <farman@linux.ibm.com> >> --- >> arch/s390/kvm/sigp.c | 14 +------------- >> 1 file changed, 1 insertion(+), 13 deletions(-) >> >> diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c >> index 683036c1c92a..cf4de80bd541 100644 >> --- a/arch/s390/kvm/sigp.c >> +++ b/arch/s390/kvm/sigp.c >> @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, >> static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, >> u64 *status_reg) >> { >> - unsigned int i; >> - struct kvm_vcpu *v; >> - bool all_stopped = true; >> - >> - kvm_for_each_vcpu(i, v, vcpu->kvm) { >> - if (v == vcpu) >> - continue; >> - if (!is_vcpu_stopped(v)) >> - all_stopped = false; >> - } >> - >> *status_reg &= 0xffffffff00000000UL; >> /* 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); >> + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; >> return SIGP_CC_STATUS_STORED; >> } > > I was initially a little bit torn by this modification, since, as you already mentioned, it could theoretically be possible that a userspace (like an older version of QEMU) does not use CZAM bit yet. But then I read an older version of the PoP which does not feature CZAM yet, and it reads: I had the same concern, if we should cope in the kernel for ancient userspace, but your explanation below makes this actually even better. And by definition in KVM we ARE always in z/Arch mode. Reviewed-by: Christian Borntraeger <borntraeger@de.ibm.com> > > "The set-architecture order is completed as follows: > • If the code in the parameter register is not 0, 1, or > 2, or if the CPU is already in the architectural > mode specified by the code, the order is not > accepted. Instead, bit 55 (invalid parameter) of > the general register designated by the R 1 field of > the SIGNAL PROCESSOR instruction is set to > one, and condition code 1 is set. > • If it is not true that all other CPUs in the configu- > ration are in the stopped or check-stop state, the > order is not accepted. Instead, bit 54 (incorrect > state) of the general register designated by the > R 1 field of the SIGNAL PROCESSOR instruction > is set to one, and condition code 1 is set. > • The architectural mode of all CPUs in the config- > uration is set as specified by the code. > ..." > > So to me this sounds like "invalid parameter" has a higher priority than "incorrect state" anyway, so we likely never > should have reported here "incorrect state"...? > > Thus, I think it's the right way to go now: > > Reviewed-by: Thomas Huth <thuth@redhat.com> >
On 08.10.21 22:31, Eric Farman wrote: > The Principles of Operations describe the various reasons that > each individual SIGP orders might be rejected, and the status > bit that are set for each condition. > > For example, for the Set Architecture order, it states: > > "If it is not true that all other CPUs in the configu- > ration are in the stopped or check-stop state, ... > bit 54 (incorrect state) ... is set to one." > > However, it also states: > > "... if the CZAM facility is installed, ... > bit 55 (invalid parameter) ... is set to one." > > Since the Configuration-z/Architecture-Architectural Mode (CZAM) > facility is unconditionally presented, there is no need to examine > each VCPU to determine if it is started/stopped. It can simply be > rejected outright with the Invalid Parameter bit. > > Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > arch/s390/kvm/sigp.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c > index 683036c1c92a..cf4de80bd541 100644 > --- a/arch/s390/kvm/sigp.c > +++ b/arch/s390/kvm/sigp.c > @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, > static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, > u64 *status_reg) > { > - unsigned int i; > - struct kvm_vcpu *v; > - bool all_stopped = true; > - > - kvm_for_each_vcpu(i, v, vcpu->kvm) { > - if (v == vcpu) > - continue; > - if (!is_vcpu_stopped(v)) > - all_stopped = false; > - } > - > *status_reg &= 0xffffffff00000000UL; > > /* 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); > + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; > return SIGP_CC_STATUS_STORED; > } > > Reviewed-by: David Hildenbrand <david@redhat.com>
On Fri, 8 Oct 2021 22:31:07 +0200 Eric Farman <farman@linux.ibm.com> wrote: > The Principles of Operations describe the various reasons that > each individual SIGP orders might be rejected, and the status > bit that are set for each condition. > > For example, for the Set Architecture order, it states: > > "If it is not true that all other CPUs in the configu- > ration are in the stopped or check-stop state, ... > bit 54 (incorrect state) ... is set to one." > > However, it also states: > > "... if the CZAM facility is installed, ... > bit 55 (invalid parameter) ... is set to one." > > Since the Configuration-z/Architecture-Architectural Mode (CZAM) > facility is unconditionally presented, there is no need to examine > each VCPU to determine if it is started/stopped. It can simply be > rejected outright with the Invalid Parameter bit. > > Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") > Signed-off-by: Eric Farman <farman@linux.ibm.com> I like removing and simplifying code while staying architecturally correct :) Reviewed-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > arch/s390/kvm/sigp.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c > index 683036c1c92a..cf4de80bd541 100644 > --- a/arch/s390/kvm/sigp.c > +++ b/arch/s390/kvm/sigp.c > @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, > static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, > u64 *status_reg) > { > - unsigned int i; > - struct kvm_vcpu *v; > - bool all_stopped = true; > - > - kvm_for_each_vcpu(i, v, vcpu->kvm) { > - if (v == vcpu) > - continue; > - if (!is_vcpu_stopped(v)) > - all_stopped = false; > - } > - > *status_reg &= 0xffffffff00000000UL; > > /* 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); > + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; > return SIGP_CC_STATUS_STORED; > } >
Am 08.10.21 um 22:31 schrieb Eric Farman: > The Principles of Operations describe the various reasons that > each individual SIGP orders might be rejected, and the status > bit that are set for each condition. > > For example, for the Set Architecture order, it states: > > "If it is not true that all other CPUs in the configu- > ration are in the stopped or check-stop state, ... > bit 54 (incorrect state) ... is set to one." > > However, it also states: > > "... if the CZAM facility is installed, ... > bit 55 (invalid parameter) ... is set to one." > > Since the Configuration-z/Architecture-Architectural Mode (CZAM) > facility is unconditionally presented, there is no need to examine > each VCPU to determine if it is started/stopped. It can simply be > rejected outright with the Invalid Parameter bit. > > Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > arch/s390/kvm/sigp.c | 14 +------------- > 1 file changed, 1 insertion(+), 13 deletions(-) > > diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c > index 683036c1c92a..cf4de80bd541 100644 > --- a/arch/s390/kvm/sigp.c > +++ b/arch/s390/kvm/sigp.c > @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, > static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, > u64 *status_reg) > { > - unsigned int i; > - struct kvm_vcpu *v; > - bool all_stopped = true; > - > - kvm_for_each_vcpu(i, v, vcpu->kvm) { > - if (v == vcpu) > - continue; > - if (!is_vcpu_stopped(v)) > - all_stopped = false; > - } > - > *status_reg &= 0xffffffff00000000UL; > > /* 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); > + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; > return SIGP_CC_STATUS_STORED; > } > > I applied this one to reduce the number of patches :-). Thanks
diff --git a/arch/s390/kvm/sigp.c b/arch/s390/kvm/sigp.c index 683036c1c92a..cf4de80bd541 100644 --- a/arch/s390/kvm/sigp.c +++ b/arch/s390/kvm/sigp.c @@ -151,22 +151,10 @@ static int __sigp_stop_and_store_status(struct kvm_vcpu *vcpu, static int __sigp_set_arch(struct kvm_vcpu *vcpu, u32 parameter, u64 *status_reg) { - unsigned int i; - struct kvm_vcpu *v; - bool all_stopped = true; - - kvm_for_each_vcpu(i, v, vcpu->kvm) { - if (v == vcpu) - continue; - if (!is_vcpu_stopped(v)) - all_stopped = false; - } - *status_reg &= 0xffffffff00000000UL; /* 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); + *status_reg |= SIGP_STATUS_INVALID_PARAMETER; return SIGP_CC_STATUS_STORED; }
The Principles of Operations describe the various reasons that each individual SIGP orders might be rejected, and the status bit that are set for each condition. For example, for the Set Architecture order, it states: "If it is not true that all other CPUs in the configu- ration are in the stopped or check-stop state, ... bit 54 (incorrect state) ... is set to one." However, it also states: "... if the CZAM facility is installed, ... bit 55 (invalid parameter) ... is set to one." Since the Configuration-z/Architecture-Architectural Mode (CZAM) facility is unconditionally presented, there is no need to examine each VCPU to determine if it is started/stopped. It can simply be rejected outright with the Invalid Parameter bit. Fixes: b697e435aeee ("KVM: s390: Support Configuration z/Architecture Mode") Signed-off-by: Eric Farman <farman@linux.ibm.com> --- arch/s390/kvm/sigp.c | 14 +------------- 1 file changed, 1 insertion(+), 13 deletions(-)