| Message ID | 20250402001557.173586-2-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | TDX attestation support | expand |
On Wed, 2025-04-02 at 08:15 +0800, Binbin Wu wrote: > Handle TDVMCALL for GetQuote to generate a TD-Quote. > > GetQuote is a doorbell-like interface used by TDX guests to request VMM > to generate a TD-Quote signed by a service hosting TD-Quoting Enclave > operating on the host. A TDX guest passes a TD Report (TDREPORT_STRUCT) in > a shared-memory area as parameter. Host VMM can access it and queue the > operation for a service hosting TD-Quoting enclave. When completed, the > Quote is returned via the same shared-memory area. > > KVM forwards the request to userspace VMM (e.g., QEMU) and userspace VMM > queues the operation asynchronously. > I think the key is GetQuote is asynchronous therefore KVM will return to guest immediately after forwarding to userspace. Whether *userspace* queues the operation asynchronously doesn't matter. > After the TDVMCALL is returned and > back to TDX guest, TDX guest can poll the status field of the shared-memory > area. How about combing the above two paragraphs into: GetQuote is an asynchronous request. KVM returns to userspace immediately after forwarding the request to userspace. The TDX guest then needs to poll the status field of the shared buffer (or wait for notification if enabled) to tell whether the Quote is ready. > > Add KVM_EXIT_TDX_GET_QUOTE as a new exit reason to userspace and forward > the request after some sanity checks. > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > Tested-by: Mikko Ylinen <mikko.ylinen@linux.intel.com> > --- > Documentation/virt/kvm/api.rst | 19 ++++++++++++++++++ > arch/x86/kvm/vmx/tdx.c | 35 ++++++++++++++++++++++++++++++++++ > include/uapi/linux/kvm.h | 7 +++++++ > 3 files changed, 61 insertions(+) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index b61371f45e78..90aa7a328dc8 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -7162,6 +7162,25 @@ The valid value for 'flags' is: > - KVM_NOTIFY_CONTEXT_INVALID -- the VM context is corrupted and not valid > in VMCS. It would run into unknown result if resume the target VM. > > +:: > + > + /* KVM_EXIT_TDX_GET_QUOTE */ > + struct tdx_get_quote { > + __u64 ret; > + __u64 gpa; > + __u64 size; > + }; > + > +If the exit reason is KVM_EXIT_TDX_GET_QUOTE, then it indicates that a TDX > +guest has requested to generate a TD-Quote signed by a service hosting > +TD-Quoting Enclave operating on the host. The 'gpa' field and 'size' specify > +the guest physical address and size of a shared-memory buffer, in which the > +TDX guest passes a TD report. > "TD report" -> "TD Report"? The changelog uses the latter. > When completed, the generated quote is returned "quote" -> "Quote"? > +via the same buffer. The 'ret' field represents the return value. > return value of the GetQuote TDVMCALL? > The userspace > +should update the return value before resuming the vCPU according to TDX GHCI > +spec. > I don't quite follow. Why userspace should "update" the return value? > It's an asynchronous request. After the TDVMCALL is returned and back to > +TDX guest, TDX guest can poll the status field of the shared-memory area. > + > :: > > /* Fix the size of the union. */ > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c > index b952bc673271..535200446c21 100644 > --- a/arch/x86/kvm/vmx/tdx.c > +++ b/arch/x86/kvm/vmx/tdx.c > @@ -1463,6 +1463,39 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu) > return 1; > } > > +static int tdx_complete_get_quote(struct kvm_vcpu *vcpu) > +{ > + tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret); > + return 1; > +} > + > +static int tdx_get_quote(struct kvm_vcpu *vcpu) > +{ > + struct vcpu_tdx *tdx = to_tdx(vcpu); > + > + u64 gpa = tdx->vp_enter_args.r12; > + u64 size = tdx->vp_enter_args.r13; > + > + /* The buffer must be shared memory. */ > + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { > + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); > + return 1; > + } It is a little bit confusing about the shared buffer check here. There are two perspectives here: 1) the buffer has already been converted to shared, i.e., the attributes are stored in the Xarray. 2) the GPA passed in the GetQuote must have the shared bit set. The key is we need 1) here. From the spec, we need the 2) as well because it *seems* that the spec requires GetQuote to provide the GPA with shared bit set, as it says "Shared GPA as input". The above check only does 2). I think we need to check 1) as well, because once you forward this GetQuote to userspace, userspace is able to access it freely. As a result, the comment /* The buffer must be shared memory. */ should also be updated to something like: /* * The buffer must be shared. GetQuote requires the GPA to have * shared bit set. */ > + > + if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size)) { > + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_ALIGN_ERROR); > + return 1; > + } > + > + vcpu->run->exit_reason = KVM_EXIT_TDX_GET_QUOTE; > + vcpu->run->tdx_get_quote.gpa = gpa; > + vcpu->run->tdx_get_quote.size = size; > + > + vcpu->arch.complete_userspace_io = tdx_complete_get_quote; > + > + return 0; > +} > + > static int handle_tdvmcall(struct kvm_vcpu *vcpu) > { > switch (tdvmcall_leaf(vcpu)) { > @@ -1472,6 +1505,8 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu) > return tdx_report_fatal_error(vcpu); > case TDVMCALL_GET_TD_VM_CALL_INFO: > return tdx_get_td_vm_call_info(vcpu); > + case TDVMCALL_GET_QUOTE: > + return tdx_get_quote(vcpu); > default: > break; > } > diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h > index c6988e2c68d5..eca86b7f0cbc 100644 > --- a/include/uapi/linux/kvm.h > +++ b/include/uapi/linux/kvm.h > @@ -178,6 +178,7 @@ struct kvm_xen_exit { > #define KVM_EXIT_NOTIFY 37 > #define KVM_EXIT_LOONGARCH_IOCSR 38 > #define KVM_EXIT_MEMORY_FAULT 39 > +#define KVM_EXIT_TDX_GET_QUOTE 41 > > /* For KVM_EXIT_INTERNAL_ERROR */ > /* Emulate instruction failed. */ > @@ -447,6 +448,12 @@ struct kvm_run { > __u64 gpa; > __u64 size; > } memory_fault; > + /* KVM_EXIT_TDX_GET_QUOTE */ > + struct { > + __u64 ret; > + __u64 gpa; > + __u64 size; > + } tdx_get_quote; > /* Fix the size of the union. */ > char padding[256]; > };
On Wed, 2025-04-02 at 00:53 +0000, Huang, Kai wrote: > > > > KVM forwards the request to userspace VMM (e.g., QEMU) and userspace VMM > > queues the operation asynchronously. > > > > I think the key is GetQuote is asynchronous therefore KVM will return to guest > immediately after forwarding to userspace. Whether *userspace* queues the > operation asynchronously doesn't matter. > > > After the TDVMCALL is returned and > > back to TDX guest, TDX guest can poll the status field of the shared-memory > > area. > > How about combing the above two paragraphs into: > > GetQuote is an asynchronous request. KVM returns to userspace immediately after > forwarding the request to userspace. The TDX guest then needs to poll the > status field of the shared buffer (or wait for notification if enabled) to tell > whether the Quote is ready. I was thinking too quickly (since needed to pickup kids) and missed that it is *userspace* who makes the GetQuote asynchronous. Sorry for the noise and please ignore the above comment :-)
On 4/2/2025 8:53 AM, Huang, Kai wrote: [...] > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index b61371f45e78..90aa7a328dc8 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -7162,6 +7162,25 @@ The valid value for 'flags' is: > - KVM_NOTIFY_CONTEXT_INVALID -- the VM context is corrupted and not valid > in VMCS. It would run into unknown result if resume the target VM. > > +:: > + > + /* KVM_EXIT_TDX_GET_QUOTE */ > + struct tdx_get_quote { > + __u64 ret; > + __u64 gpa; > + __u64 size; > + }; > + > +If the exit reason is KVM_EXIT_TDX_GET_QUOTE, then it indicates that a TDX > +guest has requested to generate a TD-Quote signed by a service hosting > +TD-Quoting Enclave operating on the host. The 'gpa' field and 'size' specify > +the guest physical address and size of a shared-memory buffer, in which the > +TDX guest passes a TD report. > > "TD report" -> "TD Report"? The changelog uses the latter. > >> When completed, the generated quote is returned > "quote" -> "Quote"? > >> +via the same buffer. The 'ret' field represents the return value. >> > return value of the GetQuote TDVMCALL? Yes, thereturn code of the GetQuote TDVMCALL. > >> The userspace >> +should update the return value before resuming the vCPU according to TDX GHCI >> +spec. >> > I don't quite follow. Why userspace should "update" the return value? Because only userspace knows whether the request has been queued successfully. According to GHCI, TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. This is implementation specific as to how many concurrent requests are allowed. The TD should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple requests simultaneously. So the userspace may set the return code as TDG.VP.VMCALL_RETRY. > >> It's an asynchronous request. After the TDVMCALL is returned and back to >> +TDX guest, TDX guest can poll the status field of the shared-memory area. >> + >> :: >> >> /* Fix the size of the union. */ >> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >> index b952bc673271..535200446c21 100644 >> --- a/arch/x86/kvm/vmx/tdx.c >> +++ b/arch/x86/kvm/vmx/tdx.c >> @@ -1463,6 +1463,39 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu) >> return 1; >> } >> >> +static int tdx_complete_get_quote(struct kvm_vcpu *vcpu) >> +{ >> + tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret); >> + return 1; >> +} >> + >> +static int tdx_get_quote(struct kvm_vcpu *vcpu) >> +{ >> + struct vcpu_tdx *tdx = to_tdx(vcpu); >> + >> + u64 gpa = tdx->vp_enter_args.r12; >> + u64 size = tdx->vp_enter_args.r13; >> + >> + /* The buffer must be shared memory. */ >> + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { >> + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); >> + return 1; >> + } > It is a little bit confusing about the shared buffer check here. There are two > perspectives here: > > 1) the buffer has already been converted to shared, i.e., the attributes are > stored in the Xarray. > 2) the GPA passed in the GetQuote must have the shared bit set. > > The key is we need 1) here. From the spec, we need the 2) as well because it > *seems* that the spec requires GetQuote to provide the GPA with shared bit set, > as it says "Shared GPA as input". > > The above check only does 2). I think we need to check 1) as well, because once > you forward this GetQuote to userspace, userspace is able to access it freely. Right. Another discussion is whether KVM should skip the sanity checks for GetQuote and let the userspace take the job. Considering checking the buffer is shared memory or not, KVM seems to be a better place. > > As a result, the comment > > /* The buffer must be shared memory. */ > > should also be updated to something like: > > /* > * The buffer must be shared. GetQuote requires the GPA to have > * shared bit set. > */
On 4/2/2025 8:53 PM, Binbin Wu wrote: [...] >>> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c >>> index b952bc673271..535200446c21 100644 >>> --- a/arch/x86/kvm/vmx/tdx.c >>> +++ b/arch/x86/kvm/vmx/tdx.c >>> @@ -1463,6 +1463,39 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu) >>> return 1; >>> } >>> +static int tdx_complete_get_quote(struct kvm_vcpu *vcpu) >>> +{ >>> + tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret); >>> + return 1; >>> +} >>> + >>> +static int tdx_get_quote(struct kvm_vcpu *vcpu) >>> +{ >>> + struct vcpu_tdx *tdx = to_tdx(vcpu); >>> + >>> + u64 gpa = tdx->vp_enter_args.r12; >>> + u64 size = tdx->vp_enter_args.r13; >>> + >>> + /* The buffer must be shared memory. */ >>> + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { >>> + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); >>> + return 1; >>> + } >> It is a little bit confusing about the shared buffer check here. There are two >> perspectives here: >> >> 1) the buffer has already been converted to shared, i.e., the attributes are >> stored in the Xarray. >> 2) the GPA passed in the GetQuote must have the shared bit set. >> >> The key is we need 1) here. From the spec, we need the 2) as well because it >> *seems* that the spec requires GetQuote to provide the GPA with shared bit set, >> as it says "Shared GPA as input". >> >> The above check only does 2). I think we need to check 1) as well, because once >> you forward this GetQuote to userspace, userspace is able to access it freely. > > Right. > > Another discussion is whether KVM should skip the sanity checks for GetQuote > and let the userspace take the job. > Considering checking the buffer is shared memory or not, KVM seems to be a > better place. A second thought. If the userspace could do the shared memory check, the whole sanity checks can be done in userspace to keep KVM as small as possible. > >> >> As a result, the comment >> >> /* The buffer must be shared memory. */ >> >> should also be updated to something like: >> >> /* >> * The buffer must be shared. GetQuote requires the GPA to have >> * shared bit set. >> */ > >
> > > > > +via the same buffer. The 'ret' field represents the return value. > > > > > return value of the GetQuote TDVMCALL? > Yes, thereturn code of the GetQuote TDVMCALL. > > > > > The userspace > > > +should update the return value before resuming the vCPU according to TDX GHCI > > > +spec. > > > > > I don't quite follow. Why userspace should "update" the return value? > Because only userspace knows whether the request has been queued successfully. > > According to GHCI, TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple > requests. This is implementation specific as to how many concurrent requests > are allowed. The TD should be able to handle TDG.VP.VMCALL_RETRY if it chooses > to issue multiple requests simultaneously. > So the userspace may set the return code as TDG.VP.VMCALL_RETRY. OK. How about just say: The 'ret' field represents the return value of the GetQuote request. KVM only bridges the request to userspace VMM after sanity checks, and the userspace VMM is responsible for setting up the return value since only userspace knows whether the request has been queued successfully or not.
On Wed, 2025-04-02 at 21:16 +0800, Binbin Wu wrote: > > > > +static int tdx_get_quote(struct kvm_vcpu *vcpu) > > > > +{ > > > > + struct vcpu_tdx *tdx = to_tdx(vcpu); > > > > + > > > > + u64 gpa = tdx->vp_enter_args.r12; > > > > + u64 size = tdx->vp_enter_args.r13; > > > > + > > > > + /* The buffer must be shared memory. */ > > > > + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { > > > > + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); > > > > + return 1; > > > > + } > > > It is a little bit confusing about the shared buffer check here. There are two > > > perspectives here: > > > > > > 1) the buffer has already been converted to shared, i.e., the attributes are > > > stored in the Xarray. > > > 2) the GPA passed in the GetQuote must have the shared bit set. > > > > > > The key is we need 1) here. From the spec, we need the 2) as well because it > > > *seems* that the spec requires GetQuote to provide the GPA with shared bit set, > > > as it says "Shared GPA as input". > > > > > > The above check only does 2). I think we need to check 1) as well, because once > > > you forward this GetQuote to userspace, userspace is able to access it freely. > > > > Right. > > > > Another discussion is whether KVM should skip the sanity checks for GetQuote > > and let the userspace take the job. > > Considering checking the buffer is shared memory or not, KVM seems to be a > > better place. > A second thought. If the userspace could do the shared memory check, the > whole sanity checks can be done in userspace to keep KVM as small as possible. I am not sure depending on userspace to check is a good idea while KVM can just do it, e.g., the userspace may forget to do the check. It's consistent with other "userspace input checks" as well. Another argument is there are multiple VMMs out there and they all will need to do such check if KVM doesn't do it.
On Wed, 2025-04-02 at 08:15 +0800, Binbin Wu wrote: > +:: > + > + /* KVM_EXIT_TDX_GET_QUOTE */ > + struct tdx_get_quote { > + __u64 ret; > + __u64 gpa; > + __u64 size; > + }; > + The shared buffer pointed by the @gpa also has a format defined in the GHCI spec. E.g., it has a 'status code' field. Does userspace VMM need to setup this 'status code' as well? I recall that we used to set the GET_QUOTE_IN_FLIGHT flag in Qemu but not sure whether it is still true? I am thinking if Qemu needs to set it, then we need to expose the structure of the shared buffer to userspace too.
On 4/3/2025 6:19 AM, Huang, Kai wrote: > On Wed, 2025-04-02 at 08:15 +0800, Binbin Wu wrote: >> +:: >> + >> + /* KVM_EXIT_TDX_GET_QUOTE */ >> + struct tdx_get_quote { >> + __u64 ret; >> + __u64 gpa; >> + __u64 size; >> + }; >> + > The shared buffer pointed by the @gpa also has a format defined in the GHCI > spec. E.g., it has a 'status code' field. Does userspace VMM need to setup > this 'status code' as well? Yes. > > I recall that we used to set the GET_QUOTE_IN_FLIGHT flag in Qemu but not sure > whether it is still true? It's still true. > > I am thinking if Qemu needs to set it, then we need to expose the structure of > the shared buffer to userspace too. For the structure of the shared buffer, since KVM doesn't care about it. IMHO, it's not necessary to define the structure in KVM and userspace can define the structure according to GHCI directly.
On 4/3/2025 6:00 AM, Huang, Kai wrote: >>>> +via the same buffer. The 'ret' field represents the return value. >>>> >>> return value of the GetQuote TDVMCALL? >> Yes, thereturn code of the GetQuote TDVMCALL. >>>> The userspace >>>> +should update the return value before resuming the vCPU according to TDX GHCI >>>> +spec. >>>> >>> I don't quite follow. Why userspace should "update" the return value? >> Because only userspace knows whether the request has been queued successfully. >> >> According to GHCI, TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple >> requests. This is implementation specific as to how many concurrent requests >> are allowed. The TD should be able to handle TDG.VP.VMCALL_RETRY if it chooses >> to issue multiple requests simultaneously. >> So the userspace may set the return code as TDG.VP.VMCALL_RETRY. > OK. How about just say: > > The 'ret' field represents the return value of the GetQuote request. KVM only > bridges the request to userspace VMM after sanity checks, and the userspace VMM > is responsible for setting up the return value since only userspace knows > whether the request has been queued successfully or not. > This looks good to me. Regarding the sanity checks, I would appreciate inputs from others.
On Wed, Apr 02, 2025, Binbin Wu wrote: > On 4/2/2025 8:53 AM, Huang, Kai wrote: > > > +static int tdx_get_quote(struct kvm_vcpu *vcpu) > > > +{ > > > + struct vcpu_tdx *tdx = to_tdx(vcpu); > > > + > > > + u64 gpa = tdx->vp_enter_args.r12; > > > + u64 size = tdx->vp_enter_args.r13; > > > + > > > + /* The buffer must be shared memory. */ > > > + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { > > > + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); > > > + return 1; > > > + } > > It is a little bit confusing about the shared buffer check here. There are two > > perspectives here: > > > > 1) the buffer has already been converted to shared, i.e., the attributes are > > stored in the Xarray. > > 2) the GPA passed in the GetQuote must have the shared bit set. > > > > The key is we need 1) here. From the spec, we need the 2) as well because it > > *seems* that the spec requires GetQuote to provide the GPA with shared bit set, > > as it says "Shared GPA as input". > > > > The above check only does 2). I think we need to check 1) as well, because once > > you forward this GetQuote to userspace, userspace is able to access it freely. (1) is inherently racy. By the time KVM exits to userspace, the page could have already been converted to private in the memory attributes. KVM doesn't control shared<=>private conversions, so ultimately it's userspace's responsibility to handle this check. E.g. userspace needs to take its lock on conversions across the check+access on the buffer. Or if userpsace unmaps its shared mappings when a gfn is private, userspace could blindly access the region and handle the resulting SIGBUS (or whatever error manifests). For (2), the driving motiviation for doing the checks (or not) is KVM's ABI. I.e. whether nor KVM should handle the check depends on what KVM does for similar exits to userspace. Helping userspace is nice-to-have, but not mandatory (and helping userspace can also create undesirable ABI). My preference would be that KVM doesn't bleed the SHARED bit into its exit ABI. And at a glance, that's exactly what KVM does for KVM_HC_MAP_GPA_RANGE. In __tdx_map_gpa(), the so called "direct" bits are dropped (OMG, who's brilliant idea was it to add more use of "direct" in the MMU code): tdx->vcpu.run->hypercall.args[0] = gpa & ~gfn_to_gpa(kvm_gfn_direct_bits(tdx->vcpu.kvm)); tdx->vcpu.run->hypercall.args[1] = size / PAGE_SIZE; tdx->vcpu.run->hypercall.args[2] = vt_is_tdx_private_gpa(tdx->vcpu.kvm, gpa) ? KVM_MAP_GPA_RANGE_ENCRYPTED : KVM_MAP_GPA_RANGE_DECRYPTED; So, KVM should keep the vt_is_tdx_private_gpa(), but KVM also needs to strip the SHARED bit from the GPA reported to userspace.
On 4/9/2025 9:49 PM, Sean Christopherson wrote: > On Wed, Apr 02, 2025, Binbin Wu wrote: >> On 4/2/2025 8:53 AM, Huang, Kai wrote: >>>> +static int tdx_get_quote(struct kvm_vcpu *vcpu) >>>> +{ >>>> + struct vcpu_tdx *tdx = to_tdx(vcpu); >>>> + >>>> + u64 gpa = tdx->vp_enter_args.r12; >>>> + u64 size = tdx->vp_enter_args.r13; >>>> + >>>> + /* The buffer must be shared memory. */ >>>> + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { >>>> + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); >>>> + return 1; >>>> + } >>> It is a little bit confusing about the shared buffer check here. There are two >>> perspectives here: >>> >>> 1) the buffer has already been converted to shared, i.e., the attributes are >>> stored in the Xarray. >>> 2) the GPA passed in the GetQuote must have the shared bit set. >>> >>> The key is we need 1) here. From the spec, we need the 2) as well because it >>> *seems* that the spec requires GetQuote to provide the GPA with shared bit set, >>> as it says "Shared GPA as input". >>> >>> The above check only does 2). I think we need to check 1) as well, because once >>> you forward this GetQuote to userspace, userspace is able to access it freely. > (1) is inherently racy. By the time KVM exits to userspace, the page could have > already been converted to private in the memory attributes. KVM doesn't control > shared<=>private conversions, so ultimately it's userspace's responsibility to > handle this check. E.g. userspace needs to take its lock on conversions across > the check+access on the buffer. Or if userpsace unmaps its shared mappings when > a gfn is private, userspace could blindly access the region and handle the > resulting SIGBUS (or whatever error manifests). > > For (2), the driving motiviation for doing the checks (or not) is KVM's ABI. > I.e. whether nor KVM should handle the check depends on what KVM does for > similar exits to userspace. Helping userspace is nice-to-have, but not mandatory > (and helping userspace can also create undesirable ABI). > > My preference would be that KVM doesn't bleed the SHARED bit into its exit ABI. > And at a glance, that's exactly what KVM does for KVM_HC_MAP_GPA_RANGE. In > __tdx_map_gpa(), the so called "direct" bits are dropped (OMG, who's brilliant > idea was it to add more use of "direct" in the MMU code): > > tdx->vcpu.run->hypercall.args[0] = gpa & ~gfn_to_gpa(kvm_gfn_direct_bits(tdx->vcpu.kvm)); > tdx->vcpu.run->hypercall.args[1] = size / PAGE_SIZE; > tdx->vcpu.run->hypercall.args[2] = vt_is_tdx_private_gpa(tdx->vcpu.kvm, gpa) ? > KVM_MAP_GPA_RANGE_ENCRYPTED : > KVM_MAP_GPA_RANGE_DECRYPTED; GetQuote is the first TDX specific KVM exit reason, previous TDVMCALLs that exit to userspace are converted to exist KVM exit ABIs, e.g., TDVMCALL_MAP_GPA is converted to KVM_EXIT_HYPERCALL with KVM_HC_MAP_GPA_RANGE, so the GPA passed to userspace must have the shared bit dropped. > > So, KVM should keep the vt_is_tdx_private_gpa(), but KVM also needs to strip the > SHARED bit from the GPA reported to userspace. It makes sense to make the GPA format consistent to userspace. Thanks for the suggestion!
On Wed, 2025-04-09 at 06:49 -0700, Sean Christopherson wrote: > On Wed, Apr 02, 2025, Binbin Wu wrote: > > On 4/2/2025 8:53 AM, Huang, Kai wrote: > > > > +static int tdx_get_quote(struct kvm_vcpu *vcpu) > > > > +{ > > > > + struct vcpu_tdx *tdx = to_tdx(vcpu); > > > > + > > > > + u64 gpa = tdx->vp_enter_args.r12; > > > > + u64 size = tdx->vp_enter_args.r13; > > > > + > > > > + /* The buffer must be shared memory. */ > > > > + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { > > > > + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); > > > > + return 1; > > > > + } > > > It is a little bit confusing about the shared buffer check here. There are two > > > perspectives here: > > > > > > 1) the buffer has already been converted to shared, i.e., the attributes are > > > stored in the Xarray. > > > 2) the GPA passed in the GetQuote must have the shared bit set. > > > > > > The key is we need 1) here. From the spec, we need the 2) as well because it > > > *seems* that the spec requires GetQuote to provide the GPA with shared bit set, > > > as it says "Shared GPA as input". > > > > > > The above check only does 2). I think we need to check 1) as well, because once > > > you forward this GetQuote to userspace, userspace is able to access it freely. > > (1) is inherently racy. By the time KVM exits to userspace, the page could have > already been converted to private in the memory attributes. KVM doesn't control > shared<=>private conversions, so ultimately it's userspace's responsibility to > handle this check. E.g. userspace needs to take its lock on conversions across > the check+access on the buffer. Or if userpsace unmaps its shared mappings when > a gfn is private, userspace could blindly access the region and handle the > resulting SIGBUS (or whatever error manifests). Oh right it is racy. :-) > > For (2), the driving motiviation for doing the checks (or not) is KVM's ABI. Right. > I.e. whether nor KVM should handle the check depends on what KVM does for > similar exits to userspace. Helping userspace is nice-to-have, but not mandatory > (and helping userspace can also create undesirable ABI). > > My preference would be that KVM doesn't bleed the SHARED bit into its exit ABI. > And at a glance, that's exactly what KVM does for KVM_HC_MAP_GPA_RANGE. In > __tdx_map_gpa(), the so called "direct" bits are dropped (OMG, who's brilliant > idea was it to add more use of "direct" in the MMU code): > > tdx->vcpu.run->hypercall.args[0] = gpa & ~gfn_to_gpa(kvm_gfn_direct_bits(tdx->vcpu.kvm)); > tdx->vcpu.run->hypercall.args[1] = size / PAGE_SIZE; > tdx->vcpu.run->hypercall.args[2] = vt_is_tdx_private_gpa(tdx->vcpu.kvm, gpa) ? > KVM_MAP_GPA_RANGE_ENCRYPTED : > KVM_MAP_GPA_RANGE_DECRYPTED; > > So, KVM should keep the vt_is_tdx_private_gpa(), but KVM also needs to strip the > SHARED bit from the GPA reported to userspace. Yeah fine to me.
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index b61371f45e78..90aa7a328dc8 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -7162,6 +7162,25 @@ The valid value for 'flags' is: - KVM_NOTIFY_CONTEXT_INVALID -- the VM context is corrupted and not valid in VMCS. It would run into unknown result if resume the target VM. +:: + + /* KVM_EXIT_TDX_GET_QUOTE */ + struct tdx_get_quote { + __u64 ret; + __u64 gpa; + __u64 size; + }; + +If the exit reason is KVM_EXIT_TDX_GET_QUOTE, then it indicates that a TDX +guest has requested to generate a TD-Quote signed by a service hosting +TD-Quoting Enclave operating on the host. The 'gpa' field and 'size' specify +the guest physical address and size of a shared-memory buffer, in which the +TDX guest passes a TD report. When completed, the generated quote is returned +via the same buffer. The 'ret' field represents the return value. The userspace +should update the return value before resuming the vCPU according to TDX GHCI +spec. It's an asynchronous request. After the TDVMCALL is returned and back to +TDX guest, TDX guest can poll the status field of the shared-memory area. + :: /* Fix the size of the union. */ diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c index b952bc673271..535200446c21 100644 --- a/arch/x86/kvm/vmx/tdx.c +++ b/arch/x86/kvm/vmx/tdx.c @@ -1463,6 +1463,39 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu) return 1; } +static int tdx_complete_get_quote(struct kvm_vcpu *vcpu) +{ + tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret); + return 1; +} + +static int tdx_get_quote(struct kvm_vcpu *vcpu) +{ + struct vcpu_tdx *tdx = to_tdx(vcpu); + + u64 gpa = tdx->vp_enter_args.r12; + u64 size = tdx->vp_enter_args.r13; + + /* The buffer must be shared memory. */ + if (vt_is_tdx_private_gpa(vcpu->kvm, gpa) || size == 0) { + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND); + return 1; + } + + if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size)) { + tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_ALIGN_ERROR); + return 1; + } + + vcpu->run->exit_reason = KVM_EXIT_TDX_GET_QUOTE; + vcpu->run->tdx_get_quote.gpa = gpa; + vcpu->run->tdx_get_quote.size = size; + + vcpu->arch.complete_userspace_io = tdx_complete_get_quote; + + return 0; +} + static int handle_tdvmcall(struct kvm_vcpu *vcpu) { switch (tdvmcall_leaf(vcpu)) { @@ -1472,6 +1505,8 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu) return tdx_report_fatal_error(vcpu); case TDVMCALL_GET_TD_VM_CALL_INFO: return tdx_get_td_vm_call_info(vcpu); + case TDVMCALL_GET_QUOTE: + return tdx_get_quote(vcpu); default: break; } diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h index c6988e2c68d5..eca86b7f0cbc 100644 --- a/include/uapi/linux/kvm.h +++ b/include/uapi/linux/kvm.h @@ -178,6 +178,7 @@ struct kvm_xen_exit { #define KVM_EXIT_NOTIFY 37 #define KVM_EXIT_LOONGARCH_IOCSR 38 #define KVM_EXIT_MEMORY_FAULT 39 +#define KVM_EXIT_TDX_GET_QUOTE 41 /* For KVM_EXIT_INTERNAL_ERROR */ /* Emulate instruction failed. */ @@ -447,6 +448,12 @@ struct kvm_run { __u64 gpa; __u64 size; } memory_fault; + /* KVM_EXIT_TDX_GET_QUOTE */ + struct { + __u64 ret; + __u64 gpa; + __u64 size; + } tdx_get_quote; /* Fix the size of the union. */ char padding[256]; };