| Message ID | 20231201181657.1614645-1-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: s390: fix cc for successful PQAP | expand |
On Fri, 1 Dec 2023 19:16:57 +0100 Eric Farman <farman@linux.ibm.com> wrote: > The various errors that are possible when processing a PQAP > instruction (the absence of a driver hook, an error FROM that > hook), all correctly set the PSW condition code to 3. But if > that processing works successfully, CC0 needs to be set to > convey that everything was fine. > > Fix the check so that the guest can examine the condition code > to determine whether GPR1 has meaningful data. > > Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for AQIC") > Signed-off-by: Eric Farman <farman@linux.ibm.com> Reviewed-by: Halil Pasic <pasic@linux.ibm.com> > --- > arch/s390/kvm/priv.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 621a17fd1a1b..f875a404a0a0 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -676,8 +676,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > if (vcpu->kvm->arch.crypto.pqap_hook) { > pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > ret = pqap_hook(vcpu); > - if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > - kvm_s390_set_psw_cc(vcpu, 3); Maybe a comment that tells pqap_hook() returns 0 or -EOPNOTSUPP that singnals this should be handled by QEMU. But that can certainly be done on top, and it is not a part of a minimal fix. > + if (!ret) { > + if (vcpu->run->s.regs.gprs[1] & 0x00ff0000) > + kvm_s390_set_psw_cc(vcpu, 3); > + else > + kvm_s390_set_psw_cc(vcpu, 0); > + } > up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > return ret; > }
On 12/1/23 1:16 PM, Eric Farman wrote: > The various errors that are possible when processing a PQAP > instruction (the absence of a driver hook, an error FROM that > hook), all correctly set the PSW condition code to 3. But if > that processing works successfully, CC0 needs to be set to > convey that everything was fine. > > Fix the check so that the guest can examine the condition code > to determine whether GPR1 has meaningful data. > > Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for AQIC") > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > arch/s390/kvm/priv.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > index 621a17fd1a1b..f875a404a0a0 100644 > --- a/arch/s390/kvm/priv.c > +++ b/arch/s390/kvm/priv.c > @@ -676,8 +676,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > if (vcpu->kvm->arch.crypto.pqap_hook) { > pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > ret = pqap_hook(vcpu); > - if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > - kvm_s390_set_psw_cc(vcpu, 3); > + if (!ret) { > + if (vcpu->run->s.regs.gprs[1] & 0x00ff0000) > + kvm_s390_set_psw_cc(vcpu, 3); > + else > + kvm_s390_set_psw_cc(vcpu, 0); > + } The cc is not set if pqap_hook returns a non-zero rc; however, this point may be moot given the only non-zero rc is -EOPNOTSUPP. I'm a bit foggy on what happens when non-zero return codes are passed up the stack. > up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > return ret; > }
On Thu, 2023-12-07 at 10:39 -0500, Anthony Krowiak wrote: > > On 12/1/23 1:16 PM, Eric Farman wrote: > > The various errors that are possible when processing a PQAP > > instruction (the absence of a driver hook, an error FROM that > > hook), all correctly set the PSW condition code to 3. But if > > that processing works successfully, CC0 needs to be set to > > convey that everything was fine. > > > > Fix the check so that the guest can examine the condition code > > to determine whether GPR1 has meaningful data. > > > > Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for > > AQIC") > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > --- > > arch/s390/kvm/priv.c | 8 ++++++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c > > index 621a17fd1a1b..f875a404a0a0 100644 > > --- a/arch/s390/kvm/priv.c > > +++ b/arch/s390/kvm/priv.c > > @@ -676,8 +676,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu) > > if (vcpu->kvm->arch.crypto.pqap_hook) { > > pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; > > ret = pqap_hook(vcpu); > > - if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) > > - kvm_s390_set_psw_cc(vcpu, 3); > > + if (!ret) { > > + if (vcpu->run->s.regs.gprs[1] & 0x00ff0000) > > + kvm_s390_set_psw_cc(vcpu, 3); > > + else > > + kvm_s390_set_psw_cc(vcpu, 0); > > + } > > > The cc is not set if pqap_hook returns a non-zero rc; however, this > point may be moot given the only non-zero rc is -EOPNOTSUPP. I'm a > bit > foggy on what happens when non-zero return codes are passed up the > stack. Right, a non-zero RC will get reflected to the interception handlers, where EOPNOTSUPP instructs control to be given to userspace. So not setting a condition code is correct here, as userspace will be expected to do that. > > > > up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); > > return ret; > > }
On 12/7/23 11:11 AM, Eric Farman wrote: > On Thu, 2023-12-07 at 10:39 -0500, Anthony Krowiak wrote: >> On 12/1/23 1:16 PM, Eric Farman wrote: >>> The various errors that are possible when processing a PQAP >>> instruction (the absence of a driver hook, an error FROM that >>> hook), all correctly set the PSW condition code to 3. But if >>> that processing works successfully, CC0 needs to be set to >>> convey that everything was fine. >>> >>> Fix the check so that the guest can examine the condition code >>> to determine whether GPR1 has meaningful data. >>> >>> Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for >>> AQIC") >>> Signed-off-by: Eric Farman <farman@linux.ibm.com> >>> --- >>> arch/s390/kvm/priv.c | 8 ++++++-- >>> 1 file changed, 6 insertions(+), 2 deletions(-) >>> >>> diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c >>> index 621a17fd1a1b..f875a404a0a0 100644 >>> --- a/arch/s390/kvm/priv.c >>> +++ b/arch/s390/kvm/priv.c >>> @@ -676,8 +676,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu) >>> if (vcpu->kvm->arch.crypto.pqap_hook) { >>> pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; >>> ret = pqap_hook(vcpu); >>> - if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) >>> - kvm_s390_set_psw_cc(vcpu, 3); >>> + if (!ret) { >>> + if (vcpu->run->s.regs.gprs[1] & 0x00ff0000) >>> + kvm_s390_set_psw_cc(vcpu, 3); >>> + else >>> + kvm_s390_set_psw_cc(vcpu, 0); >>> + } >> >> The cc is not set if pqap_hook returns a non-zero rc; however, this >> point may be moot given the only non-zero rc is -EOPNOTSUPP. I'm a >> bit >> foggy on what happens when non-zero return codes are passed up the >> stack. > Right, a non-zero RC will get reflected to the interception handlers, > where EOPNOTSUPP instructs control to be given to userspace. So not > setting a condition code is correct here, as userspace will be expected > to do that. Thanks for confirming that. With that said: Reviewed-by: Tony Krowiak <akrowiak@linux.ibm.com> > >> >>> up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); >>> return ret; >>> }
On 12/1/23 19:16, Eric Farman wrote: > The various errors that are possible when processing a PQAP > instruction (the absence of a driver hook, an error FROM that > hook), all correctly set the PSW condition code to 3. But if > that processing works successfully, CC0 needs to be set to > convey that everything was fine. > > Fix the check so that the guest can examine the condition code > to determine whether GPR1 has meaningful data. > Hey Eric, I have yet to see this produce a fail in my AP KVM unit tests. If you find some spare time I'd like to discuss how I can extend my test so that I can see the fail before it's fixed.
On Fri, 2023-12-08 at 11:31 +0100, Janosch Frank wrote: > On 12/1/23 19:16, Eric Farman wrote: > > The various errors that are possible when processing a PQAP > > instruction (the absence of a driver hook, an error FROM that > > hook), all correctly set the PSW condition code to 3. But if > > that processing works successfully, CC0 needs to be set to > > convey that everything was fine. > > > > Fix the check so that the guest can examine the condition code > > to determine whether GPR1 has meaningful data. > > > > Hey Eric, I have yet to see this produce a fail in my AP KVM unit > tests. > If you find some spare time I'd like to discuss how I can extend my > test > so that I can see the fail before it's fixed. > Hi Janosch, absolutely. I had poked around kvm-unit-tests before I sent this up to see if I could adapt something to show this scenario, but came up empty and didn't want to go too far down that rabbit hole creating something from scratch. I'll ping you offline to find a time to talk. Eric
On 12/8/23 6:24 AM, Eric Farman wrote: > On Fri, 2023-12-08 at 11:31 +0100, Janosch Frank wrote: >> On 12/1/23 19:16, Eric Farman wrote: >>> The various errors that are possible when processing a PQAP >>> instruction (the absence of a driver hook, an error FROM that >>> hook), all correctly set the PSW condition code to 3. But if >>> that processing works successfully, CC0 needs to be set to >>> convey that everything was fine. >>> >>> Fix the check so that the guest can examine the condition code >>> to determine whether GPR1 has meaningful data. >>> >> Hey Eric, I have yet to see this produce a fail in my AP KVM unit >> tests. >> If you find some spare time I'd like to discuss how I can extend my >> test >> so that I can see the fail before it's fixed. >> > Hi Janosch, absolutely. I had poked around kvm-unit-tests before I sent > this up to see if I could adapt something to show this scenario, but > came up empty and didn't want to go too far down that rabbit hole > creating something from scratch. I'll ping you offline to find a time > to talk. If this is recreateable, I'd like to know how. I don't see any code path that would cause this result. > > Eric >
On Fri, 2023-12-08 at 09:21 -0500, Anthony Krowiak wrote: > > On 12/8/23 6:24 AM, Eric Farman wrote: > > On Fri, 2023-12-08 at 11:31 +0100, Janosch Frank wrote: > > > On 12/1/23 19:16, Eric Farman wrote: > > > > The various errors that are possible when processing a PQAP > > > > instruction (the absence of a driver hook, an error FROM that > > > > hook), all correctly set the PSW condition code to 3. But if > > > > that processing works successfully, CC0 needs to be set to > > > > convey that everything was fine. > > > > > > > > Fix the check so that the guest can examine the condition code > > > > to determine whether GPR1 has meaningful data. > > > > > > > Hey Eric, I have yet to see this produce a fail in my AP KVM unit > > > tests. > > > If you find some spare time I'd like to discuss how I can extend > > > my > > > test > > > so that I can see the fail before it's fixed. > > > > > Hi Janosch, absolutely. I had poked around kvm-unit-tests before I > > sent > > this up to see if I could adapt something to show this scenario, > > but > > came up empty and didn't want to go too far down that rabbit hole > > creating something from scratch. I'll ping you offline to find a > > time > > to talk. > > > If this is recreateable, I'd like to know how. I don't see any code > path > that would cause this result. Janosch and I spoke offline... He was using a proposed series of kvm- unit-tests [1] as a base, but the condition code of the PSW was zero at the time of the PQAP, meaning everything seemed fine. By dirtying the CC before the PQAP, this problem pops up quite easily, and this patch gets things back in line. [1] https://lore.kernel.org/kvm/20231117151939.971079-1-frankja@linux.ibm.com/ > > > > > > Eric > > >
On 12/1/23 19:16, Eric Farman wrote: > The various errors that are possible when processing a PQAP > instruction (the absence of a driver hook, an error FROM that > hook), all correctly set the PSW condition code to 3. But if > that processing works successfully, CC0 needs to be set to > convey that everything was fine. > > Fix the check so that the guest can examine the condition code > to determine whether GPR1 has meaningful data. > I've needed some time to remember this patch set, thanks for the ping. The patch has been pushed to devel for some CI coverage.
diff --git a/arch/s390/kvm/priv.c b/arch/s390/kvm/priv.c index 621a17fd1a1b..f875a404a0a0 100644 --- a/arch/s390/kvm/priv.c +++ b/arch/s390/kvm/priv.c @@ -676,8 +676,12 @@ static int handle_pqap(struct kvm_vcpu *vcpu) if (vcpu->kvm->arch.crypto.pqap_hook) { pqap_hook = *vcpu->kvm->arch.crypto.pqap_hook; ret = pqap_hook(vcpu); - if (!ret && vcpu->run->s.regs.gprs[1] & 0x00ff0000) - kvm_s390_set_psw_cc(vcpu, 3); + if (!ret) { + if (vcpu->run->s.regs.gprs[1] & 0x00ff0000) + kvm_s390_set_psw_cc(vcpu, 3); + else + kvm_s390_set_psw_cc(vcpu, 0); + } up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem); return ret; }
The various errors that are possible when processing a PQAP instruction (the absence of a driver hook, an error FROM that hook), all correctly set the PSW condition code to 3. But if that processing works successfully, CC0 needs to be set to convey that everything was fine. Fix the check so that the guest can examine the condition code to determine whether GPR1 has meaningful data. Fixes: e5282de93105 ("s390: ap: kvm: add PQAP interception for AQIC") Signed-off-by: Eric Farman <farman@linux.ibm.com> --- arch/s390/kvm/priv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)