[PATCH v9 0/6] Add TDX Guest Attestation support

[PATCH v9 0/6] Add TDX Guest Attestation support

[Kuppuswamy Sathyanarayanan]

Hi All,

Intel's Trust Domain Extensions (TDX) protect guest VMs from malicious
hosts and some physical attacks. VM guest with TDX support is called
as TD Guest.

In TD Guest, the attestation process is used to verify the 
trustworthiness of TD guest to the 3rd party servers. Such attestation
process is required by 3rd party servers before sending sensitive
information to TD guests. One usage example is to get encryption keys
from the key server for mounting the encrypted rootfs or secondary drive.

During TD launch, the initial contents and configuration of the TD are
recorded by the Intel TDX module in the build time measurement register
(MRTD). In addition, run-time measurement registers (RTMRs)  can be
used by the guest TD software, e.g., to measure a boot process. At
run-time, the Intel TDX module reuses the Intel SGX attestation
infrastructure to provide support for attesting to these measurements
as described below.

Intel TDX attestation is intended to be used in two phases:

TD Guest can use the TDCALL(TDG.MR.REPORT) function to request the
Intel TDX Module to generate an integrity-protected TDREPORT structure
(TDREPORT_STRUCT). This structure includes the TD’s measurements, the
Intel TDX module’s measurements, and a value provided by the guest TD
software. This will typically be an asymmetric key that the attestation
verifier can use to establish a secure channel or protect sensitive
data to be sent to the TD software.

To protect the integrity of TDREPORT_STRUCT, it is HMAC'ed using an
HMAC key which by design can only be verified on the local platform via
the SGX ENCLU(EVERIFYREPORT2) instruction. So to create a remotely
verifiable attestation, the TDREPORT_STRUCT should be converted into a
Quote signed by a certified Quote signing key.

An Intel SGX Quoting Enclave (QE), written specifically to support
quoting Intel TDX TDs, uses EVERIFYREPORT2, to help check the integrity
of the TDG.MR.REPORT. If it passes, the QE can use a certified quote
signing key to sign a quote containing the guest TD’s measurements and
the additional data being quoted.

Method of sending the TDREPORT to QE and to get the Quote depends on
the implementation and deployment of QE. For quote requests from the
guest user space, communication models like vsock or TCP/IP can be
used to get the Quote from QE. But for requests generated during
pre-boot time, since such communication methods may not be available,
QEs at basic had to support GetQuote hypercall model. To keep the
attestation communication protocol same during pre-boot and
post-boot time, some QEs may just opt only to support GetQuote
hypercall request model. So add support to allow guest TD user space
get the Quote using GetQuote hypercall.

Since the Quote generation involves communication with QE, the response
time cannot be accurately defined and is generally long. So to let the TD
guest not wait for the response, host VMM supports callback event
notification via an interrupt vector. This can be used by the host to
notify the TD regarding the completion of the Quote request.
    
Following patches add the attestation support to the TDX guest which
includes attestation user interface driver, related hypercall
support and self test modules.

Patch 1-2/6 -> Adds TDREPORT support and related selftest.
Patch 3/6   -> Adds event notification IRQ support
Patch 4/6   -> Adds cc_decrypted_alloc/free() support
Patch 5-6/6 -> Adds Quote generation support and related selftest.

Any distribution enabling TDX is also expected to need attestation. So
enable it by default with TDX guest support. The compiled size is
quite small (~500 bytes).

Changes since v8:
 * To keep the interface generic to TDX guest, changed interface device
   name from tdx-attest -> tdx-guest. Also moved interface device creation
   logic from attest.c -> tdx.c.
 * Converted test app into self test module as per review suggestion.
 * Added subtype and length parameters to tdx_report_req ABI.
 * Regarding event notification IRQ, changed static IRQ vector
   reservation to runtime model.
 * For quote generation shared buffer allocation, replace vmap based
   allocation with DMA wrappers (cc_decrypted_alloc/free()).
 * Fixed commit log as per review comments.

Changes since v7:
 * Removed exports symbols for tdx_setup_ev_notify_handler() and
   tdx_remove_ev_notify_handler().
 * Changed "struct quote_buf *buf" member in "struct quote_entry"
   from pointer to embed object.
 * Rebased on top of v5.19-rc1.

Changes since v6:
 * Fixed race between wait_for_completion_*() and
   quote_callback_handler() in tdx_get_quote() when user terminates the
   request.
 * Fixed commit log and comments.

Changes since v5:
 * Added support for parallel GetQuote requests.
 * Add noalias variants of set_memory_*crypted() functions to
   changes page attribute without touching direct map.
 * Made set_memory_*crypted() functions vmalloc address compatible.
 * Use vmap()/set_memory_*crypted() functions to share/unshare
   memory without touching the direct map.
 * Add support to let driver handle the memory cleanup for the
   early termination of user requests.
 * Removed unused headers in attest.c
 * Fixed commit log and comments as per review comments.

Changes since v4:
 * Removed platform driver model in attestation driver and used
   miscdevice and initcall approach.
 * Since dma_alloc*() APIs require a valid device reference,
   replaced it with __get_free_pages() and set_memory_decrypted()
   for quote memory allocation.
 * Removed tdx_mcall_tdreport() and moved TDG.MR.REPORT TDCALL code
   to tdx_get_report().
 * Used kmalloc() for TDREPORT memory allocation instead of
   get_zeroed_page().
 * Returned -EINVAL in default case of tdx_attest_ioctl().
 * Added struct tdx_report_req to explicitly mention the
   TDX_CMD_GET_REPORT IOCTL argument.
 * Removed tdx_get_quote_hypercall() and moved hypercall code to
   attestation driver itself.
 * Removed GetQuote timeout support (since it is not defined in
   spec)
 * Added support to check for spurious callback interrupt in GetQuote
   request.
 * Fixed commit log and comments as per review suggestions.
   

Changes since v3:
 * Moved the attestation driver from platform/x86 to arch/x86/coco/tdx/ and
   renamed intel_tdx_attest.c to attest.c.
 * Dropped CONFIG_INTEL_TDX_ATTESTATION and added support to compile
   attestation changes with CONFIG_INTEL_TDX_GUEST option.
 * Merged patch titled "x86/tdx: Add tdx_mcall_tdreport() API support" and
   "platform/x86: intel_tdx_attest: Add TDX Guest attestation interface" into
   a single patch.
 * Moved GetQuote IOCTL support changes from patch titled "platform/x86:
   intel_tdx_attest: Add TDX Guest attestation interface driver" to a
   separate patch.
 * Removed 8K size restriction when requesting quote, and added support
   to let userspace decide the quote size.
 * Added support to allow attestation agent configure quote generation
   timeout value.
 * Fixed commit log and comments as per review comments.

Changes since v2:
 * As per Han's suggestion, modified the attestation driver to use
   platform device driver model.
 * Modified tdx_hcall_get_quote() and tdx_mcall_tdreport() APIs to
   return TDCALL error code instead of generic error info (like -EIO).
 * Removed attestation test app patch from this series to simplify
   the patchset and review process. Test app patches will be submitted
   once attestation support patches are merged.
 * Since patches titled "x86/tdx: Add SetupEventNotifyInterrupt TDX
   hypercall support" and "x86/tdx: Add TDX Guest event notify
   interrupt vector support" are related, combining them into a
   single patch.

Changes since v1:
 * Moved test driver from "tools/tdx/attest/tdx-attest-test.c" to
   "tools/arch/x86/tdx/attest/tdx-attest-test.c" as per Hans review
   suggestion.
 * Minor commit log and comment fixes in patches titled
   "x86/tdx: Add tdx_mcall_tdreport() API support" and "x86/tdx:
   Add tdx_hcall_get_quote() API support"
 * Extended tdx_hcall_get_quote() API to accept GPA length as argument
   to accomodate latest TDQUOTE TDVMCALL related specification update.
 * Added support for tdx_setup_ev_notify_handler() and
   tdx_remove_ev_notify_handler() in patch titled "x86/tdx: Add TDX
   Guest event notify interrupt vector support"


Kuppuswamy Sathyanarayanan (6):
  x86/tdx: Add TDX Guest attestation interface driver
  selftests: tdx: Test GetReport TDX attestation feature
  x86/tdx: Add TDX Guest event notify interrupt support
  x86/coco: Add cc_decrypted_alloc/free() interfaces
  x86/tdx: Add Quote generation support
  selftests: tdx: Test GetQuote TDX attestation feature

 arch/x86/coco/Makefile                        |   2 +-
 arch/x86/coco/mem.c                           |  90 +++++
 arch/x86/coco/tdx/Makefile                    |   2 +-
 arch/x86/coco/tdx/attest.c                    | 363 ++++++++++++++++++
 arch/x86/coco/tdx/tdx.c                       | 149 +++++++
 arch/x86/coco/tdx/tdx.h                       |  11 +
 arch/x86/include/asm/coco.h                   |  10 +
 arch/x86/include/asm/tdx.h                    |   2 +
 arch/x86/include/uapi/asm/tdx.h               |  96 +++++
 tools/testing/selftests/Makefile              |   1 +
 tools/testing/selftests/tdx/Makefile          |   7 +
 tools/testing/selftests/tdx/tdx_attest_test.c | 214 +++++++++++
 12 files changed, 945 insertions(+), 2 deletions(-)
 create mode 100644 arch/x86/coco/mem.c
 create mode 100644 arch/x86/coco/tdx/attest.c
 create mode 100644 arch/x86/coco/tdx/tdx.h
 create mode 100644 arch/x86/include/uapi/asm/tdx.h
 create mode 100644 tools/testing/selftests/tdx/Makefile
 create mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c

-- 
2.25.1

[PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Kuppuswamy Sathyanarayanan]

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

During the TD launch, the initial contents and configuration of the TD
are recorded by the Intel TDX module in build time measurement register
(MRTD). It is a SHA384 digest created using data from TD private pages(
including TD firmware) and the configuration of the TD.

After TD build, run-time measurement registers (RTMRs)  can be used by
the guest TD software to extend the TD measurements. TDX supports 4
RTMR registers, and TDG.MR.RTMR.EXTEND TDCALL is used to update the
RTMR registers securely. RTMRs are mainly used to record measurements
related to sections like the kernel image, command line parameters,
initrd, ACPI tables, firmware data, configuration firmware volume (CFV)
of TDVF, etc. For more details, please refer to TDX Virtual Firmware
design specification, sec titled "TD Measurement".

At TD runtime, the Intel TDX module reuses the Intel SGX attestation
infrastructure to provide support for attesting to these measurements
as described below.

The attestation process consists of two steps: TDREPORT generation and
Quote generation.

TDREPORT (TDREPORT_STRUCT) is a fixed-size data structure generated by
the TDX module which contains TD-specific information (such as TD
measurements), platform security version, and the MAC to protect the
integrity of the TDREPORT. The TD kernel uses TDCALL[TDG.MR.REPORT] to
get the TDREPORT from the TDX module. A user-provided 64-Byte
REPORTDATA is used as input and included in the TDREPORT. Typically it
can be some nonce provided by attestation service so the TDREPORT can
be verified uniquely. More details about TDREPORT can be found in
Intel TDX Module specification, section titled "TDG.MR.REPORT Leaf".

TDREPORT can only be verified on local platform as the MAC key is bound
to the platform. To support remote verification of the TDREPORT, TDX
leverages Intel SGX Quote Enclave (QE) to verify the TDREPORT locally
and convert it to a remote verifiable Quote.

After getting the TDREPORT, the second step of the attestation process
is to send it to the QE to generate the Quote. TDX doesn't support SGX
inside the TD, so the QE can be deployed in the host, or in another
legacy VM with SGX support. How to send the TDREPORT to QE and receive
the Quote is implementation and deployment specific.

Implement a basic TD guest misc driver to allow TD userspace to get the
TDREPORT. The TD userspace attestation software can get the TDREPORT
and then choose whatever communication channel available (i.e. vsock
or hypercall) to send the TDREPORT to QE and receive the Quote.

Also note that explicit access permissions are not enforced in this
driver because the quote and measurements are not a secret. However
the access permissions of the device node can be used to set any
desired access policy. The udev default is usually root access
only.

Operations like getting TDREPORT or Quote generation involves sending
a blob of data as input and getting another blob of data as output. It
was considered to use a sysfs interface for this, but it doesn't fit
well into the standard sysfs model for configuring values. It would be
possible to do read/write on files, but it would need multiple file
descriptors, which would be somewhat messy. IOCTLs seems to be the best
fitting and simplest model for this use case. This is similar to AMD
SEV platform, which also uses IOCTL interface to support attestation.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Kai Huang <kai.huang@intel.com>
Acked-by: Wander Lairson Costa <wander@redhat.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/Makefile      |  2 +-
 arch/x86/coco/tdx/attest.c      | 81 +++++++++++++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c         | 55 ++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.h         |  9 ++++
 arch/x86/include/uapi/asm/tdx.h | 51 +++++++++++++++++++++
 5 files changed, 197 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/coco/tdx/attest.c
 create mode 100644 arch/x86/coco/tdx/tdx.h
 create mode 100644 arch/x86/include/uapi/asm/tdx.h

diff --git a/arch/x86/coco/tdx/Makefile b/arch/x86/coco/tdx/Makefile
index 46c55998557d..d2db3e6770e5 100644
--- a/arch/x86/coco/tdx/Makefile
+++ b/arch/x86/coco/tdx/Makefile
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0
 
