Re: [PATCH] i386/kvm: Set return value after handling KVM_EXIT_HYPERCALL

[Binbin Wu <binbin.wu@linux.intel.com>]

On 12/12/2024 3:09 PM, Xiaoyao Li wrote:
> On 12/12/2024 1:18 PM, Binbin Wu wrote:
>>
>>
>>
>> On 12/12/2024 11:44 AM, Xiaoyao Li wrote:
>>> On 12/12/2024 11:26 AM, Binbin Wu wrote:
>>>> Userspace should set the ret field of hypercall after handling
>>>> KVM_EXIT_HYPERCALL.  Otherwise, a stale value could be returned to KVM.
>>>>
>>>> Fixes: 47e76d03b15 ("i386/kvm: Add KVM_EXIT_HYPERCALL handling for KVM_HC_MAP_GPA_RANGE")
>>>> Reported-by: Farrah Chen <farrah.chen@intel.com>
>>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>>>> Tested-by: Farrah Chen <farrah.chen@intel.com>
>>>> ---
>>>> To test the TDX code in kvm-coco-queue, please apply the patch to the QEMU,
>>>> otherwise, TDX guest boot could fail.
>>>> A matching QEMU tree including this patch is here:
>>>> https://github.com/intel-staging/qemu-tdx/releases/tag/tdx-qemu- upstream-v6.1-fix_kvm_hypercall_return_value
>>>>
>>>> Previously, the issue was not triggered because no one would modify the ret
>>>> value. But with the refactor patch for __kvm_emulate_hypercall() in KVM,
>>>> https://lore.kernel.org/kvm/20241128004344.4072099-7- seanjc@google.com/, the
>>>> value could be modified.
>>>> ---
>>>>   target/i386/kvm/kvm.c | 8 ++++++--
>>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
>>>> index 8e17942c3b..4bcccb48d1 100644
>>>> --- a/target/i386/kvm/kvm.c
>>>> +++ b/target/i386/kvm/kvm.c
>>>> @@ -6005,10 +6005,14 @@ static int kvm_handle_hc_map_gpa_range(struct kvm_run *run)
>>>>     static int kvm_handle_hypercall(struct kvm_run *run)
>>>>   {
>>>> +    int ret = -EINVAL;
>>>> +
>>>>       if (run->hypercall.nr == KVM_HC_MAP_GPA_RANGE)
>>>> -        return kvm_handle_hc_map_gpa_range(run);
>>>> +        ret = kvm_handle_hc_map_gpa_range(run);
>>>> +
>>>> +    run->hypercall.ret = ret;
>>>
>>> Updating run->hypercall.ret is useful only when QEMU needs to re-enter the guest. For the case of ret < 0, QEMU will stop the vcpu.
>>
>> IMHO, assign run->hypercall.ret anyway should be OK, no need to add a
>> per-condition on ret, although the value is not used when ret < 0.
>>
>> Currently, since QEMU will stop the vcpu when ret < 0, this patch doesn't
>> convert ret to -Exxx that the ABI expects.
>
> I was thinking if it is better to let each specific handling function to   update run->hypercall.ret with its own logic. E.g., for this case, let kvm_handle_hc_map_gpa_range() to update the run->hypercall.ret.
I think it makes sense.

Also, each handling function can decide whether the vcpu should continue if the handling failed.
- Return 0 and set the error code ( 0 or -Exxx) to run->hypercall.ret if it want to continue.
- Return negative value if it want to stop the vcpu thread.

>
> Reusing the return value of the handling function to update
> run->hypercall.ret seems not logically correct to me.
>
>>>
>>> I think we might need re-think on the handling of KVM_EXIT_HYPERCALL. E.g., in what error case should QEMU stop the vcpu, and in what case can QEMU return the error back to the guest via run->hypercall.ret.
>>
>> Actually, I had the similar question before.
>> https://lore.kernel.org/kvm/ d25cc62c-0f56-4be2-968a-63c8b1d63b5a@linux.intel.com/
>>
>> It might depends on the hypercall number?
>> Another option is QEMU always sets run->hypercall.ret appropriately and continues the vcpu thread.
>>
>>
>>>
>>>> -    return -EINVAL;
>>>> +    return ret;
>>>>   }
>>>>     #define VMX_INVALID_GUEST_STATE 0x80000021
>>>>
>>>> base-commit: ae35f033b874c627d81d51070187fbf55f0bf1a7
>>>
>>
>