| Message ID | ab54980da397e6e9b7b8d6636dc88c11c303364f.1708933498.git.isaku.yamahata@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v19,001/130] x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro | expand |
>+static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) >+{ >+ unsigned long nr, a0, a1, a2, a3, ret; >+ do you need to emulate xen/hyper-v hypercalls here? Nothing tells userspace that xen/hyper-v hypercalls are not supported and so userspace may expose related CPUID leafs to TD guests. >+ /* >+ * ABI for KVM tdvmcall argument: >+ * In Guest-Hypervisor Communication Interface(GHCI) specification, >+ * Non-zero leaf number (R10 != 0) is defined to indicate >+ * vendor-specific. KVM uses this for KVM hypercall. NOTE: KVM >+ * hypercall number starts from one. Zero isn't used for KVM hypercall >+ * number. >+ * >+ * R10: KVM hypercall number >+ * arguments: R11, R12, R13, R14. >+ */ >+ nr = kvm_r10_read(vcpu); >+ a0 = kvm_r11_read(vcpu); >+ a1 = kvm_r12_read(vcpu); >+ a2 = kvm_r13_read(vcpu); >+ a3 = kvm_r14_read(vcpu); >+ >+ ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, true, 0); >+ >+ tdvmcall_set_return_code(vcpu, ret); >+ >+ if (nr == KVM_HC_MAP_GPA_RANGE && !ret) >+ return 0; Can you add a comment to call out that KVM_HC_MAP_GPA_RANGE is redirected to the userspace? >+ return 1; >+}
On Tue, Apr 02, 2024 at 04:52:46PM +0800, Chao Gao <chao.gao@intel.com> wrote: > >+static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) > >+{ > >+ unsigned long nr, a0, a1, a2, a3, ret; > >+ > > do you need to emulate xen/hyper-v hypercalls here? No. kvm_emulate_hypercall() handles xen/hyper-v hypercalls, __kvm_emulate_hypercall() doesn't. > Nothing tells userspace that xen/hyper-v hypercalls are not supported and > so userspace may expose related CPUID leafs to TD guests. > > >+ /* > >+ * ABI for KVM tdvmcall argument: > >+ * In Guest-Hypervisor Communication Interface(GHCI) specification, > >+ * Non-zero leaf number (R10 != 0) is defined to indicate > >+ * vendor-specific. KVM uses this for KVM hypercall. NOTE: KVM > >+ * hypercall number starts from one. Zero isn't used for KVM hypercall > >+ * number. > >+ * > >+ * R10: KVM hypercall number > >+ * arguments: R11, R12, R13, R14. > >+ */ > >+ nr = kvm_r10_read(vcpu); > >+ a0 = kvm_r11_read(vcpu); > >+ a1 = kvm_r12_read(vcpu); > >+ a2 = kvm_r13_read(vcpu); > >+ a3 = kvm_r14_read(vcpu); > >+ > >+ ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, true, 0); > >+ > >+ tdvmcall_set_return_code(vcpu, ret); > >+ > >+ if (nr == KVM_HC_MAP_GPA_RANGE && !ret) > >+ return 0; > > Can you add a comment to call out that KVM_HC_MAP_GPA_RANGE is redirected to > the userspace? Yes, this is confusing. We should refactor kvm_emulate_hypercall() more so that the caller shouldn't care about the return value like this. Will refactor it and update this patch.
On 4/4/2024 9:27 AM, Isaku Yamahata wrote: > On Tue, Apr 02, 2024 at 04:52:46PM +0800, > Chao Gao <chao.gao@intel.com> wrote: > >>> +static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) >>> +{ >>> + unsigned long nr, a0, a1, a2, a3, ret; >>> + >> do you need to emulate xen/hyper-v hypercalls here? > > No. kvm_emulate_hypercall() handles xen/hyper-v hypercalls, > __kvm_emulate_hypercall() doesn't. So for TDX, kvm doesn't support xen/hyper-v, right? Then, should KVM_CAP_XEN_HVM and KVM_CAP_HYPERV be filtered out for TDX? > >> Nothing tells userspace that xen/hyper-v hypercalls are not supported and >> so userspace may expose related CPUID leafs to TD guests. >>
On Wed, Apr 17, 2024 at 02:16:57PM +0800, Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > On 4/4/2024 9:27 AM, Isaku Yamahata wrote: > > On Tue, Apr 02, 2024 at 04:52:46PM +0800, > > Chao Gao <chao.gao@intel.com> wrote: > > > > > > +static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) > > > > +{ > > > > + unsigned long nr, a0, a1, a2, a3, ret; > > > > + > > > do you need to emulate xen/hyper-v hypercalls here? > > > > No. kvm_emulate_hypercall() handles xen/hyper-v hypercalls, > > __kvm_emulate_hypercall() doesn't. > So for TDX, kvm doesn't support xen/hyper-v, right? > > Then, should KVM_CAP_XEN_HVM and KVM_CAP_HYPERV be filtered out for TDX? That's right. We should update kvm_vm_ioctl_check_extension() and kvm_vcpu_ioctl_enable_cap(). I didn't pay attention to them.
On 4/17/2024 3:02 PM, Isaku Yamahata wrote: > On Wed, Apr 17, 2024 at 02:16:57PM +0800, > Binbin Wu <binbin.wu@linux.intel.com> wrote: > >> >> On 4/4/2024 9:27 AM, Isaku Yamahata wrote: >>> On Tue, Apr 02, 2024 at 04:52:46PM +0800, >>> Chao Gao <chao.gao@intel.com> wrote: >>> >>>>> +static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) >>>>> +{ >>>>> + unsigned long nr, a0, a1, a2, a3, ret; >>>>> + >>>> do you need to emulate xen/hyper-v hypercalls here? >>> No. kvm_emulate_hypercall() handles xen/hyper-v hypercalls, >>> __kvm_emulate_hypercall() doesn't. >> So for TDX, kvm doesn't support xen/hyper-v, right? >> >> Then, should KVM_CAP_XEN_HVM and KVM_CAP_HYPERV be filtered out for TDX? > That's right. We should update kvm_vm_ioctl_check_extension() and > kvm_vcpu_ioctl_enable_cap(). I didn't pay attention to them. Currently, QEMU checks the capabilities for Hyper-v/Xen via kvm_check_extension(), which is the global version. Only modifications in KVM can't hide these capabilities. It needs userspace to use VM or vCPU version to check the capabilities for Hyper-v and Xen. Is it a change of ABI when the old global version is still workable, but userspace switches to use VM/vCPU version to check capabilities for Hyper-v and Xen? Are there objections if both QEMU and KVM are modified in order to hide Hyper-v/Xen capabilities for TDX?
On Mon, May 27, 2024 at 08:57:28AM +0800, Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > On 4/17/2024 3:02 PM, Isaku Yamahata wrote: > > On Wed, Apr 17, 2024 at 02:16:57PM +0800, > > Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > > > > > > On 4/4/2024 9:27 AM, Isaku Yamahata wrote: > > > > On Tue, Apr 02, 2024 at 04:52:46PM +0800, > > > > Chao Gao <chao.gao@intel.com> wrote: > > > > > > > > > > +static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) > > > > > > +{ > > > > > > + unsigned long nr, a0, a1, a2, a3, ret; > > > > > > + > > > > > do you need to emulate xen/hyper-v hypercalls here? > > > > No. kvm_emulate_hypercall() handles xen/hyper-v hypercalls, > > > > __kvm_emulate_hypercall() doesn't. > > > So for TDX, kvm doesn't support xen/hyper-v, right? > > > > > > Then, should KVM_CAP_XEN_HVM and KVM_CAP_HYPERV be filtered out for TDX? > > That's right. We should update kvm_vm_ioctl_check_extension() and > > kvm_vcpu_ioctl_enable_cap(). I didn't pay attention to them. > Currently, QEMU checks the capabilities for Hyper-v/Xen via > kvm_check_extension(), which is the global version. > Only modifications in KVM can't hide these capabilities. It needs userspace > to use VM or vCPU version to check the capabilities for Hyper-v and Xen. > Is it a change of ABI when the old global version is still workable, but > userspace switches to use VM/vCPU version to check capabilities for Hyper-v > and Xen? > Are there objections if both QEMU and KVM are modified in order to > hide Hyper-v/Xen capabilities for TDX? I think it's okay for KVM_X86_TDX_VM as long as we don't change the value for KVM_X86_DEFAULT_VM. Because vm_type KVM_X86_TDX_VM is different from the default and the document (Documentation/virt/kvm/api.rst), 4.4 KVM_CHECK_EXTENSION explicitly encourages VM version. Based on their initialization different VMs may have different capabilities. It is thus encouraged to use the vm ioctl to query for capabilities (available with KVM_CAP_CHECK_EXTENSION_VM on the vm fd) The change to qemu will be mostly trivial with the quick check.
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index 0be58cd428b3..c8eb47591105 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -1008,8 +1008,41 @@ static int tdx_handle_triple_fault(struct kvm_vcpu *vcpu) return 0; } +static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu) +{ + unsigned long nr, a0, a1, a2, a3, ret; + + /* + * ABI for KVM tdvmcall argument: + * In Guest-Hypervisor Communication Interface(GHCI) specification, + * Non-zero leaf number (R10 != 0) is defined to indicate + * vendor-specific. KVM uses this for KVM hypercall. NOTE: KVM + * hypercall number starts from one. Zero isn't used for KVM hypercall + * number. + * + * R10: KVM hypercall number + * arguments: R11, R12, R13, R14. + */ + nr = kvm_r10_read(vcpu); + a0 = kvm_r11_read(vcpu); + a1 = kvm_r12_read(vcpu); + a2 = kvm_r13_read(vcpu); + a3 = kvm_r14_read(vcpu); + + ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, true, 0); + + tdvmcall_set_return_code(vcpu, ret); + + if (nr == KVM_HC_MAP_GPA_RANGE && !ret) + return 0; + return 1; +} + static int handle_tdvmcall(struct kvm_vcpu *vcpu) { + if (tdvmcall_exit_type(vcpu)) + return tdx_emulate_vmcall(vcpu); + switch (tdvmcall_leaf(vcpu)) { default: break;