* 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 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Huang, Kai @ 2026-04-13 11:24 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-6-seanjc@google.com>
On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> Convert regs_{avail,dirty} and all related masks to "unsigned long" values
> as an intermediate step towards declaring the fields as actual bitmaps, and
> as a step toward support APX, which will push the total number of registers
Nit: a step toward supporting APX.
^ permalink raw reply
* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Huang, Kai @ 2026-04-13 11:28 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-6-seanjc@google.com>
On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> -#define TDX_REGS_AVAIL_SET (BIT_ULL(VCPU_REG_EXIT_INFO_1) | \
> - BIT_ULL(VCPU_REG_EXIT_INFO_2) | \
> - BIT_ULL(VCPU_REGS_RAX) | \
> - BIT_ULL(VCPU_REGS_RBX) | \
> - BIT_ULL(VCPU_REGS_RCX) | \
> - BIT_ULL(VCPU_REGS_RDX) | \
> - BIT_ULL(VCPU_REGS_RBP) | \
> - BIT_ULL(VCPU_REGS_RSI) | \
> - BIT_ULL(VCPU_REGS_RDI) | \
> - BIT_ULL(VCPU_REGS_R8) | \
> - BIT_ULL(VCPU_REGS_R9) | \
> - BIT_ULL(VCPU_REGS_R10) | \
> - BIT_ULL(VCPU_REGS_R11) | \
> - BIT_ULL(VCPU_REGS_R12) | \
> - BIT_ULL(VCPU_REGS_R13) | \
> - BIT_ULL(VCPU_REGS_R14) | \
> - BIT_ULL(VCPU_REGS_R15))
> +#define TDX_REGS_AVAIL_SET (BIT(VCPU_REG_EXIT_INFO_1) | \
> + BIT(VCPU_REG_EXIT_INFO_2) | \
> + BIT(VCPU_REGS_RAX) | \
> + BIT(VCPU_REGS_RBX) | \
> + BIT(VCPU_REGS_RCX) | \
> + BIT(VCPU_REGS_RDX) | \
> + BIT(VCPU_REGS_RBP) | \
> + BIT(VCPU_REGS_RSI) | \
> + BIT(VCPU_REGS_RDI) | \
> + BIT(VCPU_REGS_R8) | \
> + BIT(VCPU_REGS_R9) | \
> + BIT(VCPU_REGS_R10) | \
> + BIT(VCPU_REGS_R11) | \
> + BIT(VCPU_REGS_R12) | \
> + BIT(VCPU_REGS_R13) | \
> + BIT(VCPU_REGS_R14) | \
> + BIT(VCPU_REGS_R15))
>
Not related to this series, but this made me look into whether these
registers are truly needed to be set as available for TDX.
Firstly, all the listed registers are marked as available immediately after
exiting from tdh_vp_enter(), but except VCPU_REG_EXIT_INFO_1 and
VCPU_REG_EXIT_INFO_2 are immediately saved to the common 'struct vcpu_vt',
all other GPRs are not saved to vcpu->arch.regs[], which means marking GPRs
available immediately doesn't quite make sense.
In fact, IIUC other than when the TD exits with TDVMCALL on which TD shares
couple of GPRs with KVM, KVM has no way to get TD's GPRs. So perhaps it
makes more sense is to mark the shared GPRs available upon TDVMCALL.
But even that does not make sense from KVM's "GPR available" perspective,
because TDVMCALL has a different ABI from KVM's existing infrastructure for
e.g., CPUID/MSR emulation. E.g., KVM uses RCX/RAX/RDX for MSR emulation,
but TDVMCALL<MSR.WRITE> uses R12 and R13 to convey MSR index/value:
case EXIT_REASON_MSR_WRITE:
kvm_rcx_write(vcpu, tdx->vp_enter_args.r12);
kvm_rax_write(vcpu, tdx->vp_enter_args.r13 & -1u);
kvm_rdx_write(vcpu, tdx->vp_enter_args.r13 >> 32);
So I think the most accurate way is to explicitly mark the relevant GPRs
available for each type of TDVMCALL. I am not sure whether it's worth to do
though, because AFAICT there's no real bug in the existing code, other than
"marking GPRs not in vcpu->arch.regs[] as available looks wrong".
A less invasive way is to mark all possible GPRs that can be used in
TDVMCALL emulation available once after TD exits. AFAICT the KVM hypercall
uses most GPRs (RAX/RBX/RCX/RDX/RSI) and all other TDVMCALLs only use a
subset, so maybe we can remove other GPRs from the available list (the diff
in [*] passed my test of booting/destroying TD).
Bug again, not sure whether it's worth doing.
[*]:
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 85f28363e4cc..7b4c182c22cf 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1019,17 +1019,7 @@ static fastpath_t tdx_exit_handlers_fastpath(struct
kvm_vcpu *vcpu)
BIT(VCPU_REGS_RBX) | \
BIT(VCPU_REGS_RCX) | \
BIT(VCPU_REGS_RDX) | \
- BIT(VCPU_REGS_RBP) | \
- BIT(VCPU_REGS_RSI) | \
- BIT(VCPU_REGS_RDI) | \
- BIT(VCPU_REGS_R8) | \
- BIT(VCPU_REGS_R9) | \
- BIT(VCPU_REGS_R10) | \
- BIT(VCPU_REGS_R11) | \
- BIT(VCPU_REGS_R12) | \
- BIT(VCPU_REGS_R13) | \
- BIT(VCPU_REGS_R14) | \
- BIT(VCPU_REGS_R15))
+ BIT(VCPU_REGS_RSI))
^ permalink raw reply related
* Re: [PATCH v2 0/6] KVM: x86: Reg cleanups / prep work for APX
From: Huang, Kai @ 2026-04-13 11:31 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-1-seanjc@google.com>
On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> Clean up KVM's register tracking and storage, primarily to prepare for landing
> APX, which expands the maximum number of GPRs from 16 to 32.
>
Tested booting/destroying both normal VMX guest and TD worked fine:
Reviewed-by: Kai Huang <kai.huang@intel.com>
Tested-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply
* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Sean Christopherson @ 2026-04-13 14:54 UTC (permalink / raw)
To: Kai Huang
Cc: pbonzini@redhat.com, kas@kernel.org, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Chang Seok Bae,
linux-kernel@vger.kernel.org, x86@kernel.org
In-Reply-To: <e6e82905e6b9b1637ccb640097e67d21793f5895.camel@intel.com>
On Mon, Apr 13, 2026, Kai Huang wrote:
> On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> > -#define TDX_REGS_AVAIL_SET (BIT_ULL(VCPU_REG_EXIT_INFO_1) | \
> > - BIT_ULL(VCPU_REG_EXIT_INFO_2) | \
> > - BIT_ULL(VCPU_REGS_RAX) | \
> > - BIT_ULL(VCPU_REGS_RBX) | \
> > - BIT_ULL(VCPU_REGS_RCX) | \
> > - BIT_ULL(VCPU_REGS_RDX) | \
> > - BIT_ULL(VCPU_REGS_RBP) | \
> > - BIT_ULL(VCPU_REGS_RSI) | \
> > - BIT_ULL(VCPU_REGS_RDI) | \
> > - BIT_ULL(VCPU_REGS_R8) | \
> > - BIT_ULL(VCPU_REGS_R9) | \
> > - BIT_ULL(VCPU_REGS_R10) | \
> > - BIT_ULL(VCPU_REGS_R11) | \
> > - BIT_ULL(VCPU_REGS_R12) | \
> > - BIT_ULL(VCPU_REGS_R13) | \
> > - BIT_ULL(VCPU_REGS_R14) | \
> > - BIT_ULL(VCPU_REGS_R15))
> > +#define TDX_REGS_AVAIL_SET (BIT(VCPU_REG_EXIT_INFO_1) | \
> > + BIT(VCPU_REG_EXIT_INFO_2) | \
> > + BIT(VCPU_REGS_RAX) | \
> > + BIT(VCPU_REGS_RBX) | \
> > + BIT(VCPU_REGS_RCX) | \
> > + BIT(VCPU_REGS_RDX) | \
> > + BIT(VCPU_REGS_RBP) | \
> > + BIT(VCPU_REGS_RSI) | \
> > + BIT(VCPU_REGS_RDI) | \
> > + BIT(VCPU_REGS_R8) | \
> > + BIT(VCPU_REGS_R9) | \
> > + BIT(VCPU_REGS_R10) | \
> > + BIT(VCPU_REGS_R11) | \
> > + BIT(VCPU_REGS_R12) | \
> > + BIT(VCPU_REGS_R13) | \
> > + BIT(VCPU_REGS_R14) | \
> > + BIT(VCPU_REGS_R15))
> >
>
> Not related to this series, but this made me look into whether these
> registers are truly needed to be set as available for TDX.
>
> Firstly, all the listed registers are marked as available immediately after
> exiting from tdh_vp_enter(), but except VCPU_REG_EXIT_INFO_1 and
> VCPU_REG_EXIT_INFO_2 are immediately saved to the common 'struct vcpu_vt',
> all other GPRs are not saved to vcpu->arch.regs[], which means marking GPRs
> available immediately doesn't quite make sense.
>
> In fact, IIUC other than when the TD exits with TDVMCALL on which TD shares
> couple of GPRs with KVM, KVM has no way to get TD's GPRs. So perhaps it
> makes more sense is to mark the shared GPRs available upon TDVMCALL.
>
> But even that does not make sense from KVM's "GPR available" perspective,
> because TDVMCALL has a different ABI from KVM's existing infrastructure for
> e.g., CPUID/MSR emulation. E.g., KVM uses RCX/RAX/RDX for MSR emulation,
> but TDVMCALL<MSR.WRITE> uses R12 and R13 to convey MSR index/value:
>
> case EXIT_REASON_MSR_WRITE:
> kvm_rcx_write(vcpu, tdx->vp_enter_args.r12);
> kvm_rax_write(vcpu, tdx->vp_enter_args.r13 & -1u);
> kvm_rdx_write(vcpu, tdx->vp_enter_args.r13 >> 32);
>
> So I think the most accurate way is to explicitly mark the relevant GPRs
> available for each type of TDVMCALL. I am not sure whether it's worth to do
> though, because AFAICT there's no real bug in the existing code, other than
> "marking GPRs not in vcpu->arch.regs[] as available looks wrong".
>
> A less invasive way is to mark all possible GPRs that can be used in
> TDVMCALL emulation available once after TD exits. AFAICT the KVM hypercall
> uses most GPRs (RAX/RBX/RCX/RDX/RSI) and all other TDVMCALLs only use a
> subset, so maybe we can remove other GPRs from the available list (the diff
> in [*] passed my test of booting/destroying TD).
>
> Bug again, not sure whether it's worth doing.
Not worth doing. Because VMX and SVM make all GRPs available immediately, except
for RSP, KVM ignores avail/dirty for GPRs. I.e. "fixing" TDX will just shift the
"bugs" elsewhere.
More importantly, because the TDX-Module *requires* RCX (the GPR that holds the
mask of registers to expose to the VMM) to be hidden on TDVMCALL, KVM *can't*
do any kind of meaningful "available" tracking. Versus sev_es_validate_vmgexit(),
which can at least sanity check that the registers needed to service a hypercall
have valid data.
So unfortunately, since we need to rely on testing to verify KVM's implementation
no matter what, I don't think it'd be a net positive to overhaul KVM's handling
of GPRs to support SEV-ES+'s and TDX's "sometimes available" GPR set.
^ permalink raw reply
* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Edgecombe, Rick P @ 2026-04-13 19:08 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-7-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> TDX module updates require userspace to select the appropriate module
> to load. Expose necessary information to facilitate this decision. Two
> values are needed:
>
> - P-SEAMLDR version: for compatibility checks between TDX module and
> P-SEAMLDR
> - num_remaining_updates: indicates how many updates can be performed
Can you explain how all of these overlap?
- TDX module supports module update
- SEAMLDR supports NUM_REMAINING_UPDATES info
- SEAMLDR supports VERSION info
If the TDX module supports module update, do we know the SEAMLDR supports this
other stuff somehow? It might be worth a comment the reasoning.
>
> Expose them as tdx-host device attributes. Make seamldr attributes
> visible only when the update feature is supported, as that's their sole
> purpose.
>
> Unconditional exposure is also problematic because reading them
> triggers P-SEAMLDR calls that break KVM on CPUs with a specific erratum
> (to be enumerated and handled in a later patch).
Since this is later handled with the errata check, what is the point being made
here?
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
^ permalink raw reply
* Re: [PATCH v7 18/22] coco/tdx-host: Don't expose P-SEAMLDR features on CPUs with erratum
From: Edgecombe, Rick P @ 2026-04-13 19:28 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-19-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> Some TDX-capable CPUs have an erratum, as documented in Intel® Trust
> Domain CPU Architectural Extensions (May 2021 edition) Chapter 2.3:
>
> SEAMRET from the P-SEAMLDR clears the current VMCS structure pointed
> to by the current-VMCS pointer. A VMM that invokes the P-SEAMLDR using
> SEAMCALL must reload the current-VMCS, if required, using the VMPTRLD
> instruction.
>
> Clearing the current VMCS behind KVM's back will break KVM.
>
> This erratum is not present when IA32_VMX_BASIC[60] is set. Add a CPU
> bug bit for this erratum and refuse to expose P-SEAMLDR features (e.g.,
> TDX module updates) on affected CPUs.
>
> == Alternatives ==
> Two workarounds were considered but both were rejected:
>
> 1. Save/restore the current VMCS around P-SEAMLDR calls. This produces ugly
> assembly code [1] and doesn't play well with #MCE or #NMI if they
> need to use the current VMCS.
>
> 2. Move KVM's VMCS tracking logic to the TDX core code, which would break
> the boundary between KVM and the TDX core code [2].
>
> 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>
> Link: https://lore.kernel.org/kvm/fedb3192-e68c-423c-93b2-a4dc2f964148@intel.com/ # [1]
> Link: https://lore.kernel.org/kvm/aYIXFmT-676oN6j0@google.com/ # [2]
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/vmx.h | 1 +
> arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
> drivers/virt/coco/tdx-host/tdx-host.c | 8 ++++++++
> 4 files changed, 21 insertions(+)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index dbe104df339b..377d009b7e2e 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -572,4 +572,5 @@
> #define X86_BUG_ITS_NATIVE_ONLY X86_BUG( 1*32+ 8) /* "its_native_only" CPU is affected by ITS, VMX is not affected */
> #define X86_BUG_TSA X86_BUG( 1*32+ 9) /* "tsa" CPU is affected by Transient Scheduler Attacks */
> #define X86_BUG_VMSCAPE X86_BUG( 1*32+10) /* "vmscape" CPU is affected by VMSCAPE attacks from guests */
> +#define X86_BUG_SEAMRET_INVD_VMCS X86_BUG( 1*32+11) /* "seamret_invd_vmcs" SEAMRET from P-SEAMLDR clears the current VMCS */
> #endif /* _ASM_X86_CPUFEATURES_H */
> diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
> index b92ff87e3560..a5a5b373ec42 100644
> --- a/arch/x86/include/asm/vmx.h
> +++ b/arch/x86/include/asm/vmx.h
> @@ -136,6 +136,7 @@
> #define VMX_BASIC_INOUT BIT_ULL(54)
> #define VMX_BASIC_TRUE_CTLS BIT_ULL(55)
> #define VMX_BASIC_NO_HW_ERROR_CODE_CC BIT_ULL(56)
> +#define VMX_BASIC_NO_SEAMRET_INVD_VMCS BIT_ULL(60)
>
> static inline u32 vmx_basic_vmcs_revision_id(u64 vmx_basic)
> {
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index d144860e17c2..92ab1d98e1b8 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -39,6 +39,7 @@
> #include <asm/cpu_device_id.h>
> #include <asm/processor.h>
> #include <asm/mce.h>
> +#include <asm/vmx.h>
>
> #include "seamcall_internal.h"
> #include "tdx.h"
> @@ -1462,6 +1463,8 @@ static struct notifier_block tdx_memory_nb = {
>
> static void __init check_tdx_erratum(void)
> {
> + u64 basic_msr;
> +
> /*
> * These CPUs have an erratum. A partial write from non-TD
> * software (e.g. via MOVNTI variants or UC/WC mapping) to TDX
> @@ -1473,6 +1476,14 @@ static void __init check_tdx_erratum(void)
> case INTEL_EMERALDRAPIDS_X:
> setup_force_cpu_bug(X86_BUG_TDX_PW_MCE);
> }
> +
> + /*
> + * Some TDX-capable CPUs have an erratum where the current VMCS is
> + * cleared after calling into P-SEAMLDR.
> + */
> + rdmsrq(MSR_IA32_VMX_BASIC, basic_msr);
> + if (!(basic_msr & VMX_BASIC_NO_SEAMRET_INVD_VMCS))
> + setup_force_cpu_bug(X86_BUG_SEAMRET_INVD_VMCS);
> }
>
> void __init tdx_init(void)
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 746a5eef004d..71ea94da8e22 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -100,6 +100,14 @@ static bool can_expose_seamldr(void)
> if (!sysinfo)
> return false;
>
> + /*
> + * Calling P-SEAMLDR on CPUs with the seamret_invd_vmcs bug clears
> + * the current VMCS, which breaks KVM. Verify the erratum is not
> + * present before exposing P-SEAMLDR features.
> + */
> + if (boot_cpu_has_bug(X86_BUG_SEAMRET_INVD_VMCS))
> + return false;
> +
> return tdx_supports_runtime_update(sysinfo);
> }
>
^ permalink raw reply
* Re: [PATCH v7 19/22] x86/virt/tdx: Enable TDX module runtime updates
From: Edgecombe, Rick P @ 2026-04-13 19:40 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-20-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> All pieces of TDX module runtime updates are in place. Enable it if it
> is supported.
>
> 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: Kiryl Shutsemau (Meta) <kas@kernel.org>
> ---
> arch/x86/include/asm/tdx.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* [PATCH v4 0/7] Add RMPOPT support.
From: Ashish Kalra @ 2026-04-13 19:42 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
From: Ashish Kalra <ashish.kalra@amd.com>
In the SEV-SNP architecture, hypervisor and non-SNP guests are subject
to RMP checks on writes to provide integrity of SEV-SNP guest memory.
The RMPOPT architecture enables optimizations whereby the RMP checks
can be skipped if 1GB regions of memory are known to not contain any
SNP guest memory.
RMPOPT is a new instruction designed to minimize the performance
overhead of RMP checks for the hypervisor and non-SNP guests.
RMPOPT instruction currently supports two functions. In case of the
verify and report status function the CPU will read the RMP contents,
verify the entire 1GB region starting at the provided SPA is HV-owned.
For the entire 1GB region it checks that all RMP entries in this region
are HV-owned (i.e, not in assigned state) and then accordingly updates
the RMPOPT table to indicate if optimization has been enabled and
provide indication to software if the optimization was successful.
In case of report status function, the CPU returns the optimization
status for the 1GB region.
The RMPOPT table is managed by a combination of software and hardware.
Software uses the RMPOPT instruction to set bits in the table,
indicating that regions of memory are entirely HV-owned. Hardware
automatically clears bits in the RMPOPT table when RMP contents are
changed during RMPUPDATE instruction.
For more information on the RMPOPT instruction, see the AMD64 RMPOPT
technical documentation.
As SNP is enabled by default the hypervisor and non-SNP guests are
subject to RMP write checks to provide integrity of SNP guest memory.
This patch-series adds support to enable RMP optimizations for up to
2TB of system RAM across the system and allow RMPUPDATE to disable
those optimizations as SNP guests are launched.
Support for RAM larger than 2 TB will be added in follow-on series.
This series also introduces support to re-enable RMP optimizations
during SNP guest termination, after guest pages have been converted
back to shared.
RMP optimizations are performed asynchronously by queuing work on a
dedicated workqueue after a 10 second delay.
Delaying work allows batching of multiple SNP guest terminations.
Once 1GB hugetlb guest_memfd support is merged, support for
re-enabling RMPOPT optimizations during 1GB page cleanup will be added
in follow-on series.
Additionally add debugfs interface to report per-CPU RMPOPT status
across all system RAM.
v4:
- Add new wrmsrq_on_cpus() helper to write same u64 value to a
per-CPU MSR across a cpumask without per-cpu struct allocation
overhead.
- Rename configure_and_enable_rmpopt() to snp_setup_rmpopt().
- Use wrmsrq_on_cpus() instead of wrmsrq_on_cpu() loop for
programming RMPOPT_BASE MSRs.
- Add setup_clear_cpu_cap(X86_FEATURE_RMPOPT) if segmented RMP
setup fails or workqueue allocation fails.
- Add X86_FEATURE_RMPOPT feature clear logic in amd_cc_platform_clear()
for CC_ATTR_HOST_SEV_SNP.
- All of the above allow checking for only X86_FEATURE_RMPOPT for both
RMPOPT setup/enable and RMP re-optimizations.
- Rename snp_perform_rmp_optimization() to snp_rmpopt_all_physmem().
- Split rmpopt() into rmpopt() and rmpopt_smp() for SMP callback use.
- Introduce separate rmpopt_report_cpumask for debugfs reporting,
distinct from rmpopt_cpumask used for primary thread tracking.
- Remove snp_perform_rmp_optimization() call from __sev_snp_init_locked()
and instead setup and enable RMPOPT after SNP is enabled and
initialized.
v3:
- Drop all RMPOPT kthread support and introduce adding custom and
dedicated workqueue to schedule delayed and asynchronous RMPOPT work.
- Drop the guest_memfd inode cleanup interface and add support to
re-enable RMP optimizations during guest shutdown using the
asynchronous and delayed workqueue interface.
- Introduce new __rmpopt() helper and rmpopt() and
rmpopt_report_status() wrappers on top which use rax and rcx
parameters to closely match RMPOPT specs.
- Use new optimized RMPOPT loop to issue RMPOPT instructions on all
system RAM upto 2TB and all CPUs, by optimizing each range on one CPU
first, then let other CPUs execute RMPOPT in parallel so they can skip
most work as the range has already been optimized.
- Also add support for running the optimized RMPOPT loop only on
one thread per core.
- Replace all PUD_SIZE references with SZ_1G to conform to 1GB regions
as specified by RMPOPT specifications and not be dependent on PUD_SIZE
which makes the RMPOPT patch-set independent of x86 page table sizes.
- Use wrmsrq_on_cpu() to program the RMPOPT_BASE MSR registers on
all CPUs that removes all ugly casting to use on_each_cpu_mask().
- Fix inline commits and patch commit messages
v2:
- Drop all NUMA and Socket configuration and enablement support and
enable RMPOPT support for up to 2TB of system RAM.
- Drop get_cpumask_of_primary_threads() and enable per-core RMPOPT
base MSRs and issue RMPOPT instruction on all CPUs.
- Drop the configfs interface to manually re-enable RMP optimizations.
- Add new guest_memfd cleanup interface to automatically re-enable
RMP optimizations during guest shutdown.
- Include references to the public RMPOPT documentation.
- Move debugfs directory for RMPOPT under architecuture specific
parent directory.
Ashish Kalra (7):
x86/cpufeatures: Add X86_FEATURE_AMD_RMPOPT feature flag
x86/msr: add wrmsrq_on_cpus helper
x86/sev: Initialize RMPOPT configuration MSRs
x86/sev: Add support to perform RMP optimizations asynchronously
x86/sev: Add interface to re-enable RMP optimizations.
KVM: SEV: Perform RMP optimizations on SNP guest shutdown
x86/sev: Add debugfs support for RMPOPT
arch/x86/coco/core.c | 1 +
arch/x86/include/asm/cpufeatures.h | 2 +-
arch/x86/include/asm/msr-index.h | 3 +
arch/x86/include/asm/msr.h | 5 +
arch/x86/include/asm/sev.h | 4 +
arch/x86/kernel/cpu/scattered.c | 1 +
arch/x86/kvm/svm/sev.c | 2 +
arch/x86/lib/msr-smp.c | 20 +++
arch/x86/virt/svm/sev.c | 271 ++++++++++++++++++++++++++++-
drivers/crypto/ccp/sev-dev.c | 3 +
10 files changed, 310 insertions(+), 2 deletions(-)
--
2.43.0
^ permalink raw reply
* [PATCH v4 1/7] x86/cpufeatures: Add X86_FEATURE_AMD_RMPOPT feature flag
From: Ashish Kalra @ 2026-04-13 19:42 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
Add a flag indicating whether RMPOPT instruction is supported.
RMPOPT is a new instruction designed to minimize the performance
overhead of RMP checks on the hypervisor and on non-SNP guests by
allowing RMP checks to be skipped when 1G regions of memory are known
not to contain any SEV-SNP guest memory.
For more information on the RMPOPT instruction, see the AMD64 RMPOPT
technical documentation.
Reviewed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/include/asm/cpufeatures.h | 2 +-
arch/x86/kernel/cpu/scattered.c | 1 +
2 files changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index dbe104df339b..bce1b2e2a35c 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -76,7 +76,7 @@
#define X86_FEATURE_K8 ( 3*32+ 4) /* Opteron, Athlon64 */
#define X86_FEATURE_ZEN5 ( 3*32+ 5) /* CPU based on Zen5 microarchitecture */
#define X86_FEATURE_ZEN6 ( 3*32+ 6) /* CPU based on Zen6 microarchitecture */
-/* Free ( 3*32+ 7) */
+#define X86_FEATURE_RMPOPT ( 3*32+ 7) /* Support for AMD RMPOPT instruction */
#define X86_FEATURE_CONSTANT_TSC ( 3*32+ 8) /* "constant_tsc" TSC ticks at a constant rate */
#define X86_FEATURE_UP ( 3*32+ 9) /* "up" SMP kernel running on UP */
#define X86_FEATURE_ART ( 3*32+10) /* "art" Always running timer (ART) */
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 42c7eac0c387..7ac3818c4502 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -65,6 +65,7 @@ static const struct cpuid_bit cpuid_bits[] = {
{ X86_FEATURE_PERFMON_V2, CPUID_EAX, 0, 0x80000022, 0 },
{ X86_FEATURE_AMD_LBR_V2, CPUID_EAX, 1, 0x80000022, 0 },
{ X86_FEATURE_AMD_LBR_PMC_FREEZE, CPUID_EAX, 2, 0x80000022, 0 },
+ { X86_FEATURE_RMPOPT, CPUID_EDX, 0, 0x80000025, 0 },
{ X86_FEATURE_AMD_HTR_CORES, CPUID_EAX, 30, 0x80000026, 0 },
{ 0, 0, 0, 0, 0 }
};
--
2.43.0
^ permalink raw reply related
* [PATCH v4 2/7] x86/msr: add wrmsrq_on_cpus helper
From: Ashish Kalra @ 2026-04-13 19:42 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
The existing wrmsr_on_cpus() takes a per-cpu struct msr array, requiring
callers to allocate and populate per-cpu storage even when every CPU
receives the same value. This is unnecessary overhead for the common
case of writing a single uniform u64 to a per-CPU MSR across multiple
CPUs.
Add wrmsrq_on_cpus() which writes the same u64 value to the specified
MSR on all CPUs in the given cpumask.
Co-developed-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/include/asm/msr.h | 5 +++++
arch/x86/lib/msr-smp.c | 20 ++++++++++++++++++++
2 files changed, 25 insertions(+)
diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 9c2ea29e12a9..f5f63b4115c8 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -260,6 +260,7 @@ int rdmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
int wrmsr_on_cpu(unsigned int cpu, u32 msr_no, u32 l, u32 h);
int rdmsrq_on_cpu(unsigned int cpu, u32 msr_no, u64 *q);
int wrmsrq_on_cpu(unsigned int cpu, u32 msr_no, u64 q);
+void wrmsrq_on_cpus(const struct cpumask *mask, u32 msr_no, u64 q);
void rdmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs);
void wrmsr_on_cpus(const struct cpumask *mask, u32 msr_no, struct msr __percpu *msrs);
int rdmsr_safe_on_cpu(unsigned int cpu, u32 msr_no, u32 *l, u32 *h);
@@ -289,6 +290,10 @@ static inline int wrmsrq_on_cpu(unsigned int cpu, u32 msr_no, u64 q)
wrmsrq(msr_no, q);
return 0;
}
+static inline void wrmsrq_on_cpus(const struct cpumask *mask, u32 msr_no, u64 q)
+{
+ wrmsrq_on_cpu(0, msr_no, q);
+}
static inline void rdmsr_on_cpus(const struct cpumask *m, u32 msr_no,
struct msr __percpu *msrs)
{
diff --git a/arch/x86/lib/msr-smp.c b/arch/x86/lib/msr-smp.c
index b8f63419e6ae..d2c91c9bb47b 100644
--- a/arch/x86/lib/msr-smp.c
+++ b/arch/x86/lib/msr-smp.c
@@ -94,6 +94,26 @@ int wrmsrq_on_cpu(unsigned int cpu, u32 msr_no, u64 q)
}
EXPORT_SYMBOL(wrmsrq_on_cpu);
+void wrmsrq_on_cpus(const struct cpumask *mask, u32 msr_no, u64 q)
+{
+ struct msr_info rv;
+ int this_cpu;
+
+ memset(&rv, 0, sizeof(rv));
+
+ rv.msr_no = msr_no;
+ rv.reg.q = q;
+
+ this_cpu = get_cpu();
+
+ if (cpumask_test_cpu(this_cpu, mask))
+ __wrmsr_on_cpu(&rv);
+
+ smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
+ put_cpu();
+}
+EXPORT_SYMBOL(wrmsrq_on_cpus);
+
static void __rwmsr_on_cpus(const struct cpumask *mask, u32 msr_no,
struct msr __percpu *msrs,
void (*msr_func) (void *info))
--
2.43.0
^ permalink raw reply related
* [PATCH v4 3/7] x86/sev: Initialize RMPOPT configuration MSRs
From: Ashish Kalra @ 2026-04-13 19:43 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
The new RMPOPT instruction helps manage per-CPU RMP optimization
structures inside the CPU. It takes a 1GB-aligned physical address
and either returns the status of the optimizations or tries to enable
the optimizations.
Per-CPU RMPOPT tables support at most 2 TB of addressable memory for
RMP optimizations.
Initialize the per-CPU RMPOPT table base to the starting physical
address. This enables RMP optimization for up to 2 TB of system RAM on
all CPUs.
Additionally, add support to setup and enable RMPOPT once SNP is
enabled and initialized.
Suggested-by: Thomas Lendacky <thomas.lendacky@amd.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/coco/core.c | 1 +
arch/x86/include/asm/msr-index.h | 3 +++
arch/x86/include/asm/sev.h | 2 ++
arch/x86/virt/svm/sev.c | 41 +++++++++++++++++++++++++++++++-
drivers/crypto/ccp/sev-dev.c | 3 +++
5 files changed, 49 insertions(+), 1 deletion(-)
diff --git a/arch/x86/coco/core.c b/arch/x86/coco/core.c
index 989ca9f72ba3..7fdef00ca8f2 100644
--- a/arch/x86/coco/core.c
+++ b/arch/x86/coco/core.c
@@ -172,6 +172,7 @@ static void amd_cc_platform_clear(enum cc_attr attr)
switch (attr) {
case CC_ATTR_HOST_SEV_SNP:
cc_flags.host_sev_snp = 0;
+ setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
break;
default:
break;
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index be3e3cc963b2..9c8a6dfd7891 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -758,6 +758,9 @@
#define MSR_AMD64_SEG_RMP_ENABLED_BIT 0
#define MSR_AMD64_SEG_RMP_ENABLED BIT_ULL(MSR_AMD64_SEG_RMP_ENABLED_BIT)
#define MSR_AMD64_RMP_SEGMENT_SHIFT(x) (((x) & GENMASK_ULL(13, 8)) >> 8)
+#define MSR_AMD64_RMPOPT_BASE 0xc0010139
+#define MSR_AMD64_RMPOPT_ENABLE_BIT 0
+#define MSR_AMD64_RMPOPT_ENABLE BIT_ULL(MSR_AMD64_RMPOPT_ENABLE_BIT)
#define MSR_SVSM_CAA 0xc001f000
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 09e605c85de4..409ab3372f7c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -662,6 +662,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
__snp_leak_pages(pfn, pages, true);
}
void snp_prepare(void);
+void snp_setup_rmpopt(void);
void snp_shutdown(void);
#else
static inline bool snp_probe_rmptable_info(void) { return false; }
@@ -680,6 +681,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
static inline void kdump_sev_callback(void) { }
static inline void snp_fixup_e820_tables(void) {}
static inline void snp_prepare(void) {}
+static inline void snp_setup_rmpopt(void) {}
static inline void snp_shutdown(void) {}
#endif
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 41f76f15caa1..4f942abaf86e 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -124,6 +124,9 @@ static void *rmp_bookkeeping __ro_after_init;
static u64 probed_rmp_base, probed_rmp_size;
+static cpumask_t rmpopt_cpumask;
+static phys_addr_t rmpopt_pa_start;
+
static LIST_HEAD(snp_leaked_pages_list);
static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
@@ -488,9 +491,12 @@ static bool __init setup_segmented_rmptable(void)
static bool __init setup_rmptable(void)
{
if (rmp_cfg & MSR_AMD64_SEG_RMP_ENABLED) {
- if (!setup_segmented_rmptable())
+ if (!setup_segmented_rmptable()) {
+ setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
return false;
+ }
} else {
+ setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
if (!setup_contiguous_rmptable())
return false;
}
@@ -554,6 +560,39 @@ void snp_shutdown(void)
}
EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
+void snp_setup_rmpopt(void)
+{
+ u64 rmpopt_base;
+ int cpu;
+
+ if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
+ return;
+
+ /*
+ * RMPOPT_BASE MSR is per-core, so only one thread per core needs to
+ * setup RMPOPT_BASE MSR.
+ */
+
+ for_each_online_cpu(cpu) {
+ if (!topology_is_primary_thread(cpu))
+ continue;
+
+ cpumask_set_cpu(cpu, &rmpopt_cpumask);
+ }
+
+ rmpopt_pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
+ rmpopt_base = rmpopt_pa_start | MSR_AMD64_RMPOPT_ENABLE;
+
+ /*
+ * Per-CPU RMPOPT tables support at most 2 TB of addressable memory
+ * for RMP optimizations. Initialize the per-CPU RMPOPT table base
+ * to the starting physical address to enable RMP optimizations for
+ * up to 2 TB of system RAM on all CPUs.
+ */
+ wrmsrq_on_cpus(&rmpopt_cpumask, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+}
+EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
+
/*
* Do the necessary preparations which are verified by the firmware as
* described in the SNP_INIT_EX firmware command description in the SNP
diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
index 939fa8aa155c..901395ad7d51 100644
--- a/drivers/crypto/ccp/sev-dev.c
+++ b/drivers/crypto/ccp/sev-dev.c
@@ -1476,6 +1476,9 @@ static int __sev_snp_init_locked(int *error, unsigned int max_snp_asid)
}
snp_hv_fixed_pages_state_update(sev, HV_FIXED);
+
+ snp_setup_rmpopt();
+
sev->snp_initialized = true;
dev_dbg(sev->dev, "SEV-SNP firmware initialized, SEV-TIO is %s\n",
data.tio_en ? "enabled" : "disabled");
--
2.43.0
^ permalink raw reply related
* [PATCH v4 4/7] x86/sev: Add support to perform RMP optimizations asynchronously
From: Ashish Kalra @ 2026-04-13 19:43 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
When SEV-SNP is enabled, all writes to memory are checked to ensure
integrity of SNP guest memory. This imposes performance overhead on the
whole system.
RMPOPT is a new instruction that minimizes the performance overhead of
RMP checks on the hypervisor and on non-SNP guests by allowing RMP
checks to be skipped for 1GB regions of memory that are known not to
contain any SEV-SNP guest memory.
Add support for performing RMP optimizations asynchronously using a
dedicated workqueue.
Enable RMPOPT optimizations globally for all system RAM up to 2TB at
RMP initialization time. RMP checks can initially be skipped for 1GB
memory ranges that do not contain SEV-SNP guest memory (excluding
preassigned pages such as the RMP table and firmware pages). As SNP
guests are launched, RMPUPDATE will disable the corresponding RMPOPT
optimizations.
Suggested-by: Thomas Lendacky <thomas.lendacky@amd.com>
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/virt/svm/sev.c | 117 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 115 insertions(+), 2 deletions(-)
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 4f942abaf86e..56c9fc3fe53a 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -19,6 +19,7 @@
#include <linux/iommu.h>
#include <linux/amd-iommu.h>
#include <linux/nospec.h>
+#include <linux/workqueue.h>
#include <asm/sev.h>
#include <asm/processor.h>
@@ -125,7 +126,17 @@ static void *rmp_bookkeeping __ro_after_init;
static u64 probed_rmp_base, probed_rmp_size;
static cpumask_t rmpopt_cpumask;
-static phys_addr_t rmpopt_pa_start;
+static phys_addr_t rmpopt_pa_start, rmpopt_pa_end;
+
+enum rmpopt_function {
+ RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS,
+ RMPOPT_FUNC_REPORT_STATUS
+};
+
+#define RMPOPT_WORK_TIMEOUT 10000
+
+static struct workqueue_struct *rmpopt_wq;
+static struct delayed_work rmpopt_delayed_work;
static LIST_HEAD(snp_leaked_pages_list);
static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
@@ -560,6 +571,83 @@ void snp_shutdown(void)
}
EXPORT_SYMBOL_FOR_MODULES(snp_shutdown, "ccp");
+static inline bool __rmpopt(u64 rax, u64 rcx)
+{
+ bool optimized;
+
+ asm volatile(".byte 0xf2, 0x0f, 0x01, 0xfc"
+ : "=@ccc" (optimized)
+ : "a" (rax), "c" (rcx)
+ : "memory", "cc");
+
+ return optimized;
+}
+
+/*
+ * 'val' is a system physical address.
+ */
+static void rmpopt_smp(void *val)
+{
+ u64 rax = ALIGN_DOWN((u64)val, SZ_1G);
+ u64 rcx = RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS;
+
+ __rmpopt(rax, rcx);
+}
+
+static void rmpopt(u64 pa)
+{
+ u64 rax = ALIGN_DOWN(pa, SZ_1G);
+ u64 rcx = RMPOPT_FUNC_VERIFY_AND_REPORT_STATUS;
+
+ __rmpopt(rax, rcx);
+}
+
+/*
+ * RMPOPT optimizations skip RMP checks at 1GB granularity if this
+ * range of memory does not contain any SNP guest memory.
+ */
+static void rmpopt_work_handler(struct work_struct *work)
+{
+ bool current_cpu_cleared = false;
+ phys_addr_t pa;
+
+ pr_info("Attempt RMP optimizations on physical address range @1GB alignment [0x%016llx - 0x%016llx]\n",
+ rmpopt_pa_start, rmpopt_pa_end);
+
+ /*
+ * RMPOPT scans the RMP table, stores the result of the scan in the
+ * reserved processor memory. The RMP scan is the most expensive
+ * part. If a second RMPOPT occurs, it can skip the expensive scan
+ * if they can see a cached result in the reserved processor memory.
+ *
+ * Do RMPOPT on one CPU alone. Then, follow that up with RMPOPT
+ * on every other primary thread. This potentially allows the
+ * followers to use the "cached" scan results to avoid repeating
+ * full scans.
+ */
+
+ if (cpumask_test_cpu(smp_processor_id(), &rmpopt_cpumask)) {
+ cpumask_clear_cpu(smp_processor_id(), &rmpopt_cpumask);
+ current_cpu_cleared = true;
+ }
+
+ /* current CPU */
+ for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G)
+ rmpopt(pa);
+
+ for (pa = rmpopt_pa_start; pa < rmpopt_pa_end; pa += SZ_1G) {
+ on_each_cpu_mask(&rmpopt_cpumask, rmpopt_smp,
+ (void *)pa, true);
+
+ /* Give a chance for other threads to run */
+ cond_resched();
+
+ }
+
+ if (current_cpu_cleared)
+ cpumask_set_cpu(smp_processor_id(), &rmpopt_cpumask);
+}
+
void snp_setup_rmpopt(void)
{
u64 rmpopt_base;
@@ -568,9 +656,20 @@ void snp_setup_rmpopt(void)
if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
return;
+ /*
+ * Create an RMPOPT-specific workqueue to avoid scheduling
+ * RMPOPT workitem on the global system workqueue.
+ */
+ rmpopt_wq = alloc_workqueue("rmpopt_wq", WQ_UNBOUND, 1);
+ if (!rmpopt_wq) {
+ setup_clear_cpu_cap(X86_FEATURE_RMPOPT);
+ return;
+ }
+
/*
* RMPOPT_BASE MSR is per-core, so only one thread per core needs to
- * setup RMPOPT_BASE MSR.
+ * setup RMPOPT_BASE MSR. Additionally only one thread per core needs
+ * to issue the RMPOPT instruction.
*/
for_each_online_cpu(cpu) {
@@ -590,6 +689,20 @@ void snp_setup_rmpopt(void)
* up to 2 TB of system RAM on all CPUs.
*/
wrmsrq_on_cpus(&rmpopt_cpumask, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+
+ INIT_DELAYED_WORK(&rmpopt_delayed_work, rmpopt_work_handler);
+
+ rmpopt_pa_end = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
+
+ /* Limit memory scanning to the first 2 TB of RAM */
+ if ((rmpopt_pa_end - rmpopt_pa_start) > SZ_2T)
+ rmpopt_pa_end = rmpopt_pa_start + SZ_2T;
+
+ /*
+ * Once all per-CPU RMPOPT tables have been configured, enable RMPOPT
+ * optimizations on all physical memory.
+ */
+ queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
}
EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
--
2.43.0
^ permalink raw reply related
* [PATCH v4 5/7] x86/sev: Add interface to re-enable RMP optimizations.
From: Ashish Kalra @ 2026-04-13 19:43 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
RMPOPT table is a per-CPU table which indicates if 1GB regions of
physical memory are entirely hypervisor-owned or not.
When performing host memory accesses in hypervisor mode as well as
non-SNP guest mode, the processor may consult the RMPOPT table to
potentially skip an RMP access and improve performance.
Events such as RMPUPDATE can clear RMP optimizations. Add an interface
to re-enable those optimizations.
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/include/asm/sev.h | 2 ++
arch/x86/virt/svm/sev.c | 10 ++++++++++
2 files changed, 12 insertions(+)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 409ab3372f7c..dc9086057060 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -662,6 +662,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int pages)
__snp_leak_pages(pfn, pages, true);
}
void snp_prepare(void);
+void snp_rmpopt_all_physmem(void);
void snp_setup_rmpopt(void);
void snp_shutdown(void);
#else
@@ -681,6 +682,7 @@ static inline void snp_leak_pages(u64 pfn, unsigned int npages) {}
static inline void kdump_sev_callback(void) { }
static inline void snp_fixup_e820_tables(void) {}
static inline void snp_prepare(void) {}
+static inline void snp_rmpopt_all_physmem(void) {}
static inline void snp_setup_rmpopt(void) {}
static inline void snp_shutdown(void) {}
#endif
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 56c9fc3fe53a..74ba8ec9de35 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -648,6 +648,16 @@ static void rmpopt_work_handler(struct work_struct *work)
cpumask_set_cpu(smp_processor_id(), &rmpopt_cpumask);
}
+void snp_rmpopt_all_physmem(void)
+{
+ if (!cpu_feature_enabled(X86_FEATURE_RMPOPT))
+ return;
+
+ queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work,
+ msecs_to_jiffies(RMPOPT_WORK_TIMEOUT));
+}
+EXPORT_SYMBOL_GPL(snp_rmpopt_all_physmem);
+
void snp_setup_rmpopt(void)
{
u64 rmpopt_base;
--
2.43.0
^ permalink raw reply related
* [PATCH v4 6/7] KVM: SEV: Perform RMP optimizations on SNP guest shutdown
From: Ashish Kalra @ 2026-04-13 19:44 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
Pages are converted from shared to private as SNP guests are launched.
This destroys exisiting RMPOPT optimizations in the regions where
pages are converted.
Conversely, guest pages are converted back to shared during SNP guest
termination and their region may become eligible for RMPOPT
optimization.
To take advantage of this, perform RMPOPT after guest termination.
Do it after a delay so that a single RMPOPT pass can be done if
multiple guests terminate in a short period of time.
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/kvm/svm/sev.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 3f9c1aa39a0a..e0f4f8ebef68 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2942,6 +2942,8 @@ void sev_vm_destroy(struct kvm *kvm)
if (sev_snp_guest(kvm)) {
snp_guest_req_cleanup(kvm);
+ snp_rmpopt_all_physmem();
+
/*
* Decomission handles unbinding of the ASID. If it fails for
* some unexpected reason, just leak the ASID.
--
2.43.0
^ permalink raw reply related
* [PATCH v4 7/7] x86/sev: Add debugfs support for RMPOPT
From: Ashish Kalra @ 2026-04-13 19:44 UTC (permalink / raw)
To: tglx, mingo, bp, dave.hansen, x86, hpa, seanjc, peterz,
thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <cover.1775874970.git.ashish.kalra@amd.com>
From: Ashish Kalra <ashish.kalra@amd.com>
Add a debugfs interface to report per-CPU RMPOPT status across all
system RAM.
To dump the per-CPU RMPOPT status for all system RAM:
/sys/kernel/debug/rmpopt# cat rmpopt-table
Memory @ 0GB: CPU(s): none
Memory @ 1GB: CPU(s): none
Memory @ 2GB: CPU(s): 0-1023
Memory @ 3GB: CPU(s): 0-1023
Memory @ 4GB: CPU(s): none
Memory @ 5GB: CPU(s): 0-1023
Memory @ 6GB: CPU(s): 0-1023
Memory @ 7GB: CPU(s): 0-1023
...
Memory @1025GB: CPU(s): 0-1023
Memory @1026GB: CPU(s): 0-1023
Memory @1027GB: CPU(s): 0-1023
Memory @1028GB: CPU(s): 0-1023
Memory @1029GB: CPU(s): 0-1023
Memory @1030GB: CPU(s): 0-1023
Memory @1031GB: CPU(s): 0-1023
Memory @1032GB: CPU(s): 0-1023
Memory @1033GB: CPU(s): 0-1023
Memory @1034GB: CPU(s): 0-1023
Memory @1035GB: CPU(s): 0-1023
Memory @1036GB: CPU(s): 0-1023
Memory @1037GB: CPU(s): 0-1023
Memory @1038GB: CPU(s): none
Suggested-by: Thomas Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
---
arch/x86/virt/svm/sev.c | 107 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 107 insertions(+)
diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c
index 74ba8ec9de35..dee2e853b4ad 100644
--- a/arch/x86/virt/svm/sev.c
+++ b/arch/x86/virt/svm/sev.c
@@ -20,6 +20,8 @@
#include <linux/amd-iommu.h>
#include <linux/nospec.h>
#include <linux/workqueue.h>
+#include <linux/debugfs.h>
+#include <linux/seq_file.h>
#include <asm/sev.h>
#include <asm/processor.h>
@@ -143,6 +145,13 @@ static DEFINE_SPINLOCK(snp_leaked_pages_list_lock);
static unsigned long snp_nr_leaked_pages;
+static cpumask_t rmpopt_report_cpumask;
+static struct dentry *rmpopt_debugfs;
+
+struct seq_paddr {
+ phys_addr_t next_seq_paddr;
+};
+
#undef pr_fmt
#define pr_fmt(fmt) "SEV-SNP: " fmt
@@ -580,6 +589,8 @@ static inline bool __rmpopt(u64 rax, u64 rcx)
: "a" (rax), "c" (rcx)
: "memory", "cc");
+ assign_cpu(smp_processor_id(), &rmpopt_report_cpumask, optimized);
+
return optimized;
}
@@ -602,6 +613,100 @@ static void rmpopt(u64 pa)
__rmpopt(rax, rcx);
}
+/*
+ * 'val' is a system physical address.
+ */
+static void rmpopt_report_status(void *val)
+{
+ u64 rax = ALIGN_DOWN((u64)val, SZ_1G);
+ u64 rcx = RMPOPT_FUNC_REPORT_STATUS;
+
+ __rmpopt(rax, rcx);
+}
+
+/*
+ * start() can be called multiple times if allocated buffer has overflowed
+ * and bigger buffer is allocated.
+ */
+static void *rmpopt_table_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ phys_addr_t end_paddr = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
+ struct seq_paddr *p = seq->private;
+
+ if (*pos == 0) {
+ p->next_seq_paddr = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
+ return &p->next_seq_paddr;
+ }
+
+ if (p->next_seq_paddr == end_paddr)
+ return NULL;
+
+ return &p->next_seq_paddr;
+}
+
+static void *rmpopt_table_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ phys_addr_t end_paddr = ALIGN(PFN_PHYS(max_pfn), SZ_1G);
+ phys_addr_t *curr_paddr = v;
+
+ (*pos)++;
+ if (*curr_paddr == end_paddr)
+ return NULL;
+ *curr_paddr += SZ_1G;
+
+ return curr_paddr;
+}
+
+static void rmpopt_table_seq_stop(struct seq_file *seq, void *v)
+{
+}
+
+static int rmpopt_table_seq_show(struct seq_file *seq, void *v)
+{
+ phys_addr_t *curr_paddr = v;
+
+ seq_printf(seq, "Memory @%3lluGB: ",
+ *curr_paddr >> (get_order(SZ_1G) + PAGE_SHIFT));
+
+ cpumask_clear(&rmpopt_report_cpumask);
+ on_each_cpu_mask(cpu_online_mask, rmpopt_report_status,
+ (void *)*curr_paddr, true);
+
+ if (cpumask_empty(&rmpopt_report_cpumask))
+ seq_puts(seq, "CPU(s): none\n");
+ else
+ seq_printf(seq, "CPU(s): %*pbl\n", cpumask_pr_args(&rmpopt_report_cpumask));
+
+ return 0;
+}
+
+static const struct seq_operations rmpopt_table_seq_ops = {
+ .start = rmpopt_table_seq_start,
+ .next = rmpopt_table_seq_next,
+ .stop = rmpopt_table_seq_stop,
+ .show = rmpopt_table_seq_show
+};
+
+static int rmpopt_table_open(struct inode *inode, struct file *file)
+{
+ return seq_open_private(file, &rmpopt_table_seq_ops, sizeof(struct seq_paddr));
+}
+
+static const struct file_operations rmpopt_table_fops = {
+ .open = rmpopt_table_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_release_private,
+};
+
+static void rmpopt_debugfs_setup(void)
+{
+ rmpopt_debugfs = debugfs_create_dir("rmpopt", arch_debugfs_dir);
+
+ debugfs_create_file("rmpopt-table", 0444, rmpopt_debugfs,
+ NULL, &rmpopt_table_fops);
+}
+
/*
* RMPOPT optimizations skip RMP checks at 1GB granularity if this
* range of memory does not contain any SNP guest memory.
@@ -713,6 +818,8 @@ void snp_setup_rmpopt(void)
* optimizations on all physical memory.
*/
queue_delayed_work(rmpopt_wq, &rmpopt_delayed_work, 0);
+
+ rmpopt_debugfs_setup();
}
EXPORT_SYMBOL_FOR_MODULES(snp_setup_rmpopt, "ccp");
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7 21/22] x86/virt/tdx: Document TDX module update
From: Edgecombe, Rick P @ 2026-04-13 19:54 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
Gao, Chao
Cc: corbet@lwn.net, 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, skhan@linuxfoundation.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-22-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> Document TDX module update as a subsection of "TDX Host Kernel Support" to
> provide background information and cover key points that developers and
> users may need to know, for example:
>
> - update is done in stop_machine() context
> - update instructions and results
> - update policy and tooling
>
> 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>
> ---
> v5:
> - use "update" when refer to the update feature/concept [Kai]
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v7 22/22] x86/virt/seamldr: Log TDX module update failures
From: Edgecombe, Rick P @ 2026-04-13 20:04 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-23-chao.gao@intel.com>
On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> Currently, there is no way to restore a TDX module from shutdown state to
> running state. This means if errors occur after a successful module
> shutdown, they are unrecoverable since the old module is gone but the new
> module isn't installed. All subsequent SEAMCALLs to the TDX module will
> fail, so TDs will be killed due to SEAMCALL failures.
>
> Log a message to clarify that SEAMCALL errors are expected in this
> scenario. This ensures that after update failures, the first message in
> dmesg explains the situation rather than showing confusing call traces from
> various code paths.
Why is this patch at the end? Is it not valid initial behavior?
>
> 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>
> Acked-by: Kai Huang <kai.huang@intel.com>
> ---
> v4:
> - Use pr_warn_once() instead of reinventing it [Yilun]
> v3:
> - Rephrase the changelog to eliminate the confusing uses of 'i.e.' and 'e.g.'
> [Dave/Yilun]
> ---
> arch/x86/virt/vmx/tdx/seamldr.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index b3f3b40627c3..a972f9ba6877 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -252,6 +252,11 @@ static void ack_state(void)
> set_target_state(update_data.state + 1);
> }
>
> +static void print_update_failure_message(void)
> +{
> + pr_err_once("update failed, SEAMCALLs will report failure until TDs killed\n");
> +}
Ehh, it seems ok. But is very KVM specific. I guess TDX connect will need to
update the message. The sys disable will fail too. But it does communicate the
main current actionable step for users.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> +
> /*
> * See multi_cpu_stop() from where this multi-cpu state-machine was
> * adopted, and the rationale for touch_nmi_watchdog().
> @@ -291,10 +296,13 @@ static int do_seamldr_install_module(void *seamldr_params)
> break;
> }
>
> - if (ret)
> + if (ret) {
> WRITE_ONCE(update_data.failed, true);
> - else
> + if (curstate > MODULE_UPDATE_SHUTDOWN)
> + print_update_failure_message();
> + } else {
> ack_state();
> + }
> } else {
> touch_nmi_watchdog();
> rcu_momentary_eqs();
^ permalink raw reply
* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Huang, Kai @ 2026-04-13 23:03 UTC (permalink / raw)
To: seanjc@google.com
Cc: Bae, Chang Seok, kvm@vger.kernel.org, pbonzini@redhat.com,
kas@kernel.org, linux-kernel@vger.kernel.org,
linux-coco@lists.linux.dev, x86@kernel.org
In-Reply-To: <ad0DrDUsUKTMfrDW@google.com>
On Mon, 2026-04-13 at 07:54 -0700, Sean Christopherson wrote:
> On Mon, Apr 13, 2026, Kai Huang wrote:
> > On Thu, 2026-04-09 at 15:42 -0700, Sean Christopherson wrote:
> > > -#define TDX_REGS_AVAIL_SET (BIT_ULL(VCPU_REG_EXIT_INFO_1) | \
> > > - BIT_ULL(VCPU_REG_EXIT_INFO_2) | \
> > > - BIT_ULL(VCPU_REGS_RAX) | \
> > > - BIT_ULL(VCPU_REGS_RBX) | \
> > > - BIT_ULL(VCPU_REGS_RCX) | \
> > > - BIT_ULL(VCPU_REGS_RDX) | \
> > > - BIT_ULL(VCPU_REGS_RBP) | \
> > > - BIT_ULL(VCPU_REGS_RSI) | \
> > > - BIT_ULL(VCPU_REGS_RDI) | \
> > > - BIT_ULL(VCPU_REGS_R8) | \
> > > - BIT_ULL(VCPU_REGS_R9) | \
> > > - BIT_ULL(VCPU_REGS_R10) | \
> > > - BIT_ULL(VCPU_REGS_R11) | \
> > > - BIT_ULL(VCPU_REGS_R12) | \
> > > - BIT_ULL(VCPU_REGS_R13) | \
> > > - BIT_ULL(VCPU_REGS_R14) | \
> > > - BIT_ULL(VCPU_REGS_R15))
> > > +#define TDX_REGS_AVAIL_SET (BIT(VCPU_REG_EXIT_INFO_1) | \
> > > + BIT(VCPU_REG_EXIT_INFO_2) | \
> > > + BIT(VCPU_REGS_RAX) | \
> > > + BIT(VCPU_REGS_RBX) | \
> > > + BIT(VCPU_REGS_RCX) | \
> > > + BIT(VCPU_REGS_RDX) | \
> > > + BIT(VCPU_REGS_RBP) | \
> > > + BIT(VCPU_REGS_RSI) | \
> > > + BIT(VCPU_REGS_RDI) | \
> > > + BIT(VCPU_REGS_R8) | \
> > > + BIT(VCPU_REGS_R9) | \
> > > + BIT(VCPU_REGS_R10) | \
> > > + BIT(VCPU_REGS_R11) | \
> > > + BIT(VCPU_REGS_R12) | \
> > > + BIT(VCPU_REGS_R13) | \
> > > + BIT(VCPU_REGS_R14) | \
> > > + BIT(VCPU_REGS_R15))
> > >
> >
> > Not related to this series, but this made me look into whether these
> > registers are truly needed to be set as available for TDX.
> >
> > Firstly, all the listed registers are marked as available immediately after
> > exiting from tdh_vp_enter(), but except VCPU_REG_EXIT_INFO_1 and
> > VCPU_REG_EXIT_INFO_2 are immediately saved to the common 'struct vcpu_vt',
> > all other GPRs are not saved to vcpu->arch.regs[], which means marking GPRs
> > available immediately doesn't quite make sense.
> >
> > In fact, IIUC other than when the TD exits with TDVMCALL on which TD shares
> > couple of GPRs with KVM, KVM has no way to get TD's GPRs. So perhaps it
> > makes more sense is to mark the shared GPRs available upon TDVMCALL.
> >
> > But even that does not make sense from KVM's "GPR available" perspective,
> > because TDVMCALL has a different ABI from KVM's existing infrastructure for
> > e.g., CPUID/MSR emulation. E.g., KVM uses RCX/RAX/RDX for MSR emulation,
> > but TDVMCALL<MSR.WRITE> uses R12 and R13 to convey MSR index/value:
> >
> > case EXIT_REASON_MSR_WRITE:
> > kvm_rcx_write(vcpu, tdx->vp_enter_args.r12);
> > kvm_rax_write(vcpu, tdx->vp_enter_args.r13 & -1u);
> > kvm_rdx_write(vcpu, tdx->vp_enter_args.r13 >> 32);
> >
> > So I think the most accurate way is to explicitly mark the relevant GPRs
> > available for each type of TDVMCALL. I am not sure whether it's worth to do
> > though, because AFAICT there's no real bug in the existing code, other than
> > "marking GPRs not in vcpu->arch.regs[] as available looks wrong".
> >
> > A less invasive way is to mark all possible GPRs that can be used in
> > TDVMCALL emulation available once after TD exits. AFAICT the KVM hypercall
> > uses most GPRs (RAX/RBX/RCX/RDX/RSI) and all other TDVMCALLs only use a
> > subset, so maybe we can remove other GPRs from the available list (the diff
> > in [*] passed my test of booting/destroying TD).
> >
> > Bug again, not sure whether it's worth doing.
>
> Not worth doing.
>
Fine to me. :-)
> Because VMX and SVM make all GRPs available immediately, except
> for RSP, KVM ignores avail/dirty for GPRs. I.e. "fixing" TDX will just shift the
> "bugs" elsewhere.
Just want to understand:
I thought the fix could be we simply remove the wrong GPRs from the list.
Not sure how fixing TDX will shift bugs elsewhere?
>
> More importantly, because the TDX-Module *requires* RCX (the GPR that holds the
> mask of registers to expose to the VMM) to be hidden on TDVMCALL, KVM *can't*
> do any kind of meaningful "available" tracking.
>
Hmm I think RCX conveys the shared GPRs and VMM can read. Per "Table 5.323:
TDH.VP.ENTER Output Operands Format #5 Definition: On TDCALL(TDG.VP.VMCALL)
Following a TD Entry":
RCX ...
Bit(s) Name Description
31:0 PARAMS_MASK Value as passed into TDCALL(TDG.VP.VMCALL) by
the guest TD: indicates which part of the guest
TD GPR and XMM state is passed as-is to the
VMM
and back. For details, see the description of
TDG.VP.VMCALL in 5.5.26.
I think the problem is, as said previously, currently KVM TDX code uses
KVM's existing infrastructure to emulate MSR, KVM hypercall etc, but
TDVMCALL has a different ABI, thus there's a mismatch here.
^ permalink raw reply
* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Xiaoyao Li @ 2026-04-14 2:12 UTC (permalink / raw)
To: Huang, Kai, seanjc@google.com
Cc: Bae, Chang Seok, kvm@vger.kernel.org, pbonzini@redhat.com,
kas@kernel.org, linux-kernel@vger.kernel.org,
linux-coco@lists.linux.dev, x86@kernel.org
In-Reply-To: <d6f05e5fa781ccb465d27a4fb1c7c1ac1e9e95ff.camel@intel.com>
On 4/14/2026 7:03 AM, Huang, Kai wrote:
>> Because VMX and SVM make all GRPs available immediately, except
>> for RSP, KVM ignores avail/dirty for GPRs. I.e. "fixing" TDX will just shift the
>> "bugs" elsewhere.
> Just want to understand:
>
> I thought the fix could be we simply remove the wrong GPRs from the list.
> Not sure how fixing TDX will shift bugs elsewhere?
I'm curious too.
>> More importantly, because the TDX-Module*requires* RCX (the GPR that holds the
>> mask of registers to expose to the VMM) to be hidden on TDVMCALL, KVM*can't*
>> do any kind of meaningful "available" tracking.
>>
> Hmm I think RCX conveys the shared GPRs and VMM can read. Per "Table 5.323:
> TDH.VP.ENTER Output Operands Format #5 Definition: On TDCALL(TDG.VP.VMCALL)
> Following a TD Entry":
>
> RCX ...
> Bit(s) Name Description
>
> 31:0 PARAMS_MASK Value as passed into TDCALL(TDG.VP.VMCALL) by
> the guest TD: indicates which part of the guest
> TD GPR and XMM state is passed as-is to the
> VMM
> and back. For details, see the description of
> TDG.VP.VMCALL in 5.5.26.
>
> I think the problem is, as said previously, currently KVM TDX code uses
> KVM's existing infrastructure to emulate MSR, KVM hypercall etc, but
> TDVMCALL has a different ABI, thus there's a mismatch here.
I once had patch for it internally.
It adds back the available check for GPRs when accessing instead of
assuming they are always available. For normal VMX and SVM, all the GPRs
are still always available. But for TDX, only EXIT_INFO_1 and
EXIT_INFO_2 are always marked available, while others need to be
explicitly set case by case.
The good thing is it makes TDX safer that KVM won't consume invalid data
silently for TDX. But it adds additional overhead of checking the
unnecessary register availability for VMX and SVM case.
-----------------------------&<-------------------------------------
From: Xiaoyao Li <xiaoyao.li@intel.com>
Date: Tue, 11 Mar 2025 07:13:29 -0400
Subject: [PATCH] KVM: x86: Add available check for GPRs
Since commit de3cd117ed2f ("KVM: x86: Omit caching logic for
always-available GPRs"), KVM doesn't check the availability of GPRs
except RSP and RIP when accessing them, because they are always
available.
However, it's not true when it comes to TDX. The GPRs are not available
after TD vcpu exits actually. And it relies on KVM manually sets the
GPRs value when needed, e.g.
- setting rax, rbx, rcx, rdx, rsi, for hypercall emulation in
tdx_emulate_tdvmall();
- setting rax, rcx and rdx before MSR write emulation;
Add the available check of GPRs read, and WARN_ON_ONCE() when unavailable.
It can help capture the cases of undesired GPRs consumption by TDX.
Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
---
arch/x86/kvm/kvm_cache_regs.h | 60 +++++++++++++++++++----------------
arch/x86/kvm/vmx/tdx.c | 25 +++------------
2 files changed, 37 insertions(+), 48 deletions(-)
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h
index 8ddb01191d6f..b2fa01ee2b4b 100644
--- a/arch/x86/kvm/kvm_cache_regs.h
+++ b/arch/x86/kvm/kvm_cache_regs.h
@@ -16,34 +16,6 @@
static_assert(!(KVM_POSSIBLE_CR0_GUEST_BITS & X86_CR0_PDPTR_BITS));
-#define BUILD_KVM_GPR_ACCESSORS(lname, uname) \
-static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu
*vcpu)\
-{ \
- return vcpu->arch.regs[VCPU_REGS_##uname]; \
-} \
-static __always_inline void kvm_##lname##_write(struct kvm_vcpu *vcpu,
\
- unsigned long val) \
-{ \
- vcpu->arch.regs[VCPU_REGS_##uname] = val; \
-}
-BUILD_KVM_GPR_ACCESSORS(rax, RAX)
-BUILD_KVM_GPR_ACCESSORS(rbx, RBX)
-BUILD_KVM_GPR_ACCESSORS(rcx, RCX)
-BUILD_KVM_GPR_ACCESSORS(rdx, RDX)
-BUILD_KVM_GPR_ACCESSORS(rbp, RBP)
-BUILD_KVM_GPR_ACCESSORS(rsi, RSI)
-BUILD_KVM_GPR_ACCESSORS(rdi, RDI)
-#ifdef CONFIG_X86_64
-BUILD_KVM_GPR_ACCESSORS(r8, R8)
-BUILD_KVM_GPR_ACCESSORS(r9, R9)
-BUILD_KVM_GPR_ACCESSORS(r10, R10)
-BUILD_KVM_GPR_ACCESSORS(r11, R11)
-BUILD_KVM_GPR_ACCESSORS(r12, R12)
-BUILD_KVM_GPR_ACCESSORS(r13, R13)
-BUILD_KVM_GPR_ACCESSORS(r14, R14)
-BUILD_KVM_GPR_ACCESSORS(r15, R15)
-#endif
-
/*
* Using the register cache from interrupt context is generally not
allowed, as
* caching a register and marking it available/dirty can't be done
atomically,
@@ -92,6 +64,38 @@ static inline void kvm_register_mark_dirty(struct
kvm_vcpu *vcpu,
__set_bit(reg, (unsigned long *)&vcpu->arch.regs_dirty);
}
+#define BUILD_KVM_GPR_ACCESSORS(lname, uname) \
+static __always_inline unsigned long kvm_##lname##_read(struct kvm_vcpu
*vcpu)\
+{ \
+ if (WARN_ON_ONCE(!kvm_register_is_available(vcpu, VCPU_REGS_##uname)))\
+ return 0; \
+ \
+ return vcpu->arch.regs[VCPU_REGS_##uname]; \
+} \
+static __always_inline void kvm_##lname##_write(struct kvm_vcpu *vcpu,
\
+ unsigned long val) \
+{ \
+ vcpu->arch.regs[VCPU_REGS_##uname] = val; \
+ kvm_register_mark_available(vcpu, VCPU_REGS_##uname); \
+}
+BUILD_KVM_GPR_ACCESSORS(rax, RAX)
+BUILD_KVM_GPR_ACCESSORS(rbx, RBX)
+BUILD_KVM_GPR_ACCESSORS(rcx, RCX)
+BUILD_KVM_GPR_ACCESSORS(rdx, RDX)
+BUILD_KVM_GPR_ACCESSORS(rbp, RBP)
+BUILD_KVM_GPR_ACCESSORS(rsi, RSI)
+BUILD_KVM_GPR_ACCESSORS(rdi, RDI)
+#ifdef CONFIG_X86_64
+BUILD_KVM_GPR_ACCESSORS(r8, R8)
+BUILD_KVM_GPR_ACCESSORS(r9, R9)
+BUILD_KVM_GPR_ACCESSORS(r10, R10)
+BUILD_KVM_GPR_ACCESSORS(r11, R11)
+BUILD_KVM_GPR_ACCESSORS(r12, R12)
+BUILD_KVM_GPR_ACCESSORS(r13, R13)
+BUILD_KVM_GPR_ACCESSORS(r14, R14)
+BUILD_KVM_GPR_ACCESSORS(r15, R15)
+#endif
+
/*
* kvm_register_test_and_mark_available() is a special snowflake that
uses an
* arch bitop directly to avoid the explicit instrumentation that
comes with
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index cefe6cdd60a9..2b90a60e6f64 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -969,6 +969,9 @@ static __always_inline u32
tdx_to_vmx_exit_reason(struct kvm_vcpu *vcpu)
return exit_reason;
}
+#define TDX_REGS_AVAIL_SET (BIT_ULL(VCPU_EXREG_EXIT_INFO_1) | \
+ BIT_ULL(VCPU_EXREG_EXIT_INFO_2))
+
static noinstr void tdx_vcpu_enter_exit(struct kvm_vcpu *vcpu)
{
struct vcpu_tdx *tdx = to_tdx(vcpu);
@@ -985,6 +988,8 @@ static noinstr void tdx_vcpu_enter_exit(struct
kvm_vcpu *vcpu)
tdx->exit_gpa = tdx->vp_enter_args.r8;
vt->exit_intr_info = tdx->vp_enter_args.r9;
+ vcpu->arch.regs_avail &= TDX_REGS_AVAIL_SET;
+
vmx_handle_nmi(vcpu);
guest_state_exit_irqoff();
@@ -1017,24 +1022,6 @@ static fastpath_t
tdx_exit_handlers_fastpath(struct kvm_vcpu *vcpu)
return EXIT_FASTPATH_NONE;
}
-#define TDX_REGS_AVAIL_SET (BIT_ULL(VCPU_EXREG_EXIT_INFO_1) | \
- BIT_ULL(VCPU_EXREG_EXIT_INFO_2) | \
- BIT_ULL(VCPU_REGS_RAX) | \
- BIT_ULL(VCPU_REGS_RBX) | \
- BIT_ULL(VCPU_REGS_RCX) | \
- BIT_ULL(VCPU_REGS_RDX) | \
- BIT_ULL(VCPU_REGS_RBP) | \
- BIT_ULL(VCPU_REGS_RSI) | \
- BIT_ULL(VCPU_REGS_RDI) | \
- BIT_ULL(VCPU_REGS_R8) | \
- BIT_ULL(VCPU_REGS_R9) | \
- BIT_ULL(VCPU_REGS_R10) | \
- BIT_ULL(VCPU_REGS_R11) | \
- BIT_ULL(VCPU_REGS_R12) | \
- BIT_ULL(VCPU_REGS_R13) | \
- BIT_ULL(VCPU_REGS_R14) | \
- BIT_ULL(VCPU_REGS_R15))
-
static void tdx_load_host_xsave_state(struct kvm_vcpu *vcpu)
{
struct kvm_tdx *kvm_tdx = to_kvm_tdx(vcpu->kvm);
@@ -1108,8 +1095,6 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, u64
run_flags)
tdx_load_host_xsave_state(vcpu);
- vcpu->arch.regs_avail &= TDX_REGS_AVAIL_SET;
-
if (unlikely(tdx->vp_enter_ret == EXIT_REASON_EPT_MISCONFIG))
return EXIT_FASTPATH_NONE;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-04-14 9:43 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, 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: <a6a3426ba9b84ea25f3653e33a181dab0096b35f.camel@intel.com>
On Sat, Apr 11, 2026 at 09:14:36AM +0800, Edgecombe, Rick P wrote:
>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?
Yes. how about adding the following in the changelog:
Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure. The
header is consumed solely by the kernel to extract the sigstruct and
module, so validate it before processing. The sigstruct and module are
passed to and validated by P-SEAMLDR, so don't duplicate any validation
in the kernel.
>And how it explains the checks below?
>> +static void free_seamldr_params(struct seamldr_params *params)
>> +{
>> + free_page((unsigned long)params);
>> +}
>
>Do we really need this helper? This doesn't work?
No. This works. Will do.
>
>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?
I think we all agree: we need to do something to protect the loop. So, the
question is just how.
looks like you prefer:
module_size = MIN(module_size, SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K);
sig_size = MIN(sig_size, SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K);
But I think the MIN approach actually requires more justification than a plain
bounds check.
With MIN, the reasoning chain is: we don't want conditionals -> userspace
can deal with the module error -> and that error won't be fatal to the
system (or even if it is, it's admin-initiated). That's a lot of assumptions
to validate.
With a bounds check and early return, the reasoning is simply: the input is out
of range -> reject it as invalid.
>
>> +
>> + /*
>> + * 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?
If the blob->offset_of_module or blob->size is misaligned, the loops
silently drop the unaligned portion. P-SEAMLDR will then reject the update
after module shutdown and TDs will be killed. Since this is admin-initiated
with an intentionally invalid blob, the consequence is acceptable.
>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 agree addresses/sizes are page-aligned for valid blobs, but they are not
guaranteed (e.g., if offset_of_module is misaligned) for all inputs. I
removed the check once in v6 and Sashiko questioned it, so I thought the
assumption isn't obviously correct to everyone.
Without the check, we'd need something like:
/*
* Assume sig/module base addresses and sizes are page-aligned.
* If violated, P-SEAMLDR will reject the update.
*/
It's a few lines of straightforward validation vs. a comment that requires
readers to verify the assumption chain (vmalloc internals, userspace
contract, P-SEAMLDR behavior). Not sure the trade is worth it.
If you think the comment is sufficient, I'm fine dropping the checks.
>
>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?
All current P-SEAMLDRs only support version 0. Version 1 is a planned
extension but not yet implemented by any P-SEAMLDR. Always requiring
version 1 just saves us a conditional but disables updates for all existing
P-SEAMLDRs for no reason. I think it is not worthwhile.
>But also could it be:
>
>params->version = sig_size > SZ_4K;
ok with me. I just wanted to make it match with "version 1" in the
comment right above.
>> +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.
Fine with merging them. How about:
static struct seamldr_params *alloc_seamldr_params(const u8 *data, u32 size)
{
const struct tdx_blob *blob = (const void *)data;
struct seamldr_params *params;
int module_size, sig_size;
const void *sig, *module;
const u8 *ptr;
int i;
/*
* Ensure the size is valid otherwise reading any field from the
* blob may overflow.
*/
if (size <= sizeof(struct tdx_blob))
return ERR_PTR(-EINVAL);
if (blob->version != TDX_BLOB_VERSION_1)
return ERR_PTR(-EINVAL);
if (blob->length != size)
return ERR_PTR(-EINVAL);
if (memcmp(blob->signature, "TDX-BLOB", 8))
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;
if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K || module_size <= 0)
return ERR_PTR(-EINVAL);
if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K || sig_size <= 0)
return ERR_PTR(-EINVAL);
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.
*/
params->version = sig_size > SZ_4K;
params->scenario = SEAMLDR_SCENARIO_UPDATE;
ptr = sig;
/*
* Assume sig/module base addresses and sizes are page-aligned.
* If violated, P-SEAMLDR will reject the update.
*/
for (i = 0; i < sig_size / SZ_4K; i++) {
params->sigstruct_pa[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
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;
ptr += SZ_4K;
}
return params;
}
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-04-14 9:50 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, 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: <91f8645bff41f5b28bf964b215429b8c7ad53715.camel@intel.com>
On Sat, Apr 11, 2026 at 08:26:23AM +0800, Edgecombe, Rick P wrote:
>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?
Good point.
The series is ordered top-down (user interface first, implementation details
later). I could either move this patch later, or add each error mapping in the
patch that introduces the corresponding return value. I'd prefer the latter to
keep the top-down ordering. Would that work for you?
^ permalink raw reply
* Re: [PATCH v7 10/22] x86/virt/seamldr: Abort updates if errors occurred midway
From: Chao Gao @ 2026-04-14 9:59 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, 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: <fb472138f6ba06efd90a3834a5748662d7b7ac44.camel@intel.com>
On Sat, Apr 11, 2026 at 09:26:58AM +0800, Edgecombe, Rick P wrote:
>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?
Good question.
The main point is correctness, not saving time.
If shutdown fails midway, the update is still recoverable — TDs can continue
running. But if we proceed to seamldr.install anyway, it becomes destructive.
Aborting early on shutdown failure preserves recoverability (this is needed to
handle races between updates and TD build/migration).
If seamldr.install itself fails, it's already destructive, so aborting early
there just saves time. But using the same abort mechanism for both keeps the
error handling uniform.
^ permalink raw reply
* Re: [PATCH v7 11/22] x86/virt/seamldr: Shut down the current TDX module
From: Chao Gao @ 2026-04-14 10:09 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, 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 Sat, Apr 11, 2026 at 09:35:32AM +0800, Edgecombe, Rick P wrote:
>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.
Yes. How about:
Retrieve the module's handoff version from TDX global metadata and add an
update step to shut down the module. Module shutdown has global effect, so
it only needs to run on one CPU.
^ permalink raw reply
* Re: [PATCH 0/3] arm64/virt: Add Arm CCA measurement register support
From: Suzuki K Poulose @ 2026-04-14 10:10 UTC (permalink / raw)
To: Jason Gunthorpe, Sami Mujawar, Dan Williams
Cc: linux-arm-kernel, linux-kernel, catalin.marinas, will, thuth,
steven.price, gshan, YeoReum.Yun, cedric.xing, Dan Williams,
Dionna Glaze, Aneesh Kumar K . V, Alexey Kardashevskiy,
linux-coco@lists.linux.dev
In-Reply-To: <20260413125925.GK3694781@ziepe.ca>
Cc: Dan, Cedric, Dionna, Aneesh, Alexey. linux-coco
Hi Jason,
On 13/04/2026 13:59, Jason Gunthorpe wrote:
> On Mon, Apr 13, 2026 at 09:49:54AM +0100, Sami Mujawar wrote:
>> This series adds support for Arm Confidential Compute Architecture (CCA)
>> measurement registers in the Linux kernel, enabling guest Realms to
>> access, extend, and expose measurement values for attestation and runtime
>> integrity tracking.
>>
>> The Realm Management Monitor (RMM) defines a set of measurement registers
>> consisting of a Realm Initial Measurement (RIM) and a number of Realm
>> Extensible Measurements (REMs). This series introduces the necessary
>> infrastructure to interact with these registers via the RSI interface
>> and exposes them to userspace through the TSM measurement framework.
>>
>> At a high level, the series includes:
>> - Helper interfaces for reading and extending measurement
>> registers via RSI
>> - Definitions for Realm hash algorithms as defined by the
>> RMM specification
>> - Integration with the TSM measurement subsystem and sysfs
>> exposure for userspace visibility and interaction
>>
>> After applying this series, measurement registers are exposed under:
>> /sys/devices/virtual/misc/arm_cca_guest/measurements/
>
> I'm surprised we get some random sysfs files? How does some more
> generic userspace figure out to use this vs a TPM or some other
> platform's version of it?
That is true. This is the infrastructure for exposing Runtime
Measurement registers (R/W) for use by the OS, complementing the
TSM_REPORTS (Read Only Platform measurements+Attestation Reports, e.g.
on CCA Attestation Report from RMM). Unlike the TSM reports,
this doesn't have a generic interface for userspace.
> I also think exposing PCRs as was done for TPM in sysfs was something
> of a mistake.. Allowing extension without logging is too low level and
> is very hard to build an entire attestation system around.
>
> I really think we are missing a subsystem here, TPM has sort of been
> filling this role in a non-generic way, but we should have a
> common uAPI for platform measurement & attestation:
Agreed, such a subsystem would solve the below.
> - Discover available measurements
> - Report signed measurements, with ingesting a nonce
> - Report measurement logs
> - Extend measurements and udpate logs
> - Report certificates used in signing
> - General reporting of various kinds of attestation evidence
>
> And it would be nice for the PCI devices and others to plug into the
> general framework as well instead of building a parallel TSM framework
> for handling evidence.
That makes sense and AFAIU, there are efforts in progress to expose
the Device measurements+Certificates in a different form. May be a good
idea to intervene early enough to see if we can find a common ground.
>
> Isn't this also sort of incomplete? Doesn't anything serious need
> signed measurements? Isnt't there alot more data that comes out of RMM
> than just a few measurement registers?
As mentioned above, this series adds the support for Runtime Extendible
Measurements (REM in CCA, RTMR on TDX). The RIM+Platform Attestation is
already provided via the TSM_REPORT
Kind regards
Suzuki
>
> Jason
^ 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