-obj-y += tdx.o tdcall.o
+obj-y += tdx.o tdcall.o attest.o
diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
new file mode 100644
index 000000000000..46a2f3612753
--- /dev/null
+++ b/arch/x86/coco/tdx/attest.c
@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * attest.c - TDX attestation feature support.
+ *
+ * Implements attestation related IOCTL handlers.
+ *
+ * Copyright (C) 2022 Intel Corporation
+ *
+ */
+
+#include <linux/mm.h>
+#include <linux/io.h>
+#include <asm/tdx.h>
+
+#include "tdx.h"
+
+/* TDREPORT module call leaf ID */
+#define TDX_GET_REPORT			4
+
+long tdx_get_report(void __user *argp)
+{
+	u8 *reportdata = NULL, *tdreport = NULL;
+	struct tdx_report_req req;
+	long ret;
+
+	/* Copy request struct from the user buffer */
+	if (copy_from_user(&req, argp, sizeof(req)))
+		return -EFAULT;
+
+	/*
+	 * Per TDX Module 1.0 specification, section titled
+	 * "TDG.MR.REPORT", REPORTDATA and TDREPORT length
+	 * is fixed as TDX_REPORTDATA_LEN and TDX_REPORT_LEN.
+	 */
+	if (req.rpd_len != TDX_REPORTDATA_LEN || req.tdr_len != TDX_REPORT_LEN)
+		return -EINVAL;
+
+	/* Allocate kernel buffers for REPORTDATA and TDREPORT */
+	reportdata = kzalloc(req.rpd_len, GFP_KERNEL);
+	if (!reportdata) {
+		ret = -ENOMEM;
+		goto report_failed;
+	}
+
+	tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
+	if (!tdreport) {
+		ret = -ENOMEM;
+		goto report_failed;
+	}
+
+
+	/* Copy REPORTDATA from user to kernel buffer */
+	if (copy_from_user(reportdata, (void *)req.reportdata, req.rpd_len)) {
+		ret = -EFAULT;
+		goto report_failed;
+	}
+
+	/*
+	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
+	 *
+	 * Get the TDREPORT using REPORTDATA as input. Refer to
+	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
+	 * Specification for detailed information.
+	 */
+	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
+				virt_to_phys(reportdata), req.subtype,
+				0, NULL);
+	if (ret) {
+		ret = -EIO;
+		goto report_failed;
+	}
+
+	/* Copy TDREPORT data back to the user buffer */
+	if (copy_to_user((void *)req.tdreport, tdreport, req.tdr_len))
+		ret = -EFAULT;
+
+report_failed:
+	kfree(reportdata);
+	kfree(tdreport);
+	return ret;
+}
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 928dcf7a20d9..205f98f42da8 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -5,6 +5,9 @@
 #define pr_fmt(fmt)     "tdx: " fmt
 
 #include <linux/cpufeature.h>
+#include <linux/miscdevice.h>
+#include <linux/mm.h>
+#include <linux/io.h>
 #include <asm/coco.h>
 #include <asm/tdx.h>
 #include <asm/vmx.h>
@@ -12,6 +15,8 @@
 #include <asm/insn-eval.h>
 #include <asm/pgtable.h>
 
+#include "tdx.h"
+
 /* TDX module Call Leaf IDs */
 #define TDX_GET_INFO			1
 #define TDX_GET_VEINFO			3
@@ -34,6 +39,10 @@
 #define VE_GET_PORT_NUM(e)	((e) >> 16)
 #define VE_IS_IO_STRING(e)	((e) & BIT(4))
 
+#define DRIVER_NAME	"tdx-guest"
+
+static struct miscdevice tdx_misc_dev;
+
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
  * return code.
@@ -775,3 +784,49 @@ void __init tdx_early_init(void)
 
 	pr_info("Guest detected\n");
 }
+
+static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
+			    unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	long ret = -EINVAL;
+
+	switch (cmd) {
+	case TDX_CMD_GET_REPORT:
+		ret = tdx_get_report(argp);
+		break;
+	default:
+		pr_debug("cmd %d not supported\n", cmd);
+		break;
+	}
+
+	return ret;
+}
+
+static const struct file_operations tdx_guest_fops = {
+	.owner		= THIS_MODULE,
+	.unlocked_ioctl	= tdx_guest_ioctl,
+	.llseek		= no_llseek,
+};
+
+static int __init tdx_guest_init(void)
+{
+	int ret;
+
+	/* Make sure we are in a valid TDX platform */
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return -EIO;
+
+	tdx_misc_dev.name = DRIVER_NAME;
+	tdx_misc_dev.minor = MISC_DYNAMIC_MINOR;
+	tdx_misc_dev.fops = &tdx_guest_fops;
+
+	ret = misc_register(&tdx_misc_dev);
+	if (ret) {
+		pr_err("misc device registration failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+device_initcall(tdx_guest_init)
diff --git a/arch/x86/coco/tdx/tdx.h b/arch/x86/coco/tdx/tdx.h
new file mode 100644
index 000000000000..67231d6d1027
--- /dev/null
+++ b/arch/x86/coco/tdx/tdx.h
@@ -0,0 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __X86_COCO_TDX_H__
+#define __X86_COCO_TDX_H__
+
+#include <uapi/asm/tdx.h>
+
+long tdx_get_report(void __user *argp);
+
+#endif /* __X86_COCO_TDX_H__ */
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
new file mode 100644
index 000000000000..20db8081772b
--- /dev/null
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_ASM_X86_TDX_H
+#define _UAPI_ASM_X86_TDX_H
+
+#include <linux/types.h>
+#include <linux/ioctl.h>
+
+/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORTDATA_LEN		64
+
+/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
+#define TDX_REPORT_LEN			1024
+
+/**
+ * struct tdx_report_req: Get TDREPORT using REPORTDATA as input.
+ *
+ * @subtype        : Subtype of TDREPORT (fixed as 0 by TDX Module
+ *                   specification, but added a parameter to handle
+ *                   future extension).
+ * @reportdata     : User-defined REPORTDATA to be included into
+ *                   TDREPORT. Typically it can be some nonce
+ *                   provided by attestation service, so the
+ *                   generated TDREPORT can be uniquely verified.
+ * @rpd_len        : Length of the REPORTDATA (fixed as 64 bytes by
+ *                   the TDX Module specification, but parameter is
+ *                   added to handle future extension).
+ * @tdreport       : TDREPORT output from TDCALL[TDG.MR.REPORT].
+ * @tdr_len        : Length of the TDREPORT (fixed as 1024 bytes by
+ *                   the TDX Module specification, but a parameter
+ *                   is added to accommodate future extension).
+ *
+ * Used in TDX_CMD_GET_REPORT IOCTL request.
+ */
+struct tdx_report_req {
+	__u8  subtype;
+	__u64 reportdata;
+	__u32 rpd_len;
+	__u64 tdreport;
+	__u32 tdr_len;
+};
+
+/*
+ * TDX_CMD_GET_REPORT - Get TDREPORT using TDCALL[TDG.MR.REPORT]
+ *
+ * Return 0 on success, -EIO on TDCALL execution failure, and
+ * standard errno on other general error cases.
+ *
+ */
+#define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, __u64)
+
+#endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

[PATCH v9 2/6] selftests: tdx: Test GetReport TDX attestation feature

[Kuppuswamy Sathyanarayanan]

In TDX guest, attestation is used to verify the trustworthiness of a
TD. During the TD bring-up, Intel TDX module measures and records the
initial contents and configuration of TD, and at runtime, TD software
uses runtime measurement registers (RMTRs) to measure and record
details related to kernel image, command line params, ACPI tables,
initrd, etc. At TD runtime, Intel SGX attestation infrastructure is
re-used to attest to these measurement data.

First step in the TDX attestation process is to get the TDREPORT data.
It is fixed size data structure generated by the TDX module which
includes the above mentioned measurements data, a MAC to protect the
integerity of the TDREPORT, and a 64-Byte of user specified data passed
during TDREPORT request which can uniquely identify the TDREPORT.

Intel's TDX guest driver exposes TDX_CMD_GET_REPORT IOCTL interface to
get the TDREPORT from the user space.

Add a kernel selftest module to test this ABI and verify the validity
of generated TDREPORT.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@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/Makefile              |   1 +
 tools/testing/selftests/tdx/Makefile          |   7 +
 tools/testing/selftests/tdx/tdx_attest_test.c | 160 ++++++++++++++++++
 3 files changed, 168 insertions(+)
 create mode 100644 tools/testing/selftests/tdx/Makefile
 create mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index de11992dc577..807a839d69c4 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -69,6 +69,7 @@ TARGETS += sync
 TARGETS += syscall_user_dispatch
 TARGETS += sysctl
 TARGETS += tc-testing
+TARGETS += tdx
 TARGETS += timens
 ifneq (1, $(quicktest))
 TARGETS += timers
diff --git a/tools/testing/selftests/tdx/Makefile b/tools/testing/selftests/tdx/Makefile
new file mode 100644
index 000000000000..281db209f9d6
--- /dev/null
+++ b/tools/testing/selftests/tdx/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+
+CFLAGS += -O3 -Wl,-no-as-needed -Wall -static
+
+TEST_GEN_PROGS := tdx_attest_test
+
+include ../lib.mk
diff --git a/tools/testing/selftests/tdx/tdx_attest_test.c b/tools/testing/selftests/tdx/tdx_attest_test.c
new file mode 100644
index 000000000000..7155cc751eaa
--- /dev/null
+++ b/tools/testing/selftests/tdx/tdx_attest_test.c
@@ -0,0 +1,160 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Test TDX attestation feature
+ *
+ * Copyright (C) 2022 Intel Corporation. All rights reserved.
+ *
+ * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
+ */
+
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/ioctl.h>
+#include <sys/time.h>
+#include <sys/types.h>
+#include <time.h>
+#include <unistd.h>
+
+#include "../kselftest_harness.h"
+#include "../../../../arch/x86/include/uapi/asm/tdx.h"
+
+#define devname         "/dev/tdx-guest"
+#define HEX_DUMP_SIZE	8
+
+/*
+ * struct td_info - It contains the measurements and initial configuration of
+ * the TD that was locked at initialization and a set of measurement
+ * registers that are run-time extendable. These values are copied from the
+ * TDCS by the TDG.MR.REPORT function.
+ */
+struct td_info {
+	/* TD attributes (like debug, spet_disable, etc) */
+	__u8 attr[8];
+	__u64 xfam;
+	/* Measurement registers */
+	__u64 mrtd[6];
+	__u64 mrconfigid[6];
+	__u64 mrowner[6];
+	__u64 mrownerconfig[6];
+	/* Runtime measurement registers */
+	__u64 rtmr[24];
+	__u64 reserved[14];
+};
+
+/*
+ * Trusted Execution Environment (TEE) report (TDREPORT_STRUCT) type,
+ * sub type and version..
+ */
+struct tdreport_type {
+	/* 0 - SGX, 81 -TDX, rest are reserved */
+	__u8 type;
+	/* Default value is 0 */
+	__u8 sub_type;
+	/* Default value is 0 */
+	__u8 version;
+	__u8 reserved;
+};
+
+/*
+ * struct reportmac - First field in the TEE report structure
+ * (TRDREPORT_STRUCT). It is common to Intel’s TEE's e.g., SGX and TDX.
+ * It is MAC-protected and contains hashes of the remainder of the report
+ * structure which includes the TEE’s measurements, and where applicable,
+ * the measurements of additional TCB elements not reflected in CPUSVN –
+ * e.g., a SEAM’s measurements.
+ */
+struct reportmac {
+	struct tdreport_type type;
+	__u8 reserved1[12];
+	/* CPU security version */
+	__u8 cpu_svn[16];
+	/* SHA384 hash of TEE TCB INFO */
+	__u8 tee_tcb_info_hash[48];
+	/* SHA384 hash of TDINFO_STRUCT */
+	__u8 tee_td_info_hash[48];
+	/* User defined unique data passed in TDG.MR.REPORT request */
+	__u8 reportdata[64];
+	__u8 reserved2[32];
+	__u8 mac[32];
+};
+
+struct tee_tcb_info {
+	__u8 data[239];
+};
+
+struct tdreport_data {
+	struct reportmac _reportmac;
+	struct tee_tcb_info _tcb_info;
+	__u8 reserved[17];
+	struct td_info _tdinfo;
+};
+
+#ifdef DEBUG
+static void print_array_hex(const char *title, const char *prefix_str,
+		const void *buf, int len)
+{
+	const __u8 *ptr = buf;
+	int i, rowsize = HEX_DUMP_SIZE;
+
+	if (!len || !buf)
+		return;
+
+	printf("\t\t%s", title);
+
+	for (i = 0; i < len; i++) {
+		if (!(i % rowsize))
+			printf("\n%s%.8x:", prefix_str, i);
+		printf(" %.2x", ptr[i]);
+	}
+
+	printf("\n");
+}
+#endif
+
+TEST(verify_report)
+{
+	__u8 reportdata[TDX_REPORTDATA_LEN];
+	struct tdreport_data *tdr_data;
+	__u8 tdreport[TDX_REPORT_LEN];
+	struct tdx_report_req req;
+	int devfd, i;
+
+	devfd = open(devname, O_RDWR | O_SYNC);
+
+	ASSERT_LT(0, devfd);
+
+	/* Generate sample report data */
+	for (i = 0; i < TDX_REPORTDATA_LEN; i++)
+		reportdata[i] = i;
+
+	/* Initialize IOCTL request */
+	req.subtype     = 0;
+	req.reportdata  = (__u64)reportdata;
+	req.rpd_len     = TDX_REPORTDATA_LEN;
+	req.tdreport    = (__u64)tdreport;
+	req.tdr_len     = TDX_REPORT_LEN;
+
+	/* Get TDREPORT */
+	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT, &req));
+
+	tdr_data = (struct tdreport_data *)tdreport;
+
+#ifdef DEBUG
+	print_array_hex("\n\t\tTDX report data\n", "", reportdata,
+			sizeof(reportdata));
+
+	print_array_hex("\n\t\tTDX tdreport data\n", "", &tdreport,
+			sizeof(tdreport));
+#endif
+
+	/* Make sure TDREPORT data includes the REPORTDATA passed */
+	ASSERT_EQ(0, memcmp(&tdr_data->_reportmac.reportdata[0], reportdata,
+				sizeof(reportdata)));
+
+	ASSERT_EQ(0, close(devfd));
+}
+
+TEST_HARNESS_MAIN
-- 
2.25.1

[PATCH v9 3/6] 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 to the VMM. Details about the SetupEventNotifyInterrupt
hypercall can be found in TDX Guest-Host Communication Interface
(GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
Add a tdx_hcall_set_notify_intr() helper function to implement the
SetupEventNotifyInterrupt hypercall.

As per design VMM will post the event completion IRQ using the same CPU
in 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 current CPU.

Please note that this patch only reserves the IRQ number. It is
expected that the user of event notify IRQ (like GetQuote handler) will
directly register the handler for "tdx_notify_irq" IRQ by using
request_irq() with  IRQF_SHARED flag set. Using IRQF_SHARED will enable
multiple users to use the same IRQ for event notification.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@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>
---
 arch/x86/coco/tdx/tdx.c    | 84 ++++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/tdx.h |  2 +
 2 files changed, 86 insertions(+)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 205f98f42da8..3563b208979c 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -8,12 +8,16 @@
 #include <linux/miscdevice.h>
 #include <linux/mm.h>
 #include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/numa.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>
 
 #include "tdx.h"
 
@@ -24,6 +28,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
 
 /* MMIO direction */
 #define EPT_READ	0
@@ -42,6 +47,7 @@
 #define DRIVER_NAME	"tdx-guest"
 
 static struct miscdevice tdx_misc_dev;
+int tdx_notify_irq = -1;
 
 /*
  * Wrapper for standard use of __tdx_hypercall with no output aside from
@@ -107,6 +113,31 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
 		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
 }
 
+/*
+ * tdx_hcall_set_notify_intr() - Setup Event Notify Interrupt Vector.
+ *
+ * @vector: Vector address to be used for notification.
+ *
+ * return 0 on success or failure error number.
+ */
+static long tdx_hcall_set_notify_intr(u8 vector)
+{
+	/* Minimum vector value allowed is 32 */
+	if (vector < 32)
+		return -EINVAL;
+
+	/*
+	 * 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, vector, 0, 0, 0))
+		return -EIO;
+
+	return 0;
+}
+
 static u64 get_cc_mask(void)
 {
 	struct tdx_module_output out;
@@ -830,3 +861,56 @@ static int __init tdx_guest_init(void)
 	return 0;
 }
 device_initcall(tdx_guest_init)
+
+/* Reserve an IRQ from x86_vector_domain for TD event notification */
+static int __init tdx_arch_init(void)
+{
+	struct irq_alloc_info info;
+	struct irq_cfg *cfg;
+	int cpu;
+
+	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+		return 0;
+
+	/* Make sure x86 vector domain is initialized */
+	if (!x86_vector_domain) {
+		pr_err("x86 vector domain is NULL\n");
+		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 = get_cpu();
+
+	info.mask = cpumask_of(cpu);
+
+	tdx_notify_irq = irq_domain_alloc_irqs(x86_vector_domain, 1,
+				NUMA_NO_NODE, &info);
+
+	if (tdx_notify_irq < 0) {
+		pr_err("Event notification IRQ allocation failed %d\n",
+				tdx_notify_irq);
+		goto init_failed;
+	}
+
+	irq_set_handler(tdx_notify_irq, handle_edge_irq);
+
+	cfg = irq_cfg(tdx_notify_irq);
+	if (!cfg) {
+		pr_err("Event notification IRQ config not found\n");
+		goto init_failed;
+	}
+
+	if (tdx_hcall_set_notify_intr(cfg->vector))
+		pr_err("Setting event notification interrupt failed\n");
+
+init_failed:
+	put_cpu();
+	return 0;
+}
+arch_initcall(tdx_arch_init);
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 020c81a7c729..8f933e67547f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -67,6 +67,8 @@ void tdx_safe_halt(void);
 
 bool tdx_early_handle_ve(struct pt_regs *regs);
 
+extern int tdx_notify_irq;
+
 #else
 
 static inline void tdx_early_init(void) { };
-- 
2.25.1

[PATCH v9 4/6] x86/coco: Add cc_decrypted_alloc/free() interfaces

[Kuppuswamy Sathyanarayanan]

Confidential computing platforms, such as AMD SEV and Intel TDX,
protect memory from VMM access. Any memory that is required for
communication with the VMM must be explicitly shared. It involves
adjusting page table entries to indicate that the memory is shared and
notifies VMM about the change.

set_memory_decrypted() converts memory to shared. Before freeing
memory it has to be converted back with set_memory_encrypted().

The interface works fine for long-term allocations, but for frequent
short-lived allocations it causes problems. Conversion takes time and
direct mapping modification leads to its fracturing and performance
degradation over time.

Direct mapping modifications can be avoided by creating a vmap that
maps allocated pages as shared while direct mapping is untouched.

But having private mapping of a shared memory causes problems too.
Any access of such memory via private mapping in TDX guest would
trigger unrecoverable SEPT violation and termination of the virtual
machine. It is known that load_unaligned_zeropad() can issue such
unwanted loads across page boundaries that can trigger the issue.

It can also be fixed by allocating a guard page in front of any memory
that has to be converted to shared, so load_unaligned_zeropad() will
roll off to the guard page instead. But it is wasteful and does not
address cost of the memory conversion.

The next logical step is to introduce a pool of shared memory that can
share a single guard page and makes conversion less frequent.

Fortunately, the kernel already has such a pool of memory: SWIOTLB
buffer is used by the DMA API to allocate memory for I/O. The buffer is
allocated once during the boot, so direct mapping fracturing is not an
issue and no need for vmap tricks.

Tapping into the SWIOTLB pool requires a device structure and using DMA
API. Provide a couple of simple helpers to allocate and free shared
memory that hide required plumbing.

Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/Makefile      |  2 +-
 arch/x86/coco/mem.c         | 90 +++++++++++++++++++++++++++++++++++++
 arch/x86/include/asm/coco.h | 10 +++++
 3 files changed, 101 insertions(+), 1 deletion(-)
 create mode 100644 arch/x86/coco/mem.c

diff --git a/arch/x86/coco/Makefile b/arch/x86/coco/Makefile
index c816acf78b6a..96fc4ec4497f 100644
--- a/arch/x86/coco/Makefile
+++ b/arch/x86/coco/Makefile
@@ -3,6 +3,6 @@ CFLAGS_REMOVE_core.o	= -pg
 KASAN_SANITIZE_core.o	:= n
 CFLAGS_core.o		+= -fno-stack-protector
 
-obj-y += core.o
+obj-y += core.o mem.o
 
 obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx/
diff --git a/arch/x86/coco/mem.c b/arch/x86/coco/mem.c
new file mode 100644
index 000000000000..78bcce11452e
--- /dev/null
+++ b/arch/x86/coco/mem.c
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Confidential Computing Decrypted Memory Allocator
+ *
+ * Copyright (C) 2022 Intel Corporation, Inc.
+ *
+ */
+
+#undef pr_fmt
+#define pr_fmt(fmt)     "cc/mem: " fmt
+
+#include <linux/export.h>
+#include <linux/mm.h>
+#include <linux/cc_platform.h>
+#include <linux/set_memory.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma-direct.h>
+
+#include <asm/coco.h>
+#include <asm/processor.h>
+
+#define CC_MEM_DRIVER		"ccmem"
+
+static struct platform_device *mem_pdev;
+
+static inline dma_addr_t virt_to_dma(void *vaddr)
+{
+	return phys_to_dma(&mem_pdev->dev, virt_to_phys(vaddr));
+}
+
+/* Allocate decrypted memory of given size */
+void *cc_decrypted_alloc(size_t size, gfp_t gfp)
+{
+	dma_addr_t handle;
+	void *vaddr;
+
+	if (!mem_pdev)
+		return NULL;
+
+	vaddr = dma_alloc_coherent(&mem_pdev->dev, size, &handle, gfp);
+
+	/*
+	 * Since we rely on virt_to_dma() in cc_decrypted_free() to
+	 * calculate DMA address, make sure address translation works.
+	 */
+	VM_BUG_ON(virt_to_dma(vaddr) != handle);
+
+	return vaddr;
+}
+
+/* Free the given decrypted memory */
+void cc_decrypted_free(void *addr, size_t size)
+{
+	if (!mem_pdev || !addr)
+		return;
+
+	dma_free_coherent(&mem_pdev->dev, size, addr, virt_to_phys(addr));
+}
+
+static struct platform_driver cc_mem_driver = {
+	.driver.name = CC_MEM_DRIVER,
+};
+
+static int __init cc_mem_init(void)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	if (!cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT))
+		return -ENODEV;
+
+	ret =  platform_driver_register(&cc_mem_driver);
+	if (ret)
+		return ret;
+
+	pdev = platform_device_register_simple(CC_MEM_DRIVER, -1, NULL, 0);
+	if (IS_ERR(pdev)) {
+		platform_driver_unregister(&cc_mem_driver);
+		return PTR_ERR(pdev);
+	}
+
+	if (dma_set_coherent_mask(&pdev->dev, DMA_BIT_MASK(64)))
+		return -EIO;
+
+	mem_pdev = pdev;
+
+	return 0;
+}
+device_initcall(cc_mem_init);
diff --git a/arch/x86/include/asm/coco.h b/arch/x86/include/asm/coco.h
index 3d98c3a60d34..74e10213289c 100644
--- a/arch/x86/include/asm/coco.h
+++ b/arch/x86/include/asm/coco.h
@@ -17,6 +17,8 @@ void cc_set_mask(u64 mask);
 #ifdef CONFIG_ARCH_HAS_CC_PLATFORM
 u64 cc_mkenc(u64 val);
 u64 cc_mkdec(u64 val);
+void *cc_decrypted_alloc(size_t size, gfp_t gfp);
+void cc_decrypted_free(void *addr, size_t size);
 #else
 static inline u64 cc_mkenc(u64 val)
 {
@@ -27,6 +29,14 @@ static inline u64 cc_mkdec(u64 val)
 {
 	return val;
 }
+
+static inline void *cc_decrypted_alloc(size_t size, gfp_t gfp)
+{
+	return NULL;
+}
+
+static inline void cc_decrypted_free(void *addr, size_t size) { }
+
 #endif
 
 #endif /* _ASM_X86_COCO_H */
-- 
2.25.1

[PATCH v9 5/6] x86/tdx: 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>.

Since GetQuote is an asynchronous request hypercall, it will not block
till the TD Quote is generated. So VMM uses callback interrupt vector
configured by SetupEventNotifyInterrupt hypercall to notify the guest
about Quote generation completion or failure.

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 without breaking the direct map, use
cc_decrypted_alloc/free() APIs.

Also note that, shared buffer allocation is currently handled in IOCTL
handler, although it will increase the TDX_CMD_GET_QUOTE IOCTL response
time, it is negligible compared to the time required for the quote
generation completion. So IOCTL performance optimization is not
considered at this time.

Add support for TDX_CMD_GET_QUOTE IOCTL to allow attestation agent
submit GetQuote requests from the user space. Since Quote generation
is an asynchronous request, IOCTL will block indefinitely for the VMM
response in wait_for_completion_interruptible() call. Using this call
will also add an option for the user to end the current request
prematurely by raising any signals. This can be used by attestation
agent to implement Quote generation timeout feature. If attestation
agent is aware of time it can validly wait for QE/QGS response, then
a possible timeout support can be implemented in the user application
using signals. Quote generation timeout feature is currently not
implemented in the driver because the current TDX specification does
not have any recommendation for it.

After submitting the GetQuote request using hypercall, the shared buffer
allocated for the current request is owned by the VMM. So, during this
wait window, if the user terminates the request by raising a signal or
by terminating the application, add a logic to do the memory cleanup
after receiving the VMM response at a later time.

To support parallel GetQuote requests, use linked list to track the
active GetQuote requests and upon receiving the callback IRQ, loop
through the active requests and mark the processed requests complete.
Users can open multiple instances of the attestation device and send
GetQuote requests in parallel.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Reviewed-by: Andi Kleen <ak@linux.intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---
 arch/x86/coco/tdx/attest.c      | 282 ++++++++++++++++++++++++++++++++
 arch/x86/coco/tdx/tdx.c         |  10 ++
 arch/x86/coco/tdx/tdx.h         |   2 +
 arch/x86/include/uapi/asm/tdx.h |  45 +++++
 4 files changed, 339 insertions(+)

diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
index 46a2f3612753..8fc7c6e52983 100644
--- a/arch/x86/coco/tdx/attest.c
+++ b/arch/x86/coco/tdx/attest.c
@@ -8,14 +8,56 @@
  *
  */
 
+#define pr_fmt(fmt) "x86/tdx: attest: " fmt
+
 #include <linux/mm.h>
 #include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/mutex.h>
 #include <asm/tdx.h>
+#include <asm/coco.h>
 
 #include "tdx.h"
 
 /* TDREPORT module call leaf ID */
 #define TDX_GET_REPORT			4
+/* GetQuote hypercall leaf ID */
+#define TDVMCALL_GET_QUOTE             0x10002
+
+/* Used for buffer allocation in GetQuote request */
+struct quote_buf {
+	/* Address of kernel buffer (size is page aligned) */
+	void *vmaddr;
+	/* Size of the allocated memory */
+	int size;
+};
+
+/* List entry of quote_list */
+struct quote_entry {
+	/* Flag to check validity of the GetQuote request */
+	bool valid;
+	/* Kernel buffer to share data with VMM */
+	struct quote_buf buf;
+	/* Completion object to track completion of GetQuote request */
+	struct completion compl;
+	struct list_head list;
+};
+
+/*
+ * To support parallel GetQuote requests, use the list
+ * to track active GetQuote requests.
+ */
+static LIST_HEAD(quote_list);
+
+/* Lock to protect quote_list */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * Workqueue to handle Quote data after Quote generation
+ * notification from VMM.
+ */
+struct workqueue_struct *quote_wq;
+struct work_struct quote_work;
 
 long tdx_get_report(void __user *argp)
 {
@@ -79,3 +121,243 @@ long tdx_get_report(void __user *argp)
 	kfree(tdreport);
 	return ret;
 }
+
+/* tdx_get_quote_hypercall() - Request to get TD Quote using TDREPORT */
+static long tdx_get_quote_hypercall(struct quote_buf *buf)
+{
+	struct tdx_hypercall_args args = {0};
+
+	args.r10 = TDX_HYPERCALL_STANDARD;
+	args.r11 = TDVMCALL_GET_QUOTE;
+	args.r12 = virt_to_phys(buf->vmaddr);
+	args.r13 = buf->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. More info about ABI
+	 * can be found in TDX Guest-Host-Communication Interface
+	 * (GHCI), sec titled "TDG.VP.VMCALL<GetQuote>".
+	 */
+	return __tdx_hypercall(&args, 0);
+}
+
+/*
+ * init_quote_buf() - Initialize the quote buffer by allocating
+ *                    a shared buffer of given size.
+ *
+ * Size is page aligned and the allocated memory is decrypted
+ * to allow VMM to access it.
+ */
+static int init_quote_buf(struct quote_buf *buf, u64 req_size)
+{
+	int size = PAGE_ALIGN(req_size);
+	void *vmaddr;
+
+	vmaddr = cc_decrypted_alloc(size, GFP_KERNEL);
+	if (!vmaddr)
+		return -ENOMEM;
+
+	buf->vmaddr = vmaddr;
+	buf->size = size;
+
+	return 0;
+}
+
+/* Free the decrypted memory */
+static void deinit_quote_buf(struct quote_buf *buf)
+{
+	cc_decrypted_free(buf->vmaddr, buf->size);
+}
+
+static struct quote_entry *alloc_quote_entry(u64 buf_len)
+{
+	struct quote_entry *entry = NULL;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return NULL;
+
+	/* Init buffer for quote request */
+	if (init_quote_buf(&entry->buf, buf_len)) {
+		pr_err("Shared buffer allocation failed\n");
+		kfree(entry);
+		return NULL;
+	}
+
+	init_completion(&entry->compl);
+	entry->valid = true;
+
+	return entry;
+}
+
+static void free_quote_entry(struct quote_entry *entry)
+{
+	deinit_quote_buf(&entry->buf);
+	kfree(entry);
+}
+
+/* Must be called with quote_lock held */
+static void _del_quote_entry(struct quote_entry *entry)
+{
+	list_del(&entry->list);
+	free_quote_entry(entry);
+}
+
+static void del_quote_entry(struct quote_entry *entry)
+{
+	mutex_lock(&quote_lock);
+	_del_quote_entry(entry);
+	mutex_unlock(&quote_lock);
+}
+
+/* Handles early termination of GetQuote requests */
+static void terminate_quote_request(struct quote_entry *entry)
+{
+	struct tdx_quote_hdr *quote_hdr;
+
+	/*
+	 * For early termination, if the request is not yet
+	 * processed by VMM (GET_QUOTE_IN_FLIGHT), the VMM
+	 * still owns the shared buffer, so mark the request
+	 * invalid to let quote_callback_handler() handle the
+	 * memory cleanup function. If the request is already
+	 * processed, then do the cleanup and return.
+	 */
+
+	mutex_lock(&quote_lock);
+	quote_hdr = (struct tdx_quote_hdr *)entry->buf.vmaddr;
+	if (quote_hdr->status == GET_QUOTE_IN_FLIGHT) {
+		entry->valid = false;
+		mutex_unlock(&quote_lock);
+		return;
+	}
+	_del_quote_entry(entry);
+	mutex_unlock(&quote_lock);
+}
+
+long tdx_get_quote(void __user *argp)
+{
+	struct quote_entry *entry;
+	struct tdx_quote_req req;
+	struct quote_buf *buf;
+	long ret;
+
+	/* Copy GetQuote request struct from user buffer */
+	if (copy_from_user(&req, argp, sizeof(struct tdx_quote_req)))
+		return -EFAULT;
+
+	/* Make sure the length is valid */
+	if (!req.len) {
+		pr_err("Invalid Quote buffer length\n");
+		return -EINVAL;
+	}
+
+	entry = alloc_quote_entry(req.len);
+	if (!entry) {
+		pr_err("Quote entry allocation failed\n");
+		return -ENOMEM;
+	}
+
+	buf = &entry->buf;
+
+	/* Copy data (with TDREPORT) from user buffer to kernel Quote buffer */
+	if (copy_from_user(buf->vmaddr, (void __user *)req.buf, req.len)) {
+		free_quote_entry(entry);
+		return -EFAULT;
+	}
+
+	mutex_lock(&quote_lock);
+
+	/* Submit GetQuote Request */
+	ret = tdx_get_quote_hypercall(buf);
+	if (ret) {
+		mutex_unlock(&quote_lock);
+		pr_err("GetQuote hypercall failed, status:%lx\n", ret);
+		free_quote_entry(entry);
+		return -EIO;
+	}
+
+	/* Add current quote entry to quote_list to track active requests */
+	list_add_tail(&entry->list, &quote_list);
+
+	mutex_unlock(&quote_lock);
+
+	/* Wait for attestation completion */
+	ret = wait_for_completion_interruptible(&entry->compl);
+	if (ret < 0) {
+		pr_err("GetQuote request terminated\n");
+		terminate_quote_request(entry);
+		return -EINTR;
+	}
+
+	/*
+	 * If GetQuote request completed successfully, copy the result
+	 * back to the user and do the cleanup.
+	 */
+	if (copy_to_user((void __user *)req.buf, buf->vmaddr, req.len))
+		ret = -EFAULT;
+
+	/*
+	 * Reaching here means GetQuote request is processed
+	 * successfully. So do the cleanup and return 0.
+	 */
+	del_quote_entry(entry);
+
+	return 0;
+}
+
+static irqreturn_t attestation_callback_handler(int irq, void *dev_id)
+{
+	queue_work(quote_wq, &quote_work);
+	return IRQ_HANDLED;
+}
+
+static void quote_callback_handler(struct work_struct *work)
+{
+	struct tdx_quote_hdr *quote_hdr;
+	struct quote_entry *entry, *next;
+
+	/* Find processed quote request and mark it complete */
+	mutex_lock(&quote_lock);
+	list_for_each_entry_safe(entry, next, &quote_list, list) {
+		quote_hdr = (struct tdx_quote_hdr *)entry->buf.vmaddr;
+		if (quote_hdr->status == GET_QUOTE_IN_FLIGHT)
+			continue;
+		/*
+		 * If user invalidated the current request, remove the
+		 * entry from the quote list and free it. If the request
+		 * is still valid, mark it complete.
+		 */
+		if (entry->valid)
+			complete(&entry->compl);
+		else
+			_del_quote_entry(entry);
+	}
+	mutex_unlock(&quote_lock);
+}
+
+int __init tdx_attest_init(void *data)
+{
+	quote_wq = create_singlethread_workqueue("tdx_quote_handler");
+
+	INIT_WORK(&quote_work, quote_callback_handler);
+
+	/*
+	 * Register event notification IRQ to get Quote completion
+	 * notification. Since tdx_notify_irq is not specific to the
+	 * attestation feature, use IRQF_SHARED to make it shared IRQ.
+	 * Use IRQF_NOBALANCING to make sure the IRQ affinity will not
+	 * be changed.
+	 */
+	if (request_irq(tdx_notify_irq, attestation_callback_handler,
+				IRQF_NOBALANCING | IRQF_SHARED,
+				"tdx_quote_irq", data)) {
+		pr_err("notify IRQ request failed\n");
+		destroy_workqueue(quote_wq);
+		return -EIO;
+	}
+
+	return 0;
+}
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 3563b208979c..801d4f14fa27 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -826,6 +826,9 @@ static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
 	case TDX_CMD_GET_REPORT:
 		ret = tdx_get_report(argp);
 		break;
+	case TDX_CMD_GET_QUOTE:
+		ret = tdx_get_quote(argp);
+		break;
 	default:
 		pr_debug("cmd %d not supported\n", cmd);
 		break;
@@ -858,6 +861,13 @@ static int __init tdx_guest_init(void)
 		return ret;
 	}
 
+	ret = tdx_attest_init(&tdx_misc_dev);
+	if (ret) {
+		pr_err("attestation init failed\n");
+		misc_deregister(&tdx_misc_dev);
+		return ret;
+	}
+
 	return 0;
 }
 device_initcall(tdx_guest_init)
diff --git a/arch/x86/coco/tdx/tdx.h b/arch/x86/coco/tdx/tdx.h
index 67231d6d1027..9d40d2e4b680 100644
--- a/arch/x86/coco/tdx/tdx.h
+++ b/arch/x86/coco/tdx/tdx.h
@@ -5,5 +5,7 @@
 #include <uapi/asm/tdx.h>
 
 long tdx_get_report(void __user *argp);
+long tdx_get_quote(void __user *argp);
+int __init tdx_attest_init(void *data);
 
 #endif /* __X86_COCO_TDX_H__ */
diff --git a/arch/x86/include/uapi/asm/tdx.h b/arch/x86/include/uapi/asm/tdx.h
index 20db8081772b..2cbe42e959f9 100644
--- a/arch/x86/include/uapi/asm/tdx.h
+++ b/arch/x86/include/uapi/asm/tdx.h
@@ -48,4 +48,49 @@ struct tdx_report_req {
  */
 #define TDX_CMD_GET_REPORT		_IOWR('T', 0x01, __u64)
 
+/* 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
+
+/*
+ * Format of Quote data header. More details 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 {
+	/* Quote version, filled by TD */
+	__u64 version;
+	/* Status code of Quote request, filled by VMM */
+	__u64 status;
+	/* Length of TDREPORT, filled by TD */
+	__u32 in_len;
+	/* Length of Quote, filled by VMM */
+	__u32 out_len;
+	/* Actual Quote data or TDREPORT on input */
+	__u64 data[0];
+};
+
+/* struct tdx_quote_req: Request to generate TD Quote using TDREPORT
+ *
+ * @buf         : Pass user data that includes TDREPORT as input. 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_QUOTE - Get TD Quote from QE/QGS using GetQuote
+ *		       TDVMCALL.
+ *
+ * Returns 0 on success, -EINTR for interrupted request, and
+ * standard errono on other failures.
+ */
+#define TDX_CMD_GET_QUOTE		_IOR('T', 0x02, __u64)
+
 #endif /* _UAPI_ASM_X86_TDX_H */
-- 
2.25.1

[PATCH v9 6/6] 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>
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_attest_test.c | 74 ++++++++++++++++---
 1 file changed, 64 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/tdx/tdx_attest_test.c b/tools/testing/selftests/tdx/tdx_attest_test.c
index 7155cc751eaa..14125b97e308 100644
--- a/tools/testing/selftests/tdx/tdx_attest_test.c
+++ b/tools/testing/selftests/tdx/tdx_attest_test.c
@@ -23,6 +23,7 @@
 
 #define devname         "/dev/tdx-guest"
 #define HEX_DUMP_SIZE	8
+#define QUOTE_SIZE	8192
 
 /*
  * struct td_info - It contains the measurements and initial configuration of
@@ -114,17 +115,11 @@ static void print_array_hex(const char *title, const char *prefix_str,
 }
 #endif
 
-TEST(verify_report)
+/* Helper function to get TDREPORT */
+long get_tdreport(int devfd, __u8 *reportdata, __u8 *tdreport)
 {
-	__u8 reportdata[TDX_REPORTDATA_LEN];
-	struct tdreport_data *tdr_data;
-	__u8 tdreport[TDX_REPORT_LEN];
 	struct tdx_report_req req;
-	int devfd, i;
-
-	devfd = open(devname, O_RDWR | O_SYNC);
-
-	ASSERT_LT(0, devfd);
+	int i;
 
 	/* Generate sample report data */
 	for (i = 0; i < TDX_REPORTDATA_LEN; i++)
@@ -137,8 +132,22 @@ TEST(verify_report)
 	req.tdreport    = (__u64)tdreport;
 	req.tdr_len     = TDX_REPORT_LEN;
 
+	return ioctl(devfd, TDX_CMD_GET_REPORT, &req);
+}
+
+TEST(verify_report)
+{
+	__u8 reportdata[TDX_REPORTDATA_LEN];
+	struct tdreport_data *tdr_data;
+	__u8 tdreport[TDX_REPORT_LEN];
+	int devfd;
+
+	devfd = open(devname, O_RDWR | O_SYNC);
+
+	ASSERT_LT(0, devfd);
+
 	/* Get TDREPORT */
-	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT, &req));
+	ASSERT_EQ(0, get_tdreport(devfd, reportdata, tdreport));
 
 	tdr_data = (struct tdreport_data *)tdreport;
 
