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

[Anthony Krowiak <akrowiak@linux.ibm.com>]

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