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

[Steven Price <steven.price@arm.com>]

On 08/04/2025 06:19, Gavin Shan wrote:
> 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 for the pointers, I had not been aware of that change. Yes this
code should be updated to match.

Thanks,
Steve

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