[PATCH] KVM: s390: fix cc for successful PQAP

[PATCH] KVM: s390: fix cc for successful PQAP

[Eric Farman]

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);
+		}
 		up_read(&vcpu->kvm->arch.crypto.pqap_hook_rwsem);
 		return ret;
 	}
-- 
2.40.1

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Halil Pasic]

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;
>  	}

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Anthony Krowiak]

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;
>   	}

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Eric Farman]

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;
> >         }

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Anthony Krowiak]

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;
>>>          }

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Janosch Frank]

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.

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Eric Farman]

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

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Anthony Krowiak]

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
>

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Eric Farman]

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
> > 
> 

Re: [PATCH] KVM: s390: fix cc for successful PQAP

[Janosch Frank]

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.