[PATCH 0/2] TDX attestation support

[PATCH 0/2] TDX attestation support

[Binbin Wu]

Hi,

TDX Guest-Host Communication Interface (GHCI) [0] spec defines two
TDVMCALLs for TDX attestation mechanism.

Paolo mentioned possibly wanting to include attestation in the initial TDX
support. Please consider it with that as the tentative plan. If it is not
included in the initial support, it would require an opt-in when the
support is added.

- TDG.VP.VMCALL<GetQuote>
  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.
- TDG.VP.VMCALL<SetupEventNotifyInterrupt>
  SetupEventNotifyInterrupt can be used by TDX guests to specify an
  interrupt vector as an event-notify vector for GetQuote operation, which
  may take several seconds. If a TDX guest has setup the event-notify
  vector, the host VMM injects an interrupt with the specified vector to
  the TDX guest on completion of the operation.

This patch series adds the support for TDX attestation on KVM side. KVM
forwards the requests to userspace VMM after sanity checks.  Instead of
using a single KVM_EXIT_TDX, it's preferred that each TDVMCALL that KVM
wants to forward needs a dedicated KVM_EXIT_<reason> and associated struct
in the exit union [1].  The TDVMCALLs supported in [2] reuse the existing
KVM exit reasons.  For TDX attestation support, add two TDX specific KVM
exit reasons based on the discussion in the PUCK meeting on 2025.02.19 [3].
After returning from userspace, KVM sets the return code specified by
userspace before resuming the vCPU.

Note that AMD has taken 40 for KVM_EXIT_SNP_REQ_CERTS in the patch [4]
under review, to avoid conflict, use number 41 for KVM_EXIT_TDX_GET_QUOTE
and number 42 for KVM_EXIT_TDX_SETUP_EVENT_NOTIFY.

Opens
=====
Linux TDX guests don't use SetupEventNotifyInterrupt for TD attestation
currently. If no other TDX guests use it, the support for
SetupEventNotifyInterrupt could be dropped. But it would require an opt-in
if the support is added later.

In this patch series, KVM does sanity checks for the TDVMCALLs so that
different userspace VMMs can save the code for sanity checks. But it could
be dropped if it's preferred to keep KVM code simpler and let the userspace
VMMs take the responsibility.


Base of this series
===================
This series is based on kvm-coco-queue with the commit:
- 'b7f073baa6ad' ("KVM: SVM: Enable Secure TSC for SNP guests")


Notable changes since v19 [5]
=============================
The KVM exit reason to userspace for the two TDVMCALLs was KVM_EXIT_TDX in
v19 and Sean suggested to use a dedicated KVM_EXIT_<reason> for each
TDVMCALL that exits to userspace [1].  The attestation support was dropped
in the TDX userspace exit series [6].  Now, it's changed to use two new TDX
specific KVM exit reasons KVM_EXIT_TDX_GET_QUOTE and
KVM_EXIT_TDX_SETUP_EVENT_NOTIFY.


Repos
=====
The full KVM branch is here:
https://github.com/intel/tdx/tree/tdx_kvm_dev-2025-04-01

A matching QEMU is here:
https://github.com/intel-staging/qemu-tdx/tree/tdx-qemu-wip-2025-03-19

