[PATCH v2 0/3] TDX Guest Quote generation support

[PATCH v2 0/3] TDX Guest Quote generation support

[Kuppuswamy Sathyanarayanan]

Hi All,

In TDX guest, the attestation process is used to verify the TDX guest
trustworthiness to other entities before provisioning secrets to the
guest.

The TDX guest attestation process consists of two steps:

1. TDREPORT generation
2. Quote generation.

The First step (TDREPORT generation) involves getting the TDX guest
measurement data in the format of TDREPORT which is further used to
validate the authenticity of the TDX guest. The second step involves
sending the TDREPORT to a Quoting Enclave (QE) server to generate a
remotely verifiable Quote. TDREPORT by design can only be verified on
the local platform. To support remote verification of the TDREPORT,
TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT
locally and convert it to a remotely verifiable Quote. Although
attestation software can use communication methods like TCP/IP or
vsock to send the TDREPORT to QE, not all platforms support these
communication models. So TDX GHCI specification [1] defines a method
for Quote generation via hypercalls. Please check the discussion from
Google [2] and Alibaba [3] which clarifies the need for hypercall based
Quote generation support. This patch set adds this support.

Support for TDREPORT generation already exists in the TDX guest driver. 
This patchset extends the same driver to add the Quote generation
support.

Following are the details of the patch set:

Patch 1/3 -> Adds event notification IRQ support.
Patch 2/3 -> Adds Quote generation support.
Patch 3/3 -> Adds selftest support for Quote generation feature.

[1] https://cdrdv2.intel.com/v1/dl/getContent/726790, section titled "TDG.VP.VMCALL<GetQuote>".
[2] https://lore.kernel.org/lkml/CAAYXXYxxs2zy_978GJDwKfX5Hud503gPc8=1kQ-+JwG_kA79mg@mail.gmail.com/
[3] https://lore.kernel.org/lkml/a69faebb-11e8-b386-d591-dbd08330b008@linux.alibaba.com/

Kuppuswamy Sathyanarayanan (3):
  x86/tdx: Add TDX Guest event notify interrupt support
  virt: tdx-guest: Add Quote generation support
  selftests/tdx: Test GetQuote TDX attestation feature

 Documentation/virt/coco/tdx-guest.rst        |  11 ++
 arch/x86/coco/tdx/tdx.c                      | 196 +++++++++++++++++++
 arch/x86/include/asm/tdx.h                   |   8 +
 drivers/virt/coco/tdx-guest/tdx-guest.c      | 168 +++++++++++++++-
 include/uapi/linux/tdx-guest.h               |  43 ++++
 tools/testing/selftests/tdx/tdx_guest_test.c |  68 ++++++-
 6 files changed, 487 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Kuppuswamy Sathyanarayanan]

Host-guest event notification via configured interrupt vector is useful
in cases where a guest makes an asynchronous request and needs a
callback from the host to indicate the completion or to let the host
notify the guest about events like device removal. One usage example is,
callback requirement of GetQuote asynchronous hypercall.

In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
guest to specify which interrupt vector to use as an event-notify
vector from the VMM. Details about the SetupEventNotifyInterrupt
hypercall can be found in TDX Guest-Host Communication Interface
(GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".

As per design, VMM will post the event completion IRQ using the same
CPU on which SetupEventNotifyInterrupt hypercall request is received.
So allocate an IRQ vector from "x86_vector_domain", and set the CPU
affinity of the IRQ vector to the CPU on which
SetupEventNotifyInterrupt hypercall is made.

Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
interfaces to allow drivers register/unregister event noficiation
handlers.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Wander Lairson Costa <wander@redhat.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v1:
 * Used early_initcall() instead of arch_initcall() to trigger
   tdx_event_irq_init().
 * Removed unused headers and included headers for spinlock and list
   explicitly.
 * Since during the early_initcall() only one CPU would be enabled, remove
   CPU locking logic (like using set_cpus_allowed_ptr() or get_cpu())

 arch/x86/coco/tdx/tdx.c    | 156 +++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/tdx.h |   6 ++
 2 files changed, 162 insertions(+)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 055300e08fb3..26f6e2eaf5c8 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -7,12 +7,17 @@
 #include <linux/cpufeature.h>
 #include <linux/export.h>
 #include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/list.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
 #include <asm/coco.h>
 #include <asm/tdx.h>
 #include <asm/vmx.h>
 #include <asm/insn.h>
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
+#include <asm/irqdomain.h>
 
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
@@ -27,6 +32,7 @@
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
 #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -51,6 +57,16 @@
 
 #define TDREPORT_SUBTYPE_0	0
 
+struct event_irq_entry {
+	tdx_event_irq_cb_t handler;
+	void *data;
+	struct list_head head;
+};
+
+static int tdx_event_irq __ro_after_init;
+static LIST_HEAD(event_irq_cb_list);
+static DEFINE_SPINLOCK(event_irq_cb_lock);
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -873,3 +889,143 @@ void __init tdx_early_init(void)
 
 	pr_info("Guest detected\n");
 }
+
+static irqreturn_t tdx_event_irq_handler(int irq, void *dev_id)
+{
+	struct event_irq_entry *entry;
+
+	spin_lock(&event_irq_cb_lock);
+	list_for_each_entry(entry, &event_irq_cb_list, head) {
+		if (entry->handler)
+			entry->handler(entry->data);
+	}
+	spin_unlock(&event_irq_cb_lock);
+
+	return IRQ_HANDLED;
+}
+
+/* Reserve an IRQ from x86_vector_domain for TD event notification */
+static int __init tdx_event_irq_init(void)
+{
+	struct irq_alloc_info info;
+	struct irq_cfg *cfg;
+	int irq;
+
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return 0;
+
+	init_irq_alloc_info(&info, NULL);
+
+	/*
+	 * Event notification vector will be delivered to the CPU
+	 * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
+	 * So set the IRQ affinity to the current CPU (CPU 0).
+	 */
+	info.mask = cpumask_of(0);
+
+	irq = irq_domain_alloc_irqs(x86_vector_domain, 1, cpu_to_node(0), &info);
+	if (irq <= 0) {
+		pr_err("Event notification IRQ allocation failed %d\n", irq);
+		return -EIO;
+	}
+
+	irq_set_handler(irq, handle_edge_irq);
+
+	/* Since the IRQ affinity is set, it cannot be balanced */
+	if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
+			"tdx_event_irq", NULL)) {
+		pr_err("Event notification IRQ request failed\n");
+		goto err_free_domain_irqs;
+	}
+
+	cfg = irq_cfg(irq);
+
+	/*
+	 * Since tdx_event_irq_init() is triggered via early_initcall(),
+	 * it will called before secondary CPUs bringup. Since there is
+	 * only one CPU, it complies with the requirement of executing
+	 * the TDVMCALL_SETUP_NOTIFY_INTR hypercall on the same CPU where
+	 * the IRQ vector is allocated.
+	 *
+	 * Register callback vector address with VMM. More details
+	 * about the ABI can be found in TDX Guest-Host-Communication
+	 * Interface (GHCI), sec titled
+	 * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
+	 */
+	if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
+		pr_err("Event notification hypercall failed\n");
+		goto err_free_irqs;
+	}
+
+	tdx_event_irq = irq;
+
+	return 0;
+
+err_free_irqs:
+	free_irq(irq, NULL);
+err_free_domain_irqs:
+	irq_domain_free_irqs(irq, 1);
+
+	return -EIO;
+}
+early_initcall(tdx_event_irq_init)
+
+/**
+ * tdx_register_event_irq_cb() - Register TDX event IRQ callback handler.
+ * @handler: Address of driver specific event IRQ callback handler. Handler
+ *           will be called in IRQ context and hence cannot sleep.
+ * @data: Context data to be passed to the callback handler.
+ *
+ * Return: 0 on success or standard error code on other failures.
+ */
+int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data)
+{
+	struct event_irq_entry *entry;
+	unsigned long flags;
+
+	if (tdx_event_irq <= 0)
+		return -EIO;
+
+	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->data = data;
+	entry->handler = handler;
+
+	spin_lock_irqsave(&event_irq_cb_lock, flags);
+	list_add_tail(&entry->head, &event_irq_cb_list);
+	spin_unlock_irqrestore(&event_irq_cb_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_register_event_irq_cb);
+
+/**
+ * tdx_unregister_event_irq_cb() - Unregister TDX event IRQ callback handler.
+ * @handler: Address of driver specific event IRQ callback handler.
+ * @data: Context data to be passed to the callback handler.
+ *
+ * Return: 0 on success or -EIO if event IRQ is not allocated.
+ */
+int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data)
+{
+	struct event_irq_entry *entry;
+	unsigned long flags;
+
+	if (tdx_event_irq <= 0)
+		return -EIO;
+
+	spin_lock_irqsave(&event_irq_cb_lock, flags);
+	list_for_each_entry(entry, &event_irq_cb_list, head) {
+		if (entry->handler == handler && entry->data == data) {
+			list_del(&entry->head);
+			kfree(entry);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&event_irq_cb_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tdx_unregister_event_irq_cb);
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 28d889c9aa16..8807fe1b1f3f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -53,6 +53,8 @@ struct ve_info {
 
 #ifdef CONFIG_INTEL_TDX_GUEST
 
+typedef int (*tdx_event_irq_cb_t)(void *);
+
 void __init tdx_early_init(void);
 
 /* Used to communicate with the TDX module */
@@ -69,6 +71,10 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
 
 int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
 
+int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
+
+int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
+
 #else
 
 static inline void tdx_early_init(void) { };
-- 
2.34.1

Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Huang, Kai]

On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
> Host-guest event notification via configured interrupt vector is useful
> in cases where a guest makes an asynchronous request and needs a
> callback from the host to indicate the completion or to let the host
> notify the guest about events like device removal. One usage example is,
> callback requirement of GetQuote asynchronous hypercall.
> 
> In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
> guest to specify which interrupt vector to use as an event-notify
> vector from the VMM. Details about the SetupEventNotifyInterrupt
> hypercall can be found in TDX Guest-Host Communication Interface
> (GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".
> 
> As per design, VMM will post the event completion IRQ using the same
> CPU on which SetupEventNotifyInterrupt hypercall request is received.
> So allocate an IRQ vector from "x86_vector_domain", and set the CPU
> affinity of the IRQ vector to the CPU on which
> SetupEventNotifyInterrupt hypercall is made.
> 
> Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
> interfaces to allow drivers register/unregister event noficiation
			      ^
			      to register/unregister
> handlers.
> 
> 

[...]

> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
> +static int __init tdx_event_irq_init(void)
> +{
> +	struct irq_alloc_info info;
> +	struct irq_cfg *cfg;
> +	int irq;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return 0;
> +
> +	init_irq_alloc_info(&info, NULL);
> +
> +	/*
> +	 * Event notification vector will be delivered to the CPU
> +	 * in which TDVMCALL_SETUP_NOTIFY_INTR hypercall is requested.
	   ^
	   on which (to be consistent with the changelog)

> +	 * So set the IRQ affinity to the current CPU (CPU 0).
> +	 */
> +	info.mask = cpumask_of(0);

I think we should use smp_processor_id() to get the CPU id even for BSP.

> +
> +	irq = irq_domain_alloc_irqs(x86_vector_domain, 1, cpu_to_node(0), &info);
> +	if (irq <= 0) {
> +		pr_err("Event notification IRQ allocation failed %d\n", irq);
> +		return -EIO;
> +	}
> +
> +	irq_set_handler(irq, handle_edge_irq);
> +
> +	/* Since the IRQ affinity is set, it cannot be balanced */
> +	if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
> +			"tdx_event_irq", NULL)) {
> +		pr_err("Event notification IRQ request failed\n");
> +		goto err_free_domain_irqs;
> +	}

Firstly, this comment isn't right.  The @info->mask is only used to allocate the
vector of the IRQ, but it doesn't have anything to do with IRQ affinity. 
irq_domain_alloc_irqs() will set the IRQ to have the default affinity in fact.  

The comment should be something like below instead:

	/*
	 * The IRQ cannot be migrated because VMM always injects the vector
	 * event to the cpu on which the SetupEventNotifyInterrupt TDVMCALL
	 * is called.  Set the IRQ with IRQF_NOBALANCING to prevent its
affinity 
	 * from being changed.
	 */

That also being said, you actually haven't set IRQ's affinity to the BSP yet
before request_irq().  IIUC you can either:

1) Explicitly call irq_set_affinity() after irq_domain_alloc_irqs() to set
affinity to BSP; or

2) Use __irq_domain_alloc_irqs(), which allows you to set the affinity directly,
to allocate the IRQ instead of irq_domain_alloc_irqs().

	struct irq_affinity_desc desc;

	cpumask_set_cpu(smp_processor_id(), &desc.mask);

	irq = __irq_domain_alloc_irqs(..., &desc);

Personally I think 2) is more straightforward.

Using 2) also allows you to alternatively set desc.is_managed to 1 to set the
IRQ as kernel managed.  This will prevent userspace from changing IRQ affinity.
Kernel can still change the affinity, though, but no other kernel code will do
that.

So both kernel managed affinity IRQ and IRQF_NOBALANCING should work.  I have no
opinion on this.

And IIUC if you already set IRQ's affinity to BSP before request_irq(), then you
don't need to do:

	info.mask = cpumask_of(0);

before allocating the IRQ as the vector will be allocated in request_irq() on
the BSP anyway.


> +
> +	cfg = irq_cfg(irq);
> +
> +	/*
> +	 * Since tdx_event_irq_init() is triggered via early_initcall(),
> +	 * it will called before secondary CPUs bringup. Since there is
> +	 * only one CPU, it complies with the requirement of executing
> +	 * the TDVMCALL_SETUP_NOTIFY_INTR hypercall on the same CPU where
> +	 * the IRQ vector is allocated.

IMHO this is unnecessary complicated.

In fact, IMHO we can just have one simple comment at the very beginning to
explain the whole thing:

	/*
	 * TDX guest uses SetupEventNotifyInterrupt TDVMCALL to allow the
	 * hypervisor to notify the TDX guest when needed, for instance,
	 * when VMM finishes the GetQuote request from the TDX guest.  
	 *
	 * The VMM always notifies the TDX guest via the vector specified in
the
	 * SetupEventNotifyInterrupt TDVMCALL to the cpu on which the TDVMCALL
	 * is called.  For simplicity, just allocate an IRQ (and a vector) 
	 * directly from x86_vector_domain for such notification and pin the
	 * IRQ to the BSP.
	 */

And then all the code follows.

> +	 *
> +	 * Register callback vector address with VMM. More details
> +	 * about the ABI can be found in TDX Guest-Host-Communication
> +	 * Interface (GHCI), sec titled
> +	 * "TDG.VP.VMCALL<SetupEventNotifyInterrupt>".
> +	 */
> +	if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
> +		pr_err("Event notification hypercall failed\n");
> +		goto err_free_irqs;
> +	}
> +
> +	tdx_event_irq = irq;
> +
> +	return 0;
> +
> +err_free_irqs:
> +	free_irq(irq, NULL);
> +err_free_domain_irqs:
> +	irq_domain_free_irqs(irq, 1);
> +
> +	return -EIO;
> +}
> +early_initcall(tdx_event_irq_init)
> +
> 

[...]

Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 4/14/23 6:34 AM, Huang, Kai wrote:
> On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
>> Host-guest event notification via configured interrupt vector is useful
>> in cases where a guest makes an asynchronous request and needs a
>> callback from the host to indicate the completion or to let the host
>> notify the guest about events like device removal. One usage example is,
>> callback requirement of GetQuote asynchronous hypercall.
>>
>> In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
>> guest to specify which interrupt vector to use as an event-notify
>> vector from the VMM. Details about the SetupEventNotifyInterrupt
>> hypercall can be found in TDX Guest-Host Communication Interface
>> (GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".
>>
>> As per design, VMM will post the event completion IRQ using the same
>> CPU on which SetupEventNotifyInterrupt hypercall request is received.
>> So allocate an IRQ vector from "x86_vector_domain", and set the CPU
>> affinity of the IRQ vector to the CPU on which
>> SetupEventNotifyInterrupt hypercall is made.
>>
>> Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
>> interfaces to allow drivers register/unregister event noficiation
> 			      ^
> 			      to register/unregister
>> handlers.
>>
>>
> 
> [...]
> 

With suggested changes, the final version looks like below.

+/**
+ * tdx_event_irq_init() - Register IRQ for event notification from the VMM to
+ *                       the TDX Guest.
+ *
+ * Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
+ * IRQ with the VMM, which is used by the VMM to notify the TDX guest when
+ * needed, for instance, when VMM finishes the GetQuote request from the TDX
+ * guest. The VMM always notifies the TDX guest via the same CPU on which the
+ * SetupEventNotifyInterrupt TDVMCALL is called. For simplicity, just allocate
+ * an IRQ (and a vector) directly from x86_vector_domain for such notification
+ * and pin the IRQ to the same CPU on which TDVMCALL is called.
+ *
+ * Since tdx_event_irq_init() is triggered via early_initcall(), it will be
+ * called before secondary CPUs bring up, so no special logic is required to
+ * ensure that the same CPU is used for SetupEventNotifyInterrupt TDVMCALL and
+ * IRQ allocation.
+ */
+static int __init tdx_event_irq_init(void)
+{
+       struct irq_affinity_desc desc;
+       struct irq_alloc_info info;
+       struct irq_cfg *cfg;
+       int irq;
+
+       if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+               return 0;
+
+       init_irq_alloc_info(&info, NULL);
+
+       cpumask_set_cpu(smp_processor_id(), &desc.mask);
+
+       irq = __irq_domain_alloc_irqs(x86_vector_domain, -1, 1, cpu_to_node(0),
+                                     &info, false, &desc);
+       if (irq <= 0) {
+               pr_err("Event notification IRQ allocation failed %d\n", irq);
+               return -EIO;
+       }
+
+       irq_set_handler(irq, handle_edge_irq);
+
+       /*
+        * The IRQ cannot be migrated because VMM always notifies the TDX
+        * guest on the same CPU on which the SetupEventNotifyInterrupt
+        * TDVMCALL is called. Set the IRQ with IRQF_NOBALANCING to prevent
+        * its affinity from being changed.
+        */
+       if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
+                       "tdx_event_irq", NULL)) {
+               pr_err("Event notification IRQ request failed\n");
+               goto err_free_domain_irqs;
+       }
+
+       cfg = irq_cfg(irq);
+
+       if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
+               pr_err("Event notification hypercall failed\n");
+               goto err_free_irqs;
+       }
+
+       tdx_event_irq = irq;
+
+       return 0;
+
+err_free_irqs:
+       free_irq(irq, NULL);
+err_free_domain_irqs:
+       irq_domain_free_irqs(irq, 1);
+
+       return -EIO;
+}
+early_initcall(tdx_event_irq_init)




-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Huang, Kai]

