Re: [PATCH v7 28/45] arm64: rme: support RSI_HOST_CALL

[Gavin Shan <gshan@redhat.com>]

On 4/8/25 2:34 AM, Steven Price wrote:
> On 04/03/2025 06:01, Gavin Shan wrote:
>> On 2/14/25 2:14 AM, Steven Price wrote:
>>> From: Joey Gouly <joey.gouly@arm.com>
>>>
>>> Forward RSI_HOST_CALLS to KVM's HVC handler.
>>>
>>> Signed-off-by: Joey Gouly <joey.gouly@arm.com>
>>> Signed-off-by: Steven Price <steven.price@arm.com>
>>> ---
>>> Changes since v4:
>>>    * Setting GPRS is now done by kvm_rec_enter() rather than
>>>      rec_exit_host_call() (see previous patch - arm64: RME: Handle realm
>>>      enter/exit). This fixes a bug where the registers set by user space
>>>      were being ignored.
>>> ---
>>>    arch/arm64/kvm/rme-exit.c | 22 ++++++++++++++++++++++
>>>    1 file changed, 22 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/rme-exit.c b/arch/arm64/kvm/rme-exit.c
>>> index c785005f821f..4f7602aa3c6c 100644
>>> --- a/arch/arm64/kvm/rme-exit.c
>>> +++ b/arch/arm64/kvm/rme-exit.c
>>> @@ -107,6 +107,26 @@ static int rec_exit_ripas_change(struct kvm_vcpu
>>> *vcpu)
>>>        return -EFAULT;
>>>    }
>>>    +static int rec_exit_host_call(struct kvm_vcpu *vcpu)
>>> +{
>>> +    int ret, i;
>>> +    struct realm_rec *rec = &vcpu->arch.rec;
>>> +
>>> +    vcpu->stat.hvc_exit_stat++;
>>> +
>>> +    for (i = 0; i < REC_RUN_GPRS; i++)
>>> +        vcpu_set_reg(vcpu, i, rec->run->exit.gprs[i]);
>>> +
>>> +    ret = kvm_smccc_call_handler(vcpu);
>>> +
>>> +    if (ret < 0) {
>>> +        vcpu_set_reg(vcpu, 0, ~0UL);
>>> +        ret = 1;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>
>> I don't understand how a negative error can be returned from
>> kvm_smccc_call_handler().
> 
> I don't believe it really can. However kvm_smccc_call_handler() calls
> kvm_psci_call() and that has a documentation block which states:
> 
>   * This function returns: > 0 (success), 0 (success but exit to user
>   * space), and < 0 (errors)
>   *
>   * Errors:
>   * -EINVAL: Unrecognized PSCI function
> 
> But I can't actually see code which returns the negative value...
> 

I think the comments for kvm_psci_call() aren't correct since its return value
can't be negative after 7e484d2785e2 ("KVM: arm64: Return NOT_SUPPORTED to guest
for unknown PSCI version"). The comments should have been adjusted in that commit.

Please take a look on 37c8e4947947 ("KVM: arm64: Let errors from SMCCC emulation
to reach userspace"). Similarly, the block of code to set GPR0 to ~0ULL when negative
error is returned from kvm_smccc_call_handler() in this patch needs to be dropped.

>> Besides, SMCCC_RET_NOT_SUPPORTED has been set to GPR[0 - 3] if the
>> request can't be
>> supported. Why we need to set GPR[0] to ~0UL, which corresponds to
>> SMCCC_RET_NOT_SUPPORTED
>> if I'm correct. I guess change log or a comment to explain the questions
>> would be
>> nice.
> 
> I'll add a comment explaining we don't expect negative codes. And I'll
> expand ~0UL to SMCCC_RET_NOT_SUPPORTED which is what it should be.
> 

Please refer to the above reply. The block of code needs to be dropped.

> Thanks,
> Steve
> 
>>>    static void update_arch_timer_irq_lines(struct kvm_vcpu *vcpu)
>>>    {
>>>        struct realm_rec *rec = &vcpu->arch.rec;
>>> @@ -168,6 +188,8 @@ int handle_rec_exit(struct kvm_vcpu *vcpu, int
>>> rec_run_ret)
>>>            return rec_exit_psci(vcpu);
>>>        case RMI_EXIT_RIPAS_CHANGE:
>>>            return rec_exit_ripas_change(vcpu);
>>> +    case RMI_EXIT_HOST_CALL:
>>> +        return rec_exit_host_call(vcpu);
>>>        }
>>>          kvm_pr_unimpl("Unsupported exit reason: %u\n",
>>

Thanks,
Gavin