It requires TDX module 1.5.06.00.0744 [7], or later.
A working edk2 commit is 95d8a1c ("UnitTestFrameworkPkg: Use TianoCore
mirror of subhook submodule").


Testing
=======
This patch series has been tested as part of the development branch for the
TDX base series.  The testing consisted of TDX kvm-unit-tests and booting a
Linux TD, and TDX enhanced KVM selftests.  It also passed the TDX related
test cases defined in the LKVS test suite as described in:
https://github.com/intel/lkvs/blob/main/KVM/docs/lkvs_on_avocado.md

The functionality of GetQuote has been tested with the Quote Generation
Service deployed on the host, thanks to Mikko Ylinen.

Two KVM selftests patches below were used to test the flows of GetQuote and
SetupEventNotifyInterrupt as well:
https://github.com/intel/tdx/commit/9f059b6b40d957364aeeefbbf743d080feaed501
https://github.com/intel/tdx/commit/4dd797afe5bccb7a36475aae9df8f71eabb7676a


[0] https://cdrdv2.intel.com/v1/dl/getContent/726792
[1] https://lore.kernel.org/kvm/Zg18ul8Q4PGQMWam@google.com
[2] https://lore.kernel.org/kvm/20250222014225.897298-1-binbin.wu@linux.intel.com
[3] https://drive.google.com/file/d/1fk957DWsyqWk-K-FqhBxdUtgrQYcZrqH/view?usp=drive_link&resourcekey=0-JFJuzmaZIux6_D6lhcxT7Q
[4] https://lore.kernel.org/kvm/20250219151505.3538323-2-michael.roth@amd.com
[5] https://lore.kernel.org/kvm/b9fbb0844fc6505f8fb1e9a783615b299a5a5bb3.1708933498.git.isaku.yamahata@intel.com/
[6] https://lore.kernel.org/kvm/20250222014225.897298-1-binbin.wu@linux.intel.com
[7] https://github.com/intel/tdx-module/releases/tag/TDX_1.5.06

Binbin Wu (2):
  KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>
  KVM: TDX: Handle TDG.VP.VMCALL<SetupEventNotifyInterrupt>

 Documentation/virt/kvm/api.rst    | 39 +++++++++++++++++++++
 arch/x86/include/asm/shared/tdx.h |  1 +
 arch/x86/kvm/vmx/tdx.c            | 56 +++++++++++++++++++++++++++++++
 include/uapi/linux/kvm.h          | 13 +++++++
 4 files changed, 109 insertions(+)

-- 
2.46.0

[PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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.  After the TDVMCALL is returned and
back to TDX guest, TDX guest can poll the status field of the shared-memory
area.

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. 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];
 	};
-- 
2.46.0

[PATCH 2/2] KVM: TDX: Handle TDG.VP.VMCALL<SetupEventNotifyInterrupt>

[Binbin Wu]

Handle TDVMCALL for SetupEventNotifyInterrupt to set up an event-notify
vector.

TDX guests can make a request to host VMMs to specify which interrupt
vector to use as an event-notify vector.  E.g., for GetQuote operation,
which may take several seconds, if a TDX guest has set up the event-notify
vector, the host VMM can inject an interrupt with the specified vector
to the TDX guest on the completion of the operation.

KVM itself doesn't use this mechanism.  Add KVM_EXIT_TDX_SETUP_EVENT_NOTIFY
as a new exit reason to userspace to forward the request to userspace VMM
(e.g., QEMU) after sanity checks, so that userspace can inject an interrupt
with the event-notify vector to the TDX guest when the operation completes.

Since there is nothing special for SetupEventNotifyInterrupt beyond setting
the return code in the complete_userspace_io() callback, the code reuses
the version developed for GetQuote.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
---
 Documentation/virt/kvm/api.rst    | 22 +++++++++++++++++++++-
 arch/x86/include/asm/shared/tdx.h |  1 +
 arch/x86/kvm/vmx/tdx.c            | 25 +++++++++++++++++++++++--
 include/uapi/linux/kvm.h          |  6 ++++++
 4 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst
index 90aa7a328dc8..16e15f151620 100644
--- a/Documentation/virt/kvm/api.rst
+++ b/Documentation/virt/kvm/api.rst
@@ -7179,7 +7179,27 @@ 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.
+TDX guest, TDX guest can poll the status field of the shared-memory area. Or TDX
+guest can register an event-notify vector by TDVMCALL_SETUP_EVENT_NOTIFY, so
+that on completion, an interrupt can be injected to TDX guest.
+
+::
+
+		/* KVM_EXIT_TDX_SETUP_EVENT_NOTIFY */
+		struct tdx_get_quote {
+			__u64 ret;
+			__u8 vector;
+		};
+
+If the exit reason is KVM_EXIT_TDX_SETUP_EVENT_NOTIFY, then it indicates that a
+TDX guest has requested to specify an interrupt vector used as the event-notify
+vector. E.g., for GetQuote operation, which may take several seconds, if the
+TDX guest has set up the event-notify vector, the host injects an interrupt
+with the specified vector to the guest on the completion of the operation. The
+'vector' field specifies the interrupt vector, with a valid range [32, 255], to
+be used for the event-notify. The 'ret' field represents the return value. The
+userspace should update the return value before resuming the vCPU according
+to TDX GHCI spec.
 
 ::
 
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 606d93a1cbac..eddc3f696b38 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -71,6 +71,7 @@
 #define TDVMCALL_MAP_GPA		0x10001
 #define TDVMCALL_GET_QUOTE		0x10002
 #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
+#define TDVMCALL_SETUP_EVENT_NOTIFY	0x10004
 
 /*
  * TDG.VP.VMCALL Status Codes (returned in R10)
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 535200446c21..48461520cb44 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1463,7 +1463,10 @@ static int tdx_get_td_vm_call_info(struct kvm_vcpu *vcpu)
 	return 1;
 }
 
-static int tdx_complete_get_quote(struct kvm_vcpu *vcpu)
+static_assert(offsetof(struct kvm_run, tdx_setup_event_notify.ret) ==
+	      offsetof(struct kvm_run, tdx_get_quote.ret));
+
+static int tdx_complete_tdcall_common(struct kvm_vcpu *vcpu)
 {
 	tdvmcall_set_return_code(vcpu, vcpu->run->tdx_get_quote.ret);
 	return 1;
@@ -1491,7 +1494,23 @@ static int tdx_get_quote(struct kvm_vcpu *vcpu)
 	vcpu->run->tdx_get_quote.gpa = gpa;
 	vcpu->run->tdx_get_quote.size = size;
 
-	vcpu->arch.complete_userspace_io = tdx_complete_get_quote;
+	vcpu->arch.complete_userspace_io = tdx_complete_tdcall_common;
+
+	return 0;
+}
+
+static int tdx_setup_event_notify(struct kvm_vcpu *vcpu)
+{
+	u64 vector = to_tdx(vcpu)->vp_enter_args.r12;
+
+	if (vector < 32 || vector > 255) {
+		tdvmcall_set_return_code(vcpu, TDVMCALL_STATUS_INVALID_OPERAND);
+		return 1;
+	}
+
+	vcpu->run->exit_reason = KVM_EXIT_TDX_SETUP_EVENT_NOTIFY;
+	vcpu->run->tdx_setup_event_notify.vector = (u8)vector;
+	vcpu->arch.complete_userspace_io = tdx_complete_tdcall_common;
 
 	return 0;
 }
@@ -1507,6 +1526,8 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu)
 		return tdx_get_td_vm_call_info(vcpu);
 	case TDVMCALL_GET_QUOTE:
 		return tdx_get_quote(vcpu);
+	case TDVMCALL_SETUP_EVENT_NOTIFY:
+		return tdx_setup_event_notify(vcpu);
 	default:
 		break;
 	}
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index eca86b7f0cbc..1a9397e8997b 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -179,6 +179,7 @@ struct kvm_xen_exit {
 #define KVM_EXIT_LOONGARCH_IOCSR  38
 #define KVM_EXIT_MEMORY_FAULT     39
 #define KVM_EXIT_TDX_GET_QUOTE    41
+#define KVM_EXIT_TDX_SETUP_EVENT_NOTIFY 42
 
 /* For KVM_EXIT_INTERNAL_ERROR */
 /* Emulate instruction failed. */
@@ -454,6 +455,11 @@ struct kvm_run {
 			__u64 gpa;
 			__u64 size;
 		} tdx_get_quote;
+		/* KVM_EXIT_TDX_SETUP_EVENT_NOTIFY */
+		struct {
+			__u64 ret;
+			__u8 vector;
+		} tdx_setup_event_notify;
 		/* Fix the size of the union. */
 		char padding[256];
 	};
-- 
2.46.0

Re: [PATCH 0/2] TDX attestation support

[Edgecombe, Rick P]

On Wed, 2025-04-02 at 08:15 +0800, Binbin Wu wrote:
> Opens
> =====
> Linux TDX guests don't use SetupEventNotifyInterrupt for TD attestation
> currently. If no other TDX guests use it, the support for
> SetupEventNotifyInterrupt could be dropped. But it would require an opt-in
> if the support is added later.