On Tue, 2023-04-25 at 16:47 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
> 
> On 4/14/23 6:34 AM, Huang, Kai wrote:
> > On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
> > > Host-guest event notification via configured interrupt vector is useful
> > > in cases where a guest makes an asynchronous request and needs a
> > > callback from the host to indicate the completion or to let the host
> > > notify the guest about events like device removal. One usage example is,
> > > callback requirement of GetQuote asynchronous hypercall.
> > > 
> > > In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
> > > guest to specify which interrupt vector to use as an event-notify
> > > vector from the VMM. Details about the SetupEventNotifyInterrupt
> > > hypercall can be found in TDX Guest-Host Communication Interface
> > > (GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".
> > > 
> > > As per design, VMM will post the event completion IRQ using the same
> > > CPU on which SetupEventNotifyInterrupt hypercall request is received.
> > > So allocate an IRQ vector from "x86_vector_domain", and set the CPU
> > > affinity of the IRQ vector to the CPU on which
> > > SetupEventNotifyInterrupt hypercall is made.
> > > 
> > > Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
> > > interfaces to allow drivers register/unregister event noficiation
> > 			      ^
> > 			      to register/unregister
> > > handlers.
> > > 
> > > 
> > 
> > [...]
> > 
> 
> With suggested changes, the final version looks like below.
> 
> +/**
> + * tdx_event_irq_init() - Register IRQ for event notification from the VMM to
> + *                       the TDX Guest.
> + *
> + * Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
> + * IRQ with the VMM, which is used by the VMM to notify the TDX guest when
> + * needed, for instance, when VMM finishes the GetQuote request from the TDX
> + * guest. The VMM always notifies the TDX guest via the same CPU on which the
> + * SetupEventNotifyInterrupt TDVMCALL is called. For simplicity, just allocate
> + * an IRQ (and a vector) directly from x86_vector_domain for such notification
> + * and pin the IRQ to the same CPU on which TDVMCALL is called.

I think "for simplicity" applies to allocate IRQ/vector "from BSP using
early_initcall()" (so TDVMCALL is easily guaranteed to be called on the same cpu
where vector is allocated), but doesn't apply to allocate IRQ/vector from
x86_vector_domain and "pin the IRQ to the same CPU on which TDVMCALAL is
called".  The latter is something you must do (otherwise you need to allocate
the same vector on all cpus), but not something that you do "for simplicity".

> + *
> + * Since tdx_event_irq_init() is triggered via early_initcall(), it will be
> + * called before secondary CPUs bring up, so no special logic is required to
> + * ensure that the same CPU is used for SetupEventNotifyInterrupt TDVMCALL and
> + * IRQ allocation.

IMHO the second paragraph is obvious and no need to mention.

As explained above, I guess you just need to at somewhere simply mention
something like: "for simplicity use early_initcall() to allocate and pin the
IRQ/vector on BSP and also call the TDVMCALL on BSP".  Or probably "also call
the TDVMCALL on BSP" can also be omitted as it's kinda already explained in the
nature of the TDVMCALL.

> + */
> +static int __init tdx_event_irq_init(void)
> +{
> +       struct irq_affinity_desc desc;
> +       struct irq_alloc_info info;
> +       struct irq_cfg *cfg;
> +       int irq;
> +
> +       if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +               return 0;
> +
> +       init_irq_alloc_info(&info, NULL);
> +
> +       cpumask_set_cpu(smp_processor_id(), &desc.mask);
> +
> +       irq = __irq_domain_alloc_irqs(x86_vector_domain, -1, 1, cpu_to_node(0),

cpu_to_node(smp_processor_id())?

> +                                     &info, false, &desc);
> +       if (irq <= 0) {
> +               pr_err("Event notification IRQ allocation failed %d\n", irq);
> +               return -EIO;
> +       }
> +
> +       irq_set_handler(irq, handle_edge_irq);
> +
> +       /*
> +        * The IRQ cannot be migrated because VMM always notifies the TDX
> +        * guest on the same CPU on which the SetupEventNotifyInterrupt
> +        * TDVMCALL is called. Set the IRQ with IRQF_NOBALANCING to prevent
> +        * its affinity from being changed.
> +        */
> +       if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
> +                       "tdx_event_irq", NULL)) {
> +               pr_err("Event notification IRQ request failed\n");
> +               goto err_free_domain_irqs;
> +       }
> +
> +       cfg = irq_cfg(irq);
> +
> +       if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
> +               pr_err("Event notification hypercall failed\n");
> +               goto err_free_irqs;
> +       }
> +
> +       tdx_event_irq = irq;
> +
> +       return 0;
> +
> +err_free_irqs:
> +       free_irq(irq, NULL);
> +err_free_domain_irqs:
> +       irq_domain_free_irqs(irq, 1);
> +
> +       return -EIO;
> +}
> +early_initcall(tdx_event_irq_init)
> 
> 
> 
> 
> -- 
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer

Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Sathyanarayanan Kuppuswamy]

On 4/25/23 6:59 PM, Huang, Kai wrote:
> On Tue, 2023-04-25 at 16:47 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi Kai,
>>
>> On 4/14/23 6:34 AM, Huang, Kai wrote:
>>> On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
>>>> Host-guest event notification via configured interrupt vector is useful
>>>> in cases where a guest makes an asynchronous request and needs a
>>>> callback from the host to indicate the completion or to let the host
>>>> notify the guest about events like device removal. One usage example is,
>>>> callback requirement of GetQuote asynchronous hypercall.
>>>>
>>>> In TDX guest, SetupEventNotifyInterrupt hypercall can be used by the
>>>> guest to specify which interrupt vector to use as an event-notify
>>>> vector from the VMM. Details about the SetupEventNotifyInterrupt
>>>> hypercall can be found in TDX Guest-Host Communication Interface
>>>> (GHCI) Specification, section "VP.VMCALL<SetupEventNotifyInterrupt>".
>>>>
>>>> As per design, VMM will post the event completion IRQ using the same
>>>> CPU on which SetupEventNotifyInterrupt hypercall request is received.
>>>> So allocate an IRQ vector from "x86_vector_domain", and set the CPU
>>>> affinity of the IRQ vector to the CPU on which
>>>> SetupEventNotifyInterrupt hypercall is made.
>>>>
>>>> Add tdx_register_event_irq_cb()/tdx_unregister_event_irq_cb()
>>>> interfaces to allow drivers register/unregister event noficiation
>>> 			      ^
>>> 			      to register/unregister
>>>> handlers.
>>>>
>>>>
>>>
>>> [...]
>>>
>>
>> With suggested changes, the final version looks like below.
>>
>> +/**
>> + * tdx_event_irq_init() - Register IRQ for event notification from the VMM to
>> + *                       the TDX Guest.
>> + *
>> + * Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
>> + * IRQ with the VMM, which is used by the VMM to notify the TDX guest when
>> + * needed, for instance, when VMM finishes the GetQuote request from the TDX
>> + * guest. The VMM always notifies the TDX guest via the same CPU on which the
>> + * SetupEventNotifyInterrupt TDVMCALL is called. For simplicity, just allocate
>> + * an IRQ (and a vector) directly from x86_vector_domain for such notification
>> + * and pin the IRQ to the same CPU on which TDVMCALL is called.
> 
> I think "for simplicity" applies to allocate IRQ/vector "from BSP using
> early_initcall()" (so TDVMCALL is easily guaranteed to be called on the same cpu
> where vector is allocated), but doesn't apply to allocate IRQ/vector from
> x86_vector_domain and "pin the IRQ to the same CPU on which TDVMCALAL is
> called".  The latter is something you must do (otherwise you need to allocate
> the same vector on all cpus), but not something that you do "for simplicity".
> 
>> + *
>> + * Since tdx_event_irq_init() is triggered via early_initcall(), it will be
>> + * called before secondary CPUs bring up, so no special logic is required to
>> + * ensure that the same CPU is used for SetupEventNotifyInterrupt TDVMCALL and
>> + * IRQ allocation.
> 
> IMHO the second paragraph is obvious and no need to mention.
> 
> As explained above, I guess you just need to at somewhere simply mention
> something like: "for simplicity use early_initcall() to allocate and pin the
> IRQ/vector on BSP and also call the TDVMCALL on BSP".  Or probably "also call
> the TDVMCALL on BSP" can also be omitted as it's kinda already explained in the
> nature of the TDVMCALL.

How about the following?

Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
IRQ with the VMM, which is used by the VMM to notify the TDX guest when
needed, for instance, when VMM finishes the GetQuote request from the TDX
guest. The VMM always notifies the TDX guest via the same CPU that calls the
SetupEventNotifyInterrupt TDVMCALL. Allocate an IRQ/vector from the
x86_vector_domain and pin it on the same CPU on which TDVMCALL is called.
For simplicity, use early_initcall() to allow both IRQ allocation and
TDVMCALL to use BSP.

> 
>> + */
>> +static int __init tdx_event_irq_init(void)
>> +{
>> +       struct irq_affinity_desc desc;
>> +       struct irq_alloc_info info;
>> +       struct irq_cfg *cfg;
>> +       int irq;
>> +
>> +       if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +               return 0;
>> +
>> +       init_irq_alloc_info(&info, NULL);
>> +
>> +       cpumask_set_cpu(smp_processor_id(), &desc.mask);
>> +
>> +       irq = __irq_domain_alloc_irqs(x86_vector_domain, -1, 1, cpu_to_node(0),
> 
> cpu_to_node(smp_processor_id())?
> 
>> +                                     &info, false, &desc);
>> +       if (irq <= 0) {
>> +               pr_err("Event notification IRQ allocation failed %d\n", irq);
>> +               return -EIO;
>> +       }
>> +
>> +       irq_set_handler(irq, handle_edge_irq);
>> +
>> +       /*
>> +        * The IRQ cannot be migrated because VMM always notifies the TDX
>> +        * guest on the same CPU on which the SetupEventNotifyInterrupt
>> +        * TDVMCALL is called. Set the IRQ with IRQF_NOBALANCING to prevent
>> +        * its affinity from being changed.
>> +        */
>> +       if (request_irq(irq, tdx_event_irq_handler, IRQF_NOBALANCING,
>> +                       "tdx_event_irq", NULL)) {
>> +               pr_err("Event notification IRQ request failed\n");
>> +               goto err_free_domain_irqs;
>> +       }
>> +
>> +       cfg = irq_cfg(irq);
>> +
>> +       if (_tdx_hypercall(TDVMCALL_SETUP_NOTIFY_INTR, cfg->vector, 0, 0, 0)) {
>> +               pr_err("Event notification hypercall failed\n");
>> +               goto err_free_irqs;
>> +       }
>> +
>> +       tdx_event_irq = irq;
>> +
>> +       return 0;
>> +
>> +err_free_irqs:
>> +       free_irq(irq, NULL);
>> +err_free_domain_irqs:
>> +       irq_domain_free_irqs(irq, 1);
>> +
>> +       return -EIO;
>> +}
>> +early_initcall(tdx_event_irq_init)
>>
>>
>>
>>
>> -- 
>> Sathyanarayanan Kuppuswamy
>> Linux Kernel Developer
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 1/3] x86/tdx: Add TDX Guest event notify interrupt support

[Huang, Kai]

On Tue, 2023-04-25 at 23:07 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > +/**
> > > + * tdx_event_irq_init() - Register IRQ for event notification from the
> > > VMM to
> > > + *                       the TDX Guest.
> > > + *
> > > + * Use SetupEventNotifyInterrupt TDVMCALL to register the event
> > > notification
> > > + * IRQ with the VMM, which is used by the VMM to notify the TDX guest
> > > when
> > > + * needed, for instance, when VMM finishes the GetQuote request from the
> > > TDX
> > > + * guest. The VMM always notifies the TDX guest via the same CPU on which
> > > the
> > > + * SetupEventNotifyInterrupt TDVMCALL is called. For simplicity, just
> > > allocate
> > > + * an IRQ (and a vector) directly from x86_vector_domain for such
> > > notification
> > > + * and pin the IRQ to the same CPU on which TDVMCALL is called.
> > 
> > I think "for simplicity" applies to allocate IRQ/vector "from BSP using
> > early_initcall()" (so TDVMCALL is easily guaranteed to be called on the same
> > cpu
> > where vector is allocated), but doesn't apply to allocate IRQ/vector from
> > x86_vector_domain and "pin the IRQ to the same CPU on which TDVMCALAL is
> > called".  The latter is something you must do (otherwise you need to
> > allocate
> > the same vector on all cpus), but not something that you do "for
> > simplicity".
> > 
> > > + *
> > > + * Since tdx_event_irq_init() is triggered via early_initcall(), it will
> > > be
> > > + * called before secondary CPUs bring up, so no special logic is required
> > > to
> > > + * ensure that the same CPU is used for SetupEventNotifyInterrupt
> > > TDVMCALL and
> > > + * IRQ allocation.
> > 
> > IMHO the second paragraph is obvious and no need to mention.
> > 
> > As explained above, I guess you just need to at somewhere simply mention
> > something like: "for simplicity use early_initcall() to allocate and pin the
> > IRQ/vector on BSP and also call the TDVMCALL on BSP".  Or probably "also
> > call
> > the TDVMCALL on BSP" can also be omitted as it's kinda already explained in
> > the
> > nature of the TDVMCALL.
> 
> How about the following?
> 
> Use SetupEventNotifyInterrupt TDVMCALL to register the event notification
> IRQ with the VMM, which is used by the VMM to notify the TDX guest when
> needed, for instance, when VMM finishes the GetQuote request from the TDX
> guest. The VMM always notifies the TDX guest via the same CPU that calls the
> SetupEventNotifyInterrupt TDVMCALL. Allocate an IRQ/vector from the
> x86_vector_domain and pin it on the same CPU on which TDVMCALL is called.
> For simplicity, use early_initcall() to allow both IRQ allocation and
> TDVMCALL to use BSP.

Sorry I missed your reply.  OK to me.

[PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Kuppuswamy Sathyanarayanan]

In TDX guest, the second stage in attestation process is to send the
TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
not support communication channels like vsock or TCP/IP, implement
support to get TD Quote using hypercall. GetQuote hypercall can be used
by the TD guest to request VMM facilitate the Quote generation via
QE/QGS. More details about GetQuote hypercall can be found in TDX
Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
titled "TDG.VP.VMCALL<GetQuote>".

Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
submit GetQuote requests from the user space using GetQuote hypercall.

Since GetQuote is an asynchronous request hypercall, VMM will use
callback interrupt vector configured by SetupEventNotifyInterrupt
hypercall to notify the guest about Quote generation completion or
failure. So register an IRQ handler for it.

GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer, allocate the required memory using alloc_pages() and
mark it shared using set_memory_decrypted() in tdx_guest_init(). This
buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
IOCTL handler.

Although this method will reserve a fixed chunk of memory for
GetQuote requests during the init time, it is preferable to the
alternative choice of allocating/freeing the shared buffer in the
TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v1:
 * Removed platform bus device support.
 * Instead of allocating the shared buffers using DMA APIs in IOCTL
   handler, allocated it once in tdx_guest_init() and re-used it in
   GetQuote IOCTL handler.
 * To simplify the design, removed the support for parallel GetQuote
   requests. It can be added when there is a real requirement for it.
 * Fixed commit log and comments to reflect the latest changes.

 Documentation/virt/coco/tdx-guest.rst   |  11 ++
 arch/x86/coco/tdx/tdx.c                 |  40 ++++++
 arch/x86/include/asm/tdx.h              |   2 +
 drivers/virt/coco/tdx-guest/tdx-guest.c | 168 +++++++++++++++++++++++-
 include/uapi/linux/tdx-guest.h          |  43 ++++++
 5 files changed, 263 insertions(+), 1 deletion(-)

diff --git a/Documentation/virt/coco/tdx-guest.rst b/Documentation/virt/coco/tdx-guest.rst
index 46e316db6bb4..54601dcd5864 100644
--- a/Documentation/virt/coco/tdx-guest.rst
+++ b/Documentation/virt/coco/tdx-guest.rst
@@ -42,6 +42,17 @@ ABI. However, in the future, if the TDX Module supports more than one subtype,
 a new IOCTL CMD will be created to handle it. To keep the IOCTL naming
 consistent, a subtype index is added as part of the IOCTL CMD.
 
+2.2 TDX_CMD_GET_QUOTE
+----------------------
+
+:Input parameters: struct tdx_quote_req
+:Output: Return 0 on success, -EIO on TDCALL failure or standard error number
+         on common failures. Upon successful execution, QUOTE data is copied
+         to tdx_quote_req.buf.
+
+The TDX_CMD_GET_QUOTE IOCTL can be used by attestation software to generate
+QUOTE for the given TDREPORT using TDG.VP.VMCALL<GetQuote> hypercall.
+
 Reference
 ---------
 
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 26f6e2eaf5c8..09b5925eec67 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -33,6 +33,7 @@
 #define TDVMCALL_MAP_GPA		0x10001
 #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
 #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
+#define TDVMCALL_GET_QUOTE		0x10002
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
 		__tdx_hypercall(&args, 0);
 }
 
+/**
+ * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
+ *                         hypercall.
+ * @tdquote: Address of the direct mapped shared kernel buffer which
+ * 	     contains TDREPORT data. The same buffer will be used by
+ * 	     VMM to store the generated TD Quote output.
+ * @size: size of the tdquote buffer.
+ *
+ * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
+ * v1.0 specification for more information on GetQuote hypercall.
+ * It is used in the TDX guest driver module to get the TD Quote.
+ *
+ * Return 0 on success or error code on failure.
+ */
+int tdx_hcall_get_quote(u8 *tdquote, size_t size)
+{
+	struct tdx_hypercall_args args = {0};
+
+	/*
+	 * TDX guest driver is the only user of this function and it uses
+	 * the kernel mapped memory. So use virt_to_phys() to get the
+	 * physical address of the TDQuote buffer without any additional
+	 * checks for memory type.
+	 */
+	args.r10 = TDX_HYPERCALL_STANDARD;
+	args.r11 = TDVMCALL_GET_QUOTE;
+	args.r12 = cc_mkdec(virt_to_phys(tdquote));
+	args.r13 = size;
+
+	/*
+	 * Pass the physical address of TDREPORT to the VMM and
+	 * trigger the Quote generation. It is not a blocking
+	 * call, hence completion of this request will be notified to
+	 * the TD guest via a callback interrupt.
+	 */
+	return __tdx_hypercall(&args, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
 static void tdx_parse_tdinfo(u64 *cc_mask)
 {
 	struct tdx_module_output out;
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 8807fe1b1f3f..a72bd7b96564 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -75,6 +75,8 @@ int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
 
 int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
 
+int tdx_hcall_get_quote(u8 *tdquote, size_t size);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 5e44a0fa69bd..a275d6b55f33 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -12,12 +12,105 @@
 #include <linux/mod_devicetable.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
+#include <linux/set_memory.h>
 
 #include <uapi/linux/tdx-guest.h>
 
 #include <asm/cpu_device_id.h>
 #include <asm/tdx.h>
 
+#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
+
+/**
+ * struct quote_entry - Quote request struct
+ * @valid: Flag to check validity of the GetQuote request.
+ * @buf: Kernel buffer to share data with VMM (size is page aligned).
+ * @buf_len: Size of the buf in bytes.
+ * @compl: Completion object to track completion of GetQuote request.
+ */
+struct quote_entry {
+	bool valid;
+	void *buf;
+	size_t buf_len;
+	struct completion compl;
+};
+
+/* Quote data entry */
+static struct quote_entry *qentry;
+
+/* Lock to streamline quote requests */
+static DEFINE_MUTEX(quote_lock);
+
+static int quote_cb_handler(void *dev_id)
+{
+	struct quote_entry *entry = dev_id;
+	struct tdx_quote_hdr *quote_hdr = entry->buf;
+
+	if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
+		complete(&entry->compl);
+
+	return 0;
+}
+
+static void free_shared_pages(void *buf, size_t len)
+{
+	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
+
+	if (!buf)
+		return;
+
+	set_memory_encrypted((unsigned long)buf, count);
+
+	__free_pages(virt_to_page(buf), get_order(len));
+}
+
+static void *alloc_shared_pages(size_t len)
+{
+	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
+	struct page *page;
+	int ret;
+
+	page = alloc_pages(GFP_KERNEL, get_order(len));
+	if (!page)
+		return NULL;
+
+	ret = set_memory_decrypted((unsigned long)page_address(page), count);
+	if (ret) {
+		__free_pages(page, get_order(len));
+		return NULL;
+	}
+
+	return page_address(page);
+}
+
+static struct quote_entry *alloc_quote_entry(size_t len)
+{
+	struct quote_entry *entry = NULL;
+	size_t new_len = PAGE_ALIGN(len);
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return NULL;
+
+	entry->buf = alloc_shared_pages(new_len);
+	if (!entry->buf) {
+		kfree(entry);
+		return NULL;
+	}
+
+	entry->buf_len = new_len;
+	init_completion(&entry->compl);
+	entry->valid = false;
+
+	return entry;
+}
+
+static void free_quote_entry(struct quote_entry *entry)
+{
+	free_shared_pages(entry->buf, entry->buf_len);
+	kfree(entry);
+}
+
 static long tdx_get_report0(struct tdx_report_req __user *req)
 {
 	u8 *reportdata, *tdreport;
@@ -53,12 +146,59 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
 	return ret;
 }
 
+static long tdx_get_quote(struct tdx_quote_req __user *ureq)
+{
+	struct tdx_quote_req req;
+	long ret;
+
+	if (copy_from_user(&req, ureq, sizeof(req)))
+		return -EFAULT;
+
+	mutex_lock(&quote_lock);
+
+	if (!req.len || req.len > qentry->buf_len) {
+		ret = -EINVAL;
+		goto quote_failed;
+	}
+
+	if (copy_from_user(qentry->buf, (void __user *)req.buf, req.len)) {
+		ret = -EFAULT;
+		goto quote_failed;
+	}
+
+	qentry->valid = true;
+
+	reinit_completion(&qentry->compl);
+
+	/* Submit GetQuote Request using GetQuote hypercall */
+	ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
+	if (ret) {
+		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
+		ret = -EIO;
+		goto quote_failed;
+	}
+
+	/* Wait till GetQuote completion */
+	wait_for_completion(&qentry->compl);
+
+	if (copy_to_user((void __user *)req.buf, qentry->buf, req.len))
+		ret = -EFAULT;
+
+quote_failed:
+	qentry->valid = false;
+	mutex_unlock(&quote_lock);
+
+	return ret;
+}
+
 static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
 	switch (cmd) {
 	case TDX_CMD_GET_REPORT0:
 		return tdx_get_report0((struct tdx_report_req __user *)arg);
+	case TDX_CMD_GET_QUOTE:
+		return tdx_get_quote((struct tdx_quote_req *)arg);
 	default:
 		return -ENOTTY;
 	}
@@ -84,15 +224,41 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
 
 static int __init tdx_guest_init(void)
 {
+	int ret;
+
 	if (!x86_match_cpu(tdx_guest_ids))
 		return -ENODEV;
 
-	return misc_register(&tdx_misc_dev);
+	ret = misc_register(&tdx_misc_dev);
+	if (ret)
+		return ret;
+
+	qentry = alloc_quote_entry(GET_QUOTE_MAX_SIZE);
+	if (!qentry) {
+		pr_err("Quote entry allocation failed\n");
+		ret = -ENOMEM;
+		goto free_misc;
+	}
+
+	ret = tdx_register_event_irq_cb(quote_cb_handler, qentry);
+	if (ret)
+		goto free_quote;
+
+	return 0;
+
+free_quote:
+	free_quote_entry(qentry);
+free_misc:
+	misc_deregister(&tdx_misc_dev);
+
+	return ret;
 }
 module_init(tdx_guest_init);
 
 static void __exit tdx_guest_exit(void)
 {
+	tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
+	free_quote_entry(qentry);
 	misc_deregister(&tdx_misc_dev);
 }
 module_exit(tdx_guest_exit);
diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
index a6a2098c08ff..500cdfa025ad 100644
--- a/include/uapi/linux/tdx-guest.h
+++ b/include/uapi/linux/tdx-guest.h
@@ -17,6 +17,12 @@
 /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
 #define TDX_REPORT_LEN                  1024
 
+/* TD Quote status codes */
+#define GET_QUOTE_SUCCESS               0
+#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
+#define GET_QUOTE_ERROR                 0x8000000000000000
+#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
+
 /**
  * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
  *
@@ -30,6 +36,35 @@ struct tdx_report_req {
 	__u8 tdreport[TDX_REPORT_LEN];
 };
 
+/* struct tdx_quote_hdr: Format of Quote request buffer header.
+ * @version: Quote format version, filled by TD.
+ * @status: Status code of Quote request, filled by VMM.
+ * @in_len: Length of TDREPORT, filled by TD.
+ * @out_len: Length of Quote data, filled by VMM.
+ * @data: Quote data on output or TDREPORT on input.
+ *
+ * More details of Quote data header can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_hdr {
+	__u64 version;
+	__u64 status;
+	__u32 in_len;
+	__u32 out_len;
+	__u64 data[];
+};
+
+/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
+ * @buf: Address of user buffer that includes TDREPORT. Upon successful
+ *	 completion of IOCTL, output is copied back to the same buffer.
+ * @len: Length of the Quote buffer.
+ */
+struct tdx_quote_req {
+	__u64 buf;
+	__u64 len;
+};
+
 /*
  * TDX_CMD_GET_REPORT0 - Get TDREPORT0 (a.k.a. TDREPORT subtype 0) using
  *                       TDCALL[TDG.MR.REPORT]
@@ -39,4 +74,12 @@ struct tdx_report_req {
  */
 #define TDX_CMD_GET_REPORT0              _IOWR('T', 1, struct tdx_report_req)
 
+/*
+ * TDX_CMD_GET_QUOTE - Get TD Guest Quote from QE/QGS using GetQuote
+ *		       TDVMCALL.
+ *
+ * Returns 0 on success or standard errno on other failures.
+ */
+#define TDX_CMD_GET_QUOTE		_IOR('T', 2, struct tdx_quote_req)
+
 #endif /* _UAPI_LINUX_TDX_GUEST_H_ */
-- 
2.34.1

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Dionna Amalie Glaze]

On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> In TDX guest, the second stage in attestation process is to send the
> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does

Is it common to state TDREPORT when TDREPORT_STRUCT is meant? Here and below.
The GHCI documentation seems to use the two as synonyms but doesn't
explicitly say so.

nit: platforms that do not

> not support communication channels like vsock or TCP/IP, implement
> support to get TD Quote using hypercall. GetQuote hypercall can be used
> by the TD guest to request VMM facilitate the Quote generation via

nit: request the VMM to facilitate

> QE/QGS. More details about GetQuote hypercall can be found in TDX
> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
> titled "TDG.VP.VMCALL<GetQuote>".
>
> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent

nit: an attestation agent to

> submit GetQuote requests from the user space using GetQuote hypercall.
>
> Since GetQuote is an asynchronous request hypercall, VMM will use
> callback interrupt vector configured by SetupEventNotifyInterrupt

nit: a callback interrupt vector configured by the SetupEventNotifyInterrupt

> hypercall to notify the guest about Quote generation completion or
> failure. So register an IRQ handler for it.
>
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate the required memory using alloc_pages() and
> mark it shared using set_memory_decrypted() in tdx_guest_init(). This
> buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE

suggestion: will be cleared and re-used

> IOCTL handler.
>
> Although this method will reserve a fixed chunk of memory for
> GetQuote requests during the init time, it is preferable to the

The reservation isn't just during the init time. The reservation is
for the lifetime of the driver.

> alternative choice of allocating/freeing the shared buffer in the
> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.
>

Why does allocation during the ioctl damage the direct map and
allocation on init doesn't?
I would suggest rephrasing to say that you're avoiding multiple
bookkeeping round-trips with the VMM for direct map updates.

> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>
> Changes since v1:
>  * Removed platform bus device support.
>  * Instead of allocating the shared buffers using DMA APIs in IOCTL
>    handler, allocated it once in tdx_guest_init() and re-used it in
>    GetQuote IOCTL handler.
>  * To simplify the design, removed the support for parallel GetQuote
>    requests. It can be added when there is a real requirement for it.
>  * Fixed commit log and comments to reflect the latest changes.
>
>  Documentation/virt/coco/tdx-guest.rst   |  11 ++
>  arch/x86/coco/tdx/tdx.c                 |  40 ++++++
>  arch/x86/include/asm/tdx.h              |   2 +
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 168 +++++++++++++++++++++++-
>  include/uapi/linux/tdx-guest.h          |  43 ++++++
>  5 files changed, 263 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/virt/coco/tdx-guest.rst b/Documentation/virt/coco/tdx-guest.rst
> index 46e316db6bb4..54601dcd5864 100644
> --- a/Documentation/virt/coco/tdx-guest.rst
> +++ b/Documentation/virt/coco/tdx-guest.rst
> @@ -42,6 +42,17 @@ ABI. However, in the future, if the TDX Module supports more than one subtype,
>  a new IOCTL CMD will be created to handle it. To keep the IOCTL naming
>  consistent, a subtype index is added as part of the IOCTL CMD.
>
> +2.2 TDX_CMD_GET_QUOTE
> +----------------------
> +
> +:Input parameters: struct tdx_quote_req
> +:Output: Return 0 on success, -EIO on TDCALL failure or standard error number
> +         on common failures. Upon successful execution, QUOTE data is copied
> +         to tdx_quote_req.buf.
> +
> +The TDX_CMD_GET_QUOTE IOCTL can be used by attestation software to generate
> +QUOTE for the given TDREPORT using TDG.VP.VMCALL<GetQuote> hypercall.
> +
>  Reference
>  ---------
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 26f6e2eaf5c8..09b5925eec67 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -33,6 +33,7 @@
>  #define TDVMCALL_MAP_GPA               0x10001
>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>  #define TDVMCALL_SETUP_NOTIFY_INTR     0x10004
> +#define TDVMCALL_GET_QUOTE             0x10002
>
>  /* MMIO direction */
>  #define EPT_READ       0
> @@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
>                 __tdx_hypercall(&args, 0);
>  }
>
> +/**
> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> + *                         hypercall.
> + * @tdquote: Address of the direct mapped shared kernel buffer which
> + *          contains TDREPORT data. The same buffer will be used by
> + *          VMM to store the generated TD Quote output.
> + * @size: size of the tdquote buffer.
> + *
> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> + * v1.0 specification for more information on GetQuote hypercall.
> + * It is used in the TDX guest driver module to get the TD Quote.
> + *
> + * Return 0 on success or error code on failure.
> + */
> +int tdx_hcall_get_quote(u8 *tdquote, size_t size)
> +{
> +       struct tdx_hypercall_args args = {0};
> +
> +       /*
> +        * TDX guest driver is the only user of this function and it uses
> +        * the kernel mapped memory. So use virt_to_phys() to get the
> +        * physical address of the TDQuote buffer without any additional
> +        * checks for memory type.
> +        */
> +       args.r10 = TDX_HYPERCALL_STANDARD;
> +       args.r11 = TDVMCALL_GET_QUOTE;
> +       args.r12 = cc_mkdec(virt_to_phys(tdquote));
> +       args.r13 = size;
> +
> +       /*
> +        * Pass the physical address of TDREPORT to the VMM and
> +        * trigger the Quote generation. It is not a blocking
> +        * call, hence completion of this request will be notified to
> +        * the TD guest via a callback interrupt.
> +        */
> +       return __tdx_hypercall(&args, 0);
> +}
> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> +
>  static void tdx_parse_tdinfo(u64 *cc_mask)
>  {
>         struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 8807fe1b1f3f..a72bd7b96564 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -75,6 +75,8 @@ int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>
>  int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>
> +int tdx_hcall_get_quote(u8 *tdquote, size_t size);
> +
>  #else
>
>  static inline void tdx_early_init(void) { };
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 5e44a0fa69bd..a275d6b55f33 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -12,12 +12,105 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
>
>  #include <uapi/linux/tdx-guest.h>
>
>  #include <asm/cpu_device_id.h>
>  #include <asm/tdx.h>
>
> +#define GET_QUOTE_MAX_SIZE             (4 * PAGE_SIZE)
> +
> +/**
> + * struct quote_entry - Quote request struct
> + * @valid: Flag to check validity of the GetQuote request.
> + * @buf: Kernel buffer to share data with VMM (size is page aligned).
> + * @buf_len: Size of the buf in bytes.
> + * @compl: Completion object to track completion of GetQuote request.
> + */
> +struct quote_entry {
> +       bool valid;
> +       void *buf;
> +       size_t buf_len;
> +       struct completion compl;
> +};
> +
> +/* Quote data entry */
> +static struct quote_entry *qentry;
> +
> +/* Lock to streamline quote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
> +static int quote_cb_handler(void *dev_id)
> +{
> +       struct quote_entry *entry = dev_id;
> +       struct tdx_quote_hdr *quote_hdr = entry->buf;
> +
> +       if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
> +               complete(&entry->compl);
> +
> +       return 0;
> +}
> +
> +static void free_shared_pages(void *buf, size_t len)
> +{
> +       unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
> +
> +       if (!buf)
> +               return;
> +
> +       set_memory_encrypted((unsigned long)buf, count);
> +
> +       __free_pages(virt_to_page(buf), get_order(len));
> +}
> +
> +static void *alloc_shared_pages(size_t len)
> +{
> +       unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
> +       struct page *page;
> +       int ret;
> +
> +       page = alloc_pages(GFP_KERNEL, get_order(len));
> +       if (!page)
> +               return NULL;
> +
> +       ret = set_memory_decrypted((unsigned long)page_address(page), count);
> +       if (ret) {
> +               __free_pages(page, get_order(len));
> +               return NULL;
> +       }
> +
> +       return page_address(page);
> +}
> +
> +static struct quote_entry *alloc_quote_entry(size_t len)
> +{
> +       struct quote_entry *entry = NULL;
> +       size_t new_len = PAGE_ALIGN(len);
> +
> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +       if (!entry)
> +               return NULL;
> +
> +       entry->buf = alloc_shared_pages(new_len);
> +       if (!entry->buf) {
> +               kfree(entry);
> +               return NULL;
> +       }
> +
> +       entry->buf_len = new_len;
> +       init_completion(&entry->compl);
> +       entry->valid = false;
> +
> +       return entry;
> +}
> +
> +static void free_quote_entry(struct quote_entry *entry)
> +{
> +       free_shared_pages(entry->buf, entry->buf_len);
> +       kfree(entry);
> +}
> +
>  static long tdx_get_report0(struct tdx_report_req __user *req)
>  {
>         u8 *reportdata, *tdreport;
> @@ -53,12 +146,59 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>         return ret;
>  }
>
> +static long tdx_get_quote(struct tdx_quote_req __user *ureq)
> +{
> +       struct tdx_quote_req req;
> +       long ret;
> +
> +       if (copy_from_user(&req, ureq, sizeof(req)))
> +               return -EFAULT;
> +
> +       mutex_lock(&quote_lock);
> +
> +       if (!req.len || req.len > qentry->buf_len) {
> +               ret = -EINVAL;
> +               goto quote_failed;
> +       }
> +

Since the qentry is reused across calls, I think you need to clear it
out before repopulating it with maybe less data:

memset(qentry->buf, 0, qentry->buf_len)

I'm not particularly clear on why there is a length though, since the
buffer should always be a TDREPORT_STRUCT. I guess version updates
could expand the size.

> +       if (copy_from_user(qentry->buf, (void __user *)req.buf, req.len)) {
> +               ret = -EFAULT;
> +               goto quote_failed;
> +       }
> +
> +       qentry->valid = true;

Is the valid field used anywhere? I only see it written and never read.

> +
> +       reinit_completion(&qentry->compl);
> +
> +       /* Submit GetQuote Request using GetQuote hypercall */
> +       ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
> +       if (ret) {
> +               pr_err("GetQuote hypercall failed, status:%lx\n", ret);
> +               ret = -EIO;
> +               goto quote_failed;
> +       }
> +
> +       /* Wait till GetQuote completion */
> +       wait_for_completion(&qentry->compl);
> +
> +       if (copy_to_user((void __user *)req.buf, qentry->buf, req.len))
> +               ret = -EFAULT;
> +
> +quote_failed:
> +       qentry->valid = false;
> +       mutex_unlock(&quote_lock);
> +
> +       return ret;
> +}
> +
>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>                             unsigned long arg)
>  {
>         switch (cmd) {
>         case TDX_CMD_GET_REPORT0:
>                 return tdx_get_report0((struct tdx_report_req __user *)arg);
> +       case TDX_CMD_GET_QUOTE:
> +               return tdx_get_quote((struct tdx_quote_req *)arg);
>         default:
>                 return -ENOTTY;
>         }
> @@ -84,15 +224,41 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>
>  static int __init tdx_guest_init(void)
>  {
> +       int ret;
> +
>         if (!x86_match_cpu(tdx_guest_ids))
>                 return -ENODEV;
>
> -       return misc_register(&tdx_misc_dev);
> +       ret = misc_register(&tdx_misc_dev);
> +       if (ret)
> +               return ret;
> +
> +       qentry = alloc_quote_entry(GET_QUOTE_MAX_SIZE);
> +       if (!qentry) {
> +               pr_err("Quote entry allocation failed\n");
> +               ret = -ENOMEM;
> +               goto free_misc;
> +       }
> +
> +       ret = tdx_register_event_irq_cb(quote_cb_handler, qentry);
> +       if (ret)
> +               goto free_quote;
> +
> +       return 0;
> +
> +free_quote:
> +       free_quote_entry(qentry);
> +free_misc:
> +       misc_deregister(&tdx_misc_dev);
> +
> +       return ret;
>  }
>  module_init(tdx_guest_init);
>
>  static void __exit tdx_guest_exit(void)
>  {
> +       tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
> +       free_quote_entry(qentry);
>         misc_deregister(&tdx_misc_dev);
>  }
>  module_exit(tdx_guest_exit);
> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
> index a6a2098c08ff..500cdfa025ad 100644
> --- a/include/uapi/linux/tdx-guest.h
> +++ b/include/uapi/linux/tdx-guest.h
> @@ -17,6 +17,12 @@
>  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>  #define TDX_REPORT_LEN                  1024
>
> +/* TD Quote status codes */
> +#define GET_QUOTE_SUCCESS               0
> +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
> +#define GET_QUOTE_ERROR                 0x8000000000000000
> +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
> +
>  /**
>   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
>   *
> @@ -30,6 +36,35 @@ struct tdx_report_req {
>         __u8 tdreport[TDX_REPORT_LEN];
>  };
>
> +/* struct tdx_quote_hdr: Format of Quote request buffer header.
> + * @version: Quote format version, filled by TD.
> + * @status: Status code of Quote request, filled by VMM.
> + * @in_len: Length of TDREPORT, filled by TD.
> + * @out_len: Length of Quote data, filled by VMM.
> + * @data: Quote data on output or TDREPORT on input.
> + *
> + * More details of Quote data header can be found in TDX
> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> + * section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_hdr {
> +       __u64 version;
> +       __u64 status;
> +       __u32 in_len;
> +       __u32 out_len;
> +       __u64 data[];
> +};
> +
> +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
> + * @buf: Address of user buffer that includes TDREPORT. Upon successful
> + *      completion of IOCTL, output is copied back to the same buffer.
> + * @len: Length of the Quote buffer.
> + */
> +struct tdx_quote_req {
> +       __u64 buf;
> +       __u64 len;
> +};
> +
>  /*
>   * TDX_CMD_GET_REPORT0 - Get TDREPORT0 (a.k.a. TDREPORT subtype 0) using
>   *                       TDCALL[TDG.MR.REPORT]
> @@ -39,4 +74,12 @@ struct tdx_report_req {
>   */
>  #define TDX_CMD_GET_REPORT0              _IOWR('T', 1, struct tdx_report_req)
>
> +/*
> + * TDX_CMD_GET_QUOTE - Get TD Guest Quote from QE/QGS using GetQuote
> + *                    TDVMCALL.
> + *
> + * Returns 0 on success or standard errno on other failures.
> + */
> +#define TDX_CMD_GET_QUOTE              _IOR('T', 2, struct tdx_quote_req)
> +
>  #endif /* _UAPI_LINUX_TDX_GUEST_H_ */
> --
> 2.34.1
>


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi Dionna Amalie Glaze,

Thanks for the review.

On 4/26/23 8:40 AM, Dionna Amalie Glaze wrote:
> On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> In TDX guest, the second stage in attestation process is to send the
>> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
> 
> Is it common to state TDREPORT when TDREPORT_STRUCT is meant? Here and below.
> The GHCI documentation seems to use the two as synonyms but doesn't
> explicitly say so.

In general, we just use the term "TDREPORT" to refer to the generated
report. TDREPORT_STRUCT is mainly used when we want to make reference to
the structure of the TDREPORT in the GHCI specification (very rarely used).
Also, as you mentioned, even in GHCI specification, both terms are used
as synonyms.

> 
> nit: platforms that do not
> 
>> not support communication channels like vsock or TCP/IP, implement
>> support to get TD Quote using hypercall. GetQuote hypercall can be used
>> by the TD guest to request VMM facilitate the Quote generation via
> 
> nit: request the VMM to facilitate
> 
>> QE/QGS. More details about GetQuote hypercall can be found in TDX
>> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
>> titled "TDG.VP.VMCALL<GetQuote>".
>>
>> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
> 
> nit: an attestation agent to
> 
>> submit GetQuote requests from the user space using GetQuote hypercall.
>>
>> Since GetQuote is an asynchronous request hypercall, VMM will use
>> callback interrupt vector configured by SetupEventNotifyInterrupt
> 
> nit: a callback interrupt vector configured by the SetupEventNotifyInterrupt
> 
>> hypercall to notify the guest about Quote generation completion or
>> failure. So register an IRQ handler for it.
>>
>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>> with TDREPORT data as input, which is further used by the VMM to copy
>> the TD Quote result after successful Quote generation. To create the
>> shared buffer, allocate the required memory using alloc_pages() and
>> mark it shared using set_memory_decrypted() in tdx_guest_init(). This
>> buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
> 
> suggestion: will be cleared and re-used
> 
>> IOCTL handler.
>>
>> Although this method will reserve a fixed chunk of memory for
>> GetQuote requests during the init time, it is preferable to the
> 
> The reservation isn't just during the init time. The reservation is
> for the lifetime of the driver.

I think I can remove the "during the init time" part.

> 
>> alternative choice of allocating/freeing the shared buffer in the
>> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.
>>
> 
> Why does allocation during the ioctl damage the direct map and
> allocation on init doesn't?> I would suggest rephrasing to say that you're avoiding multiple
> bookkeeping round-trips with the VMM for direct map updates.

How about the following?

Although this method reserves a fixed chunk of memory for GetQuote
requests, such one-time allocation is preferable to the alternative
choice of repeatedly allocating/freeing the shared buffer in the
TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map (because
the sharing/unsharing process modifies the direct map). This allocation
model is similar to that used by the AMD SEV guest driver.


> 
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>
>> Changes since v1:
>>  * Removed platform bus device support.
>>  * Instead of allocating the shared buffers using DMA APIs in IOCTL
>>    handler, allocated it once in tdx_guest_init() and re-used it in
>>    GetQuote IOCTL handler.
>>  * To simplify the design, removed the support for parallel GetQuote
>>    requests. It can be added when there is a real requirement for it.
>>  * Fixed commit log and comments to reflect the latest changes.
>>
>>  Documentation/virt/coco/tdx-guest.rst   |  11 ++
>>  arch/x86/coco/tdx/tdx.c                 |  40 ++++++
>>  arch/x86/include/asm/tdx.h              |   2 +
>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 168 +++++++++++++++++++++++-
>>  include/uapi/linux/tdx-guest.h          |  43 ++++++
>>  5 files changed, 263 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virt/coco/tdx-guest.rst b/Documentation/virt/coco/tdx-guest.rst
>> index 46e316db6bb4..54601dcd5864 100644
>> --- a/Documentation/virt/coco/tdx-guest.rst
>> +++ b/Documentation/virt/coco/tdx-guest.rst
>> @@ -42,6 +42,17 @@ ABI. However, in the future, if the TDX Module supports more than one subtype,
>>  a new IOCTL CMD will be created to handle it. To keep the IOCTL naming
>>  consistent, a subtype index is added as part of the IOCTL CMD.
>>
>> +2.2 TDX_CMD_GET_QUOTE
>> +----------------------
>> +
>> +:Input parameters: struct tdx_quote_req
>> +:Output: Return 0 on success, -EIO on TDCALL failure or standard error number
>> +         on common failures. Upon successful execution, QUOTE data is copied
>> +         to tdx_quote_req.buf.
>> +
>> +The TDX_CMD_GET_QUOTE IOCTL can be used by attestation software to generate
>> +QUOTE for the given TDREPORT using TDG.VP.VMCALL<GetQuote> hypercall.
>> +
>>  Reference
>>  ---------
>>
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 26f6e2eaf5c8..09b5925eec67 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -33,6 +33,7 @@
>>  #define TDVMCALL_MAP_GPA               0x10001
>>  #define TDVMCALL_REPORT_FATAL_ERROR    0x10003
>>  #define TDVMCALL_SETUP_NOTIFY_INTR     0x10004
>> +#define TDVMCALL_GET_QUOTE             0x10002
>>
>>  /* MMIO direction */
>>  #define EPT_READ       0
>> @@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
>>                 __tdx_hypercall(&args, 0);
>>  }
>>
>> +/**
>> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
>> + *                         hypercall.
>> + * @tdquote: Address of the direct mapped shared kernel buffer which
>> + *          contains TDREPORT data. The same buffer will be used by
>> + *          VMM to store the generated TD Quote output.
>> + * @size: size of the tdquote buffer.
>> + *
>> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
>> + * v1.0 specification for more information on GetQuote hypercall.
>> + * It is used in the TDX guest driver module to get the TD Quote.
>> + *
>> + * Return 0 on success or error code on failure.
>> + */
>> +int tdx_hcall_get_quote(u8 *tdquote, size_t size)
>> +{
>> +       struct tdx_hypercall_args args = {0};
>> +
>> +       /*
>> +        * TDX guest driver is the only user of this function and it uses
>> +        * the kernel mapped memory. So use virt_to_phys() to get the
>> +        * physical address of the TDQuote buffer without any additional
>> +        * checks for memory type.
>> +        */
>> +       args.r10 = TDX_HYPERCALL_STANDARD;
>> +       args.r11 = TDVMCALL_GET_QUOTE;
>> +       args.r12 = cc_mkdec(virt_to_phys(tdquote));
>> +       args.r13 = size;
>> +
>> +       /*
>> +        * Pass the physical address of TDREPORT to the VMM and
>> +        * trigger the Quote generation. It is not a blocking
>> +        * call, hence completion of this request will be notified to
>> +        * the TD guest via a callback interrupt.
>> +        */
>> +       return __tdx_hypercall(&args, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>> +
>>  static void tdx_parse_tdinfo(u64 *cc_mask)
>>  {
>>         struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 8807fe1b1f3f..a72bd7b96564 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -75,6 +75,8 @@ int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>>
>>  int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>>
>> +int tdx_hcall_get_quote(u8 *tdquote, size_t size);
>> +
>>  #else
>>
>>  static inline void tdx_early_init(void) { };
>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> index 5e44a0fa69bd..a275d6b55f33 100644
>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> @@ -12,12 +12,105 @@
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/string.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/set_memory.h>
>>
>>  #include <uapi/linux/tdx-guest.h>
>>
>>  #include <asm/cpu_device_id.h>
>>  #include <asm/tdx.h>
>>
>> +#define GET_QUOTE_MAX_SIZE             (4 * PAGE_SIZE)
>> +
>> +/**
>> + * struct quote_entry - Quote request struct
>> + * @valid: Flag to check validity of the GetQuote request.
>> + * @buf: Kernel buffer to share data with VMM (size is page aligned).
>> + * @buf_len: Size of the buf in bytes.
>> + * @compl: Completion object to track completion of GetQuote request.
>> + */
>> +struct quote_entry {
>> +       bool valid;
>> +       void *buf;
>> +       size_t buf_len;
>> +       struct completion compl;
>> +};
>> +
>> +/* Quote data entry */
>> +static struct quote_entry *qentry;
>> +
>> +/* Lock to streamline quote requests */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>> +static int quote_cb_handler(void *dev_id)
>> +{
>> +       struct quote_entry *entry = dev_id;
>> +       struct tdx_quote_hdr *quote_hdr = entry->buf;
>> +
>> +       if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
>> +               complete(&entry->compl);
>> +
>> +       return 0;
>> +}
>> +
>> +static void free_shared_pages(void *buf, size_t len)
>> +{
>> +       unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
>> +
>> +       if (!buf)
>> +               return;
>> +
>> +       set_memory_encrypted((unsigned long)buf, count);
>> +
>> +       __free_pages(virt_to_page(buf), get_order(len));
>> +}
>> +
>> +static void *alloc_shared_pages(size_t len)
>> +{
>> +       unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
>> +       struct page *page;
>> +       int ret;
>> +
>> +       page = alloc_pages(GFP_KERNEL, get_order(len));
>> +       if (!page)
>> +               return NULL;
>> +
>> +       ret = set_memory_decrypted((unsigned long)page_address(page), count);
>> +       if (ret) {
>> +               __free_pages(page, get_order(len));
>> +               return NULL;
>> +       }
>> +
>> +       return page_address(page);
>> +}
>> +
>> +static struct quote_entry *alloc_quote_entry(size_t len)
>> +{
>> +       struct quote_entry *entry = NULL;
>> +       size_t new_len = PAGE_ALIGN(len);
>> +
>> +       entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +       if (!entry)
>> +               return NULL;
>> +
>> +       entry->buf = alloc_shared_pages(new_len);
>> +       if (!entry->buf) {
>> +               kfree(entry);
>> +               return NULL;
>> +       }
>> +
>> +       entry->buf_len = new_len;
>> +       init_completion(&entry->compl);
>> +       entry->valid = false;
>> +
>> +       return entry;
>> +}
>> +
>> +static void free_quote_entry(struct quote_entry *entry)
>> +{
>> +       free_shared_pages(entry->buf, entry->buf_len);
>> +       kfree(entry);
>> +}
>> +
>>  static long tdx_get_report0(struct tdx_report_req __user *req)
>>  {
>>         u8 *reportdata, *tdreport;
>> @@ -53,12 +146,59 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>>         return ret;
>>  }
>>
>> +static long tdx_get_quote(struct tdx_quote_req __user *ureq)
>> +{
>> +       struct tdx_quote_req req;
>> +       long ret;
>> +
>> +       if (copy_from_user(&req, ureq, sizeof(req)))
>> +               return -EFAULT;
>> +
>> +       mutex_lock(&quote_lock);
>> +
>> +       if (!req.len || req.len > qentry->buf_len) {
>> +               ret = -EINVAL;
>> +               goto quote_failed;
>> +       }
>> +
> 
> Since the qentry is reused across calls, I think you need to clear it
> out before repopulating it with maybe less data:
> 
> memset(qentry->buf, 0, qentry->buf_len)

Good point. I will fix it.

> 
> I'm not particularly clear on why there is a length though, since the
> buffer should always be a TDREPORT_STRUCT. I guess version updates
> could expand the size.

Since ABI cannot be changed, added it to handle future changes.

> 
>> +       if (copy_from_user(qentry->buf, (void __user *)req.buf, req.len)) {
>> +               ret = -EFAULT;
>> +               goto quote_failed;
>> +       }
>> +
>> +       qentry->valid = true;
> 
> Is the valid field used anywhere? I only see it written and never read.

It is used in quote_cb_handler().



-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Dionna Amalie Glaze]

>
> How about the following?
>
> Although this method reserves a fixed chunk of memory for GetQuote
> requests, such one-time allocation is preferable to the alternative
> choice of repeatedly allocating/freeing the shared buffer in the
> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map (because
> the sharing/unsharing process modifies the direct map). This allocation
> model is similar to that used by the AMD SEV guest driver.
>
>

Yes, this is clear, thank you.


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Huang, Kai]

On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, the second stage in attestation process is to send the
> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
> not support communication channels like vsock or TCP/IP, implement
> support to get TD Quote using hypercall. GetQuote hypercall can be used
> by the TD guest to request VMM facilitate the Quote generation via
> QE/QGS. More details about GetQuote hypercall can be found in TDX
> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
> titled "TDG.VP.VMCALL<GetQuote>".

When this patch gets merged, the patch to get the TDREPORT would be long before
this patch.  To help the git blamers to understand more easily, I think it's
better to provide some background here.

FYI below:

"
In TDX attestation, the TDREPORT of a TDX guest contains information to uniquely
identify the TDX guest along with the TEE environment of the local machine. 
TDREPORT is integrity-protected and can only be verified on the local machine.  

To support TDX remote attestation, in SGX-based attestation, after the TDX guest
gets the TDREPORT from the TDX module, the TDREPORT needs to be sent to the SGX
Quoting Enclave (QE) to convert it to a remote verifiable Quote.

SGX QE can only run outside of the TDX guest (i.e. in a host process or in a
normal VM).  For security concern the TDX guest may not support normal
communication channels like vsock or TCP/IP.  To support remote attestation for
such case, TDX uses GetQuote TDVMCALL to ask the host VMM to communicate with
the SGX QE.  More details about GetQuote TDVMCALL can be found in ...
"

> 
> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
> submit GetQuote requests from the user space using GetQuote hypercall.
> 
> Since GetQuote is an asynchronous request hypercall, VMM will use
> callback interrupt vector configured by SetupEventNotifyInterrupt
> hypercall to notify the guest about Quote generation completion or
> failure. So register an IRQ handler for it.
> 
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate the required memory using alloc_pages() and
> mark it shared using set_memory_decrypted() in tdx_guest_init(). 
> 

Personally I think you don't need to mention "using alloc_pages() ... 
set_memory_decrypted()" staff.  They belong to details and the code can tell.

> This
> buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
> IOCTL handler.

Besides the "re-used" part, I think it's better to explain the rational of
choosing a fixed 16K (4 pages) shared buffer.  For instance, in practice in
Intel's SGX QE implementation a Quote is less than 8K, and 16K should be big
enough in the foreseeable future even considering 3rd party's own
implementation.

Also, I guess it's better to call out somewhere currently we don't support
multiple GetQuote in parallel because of <xxx>, so allocating a single shared
buffer at early time is OK.

> 
> Although this method will reserve a fixed chunk of memory for
> GetQuote requests during the init time, it is preferable to the
> alternative choice of allocating/freeing the shared buffer in the
> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> Changes since v1:
>  * Removed platform bus device support.
>  * Instead of allocating the shared buffers using DMA APIs in IOCTL
>    handler, allocated it once in tdx_guest_init() and re-used it in
>    GetQuote IOCTL handler.
>  * To simplify the design, removed the support for parallel GetQuote
>    requests. It can be added when there is a real requirement for it.
>  * Fixed commit log and comments to reflect the latest changes.
> 
>  Documentation/virt/coco/tdx-guest.rst   |  11 ++
>  arch/x86/coco/tdx/tdx.c                 |  40 ++++++
>  arch/x86/include/asm/tdx.h              |   2 +
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 168 +++++++++++++++++++++++-
>  include/uapi/linux/tdx-guest.h          |  43 ++++++
>  5 files changed, 263 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/virt/coco/tdx-guest.rst b/Documentation/virt/coco/tdx-guest.rst
> index 46e316db6bb4..54601dcd5864 100644
> --- a/Documentation/virt/coco/tdx-guest.rst
> +++ b/Documentation/virt/coco/tdx-guest.rst
> @@ -42,6 +42,17 @@ ABI. However, in the future, if the TDX Module supports more than one subtype,
>  a new IOCTL CMD will be created to handle it. To keep the IOCTL naming
>  consistent, a subtype index is added as part of the IOCTL CMD.
>  
> +2.2 TDX_CMD_GET_QUOTE
> +----------------------
> +
> +:Input parameters: struct tdx_quote_req
> +:Output: Return 0 on success, -EIO on TDCALL failure or standard error number
> +         on common failures. Upon successful execution, QUOTE data is copied
> +         to tdx_quote_req.buf.
> +
> +The TDX_CMD_GET_QUOTE IOCTL can be used by attestation software to generate
> +QUOTE for the given TDREPORT using TDG.VP.VMCALL<GetQuote> hypercall.

Here QUOTE is used, but I see 'Quote' is used in changelog and the code as well,
perhaps we need to make sure the consistency.

> +
>  Reference
>  ---------
>  
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 26f6e2eaf5c8..09b5925eec67 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -33,6 +33,7 @@
>  #define TDVMCALL_MAP_GPA		0x10001
>  #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
>  #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
> +#define TDVMCALL_GET_QUOTE		0x10002
>  
>  /* MMIO direction */
>  #define EPT_READ	0
> @@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
>  		__tdx_hypercall(&args, 0);
>  }
>  
> +/**
> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> + *                         hypercall.
> + * @tdquote: Address of the direct mapped shared kernel buffer which
> + * 	     contains TDREPORT data. The same buffer will be used by
> + * 	     VMM to store the generated TD Quote output.
> + * @size: size of the tdquote buffer.
> + *
> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> + * v1.0 specification for more information on GetQuote hypercall.
> + * It is used in the TDX guest driver module to get the TD Quote.
> + *
> + * Return 0 on success or error code on failure.
> + */
> +int tdx_hcall_get_quote(u8 *tdquote, size_t size)
> +{
> +	struct tdx_hypercall_args args = {0};
> +
> +	/*
> +	 * TDX guest driver is the only user of this function and it uses
> +	 * the kernel mapped memory. So use virt_to_phys() to get the
> +	 * physical address of the TDQuote buffer without any additional
> +	 * checks for memory type.
> +	 */

How about just call out this function requires the buffer must be shared in kdoc
style comment above this function?  We should just focus on what this function
is doing.

> +	args.r10 = TDX_HYPERCALL_STANDARD;
> +	args.r11 = TDVMCALL_GET_QUOTE;
> +	args.r12 = cc_mkdec(virt_to_phys(tdquote));
> +	args.r13 = size;
> +
> +	/*
> +	 * Pass the physical address of TDREPORT to the VMM and
> +	 * trigger the Quote generation. It is not a blocking
> +	 * call, hence completion of this request will be notified to
> +	 * the TD guest via a callback interrupt.
> +	 */
> +	return __tdx_hypercall(&args, 0);
> +}
> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> +
>  static void tdx_parse_tdinfo(u64 *cc_mask)
>  {
>  	struct tdx_module_output out;
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 8807fe1b1f3f..a72bd7b96564 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -75,6 +75,8 @@ int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>  
>  int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>  
> +int tdx_hcall_get_quote(u8 *tdquote, size_t size);
> +
>  #else
>  
>  static inline void tdx_early_init(void) { };
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 5e44a0fa69bd..a275d6b55f33 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -12,12 +12,105 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/string.h>
>  #include <linux/uaccess.h>
> +#include <linux/set_memory.h>
>  
>  #include <uapi/linux/tdx-guest.h>
>  
>  #include <asm/cpu_device_id.h>
>  #include <asm/tdx.h>
>  
> +#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)

A comment to explain why this particular size is chosen would be helpful.

> +
> +/**
> + * struct quote_entry - Quote request struct
> + * @valid: Flag to check validity of the GetQuote request.
> + * @buf: Kernel buffer to share data with VMM (size is page aligned).
> + * @buf_len: Size of the buf in bytes.
> + * @compl: Completion object to track completion of GetQuote request.
> + */
> +struct quote_entry {
> +	bool valid;
> +	void *buf;
> +	size_t buf_len;
> +	struct completion compl;
> +};

We have a static global @qentry below.

The buffer size is a fixed size (16K), why do we need @buf_len here?

And why do we need @valid?  It seems ...

> +
> +/* Quote data entry */
> +static struct quote_entry *qentry;
> +
> +/* Lock to streamline quote requests */
> +static DEFINE_MUTEX(quote_lock);
> +
> +static int quote_cb_handler(void *dev_id)
> +{
> +	struct quote_entry *entry = dev_id;
> +	struct tdx_quote_hdr *quote_hdr = entry->buf;
> +
> +	if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
> +		complete(&entry->compl);

... this handler is only called when we have received the notification from the
VMM, so the VMM must have already put something into the buffer, meaning the
buffer is already valid.

Could you explain why do we need @valid?

That being said, to me I found the 'struct quote_entry' itself is quite
unnecessary.  It looks like a leftover that you didn't remove when changing from
supporting multiple GetQuote requests in parallel to only supporting one request
at one time.

> +
> +	return 0;
> +}
> +
> +static void free_shared_pages(void *buf, size_t len)
> +{
> +	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
> +
> +	if (!buf)
> +		return;

Looks like a NULL @buf cannot happen.  Use WARN_ON_ONCE()?

> +
> +	set_memory_encrypted((unsigned long)buf, count);
> +
> +	__free_pages(virt_to_page(buf), get_order(len));
> +}
> +
> +static void *alloc_shared_pages(size_t len)
> +{
> +	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
> +	struct page *page;
> +	int ret;
> +
> +	page = alloc_pages(GFP_KERNEL, get_order(len));
> +	if (!page)
> +		return NULL;
> +
> +	ret = set_memory_decrypted((unsigned long)page_address(page), count);
> +	if (ret) {
> +		__free_pages(page, get_order(len));
> +		return NULL;
> +	}
> +
> +	return page_address(page);
> +}

I think you can use alloc_pages_exact():

1) For 4 pages it doesn't matter, but in case in the future we want a non-power-
of-2 size alloc_pages_exact() can save some memory.

2) It gives you a kernel virtual address directly, so you can save the
page_address() call above.

> +
> +static struct quote_entry *alloc_quote_entry(size_t len)
> +{
> +	struct quote_entry *entry = NULL;
> +	size_t new_len = PAGE_ALIGN(len);

I don't think you need @new_len, because anyway alloc_shared_pages() will
allocate enough space if you just pass @len?

> +
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return NULL;
> +
> +	entry->buf = alloc_shared_pages(new_len);
> +	if (!entry->buf) {
> +		kfree(entry);
> +		return NULL;
> +	}
> +
> +	entry->buf_len = new_len;
> +	init_completion(&entry->compl);
> +	entry->valid = false;
> +
> +	return entry;
> +}
> +
> +static void free_quote_entry(struct quote_entry *entry)
> +{
> +	free_shared_pages(entry->buf, entry->buf_len);
> +	kfree(entry);
> +}
> +
>  static long tdx_get_report0(struct tdx_report_req __user *req)
>  {
>  	u8 *reportdata, *tdreport;
> @@ -53,12 +146,59 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>  	return ret;
>  }
>  
> +static long tdx_get_quote(struct tdx_quote_req __user *ureq)
> +{
> +	struct tdx_quote_req req;
> +	long ret;
> +
> +	if (copy_from_user(&req, ureq, sizeof(req)))
> +		return -EFAULT;
> +
> +	mutex_lock(&quote_lock);
> +
> +	if (!req.len || req.len > qentry->buf_len) {
> +		ret = -EINVAL;
> +		goto quote_failed;
> +	}
> +
> +	if (copy_from_user(qentry->buf, (void __user *)req.buf, req.len)) {
> +		ret = -EFAULT;
> +		goto quote_failed;
> +	}
> +
> +	qentry->valid = true;
> +
> +	reinit_completion(&qentry->compl);
> +
> +	/* Submit GetQuote Request using GetQuote hypercall */
> +	ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
> +	if (ret) {
> +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
> +		ret = -EIO;
> +		goto quote_failed;
> +	}
> +
> +	/* Wait till GetQuote completion */
> +	wait_for_completion(&qentry->compl);

Non-killable wait w/o timeout worries me a little bit, because it can wait
forever if VMM also couldn't get the Quote for whatever reason  and doesn't have
it's own timeout.  Unfortunately the GHCI doesn't put any requirement to the VMM
on this, so we kinda depend on the VMM.

Perhaps for now it's OK to have this simple implementation, but looks we should
at least call out the risk in the comment.

> +
> +	if (copy_to_user((void __user *)req.buf, qentry->buf, req.len))
> +		ret = -EFAULT;
> +
> +quote_failed:
> +	qentry->valid = false;
> +	mutex_unlock(&quote_lock);
> +
> +	return ret;
> +}
> +
>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>  			    unsigned long arg)
>  {
>  	switch (cmd) {
>  	case TDX_CMD_GET_REPORT0:
>  		return tdx_get_report0((struct tdx_report_req __user *)arg);
> +	case TDX_CMD_GET_QUOTE:
> +		return tdx_get_quote((struct tdx_quote_req *)arg);
>  	default:
>  		return -ENOTTY;
>  	}
> @@ -84,15 +224,41 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>  
>  static int __init tdx_guest_init(void)
>  {
> +	int ret;
> +
>  	if (!x86_match_cpu(tdx_guest_ids))
>  		return -ENODEV;
>  
> -	return misc_register(&tdx_misc_dev);
> +	ret = misc_register(&tdx_misc_dev);
> +	if (ret)
> +		return ret;
> +
> +	qentry = alloc_quote_entry(GET_QUOTE_MAX_SIZE);
> +	if (!qentry) {
> +		pr_err("Quote entry allocation failed\n");

This is a rather confusing message from user's perspective.  The result of this
error isn't clear from  this message.  I think we should have clear message ...

> +		ret = -ENOMEM;
> +		goto free_misc;
> +	}
> +
> +	ret = tdx_register_event_irq_cb(quote_cb_handler, qentry);
> +	if (ret)
> +		goto free_quote;
> +
> +	return 0;
> +
> +free_quote:
> +	free_quote_entry(qentry);
> +free_misc:
> +	misc_deregister(&tdx_misc_dev);

... here saying something like "Attestation is not available" so user can be
clear about this.

> +
> +	return ret;
>  }
>  module_init(tdx_guest_init);
>  
>  static void __exit tdx_guest_exit(void)
>  {
> +	tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
> +	free_quote_entry(qentry);
>  	misc_deregister(&tdx_misc_dev);
>  }
>  module_exit(tdx_guest_exit);
> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
> index a6a2098c08ff..500cdfa025ad 100644
> --- a/include/uapi/linux/tdx-guest.h
> +++ b/include/uapi/linux/tdx-guest.h
> @@ -17,6 +17,12 @@
>  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>  #define TDX_REPORT_LEN                  1024
>  
> +/* TD Quote status codes */
> +#define GET_QUOTE_SUCCESS               0
> +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
> +#define GET_QUOTE_ERROR                 0x8000000000000000
> +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
> +
>  /**
>   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
>   *
> @@ -30,6 +36,35 @@ struct tdx_report_req {
>  	__u8 tdreport[TDX_REPORT_LEN];
>  };
>  
> +/* struct tdx_quote_hdr: Format of Quote request buffer header.
> + * @version: Quote format version, filled by TD.
> + * @status: Status code of Quote request, filled by VMM.
> + * @in_len: Length of TDREPORT, filled by TD.
> + * @out_len: Length of Quote data, filled by VMM.
> + * @data: Quote data on output or TDREPORT on input.
> + *
> + * More details of Quote data header can be found in TDX
> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> + * section titled "TDG.VP.VMCALL<GetQuote>"
> + */
> +struct tdx_quote_hdr {
> +	__u64 version;
> +	__u64 status;
> +	__u32 in_len;
> +	__u32 out_len;
> +	__u64 data[];
> +};

This structure is weird.  It's a header, but it contains the dynamic-size
buffer.  If you have __data[] in this structure, then it is already a buffer for
the entire Quote, no?  Then should we just call it 'struct tdx_quote'?

Or do you want to remove __data[]?

> +
> +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
> + * @buf: Address of user buffer that includes TDREPORT. Upon successful
> + *	 completion of IOCTL, output is copied back to the same buffer.

This description isn't precise.  "user buffer that includes TDREPORT" doesn't
tell application writer where to put the TDREPORT at all.  We need to explicitly
call out the buffer starts with 'tdx_quote_hdr' followed by TDREPORT
immediately.
 
> + * @len: Length of the Quote buffer.
> + */
> +struct tdx_quote_req {
> +	__u64 buf;
> +	__u64 len;
> +};
> +
>  /*
>   * TDX_CMD_GET_REPORT0 - Get TDREPORT0 (a.k.a. TDREPORT subtype 0) using
>   *                       TDCALL[TDG.MR.REPORT]
> @@ -39,4 +74,12 @@ struct tdx_report_req {
>   */
>  #define TDX_CMD_GET_REPORT0              _IOWR('T', 1, struct tdx_report_req)
>  
> +/*
> + * TDX_CMD_GET_QUOTE - Get TD Guest Quote from QE/QGS using GetQuote
> + *		       TDVMCALL.
> + *
> + * Returns 0 on success or standard errno on other failures.
> + */
> +#define TDX_CMD_GET_QUOTE		_IOR('T', 2, struct tdx_quote_req)
> +
>  #endif /* _UAPI_LINUX_TDX_GUEST_H_ */

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 4/28/23 6:49 AM, Huang, Kai wrote:
> On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, the second stage in attestation process is to send the
>> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
>> not support communication channels like vsock or TCP/IP, implement
>> support to get TD Quote using hypercall. GetQuote hypercall can be used
>> by the TD guest to request VMM facilitate the Quote generation via
>> QE/QGS. More details about GetQuote hypercall can be found in TDX
>> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
>> titled "TDG.VP.VMCALL<GetQuote>".
> 
> When this patch gets merged, the patch to get the TDREPORT would be long before
> this patch.  To help the git blamers to understand more easily, I think it's
> better to provide some background here.
> 
> FYI below:
> 
> "
> In TDX attestation, the TDREPORT of a TDX guest contains information to uniquely
> identify the TDX guest along with the TEE environment of the local machine. 
> TDREPORT is integrity-protected and can only be verified on the local machine.  
> 
> To support TDX remote attestation, in SGX-based attestation, after the TDX guest
> gets the TDREPORT from the TDX module, the TDREPORT needs to be sent to the SGX
> Quoting Enclave (QE) to convert it to a remote verifiable Quote.
> 
> SGX QE can only run outside of the TDX guest (i.e. in a host process or in a
> normal VM).  For security concern the TDX guest may not support normal
> communication channels like vsock or TCP/IP.  To support remote attestation for
> such case, TDX uses GetQuote TDVMCALL to ask the host VMM to communicate with
> the SGX QE.  More details about GetQuote TDVMCALL can be found in ...
> "
> 

Ok. I will add some introduction to the commit log.

>>
>> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
>> submit GetQuote requests from the user space using GetQuote hypercall.
>>
>> Since GetQuote is an asynchronous request hypercall, VMM will use
>> callback interrupt vector configured by SetupEventNotifyInterrupt
>> hypercall to notify the guest about Quote generation completion or
>> failure. So register an IRQ handler for it.
>>
>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>> with TDREPORT data as input, which is further used by the VMM to copy
>> the TD Quote result after successful Quote generation. To create the
>> shared buffer, allocate the required memory using alloc_pages() and
>> mark it shared using set_memory_decrypted() in tdx_guest_init(). 
>>
> 
> Personally I think you don't need to mention "using alloc_pages() ... 
> set_memory_decrypted()" staff.  They belong to details and the code can tell.
> 
>> This
>> buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
>> IOCTL handler.
> 
> Besides the "re-used" part, I think it's better to explain the rational of
> choosing a fixed 16K (4 pages) shared buffer.  For instance, in practice in
> Intel's SGX QE implementation a Quote is less than 8K, and 16K should be big
> enough in the foreseeable future even considering 3rd party's own
> implementation.

I will add it part of the comment in the code.

> 
> Also, I guess it's better to call out somewhere currently we don't support
> multiple GetQuote in parallel because of <xxx>, so allocating a single shared
> buffer at early time is OK.


> 
>>
>> Although this method will reserve a fixed chunk of memory for
>> GetQuote requests during the init time, it is preferable to the
>> alternative choice of allocating/freeing the shared buffer in the
>> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.
>>

Updated commit log looks like below:

    In TDX guest, the attestation process is used to verify the TDX guest
    trustworthiness to other entities before provisioning secrets to the
    guest. The First step in the attestation process is TDREPORT
    generation, which involves getting the guest measurement data in the
    format of TDREPORT, which is further used to validate the authenticity
    of the TDX guest. TDREPORT by design is integrity-protected and can
    only be verified on the local machine.
    
    To support remote verification of the TDREPORT (in a SGX-based
    attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
    (QE) to convert it to a remote verifiable Quote. SGX QE by design can
    only run outside of the TDX guest (i.e. in a host process or in a
    normal VM) and the guest can use communication channels like vsock or
    TCP/IP to send the TDREPORT to the QE. But for security concerns, some
    platforms may not support these communication channels. To handle such
    cases, TDX defines a GetQuote hypercall which can be used by the guest
    to request the host VMM to communicate with the SGX QE. More details
    about GetQuote hypercall can be found in TDX Guest-Host Communication
    Interface (GHCI) for Intel TDX 1.0, section titled
    "TDG.VP.VMCALL<GetQuote>".
    
    Add support for TDX_CMD_GET_QUOTE IOCTL to allow an attestation agent
    to submit GetQuote requests from the user space using GetQuote
    hypercall.
    
    Since GetQuote is an asynchronous request hypercall, VMM will use the
    callback interrupt vector configured by the SetupEventNotifyInterrupt
    hypercall to notify the guest about Quote generation completion or
    failure. So register an IRQ handler for it.
    
    GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
    with TDREPORT data as input, which is further used by the VMM to copy
    the TD Quote result after successful Quote generation. Allocate the
    required shared memory once in tdx_guest_init() and reuse it in the
    TDX_CMD_GET_QUOTE IOCTL handler for GetQuote requests.
    
    Although this method reserves a fixed chunk of memory for GetQuote
    requests, such one-time allocation is preferable to the alternative
    choice of repeatedly allocating/freeing the shared buffer in the
    TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map
    (because the sharing/unsharing process modifies the direct map). This
    allocation model is similar to that used by the AMD SEV guest driver.
    
    Since the Quote generation process is not time-critical or frequently
    used, the current version do not support parallel GetQuote requests.



>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>
>> Changes since v1:
>>  * Removed platform bus device support.
>>  * Instead of allocating the shared buffers using DMA APIs in IOCTL
>>    handler, allocated it once in tdx_guest_init() and re-used it in
>>    GetQuote IOCTL handler.
>>  * To simplify the design, removed the support for parallel GetQuote
>>    requests. It can be added when there is a real requirement for it.
>>  * Fixed commit log and comments to reflect the latest changes.
>>
>>  Documentation/virt/coco/tdx-guest.rst   |  11 ++
>>  arch/x86/coco/tdx/tdx.c                 |  40 ++++++
>>  arch/x86/include/asm/tdx.h              |   2 +
>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 168 +++++++++++++++++++++++-
>>  include/uapi/linux/tdx-guest.h          |  43 ++++++
>>  5 files changed, 263 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virt/coco/tdx-guest.rst b/Documentation/virt/coco/tdx-guest.rst
>> index 46e316db6bb4..54601dcd5864 100644
>> --- a/Documentation/virt/coco/tdx-guest.rst
>> +++ b/Documentation/virt/coco/tdx-guest.rst
>> @@ -42,6 +42,17 @@ ABI. However, in the future, if the TDX Module supports more than one subtype,
>>  a new IOCTL CMD will be created to handle it. To keep the IOCTL naming
>>  consistent, a subtype index is added as part of the IOCTL CMD.
>>  
>> +2.2 TDX_CMD_GET_QUOTE
>> +----------------------
>> +
>> +:Input parameters: struct tdx_quote_req
>> +:Output: Return 0 on success, -EIO on TDCALL failure or standard error number
>> +         on common failures. Upon successful execution, QUOTE data is copied
>> +         to tdx_quote_req.buf.
>> +
>> +The TDX_CMD_GET_QUOTE IOCTL can be used by attestation software to generate
>> +QUOTE for the given TDREPORT using TDG.VP.VMCALL<GetQuote> hypercall.
> 
> Here QUOTE is used, but I see 'Quote' is used in changelog and the code as well,
> perhaps we need to make sure the consistency.

I will use "Quote" uniformly.

> 
>> +
>>  Reference
>>  ---------
>>  
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 26f6e2eaf5c8..09b5925eec67 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -33,6 +33,7 @@
>>  #define TDVMCALL_MAP_GPA		0x10001
>>  #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
>>  #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>> +#define TDVMCALL_GET_QUOTE		0x10002
>>  
>>  /* MMIO direction */
>>  #define EPT_READ	0
>> @@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
>>  		__tdx_hypercall(&args, 0);
>>  }
>>  
>> +/**
>> + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
>> + *                         hypercall.
>> + * @tdquote: Address of the direct mapped shared kernel buffer which
>> + * 	     contains TDREPORT data. The same buffer will be used by
>> + * 	     VMM to store the generated TD Quote output.
>> + * @size: size of the tdquote buffer.
>> + *
>> + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
>> + * v1.0 specification for more information on GetQuote hypercall.
>> + * It is used in the TDX guest driver module to get the TD Quote.
>> + *
>> + * Return 0 on success or error code on failure.
>> + */
>> +int tdx_hcall_get_quote(u8 *tdquote, size_t size)
>> +{
>> +	struct tdx_hypercall_args args = {0};
>> +
>> +	/*
>> +	 * TDX guest driver is the only user of this function and it uses
>> +	 * the kernel mapped memory. So use virt_to_phys() to get the
>> +	 * physical address of the TDQuote buffer without any additional
>> +	 * checks for memory type.
>> +	 */
> 
> How about just call out this function requires the buffer must be shared in kdoc
> style comment above this function?  We should just focus on what this function
> is doing.

It was suggested in the previous review to add it. A comment about why using
virt_to_phys() is fine here. 

> 
>> +	args.r10 = TDX_HYPERCALL_STANDARD;
>> +	args.r11 = TDVMCALL_GET_QUOTE;
>> +	args.r12 = cc_mkdec(virt_to_phys(tdquote));
>> +	args.r13 = size;
>> +
>> +	/*
>> +	 * Pass the physical address of TDREPORT to the VMM and
>> +	 * trigger the Quote generation. It is not a blocking
>> +	 * call, hence completion of this request will be notified to
>> +	 * the TD guest via a callback interrupt.
>> +	 */
>> +	return __tdx_hypercall(&args, 0);
>> +}
>> +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>> +
>>  static void tdx_parse_tdinfo(u64 *cc_mask)
>>  {
>>  	struct tdx_module_output out;
>> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
>> index 8807fe1b1f3f..a72bd7b96564 100644
>> --- a/arch/x86/include/asm/tdx.h
>> +++ b/arch/x86/include/asm/tdx.h
>> @@ -75,6 +75,8 @@ int tdx_register_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>>  
>>  int tdx_unregister_event_irq_cb(tdx_event_irq_cb_t handler, void *data);
>>  
>> +int tdx_hcall_get_quote(u8 *tdquote, size_t size);
>> +
>>  #else
>>  
>>  static inline void tdx_early_init(void) { };
>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> index 5e44a0fa69bd..a275d6b55f33 100644
>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> @@ -12,12 +12,105 @@
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/string.h>
>>  #include <linux/uaccess.h>
>> +#include <linux/set_memory.h>
>>  
>>  #include <uapi/linux/tdx-guest.h>
>>  
>>  #include <asm/cpu_device_id.h>
>>  #include <asm/tdx.h>
>>  
>> +#define GET_QUOTE_MAX_SIZE		(4 * PAGE_SIZE)
> 
> A comment to explain why this particular size is chosen would be helpful.

/*
 * Intel's SGX QE implementation generally uses Quote size less than 8K; Use
 * 16K as MAX size to handle future updates and other 3rd party implementations.
 */

> 
>> +
>> +/**
>> + * struct quote_entry - Quote request struct
>> + * @valid: Flag to check validity of the GetQuote request.
>> + * @buf: Kernel buffer to share data with VMM (size is page aligned).
>> + * @buf_len: Size of the buf in bytes.
>> + * @compl: Completion object to track completion of GetQuote request.
>> + */
>> +struct quote_entry {
>> +	bool valid;
>> +	void *buf;
>> +	size_t buf_len;
>> +	struct completion compl;
>> +};
> 
> We have a static global @qentry below.
> 
> The buffer size is a fixed size (16K), why do we need @buf_len here?

I have added it to support buf length changes in future (like adding a
command line option to allow user change the GET_QUOTE_MAX_SIZE).  Also,
IMO, using buf_len is more readable than just using GET_QUOTE_MAX_SIZE
macro in all places.

> 
> And why do we need @valid?  It seems ...

As a precaution against spurious event notification. I also believe that in
the future, event notification IRQs may be used for other purposes such as
vTPM or other TDVMCALL services, and that this handler may be triggered
without a valid GetQuote request. So, before we process the IRQ, I want to
make sure we have a valid buffer.

> 
>> +
>> +/* Quote data entry */
>> +static struct quote_entry *qentry;
>> +
>> +/* Lock to streamline quote requests */
>> +static DEFINE_MUTEX(quote_lock);
>> +
>> +static int quote_cb_handler(void *dev_id)
>> +{
>> +	struct quote_entry *entry = dev_id;
>> +	struct tdx_quote_hdr *quote_hdr = entry->buf;
>> +
>> +	if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
>> +		complete(&entry->compl);
> 
> ... this handler is only called when we have received the notification from the
> VMM, so the VMM must have already put something into the buffer, meaning the
> buffer is already valid.
> 
> Could you explain why do we need @valid?
> 
> That being said, to me I found the 'struct quote_entry' itself is quite
> unnecessary.  It looks like a leftover that you didn't remove when changing from
> supporting multiple GetQuote requests in parallel to only supporting one request
> at one time.

I don't want to use multiple global values. So I have bundled all Quote related
book keeping (completion object or buffer pointer) in the same struct.

> 
>> +
>> +	return 0;
>> +}
>> +
>> +static void free_shared_pages(void *buf, size_t len)
>> +{
>> +	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
>> +
>> +	if (!buf)
>> +		return;
> 
> Looks like a NULL @buf cannot happen.  Use WARN_ON_ONCE()?

Since the buf cannot be NULL as per current implementation,
I can remove it.

> 
>> +
>> +	set_memory_encrypted((unsigned long)buf, count);
>> +
>> +	__free_pages(virt_to_page(buf), get_order(len));
>> +}
>> +
>> +static void *alloc_shared_pages(size_t len)
>> +{
>> +	unsigned int count = PAGE_ALIGN(len) >> PAGE_SHIFT;
>> +	struct page *page;
>> +	int ret;
>> +
>> +	page = alloc_pages(GFP_KERNEL, get_order(len));
>> +	if (!page)
>> +		return NULL;
>> +
>> +	ret = set_memory_decrypted((unsigned long)page_address(page), count);
>> +	if (ret) {
>> +		__free_pages(page, get_order(len));
>> +		return NULL;
>> +	}
>> +
>> +	return page_address(page);
>> +}
> 
> I think you can use alloc_pages_exact():
> 
> 1) For 4 pages it doesn't matter, but in case in the future we want a non-power-
> of-2 size alloc_pages_exact() can save some memory.
> 
> 2) It gives you a kernel virtual address directly, so you can save the
> page_address() call above.

