KVM TDX basic feature support

From: Isaku Yamahata <[email protected]>

KVM TDX basic feature support

Hello. This is v19 the patch series of KVM TDX support. This is based on v6.8-rc5.

Major changes and uAPI
----------------------
The major change is uAPI change to use KVM_MEMORY_MAPPING as KVM common implementation and dropped CONFIG_KVM_MMU_PRIVATE. uAPI change requires patches to qemu or tdx kselftest as follows.

For unified KVM uAPI, I'll post independent patches and TDX support would be on top of this patch series.

Trees
-----
The tree can be found at https://github.com/intel/tdx/tree/kvm-upstream
The corresponding qemu branch is found at
https://github.com/yamahata/qemu/tree/tdx/qemu-upm
How to run/test: It's describe at https://github.com/intel/tdx/wiki/TDX-KVM

Existing VM type and SNP
------------------------
The following matrix shows helper functions returns and how the code distinguish the existing VM types (default, sw protected, and SNP) from TDX so that this patch series doesn't break the existing code.

                        default or SEV-SNP      TDX: S = (47 or 51) - 12
gfn_shared_mask         0                       S bit
kvm_is_private_gpa()    always false            true if GFN has S bit set
kvm_gfn_to_shared()     nop                     set S bit
kvm_gfn_to_private()    nop                     clear S bit

fault.is_private means that host page should be gotten from guest_memfd is_private_gpa() means that KVM MMU should invoke private MMU hooks.

Future work
-----------
This patch series supports only basic functionalities, TDX guest creation, execution and destruction. The following features are planned as follow up.
- vPMU
- Off-TD debug. qemu gdb stub support
- live migration
- etc

Required changes to qemu and tdx kselftest
------------------------------------------
Required changes to QEMU:
Please don't forget to sync linux-headers.

index 0041be589c21..60c70b6bed79 100644
@@ -714,11 +714,7 @@ static void tdx_finalize_vm(Notifier *notifier, void *unused)
     tdx_post_init_vcpus();
 
     for_each_tdx_fw_entry(tdvf, entry) {
-        struct kvm_tdx_init_mem_region mem_region = {
-            .source_addr = (__u64)entry->mem_ptr,
-            .gpa = entry->address,
-            .nr_pages = entry->size / 4096,
-        };
+        struct kvm_memory_mapping mapping;
 
         r = kvm_set_memory_attributes_private(entry->address, entry->size);
         if (r < 0) {

@@ -726,14 +722,31 @@ static void tdx_finalize_vm(Notifier *notifier, void *unused)
              exit(1);
         }
 
-        __u32 flags = entry->attributes & TDVF_SECTION_ATTRIBUTES_MR_EXTEND ?
-                      KVM_TDX_MEASURE_MEMORY_REGION : 0;
-
-        trace_kvm_tdx_init_mem_region(entry->type, entry->attributes, mem_region.source_addr, mem_region.gpa, mem_region.nr_pages);
-        r = tdx_vm_ioctl(KVM_TDX_INIT_MEM_REGION, flags, &mem_region);
+        mapping = (struct kvm_memory_mapping) {
+            .base_gfn = entry->address / 4096,
+            .nr_pages = entry->size / 4096,
+            .source = (__u64)entry->mem_ptr,
+        };
+        do {
+            r = kvm_vcpu_ioctl(first_cpu, KVM_MEMORY_MAPPING, &mapping);
+        } while (r == -EAGAIN);
         if (r < 0) {
-             error_report("KVM_TDX_INIT_MEM_REGION failed %s", strerror(-r));
-             exit(1);
+            error_report("KVM_MEMORY_MAPPING failed %s", strerror(-r));
+            exit(1);
+        }
+
+        if (entry->attributes & TDVF_SECTION_ATTRIBUTES_MR_EXTEND) {
+            mapping = (struct kvm_memory_mapping) {
+                .base_gfn = entry->address / 4096,
+                .nr_pages = entry->size / 4096,
+            };
+            do {
+                r = tdx_vm_ioctl(KVM_TDX_EXTEND_MEMORY, 0, &mapping);
+            } while (r == -EAGAIN);
+            if (r < 0) {
+                error_report("KVM_TDX_EXTEND_MEMORY failed %s", strerror(-r));
+                exit(1);
+            }
         }
 
         if (entry->type == TDVF_SECTION_TYPE_TD_HOB ||

--

Required changes to tdx kselftest.

index edc1b227a014..a2f4921c416f 100644
@@ -23,7 +23,7 @@ static char *tdx_cmd_str[] = {
 	"KVM_TDX_CAPABILITIES",
 	"KVM_TDX_INIT_VM",
 	"KVM_TDX_INIT_VCPU",
-	"KVM_TDX_INIT_MEM_REGION",
+	"KVM_TDX_EXTEND_MEMORY",
 	"KVM_TDX_FINALIZE_VM",
 	"KVM_TDX_RELEASE_VM"
 };

@@ -179,19 +179,29 @@ static void tdx_td_vcpu_init(struct kvm_vcpu *vcpu)
 static void tdx_init_mem_region(struct kvm_vm *vm, void *source_pages,
 				uint64_t gpa, uint64_t size)
 {
-	struct kvm_tdx_init_mem_region mem_region = {
-		.source_addr = (uint64_t)source_pages,
-		.gpa = gpa,
+	struct kvm_vcpu * vcpu = list_first_entry(&vm->vcpus,
+						  struct kvm_vcpu, list);
+	struct kvm_memory_mapping mapping = {
+		.base_gfn = gpa / PAGE_SIZE,
 		.nr_pages = size / PAGE_SIZE,
+		.source = (uint64_t)source_pages,
 	};
-	uint32_t metadata = KVM_TDX_MEASURE_MEMORY_REGION;
+	int r;
 
-	TEST_ASSERT((mem_region.nr_pages > 0) &&
-			    ((mem_region.nr_pages * PAGE_SIZE) == size),
+	TEST_ASSERT((mapping.nr_pages > 0) &&
+		    ((mapping.nr_pages * PAGE_SIZE) == size),
 		    "Cannot add partial pages to the guest memory.\n");
 	TEST_ASSERT(((uint64_t)source_pages & (PAGE_SIZE - 1)) == 0,
 		    "Source memory buffer is not page aligned\n");
-	tdx_ioctl(vm->fd, KVM_TDX_INIT_MEM_REGION, metadata, &mem_region);
+	r = ioctl(vcpu->fd, KVM_MEMORY_MAPPING, &mapping);
+	TEST_ASSERT(r == 0, "KVM_MEMORY_MAPPING failed: %d %d",
+		    r, errno);
+
+	mapping = (struct kvm_memory_mapping) {
+		.base_gfn = gpa / PAGE_SIZE,
+		.nr_pages = size / PAGE_SIZE,
+	};
+	tdx_ioctl(vm->fd, KVM_TDX_EXTEND_MEMORY, 0, &mapping);
 }
 
 static void tdx_td_finalizemr(struct kvm_vm *vm)

@@ -231,7 +241,7 @@ static void tdx_enable_capabilities(struct kvm_vm *vm)
 		      KVM_X2APIC_API_USE_32BIT_IDS |
 			      KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK);
 	vm_enable_cap(vm, KVM_CAP_SPLIT_IRQCHIP, 24);
-	vm_enable_cap(vm, KVM_CAP_MAX_VCPUS, 1024);
+	vm_enable_cap(vm, KVM_CAP_MAX_VCPUS, 512);
 }
 
 static void tdx_configure_memory_encryption(struct kvm_vm *vm)

--
Isaku Yamahata

Changes from v18:
- rebased to v6.8-rc5:
- uAPI brekage: KVM_TDX_INIT_MEM_REGION => KVM_MEMORY_MAPPING + KVM_TDX_EXTEND_MEMORY
- Drop CONFIG_KVM_MMU_PRIVATE
- Require NO_RBP_MOD extension of TDX module
- Use TDH.VP.RD(TD_VCPU_STATE_DETAILS) to drop buggy_hlt_workaround

Changes from v17:
- Changed tdx_seamcall() to use struct tdx_module_args
- use TDH.SYS.RD() instead of TDH.SYS.INFO()
- Drop workaround of pending interrupt
- move the initializatin of loaded_vmcss_on_cpu to vmx_init()
- More error handling on tdx 1.5 specific error code
- drop changes of tools/arch/x86/include/uapi/asm/kvm.h
- fixes typo, indent and tabs

Changes from v16:
- rebased v6.7-rc
- Switched to TDX module 1.5. Unsupport TDX module 1.0

Changes from v15:
- Added KVM_TDX_RELEASE_VM to reduce the destruction time
- Catch up the TDX module interface to use struct tdx_module_args instead of struct tdx_module_output
- Add tdh_mem_sept_rd() for SEPT_VE_DISABLE=1 and handle Secure-EPT violation with SEPT_VE_DISABLE case.
- Simplified tdx_reclaim_page()
- Reorganize the locking of tdx_release_hkid(), and use smp_call_mask() instead of smp_call_on_cpu() to hold spinlock to race with invalidation on releasing guest memfd
- Removed AMX check as the KVM upstream supports AMX.
- Added CET flag to guest supported xss
- add check if nr_pages isn't large with
(nr_page << PAGE_SHIFT) >> PAGE_SHIFT
- use __seamcall_saved_ret()
- As struct tdx_module_args doesn't match with vcpu.arch.regs, copy regs before/after calling __seamcall_saved_ret().

Changes from v14:
https://lore.kernel.org/all/[email protected]/
- rebased to v6.5-rc2, v11 KVM guest_memfd(), v11 TDX host kernel support
- ABI change to add reserved member for future compatibility, dropped unused member.
- handle EXIT_REASON_OTHER_SMI
- handle FEAT_CTL MSR access

Changes from v13:
- rbased to v6.4-rc3
- Make use of KVM gmem.
- Added check_cpuid callback for KVM_SET_CPUID2 as RFC patch.
- ABI change of KVM_TDX_VM_INIT as VM scoped KVM ioctl.
- Make TDX initialization non-depend on kvm hardware_enable. Use vmx_hardware_enable directly.
- Drop a patch to prohibit dirty logging as new KVM gmem code base
- Drop parameter only checking for some TDG.VP.VMCALL. Just default part

Changes from v12:
- ABI change of KVM_TDX_VM_INIT
- Rename kvm_gfn_{private, shared} to kvm_gfn_to_{private, shared}
- Move APIC BASE MSI initialization to KVM_TDX_VCPU_INIT
- Fix MTRR patch
- Make MapGpa hypercall always pass it to user space VMM
- Split hooks to TDP MMU into two part. populating and zapping.

Changes from v11:
- ABI change of KVM_TDX_VM_INIT
- Split the hook of TDP MMU to not modify handle_changed_spte()
- Enhanced commit message on mtrr patch
- Made KVM_CAP_MAX_VCPUS to x86 specific

Changes from v10:
- rebased to v6.2-rc3
- support mtrr with its own patches
- Integrated fd-based private page v10
- Integrated TDX host kernel support v8
- Integrated kvm_init rework v2
- removed struct tdx_td_page and its initialization logic
- cleaned up mmio spte and require enable_mmio_caching=true for TDX
- removed dubious WARN_ON_ONCE()
- split a patch adding methods as nop into several patches

Changes from v9:
- rebased to v6.1-rc2
- Integrated fd-based private page v9 as prerequisite.
- Integrated TDX host kernel support v6
- TDP MMU: Make handle_change_spte() return value.
- TDX: removed seamcall_lock and return -EAGAIN so that TDP MMU can retry

Changes from v8:
- rebased to v6.0-rc7
- Integrated with kvm hardware initialization. Check all packages has at least one online CPU when creating guest TD and refuse cpu offline during guest TDs are running.
- Integrated fd-based private page v8 as prerequisite.
- TDP MMU: Introduced more callbacks instead of single callback.

Changes from v7:
- Use xarray to track whether GFN is private or shared. Drop SPTE_SHARED_MASK. The complex state machine with SPTE_SHARED_MASK was ditched.
- Large page support is implemented. But will be posted as independent RFC patch.
- fd-based private page v7 is integrated. This is mostly same to Chao's patches. It's in github.

Changes from v6:
- rebased to v5.19

Changes from v5:
- export __seamcall and use it
- move mutex lock from callee function of smp_call_on_cpu to the caller.
- rename mmu_prezap => flush_shadow_all_private() and tdx_mmu_release_hkid
- updated comment
- drop the use of tdh_mng_key.reclaimid(): as the function is for backward compatibility to only return success
- struct kvm_tdx_cmd: metadata => flags, added __u64 error.
- make this ioctl systemwide ioctl
- ABI change to struct kvm_init_vm
- guest_tsc_khz: use kvm->arch.default_tsc_khz
- rename BUILD_BUG_ON_MEMCPY to MEMCPY_SAME_SIZE
- drop exporting kvm_set_tsc_khz().
- fix kvm_tdp_page_fault() for mtrr emulation
- rename it to kvm_gfn_shared_mask(), dropped kvm_gpa_shared_mask()
- drop kvm_is_private_gfn(), kept kvm_is_private_gpa()
keep kvm_{gfn, gpa}_private(), kvm_gpa_private()
- update commit message
- rename shadow_init_value => shadow_nonprsent_value
- added ept_violation_ve_test mode
- shadow_nonpresent_value => SHADOW_NONPRESENT_VALUE in tdp_mmu.c
- legacy MMU case
=> - mmu_topup_shadow_page_cache(), kvm_mmu_create()
- FNAME(sync_page)(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
- #VE warning:
- rename: REMOVED_SPTE => __REMOVED_SPTE, SHADOW_REMOVED_SPTE => REMOVED_SPTE
- merge into Like we discussed, this patch should be merged with patch "KVM: x86/mmu: Allow non-zero init value for shadow PTE".
- fix pointed by Sagi. check !is_private check => (kvm_gfn_shared_mask && !is_private)
- introduce kvm_gfn_for_root(kvm, root, gfn)
- add only_shared argument to kvm_tdp_mmu_handle_gfn()
- use kvm_arch_dirty_log_supported()
- rename SPTE_PRIVATE_PROHIBIT to SPTE_SHARED_MASK.
- rename: is_private_prohibit_spte() => spte_shared_mask()
- fix: shadow_nonpresent_value => SHADOW_NONPRESENT_VALUE in comment
- dropped this patch as the change was merged into kvm/queue
- update vt_apicv_post_state_restore()
- use is_64_bit_hypercall()
- comment: expand MSMI -> Machine Check System Management Interrupt
- fixed TDX_SEPT_PFERR
- tdvmcall_p[1234]_{write, read}() => tdvmcall_a[0123]_{read,write}()
- rename tdmvcall_exit_readon() => tdvmcall_leaf()
- remove optional zero check of argument.
- do a check for static_call(kvm_x86_has_emulated_msr)(kvm, MSR_IA32_SMBASE) in kvm_vcpu_ioctl_smi and __apic_accept_irq.
- WARN_ON_ONCE in tdx_smi_allowed and tdx_enable_smi_window.
- introduce vcpu_deliver_init to x86_ops
- sprinkeled KVM_BUG_ON()

Changes from v4:
- rebased to TDX host kernel patch series.
- include all the patches to make this patch series working.
- add [MARKER] patches to mark the patch layer clear.

---
* What's TDX?
TDX stands for Trust Domain Extensions, which extends Intel Virtual Machines Extensions (VMX) to introduce a kind of virtual machine guest called a Trust Domain (TD) for confidential computing.

A TD runs in a CPU mode that is designed to protect the confidentiality of its memory contents and its CPU state from any other software, including the hosting Virtual Machine Monitor (VMM), unless explicitly shared by the TD itself.

We have more detailed explanations below (***). We have the high-level design of TDX KVM below (****).

In this patch series, we use "TD" or "guest TD" to differentiate it from the current "VM" (Virtual Machine), which is supported by KVM today.

* The organization of this patch series
This patch series is on top of the patches series "TDX host kernel support":
https://lore.kernel.org/lkml/[email protected]/

This patch series is available at
https://github.com/intel/tdx/tree/kvm-upstream

The related repositories (TDX qemu, TDX OVMF(tdvf) etc) are described at
https://github.com/intel/tdx/wiki/TDX-KVM

The relations of the layers are depicted as follows.
The arrows below show the order of patch reviews we would like to have.

The below layers are chosen so that the device model, for example, qemu can exercise each layering step by step. Check if TDX is supported, create TD VM, create TD vcpu, allow vcpu running, populate TD guest private memory, and handle vcpu exits/hypercalls/interrupts to run TD fully.

TDX vcpu
interrupt/exits/hypercall<------------\

        ^                               |
        |                               |
  TD finalization                       |
        ^                               |
        |                               |
  TDX EPT violation<------------\       |
        ^                       |       |
        |                       |       |
  TD vcpu enter/exit            |       |
        ^                       |       |
        |                       |       |
  TD vcpu creation/destruction  |       \-------KVM TDP MMU MapGPA
        ^                       |                       ^
        |                       |                       |
  TD VM creation/destruction    \---------------KVM TDP MMU hooks

^ ^
| |
TDX architectural definitions KVM TDP refactoring for TDX
^ ^
| |
TDX, VMX <--------TDX host kernel KVM MMU GPA stolen bits
coexistence support

The followings are explanations of each layer. Each layer has a dummy commit that starts with [MARKER] in subject. It is intended to help to identify where each layer starts.

TDX host kernel support:
https://lore.kernel.org/lkml/[email protected]/
The guts of system-wide initialization of TDX module. There is an independent patch series for host x86. TDX KVM patches call functions this patch series provides to initialize the TDX module.

TDX, VMX coexistence:
Infrastructure to allow TDX to coexist with VMX and trigger the initialization of the TDX module.
This layer starts with
"KVM: VMX: Move out vmx_x86_ops to 'main.c' to wrap VMX and TDX"
TDX architectural definitions:
Add TDX architectural definitions and helper functions
This layer starts with
"[MARKER] The start of TDX KVM patch series: TDX architectural definitions".
TD VM creation/destruction:
Guest TD creation/destroy allocation and releasing of TDX specific vm and vcpu structure. Create an initial guest memory image with TDX measurement.
This layer starts with
"[MARKER] The start of TDX KVM patch series: TD VM creation/destruction".
TD vcpu creation/destruction:
guest TD creation/destroy Allocation and releasing of TDX specific vm and vcpu structure. Create an initial guest memory image with TDX measurement.
This layer starts with
"[MARKER] The start of TDX KVM patch series: TD vcpu creation/destruction"
TDX EPT violation:
Create an initial guest memory image with TDX measurement. Handle secure EPT violations to populate guest pages with TDX SEAMCALLs. This layer starts with
"[MARKER] The start of TDX KVM patch series: TDX EPT violation"
TD vcpu enter/exit:
Allow TDX vcpu to enter into TD and exit from TD. Save CPU state before entering into TD. Restore CPU state after exiting from TD.
This layer starts with
"[MARKER] The start of TDX KVM patch series: TD vcpu enter/exit"
TD vcpu interrupts/exit/hypercall:
Handle various exits/hypercalls and allow interrupts to be injected so that TD vcpu can continue running.
This layer starts with
"[MARKER] The start of TDX KVM patch series: TD vcpu exits/interrupts/hypercalls"

KVM MMU GPA shared bit:
Introduce framework to handle shared bit repurposed bit of GPA TDX repurposed a bit of GPA to indicate shared or private. If it's shared, it's the same as the conventional VMX EPT case. VMM can access shared guest pages. If it's private, it's handled by Secure-EPT and the guest page is encrypted.
This layer starts with
"[MARKER] The start of TDX KVM patch series: KVM MMU GPA stolen bits"
KVM TDP refactoring for TDX:
TDX Secure EPT requires different constants. e.g. initial value EPT entry value etc. Various refactoring for those differences. This layer starts with
"[MARKER] The start of TDX KVM patch series: KVM TDP refactoring for TDX"
KVM TDP MMU hooks:
Introduce framework to TDP MMU to add hooks in addition to direct EPT access TDX added Secure EPT which is an enhancement to VMX EPT. Unlike conventional VMX EPT, CPU can't directly read/write Secure EPT. Instead, use TDX SEAMCALLs to operate on Secure EPT.
This layer starts with
"[MARKER] The start of TDX KVM patch series: KVM TDP MMU hooks"
KVM TDP MMU MapGPA:
Introduce framework to handle switching guest pages from private/shared to shared/private. For a given GPA, a guest page can be assigned to a private GPA or a shared GPA exclusively. With TDX MapGPA hypercall, guest TD converts GPA assignments from private (or shared) to shared (or private).
This layer starts with
"[MARKER] The start of TDX KVM patch series: KVM TDP MMU MapGPA "

KVM guest private memory: (not shown in the above diagram)
[PATCH v4 00/12] KVM: mm: fd-based approach for supporting KVM guest private
memory: https://lkml.org/lkml/2022/1/18/395
Guest private memory requires different memory management in KVM. The patch proposes a way for it. Integration with TDX KVM.

(***)
* TDX module
A CPU-attested software module called the "TDX module" is designed to implement the TDX architecture, and it is loaded by the UEFI firmware today. It can be loaded by the kernel or driver at runtime, but in this patch series we assume that the TDX module is already loaded and initialized.

The TDX module provides two main new logical modes of operation built upon the new SEAM (Secure Arbitration Mode) root and non-root CPU modes added to the VMX architecture. TDX root mode is mostly identical to the VMX root operation mode, and the TDX functions (described later) are triggered by the new SEAMCALL instruction with the desired interface function selected by an input operand (leaf number, in RAX). TDX non-root mode is used for TD guest operation. TDX non-root operation (i.e. "guest TD" mode) is similar to the VMX non-root operation (i.e. guest VM), with changes and restrictions to better assure that no other software or hardware has direct visibility of the TD memory and state.

TDX transitions between TDX root operation and TDX non-root operation include TD Entries, from TDX root to TDX non-root mode, and TD Exits from TDX non-root to TDX root mode. A TD Exit might be asynchronous, triggered by some external event (e.g., external interrupt or SMI) or an exception, or it might be synchronous, triggered by a TDCALL (TDG.VP.VMCALL) function.

TD VCPUs can be entered using SEAMCALL(TDH.VP.ENTER) by KVM. TDH.VP.ENTER is one of the TDX interface functions as mentioned above, and "TDH" stands for Trust Domain Host. Those host-side TDX interface functions are categorized into various areas just for better organization, such as SYS (TDX module management), MNG (TD management), VP (VCPU), PHYSMEM (physical memory), MEM (private memory), etc. For example, SEAMCALL(TDH.SYS.INFO) returns the TDX module information.

TDCS (Trust Domain Control Structure) is the main control structure of a guest TD, and encrypted (using the guest TD's ephemeral private key). At a high level, TDCS holds information for controlling TD operation as a whole, execution, EPTP, MSR bitmaps, etc that KVM needs to set it up. Note that MSR bitmaps are held as part of TDCS (unlike VMX) because they are meant to have the same value for all VCPUs of the same TD.

Trust Domain Virtual Processor State (TDVPS) is the root control structure of a TD VCPU. It helps the TDX module control the operation of the VCPU, and holds the VCPU state while the VCPU is not running. TDVPS is opaque to software and DMA access, accessible only by using the TDX module interface functions (such as TDH.VP.RD, TDH.VP.WR). TDVPS includes TD VMCS, and TD VMCS auxiliary structures, such as virtual APIC page, virtualization exception information, etc.

Several VMX control structures (such as Shared EPT and Posted interrupt descriptor) are directly managed and accessed by the host VMM. These control structures are pointed to by fields in the TD VMCS.

The above means that 1) KVM needs to allocate different data structures for TDs,
2) KVM can reuse the existing code for TDs for some operations, 3) it needs to define TD-specific handling for others. 3) Redirect operations to . 3) Redirect operations to the TDX specific callbacks, like "if (is_td_vcpu(vcpu)) tdx_callback() else vmx_callback();".

*TD Private Memory
TD private memory is designed to hold TD private content, encrypted by the CPU using the TD ephemeral key. An encryption engine holds a table of encryption keys, and an encryption key is selected for each memory transaction based on a Host Key Identifier (HKID). By design, the host VMM does not have access to the encryption keys.

In the first generation of MKTME, HKID is "stolen" from the physical address by allocating a configurable number of bits from the top of the physical address. The HKID space is partitioned into shared HKIDs for legacy MKTME accesses and private HKIDs for SEAM-mode-only accesses. We use 0 for the shared HKID on the host so that MKTME can be opaque or bypassed on the host.

During TDX non-root operation (i.e. guest TD), memory accesses can be qualified as either shared or private, based on the value of a new SHARED bit in the Guest Physical Address (GPA). The CPU translates shared GPAs using the usual VMX EPT (Extended Page Table) or "Shared EPT" (in this document), which resides in host VMM memory. The Shared EPT is directly managed by the host VMM - the same as with the current VMX. Since guest TDs usually require I/O, and the data exchange needs to be done via shared memory, thus KVM needs to use the current EPT functionality even for TDs.

* Secure EPT and Minoring using the TDP code
The CPU translates private GPAs using a separate Secure EPT. The Secure EPT pages are encrypted and integrity-protected with the TD's ephemeral private key. Secure EPT can be managed _indirectly_ by the host VMM, using the TDX interface functions, and thus conceptually Secure EPT is a subset of EPT (why "subset"). Since execution of such interface functions takes much longer time than accessing memory directly, in KVM we use the existing TDP code to minor the Secure EPT for the TD.

This way, we can effectively walk Secure EPT without using the TDX interface functions.

* VM life cycle and TDX specific operations
The userspace VMM, such as QEMU, needs to build and treat TDs differently. For example, a TD needs to boot in private memory, and the host software cannot copy the initial image to private memory.

* TSC Virtualization
The TDX module helps TDs maintain reliable TSC (Time Stamp Counter) values (e.g. consistent among the TD VCPUs) and the virtual TSC frequency is determined by TD configuration, i.e. when the TD is created, not per VCPU. The current KVM owns TSC virtualization for VMs, but the TDX module does for TDs.

* MCE support for TDs
The TDX module doesn't allow VMM to inject MCE. Instead PV way is needed for TD to communicate with VMM. For now, KVM silently ignores MCE request by VMM. MSRs related to MCE (e.g, MCE bank registers) can be naturally emulated by paravirtualizing MSR access.

[1] For details, the specifications, [2], [3], [4], [5], [6], [7], are available.

* Restrictions or future work
Some features are not included to reduce patch size. Those features are addressed as future independent patch series.
- large page (2M, 1G)
- qemu gdb stub
- guest PMU
- and more

* Prerequisites
It's required to load the TDX module and initialize it. It's out of the scope of this patch series. Another independent patch for the common x86 code is planned. It defines CONFIG_INTEL_TDX_HOST and this patch series uses CONFIG_INTEL_TDX_HOST. It's assumed that With CONFIG_INTEL_TDX_HOST=y, the TDX module is initialized and ready for KVM to use the TDX module APIs for TDX guest life cycle like tdh.mng.init are ready to use.

Concretely Global initialization, LP (Logical Processor) initialization, global configuration, the key configuration, and TDMR and PAMT initialization are done. The state of the TDX module is SYS_READY. Please refer to the TDX module specification, the chapter Intel TDX Module Lifecycle State Machine

** Detecting the TDX module readiness.
TDX host patch series implements the detection of the TDX module availability and its initialization so that KVM can use it. Also it manages Host KeyID (HKID) assigned to guest TD.
The assumed APIs the TDX host patch series provides are
- const struct tdsysinfo_struct *tdx_get_sysinfo(void);
Return the system wide information about the TDX module. NULL if the TDX isn't initialized.
- int tdx_enable(void);
Initialization of TDX module so that the TDX module is ready for KVM to use.
- extern u32 tdx_global_keyid __read_mostly;
global host key id that is used for the TDX module itself.
- u32 tdx_get_num_keyid(void);
return the number of available TDX private host key id.
- int tdx_keyid_alloc(void);
Allocate HKID for guest TD.
- void tdx_keyid_free(int keyid);
Free HKID for guest TD.

(****)
* TDX KVM high-level design
- Host key ID management
Host Key ID (HKID) needs to be assigned to each TDX guest for memory encryption. It is assumed The TDX host patch series implements necessary functions, u32 tdx_get_global_keyid(void), int tdx_keyid_alloc(void) and,
void tdx_keyid_free(int keyid).

- Data structures and VM type
Because TDX is different from VMX, define its own VM/VCPU structures, struct kvm_tdx and struct vcpu_tdx instead of struct kvm_vmx and struct vcpu_vmx. To identify the VM, introduce VM-type to specify which VM type, VMX (default) or TDX, is used.

- VM life cycle and TDX specific operations
Re-purpose the existing KVM_MEMORY_ENCRYPT_OP to add TDX specific operations. New commands are used to get the TDX system parameters, set TDX specific VM/VCPU parameters, set initial guest memory and measurement.

The creation of TDX VM requires five additional operations in addition to the conventional VM creation.
- Get KVM system capability to check if TDX VM type is supported
- VM creation (KVM_CREATE_VM)
- New: Get the TDX specific system parameters. KVM_TDX_GET_CAPABILITY.
- New: Set TDX specific VM parameters. KVM_TDX_INIT_VM.
- VCPU creation (KVM_CREATE_VCPU)
- New: Set TDX specific VCPU parameters. KVM_TDX_INIT_VCPU.
- New: Initialize guest memory as boot state and extend the measurement with the memory. KVM_TDX_INIT_MEM_REGION.
- New: Finalize VM. KVM_TDX_FINALIZE. Complete measurement of the initial TDX VM contents.
- VCPU RUN (KVM_VCPU_RUN)

- Protected guest state
Because the guest state (CPU state and guest memory) is protected, the KVM VMM can't operate on them. For example, accessing CPU registers, injecting exceptions, and accessing guest memory. Those operations are handled as silently ignored, returning zero or initial reset value when it's requested via KVM API ioctls.

VM/VCPU state and callbacks for TDX specific operations. Define tdx specific VM state and VCPU state instead of VMX ones. Redirect operations to TDX specific callbacks. "if (tdx) tdx_op() else vmx_op()".

Operations on the CPU state
silently ignore operations on the guest state. For example, the write to CPU registers is ignored and the read from CPU registers returns 0.

. ignore access to CPU registers except for allowed ones.
. TSC: add a check if tsc is immutable and return an error. Because the KVM
implementation updates the internal tsc state and it's difficult to back out those changes. Instead, skip the logic.
. dirty logging: add check if dirty logging is supported.
. exceptions/SMI/MCE/SIPI/INIT: silently ignore

Note: virtual external interrupt and NMI can be injected into TDX guests.

- KVM MMU integration
One bit of the guest physical address (bit 51 or 47) is repurposed to indicate if the guest physical address is private (the bit is cleared) or shared (the bit is set). The bits are called stolen bits.

- Stolen bits framework
systematically tracks which guest physical address, shared or private, is used.

- Shared EPT and secure EPT
There are two EPTs. Shared EPT (the conventional one) and Secure EPT(the new one). Shared EPT is handled the same for the stolen bit set. Secure EPT points to private guest pages. To resolve EPT violation, KVM walks one of two EPTs based on faulted GPA. Because it's costly to access secure EPT during walking EPTs with SEAMCALLs for the private guest physical address, another private EPT is used as a shadow of Secure-EPT with the existing logic at the cost of extra memory.

The following depicts the relationship.

                    KVM                             |       TDX module
                     |                              |           |
        -------------+----------                    |           |
        |                      |                    |           |
        V                      V                    |           |
     shared GPA           private GPA               |           |
  CPU shared EPT pointer  KVM private EPT pointer   |  CPU secure EPT pointer
        |                      |                    |           |
        |                      |                    |           |
        V                      V                    |           V
  shared EPT                private EPT--------mirror----->Secure EPT
        |                      |                    |           |
        |                      \--------------------+------\    |
        |                                           |      |    |
        V                                           |      V    V
  shared guest page                                 |    private guest page
                                                    |
                                                    |
                              non-encrypted memory  |    encrypted memory
                                                    |

- Operating on Secure EPT
Use the TDX module APIs to operate on Secure EPT. To call the TDX API during resolving EPT violation, add hooks to additional operation and wiring it to TDX backend.

* References

[1] TDX specification
https://www.intel.com/content/www/us/en/developer/articles/technical/intel-trust-domain-extensions.html
[2] Intel Trust Domain Extensions (Intel TDX)
https://cdrdv2.intel.com/v1/dl/getContent/726790
[3] Intel CPU Architectural Extensions Specification
https://www.intel.com/content/dam/develop/external/us/en/documents-tps/intel-tdx-cpu-architectural-specification.pdf
[4] Intel TDX Module 1.0 Specification
https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-module-1.0-public-spec-v0.931.pdf
[5] Intel TDX Loader Interface Specification
https://www.intel.com/content/dam/develop/external/us/en/documents-tps/intel-tdx-seamldr-interface-specification.pdf
[6] Intel TDX Guest-Hypervisor Communication Interface
https://cdrdv2.intel.com/v1/dl/getContent/726790
[7] Intel TDX Virtual Firmware Design Guide
https://www.intel.com/content/dam/develop/external/us/en/documents/tdx-virtual-firmware-design-guide-rev-1.01.pdf
[8] intel public github
kvm TDX branch: https://github.com/intel/tdx/tree/kvm
TDX guest branch: https://github.com/intel/tdx/tree/guest
qemu TDX https://github.com/intel/qemu-tdx
[9] TDVF
https://github.com/tianocore/edk2-staging/tree/TDVF
This was merged into EDK2 main branch. https://github.com/tianocore/edk2

Chao Gao (2):
KVM: x86/mmu: Assume guest MMIOs are shared
KVM: x86: Allow to update cached values in kvm_user_return_msrs w/o
wrmsr

Isaku Yamahata (101):
x86/tdx: Warning with 32bit build shift-count-overflow
KVM: x86: Pass is_private to gmem hook of gmem_max_level
KVM: Add new members to struct kvm_gfn_range to operate on
KVM: x86/mmu: Pass around full 64-bit error code for the KVM page
fault
KVM: x86: Use PFERR_GUEST_ENC_MASK to indicate fault is private
KVM: Add KVM vcpu ioctl to pre-populate guest memory
KVM: Document KVM_MEMORY_MAPPING ioctl
KVM: x86: Implement kvm_arch_{, pre_}vcpu_memory_mapping()
KVM: x86: Add is_vm_type_supported callback
KVM: x86/vmx: initialize loaded_vmcss_on_cpu in vmx_init()
KVM: x86/vmx: Refactor KVM VMX module init/exit functions
KVM: TDX: Initialize the TDX module when loading the KVM intel kernel
module
KVM: TDX: Add placeholders for TDX VM/vcpu structure
KVM: TDX: Make TDX VM type supported
[MARKER] The start of TDX KVM patch series: TDX architectural
definitions
KVM: TDX: Define TDX architectural definitions
KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module
KVM: TDX: Add helper functions to print TDX SEAMCALL error [MARKER] The start of TDX KVM patch series: TD VM creation/destruction
KVM: TDX: Add helper functions to allocate/free TDX private host key
id
KVM: TDX: Add helper function to read TDX metadata in array
KVM: TDX: Get system-wide info about TDX module on initialization
KVM: TDX: Add place holder for TDX VM specific mem_enc_op ioctl
KVM: TDX: Make KVM_CAP_MAX_VCPUS backend specific
KVM: TDX: create/destroy VM structure
KVM: TDX: initialize VM with TDX specific parameters
KVM: TDX: Make pmu_intel.c ignore guest TD case
KVM: TDX: Refuse to unplug the last cpu on the package [MARKER] The start of TDX KVM patch series: TD vcpu
creation/destruction
KVM: TDX: create/free TDX vcpu structure
KVM: TDX: Do TDX specific vcpu initialization
[MARKER] The start of TDX KVM patch series: KVM MMU GPA shared bits
KVM: x86/mmu: Add address conversion functions for TDX shared bit of
GPA
[MARKER] The start of TDX KVM patch series: KVM TDP refactoring for
TDX
KVM: x86/mmu: Replace hardcoded value 0 for the initial value for SPTE
KVM: x86/mmu: Add Suppress VE bit to
shadow_mmio_mask/shadow_present_mask
KVM: x86/mmu: Track shadow MMIO value on a per-VM basis
KVM: x86/mmu: Disallow fast page fault on private GPA
KVM: VMX: Introduce test mode related to EPT violation VE [MARKER] The start of TDX KVM patch series: KVM TDP MMU hooks
KVM: x86/tdp_mmu: Init role member of struct kvm_mmu_page at
allocation
KVM: x86/mmu: Add a new is_private member for union kvm_mmu_page_role
KVM: x86/mmu: Add a private pointer to struct kvm_mmu_page
KVM: x86/tdp_mmu: Apply mmu notifier callback to only shared GPA
KVM: x86/tdp_mmu: Sprinkle __must_check
KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU [MARKER] The start of TDX KVM patch series: TDX EPT violation
KVM: TDX: Add accessors VMX VMCS helpers
KVM: TDX: Require TDP MMU and mmio caching for TDX
KVM: TDX: TDP MMU TDX support
KVM: TDX: MTRR: implement get_mt_mask() for TDX
[MARKER] The start of TDX KVM patch series: TD finalization
KVM: x86: Add hooks in kvm_arch_vcpu_memory_mapping()
KVM: TDX: Create initial guest memory
KVM: TDX: Extend memory measurement with initial guest memory
KVM: TDX: Finalize VM initialization
[MARKER] The start of TDX KVM patch series: TD vcpu enter/exit
KVM: TDX: Implement TDX vcpu enter/exit path
KVM: TDX: vcpu_run: save/restore host state(host kernel gs)
KVM: TDX: restore host xsave state when exit from the guest TD
KVM: TDX: restore user ret MSRs
[MARKER] The start of TDX KVM patch series: TD vcpu
exits/interrupts/hypercalls
KVM: TDX: Complete interrupts after tdexit
KVM: TDX: restore debug store when TD exit
KVM: TDX: handle vcpu migration over logical processor
KVM: x86: Add a switch_db_regs flag to handle TDX's auto-switched
behavior
KVM: TDX: remove use of struct vcpu_vmx from posted_interrupt.c
KVM: TDX: Implement interrupt injection
KVM: TDX: Implements vcpu request_immediate_exit
KVM: TDX: Implement methods to inject NMI
KVM: TDX: Add a place holder to handle TDX VM exit
KVM: TDX: handle EXIT_REASON_OTHER_SMI
KVM: TDX: handle ept violation/misconfig exit
KVM: TDX: handle EXCEPTION_NMI and EXTERNAL_INTERRUPT
KVM: TDX: Handle EXIT_REASON_OTHER_SMI with MSMI
KVM: TDX: Add a place holder for handler of TDX hypercalls

    (TDG.VP.VMCALL)

KVM: TDX: handle KVM hypercall with TDG.VP.VMCALL
KVM: TDX: Add KVM Exit for TDX TDG.VP.VMCALL
KVM: TDX: Handle TDX PV CPUID hypercall
KVM: TDX: Handle TDX PV HLT hypercall
KVM: TDX: Handle TDX PV port io hypercall
KVM: TDX: Implement callbacks for MSR operations for TDX
KVM: TDX: Handle TDX PV rdmsr/wrmsr hypercall
KVM: TDX: Handle MSR MTRRCap and MTRRDefType access
KVM: TDX: Handle MSR IA32_FEAT_CTL MSR and IA32_MCG_EXT_CTL
KVM: TDX: Handle TDG.VP.VMCALL<GetTdVmCallInfo> hypercall
KVM: TDX: Silently discard SMI request
KVM: TDX: Silently ignore INIT/SIPI
KVM: TDX: Add methods to ignore accesses to CPU state
KVM: TDX: Add methods to ignore guest instruction emulation
KVM: TDX: Add a method to ignore dirty logging
KVM: TDX: Add methods to ignore VMX preemption timer
KVM: TDX: Add methods to ignore accesses to TSC
KVM: TDX: Ignore setting up mce
KVM: TDX: Add a method to ignore for TDX to ignore hypercall patch
KVM: TDX: Add methods to ignore virtual apic related operation
KVM: TDX: Inhibit APICv for TDX guest Documentation/virt/kvm: Document on Trust Domain Extensions(TDX)
KVM: x86: design documentation on TDX support of x86 KVM TDP MMU
RFC: KVM: x86: Add x86 callback to check cpuid
RFC: KVM: x86, TDX: Add check for KVM_SET_CPUID2

Kai Huang (7):
x86/virt/tdx: Rename _offset to _member for TD_SYSINFO_MAP() macro x86/virt/tdx: Move TDMR metadata fields map table to local variable x86/virt/tdx: Unbind global metadata read with 'struct
tdx_tdmr_sysinfo'
x86/virt/tdx: Support global metadata read for all element sizes x86/virt/tdx: Export global metadata read infrastructure x86/virt/tdx: Export TDX KeyID information
x86/virt/tdx: Export SEAMCALL functions

Michael Roth (1):
KVM: x86: Add gmem hook for determining max NPT mapping level

Sean Christopherson (15):
KVM: x86/mmu: Introduce kvm_mmu_map_tdp_page() for use by TDX
KVM: VMX: Move out vmx_x86_ops to 'main.c' to wrap VMX and TDX
KVM: TDX: Add TDX "architectural" error codes
KVM: TDX: x86: Add ioctl to get TDX systemwide parameters
KVM: Allow page-sized MMU caches to be initialized with custom 64-bit
values
KVM: x86/mmu: Allow non-zero value for non-present SPTE and removed
SPTE
KVM: x86/tdp_mmu: Don't zap private pages for unsupported cases
KVM: VMX: Split out guts of EPT violation to common/exposed function
KVM: TDX: Add load_mmu_pgd method for TDX
KVM: TDX: Add support for find pending IRQ in a protected local APIC
KVM: x86: Assume timer IRQ was injected if APIC state is proteced
KVM: VMX: Modify NMI and INTR handlers to take intr_info as function
argument
KVM: VMX: Move NMI/exception handler to common helper
KVM: x86: Split core of hypercall emulation to helper function
KVM: TDX: Handle TDX PV MMIO hypercall

Yan Zhao (1):
KVM: x86/mmu: Do not enable page track for TD guest

Yang Weijiang (1):
KVM: TDX: Add TSX_CTRL msr into uret_msrs list

Yao Yuan (1):
KVM: TDX: Handle vmentry failure for INTEL TD guest

Yuan Yao (1):
KVM: TDX: Retry seamcall when TDX_OPERAND_BUSY with operand SEPT

 Documentation/virt/kvm/api.rst                |   37 +-
 Documentation/virt/kvm/index.rst              |    2 +
 .../virt/kvm/intel-tdx-layer-status.rst       |   33 +
 Documentation/virt/kvm/x86/index.rst          |    2 +
 Documentation/virt/kvm/x86/intel-tdx.rst      |  362 ++
 Documentation/virt/kvm/x86/tdx-tdp-mmu.rst    |  443 +++
 arch/x86/events/intel/ds.c                    |    1 +
 arch/x86/include/asm/asm-prototypes.h         |    1 +
 arch/x86/include/asm/kvm-x86-ops.h            |   21 +-
 arch/x86/include/asm/kvm_host.h               |   79 +-
 arch/x86/include/asm/shared/tdx.h             |    9 +-
 arch/x86/include/asm/tdx.h                    |   31 +-
 arch/x86/include/asm/vmx.h                    |   14 +
 arch/x86/include/uapi/asm/kvm.h               |   86 +
 arch/x86/include/uapi/asm/vmx.h               |    5 +-
 arch/x86/kvm/Kconfig                          |    3 +-
 arch/x86/kvm/Makefile                         |    3 +-
 arch/x86/kvm/cpuid.c                          |   27 +-
 arch/x86/kvm/cpuid.h                          |    2 +
 arch/x86/kvm/irq.c                            |    3 +
 arch/x86/kvm/lapic.c                          |   33 +-
 arch/x86/kvm/lapic.h                          |    2 +
 arch/x86/kvm/mmu.h                            |   37 +
 arch/x86/kvm/mmu/mmu.c                        |  247 +-
 arch/x86/kvm/mmu/mmu_internal.h               |   87 +-
 arch/x86/kvm/mmu/mmutrace.h                   |    2 +-
 arch/x86/kvm/mmu/page_track.c                 |    3 +
 arch/x86/kvm/mmu/paging_tmpl.h                |    2 +-
 arch/x86/kvm/mmu/spte.c                       |   17 +-
 arch/x86/kvm/mmu/spte.h                       |   28 +-
 arch/x86/kvm/mmu/tdp_iter.h                   |   14 +-
 arch/x86/kvm/mmu/tdp_mmu.c                    |  442 ++-
 arch/x86/kvm/mmu/tdp_mmu.h                    |    7 +-
 arch/x86/kvm/smm.h                            |    7 +-
 arch/x86/kvm/svm/svm.c                        |    8 +
 arch/x86/kvm/vmx/common.h                     |  166 +
 arch/x86/kvm/vmx/main.c                       | 1268 +++++++
 arch/x86/kvm/vmx/pmu_intel.c                  |   46 +-
 arch/x86/kvm/vmx/pmu_intel.h                  |   28 +
 arch/x86/kvm/vmx/posted_intr.c                |   43 +-
 arch/x86/kvm/vmx/posted_intr.h                |   13 +
 arch/x86/kvm/vmx/tdx.c                        | 3347 +++++++++++++++++
 arch/x86/kvm/vmx/tdx.h                        |  268 ++
 arch/x86/kvm/vmx/tdx_arch.h                   |  273 ++
 arch/x86/kvm/vmx/tdx_errno.h                  |   34 +
 arch/x86/kvm/vmx/tdx_error.c                  |   21 +
 arch/x86/kvm/vmx/tdx_ops.h                    |  400 ++
 arch/x86/kvm/vmx/vmcs.h                       |    5 +
 arch/x86/kvm/vmx/vmx.c                        |  663 +---
 arch/x86/kvm/vmx/vmx.h                        |   52 +-
 arch/x86/kvm/vmx/x86_ops.h                    |  276 ++
 arch/x86/kvm/x86.c                            |  166 +-
 arch/x86/kvm/x86.h                            |    4 +
 arch/x86/virt/vmx/tdx/seamcall.S              |    4 +
 arch/x86/virt/vmx/tdx/tdx.c                   |   95 +-
 arch/x86/virt/vmx/tdx/tdx.h                   |    2 -
 include/linux/kvm_host.h                      |    7 +
 include/linux/kvm_types.h                     |    1 +
 include/uapi/linux/kvm.h                      |   99 +
 virt/kvm/guest_memfd.c                        |    3 +
 virt/kvm/kvm_main.c                           |  115 +-
 61 files changed, 8741 insertions(+), 758 deletions(-)
 create mode 100644 Documentation/virt/kvm/intel-tdx-layer-status.rst
 create mode 100644 Documentation/virt/kvm/x86/intel-tdx.rst
 create mode 100644 Documentation/virt/kvm/x86/tdx-tdp-mmu.rst
 create mode 100644 arch/x86/kvm/vmx/common.h
 create mode 100644 arch/x86/kvm/vmx/main.c
 create mode 100644 arch/x86/kvm/vmx/pmu_intel.h
 create mode 100644 arch/x86/kvm/vmx/tdx.c
 create mode 100644 arch/x86/kvm/vmx/tdx.h
 create mode 100644 arch/x86/kvm/vmx/tdx_arch.h
 create mode 100644 arch/x86/kvm/vmx/tdx_errno.h
 create mode 100644 arch/x86/kvm/vmx/tdx_error.c
 create mode 100644 arch/x86/kvm/vmx/tdx_ops.h
 create mode 100644 arch/x86/kvm/vmx/x86_ops.h

base-commit: ffd2cb6b718e189e7e2d5d0c19c25611f92e061a

2.25.1

From: Kai Huang <[email protected]>

TD_SYSINFO_MAP() macro actually takes the member of the 'struct tdx_tdmr_sysinfo' as the second argument and uses the offsetof() to calculate the offset for that member.

Rename the macro argument _offset to _member to reflect this.

---
 arch/x86/virt/vmx/tdx/tdx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 4d6826a76f78..2aee64d2f27f 100644
@@ -297,9 +297,9 @@ struct field_mapping {
 	int offset;
 };
 
-#define TD_SYSINFO_MAP(_field_id, _offset) \
+#define TD_SYSINFO_MAP(_field_id, _member) \
 	{ .field_id = MD_FIELD_ID_##_field_id,	   \
-	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _offset) }
+	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _member) }
 
 /* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
 static const struct field_mapping fields[] = {

2.25.1

Thanks for review.

I've sent out the first 5 metadata read patches as a standalone patchset separately:

https://lore.kernel.org/kvm/[email protected]/

I'll add your Reviewed-by in next version.

(for others please comment there instead, thanks!)

From: Kai Huang <[email protected]>

The kernel reads all TDMR related global metadata fields based on a table which maps the metadata fields to the corresponding members of 'struct tdx_tdmr_sysinfo'. Currently this table is a static variable.

But this table is only used by the function which reads these metadata fields and becomes useless after reading is done. Change the table to function local variable.

---
 arch/x86/virt/vmx/tdx/tdx.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

index 2aee64d2f27f..cdcb3332bc5d 100644
@@ -301,17 +301,16 @@ struct field_mapping {
 	{ .field_id = MD_FIELD_ID_##_field_id,	   \
 	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _member) }
 
-/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
-static const struct field_mapping fields[] = {
-	TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
-	TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
-	TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
-	TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
-	TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
-};
-
 static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 {
+	/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
+	const struct field_mapping fields[] = {
+		TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
+		TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
+		TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
+		TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
+		TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
+	};
 	int ret;
 	int i;
 

2.25.1

Thanks Kirill. I've split this out and sent out v2, and have added your tag there:

https://lore.kernel.org/lkml/[email protected]/

From: Kai Huang <[email protected]>

For now the kernel only reads TDMR related global metadata fields for module initialization, and the metadata read code only works with the 'struct tdx_tdmr_sysinfo'.

KVM will need to read a bunch of non-TDMR related metadata to create and run TDX guests. It's essential to provide a generic metadata read infrastructure which is not bound to any specific structure.

To start providing such infrastructure, unbound the metadata read with the 'struct tdx_tdmr_sysinfo'.

---
 arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

index cdcb3332bc5d..eb208da4ff63 100644
@@ -273,9 +273,9 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 
 static int read_sys_metadata_field16(u64 field_id,
 				     int offset,
-				     struct tdx_tdmr_sysinfo *ts)
+				     void *stbuf)
 {
-	u16 *ts_member = ((void *)ts) + offset;
+	u16 *st_member = stbuf + offset;
 	u64 tmp;
 	int ret;
 

@@ -287,7 +287,7 @@ static int read_sys_metadata_field16(u64 field_id,
 	if (ret)
 		return ret;
 
-	*ts_member = tmp;
+	*st_member = tmp;
 
 	return 0;
 }

@@ -297,19 +297,22 @@ struct field_mapping {
 	int offset;
 };
 
-#define TD_SYSINFO_MAP(_field_id, _member) \
-	{ .field_id = MD_FIELD_ID_##_field_id,	   \
-	  .offset   = offsetof(struct tdx_tdmr_sysinfo, _member) }
+#define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
+	{ .field_id = MD_FIELD_ID_##_field_id,		\
+	  .offset   = offsetof(_struct, _member) }
+
+#define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
+	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)
 
 static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 {
 	/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
 	const struct field_mapping fields[] = {
-		TD_SYSINFO_MAP(MAX_TDMRS,	      max_tdmrs),
-		TD_SYSINFO_MAP(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
-		TD_SYSINFO_MAP(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
-		TD_SYSINFO_MAP(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
-		TD_SYSINFO_MAP(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
+		TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,		max_tdmrs),
+		TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
+		TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),
+		TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
+		TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
 	};
 	int ret;
 	int i;

2.25.1

From: Kai Huang <[email protected]>

For now the kernel only reads TDMR related global metadata fields for module initialization. All these fields are 16-bits, and the kernel only supports reading 16-bits fields.

KVM will need to read a bunch of non-TDMR related metadata to create and run TDX guests. It's essential to provide a generic metadata read infrastructure which supports reading all 8/16/32/64 bits element sizes.

Extend the metadata read to support reading all these element sizes.

---
 arch/x86/virt/vmx/tdx/tdx.c | 59 +++++++++++++++++++++++++------------
 arch/x86/virt/vmx/tdx/tdx.h |  2 --
 2 files changed, 40 insertions(+), 21 deletions(-)

index eb208da4ff63..a19adc898df6 100644
@@ -271,23 +271,35 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 	return 0;
 }
 
-static int read_sys_metadata_field16(u64 field_id,
-				     int offset,
-				     void *stbuf)
+/* Return the metadata field element size in bytes */
+static int get_metadata_field_bytes(u64 field_id)
 {
-	u16 *st_member = stbuf + offset;
+	/*
+	 * TDX supports 8/16/32/64 bits metadata field element sizes.
+	 * TDX module determines the metadata element size based on the
+	 * "element size code" encoded in the field ID (see the comment
+	 * of MD_FIELD_ID_ELE_SIZE_CODE macro for specific encodings).
+	 */
+	return 1 << MD_FIELD_ID_ELE_SIZE_CODE(field_id);
+}
+
+static int stbuf_read_sys_metadata_field(u64 field_id,
+					 int offset,
+					 int bytes,
+					 void *stbuf)
+{
+	void *st_member = stbuf + offset;
 	u64 tmp;
 	int ret;
 
-	if (WARN_ON_ONCE(MD_FIELD_ID_ELE_SIZE_CODE(field_id) !=
-			MD_FIELD_ID_ELE_SIZE_16BIT))
+	if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes))
 		return -EINVAL;
 
 	ret = read_sys_metadata_field(field_id, &tmp);
 	if (ret)
 		return ret;
 
-	*st_member = tmp;
+	memcpy(st_member, &tmp, bytes);
 
 	return 0;
 }

@@ -295,11 +307,30 @@ static int read_sys_metadata_field16(u64 field_id,
 struct field_mapping {
 	u64 field_id;
 	int offset;
+	int size;
 };
 
 #define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
 	{ .field_id = MD_FIELD_ID_##_field_id,		\
-	  .offset   = offsetof(_struct, _member) }
+	  .offset   = offsetof(_struct, _member),	\
+	  .size     = sizeof(typeof(((_struct *)0)->_member)) }
+
+static int read_sys_metadata(struct field_mapping *fields, int nr_fields,
+			     void *stbuf)
+{
+	int i, ret;
+
+	for (i = 0; i < nr_fields; i++) {
+		ret = stbuf_read_sys_metadata_field(fields[i].field_id,
+				      fields[i].offset,
+				      fields[i].size,
+				      stbuf);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
 
 #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
 	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)

@@ -314,19 +345,9 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 		TD_SYSINFO_MAP_TDMR_INFO(PAMT_2M_ENTRY_SIZE,    pamt_entry_size[TDX_PS_2M]),
 		TD_SYSINFO_MAP_TDMR_INFO(PAMT_1G_ENTRY_SIZE,    pamt_entry_size[TDX_PS_1G]),
 	};
-	int ret;
-	int i;
 
 	/* Populate 'tdmr_sysinfo' fields using the mapping structure above: */
-	for (i = 0; i < ARRAY_SIZE(fields); i++) {
-		ret = read_sys_metadata_field16(fields[i].field_id,
-						fields[i].offset,
-						tdmr_sysinfo);
-		if (ret)
-			return ret;
-	}
-
-	return 0;
+	return read_sys_metadata(fields, ARRAY_SIZE(fields), tdmr_sysinfo);
 }
 
 /* Calculate the actual TDMR size */

index b701f69485d3..4c32c8bf156a 100644
@@ -53,8 +53,6 @@
 #define MD_FIELD_ID_ELE_SIZE_CODE(_field_id)	\
 		(((_field_id) & GENMASK_ULL(33, 32)) >> 32)
 
-#define MD_FIELD_ID_ELE_SIZE_16BIT	1
-
 struct tdmr_reserved_area {
 	u64 offset;
 	u64 size;

2.25.1

Indeed. :-)

From: Kai Huang <[email protected]>

KVM will need to read a bunch of non-TDMR related metadata to create and run TDX guests. Export the metadata read infrastructure for KVM to use.

Specifically, export two helpers:

1) The helper which reads multiple metadata fields to a buffer of a structure based on the "field ID -> structure member" mapping table.

2) The low level helper which just reads a given field ID.

The two helpers cover cases when the user wants to cache a bunch of metadata fields to a certain structure and when the user just wants to query a specific metadata field on demand. They are enough for KVM to use (and also should be enough for other potential users).

---
 arch/x86/include/asm/tdx.h  | 22 ++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.c | 25 ++++++++-----------------
 2 files changed, 30 insertions(+), 17 deletions(-)

index eba178996d84..709b9483f9e4 100644
@@ -116,6 +116,28 @@ static inline u64 sc_retry(sc_func_t func, u64 fn,
 int tdx_cpu_enable(void);
 int tdx_enable(void);
 const char *tdx_dump_mce_info(struct mce *m);
+
+struct tdx_metadata_field_mapping {
+	u64 field_id;
+	int offset;
+	int size;
+};
+
+#define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
+	{ .field_id = MD_FIELD_ID_##_field_id,		\
+	  .offset   = offsetof(_struct, _member),	\
+	  .size     = sizeof(typeof(((_struct *)0)->_member)) }
+
+/*
+ * Read multiple global metadata fields to a buffer of a structure
+ * based on the "field ID -> structure member" mapping table.
+ */
+int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields,
+			  int nr_fields, void *stbuf);
+
+/* Read a single global metadata field */
+int tdx_sys_metadata_field_read(u64 field_id, u64 *data);
+
 #else
 static inline void tdx_init(void) { }
 static inline int tdx_cpu_enable(void) { return -ENODEV; }

index a19adc898df6..dc21310776ab 100644
@@ -251,7 +251,7 @@ static int build_tdx_memlist(struct list_head *tmb_list)
 	return ret;
 }
 
-static int read_sys_metadata_field(u64 field_id, u64 *data)
+int tdx_sys_metadata_field_read(u64 field_id, u64 *data)
 {
 	struct tdx_module_args args = {};
 	int ret;

@@ -270,6 +270,7 @@ static int read_sys_metadata_field(u64 field_id, u64 *data)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(tdx_sys_metadata_field_read);
 
 /* Return the metadata field element size in bytes */
 static int get_metadata_field_bytes(u64 field_id)

@@ -295,7 +296,7 @@ static int stbuf_read_sys_metadata_field(u64 field_id,
 	if (WARN_ON_ONCE(get_metadata_field_bytes(field_id) != bytes))
 		return -EINVAL;
 
-	ret = read_sys_metadata_field(field_id, &tmp);
+	ret = tdx_sys_metadata_field_read(field_id, &tmp);
 	if (ret)
 		return ret;
 

@@ -304,19 +305,8 @@ static int stbuf_read_sys_metadata_field(u64 field_id,
 	return 0;
 }
 
-struct field_mapping {
-	u64 field_id;
-	int offset;
-	int size;
-};
-
-#define TD_SYSINFO_MAP(_field_id, _struct, _member)	\
-	{ .field_id = MD_FIELD_ID_##_field_id,		\
-	  .offset   = offsetof(_struct, _member),	\
-	  .size     = sizeof(typeof(((_struct *)0)->_member)) }
-
-static int read_sys_metadata(struct field_mapping *fields, int nr_fields,
-			     void *stbuf)
+int tdx_sys_metadata_read(const struct tdx_metadata_field_mapping *fields,
+			  int nr_fields, void *stbuf)
 {
 	int i, ret;
 

@@ -331,6 +321,7 @@ static int read_sys_metadata(struct field_mapping *fields, int nr_fields,
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(tdx_sys_metadata_read);
 
 #define TD_SYSINFO_MAP_TDMR_INFO(_field_id, _member)	\
 	TD_SYSINFO_MAP(_field_id, struct tdx_tdmr_sysinfo, _member)

@@ -338,7 +329,7 @@ static int read_sys_metadata(struct field_mapping *fields, int nr_fields,
 static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 {
 	/* Map TD_SYSINFO fields into 'struct tdx_tdmr_sysinfo': */
-	const struct field_mapping fields[] = {
+	const struct tdx_metadata_field_mapping fields[] = {
 		TD_SYSINFO_MAP_TDMR_INFO(MAX_TDMRS,		max_tdmrs),
 		TD_SYSINFO_MAP_TDMR_INFO(MAX_RESERVED_PER_TDMR, max_reserved_per_tdmr),
 		TD_SYSINFO_MAP_TDMR_INFO(PAMT_4K_ENTRY_SIZE,    pamt_entry_size[TDX_PS_4K]),

@@ -347,7 +338,7 @@ static int get_tdx_tdmr_sysinfo(struct tdx_tdmr_sysinfo *tdmr_sysinfo)
 	};
 
 	/* Populate 'tdmr_sysinfo' fields using the mapping structure above: */
-	return read_sys_metadata(fields, ARRAY_SIZE(fields), tdmr_sysinfo);
+	return tdx_sys_metadata_read(fields, ARRAY_SIZE(fields), tdmr_sysinfo);
 }
 
 /* Calculate the actual TDMR size */

2.25.1

No not in this series. Such change needs to be in your series.

Thanks,
-Kai

From: Kai Huang <[email protected]>

Each TDX guest must be protected by its own unique TDX KeyID. KVM will need to tell the TDX module the unique KeyID for a TDX guest when KVM creates it.

Export the TDX KeyID range that can be used by TDX guests for KVM to use. KVM can then manage these KeyIDs and assign one for each TDX guest when it is created.

Each TDX guest has a root control structure called "Trust Domain Root" (TDR). Unlike the rest of the TDX guest, the TDR is protected by the TDX global KeyID. When tearing down the TDR, KVM will need to pass the TDX global KeyID explicitly to the TDX module to flush cache associated to the TDR.

Also export the TDX global KeyID for KVM to tear down the TDR.

---
 arch/x86/include/asm/tdx.h  |  5 +++++
 arch/x86/virt/vmx/tdx/tdx.c | 11 ++++++++---
 2 files changed, 13 insertions(+), 3 deletions(-)

index 709b9483f9e4..16be3a1e4916 100644
@@ -88,6 +88,11 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
 #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
 
 #ifdef CONFIG_INTEL_TDX_HOST
+
+extern u32 tdx_global_keyid;
+extern u32 tdx_guest_keyid_start;
+extern u32 tdx_nr_guest_keyids;
+
 u64 __seamcall(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_ret(u64 fn, struct tdx_module_args *args);
 u64 __seamcall_saved_ret(u64 fn, struct tdx_module_args *args);

index dc21310776ab..d2b8f079a637 100644
@@ -39,9 +39,14 @@
 #include <asm/mce.h>
 #include "tdx.h"
 
-static u32 tdx_global_keyid __ro_after_init;
-static u32 tdx_guest_keyid_start __ro_after_init;
-static u32 tdx_nr_guest_keyids __ro_after_init;
+u32 tdx_global_keyid __ro_after_init;
+EXPORT_SYMBOL_GPL(tdx_global_keyid);
+
+u32 tdx_guest_keyid_start __ro_after_init;
+EXPORT_SYMBOL_GPL(tdx_guest_keyid_start);
+
+u32 tdx_nr_guest_keyids __ro_after_init;
+EXPORT_SYMBOL_GPL(tdx_nr_guest_keyids);
 
 static DEFINE_PER_CPU(bool, tdx_lp_initialized);
 

2.25.1

From: Kai Huang <[email protected]>

KVM will need to make SEAMCALLs to create and run TDX guests. Export SEAMCALL functions for KVM to use.

Also add declaration of SEAMCALL functions to <asm/asm-prototypes.h> to support CONFIG_MODVERSIONS=y.

---
 arch/x86/include/asm/asm-prototypes.h | 1 +
 arch/x86/virt/vmx/tdx/seamcall.S      | 4 ++++
 2 files changed, 5 insertions(+)

index b1a98fa38828..0ec572ad75f1 100644
@@ -13,6 +13,7 @@
 #include <asm/preempt.h>
 #include <asm/asm.h>
 #include <asm/gsseg.h>
+#include <asm/tdx.h>
 
 #ifndef CONFIG_X86_CMPXCHG64
 extern void cmpxchg8b_emu(void);

index 5b1f2286aea9..e32cf82ed47e 100644
@@ -1,5 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0 */
 #include <linux/linkage.h>
+#include <linux/export.h>
 #include <asm/frame.h>
 
 #include "tdxcall.S"

@@ -21,6 +22,7 @@
 SYM_FUNC_START(__seamcall)
 	TDX_MODULE_CALL host=1
 SYM_FUNC_END(__seamcall)
+EXPORT_SYMBOL_GPL(__seamcall);
 
 /*
  * __seamcall_ret() - Host-side interface functions to SEAM software

@@ -40,6 +42,7 @@ SYM_FUNC_END(__seamcall)
 SYM_FUNC_START(__seamcall_ret)
 	TDX_MODULE_CALL host=1 ret=1
 SYM_FUNC_END(__seamcall_ret)
+EXPORT_SYMBOL_GPL(__seamcall_ret);
 
 /*
  * __seamcall_saved_ret() - Host-side interface functions to SEAM software

@@ -59,3 +62,4 @@ SYM_FUNC_END(__seamcall_ret)
 SYM_FUNC_START(__seamcall_saved_ret)
 	TDX_MODULE_CALL host=1 ret=1 saved=1
 SYM_FUNC_END(__seamcall_saved_ret)
+EXPORT_SYMBOL_GPL(__seamcall_saved_ret);

2.25.1

Could you also list the reason that we want to expose __seamcall() directly, rather than wanting to put some higher level wrappers in the TDX host code, and export them?

For example, we can give a summary of the SEAMCALLs (e.g., how many in total, and roughly introduce them based on categories) that will be used by KVM, and clarify the reasons why we want to just export __seamcall().

E.g., we can say something like this:

TD;LR:

KVM roughly will need to use dozens of SEAMCALLs, and all these are logically related to creating and running TDX guests. It makes more sense to just export __seamcall() and let KVM maintain these VM-related wrappers rather than having the TDX host code to provide wrappers for each SEAMCALL or higher-level abstraction.

Long version:

You give a detailed explanation of SEAMCALLs that will be used by KVM, and clarify logically it's better to manage these code in KVM.

Dave, care to weigh in here?

Context (whether to export the generic __seamcall for KVM's use):
https://lore.kernel.org/lkml/[email protected]/

The helpers being discussed are these:
https://lore.kernel.org/lkml/7cfd33d896fce7b49bcf4b7179d0ded22c06b8c2.1708933498.git.isaku.yamahata@intel.com/

I guess there are three options:
1. Export the low level seamcall function
2. Export a bunch of higher level helper functions
3. Duplicate __seamcall asm in KVM

Letting modules make unrestricted seamcalls is not ideal. Preventing the compiler from inlining the small logic in the static inline helpers is not ideal. Duplicating code is not ideal. Hmm.

I want to say 2 sounds the least worst of the three. But I'm not sure. I'm not sure if x86 folks would like to police new seamcalls, or be bothered by it, either.

#3 is the only objectively awful one. :)

In the end, we actually _want_ to have conversations about these things.
There are going to be considerations about what functionality should be
in KVM or the core kernel. We don't want KVM doing any calls that could affect global TDX module state, for instance.

But I'd also defer to the KVM maintainers on this. They're the ones that have to play the symbol exporting game a lot more than I ever do. If they cringe at the idea of adding 20 (or whatever) exports, then that's a lot more important than the possibility of some other silly module abusing the generic exported __seamcall.

Heh, Like this one?

        static inline u64 tdh_sys_lp_shutdown(void)
        {
        	struct tdx_module_args in = {
        	};
        
        	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
        }

Which isn't actually used...

I don't care much about exports. What I do care about is sane code, and while the current code _looks_ pretty, it's actually quite insane.

I get why y'all put SEAMCALL in assembly subroutines; the macro shenanigans I originally wrote years ago were their own brand of crazy, and dealing with GPRs that can't be asm() constraints often results in brittle code.

But the tdx_module_args structure approach generates truly atrocious code. Yes, SEAMCALL is inherently slow, but that doesn't mean that we shouldn't at least try to generate efficient code. And it's not just efficiency that is lost, the generated code ends up being much harder to read than it ought to be.

E.g. the seemingly simple

        static inline u64 tdh_mem_page_remove(hpa_t tdr, gpa_t gpa, int level,
        				      struct tdx_module_args *out)
        {
        	struct tdx_module_args in = {
        		.rcx = gpa | level,
        		.rdx = tdr,
        	};

        	return tdx_seamcall_sept(TDH_MEM_PAGE_REMOVE, &in, out);
        }

generates the below monstrosity with gcc-13. And that's just one SEAMCALL wrapper, *every* single one generates the same mess. clang-16 is kinda sorta a little better, as it at least inlines the helpers that have single callers.

So my feedback is to not worry about the exports, and instead focus on figuring out a way to make the generated code less bloated and easier to read/debug.

Dump of assembler code for function tdh_mem_page_remove:
0x0000000000032b20 <+0>: push %r15
0x0000000000032b22 <+2>: xor %eax,%eax
0x0000000000032b24 <+4>: movabs $0x8000ff0000000006,%r15

   0x0000000000032b2e <+14>:	push   %r14
   0x0000000000032b30 <+16>:	mov    %rcx,%r14
   0x0000000000032b33 <+19>:	mov    $0xb,%ecx
   0x0000000000032b38 <+24>:	push   %r13
   0x0000000000032b3a <+26>:	movslq %edx,%r13
   0x0000000000032b3d <+29>:	push   %r12
   0x0000000000032b3f <+31>:	or     %rsi,%r13
   0x0000000000032b42 <+34>:	mov    $0x11,%r12d
   0x0000000000032b48 <+40>:	push   %rbp
   0x0000000000032b49 <+41>:	movabs $0x8000020300000000,%rbp
   0x0000000000032b53 <+51>:	push   %rbx
   0x0000000000032b54 <+52>:	sub    $0x70,%rsp
   0x0000000000032b58 <+56>:	mov    %rdi,(%rsp)
   0x0000000000032b5c <+60>:	lea    0x18(%rsp),%rdi
   0x0000000000032b61 <+65>:	rep stos %rax,%es:(%rdi)
   0x0000000000032b64 <+68>:	mov    (%rsp),%rax
   0x0000000000032b68 <+72>:	mov    %r13,(%r14)
   0x0000000000032b6b <+75>:	mov    $0xa,%ebx
   0x0000000000032b70 <+80>:	mov    %rax,0x8(%r14)
   0x0000000000032b74 <+84>:	mov    0x18(%rsp),%rax
   0x0000000000032b79 <+89>:	mov    %rax,0x10(%r14)
   0x0000000000032b7d <+93>:	mov    0x20(%rsp),%rax
   0x0000000000032b82 <+98>:	mov    %rax,0x18(%r14)
   0x0000000000032b86 <+102>:	mov    0x28(%rsp),%rax
   0x0000000000032b8b <+107>:	mov    %rax,0x20(%r14)
   0x0000000000032b8f <+111>:	mov    0x30(%rsp),%rax
   0x0000000000032b94 <+116>:	mov    %rax,0x28(%r14)
   0x0000000000032b98 <+120>:	mov    0x38(%rsp),%rax
   0x0000000000032b9d <+125>:	mov    %rax,0x30(%r14)
   0x0000000000032ba1 <+129>:	mov    0x40(%rsp),%rax
   0x0000000000032ba6 <+134>:	mov    %rax,0x38(%r14)
   0x0000000000032baa <+138>:	mov    0x48(%rsp),%rax
   0x0000000000032baf <+143>:	mov    %rax,0x40(%r14)
   0x0000000000032bb3 <+147>:	mov    0x50(%rsp),%rax
   0x0000000000032bb8 <+152>:	mov    %rax,0x48(%r14)
   0x0000000000032bbc <+156>:	mov    0x58(%rsp),%rax
   0x0000000000032bc1 <+161>:	mov    %rax,0x50(%r14)
   0x0000000000032bc5 <+165>:	mov    0x60(%rsp),%rax
   0x0000000000032bca <+170>:	mov    %rax,0x58(%r14)
   0x0000000000032bce <+174>:	mov    0x68(%rsp),%rax
   0x0000000000032bd3 <+179>:	mov    %rax,0x60(%r14)
   0x0000000000032bd7 <+183>:	mov    %r14,%rsi
   0x0000000000032bda <+186>:	mov    $0x1d,%edi
   0x0000000000032bdf <+191>:	call   0x32be4 <tdh_mem_page_remove+196>
   0x0000000000032be4 <+196>:	cmp    %rbp,%rax
   0x0000000000032be7 <+199>:	jne    0x32bfd <tdh_mem_page_remove+221>
   0x0000000000032be9 <+201>:	sub    $0x1,%ebx
   0x0000000000032bec <+204>:	jne    0x32bd7 <tdh_mem_page_remove+183>
   0x0000000000032bee <+206>:	add    $0x70,%rsp
   0x0000000000032bf2 <+210>:	pop    %rbx
   0x0000000000032bf3 <+211>:	pop    %rbp
   0x0000000000032bf4 <+212>:	pop    %r12
   0x0000000000032bf6 <+214>:	pop    %r13
   0x0000000000032bf8 <+216>:	pop    %r14
   0x0000000000032bfa <+218>:	pop    %r15
   0x0000000000032bfc <+220>:	ret
   0x0000000000032bfd <+221>:	cmp    %r15,%rax
   0x0000000000032c00 <+224>:	je     0x32c2a <tdh_mem_page_remove+266>
   0x0000000000032c02 <+226>:	movabs $0x8000020000000092,%rdx
   0x0000000000032c0c <+236>:	cmp    %rdx,%rax
   0x0000000000032c0f <+239>:	jne    0x32bee <tdh_mem_page_remove+206>
   0x0000000000032c11 <+241>:	sub    $0x1,%r12d
   0x0000000000032c15 <+245>:	jne    0x32b64 <tdh_mem_page_remove+68>
   0x0000000000032c1b <+251>:	add    $0x70,%rsp
   0x0000000000032c1f <+255>:	pop    %rbx
   0x0000000000032c20 <+256>:	pop    %rbp
   0x0000000000032c21 <+257>:	pop    %r12
   0x0000000000032c23 <+259>:	pop    %r13
   0x0000000000032c25 <+261>:	pop    %r14
   0x0000000000032c27 <+263>:	pop    %r15
   0x0000000000032c29 <+265>:	ret
   0x0000000000032c2a <+266>:	call   0x32c2f <tdh_mem_page_remove+271>
   0x0000000000032c2f <+271>:	xor    %eax,%eax
   0x0000000000032c31 <+273>:	add    $0x70,%rsp
   0x0000000000032c35 <+277>:	pop    %rbx
   0x0000000000032c36 <+278>:	pop    %rbp
   0x0000000000032c37 <+279>:	pop    %r12
   0x0000000000032c39 <+281>:	pop    %r13
   0x0000000000032c3b <+283>:	pop    %r14
   0x0000000000032c3d <+285>:	pop    %r15

0x0000000000032c3f <+287>: ret
End of assembler dump.

Oh, and please make it a collaborative, public effort. I don't want to hear crickets and then see v20 dropped with a completely new SEAMCALL scheme.

And here we we're worrying that people might eventually grow tired of us adding mails to v19 and we debate every detail in public. Will do.

As a general rule, I _strongly_ prefer all review to be done on-list, in public. Copy+pasting myself from another Intel series[*]

: Correct, what I object to is Intel _requiring_ a Reviewed-by before posting. :
: And while I'm certainly not going to refuse patches that have been reviewed : internally, I _strongly_ prefer reviews be on-list so that they are public and : recorded. Being able to go back and look at the history and evolution of patches : is valuable, and the discussion itself is often beneficial to non-participants, : e.g. people that are new-ish to KVM and/or aren't familiar with the feature being : enabled can often learn new things and avoid similar pitfalls of their own.

There are definitely situations where exceptions are warranted, e.g. if someone is a first-time poster and/or wants a sanity check to make sure their idea isn't completely crazy. But even then, the internal review should only be very cursory.

In addition to the history being valuable, doing reviews in public minimizes the probability of a developer being led astray, e.g. due to someone internally saying do XYZ, and then upstream reviewers telling them to do something entirely different.

As far as noise goes, look at it this way. Every time a new TDX series is posted, I get 130+ emails. Y'all can do a _lot_ of public review and discussion before you'll get anywhere near the point where it'd be noiser than spinning a new version of the series.

[*] https://lore.kernel.org/all/[email protected]

Looks like is was turned into a NOP in TDX 1.5. So will even forever be dead code. I see one other that is unused. Thanks for pointing it out.

I guess it must be this, for the initiated:
https://lore.kernel.org/lkml/25f0d2c2f73c20309a1b578cc5fc15f4fd6b9a13.1605232743.git.isaku.yamahata@intel.com/

[snip]

Thanks for the feedback both! It sounds like everyone is flexible on the exports. As for the generated code, oof.

Kai, I see the solution has gone through some iterations already. First the macro one linked above, then that was dropped pretty quick to something that loses the asm constraints:
https://lore.kernel.org/lkml/e777bbbe10b1ec2c37d85dcca2e175fe3bc565ec.1625186503.git.isaku.yamahata@intel.com/

Then next the struct grew here, and here:
https://lore.kernel.org/linux-mm/[email protected]/
https://lore.kernel.org/linux-mm/[email protected]/

Not sure I understand all of the constraints yet. Do you have any ideas?

Hmm.. I lost memory of this. :-(

Sorry I forgot for what reason we changed from the (bunch of) macros to this.

This was due to Peter's request to unify the TDCALL/SEAMCALL and TDVMCALL assembly, which was done by this series:

https://lore.kernel.org/lkml/[email protected]/

So when Peter requested this, the __seamcall() and __tdcall() (or __tdx_module_call() before the above series) were already sharing one assembly macro. The TDVMCALL were using a different macro, though. However the two assembly macros used similar structure and similar code, so we tried to unify them.

And another reason that we changed from ...

  u64 __seamcall(u64 fn, u64 rcx, u64 rdx, ..., struct tdx_module_args *out);

to ...

  u64 __seamcall(u64, struct tdx_module_args *args);

... was the former doesn't extend.

E.g., live migration related new SEAMCALLs use more registers as input. It is just insane to have so many individual regs as function argument.

And Peter wanted to use __seamcall() to cover TDH.VP.ENTER too, which historically had been implemented in KVM using its own assembly, because VP.ENTER basically uses all GPRs as input/output.

Yeah, that's really awful.

Is all the inlining making the compiler too ambitious? Why is this all inlined in the first place?

tdh_mem_page_remove() _should_ just be logically:

* initialize tdx_module_args. Move a few things into place on the stack and zero the rest.
* Put a pointer to tdx_module_args in a register
* Put TDH_MEM_PAGE_REMOVE immediate in a register
* Some register preservation, maybe
* call
* maybe some cleanup
* return

Those logical things are *NOT* easy to spot in the disassembly.

No, whether or not the wrappers are inlined doesn't change anything. gcc actually doesn't inline any of these helpers. More below.

Likely because no one looked at the generated code. The C code is super simple and looks like it should be inlined.

And the very original code was macro heavy, i.e. relied on inlining to allow the compiler to precisely set only the actual registers needed for the SEAMCALL.

The "zero the rest" is what generates the fugly code. The underlying problem is that the SEAMCALL assembly functions unpack _all_ registers from tdx_module_args. As a result, tdx_module_args needs to be zeroed to avoid loading registers with unitialized stack data.

E.g. before I looked at the assembly code, my initial thought to clean things up by doing:

	struct tdx_module_args in;

	in.rcx = gpa | level;
	in.rdx = tdr;

but that would make one or more sanitizers (and maybe even the compiler itself) more than a bit unhappy.

The struct is 72 bytes, which adds up to a lot of wasted effort since the majority of SEAMCALLs only use a few of the 13 registers.

FWIW, the guest side of TDX is equally gross. E.g. to do kvm_hypercall1(), which without TDX is simply

0xffffffff810eb4a2 <+82>: 48 c7 c0 8c 9a 01 00 mov $0x19a8c,%rax

   0xffffffff810eb4a9 <+89>:	8b 1c 02           	mov    (%rdx,%rax,1),%ebx
   0xffffffff810eb4ac <+92>:	b8 0b 00 00 00     	mov    $0xb,%eax
   0xffffffff810eb4b1 <+97>:	0f 01 c1           	vmcall
   0xffffffff810eb4b4 <+100>:	5b                 	pop    %rbx
   0xffffffff810eb4b5 <+101>:	5d                 	pop    %rbp

the kernel blasts the unused params

0xffffffff810ffdc1 <+97>: bf 0b 00 00 00 mov $0xb,%edi

   0xffffffff810ffdc6 <+102>:	31 d2              	xor    %edx,%edx
   0xffffffff810ffdc8 <+104>:	31 c9              	xor    %ecx,%ecx
   0xffffffff810ffdca <+106>:	45 31 c0           	xor    %r8d,%r8d
   0xffffffff810ffdcd <+109>:	5b                 	pop    %rbx
   0xffffffff810ffdce <+110>:	41 5e              	pop    %r14
   0xffffffff810ffdd0 <+112>:	e9 bb 1b f0 ff     	jmp    0xffffffff81001990 <tdx_kvm_hypercall>

then loads and zeros a ton of memory (tdx_kvm_hypercall()):

   0xffffffff81001990 <+0>:	nopl   0x0(%rax,%rax,1)
   0xffffffff81001995 <+5>:	sub    $0x70,%rsp
   0xffffffff81001999 <+9>:	mov    %gs:0x28,%rax
   0xffffffff810019a2 <+18>:	mov    %rax,0x68(%rsp)
   0xffffffff810019a7 <+23>:	mov    %edi,%eax
   0xffffffff810019a9 <+25>:	movq   $0x0,0x18(%rsp)
   0xffffffff810019b2 <+34>:	movq   $0x0,0x10(%rsp)
   0xffffffff810019bb <+43>:	movq   $0x0,0x8(%rsp)
   0xffffffff810019c4 <+52>:	movq   $0x0,(%rsp)
   0xffffffff810019cc <+60>:	mov    %rax,0x20(%rsp)
   0xffffffff810019d1 <+65>:	mov    %rsi,0x28(%rsp)
   0xffffffff810019d6 <+70>:	mov    %rdx,0x30(%rsp)
   0xffffffff810019db <+75>:	mov    %rcx,0x38(%rsp)
   0xffffffff810019e0 <+80>:	mov    %r8,0x40(%rsp)
   0xffffffff810019e5 <+85>:	movq   $0x0,0x48(%rsp)
   0xffffffff810019ee <+94>:	movq   $0x0,0x50(%rsp)
   0xffffffff810019f7 <+103>:	movq   $0x0,0x58(%rsp)
   0xffffffff81001a00 <+112>:	movq   $0x0,0x60(%rsp)
   0xffffffff81001a09 <+121>:	mov    %rsp,%rdi
   0xffffffff81001a0c <+124>:	call   0xffffffff819f0a80 <__tdx_hypercall>
   0xffffffff81001a11 <+129>:	mov    %gs:0x28,%rcx
   0xffffffff81001a1a <+138>:	cmp    0x68(%rsp),%rcx
   0xffffffff81001a1f <+143>:	jne    0xffffffff81001a26 <tdx_kvm_hypercall+150>
   0xffffffff81001a21 <+145>:	add    $0x70,%rsp

0xffffffff81001a25 <+149>: ret

and then unpacks all of that memory back into registers, and reverses that last part on the way back, (__tdcall_saved_ret()):

   0xffffffff819f0b10 <+0>:	mov    %rdi,%rax
   0xffffffff819f0b13 <+3>:	mov    (%rsi),%rcx
   0xffffffff819f0b16 <+6>:	mov    0x8(%rsi),%rdx
   0xffffffff819f0b1a <+10>:	mov    0x10(%rsi),%r8
   0xffffffff819f0b1e <+14>:	mov    0x18(%rsi),%r9
   0xffffffff819f0b22 <+18>:	mov    0x20(%rsi),%r10
   0xffffffff819f0b26 <+22>:	mov    0x28(%rsi),%r11
   0xffffffff819f0b2a <+26>:	push   %rbx
   0xffffffff819f0b2b <+27>:	push   %r12
   0xffffffff819f0b2d <+29>:	push   %r13
   0xffffffff819f0b2f <+31>:	push   %r14
   0xffffffff819f0b31 <+33>:	push   %r15
   0xffffffff819f0b33 <+35>:	mov    0x30(%rsi),%r12
   0xffffffff819f0b37 <+39>:	mov    0x38(%rsi),%r13
   0xffffffff819f0b3b <+43>:	mov    0x40(%rsi),%r14
   0xffffffff819f0b3f <+47>:	mov    0x48(%rsi),%r15
   0xffffffff819f0b43 <+51>:	mov    0x50(%rsi),%rbx
   0xffffffff819f0b47 <+55>:	push   %rsi
   0xffffffff819f0b48 <+56>:	mov    0x58(%rsi),%rdi
   0xffffffff819f0b4c <+60>:	mov    0x60(%rsi),%rsi
   0xffffffff819f0b50 <+64>:	tdcall
   0xffffffff819f0b54 <+68>:	push   %rax
   0xffffffff819f0b55 <+69>:	mov    0x8(%rsp),%rax
   0xffffffff819f0b5a <+74>:	mov    %rsi,0x60(%rax)
   0xffffffff819f0b5e <+78>:	pop    %rax
   0xffffffff819f0b5f <+79>:	pop    %rsi
   0xffffffff819f0b60 <+80>:	mov    %r12,0x30(%rsi)
   0xffffffff819f0b64 <+84>:	mov    %r13,0x38(%rsi)
   0xffffffff819f0b68 <+88>:	mov    %r14,0x40(%rsi)
   0xffffffff819f0b6c <+92>:	mov    %r15,0x48(%rsi)
   0xffffffff819f0b70 <+96>:	mov    %rbx,0x50(%rsi)
   0xffffffff819f0b74 <+100>:	mov    %rdi,0x58(%rsi)
   0xffffffff819f0b78 <+104>:	mov    %rcx,(%rsi)
   0xffffffff819f0b7b <+107>:	mov    %rdx,0x8(%rsi)
   0xffffffff819f0b7f <+111>:	mov    %r8,0x10(%rsi)
   0xffffffff819f0b83 <+115>:	mov    %r9,0x18(%rsi)
   0xffffffff819f0b87 <+119>:	mov    %r10,0x20(%rsi)
   0xffffffff819f0b8b <+123>:	mov    %r11,0x28(%rsi)
   0xffffffff819f0b8f <+127>:	xor    %ecx,%ecx
   0xffffffff819f0b91 <+129>:	xor    %edx,%edx
   0xffffffff819f0b93 <+131>:	xor    %r8d,%r8d
   0xffffffff819f0b96 <+134>:	xor    %r9d,%r9d
   0xffffffff819f0b99 <+137>:	xor    %r10d,%r10d
   0xffffffff819f0b9c <+140>:	xor    %r11d,%r11d
   0xffffffff819f0b9f <+143>:	xor    %r12d,%r12d
   0xffffffff819f0ba2 <+146>:	xor    %r13d,%r13d
   0xffffffff819f0ba5 <+149>:	xor    %r14d,%r14d
   0xffffffff819f0ba8 <+152>:	xor    %r15d,%r15d
   0xffffffff819f0bab <+155>:	xor    %ebx,%ebx
   0xffffffff819f0bad <+157>:	xor    %edi,%edi
   0xffffffff819f0baf <+159>:	pop    %r15
   0xffffffff819f0bb1 <+161>:	pop    %r14
   0xffffffff819f0bb3 <+163>:	pop    %r13
   0xffffffff819f0bb5 <+165>:	pop    %r12
   0xffffffff819f0bb7 <+167>:	pop    %rbx

0xffffffff819f0bb8 <+168>: ret

It's honestly quite amusing, because y'all took one what I see as one of the big advantages of TDX over SEV (using registers instead of shared memory), and managed to effectively turn it into a disadvantage.

Again, I completely understand the maintenance and robustness benefits, but IMO the pendulum swung a bit too far in that direction.

It's the "zero the rest" and also the copy:

[...]

[...]

Having to zero-and-init the structure and store output regs to the structure is an unfortunately burden if we want to have a single API __seamcall() for all SEAMCALL leafs.

To (precisely) avoid writing to the unnecessary structure members before and after the SEAMCALL instruction, we need to use your old way to have bunch of macros. But we may end up with *a lot* macros due to needing to cover new (e.g., live migration) SEAMCALLs.

Because essentially it's a game to implement wrappers for bunch of combinations of each individual input/output regs.

I don't want to judge which way is better, but to be honest I think completely switching to the old way (using bunch of macros) isn't a realistic option at this stage.

However, I think we might be able to change ...

    u64 __seamcall(u64 fn, struct tdx_module_args *args);

... to

u64 __seamcall(u64 fn, struct tdx_module_args *in,
struct tdx_module_args *out);

.. so that the assembly can actually skip "storing output regs to the structure" if the SEAMCALL doesn't really have any output regs except RAX.

I can try to do if you guys believe this should be done, and should be done earlier than later, but I am not sure _ANY_ optimization around SEAMCALL will have meaningful performance improvement.

I don't think Sean had performance concerns.

I think he was having a justifiably violent reaction to how much more complicated the generated code is to do a SEAMCALL versus a good ol' KVM hypercall.

"Complicated" in this case means lots more instructions and control flow. That's slower, sure, but the main impact is that when you go to debug it, it's *MUCH* harder to debug the SEAMCALL entry assembly than a KVM hypercall.

My takeaway from this, though, is that we are relying on the compiler for a *LOT*. There are also so many levels in the helpers that it's hard to avoid silly things like two _separate_ retry loops.

We should probably be looking at the generated code a _bit_ more often than never, and it's OK to tinker a _bit_ to make things out-of-line or make sure that the compiler optimizes everything that we think that it should.

Also remember that there are very fun tools out there that can make this much easier than recompiling the kernel a billion times:

	https://godbolt.org/z/8ooE4d465

Ah, I automatically linked "generating better code" to "in order to have better performance". My bad.

[...]

Yeah, agreed.

Ah, this is useful. Thanks for sharing :-)

Yep. The code essentially violates the principle of least surprise. I genuinely thought I was dumping the wrong function(s) when I first looked at the output.

I think it was mistake trying to centralize TDCALL/SEAMCALL calls into few megawrappers. I think we can get better results by shifting leaf function wrappers into assembly.

We are going to have more assembly, but it should produce better result. Adding macros can help to write such wrapper and minimizer boilerplate.

Below is an example of how it can look like. It's not complete. I only converted TDCALLs, but TDVMCALLs or SEAMCALLs. TDVMCALLs are going to be more complex.

Any opinions? Is it something worth investing more time?

.set offset_rcx, TDX_MODULE_rcx
.set offset_rdx, TDX_MODULE_rdx
.set offset_r8, TDX_MODULE_r8
.set offset_r9, TDX_MODULE_r9
.set offset_r10, TDX_MODULE_r10
.set offset_r11, TDX_MODULE_r11

.macro save_output struct_reg regs:vararg
.irp reg,\regs

	movq	%\reg, offset_\reg(%\struct_reg)

.endr
.endm

.macro tdcall leaf

	movq	\leaf, %rax
	.byte	0x66,0x0f,0x01,0xcc

.endm

.macro tdcall_or_panic leaf
tdcall \leaf
testq %rax, %rax
jnz .Lpanic
.endm

SYM_FUNC_START(tdg_vm_rd)

	FRAME_BEGIN

	xorl	%ecx, %ecx
	movq	%rdi, %rdx

	tdcall_or_panic $TDG_VM_RD

	movq	%r8, %rax

	RET
	FRAME_END

SYM_FUNC_END(tdg_vm_rd)

SYM_FUNC_START(tdg_vm_wr)

	FRAME_BEGIN

	xorl	%ecx, %ecx
	movq	%rsi, %r8
	movq	%rdx, %r9
	movq	%rdi, %rdx

	tdcall_or_panic $TDG_VM_WR

	/* Old value */
	movq	%r8, %rax

	RET
	FRAME_END

SYM_FUNC_END(tdg_vm_wr)

SYM_FUNC_START(tdcs_ctls_set)

	FRAME_BEGIN

	movq	$TDCS_TD_CTLS, %rdx
	xorl	%ecx, %ecx
	movq	%rdi, %r8
	movq	%rdi, %r9

	tdcall $TDG_VM_WR

	testq	%rax, %rax
	setz	%al

	RET
	FRAME_END

SYM_FUNC_END(tdcs_ctls_set)

SYM_FUNC_START(tdg_sys_rd)

	FRAME_BEGIN

	xorl	%ecx, %ecx
	movq	%rdi, %rdx

	tdcall_or_panic $TDG_SYS_RD

	movq	%r8, %rax

	RET
	FRAME_END

SYM_FUNC_END(tdg_sys_rd)

SYM_FUNC_START(tdg_vp_veinfo_get)
FRAME_BEGIN

tdcall_or_panic $TDG_VP_VEINFO_GET

save_output struct_reg=rdi regs=rcx,rdx,r8,r9,r10

FRAME_END
RET
SYM_FUNC_END(tdg_vp_veinfo_get)

SYM_FUNC_START(tdg_vp_info)

	FRAME_BEGIN

	tdcall_or_panic $TDG_VP_INFO

	save_output struct_reg=rdi regs=rcx,rdx,r8,r9,r10,r11

	FRAME_END
	RET

SYM_FUNC_END(tdg_vp_info)

SYM_FUNC_START(tdg_mem_page_accept)

	FRAME_BEGIN

	movq	%rdi, %rcx

	tdcall $TDG_MEM_PAGE_ACCEPT

	FRAME_END
	RET

SYM_FUNC_END(tdg_mem_page_accept)

SYM_FUNC_START(tdg_mr_report)

	FRAME_BEGIN

	movq	%rdx, %r8
	movq	%rdi, %rcx
	movq	%rsi, %rdx

	tdcall $TDG_MR_REPORT

	FRAME_END
	RET

SYM_FUNC_END(tdg_mr_report)

.Lpanic:

	ud2

  Kiryl Shutsemau / Kirill A. Shutemov

We discussed offline how implementing these for each TDVM/SEAMCALL increases the chances of a bug in just one TDVM/SEAMCALL. Which could making debugging problems more challenging. Kirill raised the possibility of some code generating solution like cpufeatures.h, that could take a spec and generate correct calls.

So far no big wins have presented themselves. Kirill, do we think the path to move the messy part out-of-line will not work?

I converted all TDCALL and TDVMCALL leafs to direct assembly wrappers. Here's WIP branch: https://github.com/intel/tdx/commits/guest-tdx-asm/

I still need to clean it up and write commit messages and comments for all wrappers.

Now I think it worth the shot.

Any feedback?

  Kiryl Shutsemau / Kirill A. Shutemov

I find it hard to review for correctness, and extremely susceptible to developer error. E.g. lots of copy+paste, and manual encoding of RCX to expose registers. It also bleeds TDX ABI into C code, e.g.

	/*
	 * As per TDX GHCI CPUID ABI, r12-r15 registers contain contents of
	 * EAX, EBX, ECX, EDX registers after the CPUID instruction execution.
	 * So copy the register contents back to pt_regs.
	 */
	regs->ax = args.r12;
	regs->bx = args.r13;
	regs->cx = args.r14;
	regs->dx = args.r15;

Oh, and it requires input/output paramters, which is quite gross for C code *and* for assembly code, e.g.

	u64 tdvmcall_map_gpa(u64 *gpa, u64 size);

and then the accompanying assembly code:

		FRAME_BEGIN

		save_regs r12,r13

		movq	(%rdi), %r12
		movq	%rsi, %r13

		movq	$(TDX_R10 | TDX_R11 | TDX_R12 | TDX_R13), %rcx

		tdvmcall	$TDVMCALL_MAP_GPA

		movq	%r11, (%rdi)

		restore_regs r13,r12

		FRAME_END
		RET

I think having one trampoline makes sense, e.g. to minimize the probability of leaking register state to the VMM. The part that I don't like, and which generates awful code, is shoving register state into a memory structure.

The annoying part with the TDX ABI is that it heavily uses r8-r15, and asm() constraints don't play nice with r8-15. But that doesn't mean we can't use asm() with macros, it just means we have to play games with registers.

Because REG-REG moves are super cheap, and ignoring the fatal error goofiness, there are at most four inputs. That means having a single trampoline take *all* possible inputs is a non-issue. And we can avoiding polluting the inline code if we bury the register shuffling in the trampoline.

And if we use asm() wrappers to call the trampoline, then the trampoline doesn't need to precisely follow the C calling convention. I.e. the trampoline can return with the outputs still in r12-r15, and let the asm() wrappers extract the outputs they want.

As it stands, TDVMCALLs either have 0, 1, or 4 outputs. I.e. we only need three asm() wrappers. We could get away with one wrapper, but then users of the wrappers would need dummy variables for inputs *and* outputs, and the outputs get gross.

Completely untested, but this is what I'm thinking. Side topic, I think making "tdcall" a macro that takes a leaf is a mistake. If/when an assembler learns what tdcall is, we're going to have to rewrite all of that code. And what a coincidence, my suggestion needs a bare TDCALL! :-)

Side topic #2, I don't think the trampoline needs a stack frame, its a leaf function.

Side topic #3, the ud2 to induce panic should be out-of-line.

Weird? Yeah. But at least we one need to document one weird calling convention, and the ugliness is contained to three macros and a small assembly function.

.pushsection .noinstr.text, "ax"
SYM_FUNC_START(tdvmcall_trampoline)

	movq	$TDX_HYPERCALL_STANDARD, %r10
	movq	%rax, %r11
	movq	%rdi, %r12
	movq	%rsi, %r13
	movq	%rdx, %r14
	movq	%rcx, %r15

	movq	$(TDX_R10 | TDX_R11 | TDX_R12 | TDX_R13 | TDX_R14 | TDX_R15), %rcx

	tdcall

	testq	%rax, %rax
	jnz	.Lpanic

	ret

.Lpanic:

	ud2

SYM_FUNC_END(tdvmcall_trampoline)
.popsection

#define TDVMCALL(reason, in1, in2, in3, in4) \

({									\
	long __ret;							\
									\
	asm(								\
		"call tdvmcall_trampoline\n\t"				\
		"mov %%r10, %0\n\t"					\
		: "=r" (__ret)						\
		: "D" (in1), "S"(in2), "d"(in3), "c" (in4)		\
		: "r12", "r13", "r14", "r15"				\
	);								\
	__ret;								\
})

#define TDVMCALL_1(reason, in1, in2, in3, in4, out1) \

({									\
	long __ret;							\
									\
	asm(								\
		"call tdvmcall_trampoline\n\t"				\
		"mov %%r10, %0\n\t"					\
		"mov %%r12, %1\n\t"					\
		: "=r"(__ret) "=r" (out1)				\
		: "a"(reason), "D" (in1), "S"(in2), "d"(in3), "c" (in4)	\
		: "r12", "r13", "r14", "r15"				\
	);								\
	__ret;								\
})

#define TDVMCALL_4(reason, in1, in2, in3, in4, out1, out2, out3, out4) \

({									\
	long __ret;							\
									\
	asm(								\
		"call tdvmcall_trampoline\n\t"				\
		"mov %%r10, %0\n\t"					\
		"mov %%r12, %1\n\t"					\
		"mov %%r13, %2\n\t"					\
		"mov %%r14, %3\n\t"					\
		"mov %%r15, %4\n\t"					\
		: "=r" (__ret),						\
		  "=r" (out1), "=r" (out2), "=r" (out3), "=r" (out4)	\
		: "a"(reason), "D" (in1), "S"(in2), "d"(in3), "c" (in4)	\
		  [reason] "i" (reason) 				\
		: "r12", "r13", "r14", "r15"				\
	);								\
	__ret;								\
})

static int handle_halt(struct ve_info *ve)

{
	if (TDVMCALL(EXIT_REASON_HALT, irqs_disabled(), 0, 0, 0))
		return -EIO;

	return ve_instr_len(ve);
}

void __cpuidle tdx_safe_halt(void)

{
	WARN_ONCE(TDVMCALL(EXIT_REASON_HALT, false, 0, 0, 0),
		  "HLT instruction emulation failed");
}

static int read_msr(struct pt_regs *regs, struct ve_info *ve)

{
	u64 val;

	if (TDVMCALL_1(EXIT_REASON_MSR_READ, regs->cx, 0, 0, 0, val))
		return -EIO;

	regs->ax = lower_32_bits(val);
	regs->dx = upper_32_bits(val);

	return ve_instr_len(ve);
}

static int write_msr(struct pt_regs *regs, struct ve_info *ve)

{
	u64 val = (u64)regs->dx << 32 | regs->ax;

	if (TDVMCALL(EXIT_REASON_MSR_WRITE, regs->cx, val, 0, 0))
		return -EIO;

	return ve_instr_len(ve);
}
static int handle_cpuid(struct pt_regs *regs, struct ve_info *ve)
{
	/*
	 * Only allow VMM to control range reserved for hypervisor
	 * communication.
	 *
	 * Return all-zeros for any CPUID outside the range. It matches CPU
	 * behaviour for non-supported leaf.
	 */
	if (regs->ax < 0x40000000 || regs->ax > 0x4FFFFFFF) {
		regs->ax = regs->bx = regs->cx = regs->dx = 0;
		return ve_instr_len(ve);
	}

	if (TDVMCALL_4(EXIT_REASON_CPUID, regs->ax, regs->cx, 0, 0,
		       regs->ax, regs->bx, regs->cx, regs->dx))
		return -EIO;

	return ve_instr_len(ve);
}

static bool mmio_read(int size, u64 gpa, u64 *val)

{
	*val = 0;
	return !TDVMCALL_1(EXIT_REASON_EPT_VIOLATION, size, EPT_READ, gpa, 0, val);
}

static bool mmio_write(int size, u64 gpa, u64 val)

{
	return !TDVMCALL(EXIT_REASON_EPT_VIOLATION, size, EPT_WRITE, gpa, val);
}

Yes, I agree. The approach requires careful manual work and is error-prone. I was planning to get around and stare at every wrapper to make sure it is correct. This approach is not scalable, though.

I don't think we can get away with single trampoline. We have outliers.

See TDG.VP.VMCALL<ReportFatalError> that uses pretty much all registers as input. And I hope we wouldn't need TDG.VP.VMCALL<Instruction.PCONFIG> any time soon. It uses all possible output registers.

But I guess we can make a *few* wrappers that covers all needed cases.

I guess rename the macro (if it still needed) to something like tdcall_leaf or something.

Hm. I guess.

Yeah. I switched to the inline one while debugging one section mismatch issue and forgot to switch back.

Okay, the approach is worth exploring. I can work on it.

You focuses here on TDVMCALL. What is your take on the rest of TDCALL?

It is r11, not r12.

  Kiryl Shutsemau / Kirill A. Shutemov

Yeah, I should have said "one trampoline for all of the not insane APIs" :-)

XMM: just say no.

Yeah. I suspect two will suffice. One for the calls that say at or below four inputs, and one for the fat ones like ReportFatalError that use everything under the sun.

For the latter one, marshalling data into registers via an in-memory structure makes, especially if we move away from tdx_module_args, e.g. this is quite clean and reasonable:

	union {
		/* Define register order according to the GHCI */
		struct { u64 r14, r15, rbx, rdi, rsi, r8, r9, rdx; };

		char str[64];
	} message;

	/* VMM assumes '\0' in byte 65, if the message took all 64 bytes */
	strtomem_pad(message.str, msg, '\0');

	/*
	 * This hypercall should never return and it is not safe
	 * to keep the guest running. Call it forever if it
	 * happens to return.
	 */
	while (1)
		tdx_fat_tdvmcall(&message);

Not sure, haven't looked at them recently. At a glance, something similar? The use of high registers instead of RDI and RSI is damn annoying :-/

Hmm, but it looks like there are enough simple TDCALLs that stay away from high registers that open coding inline asm() is a viable (best?) approach.

RAX being the leaf and the return value is annoying, so maybe a simple macro to make it easier to deal with that? It won't allow for perfectly optimal code generation, but forcing a MOV for a TDCALL isn't going to affect performance, and it will make reading the code dead simple.

  #define tdcall_leaf(leaf) "mov $" leaf ", %%eax\n\t.byte 0x66,0x0f,0x01,0xcc\n\t"

Then PAGE_ACCEPT is simply:

	asm(tdcall_leaf(TDG_MEM_PAGE_ACCEPT)
	    : "=a"(ret),
            : "c"(start | page_size));
	if (ret)
		return 0;

And even the meanies that use R8 are reasonably easy to handle:

	asm("xor %%r8d, %%r8d\n\t"
	    tdcall_leaf(TDG_MR_REPORT)
	    : "=a"(ret)
	    : "c"(__pa(report)), "d"(__pa(data)));

and (though using names for the outputs, I just can't remember the syntax as I'm typing this :-/)

	asm(tdcall_leaf(TDG_VM_RD)
	    "mov %%r8, %0\n\t"
	    : "=r"(value), "=a"(ret)
	    : "c"(0), "d"(no_idea_what_this_is));

	if (ret)
		<cry>

	return value;

Or, if you wanted to get fancy, use asm_goto_output() to bail on an error so that you could optimize for using RAX as the return value, because *that's going to make all the difference :-D

I ended up with three helpers:

- tdvmcall_trampoline() as you proposed.

- tdvmcall_report_fatal_error() that does ReportFatalError specificly. Pointer to char array as an input. Never returns: no need to save/restore registers.

- hv_tdx_hypercall(). Hyper-V annoyingly uses R8 and RDX as parameters :/

I defined three helpers: TDCALL_0(), TDCALL_1() and TDCALL_5(). All takes 4 input arguments.

I've updated the WIP branch with the new version:

https://github.com/intel/tdx/commits/guest-tdx-asm/

Any feedback is welcome.

  Kiryl Shutsemau / Kirill A. Shutemov

Sorry, why do we need to panic?

It panics in cases that should never occur if the TDX module is functioning properly. For example, TDVMCALL itself should never fail, although the leaf function could.

  Kiryl Shutsemau / Kirill A. Shutemov

Panic should normally be for desperate situations when horrible things will likely happen if we continue, right? Why are we adding a panic when we didn't have one before? Is it a second change, or a side affect of the refactor?

The kernel already does panic() if TDCALL itself fails,

static inline void tdcall(u64 fn, struct tdx_module_args *args)

  {
	if (__tdcall_ret(fn, args))
		panic("TDCALL %lld failed (Buggy TDX module!)\n", fn);

}

/* Called from __tdx_hypercall() for unrecoverable failure */ noinstr void __noreturn __tdx_hypercall_failed(void)

  {
	instrumentation_begin();
	panic("TDVMCALL failed. TDX module bug?");

}

it's just doesn in C code via panic(), not in asm via a bare ud2.

Hmm, I didn't realize. It looks like today some calls do and some don't. I don't mean to reopen old debates. Just surprised that these are able to bring down the system. Which funnily enough connects back to the original issue of the patch:
whether they are safe to export for module use.

From: Isaku Yamahata <[email protected]>

This patch fixes the following warnings.

In file included from arch/x86/kernel/asm-offsets.c:22:
arch/x86/include/asm/tdx.h:92:87: warning: shift count >= width of type [-Wshift-count-overflow] arch/x86/include/asm/tdx.h:20:21: note: expanded from macro 'TDX_ERROR'
#define TDX_ERROR _BITUL(63)

                                           ^~~~~~~~~~

Also consistently use ULL for TDX_SEAMCALL_VMFAILINVALID.

---
 arch/x86/include/asm/tdx.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 16be3a1e4916..1e9dcdf9912b 100644
@@ -17,9 +17,9 @@
  * Bits 47:40 == 0xFF indicate Reserved status code class that never used by
  * TDX module.
  */
-#define TDX_ERROR			_BITUL(63)
+#define TDX_ERROR			_BITULL(63)
 #define TDX_SW_ERROR			(TDX_ERROR | GENMASK_ULL(47, 40))
-#define TDX_SEAMCALL_VMFAILINVALID	(TDX_SW_ERROR | _UL(0xFFFF0000))
+#define TDX_SEAMCALL_VMFAILINVALID	(TDX_SW_ERROR | _ULL(0xFFFF0000))
 
 #define TDX_SEAMCALL_GP			(TDX_SW_ERROR | X86_TRAP_GP)
 #define TDX_SEAMCALL_UD			(TDX_SW_ERROR | X86_TRAP_UD)

2.25.1

+Kirill.

This kinda fix should be sent out as a separate patch.

I think you trim the warning message. I don't see the actual user of the define. Define itself will not generate the warning. You need to actually use it outside of preprocessor. I don't understand who would use it in 32-bit code. Maybe fixing it this way masking other issue.

That said, I don't object the change itself. We just need to understand the context more.

  Kiryl Shutsemau / Kirill A. Shutemov

v18 used it as stub function. v19 dropped it as the stub was not needed.

Isaku Yamahata <[email protected]>

Sorry I literally don't understand what you are talking about here.

Please just clarify (at least):

- Does this problem exist in upstream code?
- If it does, what is the root cause, and how to reproduce?

No.

v18 had a problem because it has stub function. v19 doesn't have problem because it deleted the stub function.

Isaku Yamahata <[email protected]>

What is the "stub function"??

If "v19 doesn't have problem", why do you even _need_ this patch??

I am tired of guessing, but I don't care anymore given it's not a problem in upstream code.

From: Michael Roth <[email protected]>

In the case of SEV-SNP, whether or not a 2MB page can be mapped via a 2MB mapping in the guest's nested page table depends on whether or not any subpages within the range have already been initialized as private in the RMP table. The existing mixed-attribute tracking in KVM is insufficient here, for instance:

- gmem allocates 2MB page
- guest issues PVALIDATE on 2MB page
- guest later converts a subpage to shared
- SNP host code issues PSMASH to split 2MB RMP mapping to 4K
- KVM MMU splits NPT mapping to 4K

At this point there are no mixed attributes, and KVM would normally allow for 2MB NPT mappings again, but this is actually not allowed because the RMP table mappings are 4K and cannot be promoted on the hypervisor side, so the NPT mappings must still be limited to 4K to match this.

Add a hook to determine the max NPT mapping size in situations like this.

---
 arch/x86/include/asm/kvm-x86-ops.h |  1 +
 arch/x86/include/asm/kvm_host.h    |  2 ++
 arch/x86/kvm/mmu/mmu.c             | 12 ++++++++++--
 3 files changed, 13 insertions(+), 2 deletions(-)

index 378ed944b849..156832f01ebe 100644
@@ -138,6 +138,7 @@ KVM_X86_OP(complete_emulated_msr)
 KVM_X86_OP(vcpu_deliver_sipi_vector)
 KVM_X86_OP_OPTIONAL_RET0(vcpu_get_apicv_inhibit_reasons);
 KVM_X86_OP_OPTIONAL(get_untagged_addr)
+KVM_X86_OP_OPTIONAL_RET0(gmem_max_level)
 
 #undef KVM_X86_OP
 #undef KVM_X86_OP_OPTIONAL

index d271ba20a0b2..d15f5b4b1656 100644
@@ -1796,6 +1796,8 @@ struct kvm_x86_ops {
 	unsigned long (*vcpu_get_apicv_inhibit_reasons)(struct kvm_vcpu *vcpu);
 
 	gva_t (*get_untagged_addr)(struct kvm_vcpu *vcpu, gva_t gva, unsigned int flags);
+
+	int (*gmem_max_level)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, u8 *max_level);
 };
 
 struct kvm_x86_nested_ops {

index 2d6cdeab1f8a..1e5e12d2707d 100644
@@ -4308,6 +4308,7 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
 				   struct kvm_page_fault *fault)
 {
 	int max_order, r;
+	u8 max_level;
 
 	if (!kvm_slot_can_be_private(fault->slot)) {
 		kvm_mmu_prepare_memory_fault_exit(vcpu, fault);

@@ -4321,8 +4322,15 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
 		return r;
 	}
 
-	fault->max_level = min(kvm_max_level_for_order(max_order),
-			       fault->max_level);
+	max_level = kvm_max_level_for_order(max_order);
+	r = static_call(kvm_x86_gmem_max_level)(vcpu->kvm, fault->pfn,
+						fault->gfn, &max_level);
+	if (r) {
+		kvm_release_pfn_clean(fault->pfn);
+		return r;
+	}
+
+	fault->max_level = min(max_level, fault->max_level);
 	fault->map_writable = !(fault->slot->flags & KVM_MEM_READONLY);
 
 	return RET_PF_CONTINUE;

2.25.1

From: Isaku Yamahata <[email protected]>

TDX wants to know the faulting address is shared or private so that the max level is limited by Secure-EPT or not. Because fault->gfn doesn't include shared bit, gfn doesn't tell if the faulting address is shared or not. Pass is_private for TDX case.

TDX logic will be if (!is_private) return 0; else return PG_LEVEL_4K.

---
 arch/x86/include/asm/kvm_host.h | 3 ++-
 arch/x86/kvm/mmu/mmu.c          | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

index d15f5b4b1656..57ce89fc2740 100644
@@ -1797,7 +1797,8 @@ struct kvm_x86_ops {
 
 	gva_t (*get_untagged_addr)(struct kvm_vcpu *vcpu, gva_t gva, unsigned int flags);
 
-	int (*gmem_max_level)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, u8 *max_level);
+	int (*gmem_max_level)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn,
+			      bool is_private, u8 *max_level);
 };
 
 struct kvm_x86_nested_ops {

index 1e5e12d2707d..22db1a9f528a 100644
@@ -4324,7 +4324,8 @@ static int kvm_faultin_pfn_private(struct kvm_vcpu *vcpu,
 
 	max_level = kvm_max_level_for_order(max_order);
 	r = static_call(kvm_x86_gmem_max_level)(vcpu->kvm, fault->pfn,
-						fault->gfn, &max_level);
+						fault->gfn, fault->is_private,
+						&max_level);
 	if (r) {
 		kvm_release_pfn_clean(fault->pfn);
 		return r;

2.25.1

You're right, we don't need this patch.
As Paolo picked the patch to add a hook, the discussion is happening at
https://lore.kernel.org/all/[email protected]/#t

Isaku Yamahata <[email protected]>

From: Isaku Yamahata <[email protected]>

Add new members to strut kvm_gfn_range to indicate which mapping (private-vs-shared) to operate on. only_private and only_shared. Update mmu notifier, set memory attributes ioctl or KVM gmem callback to initialize them.

It was premature for set_memory_attributes ioctl to call kvm_unmap_gfn_range(). Instead, let kvm_arch_ste_memory_attributes() handle it and add a new x86 vendor callback to react to memory attribute change. [1]

- If it's from the mmu notifier, zap shared pages only
- If it's from the KVM gmem, zap private pages only
- If setting memory attributes, vendor callback checks new attributes and make decisions.
SNP would do nothing and handle it later with gmem callback TDX callback would do as follows.
When it converts pages to shared, zap private pages only. When it converts pages to private, zap shared pages only.

TDX needs to know which mapping to operate on. Shared-EPT vs. Secure-EPT. The following sequence to convert the GPA to private doesn't work for TDX because the page can already be private.

1) Update memory attributes to private in memory attributes xarray
2) Zap the GPA range irrespective of private-or-shared. Even if the page is already private, zap the entry.
3) EPT violation on the GPA
4) Populate the GPA as private
The page is zeroed, and the guest has to accept the page again.

In step 2, TDX wants to zap only shared pages and skip private ones.

[1] https://lore.kernel.org/all/[email protected]/

---
Changes v18:
- rebased to kvm-next

Changes v2 -> v3:
- Drop the KVM_GFN_RANGE flags
- Updated struct kvm_gfn_range
- Change kvm_arch_set_memory_attributes() to return bool for flush
- Added set_memory_attributes x86 op for vendor backends
- Refined commit message to describe TDX care concretely

Changes v1 -> v2:
- consolidate KVM_GFN_RANGE_FLAGS_GMEM_{PUNCH_HOLE, RELEASE} into KVM_GFN_RANGE_FLAGS_GMEM.
- Update the commit message to describe TDX more. Drop SEV_SNP.

---
 include/linux/kvm_host.h |  2 ++
 virt/kvm/guest_memfd.c   |  3 +++
 virt/kvm/kvm_main.c      | 17 +++++++++++++++++
 3 files changed, 22 insertions(+)

index 7e7fd25b09b3..0520cd8d03cc 100644
@@ -264,6 +264,8 @@ struct kvm_gfn_range {
 	gfn_t start;
 	gfn_t end;
 	union kvm_mmu_notifier_arg arg;
+	bool only_private;
+	bool only_shared;
 	bool may_block;
 };
 bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range);

index 0f4e0cf4f158..3830d50b9b67 100644
@@ -64,6 +64,9 @@ static void kvm_gmem_invalidate_begin(struct kvm_gmem *gmem, pgoff_t start,
 			.end = slot->base_gfn + min(pgoff + slot->npages, end) - pgoff,
 			.slot = slot,
 			.may_block = true,
+			/* guest memfd is relevant to only private mappings. */
+			.only_private = true,
+			.only_shared = false,
 		};
 
 		if (!found_memslot) {

index 10bfc88a69f7..0349e1f241d1 100644
@@ -634,6 +634,12 @@ static __always_inline kvm_mn_ret_t __kvm_handle_hva_range(struct kvm *kvm,
 			 */
 			gfn_range.arg = range->arg;
 			gfn_range.may_block = range->may_block;
+			/*
+			 * HVA-based notifications aren't relevant to private
+			 * mappings as they don't have a userspace mapping.
+			 */
+			gfn_range.only_private = false;
+			gfn_range.only_shared = true;
 
 			/*
 			 * {gfn(page) | page intersects with [hva_start, hva_end)} =

@@ -2486,6 +2492,16 @@ static __always_inline void kvm_handle_gfn_range(struct kvm *kvm,
 	gfn_range.arg = range->arg;
 	gfn_range.may_block = range->may_block;
 
+	/*
+	 * If/when KVM supports more attributes beyond private .vs shared, this
+	 * _could_ set only_{private,shared} appropriately if the entire target
+	 * range already has the desired private vs. shared state (it's unclear
+	 * if that is a net win).  For now, KVM reaches this point if and only
+	 * if the private flag is being toggled, i.e. all mappings are in play.
+	 */
+	gfn_range.only_private = false;
+	gfn_range.only_shared = false;
+
 	for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
 		slots = __kvm_memslots(kvm, i);
 

@@ -2542,6 +2558,7 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
 	struct kvm_mmu_notifier_range pre_set_range = {
 		.start = start,
 		.end = end,
+		.arg.attributes = attributes,
 		.handler = kvm_pre_set_memory_attributes,
 		.on_lock = kvm_mmu_invalidate_begin,
 		.flush_on_ret = true,

2.25.1

"kvm_arch_ste_memory_attributes()" -> "kvm_vm_set_mem_attributes()" ?

Which new x86 vendor callback?

Yes, will fix it.

Now we don't have it. Will drop this sentnse.

The code will be ugly like
"if (== PRIVATE || == PRIVATE_AND_SHARED)" or
"if (== SHARED || == PRIVATE_AND_SHARED)"

two boolean (or two flags) is less error-prone.

Thanks,

Isaku Yamahata <[email protected]>

Yes the enum would be awkward to handle. But I also thought the way this is specified in struct kvm_gfn_range is a little strange.

It is ambiguous what it should mean if you set:
.only_private=true;
.only_shared=true;
...as happens later in the series (although it may be a mistake).

Reading the original conversation, it seems Sean suggested this specifically. But it wasn't clear to me from the discussion what the intention of the "only" semantics was. Like why not?
bool private;
bool shared;

I see Binbin brought up this point on v18 as well:
https://lore.kernel.org/kvm/[email protected]/#t

and helpfully dug up some other discussion with Sean where he agreed the "_only" is confusing and proposed the the enum:
https://lore.kernel.org/kvm/[email protected]/

He wanted the default value (in the case the caller forgets to set them), to be to include both private and shared. I think the enum has the issues that Isaku mentioned. What about?

bool exclude_private;
bool exclude_shared;

It will become onerous if more types of aliases grow, but it clearer semantically and has the safe default behavior.

I'm fine with those names. Anyway, I'm fine with wither way, two bools or enum.

Isaku Yamahata <[email protected]>

Hi,

I don't have a strong opinion, but I'd brought it up in a previous patch series. I think that having two bools to encode three states is less intuitive and potentially more bug prone, more so than the naming itself (i.e., _only):
https://lore.kernel.org/all/[email protected]/

Cheers,
/fuad

Currently in our internal branch we switched to:
exclude_private
exclude_shared

It came together bettter in the code that uses it.

But I started to wonder if we actually really need exclude_shared. For TDX zapping private memory has to be done with more care, because it cannot be re- populated without guest coordination. But for shared memory if we are zapping a range that includes both private and shared memory, I don't think it should hurt to zap the shared memory.

Hmm, yeah, I buy that argument. We could even harded further by poisoning '0' to force KVM to explicitly. Aha! And maybe use a bitmap?

	enum {
		BUGGY_KVM_INVALIDATION		= 0,
		PROCESS_SHARED			= BIT(0),
		PROCESS_PRIVATE			= BIT(1),
		PROCESS_PRIVATE_AND_SHARED	= PROCESS_SHARED | PROCESS_PRIVATE,
	};

If the choice is between an enum and exclude_*, I would strongly prefer the enum. Using exclude_* results in inverted polarity for the code that triggers invalidations.

Hell no, I am not risking taking on more baggage in KVM where userspace or some other subsystem comes to rely on KVM spuriously zapping SPTEs in response to an unrelated userspace action.

Seems like it would work for all who have been concerned. The previous objection to the enum (can't find the mail) was for requiring logic like:

   if (zap == PROCESS_PRIVATE_AND_SHARED || zap == PROCESS_PRIVATE)
   	do_private_zap_stuff();

We are trying to tie things up internally so we can jointly have something to stare at again, as the patches are diverging. But will make this adjustment.

Right, the awkwardness lands in that code.

The processing code looks nice though:
https://lore.kernel.org/kvm/[email protected]/

Hmm, I see the point. Thanks. This was just being left for later discussion anyway.

Heh, where's your bitmask abuse spirit? It's a little evil (and by "evil" I mean awesome), but the need to process different roots is another good argument for an enum+bitmask.

enum tdp_mmu_root_types {
KVM_SHARED_ROOTS = KVM_PROCESS_SHARED,
KVM_PRIVATE_ROOTS = KVM_PROCESS_PRIVATE,
KVM_VALID_ROOTS = BIT(2),
KVM_ANY_VALID_ROOT = KVM_SHARED_ROOT | KVM_PRIVATE_ROOT | KVM_VALID_ROOT, KVM_ANY_ROOT = KVM_SHARED_ROOT | KVM_PRIVATE_ROOT,
}
static_assert(!(KVM_SHARED_ROOTS & KVM_VALID_ROOTS)); static_assert(!(KVM_PRIVATE_ROOTS & KVM_VALID_ROOTS)); static_assert(KVM_PRIVATE_ROOTS == (KVM_SHARED_ROOTS << 1));

/*
* Returns the next root after @prev_root (or the first root if @prev_root is
* NULL). A reference to the returned root is acquired, and the reference to
* @prev_root is released (the caller obviously must hold a reference to
* @prev_root if it's non-NULL).
*
* Returns NULL if the end of tdp_mmu_roots was reached.
*/
static struct kvm_mmu_page *tdp_mmu_next_root(struct kvm *kvm,
struct kvm_mmu_page *prev_root, enum tdp_mmu_root_types types)

{
	bool only_valid = types & KVM_VALID_ROOTS;
	struct kvm_mmu_page *next_root;

	/*
	 * While the roots themselves are RCU-protected, fields such as
	 * role.invalid are protected by mmu_lock.
	 */
	lockdep_assert_held(&kvm->mmu_lock);

	rcu_read_lock();

	if (prev_root)
		next_root = list_next_or_null_rcu(&kvm->arch.tdp_mmu_roots,
						  &prev_root->link,
						  typeof(*prev_root), link);
	else
		next_root = list_first_or_null_rcu(&kvm->arch.tdp_mmu_roots,
						   typeof(*next_root), link);

	while (next_root) {
		if ((!only_valid || !next_root->role.invalid) &&
		    (types & (KVM_SHARED_ROOTS << is_private_sp(root))) &&
		    kvm_tdp_mmu_get_root(next_root))
			break;

		next_root = list_next_or_null_rcu(&kvm->arch.tdp_mmu_roots,
				&next_root->link, typeof(*next_root), link);
	}

	rcu_read_unlock();

	if (prev_root)
		kvm_tdp_mmu_put_root(kvm, prev_root);

	return next_root;
}

#define __for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, _types)		\
	for (_root = tdp_mmu_next_root(_kvm, NULL, _types);			\
	     ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;		\
	     _root = tdp_mmu_next_root(_kvm, _root, _types))			\
		if (_as_id >= 0 && kvm_mmu_page_as_id(_root) != _as_id) {	\
		} else

#define for_each_valid_tdp_mmu_root_yield_safe(_kvm, _root, _as_id)		\
	__for_each_tdp_mmu_root_yield_safe(_kvm, _root, _as_id, KVM_ANY_VALID_ROOT)

#define for_each_tdp_mmu_root_yield_safe(_kvm, _root)				\
	for (_root = tdp_mmu_next_root(_kvm, NULL, KVM_ANY_ROOT);		\
	     ({ lockdep_assert_held(&(_kvm)->mmu_lock); }), _root;		\
	     _root = tdp_mmu_next_root(_kvm, _root, KVM_ANY_ROOT))

Haha. There seems to be some special love for bit math in KVM. I was just relaying my struggle to understand permission_fault() the other day.

Ohh, yes move it into the iterators. I like it a lot.

LOL, you and everyone else that's ever looked at that code. Just wait until you run into one of Paolo's decimal-based bitmasks.

From: Isaku Yamahata <[email protected]>

Because the full 64-bit error code, or more info about the fault, for the KVM page fault will be needed for protected VM, TDX and SEV-SNP, update kvm_mmu_do_page_fault() to accept the 64-bit value so it can pass it to the callbacks.

The upper 32 bits of error code are discarded at kvm_mmu_page_fault() by lower_32_bits(). Now it's passed down as full 64 bits. Currently two hardware defined bits, PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK, and one software defined bit, PFERR_IMPLICIT_ACCESS, is defined.

PFERR_IMPLICIT_ACCESS:
commit 4f4aa80e3b88 ("KVM: X86: Handle implicit supervisor access with SMAP") introduced a software defined bit PFERR_IMPLICIT_ACCESS at bit 48 to indicate implicit access for SMAP with instruction emulator. Concretely emulator_read_std() and emulator_write_std() set the bit. permission_fault() checks the bit as smap implicit access. The vendor page fault handler shouldn't pass the bit to kvm_mmu_page_fault().

PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK:
commit 147277540bbc ("kvm: svm: Add support for additional SVM NPF error codes") introduced them to optimize the nested page fault handling. Other code path doesn't use the bits. Those two bits can be safely passed down without functionality change.

The accesses of fault->error_code are as follows
- FNAME(page_fault): PFERR_IMPLICIT_ACCESS shouldn't be passed down. PFERR_GUEST_FINAL_MASK and PFERR_GUEST_PAGE_MASK aren't used.
- kvm_mmu_page_fault(): explicit mask with PFERR_RSVD_MASK, and PFERR_NESTED_GUEST_PAGE is used outside of the masking upper 32 bits.
- mmutrace: change u32 -> u64

No functional change is intended. This is a preparation to pass on more info with page fault error code.

---
Changes v3 -> v4:
- Dropped debug print part as it was deleted in the kvm-x86-next

Changes v2 -> v3:
- Make depends on a patch to clear PFERR_IMPLICIT_ACCESS
- drop clearing the upper 32 bit, instead just pass whole 64 bits
- update commit message to mention about PFERR_IMPLICIT_ACCESS and PFERR_NESTED_GUEST_PAGE

Changes v1 -> v2:
- no change

---
 arch/x86/kvm/mmu/mmu.c          | 3 +--
 arch/x86/kvm/mmu/mmu_internal.h | 4 ++--
 arch/x86/kvm/mmu/mmutrace.h     | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

index 22db1a9f528a..ccdbff3d85ec 100644
@@ -5822,8 +5822,7 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	}
 
 	if (r == RET_PF_INVALID) {
-		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa,
-					  lower_32_bits(error_code), false,
+		r = kvm_mmu_do_page_fault(vcpu, cr2_or_gpa, error_code, false,
 					  &emulation_type);
 		if (KVM_BUG_ON(r == RET_PF_INVALID, vcpu->kvm))
 			return -EIO;

index 0669a8a668ca..21f55e8b4dc6 100644
@@ -190,7 +190,7 @@ static inline bool is_nx_huge_page_enabled(struct kvm *kvm)
 struct kvm_page_fault {
 	/* arguments to kvm_mmu_do_page_fault.  */
 	const gpa_t addr;
-	const u32 error_code;
+	const u64 error_code;
 	const bool prefetch;
 
 	/* Derived from error_code.  */

@@ -280,7 +280,7 @@ enum {
 };
 
 static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
-					u32 err, bool prefetch, int *emulation_type)
+					u64 err, bool prefetch, int *emulation_type)
 {
 	struct kvm_page_fault fault = {
 		.addr = cr2_or_gpa,

index ae86820cef69..195d98bc8de8 100644
@@ -260,7 +260,7 @@ TRACE_EVENT(
 	TP_STRUCT__entry(
 		__field(int, vcpu_id)
 		__field(gpa_t, cr2_or_gpa)
-		__field(u32, error_code)
+		__field(u64, error_code)
 		__field(u64 *, sptep)
 		__field(u64, old_spte)
 		__field(u64, new_spte)

2.25.1

From: Isaku Yamahata <[email protected]>

SEV-SNP defines PFERR_GUEST_ENC_MASK (bit 32) in page-fault error bits to represent the guest page is encrypted. Use the bit to designate that the page fault is private and that it requires looking up memory attributes. The vendor kvm page fault handler should set PFERR_GUEST_ENC_MASK bit based on their fault information. It may or may not use the hardware value directly or parse the hardware value to set the bit.

For KVM_X86_SW_PROTECTED_VM, ask memory attributes for the fault privateness. For async page fault, carry the bit and use it for kvm page fault handler.

---
Changes v4 -> v5:
- Eliminate kvm_is_fault_private() by open code the function
- Make async page fault handler to carry is_private bit

Changes v3 -> v4:
- rename back struct kvm_page_fault::private => is_private
- catch up rename: KVM_X86_PROTECTED_VM => KVM_X86_SW_PROTECTED_VM

Changes v2 -> v3:
- Revive PFERR_GUEST_ENC_MASK
- rename struct kvm_page_fault::is_private => private
- Add check KVM_X86_PROTECTED_VM

Changes v1 -> v2:
- Introduced fault type and replaced is_private with fault_type.
- Add kvm_get_fault_type() to encapsulate the difference.

---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/mmu/mmu.c          | 24 +++++++++++++++++-------
 arch/x86/kvm/mmu/mmu_internal.h |  2 +-
 3 files changed, 21 insertions(+), 8 deletions(-)

index 57ce89fc2740..28314e7d546c 100644
@@ -264,6 +264,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_BIT 15
 #define PFERR_GUEST_FINAL_BIT 32
 #define PFERR_GUEST_PAGE_BIT 33
+#define PFERR_GUEST_ENC_BIT 34
 #define PFERR_IMPLICIT_ACCESS_BIT 48
 
 #define PFERR_PRESENT_MASK	BIT(PFERR_PRESENT_BIT)

@@ -275,6 +276,7 @@ enum x86_intercept_stage;
 #define PFERR_SGX_MASK		BIT(PFERR_SGX_BIT)
 #define PFERR_GUEST_FINAL_MASK	BIT_ULL(PFERR_GUEST_FINAL_BIT)
 #define PFERR_GUEST_PAGE_MASK	BIT_ULL(PFERR_GUEST_PAGE_BIT)
+#define PFERR_GUEST_ENC_MASK	BIT_ULL(PFERR_GUEST_ENC_BIT)
 #define PFERR_IMPLICIT_ACCESS	BIT_ULL(PFERR_IMPLICIT_ACCESS_BIT)
 
 #define PFERR_NESTED_GUEST_PAGE (PFERR_GUEST_PAGE_MASK |	\

@@ -1836,6 +1838,7 @@ struct kvm_arch_async_pf {
 	gfn_t gfn;
 	unsigned long cr3;
 	bool direct_map;
+	u64 error_code;
 };
 
 extern u32 __read_mostly kvm_nr_uret_msrs;

index ccdbff3d85ec..61674d6b17aa 100644
@@ -4246,18 +4246,19 @@ static u32 alloc_apf_token(struct kvm_vcpu *vcpu)
 	return (vcpu->arch.apf.id++ << 12) | vcpu->vcpu_id;
 }
 
-static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
-				    gfn_t gfn)
+static bool kvm_arch_setup_async_pf(struct kvm_vcpu *vcpu,
+				    struct kvm_page_fault *fault)
 {
 	struct kvm_arch_async_pf arch;
 
 	arch.token = alloc_apf_token(vcpu);
-	arch.gfn = gfn;
+	arch.gfn = fault->gfn;
 	arch.direct_map = vcpu->arch.mmu->root_role.direct;
 	arch.cr3 = kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu);
+	arch.error_code = fault->error_code & PFERR_GUEST_ENC_MASK;
 
-	return kvm_setup_async_pf(vcpu, cr2_or_gpa,
-				  kvm_vcpu_gfn_to_hva(vcpu, gfn), &arch);
+	return kvm_setup_async_pf(vcpu, fault->addr,
+				  kvm_vcpu_gfn_to_hva(vcpu, fault->gfn), &arch);
 }
 
 void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)

@@ -4276,7 +4277,8 @@ void kvm_arch_async_page_ready(struct kvm_vcpu *vcpu, struct kvm_async_pf *work)
 	      work->arch.cr3 != kvm_mmu_get_guest_pgd(vcpu, vcpu->arch.mmu))
 		return;
 
-	kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, 0, true, NULL);
+	kvm_mmu_do_page_fault(vcpu, work->cr2_or_gpa, work->arch.error_code,
+			      true, NULL);
 }
 
 static inline u8 kvm_max_level_for_order(int order)

@@ -4390,7 +4392,7 @@ static int __kvm_faultin_pfn(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault
 			trace_kvm_async_pf_repeated_fault(fault->addr, fault->gfn);
 			kvm_make_request(KVM_REQ_APF_HALT, vcpu);
 			return RET_PF_RETRY;
-		} else if (kvm_arch_setup_async_pf(vcpu, fault->addr, fault->gfn)) {
+		} else if (kvm_arch_setup_async_pf(vcpu, fault)) {
 			return RET_PF_RETRY;
 		}
 	}

@@ -5814,6 +5816,14 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
 		return RET_PF_RETRY;
 
+	/*
+	 * This is racy with updating memory attributes with mmu_seq.  If we
+	 * hit a race, it would result in retrying page fault.
+	 */
+	if (vcpu->kvm->arch.vm_type == KVM_X86_SW_PROTECTED_VM &&
+	    kvm_mem_is_private(vcpu->kvm, gpa_to_gfn(cr2_or_gpa)))
+		error_code |= PFERR_GUEST_ENC_MASK;
+
 	r = RET_PF_INVALID;
 	if (unlikely(error_code & PFERR_RSVD_MASK)) {
 		r = handle_mmio_page_fault(vcpu, cr2_or_gpa, direct);

index 21f55e8b4dc6..0443bfcf5d9c 100644
@@ -292,13 +292,13 @@ static inline int kvm_mmu_do_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa,
 		.user = err & PFERR_USER_MASK,
 		.prefetch = prefetch,
 		.is_tdp = likely(vcpu->arch.mmu->page_fault == kvm_tdp_page_fault),
+		.is_private = err & PFERR_GUEST_ENC_MASK,
 		.nx_huge_page_workaround_enabled =
 			is_nx_huge_page_enabled(vcpu->kvm),
 
 		.max_level = KVM_MAX_HUGEPAGE_LEVEL,
 		.req_level = PG_LEVEL_4K,
 		.goal_level = PG_LEVEL_4K,
-		.is_private = kvm_mem_is_private(vcpu->kvm, cr2_or_gpa >> PAGE_SHIFT),
 	};
 	int r;