I think we shouldn't be afraid of opt-ins. We will need one sooner or later.
Better to not add the second exit with no users.

> 
> In this patch series, KVM does sanity checks for the TDVMCALLs so that
> different userspace VMMs can save the code for sanity checks. But it could
> be dropped if it's preferred to keep KVM code simpler and let the userspace
> VMMs take the responsibility.

I say we push it to userspace to keep KVM as small as possible.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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];
>  	};

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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.
>     */

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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.
>>     */
>
>

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Sean Christopherson]

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.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

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!

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Huang, Kai]

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.

Re: [PATCH 0/2] TDX attestation support

[Binbin Wu]

On 4/2/2025 8:20 AM, Edgecombe, Rick P wrote:
> On Wed, 2025-04-02 at 08:15 +0800, Binbin Wu wrote:
>> Opens
>> =====
>> Linux TDX guests don't use SetupEventNotifyInterrupt for TD attestation
>> currently. If no other TDX guests use it, the support for
>> SetupEventNotifyInterrupt could be dropped. But it would require an opt-in
>> if the support is added later.
> I think we shouldn't be afraid of opt-ins. We will need one sooner or later.
> Better to not add the second exit with no users.
>
OK, will drop it in the next version.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Xiaoyao Li]

On 4/2/2025 8:15 AM, 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.  After the TDVMCALL is returned and
> back to TDX guest, TDX guest can poll the status field of the shared-memory
> area.
> 
> 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. 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

Number 40 is skipped and I was told internally the reason is mentioned 
in cover letter

     Note that AMD has taken 40 for KVM_EXIT_SNP_REQ_CERTS in the
     patch [4] under review, to avoid conflict, use number 41 for
     KVM_EXIT_TDX_GET_QUOTE and number 42 for
     KVM_EXIT_TDX_SETUP_EVENT_NOTIFY.

I think we shouldn't give up number 40 unless this series depends on AMD 
one or it's agreement that AMD one will be queued/merged earlier.

>   /* 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];
>   	};

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Edgecombe, Rick P]

On Tue, 2025-04-15 at 09:49 +0800, Xiaoyao Li wrote:
> > 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
> 
> Number 40 is skipped and I was told internally the reason is mentioned 
> in cover letter
> 
>      Note that AMD has taken 40 for KVM_EXIT_SNP_REQ_CERTS in the
>      patch [4] under review, to avoid conflict, use number 41 for
>      KVM_EXIT_TDX_GET_QUOTE and number 42 for
>      KVM_EXIT_TDX_SETUP_EVENT_NOTIFY.
> 
> I think we shouldn't give up number 40 unless this series depends on AMD 
> one or it's agreement that AMD one will be queued/merged earlier.

Yes, if this patch needed to sit in kvm-coco-queue with AMD patches for awhile
it might make sense. But it sounds like the plan is to include it in base
support.

Re: [PATCH 1/2] KVM: TDX: Handle TDG.VP.VMCALL<GetQuote>

[Binbin Wu]

On 4/15/2025 9:51 AM, Edgecombe, Rick P wrote:
> On Tue, 2025-04-15 at 09:49 +0800, Xiaoyao Li wrote:
>>> 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
>> Number 40 is skipped and I was told internally the reason is mentioned
>> in cover letter
>>
>>       Note that AMD has taken 40 for KVM_EXIT_SNP_REQ_CERTS in the
>>       patch [4] under review, to avoid conflict, use number 41 for
>>       KVM_EXIT_TDX_GET_QUOTE and number 42 for
>>       KVM_EXIT_TDX_SETUP_EVENT_NOTIFY.
>>
>> I think we shouldn't give up number 40 unless this series depends on AMD
>> one or it's agreement that AMD one will be queued/merged earlier.
> Yes, if this patch needed to sit in kvm-coco-queue with AMD patches for awhile
> it might make sense. But it sounds like the plan is to include it in base
> support.
Right, now it seems that this patch could probably win the race upstream,
will use number 40 for KVM_EXIT_TDX_GET_QUOTE in the next version if no
objections.