Ok makes sense. I will use alloc_pages_exact()/free_pages_exact().

> 
>> +
>> +static struct quote_entry *alloc_quote_entry(size_t len)
>> +{
>> +	struct quote_entry *entry = NULL;
>> +	size_t new_len = PAGE_ALIGN(len);
> 
> I don't think you need @new_len, because anyway alloc_shared_pages() will
> allocate enough space if you just pass @len?
> 
>> +
>> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
>> +	if (!entry)
>> +		return NULL;
>> +
>> +	entry->buf = alloc_shared_pages(new_len);
>> +	if (!entry->buf) {
>> +		kfree(entry);
>> +		return NULL;
>> +	}
>> +
>> +	entry->buf_len = new_len;
>> +	init_completion(&entry->compl);
>> +	entry->valid = false;
>> +
>> +	return entry;
>> +}
>> +
>> +static void free_quote_entry(struct quote_entry *entry)
>> +{
>> +	free_shared_pages(entry->buf, entry->buf_len);
>> +	kfree(entry);
>> +}
>> +
>>  static long tdx_get_report0(struct tdx_report_req __user *req)
>>  {
>>  	u8 *reportdata, *tdreport;
>> @@ -53,12 +146,59 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
>>  	return ret;
>>  }
>>  
>> +static long tdx_get_quote(struct tdx_quote_req __user *ureq)
>> +{
>> +	struct tdx_quote_req req;
>> +	long ret;
>> +
>> +	if (copy_from_user(&req, ureq, sizeof(req)))
>> +		return -EFAULT;
>> +
>> +	mutex_lock(&quote_lock);
>> +
>> +	if (!req.len || req.len > qentry->buf_len) {
>> +		ret = -EINVAL;
>> +		goto quote_failed;
>> +	}
>> +
>> +	if (copy_from_user(qentry->buf, (void __user *)req.buf, req.len)) {
>> +		ret = -EFAULT;
>> +		goto quote_failed;
>> +	}
>> +
>> +	qentry->valid = true;
>> +
>> +	reinit_completion(&qentry->compl);
>> +
>> +	/* Submit GetQuote Request using GetQuote hypercall */
>> +	ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
>> +	if (ret) {
>> +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
>> +		ret = -EIO;
>> +		goto quote_failed;
>> +	}
>> +
>> +	/* Wait till GetQuote completion */
>> +	wait_for_completion(&qentry->compl);
> 
> Non-killable wait w/o timeout worries me a little bit, because it can wait
> forever if VMM also couldn't get the Quote for whatever reason  and doesn't have
> it's own timeout.  Unfortunately the GHCI doesn't put any requirement to the VMM
> on this, so we kinda depend on the VMM.
> 
> Perhaps for now it's OK to have this simple implementation, but looks we should
> at least call out the risk in the comment.

How about the following comment?

/*
 * Since TDX GHCI specification does not define a valid timeout for GetQuote
 * requests, wait until VMM sends the completion notification. Note that there
 * is a risk that this wait can be infinite.
 */

> 
>> +
>> +	if (copy_to_user((void __user *)req.buf, qentry->buf, req.len))
>> +		ret = -EFAULT;
>> +
>> +quote_failed:
>> +	qentry->valid = false;
>> +	mutex_unlock(&quote_lock);
>> +
>> +	return ret;
>> +}
>> +
>>  static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>>  			    unsigned long arg)
>>  {
>>  	switch (cmd) {
>>  	case TDX_CMD_GET_REPORT0:
>>  		return tdx_get_report0((struct tdx_report_req __user *)arg);
>> +	case TDX_CMD_GET_QUOTE:
>> +		return tdx_get_quote((struct tdx_quote_req *)arg);
>>  	default:
>>  		return -ENOTTY;
>>  	}
>> @@ -84,15 +224,41 @@ MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
>>  
>>  static int __init tdx_guest_init(void)
>>  {
>> +	int ret;
>> +
>>  	if (!x86_match_cpu(tdx_guest_ids))
>>  		return -ENODEV;
>>  
>> -	return misc_register(&tdx_misc_dev);
>> +	ret = misc_register(&tdx_misc_dev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	qentry = alloc_quote_entry(GET_QUOTE_MAX_SIZE);
>> +	if (!qentry) {
>> +		pr_err("Quote entry allocation failed\n");
> 
> This is a rather confusing message from user's perspective.  The result of this
> error isn't clear from  this message.  I think we should have clear message ...

I will remove it.

> 
>> +		ret = -ENOMEM;
>> +		goto free_misc;
>> +	}
>> +
>> +	ret = tdx_register_event_irq_cb(quote_cb_handler, qentry);
>> +	if (ret)
>> +		goto free_quote;
>> +
>> +	return 0;
>> +
>> +free_quote:
>> +	free_quote_entry(qentry);
>> +free_misc:
>> +	misc_deregister(&tdx_misc_dev);
> 
> ... here saying something like "Attestation is not available" so user can be
> clear about this.
> 
>> +
>> +	return ret;
>>  }
>>  module_init(tdx_guest_init);
>>  
>>  static void __exit tdx_guest_exit(void)
>>  {
>> +	tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
>> +	free_quote_entry(qentry);
>>  	misc_deregister(&tdx_misc_dev);
>>  }
>>  module_exit(tdx_guest_exit);
>> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
>> index a6a2098c08ff..500cdfa025ad 100644
>> --- a/include/uapi/linux/tdx-guest.h
>> +++ b/include/uapi/linux/tdx-guest.h
>> @@ -17,6 +17,12 @@
>>  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>>  #define TDX_REPORT_LEN                  1024
>>  
>> +/* TD Quote status codes */
>> +#define GET_QUOTE_SUCCESS               0
>> +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
>> +#define GET_QUOTE_ERROR                 0x8000000000000000
>> +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
>> +
>>  /**
>>   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
>>   *
>> @@ -30,6 +36,35 @@ struct tdx_report_req {
>>  	__u8 tdreport[TDX_REPORT_LEN];
>>  };
>>  
>> +/* struct tdx_quote_hdr: Format of Quote request buffer header.
>> + * @version: Quote format version, filled by TD.
>> + * @status: Status code of Quote request, filled by VMM.
>> + * @in_len: Length of TDREPORT, filled by TD.
>> + * @out_len: Length of Quote data, filled by VMM.
>> + * @data: Quote data on output or TDREPORT on input.
>> + *
>> + * More details of Quote data header can be found in TDX
>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>> + */
>> +struct tdx_quote_hdr {
>> +	__u64 version;
>> +	__u64 status;
>> +	__u32 in_len;
>> +	__u32 out_len;
>> +	__u64 data[];
>> +};
> 
> This structure is weird.  It's a header, but it contains the dynamic-size
> buffer.  If you have __data[] in this structure, then it is already a buffer for
> the entire Quote, no?  Then should we just call it 'struct tdx_quote'?
> 
> Or do you want to remove __data[]?

I can name it as struct tdx_quote_data

> 
>> +
>> +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
>> + * @buf: Address of user buffer that includes TDREPORT. Upon successful
>> + *	 completion of IOCTL, output is copied back to the same buffer.
> 
> This description isn't precise.  "user buffer that includes TDREPORT" doesn't
> tell application writer where to put the TDREPORT at all.  We need to explicitly
> call out the buffer starts with 'tdx_quote_hdr' followed by TDREPORT
> immediately.

I have specified it in struct tdx_quote_hdr.data help content.

>  
>> + * @len: Length of the Quote buffer.
>> + */
>> +struct tdx_quote_req {
>> +	__u64 buf;
>> +	__u64 len;
>> +};
>> +
>>  /*
>>   * TDX_CMD_GET_REPORT0 - Get TDREPORT0 (a.k.a. TDREPORT subtype 0) using
>>   *                       TDCALL[TDG.MR.REPORT]
>> @@ -39,4 +74,12 @@ struct tdx_report_req {
>>   */
>>  #define TDX_CMD_GET_REPORT0              _IOWR('T', 1, struct tdx_report_req)
>>  
>> +/*
>> + * TDX_CMD_GET_QUOTE - Get TD Guest Quote from QE/QGS using GetQuote
>> + *		       TDVMCALL.
>> + *
>> + * Returns 0 on success or standard errno on other failures.
>> + */
>> +#define TDX_CMD_GET_QUOTE		_IOR('T', 2, struct tdx_quote_req)
>> +
>>  #endif /* _UAPI_LINUX_TDX_GUEST_H_ */
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Huang, Kai]

On Sun, 2023-04-30 at 23:03 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
> 
> On 4/28/23 6:49 AM, Huang, Kai wrote:
> > On Wed, 2023-04-12 at 20:41 -0700, Kuppuswamy Sathyanarayanan wrote:
> > > In TDX guest, the second stage in attestation process is to send the
> > > TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
> > > not support communication channels like vsock or TCP/IP, implement
> > > support to get TD Quote using hypercall. GetQuote hypercall can be used
> > > by the TD guest to request VMM facilitate the Quote generation via
> > > QE/QGS. More details about GetQuote hypercall can be found in TDX
> > > Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
> > > titled "TDG.VP.VMCALL<GetQuote>".
> > 
> > When this patch gets merged, the patch to get the TDREPORT would be long before
> > this patch.  To help the git blamers to understand more easily, I think it's
> > better to provide some background here.
> > 
> > FYI below:
> > 
> > "
> > In TDX attestation, the TDREPORT of a TDX guest contains information to uniquely
> > identify the TDX guest along with the TEE environment of the local machine. 
> > TDREPORT is integrity-protected and can only be verified on the local machine.  
> > 
> > To support TDX remote attestation, in SGX-based attestation, after the TDX guest
> > gets the TDREPORT from the TDX module, the TDREPORT needs to be sent to the SGX
> > Quoting Enclave (QE) to convert it to a remote verifiable Quote.
> > 
> > SGX QE can only run outside of the TDX guest (i.e. in a host process or in a
> > normal VM).  For security concern the TDX guest may not support normal
> > communication channels like vsock or TCP/IP.  To support remote attestation for
> > such case, TDX uses GetQuote TDVMCALL to ask the host VMM to communicate with
> > the SGX QE.  More details about GetQuote TDVMCALL can be found in ...
> > "
> > 
> 
> Ok. I will add some introduction to the commit log.
> 
> > > 
> > > Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
> > > submit GetQuote requests from the user space using GetQuote hypercall.
> > > 
> > > Since GetQuote is an asynchronous request hypercall, VMM will use
> > > callback interrupt vector configured by SetupEventNotifyInterrupt
> > > hypercall to notify the guest about Quote generation completion or
> > > failure. So register an IRQ handler for it.
> > > 
> > > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > > with TDREPORT data as input, which is further used by the VMM to copy
> > > the TD Quote result after successful Quote generation. To create the
> > > shared buffer, allocate the required memory using alloc_pages() and
> > > mark it shared using set_memory_decrypted() in tdx_guest_init(). 
> > > 
> > 
> > Personally I think you don't need to mention "using alloc_pages() ... 
> > set_memory_decrypted()" staff.  They belong to details and the code can tell.
> > 
> > > This
> > > buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
> > > IOCTL handler.
> > 
> > Besides the "re-used" part, I think it's better to explain the rational of
> > choosing a fixed 16K (4 pages) shared buffer.  For instance, in practice in
> > Intel's SGX QE implementation a Quote is less than 8K, and 16K should be big
> > enough in the foreseeable future even considering 3rd party's own
> > implementation.
> 
> I will add it part of the comment in the code.
> 
> > 
> > Also, I guess it's better to call out somewhere currently we don't support
> > multiple GetQuote in parallel because of <xxx>, so allocating a single shared
> > buffer at early time is OK.
> 
> 
> > 
> > > 
> > > Although this method will reserve a fixed chunk of memory for
> > > GetQuote requests during the init time, it is preferable to the
> > > alternative choice of allocating/freeing the shared buffer in the
> > > TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.
> > > 
> 
> Updated commit log looks like below:
> 
>     In TDX guest, the attestation process is used to verify the TDX guest
>     trustworthiness to other entities before provisioning secrets to the
>     guest. The First step in the attestation process is TDREPORT
>     generation, which involves getting the guest measurement data in the
>     format of TDREPORT, which is further used to validate the authenticity
>     of the TDX guest. TDREPORT by design is integrity-protected and can
>     only be verified on the local machine.
>     
>     To support remote verification of the TDREPORT (in a SGX-based
>     attestation), the TDREPORT needs to be sent to the SGX Quoting Enclave
>     (QE) to convert it to a remote verifiable Quote. SGX QE by design can
>     only run outside of the TDX guest (i.e. in a host process or in a
>     normal VM) and the guest can use communication channels like vsock or
>     TCP/IP to send the TDREPORT to the QE. But for security concerns, some
>     platforms may not support these communication channels. To handle such

"platforms may not support vsock/tcp/ip etc" isn't correct.  It should be the
"TDX guest" may not support those.

The security concern mainly is about the CSP doesn't want to expose vsock/tcp/ip
to TDX guest to allow it to be able to communicate to host service (Quoting
service in our example) directly.  However the host will almost certainly
support at least tcp/ip otherwise the machine is totally isolated.

>     cases, TDX defines a GetQuote hypercall which can be used by the guest
>     to request the host VMM to communicate with the SGX QE. More details
>     about GetQuote hypercall can be found in TDX Guest-Host Communication
>     Interface (GHCI) for Intel TDX 1.0, section titled
>     "TDG.VP.VMCALL<GetQuote>".
>     
>     Add support for TDX_CMD_GET_QUOTE IOCTL to allow an attestation agent
>     to submit GetQuote requests from the user space using GetQuote
>     hypercall.
>     
>     Since GetQuote is an asynchronous request hypercall, VMM will use the
>     callback interrupt vector configured by the SetupEventNotifyInterrupt
>     hypercall to notify the guest about Quote generation completion or
>     failure. So register an IRQ handler for it.
>     
>     GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>     with TDREPORT data as input, which is further used by the VMM to copy
>     the TD Quote result after successful Quote generation. Allocate the
>     required shared memory once in tdx_guest_init() and reuse it in the

"required" isn't very clear IMHO, because there's no mention of any requirement
at all regarding to the buffer size.  If you don't want to explain in changelog,
perhaps you need to at least mention something like "allocate a large enough
shared memory" etc.

>     TDX_CMD_GET_QUOTE IOCTL handler for GetQuote requests.
>     
>     Although this method reserves a fixed chunk of memory for GetQuote
>     requests, such one-time allocation is preferable to the alternative
>     choice of repeatedly allocating/freeing the shared buffer in the
>     TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map
>     (because the sharing/unsharing process modifies the direct map). This
>     allocation model is similar to that used by the AMD SEV guest driver.
>     
>     Since the Quote generation process is not time-critical or frequently
>     used, the current version do not support parallel GetQuote requests.
				^
				does not

[...]


> > >  
> > > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > > index 26f6e2eaf5c8..09b5925eec67 100644
> > > --- a/arch/x86/coco/tdx/tdx.c
> > > +++ b/arch/x86/coco/tdx/tdx.c
> > > @@ -33,6 +33,7 @@
> > >  #define TDVMCALL_MAP_GPA		0x10001
> > >  #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
> > >  #define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
> > > +#define TDVMCALL_GET_QUOTE		0x10002
> > >  
> > >  /* MMIO direction */
> > >  #define EPT_READ	0
> > > @@ -198,6 +199,45 @@ static void __noreturn tdx_panic(const char *msg)
> > >  		__tdx_hypercall(&args, 0);
> > >  }
> > >  
> > > +/**
> > > + * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
> > > + *                         hypercall.
> > > + * @tdquote: Address of the direct mapped shared kernel buffer which
> > > + * 	     contains TDREPORT data. The same buffer will be used by
> > > + * 	     VMM to store the generated TD Quote output.
> > > + * @size: size of the tdquote buffer.

Better to call out @size must be in granularity of 4K.

> > > + *
> > > + * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
> > > + * v1.0 specification for more information on GetQuote hypercall.
> > > + * It is used in the TDX guest driver module to get the TD Quote.
> > > + *
> > > + * Return 0 on success or error code on failure.
> > > + */
> > > +int tdx_hcall_get_quote(u8 *tdquote, size_t size)
> > > +{
> > > +	struct tdx_hypercall_args args = {0};
> > > +
> > > +	/*
> > > +	 * TDX guest driver is the only user of this function and it uses
> > > +	 * the kernel mapped memory. So use virt_to_phys() to get the
> > > +	 * physical address of the TDQuote buffer without any additional
> > > +	 * checks for memory type.
> > > +	 */
> > 
> > How about just call out this function requires the buffer must be shared in kdoc
> > style comment above this function?  We should just focus on what this function
> > is doing.
> 
> It was suggested in the previous review to add it. A comment about why using
> virt_to_phys() is fine here. 

Sorry tried to dig but I couldn't find the comment you mentioned.  

Anyway, IMHO you can just remove this comment, because after reading again, in
fact IMHO it only confuses people: 

1) "memory type" is confusing.  I think I (as someone who is working on TDX too)
understand by "memory type" you actually mean the virtual address is from kernel
direct mapping, or it could be from vmalloc()/vmap() (do you?), but for others
it could be cache related memory type (UC, WB, etc).

2) You already said the buffer is "direct mapped shared kernel buffer" (btw,
should be "directly mapped" really) in the comment which explains @tdquote, thus
it's clear how to get the physical address.  "TDX guest driver is the only user
of this function" doesn't matter -- when there's another user in the future and
if it changes where the @tdquote comes from, then the comment of @tdquote and
the code needs to be updated anyway.

So I don't see any good of this comment.

Btw, to me @tdquote isn't a good name because it gives me the impression that
the Quote is used as input.  How about just 'buf' (which you also used in
'tdx_quote_req' structure)?

> 
> > 
> > > +	args.r10 = TDX_HYPERCALL_STANDARD;
> > > +	args.r11 = TDVMCALL_GET_QUOTE;
> > > +	args.r12 = cc_mkdec(virt_to_phys(tdquote));

Btw can we just use __pa()?  To be honest I am ignorant on the difference
between virt_to_phys() and __pa(), i.e. when should we use which.

Also, you _may_ want to add a comment why "cc_mkdec()" is used.  By the nature
of this TDVMCALL, it's obvious the buffer needs to be shared, and the VMM must
check whether the buffer is actually shared, no matter whether the "shared-bit"
is set here or not.

So to me it's just requested by the GHCI spec that we need to include the
"shared-bit", but it _seems_ the GHCI spec doesn't explicitly say we need to do
that because it only says "Shared buffer as input".  So looks a comment can help
to clarify a little bit.

> > > +	args.r13 = size;

The @size must be 4K-aligned per GHCI.  I guess we should add a check and return
early rather than depending on the TDVMCALL to fail?

> > > +
> > > +	/*
> > > +	 * Pass the physical address of TDREPORT to the VMM and
> > > +	 * trigger the Quote generation. It is not a blocking
> > > +	 * call, hence completion of this request will be notified to
> > > +	 * the TD guest via a callback interrupt.
> > > +	 */
> > > +	return __tdx_hypercall(&args, 0);
> > > +}
> > > +EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
> > > +
> > > 
> 

[...]

> > 
> > > +
> > > +/**
> > > + * struct quote_entry - Quote request struct
> > > + * @valid: Flag to check validity of the GetQuote request.
> > > + * @buf: Kernel buffer to share data with VMM (size is page aligned).
> > > + * @buf_len: Size of the buf in bytes.
> > > + * @compl: Completion object to track completion of GetQuote request.
> > > + */
> > > +struct quote_entry {
> > > +	bool valid;
> > > +	void *buf;
> > > +	size_t buf_len;
> > > +	struct completion compl;
> > > +};
> > 
> > We have a static global @qentry below.
> > 
> > The buffer size is a fixed size (16K), why do we need @buf_len here?
> 
> I have added it to support buf length changes in future (like adding a
> command line option to allow user change the GET_QUOTE_MAX_SIZE).  Also,
> IMO, using buf_len is more readable than just using GET_QUOTE_MAX_SIZE
> macro in all places.
> 
> > 
> > And why do we need @valid?  It seems ...
> 
> As a precaution against spurious event notification. I also believe that in
> the future, event notification IRQs may be used for other purposes such as
> vTPM or other TDVMCALL services, and that this handler may be triggered
> without a valid GetQuote request. So, before we process the IRQ, I want to
> make sure we have a valid buffer.

OK.  This is an shared IRQ basically, so we need to track whether we have any
GetQuote request pending.

However I am wondering whether we need a dedicated @valid for this purpose.  If
I read correctly, we will make sure the buffer is zero-ed when there's no
request pending, thus can we just use some member in 'tdx_quote_hdr' to track?

For instance, per-GHCI the 'version' must be set to 1 for a valid request.  And
I think in a foreseeable future we can also assume @in_len being the size of
TDREPORT_STRUCT.  Can we use one of them (i.e. version) for this purpose?

> 
> > 
> > > +
> > > +/* Quote data entry */
> > > +static struct quote_entry *qentry;
> > > +
> > > +/* Lock to streamline quote requests */
> > > +static DEFINE_MUTEX(quote_lock);
> > > +
> > > +static int quote_cb_handler(void *dev_id)
> > > +{
> > > +	struct quote_entry *entry = dev_id;
> > > +	struct tdx_quote_hdr *quote_hdr = entry->buf;
> > > +
> > > +	if (entry->valid && quote_hdr->status != GET_QUOTE_IN_FLIGHT)
> > > +		complete(&entry->compl);
> > 
> > ... this handler is only called when we have received the notification from the
> > VMM, so the VMM must have already put something into the buffer, meaning the
> > buffer is already valid.
> > 
> > Could you explain why do we need @valid?
> > 
> > That being said, to me I found the 'struct quote_entry' itself is quite
> > unnecessary.  It looks like a leftover that you didn't remove when changing from
> > supporting multiple GetQuote requests in parallel to only supporting one request
> > at one time.
> 
> I don't want to use multiple global values. So I have bundled all Quote related
> book keeping (completion object or buffer pointer) in the same struct.

IMHO both @buf_len and @valid are not necessary, so you just need a static
buffer plus a static completion.  And you already have a static @quote_lock.

But no very strong opinion here..

[...]


> > > +
> > > +	/* Submit GetQuote Request using GetQuote hypercall */
> > > +	ret = tdx_hcall_get_quote(qentry->buf, qentry->buf_len);
> > > +	if (ret) {
> > > +		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
> > > +		ret = -EIO;
> > > +		goto quote_failed;
> > > +	}
> > > +
> > > +	/* Wait till GetQuote completion */
> > > +	wait_for_completion(&qentry->compl);
> > 
> > Non-killable wait w/o timeout worries me a little bit, because it can wait
> > forever if VMM also couldn't get the Quote for whatever reason  and doesn't have
> > it's own timeout.  Unfortunately the GHCI doesn't put any requirement to the VMM
> > on this, so we kinda depend on the VMM.
> > 
> > Perhaps for now it's OK to have this simple implementation, but looks we should
> > at least call out the risk in the comment.
> 
> How about the following comment?
> 
> /*
>  * Since TDX GHCI specification does not define a valid timeout for GetQuote
>  * requests, wait until VMM sends the completion notification. Note that there
>  * is a risk that this wait can be infinite.
>  */

This comment is missing the point I think.

There are two things actually:

1) The timeout from VMM