@@ -157,4 +166,49 @@ TEST(verify_report)
 	ASSERT_EQ(0, close(devfd));
 }
 
+TEST(verify_quote)
+{
+	__u8 reportdata[TDX_REPORTDATA_LEN];
+	struct tdx_quote_hdr *quote_hdr;
+	struct tdx_quote_req quote_req;
+	__u8 *quote_buf = NULL;
+	__u64 quote_buf_size;
+	int devfd;
+
+	/* Open attestation device */
+	devfd = open(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_tdreport(devfd, reportdata,
+				(__u8 *)&quote_hdr->data));
+
+	/* Fill GetQuote request */
+	quote_req.buf	  = (__u64)quote_buf;
+	quote_req.len	  = quote_buf_size;
+
+	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &quote_req));
+
+	/* Check whether GetQuote request is successful */
+	EXPECT_EQ(0, quote_hdr->status);
+
+	free(quote_buf);
+}
+
 TEST_HARNESS_MAIN
-- 
2.25.1

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

[Kai Huang]

On Wed, 2022-07-27 at 20:44 -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 to the VMM. Details about the SetupEventNotifyInterrupt
> hypercall can be found in TDX Guest-Host Communication Interface
> (GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
> Add a tdx_hcall_set_notify_intr() helper function to implement the
> SetupEventNotifyInterrupt hypercall.
> 
> As per design VMM will post the event completion IRQ using the same CPU
> in 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 current CPU.

Set the affinity to the CPU isn't good enough.  Please call out we need a non-
migratable IRQ here, i.e. userspace is not allowed to change the affinity which
can cause the vector being reallocated from another cpu.

> 
> Please note that this patch only reserves the IRQ number. It is
> expected that the user of event notify IRQ (like GetQuote handler) will
> directly register the handler for "tdx_notify_irq" IRQ by using
> request_irq() with  IRQF_SHARED flag set. Using IRQF_SHARED will enable
> multiple users to use the same IRQ for event notification.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@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>
> ---
>  arch/x86/coco/tdx/tdx.c    | 84 ++++++++++++++++++++++++++++++++++++++
>  arch/x86/include/asm/tdx.h |  2 +
>  2 files changed, 86 insertions(+)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 205f98f42da8..3563b208979c 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -8,12 +8,16 @@
>  #include <linux/miscdevice.h>
>  #include <linux/mm.h>
>  #include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/numa.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>
>  
>  #include "tdx.h"
>  
> @@ -24,6 +28,7 @@
>  
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA		0x10001
> +#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>  
>  /* MMIO direction */
>  #define EPT_READ	0
> @@ -42,6 +47,7 @@
>  #define DRIVER_NAME	"tdx-guest"
>  
>  static struct miscdevice tdx_misc_dev;
> +int tdx_notify_irq = -1;
>  
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
> @@ -107,6 +113,31 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>  		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>  }
>  
> +/*
> + * tdx_hcall_set_notify_intr() - Setup Event Notify Interrupt Vector.
> + *
> + * @vector: Vector address to be used for notification.
> + *
> + * return 0 on success or failure error number.
> + */
> +static long tdx_hcall_set_notify_intr(u8 vector)
> +{
> +	/* Minimum vector value allowed is 32 */
> +	if (vector < 32)
> +		return -EINVAL;
> +
> +	/*
> +	 * 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, vector, 0, 0, 0))
> +		return -EIO;
> +
> +	return 0;
> +}
> +

I don't think this function is super useful.  I guess it's just better to openly
call the _tdx_hypercall() directly in below.  With some comments, this way makes
the code more clear that we don't want any scheduling during the IRQ/vector
allocation and the hypervisor to guarantee the vector is always allocated on the
CPU where the hypercall is called.

Also, checking vector < 32 isn't needed, as the hypercall is guaranteed to
return error in such case as suggested by GHCI (otherwise it is VMM bug).

The irq_domain_alloc_irqs() call also guarantees the allocated vector will never
< 32.  If really needed, you can just add a WARN_ON(vector < 32) right before
calling the hypercall.

>  static u64 get_cc_mask(void)
>  {
>  	struct tdx_module_output out;
> @@ -830,3 +861,56 @@ static int __init tdx_guest_init(void)
>  	return 0;
>  }
>  device_initcall(tdx_guest_init)
> +
> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
> +static int __init tdx_arch_init(void)
> +{
> +	struct irq_alloc_info info;
> +	struct irq_cfg *cfg;
> +	int cpu;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return 0;
> +
> +	/* Make sure x86 vector domain is initialized */
> +	if (!x86_vector_domain) {
> +		pr_err("x86 vector domain is NULL\n");
> +		return 0;
> +	}

I don't think  this check is needed.  x86_vector_domain is guaranteed to be non-
NULL in arch_early_irq_init():

        x86_vector_domain = irq_domain_create_tree(...);

        BUG_ON(x86_vector_domain == NULL);

> +
> +	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 = get_cpu();
> +
> +	info.mask = cpumask_of(cpu);
> +
> +	tdx_notify_irq = irq_domain_alloc_irqs(x86_vector_domain, 1,
> +				NUMA_NO_NODE, &info);
> +
> +	if (tdx_notify_irq < 0) {
> +		pr_err("Event notification IRQ allocation failed %d\n",
> +				tdx_notify_irq);
> +		goto init_failed;
> +	}
> +
> +	irq_set_handler(tdx_notify_irq, handle_edge_irq);
> +
> +	cfg = irq_cfg(tdx_notify_irq);
> +	if (!cfg) {
> +		pr_err("Event notification IRQ config not found\n");
> +		goto init_failed;
> +	}
> +
> +	if (tdx_hcall_set_notify_intr(cfg->vector))
> +		pr_err("Setting event notification interrupt failed\n");

So the request_irq() with IRQF_NOBALANCING isn't in this patch but in later
patch.  Nor the IRQ is made affinity kernel-managed in this patch.  This means
for this patch alone, the IRQ is migratable (i.e. usespace can change its
affinity).  In another words, this patch depends on later patch to work.  I
don't think this is right/good.

Perhaps we can explicitly call irq_modify_status() to set IRQF_NOBALANCING here?

	irq_modify_status(tdx_notify_irq, 0, IRQ_NO_BALANCING);

But again, I am not expert here.  It would be helpful if some expert can help to
check whether this is good way to handle.


-- 
Thanks,
-Kai

Re: [PATCH v9 2/6] selftests: tdx: Test GetReport TDX attestation feature

[Kai Huang]

On Wed, 2022-07-27 at 20:44 -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is used to verify the trustworthiness of a
> TD. During the TD bring-up, Intel TDX module measures and records the
> initial contents and configuration of TD, and at runtime, TD software
> uses runtime measurement registers (RMTRs) to measure and record
> details related to kernel image, command line params, ACPI tables,
> initrd, etc. At TD runtime, Intel SGX attestation infrastructure is
> re-used to attest to these measurement data.
> 
> First step in the TDX attestation process is to get the TDREPORT data.
> It is fixed size data structure generated by the TDX module which
> includes the above mentioned measurements data, a MAC to protect the
> integerity of the TDREPORT, and a 64-Byte of user specified data passed
> during TDREPORT request which can uniquely identify the TDREPORT.
> 
> Intel's TDX guest driver exposes TDX_CMD_GET_REPORT IOCTL interface to
> get the TDREPORT from the user space.
> 
> Add a kernel selftest module to test this ABI and verify the validity
> of generated TDREPORT.
> 
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Reviewed-by: Andi Kleen <ak@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/Makefile              |   1 +
>  tools/testing/selftests/tdx/Makefile          |   7 +
>  tools/testing/selftests/tdx/tdx_attest_test.c | 160 ++++++++++++++++++
>  3 files changed, 168 insertions(+)
>  create mode 100644 tools/testing/selftests/tdx/Makefile
>  create mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
> 
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index de11992dc577..807a839d69c4 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -69,6 +69,7 @@ TARGETS += sync
>  TARGETS += syscall_user_dispatch
>  TARGETS += sysctl
>  TARGETS += tc-testing
> +TARGETS += tdx
>  TARGETS += timens
>  ifneq (1, $(quicktest))
>  TARGETS += timers
> diff --git a/tools/testing/selftests/tdx/Makefile b/tools/testing/selftests/tdx/Makefile
> new file mode 100644
> index 000000000000..281db209f9d6
> --- /dev/null
> +++ b/tools/testing/selftests/tdx/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -static
> +
> +TEST_GEN_PROGS := tdx_attest_test
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/tdx/tdx_attest_test.c b/tools/testing/selftests/tdx/tdx_attest_test.c
> new file mode 100644
> index 000000000000..7155cc751eaa
> --- /dev/null
> +++ b/tools/testing/selftests/tdx/tdx_attest_test.c
> @@ -0,0 +1,160 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Test TDX attestation feature
> + *
> + * Copyright (C) 2022 Intel Corporation. All rights reserved.
> + *
> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> + */
> +
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <sys/time.h>
> +#include <sys/types.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "../kselftest_harness.h"
> +#include "../../../../arch/x86/include/uapi/asm/tdx.h"
> +
> +#define devname         "/dev/tdx-guest"
> +#define HEX_DUMP_SIZE	8
> +
> +/*
> + * struct td_info - It contains the measurements and initial configuration of
> + * the TD that was locked at initialization and a set of measurement
> + * registers that are run-time extendable. These values are copied from the
> + * TDCS by the TDG.MR.REPORT function.
> + */
> +struct td_info {
> +	/* TD attributes (like debug, spet_disable, etc) */
> +	__u8 attr[8];
> +	__u64 xfam;
> +	/* Measurement registers */
> +	__u64 mrtd[6];
> +	__u64 mrconfigid[6];
> +	__u64 mrowner[6];
> +	__u64 mrownerconfig[6];
> +	/* Runtime measurement registers */
> +	__u64 rtmr[24];
> +	__u64 reserved[14];
> +};
> +
> +/*
> + * Trusted Execution Environment (TEE) report (TDREPORT_STRUCT) type,
> + * sub type and version..
> + */
> +struct tdreport_type {
> +	/* 0 - SGX, 81 -TDX, rest are reserved */
> +	__u8 type;
> +	/* Default value is 0 */
> +	__u8 sub_type;
> +	/* Default value is 0 */
> +	__u8 version;
> +	__u8 reserved;
> +};
> +
> +/*
> + * struct reportmac - First field in the TEE report structure
> + * (TRDREPORT_STRUCT). It is common to Intel’s TEE's e.g., SGX and TDX.
> + * It is MAC-protected and contains hashes of the remainder of the report
> + * structure which includes the TEE’s measurements, and where applicable,
> + * the measurements of additional TCB elements not reflected in CPUSVN –
> + * e.g., a SEAM’s measurements.
> + */
> +struct reportmac {
> +	struct tdreport_type type;
> +	__u8 reserved1[12];
> +	/* CPU security version */
> +	__u8 cpu_svn[16];
> +	/* SHA384 hash of TEE TCB INFO */
> +	__u8 tee_tcb_info_hash[48];
> +	/* SHA384 hash of TDINFO_STRUCT */
> +	__u8 tee_td_info_hash[48];
> +	/* User defined unique data passed in TDG.MR.REPORT request */
> +	__u8 reportdata[64];
> +	__u8 reserved2[32];
> +	__u8 mac[32];
> +};
> +
> +struct tee_tcb_info {
> +	__u8 data[239];
> +};
> +
> +struct tdreport_data {
> +	struct reportmac _reportmac;
> +	struct tee_tcb_info _tcb_info;
> +	__u8 reserved[17];
> +	struct td_info _tdinfo;
> +};

I think 'struct tdreport' is enough.  The _data postfix only causes it to be
more confusing.

Btw, as it appears you only verified reportdata below, is it worth to have all
those data structures (and they are used by hardware but not __packed)?  Perhaps
a macro to define REPORTDATA offset in TDREPORT is good enough?  Or maybe I am
missing something.

> +
> +#ifdef DEBUG
> +static void print_array_hex(const char *title, const char *prefix_str,
> +		const void *buf, int len)
> +{
> +	const __u8 *ptr = buf;
> +	int i, rowsize = HEX_DUMP_SIZE;
> +
> +	if (!len || !buf)
> +		return;
> +
> +	printf("\t\t%s", title);
> +
> +	for (i = 0; i < len; i++) {
> +		if (!(i % rowsize))
> +			printf("\n%s%.8x:", prefix_str, i);
> +		printf(" %.2x", ptr[i]);
> +	}
> +
> +	printf("\n");
> +}
> +#endif
> +
> +TEST(verify_report)
> +{
> +	__u8 reportdata[TDX_REPORTDATA_LEN];
> +	struct tdreport_data *tdr_data;
> +	__u8 tdreport[TDX_REPORT_LEN];
> +	struct tdx_report_req req;
> +	int devfd, i;
> +
> +	devfd = open(devname, O_RDWR | O_SYNC);
> +
> +	ASSERT_LT(0, devfd);
> +
> +	/* Generate sample report data */
> +	for (i = 0; i < TDX_REPORTDATA_LEN; i++)
> +		reportdata[i] = i;
> +
> +	/* Initialize IOCTL request */
> +	req.subtype     = 0;
> +	req.reportdata  = (__u64)reportdata;
> +	req.rpd_len     = TDX_REPORTDATA_LEN;
> +	req.tdreport    = (__u64)tdreport;
> +	req.tdr_len     = TDX_REPORT_LEN;
> +
> +	/* Get TDREPORT */
> +	ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT, &req));
> +
> +	tdr_data = (struct tdreport_data *)tdreport;
> +
> +#ifdef DEBUG
> +	print_array_hex("\n\t\tTDX report data\n", "", reportdata,
> +			sizeof(reportdata));
> +
> +	print_array_hex("\n\t\tTDX tdreport data\n", "", &tdreport,
> +			sizeof(tdreport));
> +#endif
> +
> +	/* Make sure TDREPORT data includes the REPORTDATA passed */
> +	ASSERT_EQ(0, memcmp(&tdr_data->_reportmac.reportdata[0], reportdata,
> +				sizeof(reportdata)));
> +
> +	ASSERT_EQ(0, close(devfd));
> +}
> +
> +TEST_HARNESS_MAIN

Re: [PATCH v9 2/6] selftests: tdx: Test GetReport TDX attestation feature

[Sathyanarayanan Kuppuswamy]

On 7/28/22 3:32 AM, Kai Huang wrote:
> On Wed, 2022-07-27 at 20:44 -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a
>> TD. During the TD bring-up, Intel TDX module measures and records the
>> initial contents and configuration of TD, and at runtime, TD software
>> uses runtime measurement registers (RMTRs) to measure and record
>> details related to kernel image, command line params, ACPI tables,
>> initrd, etc. At TD runtime, Intel SGX attestation infrastructure is
>> re-used to attest to these measurement data.
>>
>> First step in the TDX attestation process is to get the TDREPORT data.
>> It is fixed size data structure generated by the TDX module which
>> includes the above mentioned measurements data, a MAC to protect the
>> integerity of the TDREPORT, and a 64-Byte of user specified data passed
>> during TDREPORT request which can uniquely identify the TDREPORT.
>>
>> Intel's TDX guest driver exposes TDX_CMD_GET_REPORT IOCTL interface to
>> get the TDREPORT from the user space.
>>
>> Add a kernel selftest module to test this ABI and verify the validity
>> of generated TDREPORT.
>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@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/Makefile              |   1 +
>>  tools/testing/selftests/tdx/Makefile          |   7 +
>>  tools/testing/selftests/tdx/tdx_attest_test.c | 160 ++++++++++++++++++
>>  3 files changed, 168 insertions(+)
>>  create mode 100644 tools/testing/selftests/tdx/Makefile
>>  create mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index de11992dc577..807a839d69c4 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -69,6 +69,7 @@ TARGETS += sync
>>  TARGETS += syscall_user_dispatch
>>  TARGETS += sysctl
>>  TARGETS += tc-testing
>> +TARGETS += tdx
>>  TARGETS += timens
>>  ifneq (1, $(quicktest))
>>  TARGETS += timers
>> diff --git a/tools/testing/selftests/tdx/Makefile b/tools/testing/selftests/tdx/Makefile
>> new file mode 100644
>> index 000000000000..281db209f9d6
>> --- /dev/null
>> +++ b/tools/testing/selftests/tdx/Makefile
>> @@ -0,0 +1,7 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +CFLAGS += -O3 -Wl,-no-as-needed -Wall -static
>> +
>> +TEST_GEN_PROGS := tdx_attest_test
>> +
>> +include ../lib.mk
>> diff --git a/tools/testing/selftests/tdx/tdx_attest_test.c b/tools/testing/selftests/tdx/tdx_attest_test.c
>> new file mode 100644
>> index 000000000000..7155cc751eaa
>> --- /dev/null
>> +++ b/tools/testing/selftests/tdx/tdx_attest_test.c
>> @@ -0,0 +1,160 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Test TDX attestation feature
>> + *
>> + * Copyright (C) 2022 Intel Corporation. All rights reserved.
>> + *
>> + * Author: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> + */
>> +
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/ioctl.h>
>> +#include <sys/time.h>
>> +#include <sys/types.h>
>> +#include <time.h>
>> +#include <unistd.h>
>> +
>> +#include "../kselftest_harness.h"
>> +#include "../../../../arch/x86/include/uapi/asm/tdx.h"
>> +
>> +#define devname         "/dev/tdx-guest"
>> +#define HEX_DUMP_SIZE	8
>> +
>> +/*
>> + * struct td_info - It contains the measurements and initial configuration of
>> + * the TD that was locked at initialization and a set of measurement
>> + * registers that are run-time extendable. These values are copied from the
>> + * TDCS by the TDG.MR.REPORT function.
>> + */
>> +struct td_info {
>> +	/* TD attributes (like debug, spet_disable, etc) */
>> +	__u8 attr[8];
>> +	__u64 xfam;
>> +	/* Measurement registers */
>> +	__u64 mrtd[6];
>> +	__u64 mrconfigid[6];
>> +	__u64 mrowner[6];
>> +	__u64 mrownerconfig[6];
>> +	/* Runtime measurement registers */
>> +	__u64 rtmr[24];
>> +	__u64 reserved[14];
>> +};
>> +
>> +/*
>> + * Trusted Execution Environment (TEE) report (TDREPORT_STRUCT) type,
>> + * sub type and version..
>> + */
>> +struct tdreport_type {
>> +	/* 0 - SGX, 81 -TDX, rest are reserved */
>> +	__u8 type;
>> +	/* Default value is 0 */
>> +	__u8 sub_type;
>> +	/* Default value is 0 */
>> +	__u8 version;
>> +	__u8 reserved;
>> +};
>> +
>> +/*
>> + * struct reportmac - First field in the TEE report structure
>> + * (TRDREPORT_STRUCT). It is common to Intel’s TEE's e.g., SGX and TDX.
>> + * It is MAC-protected and contains hashes of the remainder of the report
>> + * structure which includes the TEE’s measurements, and where applicable,
>> + * the measurements of additional TCB elements not reflected in CPUSVN –
>> + * e.g., a SEAM’s measurements.
>> + */
>> +struct reportmac {
>> +	struct tdreport_type type;
>> +	__u8 reserved1[12];
>> +	/* CPU security version */
>> +	__u8 cpu_svn[16];
>> +	/* SHA384 hash of TEE TCB INFO */
>> +	__u8 tee_tcb_info_hash[48];
>> +	/* SHA384 hash of TDINFO_STRUCT */
>> +	__u8 tee_td_info_hash[48];
>> +	/* User defined unique data passed in TDG.MR.REPORT request */
>> +	__u8 reportdata[64];
>> +	__u8 reserved2[32];
>> +	__u8 mac[32];
>> +};
>> +
>> +struct tee_tcb_info {
>> +	__u8 data[239];
>> +};
>> +
>> +struct tdreport_data {
>> +	struct reportmac _reportmac;
>> +	struct tee_tcb_info _tcb_info;
>> +	__u8 reserved[17];
>> +	struct td_info _tdinfo;
>> +};
> 
> I think 'struct tdreport' is enough.  The _data postfix only causes it to be
> more confusing.

