* Re: SVSM Development Call April 8th, 2026
From: Jörg Rödel @ 2026-04-13 11:17 UTC (permalink / raw)
To: Stefano Garzarella; +Cc: coconut-svsm, linux-coco
In-Reply-To: <CAGxU2F6OApB3K61_sPujnvK_gx_K8zFWyOSTPV9mzWOCyNkBJg@mail.gmail.com>
Meeting Minutes are here, please everyone check for accuracy and completeness:
https://github.com/coconut-svsm/governance/pull/103
-Joerg
^ permalink raw reply
* Re: [PATCH v2 2/6] KVM: x86: Drop the "EX" part of "EXREG" to avoid collision with APX
From: Huang, Kai @ 2026-04-13 11:23 UTC (permalink / raw)
To: pbonzini@redhat.com, kas@kernel.org, seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Bae, Chang Seok,
linux-kernel@vger.kernel.org, x86@kernel.org
In-Reply-To: <20260409224236.2021562-3-seanjc@google.com>
On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> Now that NR_VCPU_REGS is no longer a thing, and now that now that RIP is
Nit: double "now that".
^ permalink raw reply
* Re: [PATCH v2] KVM: TDX: Fix x2APIC MSR handling in tdx_has_emulated_msr()
From: Binbin Wu @ 2026-04-13 2:13 UTC (permalink / raw)
To: Rick Edgecombe
Cc: kas, kvm, linux-coco, linux-kernel, pbonzini, seanjc, dmaluka
In-Reply-To: <20260410232654.3864196-1-rick.p.edgecombe@intel.com>
On 4/11/2026 7:26 AM, Rick Edgecombe wrote:
> Rework tdx_has_emulated_msr() to explicitly enumerate the x2APIC MSRs
> that KVM can emulate, instead of trying to enumerate the MSRs that KVM
> cannot emulate. Drop the inner switch and list the emulatable x2APIC
> registers directly in the outer switch's "return true" block.
>
> The old code had multiple bugs in the x2APIC range handling.
> X2APIC_MSR(APIC_ISR + APIC_ISR_NR) was incorrect because APIC_ISR_NR is
> 0x8, not 0x80, so the X2APIC_MSR() shift lost the lower bits, collapsing
> each range to a single MSR. IA32_X2APIC_SELF_IPI was also missing from
> the non-emulatable list.
Is it better to describe that the bug is benign for a sane guest?
>
> KVM has no visibility into whether or not a guest has enabled #VE
> reduction, which changes which MSRs the TDX-Module handles itself versus
> triggering a #VE for the guest to make a TDVMCALL. So maintaining a list
> of non-emulatable MSRs is fragile. Listing only the MSRs KVM can always
> emulate sidesteps the problem.
>
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Reported-by: Dmytro Maluka <dmaluka@chromium.org>
> Fixes: dd50294f3e3c ("KVM: TDX: Implement callbacks for MSR operations")
> Assisted-by: Claude:claude-opus-4-6
> [based on a diff from Sean, but added missed LVTCMCI case, log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
One nit below.
> ---
>
> Thanks to Dmytro for finding this. They said to feel free to take this
> over, so here is another version with Sean's suggestions. Tested in the
> TDX CI.
>
> In Sean's suggestion LVTCMCI was missed, so it's added here.
>
> arch/x86/kvm/vmx/tdx.c | 36 ++++++++++++++++++++----------------
> 1 file changed, 20 insertions(+), 16 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 1e47c194af53..76ab6805ab29 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2116,23 +2116,27 @@ bool tdx_has_emulated_msr(u32 index)
> case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
> /* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
> case MSR_KVM_POLL_CONTROL:
> + /*
> + * x2APIC registers that are virtualized by the CPU can't be
> + * emulated, KVM doesn't have access to the virtual APIC page.
> + */
Nit:
The original comment explains why certain MSRs are not emulated due to
its implementation. After the change, it lists the allowed emulated MSRs,
a quick read might give the false impression that the listed MSRs are the
ones that cannot be emulated. Maybe slightly tweak the comment to clarify
that the cases listed below are the MSRs that KVM is responsible for
emulating.
> + case X2APIC_MSR(APIC_ID):
> + case X2APIC_MSR(APIC_LVR):
> + case X2APIC_MSR(APIC_LDR):
> + case X2APIC_MSR(APIC_SPIV):
> + case X2APIC_MSR(APIC_ESR):
> + case X2APIC_MSR(APIC_LVTCMCI):
> + case X2APIC_MSR(APIC_ICR):
> + case X2APIC_MSR(APIC_LVTT):
> + case X2APIC_MSR(APIC_LVTTHMR):
> + case X2APIC_MSR(APIC_LVTPC):
> + case X2APIC_MSR(APIC_LVT0):
> + case X2APIC_MSR(APIC_LVT1):
> + case X2APIC_MSR(APIC_LVTERR):
> + case X2APIC_MSR(APIC_TMICT):
> + case X2APIC_MSR(APIC_TMCCT):
> + case X2APIC_MSR(APIC_TDCR):
> return true;
> - case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
> - /*
> - * x2APIC registers that are virtualized by the CPU can't be
> - * emulated, KVM doesn't have access to the virtual APIC page.
> - */
> - switch (index) {
> - case X2APIC_MSR(APIC_TASKPRI):
> - case X2APIC_MSR(APIC_PROCPRI):
> - case X2APIC_MSR(APIC_EOI):
> - case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
> - case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
> - case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
> - return false;
> - default:
> - return true;
> - }
> default:
> return false;
> }
^ permalink raw reply
* Re: [PATCH v2 03/31] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects
From: Dan Williams @ 2026-04-12 2:53 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, yilun.xu,
zhenzhong.duan, kvm, rick.p.edgecombe, dave.hansen, kas,
xiaoyao.li, vishal.l.verma, linux-kernel
In-Reply-To: <20260327160132.2946114-4-yilun.xu@linux.intel.com>
Xu Yilun wrote:
> Add struct tdx_page_array definition for new TDX Module object
> types - HPA_ARRAY_T and HPA_LIST_INFO. They are used as input/output
> parameters in newly defined SEAMCALLs. Also define some helpers to
> allocate, setup and free tdx_page_array.
>
> HPA_ARRAY_T and HPA_LIST_INFO are similar in most aspects. They both
> represent a list of pages for TDX Module accessing. There are several
> use cases for these 2 structures:
>
> - As SEAMCALL inputs. They are claimed by TDX Module as control pages.
> Control pages are private pages for TDX Module to hold its internal
> control structures or private data. TDR, TDCS, TDVPR... are existing
> control pages, just not added via tdx_page_array.
> - As SEAMCALL outputs. They were TDX Module control pages and now are
> released.
> - As SEAMCALL inputs. They are just temporary buffers for exchanging
> data blobs in one SEAMCALL. TDX Module will not hold them for long
> time.
>
> The 2 structures both need a 'root page' which contains a list of HPAs.
> They collapse the HPA of the root page and the number of valid HPAs
> into a 64 bit raw value for SEAMCALL parameters. The root page is
> always a medium for passing data pages, TDX Module never keeps the
> root page.
>
> A main difference is HPA_ARRAY_T requires singleton mode when
> containing just 1 functional page (page0). In this mode the root page is
> not needed and the HPA field of the raw value directly points to the
> page0. But in this patch, root page is always allocated for user
> friendly kAPIs.
I think this undersells the fact that "singleton mode" is a premature
optimization that requires complication to take advantage of the benefit
(sometimes save a single page allocation). The Linux implementation
forfeits that small benefit for the larger gain of cleaner kAPIs.
> Another small difference is HPA_LIST_INFO contains a 'first entry' field
> which could be filled by TDX Module. This simplifies host by providing
> the same structure when re-invoke the interrupted SEAMCALL. No need for
> host to touch this field.
>
> Typical usages of the tdx_page_array:
>
> 1. Add control pages:
> - struct tdx_page_array *array = tdx_page_array_create(nr_pages);
> - seamcall(TDH_XXX_CREATE, array, ...);
>
> 2. Release control pages:
> - seamcall(TDX_XXX_DELETE, array, &nr_released, &released_hpa);
It is simply a bug if TDH_XXX_DELETE does not return every resource
passed to TDH_XXX_CREATE. The only "leak" case to worry about is that
TDH_XXX_DELETE fails. In that case it should be "fatal" (TDX_BUG_ON,
system can keep hobbling along, but panic_on_warn() would not be
unreasonable). If TDH_XXX_DELETE fails it indicates some catastrophic
misunderstanding between Linux and the TDX Module.
So the seamcall in this case has no need for @nr_released or
@released_hpa, those should already be known to the kernel.
What is missing is an architectural guarantee that TDH_XXX_DELETE
success == "all resources you arranged at TDH_XXX_CREATE time are free".
I would hope that is already the case and AUX_PAGE_PA is only an
unfortunate distraction. If it can ever be the case that CREATE and
DELETE are asymmetric on success then that needs to be corrected and
Linux will wait for a future module that can make that guarantee.
I think that cleans up a bulk of the logic here to abandon caring that
the module tries to remind us what we are releasing.
^ permalink raw reply
* [PATCH] ACPI: block AML access to confidential VM private memory
From: bfoing @ 2026-04-11 22:44 UTC (permalink / raw)
To: linux-acpi
Cc: rafael, lenb, robert.moore, kirill.shutemov, thomas.lendacky,
linux-coco, linux-kernel, Bertrand Foing
From: Bertrand Foing <40759640+bfoing@users.noreply.github.com>
Add a guard in the ACPICA SystemMemory space handler that prevents AML
bytecode from reading or writing pages belonging to the confidential VM
private address range.
On TDX and SEV-SNP guests the ACPI tables are under host/VMM control.
Malicious AML ("BadAML") can issue SystemMemory region reads and writes
to arbitrary guest physical addresses, extracting secrets or corrupting
guest state without triggering any existing kernel protection.
The guard walks the kernel page tables for the target virtual address
and checks whether the page-table entry carries the platform-specific
is private the access is denied with AE_AML_ILLEGAL_ADDRESS.
Signed-off-by: Bertrand Foing <40759640+bfoing@users.noreply.github.com>
---
drivers/acpi/Makefile | 1 +
drivers/acpi/acpica/exregion.c | 12 ++++
drivers/acpi/cvm_guard.c | 121 +++++++++++++++++++++++++++++++++
3 files changed, 134 insertions(+)
create mode 100644 drivers/acpi/cvm_guard.c
diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index d1b0affb8..6743ece85 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -45,6 +45,7 @@ acpi-y += resource.o
acpi-y += acpi_processor.o
acpi-y += processor_core.o
acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
+acpi-$(CONFIG_ARCH_HAS_CC_PLATFORM) += cvm_guard.o
acpi-$(CONFIG_ACPI_EC) += ec.o
acpi-$(CONFIG_ACPI_DOCK) += dock.o
acpi-$(CONFIG_PCI) += pci_root.o pci_link.o pci_irq.o
diff --git a/drivers/acpi/acpica/exregion.c b/drivers/acpi/acpica/exregion.c
index a390a1c2b..f12cacff3 100644
--- a/drivers/acpi/acpica/exregion.c
+++ b/drivers/acpi/acpica/exregion.c
@@ -14,6 +14,12 @@
#define _COMPONENT ACPI_EXECUTER
ACPI_MODULE_NAME("exregion")
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+bool acpi_cvm_guard_deny_access(unsigned long virt_addr);
+#else
+static inline bool acpi_cvm_guard_deny_access(unsigned long v) { return false; }
+#endif
+
/*******************************************************************************
*
* FUNCTION: acpi_ex_system_memory_space_handler
@@ -176,6 +182,12 @@ acpi_ex_system_memory_space_handler(u32 function,
logical_addr_ptr = mm->logical_address +
((u64) address - (u64) mm->physical_address);
+#ifdef CONFIG_ARCH_HAS_CC_PLATFORM
+ if (acpi_cvm_guard_deny_access((unsigned long)logical_addr_ptr)) {
+ return_ACPI_STATUS(AE_AML_ILLEGAL_ADDRESS);
+ }
+#endif
+
ACPI_DEBUG_PRINT((ACPI_DB_INFO,
"System-Memory (width %u) R/W %u Address=%8.8X%8.8X\n",
bit_width, function, ACPI_FORMAT_UINT64(address)));
diff --git a/drivers/acpi/cvm_guard.c b/drivers/acpi/cvm_guard.c
new file mode 100644
index 000000000..0524bf902
--- /dev/null
+++ b/drivers/acpi/cvm_guard.c
@@ -0,0 +1,121 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * CVM Guard - Block AML access to confidential VM private memory
+ *
+ * Copyright (C) 2026 Privasys
+ *
+ * On TDX and SEV-SNP guests the host VMM controls ACPI tables, so
+ * AML bytecode executing SystemMemory reads and writes can target
+ * arbitrary guest physical addresses. This file provides a guard
+ * function called from the ACPICA SystemMemory space handler that
+ * checks whether the target virtual address maps to a page marked
+ * as encrypted (private) in the page tables, and denies the access
+ * if so.
+ *
+ * Reference: "BadAML: Exploiting AML in Confidential Virtual Machines"
+ * Takekoshi et al., ACM CCS 2025
+ */
+
+#include <linux/cc_platform.h>
+#include <linux/mm.h>
+#include <linux/printk.h>
+#include <asm/coco.h>
+
+/* Prototype to satisfy -Wmissing-prototypes; declared here rather than in
+ * internal.h because this file does not need the full ACPI driver headers.
+ */
+bool acpi_cvm_guard_deny_access(unsigned long virt_addr);
+
+/*
+ * Walk the four-level kernel page tables for @addr and return the raw
+ * PTE/PMD/PUD value. Returns 0 if the walk fails at any level.
+ * Handles 1 GB (PUD) and 2 MB (PMD) large pages.
+ */
+static unsigned long cvm_guard_pte_val(unsigned long addr)
+{
+ pgd_t *pgd;
+ p4d_t *p4d;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte;
+
+ pgd = pgd_offset_k(addr);
+ if (pgd_none(*pgd))
+ return 0;
+
+ p4d = p4d_offset(pgd, addr);
+ if (p4d_none(*p4d))
+ return 0;
+
+ pud = pud_offset(p4d, addr);
+ if (pud_none(*pud))
+ return 0;
+ if (pud_leaf(*pud))
+ return pud_val(*pud);
+
+ pmd = pmd_offset(pud, addr);
+ if (pmd_none(*pmd))
+ return 0;
+ if (pmd_leaf(*pmd))
+ return pmd_val(*pmd);
+
+ pte = pte_offset_kernel(pmd, addr);
+ if (pte_none(*pte))
+ return 0;
+
+ return pte_val(*pte);
+}
+
+/*
+ * Check whether @addr maps to a private (encrypted) page.
+ *
+ * cc_mkenc() applies the platform-specific encryption mask:
+ * AMD SEV/SEV-SNP: sets the C-bit
+ * Intel TDX: clears the shared bit
+ *
+ * If the PTE already matches its encrypted form, the page is private
+ * and must not be accessible to AML. If the walk fails (returns 0)
+ * we deny access - fail-closed is the safe default.
+ */
+static bool cvm_guard_page_is_private(unsigned long addr)
+{
+ unsigned long val;
+
+ val = cvm_guard_pte_val(addr);
+ if (!val) {
+ pr_warn_ratelimited("CVM guard: page table walk failed for %lx\n",
+ addr);
+ return true;
+ }
+
+ return val == cc_mkenc(val);
+}
+
+/**
+ * acpi_cvm_guard_deny_access - block AML access to CVM private pages
+ * @virt_addr: kernel virtual address resolved by the SystemMemory handler
+ *
+ * Called from acpi_ex_system_memory_space_handler() after the virtual
+ * address has been computed but before any read or write.
+ *
+ * On non-CVM systems (CC_ATTR_MEM_ENCRYPT not set) this returns false.
+ *
+ * Return: true if the access must be denied, false if allowed.
+ */
+bool acpi_cvm_guard_deny_access(unsigned long virt_addr)
+{
+ if (!cc_platform_has(CC_ATTR_MEM_ENCRYPT))
+ return false;
+
+ pr_info_once("CVM guard: active, AML access to private pages will be denied\n");
+
+ virt_addr &= PAGE_MASK;
+
+ if (cvm_guard_page_is_private(virt_addr)) {
+ pr_warn_ratelimited("CVM guard: denied AML access to private page at %lx\n",
+ virt_addr);
+ return true;
+ }
+
+ return false;
+}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7 15/22] x86/virt/tdx: Restore TDX module state
From: Edgecombe, Rick P @ 2026-04-11 2:06 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <adTzamrJOOy+fGyF@intel.com>
On Tue, 2026-04-07 at 20:07 +0800, Chao Gao wrote:
> > +int tdx_module_run_update(void)
> > +{
> > + struct tdx_module_args args = {};
> > + int ret;
> > +
> > + ret = seamcall_prerr(TDH_SYS_UPDATE, &args);
> > + if (ret) {
> > + pr_err("update failed (%d)\n", ret);
> > + tdx_module_status = TDX_MODULE_ERROR;
> > + return ret;
> > + }
>
> The pr_err() isn't needed as seamcall_prerr() will emit a
> message. and no need to set tdx_module_status to ERROR on
> failure as it is already done during shutdown.
>
> so, this can be simplified to:
>
> ret = seamcall_prerr(TDH_SYS_UPDATE, &args);
> if (ret)
> return ret;
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 14/22] x86/virt/seamldr: Do TDX per-CPU initialization after updates
From: Edgecombe, Rick P @ 2026-04-11 2:03 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-15-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> After installing the new TDX module, each CPU needs to be initialized
> again to make the CPU ready to run any other SEAMCALLs. So, call
> tdx_cpu_enable() on all CPUs.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 13/22] x86/virt/seamldr: Install a new TDX module
From: Edgecombe, Rick P @ 2026-04-11 2:01 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-14-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> Following the shutdown of the existing TDX module, the update process
> continues with installing the new module. P-SEAMLDR provides the
> SEAMLDR.INSTALL SEAMCALL to perform this installation, which must be
> executed on all CPUs.
>
> Implement SEAMLDR.INSTALL and execute it on every CPU.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
It seems a pretty straight forward one. My only question would be if the log
needs a bit more info. Not sure what to put though...
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> v6:
> - wrap seamldr_call(P_SEAMLDR_INSTALL..) in a helper [Kiryl]
> v5:
> - drop "serially" from the changelog as it doesn't matter to
> this patch
> ---
> arch/x86/virt/vmx/tdx/seamldr.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index a8bfa30ee55f..3e46f3bfaa8b 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -19,6 +19,7 @@
>
> /* P-SEAMLDR SEAMCALL leaf function */
> #define P_SEAMLDR_INFO 0x8000000000000000
> +#define P_SEAMLDR_INSTALL 0x8000000000000001
>
> #define SEAMLDR_MAX_NR_MODULE_4KB_PAGES 496
> #define SEAMLDR_MAX_NR_SIG_4KB_PAGES 4
> @@ -73,6 +74,13 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
> }
> EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
> +static int seamldr_install(const struct seamldr_params *params)
> +{
> + struct tdx_module_args args = { .rcx = __pa(params) };
In an earlier patch you have a wrapper as:
struct tdx_module_args args = = {};
args.rxx = foo;
Why the style difference? It would be good to standardize, but the existing code
isn't standardized. What do you think about going with this style through the
series for the one arg ones?
> +
> + return seamldr_call(P_SEAMLDR_INSTALL, &args);
> +}
> +
> static void free_seamldr_params(struct seamldr_params *params)
> {
> free_page((unsigned long)params);
> @@ -209,6 +217,7 @@ static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> enum module_update_state {
> MODULE_UPDATE_START,
> MODULE_UPDATE_SHUTDOWN,
> + MODULE_UPDATE_CPU_INSTALL,
> MODULE_UPDATE_DONE,
> };
>
> @@ -266,6 +275,9 @@ static int do_seamldr_install_module(void *seamldr_params)
> if (primary)
> ret = tdx_module_shutdown();
> break;
> + case MODULE_UPDATE_CPU_INSTALL:
> + ret = seamldr_install(seamldr_params);
> + break;
> default:
> break;
> }
^ permalink raw reply
* Re: [PATCH v7 12/22] x86/virt/tdx: Reset software states during TDX module shutdown
From: Edgecombe, Rick P @ 2026-04-11 1:56 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-13-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> The TDX module requires a one-time global initialization (TDH.SYS.INIT) and
> per-CPU initialization (TDH.SYS.LP.INIT) before use. These initializations
> are guarded by software flags to prevent repetition.
>
> After TDX module updates, the new TDX module requires the same global and
> per-CPU initializations, but the existing software flags prevent
> re-initialization.
>
> Reset all software flags guarding the initialization flows to allow the
> global and per-CPU initializations to be triggered again after updates.
>
> Set tdx_module_status to ERROR to indicate the module is unavailable. This
> is to prevent re-initialization/tdx_sysinfo reporting on a failed update.
> Using ERROR instead of UNINITIALIZED as the latter implicitly depends on
> get_tdx_sys_info() failing early to prevent re-init after successful
> shutdown followed by failed update.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
The existing code around sysinit_done/ret is very strange to me. It makes this
patch seem a bit strained, but that is not it's fault.
^ permalink raw reply
* Re: [PATCH v7 11/22] x86/virt/seamldr: Shut down the current TDX module
From: Edgecombe, Rick P @ 2026-04-11 1:36 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <a50bb8a4d8c2bdcb0795eca4551567fb5a577215.camel@intel.com>
On Fri, 2026-04-10 at 18:35 -0700, Rick Edgecombe wrote:
> This one could probably be made less verbose too.
Doh! You already said this.
^ permalink raw reply
* Re: [PATCH v7 11/22] x86/virt/seamldr: Shut down the current TDX module
From: Edgecombe, Rick P @ 2026-04-11 1:35 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-12-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> The first step of TDX module updates is shutting down the current TDX
> Module. This step also packs state information that needs to be
> preserved across updates as handoff data, which will be consumed by the
> updated module. The handoff data is stored internally in the SEAM range
> and is hidden from the kernel.
>
> To ensure a successful update, the new module must be able to consume
> the handoff data generated by the old module. Since handoff data layout
> may change between modules, the handoff data is versioned. Each module
> has a native handoff version and provides backward support for several
> older versions.
>
> The complete handoff versioning protocol is complex as it supports both
> module upgrades and downgrades. See details in Intel® Trust Domain
> Extensions (Intel® TDX) Module Base Architecture Specification, Chapter
> "Handoff Versioning".
>
> Ideally, the kernel needs to retrieve the handoff versions supported by
> the current module and the new module and select a version supported by
> both. But, since this implementation chooses to only support module
> upgrades, simply request the current module to generate handoff data
> using its highest supported version, expecting that the new module will
> likely support it.
I feel like somewhere it's missing what this patch does. It explains the
reasoning for the handoff version selection, but nothing about implement
"MODULE_UPDATE_SHUTDOWN" or anything like that.
>
> Note that only one CPU needs to call the TDX module's shutdown API.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
> v5:
> - Massage changelog [Kai]
> - Avoid "refers to the global copy while populating the tdx_sys_info
> passed as a pointer" [Rick/Yilun]
>
> v4:
> - skip the whole handoff metadata if runtime updates are not supported
> [Yilun]
> v3:
> - remove autogeneration stuff in the changelog
> v2:
> - add a comment about how handoff version is chosen.
> - remove the first !ret in get_tdx_sys_info_handoff() as we edited the
> auto-generated code anyway
> - remove !! when determining whether a CPU is the primary one
> - remove unnecessary if-break nesting in TDP_SHUTDOWN
> ---
> arch/x86/include/asm/tdx_global_metadata.h | 5 +++++
> arch/x86/virt/vmx/tdx/seamldr.c | 11 ++++++++++-
> arch/x86/virt/vmx/tdx/tdx.c | 15 +++++++++++++++
> arch/x86/virt/vmx/tdx/tdx.h | 3 +++
> arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 20 ++++++++++++++++++++
> 5 files changed, 53 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/tdx_global_metadata.h b/arch/x86/include/asm/tdx_global_metadata.h
> index 40689c8dc67e..8a9ebd895e70 100644
> --- a/arch/x86/include/asm/tdx_global_metadata.h
> +++ b/arch/x86/include/asm/tdx_global_metadata.h
> @@ -40,12 +40,17 @@ struct tdx_sys_info_td_conf {
> u64 cpuid_config_values[128][2];
> };
>
> +struct tdx_sys_info_handoff {
> + u16 module_hv;
> +};
> +
> struct tdx_sys_info {
> struct tdx_sys_info_version version;
> struct tdx_sys_info_features features;
> struct tdx_sys_info_tdmr tdmr;
> struct tdx_sys_info_td_ctrl td_ctrl;
> struct tdx_sys_info_td_conf td_conf;
> + struct tdx_sys_info_handoff handoff;
> };
>
> #endif
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index 5964bbc67944..a8bfa30ee55f 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -15,6 +15,7 @@
> #include <asm/seamldr.h>
>
> #include "seamcall_internal.h"
> +#include "tdx.h"
>
> /* P-SEAMLDR SEAMCALL leaf function */
> #define P_SEAMLDR_INFO 0x8000000000000000
> @@ -207,6 +208,7 @@ static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> */
> enum module_update_state {
> MODULE_UPDATE_START,
> + MODULE_UPDATE_SHUTDOWN,
> MODULE_UPDATE_DONE,
> };
>
> @@ -246,8 +248,12 @@ static void ack_state(void)
> static int do_seamldr_install_module(void *seamldr_params)
> {
> enum module_update_state newstate, curstate = MODULE_UPDATE_START;
> + int cpu = smp_processor_id();
> + bool primary;
> int ret = 0;
>
> + primary = cpumask_first(cpu_online_mask) == cpu;
> +
> do {
> /* Chill out and re-read update_data. */
> cpu_relax();
> @@ -256,7 +262,10 @@ static int do_seamldr_install_module(void *seamldr_params)
> if (newstate != curstate) {
> curstate = newstate;
> switch (curstate) {
> - /* TODO: add the update steps. */
> + case MODULE_UPDATE_SHUTDOWN:
> + if (primary)
> + ret = tdx_module_shutdown();
> + break;
> default:
> break;
> }
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 172f6d4133b5..f87fad429f4e 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1176,6 +1176,21 @@ int tdx_enable(void)
> }
> EXPORT_SYMBOL_FOR_KVM(tdx_enable);
>
> +int tdx_module_shutdown(void)
> +{
> + struct tdx_module_args args = {};
> +
> + /*
> + * Shut down the TDX module and prepare handoff data for the next
> + * TDX module. This SEAMCALL requires a handoff version. Use the
> + * module's handoff version, as it is the highest version the
> + * module can produce and is more likely to be supported by new
> + * modules as new modules likely have higher handoff version.
> + */
This one could probably be made less verbose too.
> + args.rcx = tdx_sysinfo.handoff.module_hv;
> + return seamcall_prerr(TDH_SYS_SHUTDOWN, &args);
> +}
> +
> static bool is_pamt_page(unsigned long phys)
> {
> struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 82bb82be8567..1c4da9540ae0 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -46,6 +46,7 @@
> #define TDH_PHYMEM_PAGE_WBINVD 41
> #define TDH_VP_WR 43
> #define TDH_SYS_CONFIG 45
> +#define TDH_SYS_SHUTDOWN 52
>
> /*
> * SEAMCALL leaf:
> @@ -118,4 +119,6 @@ struct tdmr_info_list {
> int max_tdmrs; /* How many 'tdmr_info's are allocated */
> };
>
> +int tdx_module_shutdown(void);
> +
> #endif
> diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> index 34c050d74b56..225e157805e8 100644
> --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> @@ -100,6 +100,19 @@ static int get_tdx_sys_info_td_conf(struct tdx_sys_info_td_conf *sysinfo_td_conf
> return ret;
> }
>
> +static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff)
> +{
> + int ret;
> + u64 val;
> +
> + ret = read_sys_metadata_field(0x8900000100000000, &val);
> + if (ret)
> + return ret;
> +
> + sysinfo_handoff->module_hv = val;
> + return 0;
> +}
> +
> static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> {
> int ret = 0;
> @@ -116,5 +129,12 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl);
> ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf);
>
> + /*
> + * Don't treat a module that doesn't support update as a failure.
> + * Only read the metadata optionally.
> + */
> + if (tdx_supports_runtime_update(sysinfo))
> + ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff);
> +
> return ret;
> }
^ permalink raw reply
* Re: [PATCH v7 10/22] x86/virt/seamldr: Abort updates if errors occurred midway
From: Edgecombe, Rick P @ 2026-04-11 1:26 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-11-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> The TDX module update process has multiple steps, each of which may
> encounter failures.
>
> The current state machine of updates proceeds to the next step regardless
> of errors. But continuing updates when errors occur midway is pointless.
This kind of begs the question of how much it matters if some pointless work
happens in error condition during a rare operation. I'm thinking at this point,
aha!, do we need this?
>
> Abort the update by setting a flag to indicate that a CPU has encountered
> an error, forcing all CPUs to exit the execution loop. Note that failing
> CPUs do not acknowledge the current step. This keeps all other CPUs waiting
> in the current step (since advancing to the next step requires all CPUs to
> acknowledge the current step) until they detect the fault flag and exit the
> loop.
So is the point of the patch to prevent the operation from getting stuck? Or
saving the user experiencing a failed update a little time?
^ permalink raw reply
* Re: [PATCH v7 09/22] x86/virt/seamldr: Introduce skeleton for TDX module updates
From: Edgecombe, Rick P @ 2026-04-11 1:23 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-10-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> Potential alternative to stop_machine()
> =======================================
> An alternative approach is to lock all KVM entry points and kick all
> vCPUs. Here, KVM entry points refer to KVM VM/vCPU ioctl entry points,
> implemented in KVM common code (virt/kvm). Adding a locking mechanism
> there would affect all architectures KVM supports. And to lock only TDX
> vCPUs, new logic would be needed to identify TDX vCPUs, which the KVM
> common code currently lacks. This would add significant complexity and
> maintenance overhead to KVM for this TDX-specific use case.
I'd add a little imperative ending like ", so don't take this approach."
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Edgecombe, Rick P @ 2026-04-11 1:14 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-9-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> P-SEAMLDR uses the SEAMLDR_PARAMS structure to describe TDX module
> update requests. This structure contains physical addresses pointing to
> the module binary and its signature file (or sigstruct), along with an
> update scenario field.
>
> TDX modules are distributed in the tdx_blob format defined in
> blob_structure.txt from the "Intel TDX module Binaries Repository". A
> tdx_blob contains a header, sigstruct, and module binary. This is also the
> format supplied by the userspace to the kernel.
>
> Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure
> accordingly. This structure will be passed to P-SEAMLDR to initiate the
> update.
The thing that confused me about this earlier was the exact reason why we are
checking all the fields. We discussed that we need to check the fields that
kernel processes, but we don't need to double check data that the TDX module
processes.
Should we explain it? And how it explains the checks below?
>
> Note that the sigstruct_pa field in SEAMLDR_PARAMS has been extended to
> a 4-element array. The updated "SEAM Loader (SEAMLDR) Interface
> Specification" will be published separately. P-SEAMLDR compatibility
> validation (such as 4KB vs 16KB sigstruct support) is left to userspace,
> which must verify the P-SEAMLDR version meets the TDX module's minimum
> requirements.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
> v7:
> - add blob size/alignment validation. Ensure 4KB chunking is clearly
> correct: serves as both defense and documentation
> - split a long one-line comment [Kiryl]
>
> v6:
> - clarify tdx_blob's @offset_of_module and @len fields [Kiryl]
> - clarify comment to explicitly call out the PAGE_SIZE != SZ_4K case
> [Kiryl]
>
> v5:
> - use a macro for tdx_blob version (0x100) [Yan]
> - don't do alignment checking for the binary/sigstruct [Rick]
> - drop blob's sigstruct and validation checking
> - set seamldr_params.version to 1 when necessary
> - drop the link to blob_structure.txt which might be unstable [Kai]
>
> v4:
> - Remove checksum verification as it is optional
> - Convert comments to is_vmalloc_addr() checks [Kai]
> - Explain size/alignment checks in alloc_seamldr_params() [Kai]
>
> v3:
> - Print tdx_blob version in hex [Binbin]
> - Drop redundant sigstruct alignment check [Yilun]
> - Note buffers passed from firmware upload infrastructure are
> vmalloc()'d above alloc_seamldr_params()
> ---
> arch/x86/virt/vmx/tdx/seamldr.c | 163 ++++++++++++++++++++++++++++++++
> 1 file changed, 163 insertions(+)
>
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index e93a5d90a3ee..219a8e0c7127 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -7,6 +7,7 @@
> #define pr_fmt(fmt) "seamldr: " fmt
>
> #include <linux/mm.h>
> +#include <linux/slab.h>
> #include <linux/spinlock.h>
>
> #include <asm/seamldr.h>
> @@ -16,6 +17,33 @@
> /* P-SEAMLDR SEAMCALL leaf function */
> #define P_SEAMLDR_INFO 0x8000000000000000
>
> +#define SEAMLDR_MAX_NR_MODULE_4KB_PAGES 496
> +#define SEAMLDR_MAX_NR_SIG_4KB_PAGES 4
> +
> +/*
> + * The seamldr_params "scenario" field specifies the operation mode:
> + * 0: Install TDX module from scratch (not used by kernel)
> + * 1: Update existing TDX module to a compatible version
> + */
> +#define SEAMLDR_SCENARIO_UPDATE 1
> +
> +/*
> + * This is called the "SEAMLDR_PARAMS" data structure and is defined
> + * in "SEAM Loader (SEAMLDR) Interface Specification".
> + *
> + * It describes the TDX module that will be installed.
> + */
> +struct seamldr_params {
> + u32 version;
> + u32 scenario;
> + u64 sigstruct_pa[SEAMLDR_MAX_NR_SIG_4KB_PAGES];
> + u8 reserved[80];
> + u64 num_module_pages;
> + u64 mod_pages_pa_list[SEAMLDR_MAX_NR_MODULE_4KB_PAGES];
> +} __packed;
> +
> +static_assert(sizeof(struct seamldr_params) == 4096);
> +
> /*
> * Serialize P-SEAMLDR calls since the hardware only allows a single CPU to
> * interact with P-SEAMLDR simultaneously. Use raw version as the calls can
> @@ -42,6 +70,136 @@ int seamldr_get_info(struct seamldr_info *seamldr_info)
> }
> EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
> +static void free_seamldr_params(struct seamldr_params *params)
> +{
> + free_page((unsigned long)params);
> +}
Do we really need this helper? This doesn't work?
DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
if (!IS_ERR_OR_NULL(_T)) free_page((unsigned long)_T))
> +
> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
> + const void *sig, unsigned int sig_size)
> +{
> + struct seamldr_params *params;
> + const u8 *ptr;
> + int i;
> +
> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
> + return ERR_PTR(-EINVAL);
> +
> + if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K)
> + return ERR_PTR(-EINVAL);
I don't know if it's worth that much, but we could do a MIN thing here to
protect the loop, and lose the conditionals. If userspace passes a blob that is
out of spec they can deal with the module error, no?
> +
> + /*
> + * Check that input buffers satisfy P-SEAMLDR's size and alignment
> + * constraints so they can be passed directly to P-SEAMLDR without
> + * relocation or copy.
> + */
> + if (!IS_ALIGNED(module_size, SZ_4K) || !IS_ALIGNED(sig_size, SZ_4K) ||
> + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
> + !IS_ALIGNED((unsigned long)sig, SZ_4K))
> + return ERR_PTR(-EINVAL);
I thought you are going to reduce this checking to just to reject invalid input
that the kernel processes.
What happens if we don't check this? The vmallocs are all going to be page
aligned anyway. But even still, does it mess up the below loops somehow in a way
that hurts anything?
I might be confused, but it seems different then we discussed.
> +
> + params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> + if (!params)
> + return ERR_PTR(-ENOMEM);
> +
> + /*
> + * Only use version 1 when required (sigstruct > 4KB) for backward
> + * compatibility with P-SEAMLDR that lacks version 1 support.
> + */
> + if (sig_size > SZ_4K)
> + params->version = 1;
> + else
> + params->version = 0;
I'm a bit confused by this part. What does it mean to support old P-SEAMLDRs?
But also could it be:
params->version = sig_size > SZ_4K;
> +
> + params->scenario = SEAMLDR_SCENARIO_UPDATE;
> +
> + ptr = sig;
> + for (i = 0; i < sig_size / SZ_4K; i++) {
> + /*
> + * @sig is 4KB-aligned, but that does not imply PAGE_SIZE
> + * alignment when PAGE_SIZE != SZ_4K. Always include the
> + * in-page offset.
> + */
> + params->sigstruct_pa[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
> + ptr += SZ_4K;
> + }
> +
> + params->num_module_pages = module_size / SZ_4K;
> +
> + ptr = module;
> + for (i = 0; i < params->num_module_pages; i++) {
> + params->mod_pages_pa_list[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
> + ptr += SZ_4K;
> + }
> +
> + return params;
> +}
> +
> +/*
> + * Intel TDX module blob. Its format is defined at:
> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
> + *
> + * Note this structure differs from the reference above: the two variable-length
> + * fields "@sigstruct" and "@module" are represented as a single "@data" field
> + * here and split programmatically using the offset_of_module value.
> + *
> + * Note @offset_of_module is relative to the start of struct tdx_blob, not
> + * @data, and @length is the total length of the blob, not the length of
> + * @data.
> + */
> +struct tdx_blob {
> + u16 version;
> + u16 checksum;
> + u32 offset_of_module;
> + u8 signature[8];
> + u32 length;
> + u32 reserved0;
> + u64 reserved1[509];
> + u8 data[];
> +} __packed;
> +
> +/* Supported versions of the tdx_blob */
> +#define TDX_BLOB_VERSION_1 0x100
> +
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> + int module_size, sig_size;
> + const void *sig, *module;
> +
> + /*
> + * Ensure the size is valid otherwise reading any field from the
> + * blob may overflow.
> + */
> + if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
> + return ERR_PTR(-EINVAL);
> +
> + if (blob->version != TDX_BLOB_VERSION_1)
> + return ERR_PTR(-EINVAL);
> +
> + if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
> + return ERR_PTR(-EINVAL);
> +
> + /* Split the blob into a sigstruct and a module. */
> + sig = blob->data;
> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
> + module = data + blob->offset_of_module;
> + module_size = size - blob->offset_of_module;
Did you consider just passing the tdx_blob into alloc_seamldr_params()?
Basically, this function checks the blob fields, then alloc_seamldr_params()
turns blob into struct seamldr_params without checks. The way it is, the work
seems kind of spread around two functions with various checks.
> +
> + if (sig_size <= 0 || module_size <= 0 || blob->length != size)
> + return ERR_PTR(-EINVAL);
> +
> + if (memcmp(blob->signature, "TDX-BLOB", 8))
> + return ERR_PTR(-EINVAL);
> +
> + return alloc_seamldr_params(module, module_size, sig, sig_size);
> +}
> +
> +DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> + if (!IS_ERR_OR_NULL(_T)) free_seamldr_params(_T))
> +
> /**
> * seamldr_install_module - Install a new TDX module.
> * @data: Pointer to the TDX module update blob.
> @@ -51,6 +209,11 @@ EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
> */
> int seamldr_install_module(const u8 *data, u32 size)
> {
> + struct seamldr_params *params __free(free_seamldr_params) =
> + init_seamldr_params(data, size);
> + if (IS_ERR(params))
> + return PTR_ERR(params);
> +
> /* TODO: Update TDX module here */
> return 0;
> }
^ permalink raw reply
* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Edgecombe, Rick P @ 2026-04-11 0:33 UTC (permalink / raw)
To: Hansen, Dave, Gao, Chao
Cc: linux-kernel@vger.kernel.org, linux-coco@lists.linux.dev,
Huang, Kai, kvm@vger.kernel.org, Li, Xiaoyao, Zhao, Yan Y,
dave.hansen@linux.intel.com, tony.lindgren@linux.intel.com,
Chatre, Reinette, seanjc@google.com, pbonzini@redhat.com,
binbin.wu@linux.intel.com, Weiny, Ira, nik.borisov@suse.com,
mingo@redhat.com, Verma, Vishal L, kas@kernel.org,
sagis@google.com, Annapurve, Vishal, hpa@zytor.com,
tglx@kernel.org, paulmck@kernel.org, bp@alien8.de,
yilun.xu@linux.intel.com, dan.j.williams@intel.com,
x86@kernel.org
In-Reply-To: <aczW7MZBqsCXw4gk@intel.com>
On Wed, 2026-04-01 at 16:27 +0800, Chao Gao wrote:
> And I assume we don't need WARN_ON_ONCE(PAGE_SIZE != SZ_4K) since this is
> unlikely to break soon and shouldn't be very hard to debug and fix if it
> does.
+1
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Edgecombe, Rick P @ 2026-04-11 0:26 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-8-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
> + u32 offset, u32 size, u32 *written)
> +{
> + int ret;
> +
> + /*
> + * tdx_fw_write() always processes all data on the first call with
> + * offset == 0. Since it never returns partial success (it either
> + * succeeds completely or fails), there is no subsequent call with
> + * non-zero offsets.
> + */
> + WARN_ON_ONCE(offset);
> + ret = seamldr_install_module(data, size);
> + switch (ret) {
> + case 0:
> + *written = size;
> + return FW_UPLOAD_ERR_NONE;
> + case -EBUSY:
> + return FW_UPLOAD_ERR_BUSY;
> + case -EIO:
> + return FW_UPLOAD_ERR_HW_ERROR;
> + case -ENOMEM:
> + return FW_UPLOAD_ERR_RW_ERROR;
> + default:
> + return FW_UPLOAD_ERR_FW_INVALID;
It's hard to review whether these error codes match because the function doesn't
return them yet. Why isn't this patch just done later in the series after
everything is in place?
> + }
> +}
> +
^ permalink raw reply
* Re: [PATCH v7 05/22] x86/virt/seamldr: Add a helper to retrieve P-SEAMLDR information
From: Edgecombe, Rick P @ 2026-04-11 0:13 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-6-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> P-SEAMLDR returns its information such as version number, in response to
> the SEAMLDR.INFO SEAMCALL.
>
> This information is useful for userspace. For example, the admin can decide
> which TDX module versions are compatible with the P-SEAMLDR according to
> the P-SEAMLDR version.
We know what this will be used for, so it would have been nice to say it. Like:
This information is useful for userspace admins to decide.... Future changes
will expose it to userspace...
Plus the remaining updates part is not hinted at.
>
> Add a helper to retrieve P-SEAMLDR information in preparation for
> exposing P-SEAMLDR version and other necessary information to userspace.
> Export the new kAPI for use by tdx-host.ko.
>
> Note that there are two distinct P-SEAMLDR APIs with similar names:
>
> SEAMLDR.INFO: Returns a SEAMLDR_INFO structure containing SEAMLDR
> information such as version and remaining updates.
>
> SEAMLDR.SEAMINFO: Returns a SEAMLDR_SEAMINFO structure containing SEAM
> and system information such as Convertible Memory
> Regions (CMRs) and number of CPUs and sockets.
>
> The former is used here.
>
> For details, see "Intel® Trust Domain Extensions - SEAM Loader (SEAMLDR)
> Interface Specification".
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
Either way,
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 04/22] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Edgecombe, Rick P @ 2026-04-10 23:58 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao,
linux-rt-devel@lists.linux.dev
Cc: bigeasy@linutronix.de, Li, Xiaoyao, Huang, Kai, Zhao, Yan Y,
dave.hansen@linux.intel.com, rostedt@goodmis.org, kas@kernel.org,
seanjc@google.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
mingo@redhat.com, Weiny, Ira, clrkwllms@kernel.org,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, hpa@zytor.com, tglx@kernel.org,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
dan.j.williams@intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-5-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> The TDX architecture uses the "SEAMCALL" instruction to communicate with
> SEAM mode software. Right now, the only SEAM mode software that the kernel
> communicates with is the TDX module. But, there is actually another
> component that runs in SEAM mode but it is separate from the TDX module:
> the persistent SEAM loader or "P-SEAMLDR". Right now, the only component
> that communicates with it is the BIOS which loads the TDX module itself at
> boot. But, to support updating the TDX module, the kernel now needs to be
> able to talk to it.
>
> P-SEAMLDR SEAMCALLs differ from TDX module SEAMCALLs in areas such as
> concurrency requirements. Add a P-SEAMLDR wrapper to handle these
> differences and prepare for implementing concrete functions.
>
> Use seamcall_prerr() (not '_ret') because current P-SEAMLDR calls do not
> use any output registers other than RAX.
>
> Note that unlike P-SEAMLDR, there is also a non-persistent SEAM loader
> ("NP-SEAMLDR"). This is an authenticated code module (ACM) that is not
> callable at runtime. Only BIOS launches it to load P-SEAMLDR at boot;
> the kernel does not need to interact with it for runtime update.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/733582 # [1]
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 01/22] x86/virt/tdx: Move low level SEAMCALL helpers out of <asm/tdx.h>
From: Edgecombe, Rick P @ 2026-04-10 23:42 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao
Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Chatre, Reinette, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
tony.lindgren@linux.intel.com, Annapurve, Vishal,
sagis@google.com, Duan, Zhenzhong, tglx@kernel.org,
paulmck@kernel.org, hpa@zytor.com, bp@alien8.de,
yilun.xu@linux.intel.com, dan.j.williams@intel.com,
x86@kernel.org
In-Reply-To: <20260331124214.117808-2-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> From: Kai Huang <kai.huang@intel.com>
>
> TDX host core code implements three seamcall*() helpers to make SEAMCALL
This patch already has Dave's ack. So nits are not really in order here. But it
bothers me that the very first sentence of the series has what looks like a
typo. Not the best first impression. Should be "SEAMCALLs"?
> to the TDX module. Currently, they are implemented in <asm/tdx.h> and
> are exposed to other kernel code which includes <asm/tdx.h>.
^ permalink raw reply
* [PATCH v2] KVM: TDX: Fix x2APIC MSR handling in tdx_has_emulated_msr()
From: Rick Edgecombe @ 2026-04-10 23:26 UTC (permalink / raw)
To: kas, kvm, linux-coco, linux-kernel, pbonzini, seanjc, binbin.wu,
dmaluka
Cc: rick.p.edgecombe
Rework tdx_has_emulated_msr() to explicitly enumerate the x2APIC MSRs
that KVM can emulate, instead of trying to enumerate the MSRs that KVM
cannot emulate. Drop the inner switch and list the emulatable x2APIC
registers directly in the outer switch's "return true" block.
The old code had multiple bugs in the x2APIC range handling.
X2APIC_MSR(APIC_ISR + APIC_ISR_NR) was incorrect because APIC_ISR_NR is
0x8, not 0x80, so the X2APIC_MSR() shift lost the lower bits, collapsing
each range to a single MSR. IA32_X2APIC_SELF_IPI was also missing from
the non-emulatable list.
KVM has no visibility into whether or not a guest has enabled #VE
reduction, which changes which MSRs the TDX-Module handles itself versus
triggering a #VE for the guest to make a TDVMCALL. So maintaining a list
of non-emulatable MSRs is fragile. Listing only the MSRs KVM can always
emulate sidesteps the problem.
Suggested-by: Sean Christopherson <seanjc@google.com>
Reported-by: Dmytro Maluka <dmaluka@chromium.org>
Fixes: dd50294f3e3c ("KVM: TDX: Implement callbacks for MSR operations")
Assisted-by: Claude:claude-opus-4-6
[based on a diff from Sean, but added missed LVTCMCI case, log]
Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
Thanks to Dmytro for finding this. They said to feel free to take this
over, so here is another version with Sean's suggestions. Tested in the
TDX CI.
In Sean's suggestion LVTCMCI was missed, so it's added here.
arch/x86/kvm/vmx/tdx.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 1e47c194af53..76ab6805ab29 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2116,23 +2116,27 @@ bool tdx_has_emulated_msr(u32 index)
case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
/* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
case MSR_KVM_POLL_CONTROL:
+ /*
+ * x2APIC registers that are virtualized by the CPU can't be
+ * emulated, KVM doesn't have access to the virtual APIC page.
+ */
+ case X2APIC_MSR(APIC_ID):
+ case X2APIC_MSR(APIC_LVR):
+ case X2APIC_MSR(APIC_LDR):
+ case X2APIC_MSR(APIC_SPIV):
+ case X2APIC_MSR(APIC_ESR):
+ case X2APIC_MSR(APIC_LVTCMCI):
+ case X2APIC_MSR(APIC_ICR):
+ case X2APIC_MSR(APIC_LVTT):
+ case X2APIC_MSR(APIC_LVTTHMR):
+ case X2APIC_MSR(APIC_LVTPC):
+ case X2APIC_MSR(APIC_LVT0):
+ case X2APIC_MSR(APIC_LVT1):
+ case X2APIC_MSR(APIC_LVTERR):
+ case X2APIC_MSR(APIC_TMICT):
+ case X2APIC_MSR(APIC_TMCCT):
+ case X2APIC_MSR(APIC_TDCR):
return true;
- case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
- /*
- * x2APIC registers that are virtualized by the CPU can't be
- * emulated, KVM doesn't have access to the virtual APIC page.
- */
- switch (index) {
- case X2APIC_MSR(APIC_TASKPRI):
- case X2APIC_MSR(APIC_PROCPRI):
- case X2APIC_MSR(APIC_EOI):
- case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
- case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
- case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
- return false;
- default:
- return true;
- }
default:
return false;
}
--
2.53.0
^ permalink raw reply related
* Re: [PATCH v2 00/16] fs,x86/resctrl: Add kernel-mode (e.g., PLZA) support to the resctrl subsystem
From: Moger, Babu @ 2026-04-10 22:52 UTC (permalink / raw)
To: Reinette Chatre, Babu Moger, corbet@lwn.net, tony.luck@intel.com,
Dave.Martin@arm.com, james.morse@arm.com, tglx@kernel.org,
mingo@redhat.com, bp@alien8.de, dave.hansen@linux.intel.com
Cc: skhan@linuxfoundation.org, x86@kernel.org, hpa@zytor.com,
peterz@infradead.org, juri.lelli@redhat.com,
vincent.guittot@linaro.org, dietmar.eggemann@arm.com,
rostedt@goodmis.org, bsegall@google.com, mgorman@suse.de,
vschneid@redhat.com, kas@kernel.org, rick.p.edgecombe@intel.com,
akpm@linux-foundation.org, pmladek@suse.com,
rdunlap@infradead.org, dapeng1.mi@linux.intel.com,
kees@kernel.org, elver@google.com, paulmck@kernel.org,
lirongqing@baidu.com, safinaskar@gmail.com, fvdl@google.com,
seanjc@google.com, pawan.kumar.gupta@linux.intel.com,
xin@zytor.com, tiala@microsoft.com, chang.seok.bae@intel.com,
Lendacky, Thomas, elena.reshetova@intel.com,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-coco@lists.linux.dev, kvm@vger.kernel.org,
eranian@google.com, peternewman@google.com
In-Reply-To: <68a551ea-d9f0-436a-9bef-e35fd027bb95@intel.com>
Hi Reinette,
On 4/9/2026 10:41 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/9/26 4:42 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 4/9/2026 3:50 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/9/26 11:05 AM, Moger, Babu wrote:
>>>> On 4/9/2026 12:26 PM, Reinette Chatre wrote:
>>>>> On 4/9/26 10:19 AM, Moger, Babu wrote:
>>>>>> On 4/8/2026 6:41 PM, Reinette Chatre wrote:
>>>>>
>>>>>>> When the user switches to either "global_assign_ctrl_inherit_mon_per_cpu" or
>>>>>>> 'global_assign_ctrl_assign_mon_per_cpu" then "info/kernel_mode_assignment" is created
>>>>>>> (or made visible to user space) and is expected to point to default group.
>>>>>>> User can change the group using "info/kernel_mode_assignment" at this point.
>>>>>>>
>>>>>>> If the current scenario is below ...
>>>>>>> # cat info/kernel_mode
>>>>>>> [global_assign_ctrl_inherit_mon_per_cpu]
>>>>>>> inherit_ctrl_and_mon
>>>>>>> global_assign_ctrl_assign_mon_per_cpu
>>>>>>>
>>>>>>> ... then "info/kernel_mode_assignment" will exist but what it should contain if
>>>>>>> user switches mode at this point may be up for discussion.
>>>>>>>
>>>>>>> option 1)
>>>>>>> When user switches mode to "global_assign_ctrl_assign_mon_per_cpu" then
>>>>>>> the resource group in "info/kernel_mode_assignment" is reset to the
>>>>>>> default group and all CPUs PLZA state reset to match. The kernel_mode_cpus
>>>>>>> and kernel_mode_cpuslist files become visible in default resource group
>>>>>>> and they contain "all online CPUs".
>>>>>>>
>>>>>>> option 2)
>>>>>>> When user switches mode to "global_assign_ctrl_assign_mon_per_cpu" then
>>>>>>> the resource group in "info/kernel_mode_assignment" is kept and all
>>>>>>> CPUs PLZA state set to match it while also keeping the current
>>>>>>> values of that resource group's kernel_mode_cpus and kernel_mode_cpuslist
>>>>>>> files.
>>>>>>>
>>>>>>> I am leaning towards "option 1" to keep it consistent with a switch from
>>>>>>> "inherit_ctrl_and_mon" and being deterministic about how a mode is started with
>>>>>>
>>>>>> Yes. The "option 1" seems appropriate.
>>>>>>
>>>>>>> a clean slate. What are your thoughts? What would be use case where a user would
>>>>>>> want to switch between "global_assign_ctrl_inherit_mon_per_cpu" and
>>>>>>> "global_assign_ctrl_assign_mon_per_cpu" to just switch rmid_en on and off?
>>>>>>
>>>>>>
>>>>>> This is a bit tricky.
>>>>>>
>>>>>> Currently, our requirement is to have a CTRL_MON group for
>>>>>> global_assign_ctrl_inherit_mon_per_cpu. In this scenario, we use the
>>>>>> group’s CLOSID for PLZA configuration, and RMID is not used (rmid_en
>>>>>> = 0) when setting up PLZA.
>>>>>>
>>>>>> Our requirement is also to have a CTRL_MON/MON group for
>>>>>> global_assign_ctrl_assign_mon_per_cpu. In this case as well, the
>>>>>> group’s CLOSID and RMID (rmid_en = 1) both are used configure PLZA.
>>>>>
>>>>> ah, right. Good catch.
>>>>>
>>>>>>
>>>>>> Actually, we should not allow these changes from
>>>>>> global_assign_ctrl_inherit_mon_per_cpu to
>>>>>> global_assign_ctrl_assign_mon_per_cpu or visa versa.
>>>>>
>>>>> resctrl could allow it but as part of the switch it resets the "kernel mode group" to
>>>>> be the default group every time? This would be the "option 1" above.
>>>>
>>>> Other options.
>>>>
>>>> Allow global_assign_ctrl_inherit_mon_per_cpu -> global_assign_ctrl_assign_mon_per_cpu. As part of the switch, reset the "kernel mode group" to the default group.
>>>>
>>>> Allow global_assign_ctrl_assign_mon_per_cpu -> global_assign_ctrl_inherit_mon_per_cpu. In this case switch
>>>> to CTRL_MON/MON -> CTRL_MON.
>>>>
>>>
>>> ok. Could you please return the courtesy of providing feedback on the
>>> suggestion you are responding to and also include the motivation why your
>>> suggestion is the better option?
>>
>> Yea. Sure.
>>
>> We need to allow the switch between the modes. Otherwise only way to reset is to remount the resctrl filesystem. That is not a good option.
>>
>> Allow global_assign_ctrl_inherit_mon_per_cpu -> global_assign_ctrl_assign_mon_per_cpu. As part of the switch, reset the "kernel mode group" to the default group.
>>
>> This option is same as you suggested.
>>
>> Allow global_assign_ctrl_assign_mon_per_cpu -> global_assign_ctrl_inherit_mon_per_cpu. In this case switch
>> to CTRL_MON/MON -> CTRL_MON. This option basically disables monitor (rmid_en=0). It is less disruptive. Move is between child group to parent group.
>
> ok. I am concerned that this creates an inconsistent interface. Specifically, sometimes
> when switching the mode the kernel group will reset and sometimes it won't. This inconsistency
> may be more apparent when writing the user documentation as part of this work. If you are
> able to clearly explain how this resctrl fs interface behaves (this cannot be about PLZA
> internals as above) then this could work.
>
Yes, certainly. I’ll begin work on v3, and we can continue refining it
as we move forward.
Thanks
Babu
^ permalink raw reply
* Re: [PATCH v2 1/6] KVM: x86: Add dedicated storage for guest RIP
From: Chang S. Bae @ 2026-04-10 18:43 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
Cc: kvm, x86, linux-coco, linux-kernel
In-Reply-To: <20260409224236.2021562-2-seanjc@google.com>
On 4/9/2026 3:42 PM, Sean Christopherson wrote:
> Add kvm_vcpu_arch.rip to track guest RIP instead of including it in the
> generic regs[] array. Decoupling RIP from regs[] will allow using a
> *completely* arbitrary index for RIP, as opposed to the mostly-arbitrary
> index that is currently used. That in turn will allow using indices
> 16-31 to track R16-R31 that are coming with APX....
> - unsigned long regs[NR_VCPU_REGS];
> + unsigned long regs[NR_VCPU_GENERAL_PURPOSE_REGS];
> + unsigned long rip;
Digging the history, this effectively reverts part of changes in
commit 5fdbf9765b7b ("KVM: x86: accessors for guest registers")
which had
- unsigned long regs[NR_VCPU_REGS];
- unsigned long rip; /* needs vcpu_load_rsp_rip() */
+ /*
+ * rip and regs accesses must go through
+ * kvm_{register,rip}_{read,write} functions.
+ */
+ unsigned long regs[NR_VCPU_REGS];
But its changelog didn't go into much detail about this change. I could
only relate to vcpu_load_rsp_rip() which might establish perception
coupling RSP with RIP back then.
In any case, it doesn't matter. I think this patch makes a clear
improvement - for example, now aligns with _regs[NR_EMULATOR_GPRS] in
struct x86_emulate_ctxt for general consistency.
Indeed, this and the whole series paves the way for APX. Appreciate for
the time and effort!
Reviewed-by: Chang S. Bae <chang.seok.bae@intel.com>
^ permalink raw reply
* Re: [PATCH v13 46/48] KVM: arm64: Expose KVM_ARM_VCPU_REC to user space
From: Steven Price @ 2026-04-10 15:12 UTC (permalink / raw)
To: Suzuki K Poulose, kvm, kvmarm
Cc: Catalin Marinas, Marc Zyngier, Will Deacon, James Morse,
Oliver Upton, Zenghui Yu, linux-arm-kernel, linux-kernel,
Joey Gouly, Alexandru Elisei, Christoffer Dall, Fuad Tabba,
linux-coco, Ganapatrao Kulkarni, Gavin Shan, Shanker Donthineni,
Alper Gun, Aneesh Kumar K . V, Emi Kisanuki, Vishal Annapurve
In-Reply-To: <79564d59-032e-40c9-b4eb-f79f805b8238@arm.com>
On 19/03/2026 17:36, Suzuki K Poulose wrote:
> On 18/03/2026 15:54, Steven Price wrote:
>> Increment KVM_VCPU_MAX_FEATURES to expose the new capability to user
>> space.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>
> Not needed any more as we don't need the VCPU feature.
This patch caused so much bother with rebasing in the past, I'd
completely forgotten it isn't actually needed! Thanks for spotting!
Thanks,
Steve
> Cheers
> Suzuki
>
>
>
>> ---
>> Changes since v8:
>> * Since NV is now merged and enabled, this no longer conflicts with it.
>> ---
>> arch/arm64/include/asm/kvm_host.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/
>> asm/kvm_host.h
>> index 1d5fb001408c..b02f97de4436 100644
>> --- a/arch/arm64/include/asm/kvm_host.h
>> +++ b/arch/arm64/include/asm/kvm_host.h
>> @@ -40,7 +40,7 @@
>> #define KVM_MAX_VCPUS VGIC_V3_MAX_CPUS
>> -#define KVM_VCPU_MAX_FEATURES 9
>> +#define KVM_VCPU_MAX_FEATURES 10
>> #define KVM_VCPU_VALID_FEATURES (BIT(KVM_VCPU_MAX_FEATURES) - 1)
>> #define KVM_REQ_SLEEP \
>
^ permalink raw reply
* Re: [PATCH v13 37/48] arm64: RMI: Prevent Device mappings for Realms
From: Steven Price @ 2026-04-10 15:12 UTC (permalink / raw)
To: Joey Gouly
Cc: kvm, kvmarm, Catalin Marinas, Marc Zyngier, Will Deacon,
James Morse, Oliver Upton, Suzuki K Poulose, Zenghui Yu,
linux-arm-kernel, linux-kernel, Alexandru Elisei,
Christoffer Dall, Fuad Tabba, linux-coco, Ganapatrao Kulkarni,
Gavin Shan, Shanker Donthineni, Alper Gun, Aneesh Kumar K . V,
Emi Kisanuki, Vishal Annapurve
In-Reply-To: <20260319102734.GC3942350@e124191.cambridge.arm.com>
On 19/03/2026 10:27, Joey Gouly wrote:
> On Wed, Mar 18, 2026 at 03:54:01PM +0000, Steven Price wrote:
>> Physical device assignment is not supported by RMM v1.0, so it
>
> But we're targetting 2.0 now!
Whoops ;) In my head it's still "in the future" but I guess the "future"
is now!
I'll update this to say:
Physical device assignment is not yet supported. RMM v2.0 does add the
relevant APIs, but device assignment is a big topic so will be handled
in a future patch series. For now prevent device mappings when the guest
is a realm.
Thanks,
Steve
> I guess just change it to something about device support being a later feature.
>
> Thanks,
> Joey
>
>> doesn't make much sense to allow device mappings within the realm.
>> Prevent them when the guest is a realm.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes from v6:
>> * Fix the check in user_mem_abort() to prevent all pages that are not
>> guest_memfd() from being mapped into the protected half of the IPA.
>> Changes from v5:
>> * Also prevent accesses in user_mem_abort()
>> ---
>> arch/arm64/kvm/mmu.c | 13 +++++++++++++
>> 1 file changed, 13 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index ad1300f366df..7d7caab8f573 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -1222,6 +1222,10 @@ int kvm_phys_addr_ioremap(struct kvm *kvm, phys_addr_t guest_ipa,
>> if (is_protected_kvm_enabled())
>> return -EPERM;
>>
>> + /* We don't support mapping special pages into a Realm */
>> + if (kvm_is_realm(kvm))
>> + return -EPERM;
>> +
>> size += offset_in_page(guest_ipa);
>> guest_ipa &= PAGE_MASK;
>>
>> @@ -1965,6 +1969,15 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> return 1;
>> }
>>
>> + /*
>> + * For now we shouldn't be hitting protected addresses because they are
>> + * handled in private_memslot_fault(). In the future this check may be
>> + * relaxed to support e.g. protected devices.
>> + */
>> + if (vcpu_is_rec(vcpu) &&
>> + kvm_gpa_from_fault(kvm, fault_ipa) == fault_ipa)
>> + return -EINVAL;
>> +
>> if (nested)
>> adjust_nested_fault_perms(nested, &prot, &writable);
>>
>> --
>> 2.43.0
>>
>>
^ permalink raw reply
* Re: [PATCH v13 32/48] arm64: Don't expose stolen time for realm guests
From: Steven Price @ 2026-04-10 15:12 UTC (permalink / raw)
To: Suzuki K Poulose, kvm, kvmarm
Cc: Catalin Marinas, Marc Zyngier, Will Deacon, James Morse,
Oliver Upton, Zenghui Yu, linux-arm-kernel, linux-kernel,
Joey Gouly, Alexandru Elisei, Christoffer Dall, Fuad Tabba,
linux-coco, Ganapatrao Kulkarni, Gavin Shan, Shanker Donthineni,
Alper Gun, Aneesh Kumar K . V, Emi Kisanuki, Vishal Annapurve
In-Reply-To: <4a2ab5b0-dec6-4ee2-8790-f27c2501f653@arm.com>
On 30/03/2026 11:52, Suzuki K Poulose wrote:
> On 18/03/2026 15:53, Steven Price wrote:
>> It doesn't make much sense as a realm guest wouldn't want to trust the
>> host. It will also need some extra work to ensure that KVM will only
>> attempt to write into a shared memory region. So for now just disable
>> it.
>>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Reviewed-by: Gavin Shan <gshan@redhat.com>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v7:
>> * Update the documentation to add a note about stolen time being
>> unavailable in a realm.
>> ---
>> Documentation/virt/kvm/api.rst | 3 +++
>> arch/arm64/kvm/arm.c | 5 ++++-
>> 2 files changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/
>> api.rst
>> index bc180c853faf..70911fe6d435 100644
>> --- a/Documentation/virt/kvm/api.rst
>> +++ b/Documentation/virt/kvm/api.rst
>> @@ -9240,6 +9240,9 @@ is supported, than the other should as well and
>> vice versa. For arm64
>> see Documentation/virt/kvm/devices/vcpu.rst "KVM_ARM_VCPU_PVTIME_CTRL".
>> For x86 see Documentation/virt/kvm/x86/msr.rst "MSR_KVM_STEAL_TIME".
>> +Note that steal time accounting is not available when a guest is
>> running
>> +within a Arm CCA realm (machine type KVM_VM_TYPE_ARM_REALM).
>> +
>> 8.25 KVM_CAP_S390_DIAG318
>> -------------------------
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 61182eb0cf70..7d92ddb06460 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -469,7 +469,10 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm,
>> long ext)
>> r = system_supports_mte();
>> break;
>> case KVM_CAP_STEAL_TIME:
>> - r = kvm_arm_pvtime_supported();
>> + if (kvm_is_realm(kvm))
>> + r = 0;
>> + else
>> + r = kvm_arm_pvtime_supported();
>
> Could this be handled in kvm_realm_ext_allowed() ?
Indeed it is already handled there. I'm not sure how I missed that, but
this patch is completely unnecessary now. Stolen time was an extension
that I knew about (having added it in the first place) and needed
disabling because it's implemented with the assumption that the host can
write into the guest.
In theory with some extra work it could be supported in a realm guest,
but it requires some extra plumbing to ensure the structures end up in
shared memory. My intention is that this can be revisited once the basic
CCA support is in.
Thanks,
Steve
> Suzuki
>
>
>> break;
>> case KVM_CAP_ARM_EL1_32BIT:
>> r = cpus_have_final_cap(ARM64_HAS_32BIT_EL1);
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox