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@xxxxxxxxx> Signed-off-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx> Tested-by: Farrah Chen <farrah.chen@xxxxxxxxx> --- 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@xxxxxxxxxx/, 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 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@xxxxxxxxxxxxxxx/ 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