Ok.

> 
> Btw, as it appears you only verified reportdata below, is it worth to have all
> those data structures (and they are used by hardware but not __packed)?  Perhaps
> a macro to define REPORTDATA offset in TDREPORT is good enough?  Or maybe I am
> missing something.

I have added these data structs to make it easier for readers to understand
the contents of the TDREPORT. I thought a simple offset based check would look
like a magic number. If the maintainers are fine with offset based comparison,
I am ok with it.
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Sathyanarayanan Kuppuswamy]

On 7/28/22 3:18 AM, Kai Huang wrote:
> On Wed, 2022-07-27 at 20:44 -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 to the VMM. Details about the SetupEventNotifyInterrupt
>> hypercall can be found in TDX Guest-Host Communication Interface
>> (GHCI) Specification, sec 3.5 "VP.VMCALL<SetupEventNotifyInterrupt>".
>> Add a tdx_hcall_set_notify_intr() helper function to implement the
>> SetupEventNotifyInterrupt hypercall.
>>
>> As per design VMM will post the event completion IRQ using the same CPU
>> in 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 current CPU.
> 
> Set the affinity to the CPU isn't good enough.  Please call out we need a non-
> migratable IRQ here, i.e. userspace is not allowed to change the affinity which
> can cause the vector being reallocated from another cpu.

Agree. I will include the relevant details.

> 
>>
>> Please note that this patch only reserves the IRQ number. It is
>> expected that the user of event notify IRQ (like GetQuote handler) will
>> directly register the handler for "tdx_notify_irq" IRQ by using
>> request_irq() with  IRQF_SHARED flag set. Using IRQF_SHARED will enable
>> multiple users to use the same IRQ for event notification.


>>
>> Reviewed-by: Tony Luck <tony.luck@intel.com>
>> Reviewed-by: Andi Kleen <ak@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>
>> ---
>>  arch/x86/coco/tdx/tdx.c    | 84 ++++++++++++++++++++++++++++++++++++++
>>  arch/x86/include/asm/tdx.h |  2 +
>>  2 files changed, 86 insertions(+)
>>
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 205f98f42da8..3563b208979c 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -8,12 +8,16 @@
>>  #include <linux/miscdevice.h>
>>  #include <linux/mm.h>
>>  #include <linux/io.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/numa.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>
>>  
>>  #include "tdx.h"
>>  
>> @@ -24,6 +28,7 @@
>>  
>>  /* TDX hypercall Leaf IDs */
>>  #define TDVMCALL_MAP_GPA		0x10001
>> +#define TDVMCALL_SETUP_NOTIFY_INTR	0x10004
>>  
>>  /* MMIO direction */
>>  #define EPT_READ	0
>> @@ -42,6 +47,7 @@
>>  #define DRIVER_NAME	"tdx-guest"
>>  
>>  static struct miscdevice tdx_misc_dev;
>> +int tdx_notify_irq = -1;
>>  
>>  /*
>>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>> @@ -107,6 +113,31 @@ static inline void tdx_module_call(u64 fn, u64 rcx, u64 rdx, u64 r8, u64 r9,
>>  		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);
>>  }
>>  
>> +/*
>> + * tdx_hcall_set_notify_intr() - Setup Event Notify Interrupt Vector.
>> + *
>> + * @vector: Vector address to be used for notification.
>> + *
>> + * return 0 on success or failure error number.
>> + */
>> +static long tdx_hcall_set_notify_intr(u8 vector)
>> +{
>> +	/* Minimum vector value allowed is 32 */
>> +	if (vector < 32)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * 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, vector, 0, 0, 0))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
> 
> I don't think this function is super useful.  I guess it's just better to openly
> call the _tdx_hypercall() directly in below.  With some comments, this way makes
> the code more clear that we don't want any scheduling during the IRQ/vector
> allocation and the hypervisor to guarantee the vector is always allocated on the
> CPU where the hypercall is called.
> 
> Also, checking vector < 32 isn't needed, as the hypercall is guaranteed to
> return error in such case as suggested by GHCI (otherwise it is VMM bug).
> 
> The irq_domain_alloc_irqs() call also guarantees the allocated vector will never
> < 32.  If really needed, you can just add a WARN_ON(vector < 32) right before
> calling the hypercall.

Agree. I will move this to tdx_arch_init().

> 
>>  static u64 get_cc_mask(void)
>>  {
>>  	struct tdx_module_output out;
>> @@ -830,3 +861,56 @@ static int __init tdx_guest_init(void)
>>  	return 0;
>>  }
>>  device_initcall(tdx_guest_init)
>> +
>> +/* Reserve an IRQ from x86_vector_domain for TD event notification */
>> +static int __init tdx_arch_init(void)
>> +{
>> +	struct irq_alloc_info info;
>> +	struct irq_cfg *cfg;
>> +	int cpu;
>> +
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return 0;
>> +
>> +	/* Make sure x86 vector domain is initialized */
>> +	if (!x86_vector_domain) {
>> +		pr_err("x86 vector domain is NULL\n");
>> +		return 0;
>> +	}
> 
> I don't think  this check is needed.  x86_vector_domain is guaranteed to be non-
> NULL in arch_early_irq_init():
> 
>         x86_vector_domain = irq_domain_create_tree(...);
> 
>         BUG_ON(x86_vector_domain == NULL);

Makes sense. I can remove the above check.

> 
>> +
>> +	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 = get_cpu();
>> +
>> +	info.mask = cpumask_of(cpu);
>> +
>> +	tdx_notify_irq = irq_domain_alloc_irqs(x86_vector_domain, 1,
>> +				NUMA_NO_NODE, &info);
>> +
>> +	if (tdx_notify_irq < 0) {
>> +		pr_err("Event notification IRQ allocation failed %d\n",
>> +				tdx_notify_irq);
>> +		goto init_failed;
>> +	}
>> +
>> +	irq_set_handler(tdx_notify_irq, handle_edge_irq);
>> +
>> +	cfg = irq_cfg(tdx_notify_irq);
>> +	if (!cfg) {
>> +		pr_err("Event notification IRQ config not found\n");
>> +		goto init_failed;
>> +	}
>> +
>> +	if (tdx_hcall_set_notify_intr(cfg->vector))
>> +		pr_err("Setting event notification interrupt failed\n");
> 
> So the request_irq() with IRQF_NOBALANCING isn't in this patch but in later
> patch.  Nor the IRQ is made affinity kernel-managed in this patch.  This means
> for this patch alone, the IRQ is migratable (i.e. usespace can change its
> affinity).  In another words, this patch depends on later patch to work.  I
> don't think this is right/good.
> 
> Perhaps we can explicitly call irq_modify_status() to set IRQF_NOBALANCING here?
> 
> 	irq_modify_status(tdx_notify_irq, 0, IRQ_NO_BALANCING);


I just tested it. It seems to work fine. With the above change, I see 
IRQD_NO_BALANCING dstate flag (in /sys/kernel/debug/irq/irqs/xx). I will
include this change.


> 
> But again, I am not expert here.  It would be helpful if some expert can help to
> check whether this is good way to handle.
> 
> 



-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 2/6] selftests: tdx: Test GetReport TDX attestation feature

[Kai Huang]

On Mon, 2022-08-01 at 10:49 -0700, Sathyanarayanan Kuppuswamy wrote:
> > 
> > Btw, as it appears you only verified reportdata below, is it worth to have
> > all
> > those data structures (and they are used by hardware but not __packed)? 
> > Perhaps
> > a macro to define REPORTDATA offset in TDREPORT is good enough?  Or maybe I
> > am
> > missing something.
> 
> I have added these data structs to make it easier for readers to understand
> the contents of the TDREPORT. I thought a simple offset based check would look
> like a magic number. If the maintainers are fine with offset based comparison,
> I am ok with it.

They need to be __packed, at least.

-- 
Thanks,
-Kai

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:
> In TDX guest, attestation is used to verify the trustworthiness of a TD
> to other entities before provisioning secrets to the TD.

What is the difference between a TDX guest and a TD?

I'm being told they're one and the same thing. If so, please rewrite
this commit message into something actually readable because right now
it is real hard to parse what this says.

And pls avoid the Intel marketing bla - that's a write-only text and not
meant for humans.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 8/10/22 12:09 PM, Borislav Petkov wrote:
> On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:
>> In TDX guest, attestation is used to verify the trustworthiness of a TD
>> to other entities before provisioning secrets to the TD.
> 
> What is the difference between a TDX guest and a TD?
> 
> I'm being told they're one and the same thing. If so, please rewrite

Yes. In this context, TD/TD guest means the same thing. So I will just
use TDX guest uniformly.

> this commit message into something actually readable because right now
> it is real hard to parse what this says.
> 
> And pls avoid the Intel marketing bla - that's a write-only text and not
> meant for humans.

Ok. will do.

> 
> Thx.
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:

...

> Operations like getting TDREPORT or Quote generation involves sending
> a blob of data as input and getting another blob of data as output. It
> was considered to use a sysfs interface for this, but it doesn't fit
> well into the standard sysfs model for configuring values. It would be
> possible to do read/write on files, but it would need multiple file
> descriptors, which would be somewhat messy. IOCTLs seems to be the best
> fitting and simplest model for this use case. This is similar to AMD
> SEV platform, which also uses IOCTL interface to support attestation.

So the gist of this whole commit message - with the TD<->TDX
nomenclature fixed - needs to go to Documentation/x86/tdx.rst.

> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
> new file mode 100644
> index 000000000000..46a2f3612753
> --- /dev/null
> +++ b/arch/x86/coco/tdx/attest.c
> @@ -0,0 +1,81 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * attest.c - TDX attestation feature support.

s/feature //

> + *
> + * Implements attestation related IOCTL handlers.
> + *
> + * Copyright (C) 2022 Intel Corporation
> + *
> + */
> +
> +#include <linux/mm.h>
> +#include <linux/io.h>
> +#include <asm/tdx.h>
> +
> +#include "tdx.h"
> +
> +/* TDREPORT module call leaf ID */
> +#define TDX_GET_REPORT			4

All TDX leaf definitions go to arch/x86/include/asm/shared/tdx.h, for
example.

Not spread around the tree. There are some in arch/x86/coco/tdx/tdx.c
too.

In a pre-patch: please pick a fitting header, move them there and keep
them all there.

> +long tdx_get_report(void __user *argp)
> +{
> +	u8 *reportdata = NULL, *tdreport = NULL;
> +	struct tdx_report_req req;
> +	long ret;
> +
> +	/* Copy request struct from the user buffer */

Useless comment.

> +	if (copy_from_user(&req, argp, sizeof(req)))
> +		return -EFAULT;
> +
> +	/*
> +	 * Per TDX Module 1.0 specification, section titled
> +	 * "TDG.MR.REPORT", REPORTDATA and TDREPORT length
> +	 * is fixed as TDX_REPORTDATA_LEN and TDX_REPORT_LEN.
> +	 */
> +	if (req.rpd_len != TDX_REPORTDATA_LEN || req.tdr_len != TDX_REPORT_LEN)
> +		return -EINVAL;
> +
> +	/* Allocate kernel buffers for REPORTDATA and TDREPORT */
> +	reportdata = kzalloc(req.rpd_len, GFP_KERNEL);
> +	if (!reportdata) {
> +		ret = -ENOMEM;
> +		goto report_failed;
> +	}
> +
> +	tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
> +	if (!tdreport) {
> +		ret = -ENOMEM;
> +		goto report_failed;
> +	}
> +
> +
> +	/* Copy REPORTDATA from user to kernel buffer */

Useless comment.

> +	if (copy_from_user(reportdata, (void *)req.reportdata, req.rpd_len)) {

You're trusting a user pointer without any checks?

I guess there's not a lot you can check besides the length with you do.
If there are sanity checks you can do, though, do them here.

> +		ret = -EFAULT;
> +		goto report_failed;
> +	}
> +
> +	/*
> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
> +	 *
> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
> +	 * Specification for detailed information.
> +	 */
> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> +				virt_to_phys(reportdata), req.subtype,

That subtype you're not checking either.

Where's the paranoia?!

> +				0, NULL);
> +	if (ret) {
> +		ret = -EIO;
> +		goto report_failed;
> +	}
> +
> +	/* Copy TDREPORT data back to the user buffer */

Another useless comment.

> +	if (copy_to_user((void *)req.tdreport, tdreport, req.tdr_len))
> +		ret = -EFAULT;
> +
> +report_failed:
> +	kfree(reportdata);
> +	kfree(tdreport);
> +	return ret;
> +}
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 928dcf7a20d9..205f98f42da8 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -5,6 +5,9 @@
>  #define pr_fmt(fmt)     "tdx: " fmt
>  
>  #include <linux/cpufeature.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mm.h>
> +#include <linux/io.h>
>  #include <asm/coco.h>
>  #include <asm/tdx.h>
>  #include <asm/vmx.h>
> @@ -12,6 +15,8 @@
>  #include <asm/insn-eval.h>
>  #include <asm/pgtable.h>
>  
> +#include "tdx.h"
> +
>  /* TDX module Call Leaf IDs */
>  #define TDX_GET_INFO			1
>  #define TDX_GET_VEINFO			3
> @@ -34,6 +39,10 @@
>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>  
> +#define DRIVER_NAME	"tdx-guest"

Just "tdx". When you add another driver, then you can disambiguate.

> +static struct miscdevice tdx_misc_dev;
> +
>  /*
>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>   * return code.
> @@ -775,3 +784,49 @@ void __init tdx_early_init(void)
>  
>  	pr_info("Guest detected\n");
>  }
> +
> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
> +			    unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	long ret = -EINVAL;
> +
> +	switch (cmd) {
> +	case TDX_CMD_GET_REPORT:
> +		ret = tdx_get_report(argp);
> +		break;
> +	default:
> +		pr_debug("cmd %d not supported\n", cmd);
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
> +static const struct file_operations tdx_guest_fops = {
> +	.owner		= THIS_MODULE,
> +	.unlocked_ioctl	= tdx_guest_ioctl,
> +	.llseek		= no_llseek,
> +};
> +
> +static int __init tdx_guest_init(void)
> +{
> +	int ret;
> +
> +	/* Make sure we are in a valid TDX platform */

More useless comments.

When you type comments, pls stop and think whether it even makes sense
to add them or the code you're commenting is actually clear from the
function naming and the given parameters and the position in the
function..., from all of it, that it is pretty clear what happens.

> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> +		return -EIO;
> +
> +	tdx_misc_dev.name = DRIVER_NAME;
> +	tdx_misc_dev.minor = MISC_DYNAMIC_MINOR;
> +	tdx_misc_dev.fops = &tdx_guest_fops;
> +
> +	ret = misc_register(&tdx_misc_dev);
> +	if (ret) {
> +		pr_err("misc device registration failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +device_initcall(tdx_guest_init)

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Boris,

On 8/18/22 7:18 AM, Borislav Petkov wrote:
> On Wed, Jul 27, 2022 at 08:44:15PM -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> ...
> 

We have a v10 version of this patch. We have dropped GetQuote support as per Dave's
comment. If it is not a problem, for the rest of the patches in this series, please
check v10.

https://lore.kernel.org/lkml/Yu1z0KcU5C2AJO6S@fedora/T/

>> Operations like getting TDREPORT or Quote generation involves sending
>> a blob of data as input and getting another blob of data as output. It
>> was considered to use a sysfs interface for this, but it doesn't fit
>> well into the standard sysfs model for configuring values. It would be
>> possible to do read/write on files, but it would need multiple file
>> descriptors, which would be somewhat messy. IOCTLs seems to be the best
>> fitting and simplest model for this use case. This is similar to AMD
>> SEV platform, which also uses IOCTL interface to support attestation.
> 
> So the gist of this whole commit message - with the TD<->TDX
> nomenclature fixed - needs to go to Documentation/x86/tdx.rst.

Ok. I will add it in next version.

> 
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> new file mode 100644
>> index 000000000000..46a2f3612753
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,81 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX attestation feature support.
> 
> s/feature //

Ok.

> 
>> + *
>> + * Implements attestation related IOCTL handlers.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +
>> +#include "tdx.h"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT			4
> 
> All TDX leaf definitions go to arch/x86/include/asm/shared/tdx.h, for
> example.
> 
> Not spread around the tree. There are some in arch/x86/coco/tdx/tdx.c
> too.
> 
> In a pre-patch: please pick a fitting header, move them there and keep
> them all there.

Sure.  Will move it.

> 
>> +long tdx_get_report(void __user *argp)
>> +{
>> +	u8 *reportdata = NULL, *tdreport = NULL;
>> +	struct tdx_report_req req;
>> +	long ret;
>> +
>> +	/* Copy request struct from the user buffer */
> 
> Useless comment.

Ok. I will remove it in next version.

> 
>> +	if (copy_from_user(&req, argp, sizeof(req)))
>> +		return -EFAULT;
>> +
>> +	/*
>> +	 * Per TDX Module 1.0 specification, section titled
>> +	 * "TDG.MR.REPORT", REPORTDATA and TDREPORT length
>> +	 * is fixed as TDX_REPORTDATA_LEN and TDX_REPORT_LEN.
>> +	 */
>> +	if (req.rpd_len != TDX_REPORTDATA_LEN || req.tdr_len != TDX_REPORT_LEN)
>> +		return -EINVAL;
>> +
>> +	/* Allocate kernel buffers for REPORTDATA and TDREPORT */
>> +	reportdata = kzalloc(req.rpd_len, GFP_KERNEL);
>> +	if (!reportdata) {
>> +		ret = -ENOMEM;
>> +		goto report_failed;
>> +	}
>> +
>> +	tdreport = kzalloc(req.tdr_len, GFP_KERNEL);
>> +	if (!tdreport) {
>> +		ret = -ENOMEM;
>> +		goto report_failed;
>> +	}
>> +
>> +
>> +	/* Copy REPORTDATA from user to kernel buffer */
> 
> Useless comment.

Ok. I will remove it in next version.

> 
>> +	if (copy_from_user(reportdata, (void *)req.reportdata, req.rpd_len)) {
> 
> You're trusting a user pointer without any checks?
> 
> I guess there's not a lot you can check besides the length with you do.
> If there are sanity checks you can do, though, do them here.

I will add NULL pointer check and a subtype validity check there.

> 
>> +		ret = -EFAULT;
>> +		goto report_failed;
>> +	}
>> +
>> +	/*
>> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>> +	 *
>> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
>> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
>> +	 * Specification for detailed information.
>> +	 */
>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
>> +				virt_to_phys(reportdata), req.subtype,
> 
> That subtype you're not checking either.
> 
> Where's the paranoia?!
> 
>> +				0, NULL);
>> +	if (ret) {
>> +		ret = -EIO;
>> +		goto report_failed;
>> +	}
>> +
>> +	/* Copy TDREPORT data back to the user buffer */
> 
> Another useless comment.

Will remove it.

> 
>> +	if (copy_to_user((void *)req.tdreport, tdreport, req.tdr_len))
>> +		ret = -EFAULT;
>> +
>> +report_failed:
>> +	kfree(reportdata);
>> +	kfree(tdreport);
>> +	return ret;
>> +}
>> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
>> index 928dcf7a20d9..205f98f42da8 100644
>> --- a/arch/x86/coco/tdx/tdx.c
>> +++ b/arch/x86/coco/tdx/tdx.c
>> @@ -5,6 +5,9 @@
>>  #define pr_fmt(fmt)     "tdx: " fmt
>>  
>>  #include <linux/cpufeature.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>>  #include <asm/coco.h>
>>  #include <asm/tdx.h>
>>  #include <asm/vmx.h>
>> @@ -12,6 +15,8 @@
>>  #include <asm/insn-eval.h>
>>  #include <asm/pgtable.h>
>>  
>> +#include "tdx.h"
>> +
>>  /* TDX module Call Leaf IDs */
>>  #define TDX_GET_INFO			1
>>  #define TDX_GET_VEINFO			3
>> @@ -34,6 +39,10 @@
>>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>>  
>> +#define DRIVER_NAME	"tdx-guest"
> 
> Just "tdx". When you add another driver, then you can disambiguate.

Agree.

> 
>> +static struct miscdevice tdx_misc_dev;
>> +
>>  /*
>>   * Wrapper for standard use of __tdx_hypercall with no output aside from
>>   * return code.
>> @@ -775,3 +784,49 @@ void __init tdx_early_init(void)
>>  
>>  	pr_info("Guest detected\n");
>>  }
>> +
>> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>> +			    unsigned long arg)
>> +{
>> +	void __user *argp = (void __user *)arg;
>> +	long ret = -EINVAL;
>> +
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_REPORT:
>> +		ret = tdx_get_report(argp);
>> +		break;
>> +	default:
>> +		pr_debug("cmd %d not supported\n", cmd);
>> +		break;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations tdx_guest_fops = {
>> +	.owner		= THIS_MODULE,
>> +	.unlocked_ioctl	= tdx_guest_ioctl,
>> +	.llseek		= no_llseek,
>> +};
>> +
>> +static int __init tdx_guest_init(void)
>> +{
>> +	int ret;
>> +
>> +	/* Make sure we are in a valid TDX platform */
> 
> More useless comments.
> 
> When you type comments, pls stop and think whether it even makes sense
> to add them or the code you're commenting is actually clear from the
> function naming and the given parameters and the position in the
> function..., from all of it, that it is pretty clear what happens.

Ok. I will check and remove obvious comments.

> 
>> +	if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>> +		return -EIO;
>> +
>> +	tdx_misc_dev.name = DRIVER_NAME;
>> +	tdx_misc_dev.minor = MISC_DYNAMIC_MINOR;
>> +	tdx_misc_dev.fops = &tdx_guest_fops;
>> +
>> +	ret = misc_register(&tdx_misc_dev);
>> +	if (ret) {
>> +		pr_err("misc device registration failed\n");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +device_initcall(tdx_guest_init)
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Thu, Aug 18, 2022 at 07:40:51AM -0700, Sathyanarayanan Kuppuswamy wrote:
> We have a v10 version of this patch. We have dropped GetQuote support as per Dave's
> comment. If it is not a problem, for the rest of the patches in this series, please
> check v10.
> 
> https://lore.kernel.org/lkml/Yu1z0KcU5C2AJO6S@fedora/T/

Ah, I saw that but that is only 2 patches. Seems you guys went at it
with the chainsaw... :-)

Ok, will continue with it.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 8/18/22 07:18, Borislav Petkov wrote:
>> +	/*
>> +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
>> +	 *
>> +	 * Get the TDREPORT using REPORTDATA as input. Refer to
>> +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
>> +	 * Specification for detailed information.
>> +	 */
>> +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
>> +				virt_to_phys(reportdata), req.subtype,
> That subtype you're not checking either.

I'll chime in here a bit since you're touching on something that bugged
me too.  This whole mechanism is because of two TDX shortcuts.  (calling
them shortcuts is generous, but I digress...)

     1. TDX guest attestation relies on SGX.  TDX does not have its own
	attestation mechanism.
     2. TDX guests can not run SGX enclaves.  Only TDX hosts can.

As a result, any TDX guest that wants to do the attestation dance has to
talk to the host, who them talks to the SGX enclave.  There's actually a
nice diagram of it in here (Figure 5.8):

> https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf

This "talking" can be done via any old communication mechanism.  Shared
memory, virtio, morse code, whatever.  TDX_GET_REPORT just happens to be
yet another communication mechanism dedicated *only* to these
attestation reports.

So, this is not a *STRICTLY* required ABI.  Guests _can_ use other
mechanisms to talk to an SGX attestation (quoting) enclave.  Second,
this ABI *is* TDX-specific because no other hardware architectures have
made the same design "choices".

That's why this was jettisoned for v10.  It might reappear later, though.

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Huang, Kai]

On Thu, 2022-08-18 at 09:25 -0700, Dave Hansen wrote:
> On 8/18/22 07:18, Borislav Petkov wrote:
> > > +	/*
> > > +	 * Generate TDREPORT using "TDG.MR.REPORT" TDCALL.
> > > +	 *
> > > +	 * Get the TDREPORT using REPORTDATA as input. Refer to
> > > +	 * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0
> > > +	 * Specification for detailed information.
> > > +	 */
> > > +	ret = __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport),
> > > +				virt_to_phys(reportdata), req.subtype,
> > That subtype you're not checking either.
> 
> I'll chime in here a bit since you're touching on something that bugged
> me too.  This whole mechanism is because of two TDX shortcuts.  (calling
> them shortcuts is generous, but I digress...)
> 
>      1. TDX guest attestation relies on SGX.  TDX does not have its own
> 	attestation mechanism.
>      2. TDX guests can not run SGX enclaves.  Only TDX hosts can.
> 
> As a result, any TDX guest that wants to do the attestation dance has to
> talk to the host, who them talks to the SGX enclave.  There's actually a
> nice diagram of it in here (Figure 5.8):
> 
> > https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-whitepaper-v4.pdf
> 
> This "talking" can be done via any old communication mechanism.  Shared
> memory, virtio, morse code, whatever.  TDX_GET_REPORT just happens to be
> yet another communication mechanism dedicated *only* to these
> attestation reports.

Hi Dave,

Just want to clarify, the *yet another communication  mechanism" you mentioned
is actually TDX_GET_QUOTE, but not TDX_GET_REPORT.

The TDREPORT (which this TDX_GET_REPORT ABI will report to userspace) is the
data blob that needs to be sent to SGX quoting enclave to generate a Quote
(which can be remotely verified).  It's the first step of supporting TDX 
attestation, no matter whatever communication channel is going to be used to
talk to quoting enclave (vsock, tcp, or GetQuote). 

> 
> So, this is not a *STRICTLY* required ABI.  Guests _can_ use other
> mechanisms to talk to an SGX attestation (quoting) enclave.  Second,
> this ABI *is* TDX-specific because no other hardware architectures have
> made the same design "choices".
> 
> That's why this was jettisoned for v10.  It might reappear later, though.

Agreed (assuming you mean TDX_GET_QUOTE ABI).

Btw, my thinking is perhaps we can just make GetQuote support as a Intel-
specific driver, which can be a module, can have a separate Kconfig, etc.  It
doesn't have to be a core functionality.

-- 
Thanks,
-Kai

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 8/18/22 07:18, Borislav Petkov wrote:
>>  /* TDX module Call Leaf IDs */
>>  #define TDX_GET_INFO			1
>>  #define TDX_GET_VEINFO			3
>> @@ -34,6 +39,10 @@
>>  #define VE_GET_PORT_NUM(e)	((e) >> 16)
>>  #define VE_IS_IO_STRING(e)	((e) & BIT(4))
>>  
>> +#define DRIVER_NAME	"tdx-guest"
> Just "tdx". When you add another driver, then you can disambiguate.

This actually shows up to apps, though.  They'll actually be opening
/dev/tdx.  When the other driver comes along, they'll all need to change
to /dev/tdx-guest.  That seems a bit unkind to those poor app developers.

BTW, do we consider these kernel names be part of the ABI?  Seems like
we should.

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Mon, Aug 22, 2022 at 02:19:44PM -0700, Dave Hansen wrote:
> This actually shows up to apps, though.

Blergh.

> They'll actually be opening /dev/tdx. When the other driver comes
> along, they'll all need to change to /dev/tdx-guest. That seems a bit
> unkind to those poor app developers.

So do you really wanna call the attestation driver "tdx-guest"?

But that's not really an attestation driver - there's tdx/tdx.c which is
all the guest code and there's an attestation "driver" in it.

So maybe this "attestation driver" thing is just an additional "ability"
of the tdx-guest driver.

Might wanna take out your cristal ball and think what you wanna call it
now so that it doesn't change because...

> BTW, do we consider these kernel names be part of the ABI? Seems like
> we should.

... yap, that.

If apps open this, then this is an ABI and cast in stone.

I'd say.

Which makes my initial suggestion of calling this whole guest
functionality a "tdx" driver not such a bad idea... Depends on
whether there will be a split at all or it'll continue gaining more
functionality.

Or so.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Dave Hansen]

On 8/22/22 14:36, Borislav Petkov wrote:
> Which makes my initial suggestion of calling this whole guest
> functionality a "tdx" driver not such a bad idea... Depends on
> whether there will be a split at all or it'll continue gaining more
> functionality.

Yep, let's get the crystal ball out.

TDX folks:

What other ioctl()s are in the pipeline for the guest side?

What ioctl()s are in the pipeline for the host side?  Are they all part
of /dev/kvm, or are there any TDX-specific "drivers" for the host?

We want to avoid both:

 1. A driver called /dev/tdx (or "tdx-guest) which is only and will only
    ever do TDX guest attestation.
 2. A driver called /dev/tdx-guest-attest which shares a ton of
    functionality with some future TDX guest feature like
    /dev/tdx-guest-snazzy-feature-foo.  Then, a new driver every time
    a new snazzy TDX feature shows up.

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi,

On 8/22/22 2:44 PM, Dave Hansen wrote:
> On 8/22/22 14:36, Borislav Petkov wrote:
>> Which makes my initial suggestion of calling this whole guest
>> functionality a "tdx" driver not such a bad idea... Depends on
>> whether there will be a split at all or it'll continue gaining more
>> functionality.
> 
> Yep, let's get the crystal ball out.
> 
> TDX folks:
> 
> What other ioctl()s are in the pipeline for the guest side?

In addition to the GetReport support currently implemented, we have
following two attestation related IOCTLs in the pipeline.

1. GetQuote     - Adds support to get signed Quote for the given TDREPORT. This
                  is currently in the review process.
2. VerifyReport - Verifies whether given Reportdata is generated in
                  the current platform. It is only enabled in v1.5 TDX
                  Module specification.

In addition to above, I think there is a possibility to add IOCTL to get
storage keys from the ACPI SVKL table. Storage Volume Key Table (SVKL) is
used by the VBIOS to share keys required to access encrypted drives. Although
we don't have a clear requirement, I suspect that we might add IOCTL for it.

Kirill/Isaku/Kai, If I missed any other IOCTL requirements, please add it.
              
> 
> What ioctl()s are in the pipeline for the host side?  Are they all part
> of /dev/kvm, or are there any TDX-specific "drivers" for the host?
> 
> We want to avoid both:
> 
>  1. A driver called /dev/tdx (or "tdx-guest) which is only and will only
>     ever do TDX guest attestation.

>  2. A driver called /dev/tdx-guest-attest which shares a ton of
>     functionality with some future TDX guest feature like
>     /dev/tdx-guest-snazzy-feature-foo.  Then, a new driver every time
>     a new snazzy TDX feature shows up.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

Hi Boris,

On 8/18/22 7:18 AM, Borislav Petkov wrote:
>> diff --git a/arch/x86/coco/tdx/attest.c b/arch/x86/coco/tdx/attest.c
>> new file mode 100644
>> index 000000000000..46a2f3612753
>> --- /dev/null
>> +++ b/arch/x86/coco/tdx/attest.c
>> @@ -0,0 +1,81 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * attest.c - TDX attestation feature support.
> s/feature //
> 
>> + *
>> + * Implements attestation related IOCTL handlers.
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + *
>> + */
>> +
>> +#include <linux/mm.h>
>> +#include <linux/io.h>
>> +#include <asm/tdx.h>
>> +
>> +#include "tdx.h"
>> +
>> +/* TDREPORT module call leaf ID */
>> +#define TDX_GET_REPORT			4
> All TDX leaf definitions go to arch/x86/include/asm/shared/tdx.h, for
> example.
> 
> Not spread around the tree. There are some in arch/x86/coco/tdx/tdx.c
> too.
> 
> In a pre-patch: please pick a fitting header, move them there and keep
> them all there.
> 

In v10 of this patch set, this code is moved to coco/tdx/tdx.c and the module
call leaf IDs are grouped together in tdx.c.

Regarding moving the leaf definition to asm/shared, the following patch from
Kirill's unaccepted memory patch set is already doing it. Do you want to me to
leave it to Kirill to handle the cleanup, or submit a new patch here?

https://lore.kernel.org/linux-mm/20220614120231.48165-13-kirill.shutemov@linux.intel.com/

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Tue, Aug 23, 2022 at 12:36:43PM -0700, Sathyanarayanan Kuppuswamy wrote:
> Do you want to me to leave it to Kirill to handle the cleanup, or
> submit a new patch here?

Yeah, leave it to him - the less confusion the better.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Borislav Petkov]

On Mon, Aug 22, 2022 at 03:41:00PM -0700, Sathyanarayanan Kuppuswamy wrote:
> In addition to above, I think there is a possibility to add IOCTL to get
> storage keys from the ACPI SVKL table. Storage Volume Key Table (SVKL) is
> used by the VBIOS to share keys required to access encrypted drives. Although
> we don't have a clear requirement, I suspect that we might add IOCTL for it.

So this all sounds like a single tdx-guest driver to me which services a
bunch of ioctls... not separate drivers.

Hmm?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Sathyanarayanan Kuppuswamy]

On 8/24/22 8:56 AM, Borislav Petkov wrote:
> On Mon, Aug 22, 2022 at 03:41:00PM -0700, Sathyanarayanan Kuppuswamy wrote:
>> In addition to above, I think there is a possibility to add IOCTL to get
>> storage keys from the ACPI SVKL table. Storage Volume Key Table (SVKL) is
>> used by the VBIOS to share keys required to access encrypted drives. Although
>> we don't have a clear requirement, I suspect that we might add IOCTL for it.
> 
> So this all sounds like a single tdx-guest driver to me which services a
> bunch of ioctls... not separate drivers.
> 
> Hmm?

Yes. You are right. This is similar to AMD sev-guest driver. 

https://github.com/torvalds/linux/blob/master/drivers/virt/coco/sev-guest/sev-guest.c

> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 0/6] Add TDX Guest Attestation support

[Dave Hansen]

On 7/27/22 20:44, Kuppuswamy Sathyanarayanan wrote:
> An Intel SGX Quoting Enclave (QE), written specifically to support
> quoting Intel TDX TDs, uses EVERIFYREPORT2, to help check the integrity
> of the TDG.MR.REPORT. If it passes, the QE can use a certified quote
> signing key to sign a quote containing the guest TD’s measurements and
> the additional data being quoted.

(maintainer hat firmly in place, not speaking as an Intel person here...)

Let's say Intel tires of SGX and zaps it from server CPUs just like it
did clients.  Or, that Intel decides that TDX is really cool and wants
it on SGX-free clients in addition to servers.

Can the guest ABI which is introduced here work for a future attestation
architecture that does not involve SGX?

Re: [PATCH v9 0/6] Add TDX Guest Attestation support

[Sathyanarayanan Kuppuswamy]

Hi,

On 8/24/22 10:12 AM, Dave Hansen wrote:
> On 7/27/22 20:44, Kuppuswamy Sathyanarayanan wrote:
>> An Intel SGX Quoting Enclave (QE), written specifically to support
>> quoting Intel TDX TDs, uses EVERIFYREPORT2, to help check the integrity
>> of the TDG.MR.REPORT. If it passes, the QE can use a certified quote
>> signing key to sign a quote containing the guest TD’s measurements and
>> the additional data being quoted.
> 
> (maintainer hat firmly in place, not speaking as an Intel person here...)
> 
> Let's say Intel tires of SGX and zaps it from server CPUs just like it
> did clients.  Or, that Intel decides that TDX is really cool and wants
> it on SGX-free clients in addition to servers.
> 
> Can the guest ABI which is introduced here work for a future attestation
> architecture that does not involve SGX?

Yes. ABI introduced here is agnostic to QE implementation. For getting a
signed Quote, the guest will pass TDREPORT with length as input and expect
signed Quote with length as output. This requirement is valid irrespective
of QE implementation(SGX/no-SGX).

As you can see below, in our ABI we pass TDREPORT with "tdx_quote_hdr" in a
buffer. Upon successful completion of the request, we expect Quote in the
same buffer with proper header details. Such header format is generic and
should work well in non-SGX environment you have mentioned as well.

Input buffer -> [tdx_quote_hdr][TDREPORT]
Output buffer -> [tdx_quote_hdr][Quote]

struct tdx_quote_hdr {
        /* Quote version, filled by TD */
        __u64 version;
        /* Status code of Quote request, filled by VMM */
        __u64 status;
        /* Length of TDREPORT, filled by TD */
        __u32 in_len;
        /* Length of Quote, filled by VMM */
        __u32 out_len;
        /* Actual Quote data or TDREPORT on input */
        __u64 data[0];
};

/* struct tdx_quote_req: Request to generate TD Quote using TDREPORT
 *
 * @buf         : Pass user data that includes TDREPORT as input. 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; // buf with header and data
        __u64 len;
};

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Huang, Kai]

On Mon, 2022-08-22 at 14:44 -0700, Dave Hansen wrote:
> What ioctl()s are in the pipeline for the host side?  Are they all part
> of /dev/kvm, or are there any TDX-specific "drivers" for the host?

Sorry for late reply.  Was on vacation last week.

AFAICT no ioctl() is in the pipeline for the host side, except those belong to
/dev/kvm.  

I am not 100% sure about TDX live migration support (TDX 1.5), though.
Supposedly even there are they should belong to /dev/kvm too.  + Wang, wei to
confirm.

-- 
Thanks,
-Kai

RE: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Wang, Wei W]

On Monday, August 29, 2022 11:15 AM, Huang, Kai wrote:
> On Mon, 2022-08-22 at 14:44 -0700, Dave Hansen wrote:
> > What ioctl()s are in the pipeline for the host side?  Are they all
> > part of /dev/kvm, or are there any TDX-specific "drivers" for the host?
> 
> Sorry for late reply.  Was on vacation last week.
> 
> AFAICT no ioctl() is in the pipeline for the host side, except those belong to
> /dev/kvm.
> 
> I am not 100% sure about TDX live migration support (TDX 1.5), though.
> Supposedly even there are they should belong to /dev/kvm too.  + Wang, wei
> to confirm.

Not /dev/kvm.
TDX migration uses kvm device fd and implements a tdx-mig specific driver on it.

Re: [PATCH v9 1/6] x86/tdx: Add TDX Guest attestation interface driver

[Huang, Kai]

On Mon, 2022-08-29 at 08:05 +0000, Wang, Wei W wrote:
> On Monday, August 29, 2022 11:15 AM, Huang, Kai wrote:
> > On Mon, 2022-08-22 at 14:44 -0700, Dave Hansen wrote:
> > > What ioctl()s are in the pipeline for the host side?  Are they all
> > > part of /dev/kvm, or are there any TDX-specific "drivers" for the host?
> > 
> > Sorry for late reply.  Was on vacation last week.
> > 
> > AFAICT no ioctl() is in the pipeline for the host side, except those belong to
> > /dev/kvm.
> > 
> > I am not 100% sure about TDX live migration support (TDX 1.5), though.
> > Supposedly even there are they should belong to /dev/kvm too.  + Wang, wei
> > to confirm.
> 
> Not /dev/kvm.
> TDX migration uses kvm device fd and implements a tdx-mig specific driver on it.

OK anyway so the "tdx-mig specific driver" will still be on top of KVM, and we
don't need a new device node at host for it.

-- 
Thanks,
-Kai