The GHCI should explicitly put some requirement on the VMM, i.e. say something
like "the VMM must not wait for the Quote infinitely but must signal the TDX
guest after a certain time, which can be implementation specific".  In this
case, we can be sure that the guest won't wait forever.

2) The timeout support in the GetQuote itself

This allows the guest to specify a timeout in the GetQuote TDVMCALL, so guest
can have its own control on how long to wait.

AFAICT for now we don't have requirement on 2).  What we truly want is 1),
because we certainly don't want to wait forever because of some careless VMM
implementation.

So, how about:

	/*
	 * Although the GHCI doesn't specifically put a hard requirement on the
	 * VMM that it must not wait for the Quote infinitely, a sane VMM
should
	 * always notify the guest after a certain time no matter whether
	 * getting the Quote is successful or not.  For now just depend on the
	 * VMM to do so.
	 */

[...]

> 
> > >  
> > >  static int __init tdx_guest_init(void)
> > >  {
> > > +	int ret;
> > > +
> > >  	if (!x86_match_cpu(tdx_guest_ids))
> > >  		return -ENODEV;
> > >  
> > > -	return misc_register(&tdx_misc_dev);
> > > +	ret = misc_register(&tdx_misc_dev);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	qentry = alloc_quote_entry(GET_QUOTE_MAX_SIZE);
> > > +	if (!qentry) {
> > > +		pr_err("Quote entry allocation failed\n");
> > 
> > This is a rather confusing message from user's perspective.  The result of this
> > error isn't clear from  this message.  I think we should have clear message ...
> 
> I will remove it.

I think it's fine to keep it, but my point is, besides above error msg, it's
better to explicitly print "attestation is not available", which is the result
of above error, to the user.

Whether the above message can be improved is another story.  For instance, I
believe "Failed to allocate Quote buffer" is better.

> 
> > 
> > > +		ret = -ENOMEM;
> > > +		goto free_misc;
> > > +	}
> > > +
> > > +	ret = tdx_register_event_irq_cb(quote_cb_handler, qentry);
> > > +	if (ret)
> > > +		goto free_quote;
> > > +
> > > +	return 0;
> > > +
> > > +free_quote:
> > > +	free_quote_entry(qentry);
> > > +free_misc:
> > > +	misc_deregister(&tdx_misc_dev);
> > 
> > ... here saying something like "Attestation is not available" so user can be
> > clear about this.
> > 
> > > +
> > > +	return ret;
> > >  }
> > >  module_init(tdx_guest_init);
> > >  
> > >  static void __exit tdx_guest_exit(void)
> > >  {
> > > +	tdx_unregister_event_irq_cb(quote_cb_handler, qentry);
> > > +	free_quote_entry(qentry);
> > >  	misc_deregister(&tdx_misc_dev);
> > >  }
> > >  module_exit(tdx_guest_exit);
> > > diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
> > > index a6a2098c08ff..500cdfa025ad 100644
> > > --- a/include/uapi/linux/tdx-guest.h
> > > +++ b/include/uapi/linux/tdx-guest.h
> > > @@ -17,6 +17,12 @@
> > >  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
> > >  #define TDX_REPORT_LEN                  1024
> > >  
> > > +/* TD Quote status codes */
> > > +#define GET_QUOTE_SUCCESS               0
> > > +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
> > > +#define GET_QUOTE_ERROR                 0x8000000000000000
> > > +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
> > > +
> > >  /**
> > >   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
> > >   *
> > > @@ -30,6 +36,35 @@ struct tdx_report_req {
> > >  	__u8 tdreport[TDX_REPORT_LEN];
> > >  };
> > >  
> > > +/* struct tdx_quote_hdr: Format of Quote request buffer header.
> > > + * @version: Quote format version, filled by TD.
> > > + * @status: Status code of Quote request, filled by VMM.
> > > + * @in_len: Length of TDREPORT, filled by TD.
> > > + * @out_len: Length of Quote data, filled by VMM.
> > > + * @data: Quote data on output or TDREPORT on input.
> > > + *
> > > + * More details of Quote data header can be found in TDX
> > > + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> > > + * section titled "TDG.VP.VMCALL<GetQuote>"
> > > + */
> > > +struct tdx_quote_hdr {
> > > +	__u64 version;
> > > +	__u64 status;
> > > +	__u32 in_len;
> > > +	__u32 out_len;
> > > +	__u64 data[];
> > > +};
> > 
> > This structure is weird.  It's a header, but it contains the dynamic-size
> > buffer.  If you have __data[] in this structure, then it is already a buffer for
> > the entire Quote, no?  Then should we just call it 'struct tdx_quote'?
> > 
> > Or do you want to remove __data[]?
> 
> I can name it as struct tdx_quote_data

If go with route, why not just 'tdx_quote', or 'tdx_tdquote'?

Or, actually I think 'tdx_quote' (or 'tdx_tdquote') seems to be the format of
the _true_ Quote, so perhaps we want 'struct tdx_quote_req_buf'?

> 
> > 
> > > +
> > > +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
> > > + * @buf: Address of user buffer that includes TDREPORT. Upon successful
> > > + *	 completion of IOCTL, output is copied back to the same buffer.
> > 
> > This description isn't precise.  "user buffer that includes TDREPORT" doesn't
> > tell application writer where to put the TDREPORT at all.  We need to explicitly
> > call out the buffer starts with 'tdx_quote_hdr' followed by TDREPORT
> > immediately.
> 
> I have specified it in struct tdx_quote_hdr.data help content.

Perhaps I missed something but I didn't say at any place this is clearly
documented.  The comment around @data above certainly doesn't.

Just say something like:

	@buf: The userspace pointer which points to the
	      'struct tdx_quote_req_buf' (whatever the final name)

> 
> >  
> > > + * @len: Length of the Quote buffer.
> > > + */
> > > +struct tdx_quote_req {
> > > +	__u64 buf;
> > > +	__u64 len;
> > > +};
> > > +
> > >  /*

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi Kai,

On 5/1/23 5:48 AM, Huang, Kai wrote:
> On Sun, 2023-04-30 at 23:03 -0700, Sathyanarayanan Kuppuswamy wrote:
>> Hi Kai,
>>
>> On 4/28/23 6:49 AM, Huang, Kai wrote:

[...]

> 
>>
>>>
>>>> +	args.r10 = TDX_HYPERCALL_STANDARD;
>>>> +	args.r11 = TDVMCALL_GET_QUOTE;
>>>> +	args.r12 = cc_mkdec(virt_to_phys(tdquote));
> 
> Btw can we just use __pa()?  To be honest I am ignorant on the difference
> between virt_to_phys() and __pa(), i.e. when should we use which.

The following link explains the difference. 

https://lkml.indiana.edu/hypermail/linux/kernel/0607.0/1607.html

In x86 ARCH, virt_to_phys() directly calls __pa(). So both are same.

But it looks like the recommendation is to use virt_to_phys().


> 
> Also, you _may_ want to add a comment why "cc_mkdec()" is used.  By the nature
> of this TDVMCALL, it's obvious the buffer needs to be shared, and the VMM must
> check whether the buffer is actually shared, no matter whether the "shared-bit"
> is set here or not.
> 
> So to me it's just requested by the GHCI spec that we need to include the
> "shared-bit", but it _seems_ the GHCI spec doesn't explicitly say we need to do
> that because it only says "Shared buffer as input".  So looks a comment can help
> to clarify a little bit.

I will add it.

>>>
>>>> +
>>>> +/**
>>>> + * struct quote_entry - Quote request struct
>>>> + * @valid: Flag to check validity of the GetQuote request.
>>>> + * @buf: Kernel buffer to share data with VMM (size is page aligned).
>>>> + * @buf_len: Size of the buf in bytes.
>>>> + * @compl: Completion object to track completion of GetQuote request.
>>>> + */
>>>> +struct quote_entry {
>>>> +	bool valid;
>>>> +	void *buf;
>>>> +	size_t buf_len;
>>>> +	struct completion compl;
>>>> +};
>>>
>>> We have a static global @qentry below.
>>>
>>> The buffer size is a fixed size (16K), why do we need @buf_len here?
>>
>> I have added it to support buf length changes in future (like adding a
>> command line option to allow user change the GET_QUOTE_MAX_SIZE).  Also,
>> IMO, using buf_len is more readable than just using GET_QUOTE_MAX_SIZE
>> macro in all places.
>>
>>>
>>> And why do we need @valid?  It seems ...
>>
>> As a precaution against spurious event notification. I also believe that in
>> the future, event notification IRQs may be used for other purposes such as
>> vTPM or other TDVMCALL services, and that this handler may be triggered
>> without a valid GetQuote request. So, before we process the IRQ, I want to
>> make sure we have a valid buffer.
> 
> OK.  This is an shared IRQ basically, so we need to track whether we have any
> GetQuote request pending.
> 
> However I am wondering whether we need a dedicated @valid for this purpose.  If
> I read correctly, we will make sure the buffer is zero-ed when there's no
> request pending, thus can we just use some member in 'tdx_quote_hdr' to track?
> 
> For instance, per-GHCI the 'version' must be set to 1 for a valid request.  And
> I think in a foreseeable future we can also assume @in_len being the size of
> TDREPORT_STRUCT.  Can we use one of them (i.e. version) for this purpose?

IMO, it is better to track it from the guest end (with a dedicated @valid). Since
quote request buffer is shared with the VMM, we should not just rely on its value
to decide whether we have an active request. If needed, in addition to the "@valid"
check we can also include check for @version. Also, I think we will not lose much
using a "bool" value to track the quote buffer valid status.



>>>> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
>>>> index a6a2098c08ff..500cdfa025ad 100644
>>>> --- a/include/uapi/linux/tdx-guest.h
>>>> +++ b/include/uapi/linux/tdx-guest.h
>>>> @@ -17,6 +17,12 @@
>>>>  /* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>>>>  #define TDX_REPORT_LEN                  1024
>>>>  
>>>> +/* TD Quote status codes */
>>>> +#define GET_QUOTE_SUCCESS               0
>>>> +#define GET_QUOTE_IN_FLIGHT             0xffffffffffffffff
>>>> +#define GET_QUOTE_ERROR                 0x8000000000000000
>>>> +#define GET_QUOTE_SERVICE_UNAVAILABLE   0x8000000000000001
>>>> +
>>>>  /**
>>>>   * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT0 IOCTL.
>>>>   *
>>>> @@ -30,6 +36,35 @@ struct tdx_report_req {
>>>>  	__u8 tdreport[TDX_REPORT_LEN];
>>>>  };
>>>>  
>>>> +/* struct tdx_quote_hdr: Format of Quote request buffer header.
>>>> + * @version: Quote format version, filled by TD.
>>>> + * @status: Status code of Quote request, filled by VMM.
>>>> + * @in_len: Length of TDREPORT, filled by TD.
>>>> + * @out_len: Length of Quote data, filled by VMM.
>>>> + * @data: Quote data on output or TDREPORT on input.
>>>> + *
>>>> + * More details of Quote data header can be found in TDX
>>>> + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
>>>> + * section titled "TDG.VP.VMCALL<GetQuote>"
>>>> + */
>>>> +struct tdx_quote_hdr {
>>>> +	__u64 version;
>>>> +	__u64 status;
>>>> +	__u32 in_len;
>>>> +	__u32 out_len;
>>>> +	__u64 data[];
>>>> +};
>>>
>>> This structure is weird.  It's a header, but it contains the dynamic-size
>>> buffer.  If you have __data[] in this structure, then it is already a buffer for
>>> the entire Quote, no?  Then should we just call it 'struct tdx_quote'?
>>>
>>> Or do you want to remove __data[]?
>>
>> I can name it as struct tdx_quote_data
> 
> If go with route, why not just 'tdx_quote', or 'tdx_tdquote'?
> 
> Or, actually I think 'tdx_quote' (or 'tdx_tdquote') seems to be the format of
> the _true_ Quote, so perhaps we want 'struct tdx_quote_req_buf'?
> 

Since this buffer is used for both request/response, we can just use
struct tdx_quote_buf.

>>
>>>
>>>> +
>>>> +/* struct tdx_quote_req: Request struct for TDX_CMD_GET_QUOTE IOCTL.
>>>> + * @buf: Address of user buffer that includes TDREPORT. Upon successful
>>>> + *	 completion of IOCTL, output is copied back to the same buffer.
>>>
>>> This description isn't precise.  "user buffer that includes TDREPORT" doesn't
>>> tell application writer where to put the TDREPORT at all.  We need to explicitly
>>> call out the buffer starts with 'tdx_quote_hdr' followed by TDREPORT
>>> immediately.
>>
>> I have specified it in struct tdx_quote_hdr.data help content.
> 
> Perhaps I missed something but I didn't say at any place this is clearly
> documented.  The comment around @data above certainly doesn't.
> 
> Just say something like:
> 
> 	@buf: The userspace pointer which points to the
> 	      'struct tdx_quote_req_buf' (whatever the final name)
> 
>>
>>>  
>>>> + * @len: Length of the Quote buffer.
>>>> + */
>>>> +struct tdx_quote_req {
>>>> +	__u64 buf;
>>>> +	__u64 len;
>>>> +};
>>>> +
>>>>  /*
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Huang, Kai]

On Thu, 2023-05-04 at 00:12 -0700, Sathyanarayanan Kuppuswamy wrote:
> Hi Kai,
> 
> On 5/1/23 5:48 AM, Huang, Kai wrote:
> > On Sun, 2023-04-30 at 23:03 -0700, Sathyanarayanan Kuppuswamy wrote:
> > > Hi Kai,
> > > 
> > > On 4/28/23 6:49 AM, Huang, Kai wrote:
> 
> [...]
> 
> > 
> > > 
> > > > 
> > > > > +	args.r10 = TDX_HYPERCALL_STANDARD;
> > > > > +	args.r11 = TDVMCALL_GET_QUOTE;
> > > > > +	args.r12 = cc_mkdec(virt_to_phys(tdquote));
> > 
> > Btw can we just use __pa()?  To be honest I am ignorant on the difference
> > between virt_to_phys() and __pa(), i.e. when should we use which.
> 
> The following link explains the difference. 
> 
> https://lkml.indiana.edu/hypermail/linux/kernel/0607.0/1607.html
> 
> In x86 ARCH, virt_to_phys() directly calls __pa(). So both are same.
> 
> But it looks like the recommendation is to use virt_to_phys().

Thanks!

[...]

> > > > 
> > > > > +
> > > > > +/**
> > > > > + * struct quote_entry - Quote request struct
> > > > > + * @valid: Flag to check validity of the GetQuote request.
> > > > > + * @buf: Kernel buffer to share data with VMM (size is page aligned).
> > > > > + * @buf_len: Size of the buf in bytes.
> > > > > + * @compl: Completion object to track completion of GetQuote request.
> > > > > + */
> > > > > +struct quote_entry {
> > > > > +	bool valid;
> > > > > +	void *buf;
> > > > > +	size_t buf_len;
> > > > > +	struct completion compl;
> > > > > +};
> > > > 
> > > > We have a static global @qentry below.
> > > > 
> > > > The buffer size is a fixed size (16K), why do we need @buf_len here?
> > > 
> > > I have added it to support buf length changes in future (like adding a
> > > command line option to allow user change the GET_QUOTE_MAX_SIZE).  Also,
> > > IMO, using buf_len is more readable than just using GET_QUOTE_MAX_SIZE
> > > macro in all places.
> > > 
> > > > 
> > > > And why do we need @valid?  It seems ...
> > > 
> > > As a precaution against spurious event notification. I also believe that in
> > > the future, event notification IRQs may be used for other purposes such as
> > > vTPM or other TDVMCALL services, and that this handler may be triggered
> > > without a valid GetQuote request. So, before we process the IRQ, I want to
> > > make sure we have a valid buffer.
> > 
> > OK.  This is an shared IRQ basically, so we need to track whether we have any
> > GetQuote request pending.
> > 
> > However I am wondering whether we need a dedicated @valid for this purpose.  If
> > I read correctly, we will make sure the buffer is zero-ed when there's no
> > request pending, thus can we just use some member in 'tdx_quote_hdr' to track?
> > 
> > For instance, per-GHCI the 'version' must be set to 1 for a valid request.  And
> > I think in a foreseeable future we can also assume @in_len being the size of
> > TDREPORT_STRUCT.  Can we use one of them (i.e. version) for this purpose?
> 
> IMO, it is better to track it from the guest end (with a dedicated @valid). Since
> quote request buffer is shared with the VMM, we should not just rely on its value
> to decide whether we have an active request. If needed, in addition to the "@valid"
> check we can also include check for @version. Also, I think we will not lose much
> using a "bool" value to track the quote buffer valid status.

But the header is never written by the VMM, right?

If the VMM is buggy then the guest is not guaranteed to work properly anyway.

So to me there's no need to have additional @valid.

[...]
> > > > 

> > > > > +/* struct tdx_quote_hdr: Format of Quote request buffer header.
> > > > > + * @version: Quote format version, filled by TD.
> > > > > + * @status: Status code of Quote request, filled by VMM.
> > > > > + * @in_len: Length of TDREPORT, filled by TD.
> > > > > + * @out_len: Length of Quote data, filled by VMM.
> > > > > + * @data: Quote data on output or TDREPORT on input.
> > > > > + *
> > > > > + * More details of Quote data header can be found in TDX
> > > > > + * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
> > > > > + * section titled "TDG.VP.VMCALL<GetQuote>"
> > > > > + */
> > > > > +struct tdx_quote_hdr {
> > > > > +	__u64 version;
> > > > > +	__u64 status;
> > > > > +	__u32 in_len;
> > > > > +	__u32 out_len;
> > > > > +	__u64 data[];
> > > > > +};
> > > > 
> > > > This structure is weird.  It's a header, but it contains the dynamic-size
> > > > buffer.  If you have __data[] in this structure, then it is already a buffer for
> > > > the entire Quote, no?  Then should we just call it 'struct tdx_quote'?
> > > > 
> > > > Or do you want to remove __data[]?
> > > 
> > > I can name it as struct tdx_quote_data
> > 
> > If go with route, why not just 'tdx_quote', or 'tdx_tdquote'?
> > 
> > Or, actually I think 'tdx_quote' (or 'tdx_tdquote') seems to be the format of
> > the _true_ Quote, so perhaps we want 'struct tdx_quote_req_buf'?
> > 
> 
> Since this buffer is used for both request/response, we can just use
> struct tdx_quote_buf.

Not my preference but will leave to others.

Re: [PATCH v2 2/3] virt: tdx-guest: Add Quote generation support

[Chong Cai]

On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> In TDX guest, the second stage in attestation process is to send the
> TDREPORT to QE/QGS to generate the TD Quote. For platforms that does
> not support communication channels like vsock or TCP/IP, implement
> support to get TD Quote using hypercall. GetQuote hypercall can be used
> by the TD guest to request VMM facilitate the Quote generation via
> QE/QGS. More details about GetQuote hypercall can be found in TDX
> Guest-Host Communication Interface (GHCI) for Intel TDX 1.0, section
> titled "TDG.VP.VMCALL<GetQuote>".
>
> Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
> submit GetQuote requests from the user space using GetQuote hypercall.
>
> Since GetQuote is an asynchronous request hypercall, VMM will use
> callback interrupt vector configured by SetupEventNotifyInterrupt
> hypercall to notify the guest about Quote generation completion or
> failure. So register an IRQ handler for it.
>
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate the required memory using alloc_pages() and
> mark it shared using set_memory_decrypted() in tdx_guest_init(). This
> buffer will be re-used for GetQuote requests in TDX_CMD_GET_QUOTE
> IOCTL handler.
>
> Although this method will reserve a fixed chunk of memory for
> GetQuote requests during the init time, it is preferable to the
> alternative choice of allocating/freeing the shared buffer in the
> TDX_CMD_GET_QUOTE IOCTL handler, which will damage the direct map.

Thanks Sathyanarayanan for the work. The patch looks good. Reserving a fixed
chunk of memory for GetQuote makes sense to me.

And just want to re-emphasize that the TDVMCALL approach is important for
many use cases that cannot depend on virtio/vsock.

[PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Kuppuswamy Sathyanarayanan]

In TDX guest, the second stage of the attestation process is Quote
generation. This process is required to convert the locally generated
TDREPORT into a remotely verifiable Quote. It involves sending the
TDREPORT data to a Quoting Enclave (QE) which will verify the
integerity of the TDREPORT and sign it with an attestation key.

Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to
allow user agent get the TD Quote.

Add a kernel selftest module to verify the Quote generation feature.

TD Quote generation involves following steps:

* Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL.
* Embed the TDREPORT data in quote buffer and request for quote
  generation via TDX_CMD_GET_QUOTE IOCTL request.
* Upon completion of the GetQuote request, check for non zero value
  in the status field of Quote header to make sure the generated
  quote is valid.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 tools/testing/selftests/tdx/tdx_guest_test.c | 68 ++++++++++++++++++--
 1 file changed, 62 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c
index 81d8cb88ea1a..2eccde54185b 100644
--- a/tools/testing/selftests/tdx/tdx_guest_test.c
+++ b/tools/testing/selftests/tdx/tdx_guest_test.c
@@ -18,6 +18,7 @@
 #define TDX_GUEST_DEVNAME "/dev/tdx_guest"
 #define HEX_DUMP_SIZE 8
 #define DEBUG 0
+#define QUOTE_SIZE 8192
 
 /**
  * struct tdreport_type - Type header of TDREPORT_STRUCT.
@@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str,
 	printf("\n");
 }
 
+/* Helper function to get TDREPORT */
+long get_tdreport0(int devfd, struct tdx_report_req *req)
+{
+	int i;
+
+	/* Generate sample report data */
+	for (i = 0; i < TDX_REPORTDATA_LEN; i++)
+		req->reportdata[i] = i;
+
+	return ioctl(devfd, TDX_CMD_GET_REPORT0, req);
+}
+
 TEST(verify_report)
 {
 	struct tdx_report_req req;
 	struct tdreport *tdreport;
-	int devfd, i;
+	int devfd;
 
 	devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
 	ASSERT_LT(0, devfd);
 
-	/* Generate sample report data */
-	for (i = 0; i < TDX_REPORTDATA_LEN; i++)
-		req.reportdata[i] = i;
-
 	/* Get TDREPORT */
-	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req));
+	ASSERT_EQ(0, get_tdreport0(devfd, &req));
 
 	if (DEBUG) {
 		print_array_hex("\n\t\tTDX report data\n", "",
@@ -160,4 +169,51 @@ TEST(verify_report)
 	ASSERT_EQ(0, close(devfd));
 }
 
+TEST(verify_quote)
+{
+	struct tdx_quote_hdr *quote_hdr;
+	struct tdx_report_req rep_req;
+	struct tdx_quote_req req;
+	__u64 quote_buf_size;
+	__u8 *quote_buf;
+	int devfd;
+
+	/* Open attestation device */
+	devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
+
+	ASSERT_LT(0, devfd);
+
+	/* Add size for quote header */
+	quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE;
+
+	/* Allocate quote buffer */
+	quote_buf = malloc(quote_buf_size);
+	ASSERT_NE(NULL, quote_buf);
+
+	quote_hdr = (struct tdx_quote_hdr *)quote_buf;
+
+	/* Initialize GetQuote header */
+	quote_hdr->version = 1;
+	quote_hdr->status  = GET_QUOTE_SUCCESS;
+	quote_hdr->in_len  = TDX_REPORT_LEN;
+	quote_hdr->out_len = 0;
+
+	/* Get TDREPORT data */
+	ASSERT_EQ(0, get_tdreport0(devfd, &rep_req));
+
+	/* Fill GetQuote request */
+	memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN);
+	req.buf	  = (__u64)quote_buf;
+	req.len	  = quote_buf_size;
+
+	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req));
+
+	/* Check whether GetQuote request is successful */
+	EXPECT_EQ(0, quote_hdr->status);
+
+	free(quote_buf);
+
+	ASSERT_EQ(0, close(devfd));
+}
+
 TEST_HARNESS_MAIN
-- 
2.34.1

Re: [PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Dionna Amalie Glaze]

On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> In TDX guest, the second stage of the attestation process is Quote
> generation. This process is required to convert the locally generated
> TDREPORT into a remotely verifiable Quote. It involves sending the
> TDREPORT data to a Quoting Enclave (QE) which will verify the
> integerity of the TDREPORT and sign it with an attestation key.

nit: integrity

>
> Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to
> allow user agent get the TD Quote.

nit: to get
>
> Add a kernel selftest module to verify the Quote generation feature.
>
> TD Quote generation involves following steps:
>
> * Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL.
> * Embed the TDREPORT data in quote buffer and request for quote
>   generation via TDX_CMD_GET_QUOTE IOCTL request.
> * Upon completion of the GetQuote request, check for non zero value
>   in the status field of Quote header to make sure the generated
>   quote is valid.
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
>  tools/testing/selftests/tdx/tdx_guest_test.c | 68 ++++++++++++++++++--
>  1 file changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c
> index 81d8cb88ea1a..2eccde54185b 100644
> --- a/tools/testing/selftests/tdx/tdx_guest_test.c
> +++ b/tools/testing/selftests/tdx/tdx_guest_test.c
> @@ -18,6 +18,7 @@
>  #define TDX_GUEST_DEVNAME "/dev/tdx_guest"
>  #define HEX_DUMP_SIZE 8
>  #define DEBUG 0
> +#define QUOTE_SIZE 8192
>
>  /**
>   * struct tdreport_type - Type header of TDREPORT_STRUCT.
> @@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str,
>         printf("\n");
>  }
>
> +/* Helper function to get TDREPORT */
> +long get_tdreport0(int devfd, struct tdx_report_req *req)
> +{
> +       int i;
> +
> +       /* Generate sample report data */
> +       for (i = 0; i < TDX_REPORTDATA_LEN; i++)
> +               req->reportdata[i] = i;
> +

Shouldn't req be zeroed before populating reportdata? We wouldn't want
uninitialized memory to leave the guest. I know this is just a test,
but it's best to model good practices for anyone that might
copy/paste.

> +       return ioctl(devfd, TDX_CMD_GET_REPORT0, req);
> +}
> +
>  TEST(verify_report)
>  {
>         struct tdx_report_req req;
>         struct tdreport *tdreport;
> -       int devfd, i;
> +       int devfd;
>
>         devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
>         ASSERT_LT(0, devfd);
>
> -       /* Generate sample report data */
> -       for (i = 0; i < TDX_REPORTDATA_LEN; i++)
> -               req.reportdata[i] = i;
> -
>         /* Get TDREPORT */
> -       ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req));
> +       ASSERT_EQ(0, get_tdreport0(devfd, &req));
>
>         if (DEBUG) {
>                 print_array_hex("\n\t\tTDX report data\n", "",
> @@ -160,4 +169,51 @@ TEST(verify_report)
>         ASSERT_EQ(0, close(devfd));
>  }
>
> +TEST(verify_quote)
> +{
> +       struct tdx_quote_hdr *quote_hdr;
> +       struct tdx_report_req rep_req;
> +       struct tdx_quote_req req;
> +       __u64 quote_buf_size;
> +       __u8 *quote_buf;
> +       int devfd;
> +
> +       /* Open attestation device */
> +       devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
> +
> +       ASSERT_LT(0, devfd);
> +
> +       /* Add size for quote header */
> +       quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE;
> +
> +       /* Allocate quote buffer */
> +       quote_buf = malloc(quote_buf_size);
> +       ASSERT_NE(NULL, quote_buf);
> +
> +       quote_hdr = (struct tdx_quote_hdr *)quote_buf;
> +
> +       /* Initialize GetQuote header */
> +       quote_hdr->version = 1;
> +       quote_hdr->status  = GET_QUOTE_SUCCESS;
> +       quote_hdr->in_len  = TDX_REPORT_LEN;
> +       quote_hdr->out_len = 0;
> +
> +       /* Get TDREPORT data */
> +       ASSERT_EQ(0, get_tdreport0(devfd, &rep_req));
> +
> +       /* Fill GetQuote request */
> +       memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN);
> +       req.buf   = (__u64)quote_buf;
> +       req.len   = quote_buf_size;
> +
> +       ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req));
> +
> +       /* Check whether GetQuote request is successful */
> +       EXPECT_EQ(0, quote_hdr->status);
> +
> +       free(quote_buf);
> +
> +       ASSERT_EQ(0, close(devfd));
> +}
> +
>  TEST_HARNESS_MAIN
> --
> 2.34.1
>


-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Sathyanarayanan Kuppuswamy]

Hi,

On 4/26/23 8:47 AM, Dionna Amalie Glaze wrote:
> On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> In TDX guest, the second stage of the attestation process is Quote
>> generation. This process is required to convert the locally generated
>> TDREPORT into a remotely verifiable Quote. It involves sending the
>> TDREPORT data to a Quoting Enclave (QE) which will verify the
>> integerity of the TDREPORT and sign it with an attestation key.
> 
> nit: integrity
> 
>>
>> Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to
>> allow user agent get the TD Quote.
> 
> nit: to get
>>
>> Add a kernel selftest module to verify the Quote generation feature.
>>
>> TD Quote generation involves following steps:
>>
>> * Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL.
>> * Embed the TDREPORT data in quote buffer and request for quote
>>   generation via TDX_CMD_GET_QUOTE IOCTL request.
>> * Upon completion of the GetQuote request, check for non zero value
>>   in the status field of Quote header to make sure the generated
>>   quote is valid.
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.intel.com>
>> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>
>> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>  tools/testing/selftests/tdx/tdx_guest_test.c | 68 ++++++++++++++++++--
>>  1 file changed, 62 insertions(+), 6 deletions(-)
>>
>> diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c
>> index 81d8cb88ea1a..2eccde54185b 100644
>> --- a/tools/testing/selftests/tdx/tdx_guest_test.c
>> +++ b/tools/testing/selftests/tdx/tdx_guest_test.c
>> @@ -18,6 +18,7 @@
>>  #define TDX_GUEST_DEVNAME "/dev/tdx_guest"
>>  #define HEX_DUMP_SIZE 8
>>  #define DEBUG 0
>> +#define QUOTE_SIZE 8192
>>
>>  /**
>>   * struct tdreport_type - Type header of TDREPORT_STRUCT.
>> @@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str,
>>         printf("\n");
>>  }
>>
>> +/* Helper function to get TDREPORT */
>> +long get_tdreport0(int devfd, struct tdx_report_req *req)
>> +{
>> +       int i;
>> +
>> +       /* Generate sample report data */
>> +       for (i = 0; i < TDX_REPORTDATA_LEN; i++)
>> +               req->reportdata[i] = i;
>> +
> 
> Shouldn't req be zeroed before populating reportdata? We wouldn't want
> uninitialized memory to leave the guest. I know this is just a test,

There are only two members in struct tdx_report_req (reportdata and tdreport).
The reportdata has already been updated here, and the tdreport will be updated
by the kernel on output. Since TDX_CMD_GET_REPORT0 IOCTL handler uses an
intermediate kernel buffer to the TDREPORT and copies the generated report back
to this user buffer, this uninitialized tdreport data never leaves the guest.

IMO, we don't need to zero it. However, because it is harmless, I am fine with
including it.

> but it's best to model good practices for anyone that might
> copy/paste.


> 
>> +       return ioctl(devfd, TDX_CMD_GET_REPORT0, req);
>> +}
>> +
>>  TEST(verify_report)
>>  {
>>         struct tdx_report_req req;
>>         struct tdreport *tdreport;
>> -       int devfd, i;
>> +       int devfd;
>>
>>         devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
>>         ASSERT_LT(0, devfd);
>>
>> -       /* Generate sample report data */
>> -       for (i = 0; i < TDX_REPORTDATA_LEN; i++)
>> -               req.reportdata[i] = i;
>> -
>>         /* Get TDREPORT */
>> -       ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req));
>> +       ASSERT_EQ(0, get_tdreport0(devfd, &req));
>>
>>         if (DEBUG) {
>>                 print_array_hex("\n\t\tTDX report data\n", "",
>> @@ -160,4 +169,51 @@ TEST(verify_report)
>>         ASSERT_EQ(0, close(devfd));
>>  }
>>
>> +TEST(verify_quote)
>> +{
>> +       struct tdx_quote_hdr *quote_hdr;
>> +       struct tdx_report_req rep_req;
>> +       struct tdx_quote_req req;
>> +       __u64 quote_buf_size;
>> +       __u8 *quote_buf;
>> +       int devfd;
>> +
>> +       /* Open attestation device */
>> +       devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC);
>> +
>> +       ASSERT_LT(0, devfd);
>> +
>> +       /* Add size for quote header */
>> +       quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE;
>> +
>> +       /* Allocate quote buffer */
>> +       quote_buf = malloc(quote_buf_size);
>> +       ASSERT_NE(NULL, quote_buf);
>> +
>> +       quote_hdr = (struct tdx_quote_hdr *)quote_buf;
>> +
>> +       /* Initialize GetQuote header */
>> +       quote_hdr->version = 1;
>> +       quote_hdr->status  = GET_QUOTE_SUCCESS;
>> +       quote_hdr->in_len  = TDX_REPORT_LEN;
>> +       quote_hdr->out_len = 0;
>> +
>> +       /* Get TDREPORT data */
>> +       ASSERT_EQ(0, get_tdreport0(devfd, &rep_req));
>> +
>> +       /* Fill GetQuote request */
>> +       memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN);
>> +       req.buf   = (__u64)quote_buf;
>> +       req.len   = quote_buf_size;
>> +
>> +       ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req));
>> +
>> +       /* Check whether GetQuote request is successful */
>> +       EXPECT_EQ(0, quote_hdr->status);
>> +
>> +       free(quote_buf);
>> +
>> +       ASSERT_EQ(0, close(devfd));
>> +}
>> +
>>  TEST_HARNESS_MAIN
>> --
>> 2.34.1
>>
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Dave Hansen]

On 4/27/23 12:10, Sathyanarayanan Kuppuswamy wrote:
>> Shouldn't req be zeroed before populating reportdata? We wouldn't want
>> uninitialized memory to leave the guest. I know this is just a test,
> There are only two members in struct tdx_report_req (reportdata and tdreport).
> The reportdata has already been updated here, and the tdreport will be updated
> by the kernel on output. Since TDX_CMD_GET_REPORT0 IOCTL handler uses an
> intermediate kernel buffer to the TDREPORT and copies the generated report back
> to this user buffer, this uninitialized tdreport data never leaves the guest.

Is that really even relevant?

I mean, we could implement the whole thing with get_user_pages() and
then just pass the physical address of the reportdata and tdreport down
to the TDX module.

It doesn't matter either way.  The data is going from guest userspace to
the guest kernel to the TDX module, all of which are trusted.

It's a selftest.  I'd just leave it alone.

Re: [PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

[Dave Hansen]

On 4/26/23 08:47, Dionna Amalie Glaze wrote:
>> +/* Helper function to get TDREPORT */
>> +long get_tdreport0(int devfd, struct tdx_report_req *req)
>> +{
>> +       int i;
>> +
>> +       /* Generate sample report data */
>> +       for (i = 0; i < TDX_REPORTDATA_LEN; i++)
>> +               req->reportdata[i] = i;
>> +
> Shouldn't req be zeroed before populating reportdata? We wouldn't want
> uninitialized memory to leave the guest. I know this is just a test,
> but it's best to model good practices for anyone that might
> copy/paste.

It's leaving the guest and going to the TDX module.  What's the problem
there?

Re: [PATCH v2 0/3] TDX Guest Quote generation support

[Erdem Aktas]

On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Hi All,
>
> In TDX guest, the attestation process is used to verify the TDX guest
> trustworthiness to other entities before provisioning secrets to the
> guest.
>
> The TDX guest attestation process consists of two steps:
>
> 1. TDREPORT generation
> 2. Quote generation.
>
> The First step (TDREPORT generation) involves getting the TDX guest
> measurement data in the format of TDREPORT which is further used to
> validate the authenticity of the TDX guest. The second step involves
> sending the TDREPORT to a Quoting Enclave (QE) server to generate a
> remotely verifiable Quote. TDREPORT by design can only be verified on
> the local platform. To support remote verification of the TDREPORT,
> TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT
> locally and convert it to a remotely verifiable Quote. Although
> attestation software can use communication methods like TCP/IP or
> vsock to send the TDREPORT to QE, not all platforms support these
> communication models. So TDX GHCI specification [1] defines a method
> for Quote generation via hypercalls. Please check the discussion from
> Google [2] and Alibaba [3] which clarifies the need for hypercall based
> Quote generation support. This patch set adds this support.


Thanks Kuppuswamy for the v2 of this patch set.
I reviewed all  3 patches and it looks good for me and it covers our use cases.

>
> Support for TDREPORT generation already exists in the TDX guest driver.
> This patchset extends the same driver to add the Quote generation
> support.
>
> Following are the details of the patch set:
>
> Patch 1/3 -> Adds event notification IRQ support.
> Patch 2/3 -> Adds Quote generation support.
> Patch 3/3 -> Adds selftest support for Quote generation feature.
>
> [1] https://cdrdv2.intel.com/v1/dl/getContent/726790, section titled "TDG.VP.VMCALL<GetQuote>".
> [2] https://lore.kernel.org/lkml/CAAYXXYxxs2zy_978GJDwKfX5Hud503gPc8=1kQ-+JwG_kA79mg@mail.gmail.com/
> [3] https://lore.kernel.org/lkml/a69faebb-11e8-b386-d591-dbd08330b008@linux.alibaba.com/
>
> Kuppuswamy Sathyanarayanan (3):
>   x86/tdx: Add TDX Guest event notify interrupt support
>   virt: tdx-guest: Add Quote generation support
>   selftests/tdx: Test GetQuote TDX attestation feature
>
>  Documentation/virt/coco/tdx-guest.rst        |  11 ++
>  arch/x86/coco/tdx/tdx.c                      | 196 +++++++++++++++++++
>  arch/x86/include/asm/tdx.h                   |   8 +
>  drivers/virt/coco/tdx-guest/tdx-guest.c      | 168 +++++++++++++++-
>  include/uapi/linux/tdx-guest.h               |  43 ++++
>  tools/testing/selftests/tdx/tdx_guest_test.c |  68 ++++++-
>  6 files changed, 487 insertions(+), 7 deletions(-)
>
> --
> 2.34.1
>

Re: [PATCH v2 0/3] TDX Guest Quote generation support

[Sathyanarayanan Kuppuswamy]

Hi Erdem,

On 5/9/23 5:10 PM, Erdem Aktas wrote:
> On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>>
>> Hi All,
>>
>> In TDX guest, the attestation process is used to verify the TDX guest
>> trustworthiness to other entities before provisioning secrets to the
>> guest.
>>
>> The TDX guest attestation process consists of two steps:
>>
>> 1. TDREPORT generation
>> 2. Quote generation.
>>
>> The First step (TDREPORT generation) involves getting the TDX guest
>> measurement data in the format of TDREPORT which is further used to
>> validate the authenticity of the TDX guest. The second step involves
>> sending the TDREPORT to a Quoting Enclave (QE) server to generate a
>> remotely verifiable Quote. TDREPORT by design can only be verified on
>> the local platform. To support remote verification of the TDREPORT,
>> TDX leverages Intel SGX Quoting Enclave to verify the TDREPORT
>> locally and convert it to a remotely verifiable Quote. Although
>> attestation software can use communication methods like TCP/IP or
>> vsock to send the TDREPORT to QE, not all platforms support these
>> communication models. So TDX GHCI specification [1] defines a method
>> for Quote generation via hypercalls. Please check the discussion from
>> Google [2] and Alibaba [3] which clarifies the need for hypercall based
>> Quote generation support. This patch set adds this support.
> 
> 
> Thanks Kuppuswamy for the v2 of this patch set.
> I reviewed all  3 patches and it looks good for me and it covers our use cases.

Thanks for the review. I will address other reviewers comments and resubmit v3
this week. I will include your Reviewed-by in it.

> 
>>
>> Support for TDREPORT generation already exists in the TDX guest driver.
>> This patchset extends the same driver to add the Quote generation
>> support.
>>
>> Following are the details of the patch set:
>>
>> Patch 1/3 -> Adds event notification IRQ support.
>> Patch 2/3 -> Adds Quote generation support.
>> Patch 3/3 -> Adds selftest support for Quote generation feature.
>>
>> [1] https://cdrdv2.intel.com/v1/dl/getContent/726790, section titled "TDG.VP.VMCALL<GetQuote>".
>> [2] https://lore.kernel.org/lkml/CAAYXXYxxs2zy_978GJDwKfX5Hud503gPc8=1kQ-+JwG_kA79mg@mail.gmail.com/
>> [3] https://lore.kernel.org/lkml/a69faebb-11e8-b386-d591-dbd08330b008@linux.alibaba.com/
>>
>> Kuppuswamy Sathyanarayanan (3):
>>   x86/tdx: Add TDX Guest event notify interrupt support
>>   virt: tdx-guest: Add Quote generation support
>>   selftests/tdx: Test GetQuote TDX attestation feature
>>
>>  Documentation/virt/coco/tdx-guest.rst        |  11 ++
>>  arch/x86/coco/tdx/tdx.c                      | 196 +++++++++++++++++++
>>  arch/x86/include/asm/tdx.h                   |   8 +
>>  drivers/virt/coco/tdx-guest/tdx-guest.c      | 168 +++++++++++++++-
>>  include/uapi/linux/tdx-guest.h               |  43 ++++
>>  tools/testing/selftests/tdx/tdx_guest_test.c |  68 ++++++-
>>  6 files changed, 487 insertions(+), 7 deletions(-)
>>
>> --
>> 2.34.1
>>

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer