* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Sean Christopherson @ 2026-02-04 1:16 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Kai Huang,
Xiaoyao Li, Yan Y Zhao, dave.hansen@linux.intel.com,
linux-kernel@vger.kernel.org, kas@kernel.org, mingo@redhat.com,
pbonzini@redhat.com, binbin.wu@linux.intel.com,
ackerleytng@google.com, Isaku Yamahata, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Vishal Annapurve, x86@kernel.org
In-Reply-To: <e3feb0224cf2665a71ba6147e4e3e3bb30f96760.camel@intel.com>
On Fri, Jan 30, 2026, Rick P Edgecombe wrote:
> On Thu, 2026-01-29 at 17:28 -0800, Sean Christopherson wrote:
> >
> > Hmm, that's probably doable, but definitely in a separate patch.
> > E.g. something
> > like:
>
> I think it would be a good change. But after more consideration, I
> think the original patch is good on its own. Better to turn a bug into
> a deterministic thing, than an opportunity to consume stack. Seems to
> be what you intended.
>
> Another idea would be to have a variant that returns an error instead
> of 0 so the callers can have there error logic triggered, but it's all
> incremental value on top of this.
I don't like that idea, at all. First and foremost, I don't want to litter KVM
with WARNs for things that simply can't happen. I'm fine adding infrastructure
that hides the sanity checks, but I don't want to bleed that into callers.
The other aspect I dislike is that returning a specific errno could lead to all
sorts of weirdness and hidden dependencies.
All in all, I think we'd be increasing the chances of creating bugs just to harden
against issues that in all likelihood will never happen.
^ permalink raw reply
* Re: [PATCH v7 0/2] SEV-SNP: Add KVM support for SNP certificate fetching
From: Sean Christopherson @ 2026-02-04 0:10 UTC (permalink / raw)
To: Sean Christopherson, kvm, Michael Roth
Cc: linux-coco, linux-kernel, pbonzini, jroedel, thomas.lendacky,
liam.merwick, huibo.wang
In-Reply-To: <20260109231732.1160759-1-michael.roth@amd.com>
On Fri, 09 Jan 2026 17:17:31 -0600, Michael Roth wrote:
> This patchset is also available at:
>
> https://github.com/amdese/linux/commits/snp-certs-v7
>
> and is based on top of kvm/next (0499add8efd7)
>
>
> [...]
Applied to kvm-x86 svm, thanks!
[1/2] KVM: Introduce KVM_EXIT_SNP_REQ_CERTS for SNP certificate-fetching
https://github.com/kvm-x86/linux/commit/fa9893fadbc2
[2/2] KVM: SEV: Add KVM_SEV_SNP_ENABLE_REQ_CERTS command
https://github.com/kvm-x86/linux/commit/20c3c4108d58
--
https://github.com/kvm-x86/linux/tree/next
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-02-03 23:54 UTC (permalink / raw)
To: Sean Christopherson
Cc: Dave Hansen, linux-coco, linux-kernel, kvm, x86, reinette.chatre,
ira.weiny, kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve,
paulmck, nik.borisov, zhenzhong.duan, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <aYIXFmT-676oN6j0@google.com>
>On Fri, Jan 30, 2026 at 8:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
>> On 1/30/26 00:08, Chao Gao wrote:
>> > AFAIK, this is a CPU implementation issue. The actual requirement is to
>> > evict (flush and invalidate) all VMCSs __cached in SEAM mode__, but big
>> > cores implement this by evicting the __entire__ VMCS cache. So, the
>> > current VMCS is invalidated and cleared.
>>
>> But why is this a P-SEAMLDR thing and not a TDX module thing?
>
>My guess is that it's because the P-SEAMLDR code loads and prepares the new TDX-
>Module by constructing the VMCS used for SEAMCALL using direct writes to memory
>(unless that TDX behavior has changed in the last few years). And so it needs
>to ensure that in-memory representation is synchronized with the VMCS cache.
>
>Hmm, but that doesn't make sense _if_ it really truly is SEAMRET that does the VMCS
>cache invalidation, because flushing the VMCS cache would ovewrite the in-memory
>state.
My understanding is:
1. SEAMCALL/SEAMRET use VMCSs.
2. P-SEAMLDR is single-threaded (likely for simplicity). So, it uses a _single_
global VMCS and only one CPU can call P-SEAMLDR calls at a time.
3. After SEAMRET from P-SEAMLDR, _if_ the global VMCS isn't flushed, other CPUs
cannot enter P-SEAMLDR because the global VMCS would be corrupted. (note the
global VMCS is cached by the original CPU).
4. To make P-SEAMLDR callable on all CPUs, SEAMRET instruction flush VMCSs.
The flush cannot be performed by the host VMM since the global VMCS is not
visible to it. P-SEAMLDR cannot do it either because SEAMRET is its final
instruction and requires a valid VMCS.
The TDX Module has per-CPU VMCSs, so it doesn't has this problem.
I'll check if SEAM ISA architects can join to explain this in more detail.
>
>> It seems like a bug, or at least a P-SEAMLDR implementation issue the
>> needs to get fixed.
>
>Yeah, 'tis odd behavior. IMO, that's all the more reason the TDX subsystem should
>hide the quirk from the rest of the kernel.
>
>[*] https://lore.kernel.org/all/20251010220403.987927-1-seanjc@google.com
^ permalink raw reply
* [PATCH v2 3/3] x86/sev: Rename SNP_FEATURES_PRESENT->SNP_FEATURES_IMPL
From: Kim Phillips @ 2026-02-03 22:24 UTC (permalink / raw)
To: linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Tom Lendacky, Michael Roth, Borislav Petkov,
Borislav Petkov, Naveen Rao, David Kaplan, Kim Phillips, stable
In-Reply-To: <20260203222405.4065706-1-kim.phillips@amd.com>
Rename SNP_FEATURES_PRESENT->SNP_FEATURES_IMPL to denote its
counterpart relationship with SNP_FEATURES_IMPL_REQ.
Fixes: 8c29f0165405 ("x86/sev: Add SEV-SNP guest feature negotiation support")
Suggested-by: Borislav Petkov (AMD) <bp@alien8.de>
Suggested-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Nikunj A Dadhania <nikunj@amd.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: stable@kernel.org
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
v2: new this series
arch/x86/boot/compressed/sev.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index 2b639703b8dd..aca5313d193c 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -198,11 +198,11 @@ bool sev_es_check_ghcb_fault(unsigned long address)
#endif
/*
- * SNP_FEATURES_PRESENT is the mask of SNP features that are implemented
+ * SNP_FEATURES_IMPL is the mask of SNP features that are implemented
* by the guest kernel. As and when a new feature is implemented in the
* guest kernel, a corresponding bit should be added to the mask.
*/
-#define SNP_FEATURES_PRESENT (MSR_AMD64_SNP_DEBUG_SWAP | \
+#define SNP_FEATURES_IMPL (MSR_AMD64_SNP_DEBUG_SWAP | \
MSR_AMD64_SNP_SECURE_TSC | \
SNP_FEATURE_SECURE_AVIC)
@@ -211,7 +211,7 @@ u64 snp_get_unsupported_features(u64 status)
if (!(status & MSR_AMD64_SEV_SNP_ENABLED))
return 0;
- return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_PRESENT;
+ return status & SNP_FEATURES_IMPL_REQ & ~SNP_FEATURES_IMPL;
}
void snp_check_features(void)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 2/3] KVM: SEV: Add support for IBPB-on-Entry
From: Kim Phillips @ 2026-02-03 22:24 UTC (permalink / raw)
To: linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Tom Lendacky, Michael Roth, Borislav Petkov,
Borislav Petkov, Naveen Rao, David Kaplan, Kim Phillips
In-Reply-To: <20260203222405.4065706-1-kim.phillips@amd.com>
AMD EPYC 5th generation and above processors support IBPB-on-Entry
for SNP guests. By invoking an Indirect Branch Prediction Barrier
(IBPB) on VMRUN, old indirect branch predictions are prevented
from influencing indirect branches within the guest.
SNP guests may choose to enable IBPB-on-Entry by setting
SEV_FEATURES bit 21 (IbpbOnEntry).
Host support for IBPB on Entry is indicated by CPUID
Fn8000_001F[IbpbOnEntry], bit 31.
If supported, indicate support for IBPB on Entry in
sev_supported_vmsa_features bit 23 (IbpbOnEntry).
For more info, refer to page 615, Section 15.36.17 "Side-Channel
Protection", AMD64 Architecture Programmer's Manual Volume 2: System
Programming Part 2, Pub. 24593 Rev. 3.42 - March 2024 (see Link).
Link: https://bugzilla.kernel.org/attachment.cgi?id=306250
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
---
v2: Added Tom's Reviewed-by.
v1: https://lore.kernel.org/kvm/20260126224205.1442196-3-kim.phillips@amd.com/
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/svm.h | 1 +
arch/x86/kvm/svm/sev.c | 9 ++++++++-
3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index c01fdde465de..3ce5dff36f78 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -459,6 +459,7 @@
#define X86_FEATURE_ALLOWED_SEV_FEATURES (19*32+27) /* Allowed SEV Features */
#define X86_FEATURE_SVSM (19*32+28) /* "svsm" SVSM present */
#define X86_FEATURE_HV_INUSE_WR_ALLOWED (19*32+30) /* Allow Write to in-use hypervisor-owned pages */
+#define X86_FEATURE_IBPB_ON_ENTRY (19*32+31) /* SEV-SNP IBPB on VM Entry */
/* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
#define X86_FEATURE_NO_NESTED_DATA_BP (20*32+ 0) /* No Nested Data Breakpoints */
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
index edde36097ddc..eebc65ec948f 100644
--- a/arch/x86/include/asm/svm.h
+++ b/arch/x86/include/asm/svm.h
@@ -306,6 +306,7 @@ static_assert((X2AVIC_4K_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AV
#define SVM_SEV_FEAT_ALTERNATE_INJECTION BIT(4)
#define SVM_SEV_FEAT_DEBUG_SWAP BIT(5)
#define SVM_SEV_FEAT_SECURE_TSC BIT(9)
+#define SVM_SEV_FEAT_IBPB_ON_ENTRY BIT(21)
#define VMCB_ALLOWED_SEV_FEATURES_VALID BIT_ULL(63)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index ea515cf41168..8a6d25db0c00 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3165,8 +3165,15 @@ void __init sev_hardware_setup(void)
cpu_feature_enabled(X86_FEATURE_NO_NESTED_DATA_BP))
sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
- if (sev_snp_enabled && tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
+ if (!sev_snp_enabled)
+ return;
+ /* the following feature bit checks are SNP specific */
+
+ if (tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;
+
+ if (cpu_feature_enabled(X86_FEATURE_IBPB_ON_ENTRY))
+ sev_supported_vmsa_features |= SVM_SEV_FEAT_IBPB_ON_ENTRY;
}
void sev_hardware_unsetup(void)
--
2.43.0
^ permalink raw reply related
* [PATCH v2 1/3] x86/sev: Allow IBPB-on-Entry feature for SNP guests
From: Kim Phillips @ 2026-02-03 22:24 UTC (permalink / raw)
To: linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Tom Lendacky, Michael Roth, Borislav Petkov,
Borislav Petkov, Naveen Rao, David Kaplan, Kim Phillips, stable
In-Reply-To: <20260203222405.4065706-1-kim.phillips@amd.com>
The SEV-SNP IBPB-on-Entry feature does not require a guest-side
implementation. The feature was added in Zen5 h/w, after the first
SNP Zen implementation, and thus was not accounted for when the
initial set of SNP features were added to the kernel.
In its abundant precaution, commit 8c29f0165405 ("x86/sev: Add SEV-SNP
guest feature negotiation support") included SEV_STATUS' IBPB-on-Entry
bit as a reserved bit, thereby masking guests from using the feature.
Allow guests to make use of IBPB-on-Entry when supported by the
hypervisor, as the bit is now architecturally defined and safe to
expose.
Fixes: 8c29f0165405 ("x86/sev: Add SEV-SNP guest feature negotiation support")
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Borislav Petkov (AMD) <bp@alien8.de>
Cc: Michael Roth <michael.roth@amd.com>
Cc: stable@kernel.org
Signed-off-by: Kim Phillips <kim.phillips@amd.com>
---
v2:
- Change title (Nikunj)
- Add reviews-by (Nikunj, Tom)
- Change the description to more generally explain what the patch does (Boris)
v1: https://lore.kernel.org/kvm/20260126224205.1442196-2-kim.phillips@amd.com/
arch/x86/boot/compressed/sev.c | 1 +
arch/x86/coco/sev/core.c | 1 +
arch/x86/include/asm/msr-index.h | 5 ++++-
3 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
index c8c1464b3a56..2b639703b8dd 100644
--- a/arch/x86/boot/compressed/sev.c
+++ b/arch/x86/boot/compressed/sev.c
@@ -188,6 +188,7 @@ bool sev_es_check_ghcb_fault(unsigned long address)
MSR_AMD64_SNP_RESERVED_BIT13 | \
MSR_AMD64_SNP_RESERVED_BIT15 | \
MSR_AMD64_SNP_SECURE_AVIC | \
+ MSR_AMD64_SNP_RESERVED_BITS19_22 | \
MSR_AMD64_SNP_RESERVED_MASK)
#ifdef CONFIG_AMD_SECURE_AVIC
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 9ae3b11754e6..13f608117411 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -122,6 +122,7 @@ static const char * const sev_status_feat_names[] = {
[MSR_AMD64_SNP_VMSA_REG_PROT_BIT] = "VMSARegProt",
[MSR_AMD64_SNP_SMT_PROT_BIT] = "SMTProt",
[MSR_AMD64_SNP_SECURE_AVIC_BIT] = "SecureAVIC",
+ [MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT] = "IBPBOnEntry",
};
/*
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 4d3566bb1a93..9016a6b00bc7 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -735,7 +735,10 @@
#define MSR_AMD64_SNP_SMT_PROT BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
#define MSR_AMD64_SNP_SECURE_AVIC_BIT 18
#define MSR_AMD64_SNP_SECURE_AVIC BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
-#define MSR_AMD64_SNP_RESV_BIT 19
+#define MSR_AMD64_SNP_RESERVED_BITS19_22 GENMASK_ULL(22, 19)
+#define MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT 23
+#define MSR_AMD64_SNP_IBPB_ON_ENTRY BIT_ULL(MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT)
+#define MSR_AMD64_SNP_RESV_BIT 24
#define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
#define MSR_AMD64_SAVIC_CONTROL 0xc0010138
#define MSR_AMD64_SAVIC_EN_BIT 0
--
2.43.0
^ permalink raw reply related
* [PATCH v2 0/3] KVM: SEV: Add support for IBPB-on-Entry
From: Kim Phillips @ 2026-02-03 22:24 UTC (permalink / raw)
To: linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Tom Lendacky, Michael Roth, Borislav Petkov,
Borislav Petkov, Naveen Rao, David Kaplan, Kim Phillips
AMD EPYC 5th generation and above processors support IBPB-on-Entry
for SNP guests. By invoking an Indirect Branch Prediction Barrier
(IBPB) on VMRUN, old indirect branch predictions are prevented
from influencing indirect branches within the guest.
The first patch is guest-side support which unmasks the Zen5+ feature
bit to allow kernel guests to set the feature.
The second patch is host-side support that checks the CPUID and
then sets the feature bit in the VMSA supported features mask.
The third patch is a trivial #define rename that was a result of
the review discussion from v1's 2/2, to clarify SEV features
that are implemented in the guest.
Based on https://github.com/kvm-x86/linux kvm-x86/next
(currently v6.19-rc6-182-ge944fe2c09f4).
This v2 series now also available here:
https://github.com/AMDESE/linux/tree/ibpb-on-entry-latest
Advance qemu bits (to add ibpb-on-entry=on/off switch) available here:
https://github.com/AMDESE/qemu/tree/ibpb-on-entry-latest
Qemu bits will be posted upstream once kernel bits are merged.
They depend on Naveen Rao's "target/i386: SEV: Add support for
enabling VMSA SEV features":
https://lore.kernel.org/qemu-devel/cover.1761648149.git.naveen@kernel.org/
---
v2:
- Change first patch's title (Nikunj)
- Add reviews-by (Nikunj, Tom)
- Change second patch's description to more generally explain what the patch does (Boris)
- Add new, third patch renaming SNP_FEATURES_PRESENT->SNP_FEATURES_IMPL
v1: https://lore.kernel.org/kvm/20260126224205.1442196-1-kim.phillips@amd.com/
Kim Phillips (3):
x86/sev: Allow IBPB-on-Entry feature for SNP guests
KVM: SEV: Add support for IBPB-on-Entry
x86/sev: Rename SNP_FEATURES_PRESENT->SNP_FEATURES_IMPL
arch/x86/boot/compressed/sev.c | 7 ++++---
arch/x86/coco/sev/core.c | 1 +
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/include/asm/msr-index.h | 5 ++++-
arch/x86/include/asm/svm.h | 1 +
arch/x86/kvm/svm/sev.c | 9 ++++++++-
6 files changed, 19 insertions(+), 5 deletions(-)
base-commit: e944fe2c09f405a2e2d147145c9b470084bc4c9a
--
2.43.0
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Huang, Kai @ 2026-02-03 21:34 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
mingo@redhat.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
ackerleytng@google.com, linux-kernel@vger.kernel.org,
Yamahata, Isaku, sagis@google.com, tglx@kernel.org,
Edgecombe, Rick P, bp@alien8.de, Annapurve, Vishal,
x86@kernel.org
In-Reply-To: <aYJVRQMW8yeTkRxR@google.com>
On Tue, 2026-02-03 at 12:06 -0800, Sean Christopherson wrote:
> On Tue, Feb 03, 2026, Kai Huang wrote:
> > On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > > Pass a pointer to iter->old_spte, not simply its value, when setting an
> > > external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> > > will be updated if the cmpxchg64 to freeze the mirror SPTE fails. The bug
> > > is currently benign as TDX is mutualy exclusive with all paths that do
> > > "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
> > >
> > > Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> >
> > Reviewed-by: Kai Huang <kai.huang@intel.com>
> >
> > Btw, do we need to cc stable?
>
> Probably not? The bug is benign until dirty logging comes along, and if someone
> backports that support (if it ever manifests) to an older kernel, it's firmly
> that person's responsibility to pick up dependencies like this.
Makes sense. :-)
^ permalink raw reply
* Re: [RFC PATCH v5 19/45] KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page allocator
From: Huang, Kai @ 2026-02-03 21:29 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
mingo@redhat.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
ackerleytng@google.com, linux-kernel@vger.kernel.org,
Yamahata, Isaku, sagis@google.com, tglx@kernel.org,
Edgecombe, Rick P, bp@alien8.de, Annapurve, Vishal,
x86@kernel.org
In-Reply-To: <aYJWvKagesT3FPfI@google.com>
On Tue, 2026-02-03 at 12:12 -0800, Sean Christopherson wrote:
> On Tue, Feb 03, 2026, Kai Huang wrote:
> > On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > > Extend "struct kvm_mmu_memory_cache" to support a custom page allocator
> > > so that x86's TDX can update per-page metadata on allocation and free().
> > >
> > > Name the allocator page_get() to align with __get_free_page(), e.g. to
> > > communicate that it returns an "unsigned long", not a "struct page", and
> > > to avoid collisions with macros, e.g. with alloc_page.
> > >
> > > Suggested-by: Kai Huang <kai.huang@intel.com>
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> >
> > I thought it could be more generic for allocating an object, but not just a
> > page.
> >
> > E.g., I thought we might be able to use it to allocate a structure which has
> > "pair of DPAMT pages" so it could be assigned to 'struct kvm_mmu_page'. But
> > it seems you abandoned this idea. May I ask why? Just want to understand
> > the reasoning here.
>
> Because that requires more complexity and there's no known use case, and I don't
> see an obvious way for a use case to come along. All of the motiviations for a
> custom allocation scheme that I can think of apply only to full pages, or fit
> nicely in a kmem_cache.
>
> Specifically, the "cache" logic is already bifurcated between "kmem_cache' and
> "page" usage. Further splitting the "page" case doesn't require modifications to
> the "kmem_cache" case, whereas providing a fully generic solution would require
> additional changes, e.g. to handle this code:
>
> page = (void *)__get_free_page(gfp_flags);
> if (page && mc->init_value)
> memset64(page, mc->init_value, PAGE_SIZE / sizeof(u64));
>
> It certainly wouldn't be much complexity, but this code is already a bit awkward,
> so I don't think it makes sense to add support for something that will probably
> never be used.
For this particular piece of code, we can add a helper for allocating normal
page table pages, get rid of mc->init_value completely and hook mc-
>page_get() to that helper.
A bonus is we can then call that helper in all places when KVM needs to
allocate a page for normal page table instead of just calling
get_zerod_pages() directly, e.g., like the one in
tdp_mmu_alloc_sp_for_split(), so that we can have a consistent way for
allocating normal page table pages.
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Huang, Kai @ 2026-02-03 21:18 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
mingo@redhat.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
ackerleytng@google.com, linux-kernel@vger.kernel.org,
Yamahata, Isaku, sagis@google.com, tglx@kernel.org,
Edgecombe, Rick P, bp@alien8.de, Annapurve, Vishal,
x86@kernel.org
In-Reply-To: <aYJX3usu7FzPrFWa@google.com>
On Tue, 2026-02-03 at 12:17 -0800, Sean Christopherson wrote:
> On Tue, Feb 03, 2026, Kai Huang wrote:
> > On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > > int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > > u64 mirror_spte);
> > > -
> > > - /* Update external page tables for page table about to be freed. */
> > > void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> > > struct kvm_mmu_page *sp);
> > > -
> > > - /* Update external page table from spte getting removed, and flush TLB. */
> >
> > The above two comments are still useful to me.
> >
> > Not sure why do you want to remove them, especially in _this_ patch?
>
> My intent was to replace the individual comments with a more generic comment for
> all of the "external" hooks. For things like "and flush TLB", IMO those comments
> belong at the call sites, not at this point. E.g. _KVM_ doesn't require a TLB
> flush in all cases. And so for the definition of the hooks, I would prefer a more
> generic comment, so that if there are details that matter to the usage, they are
> documented there.
I see. You actually mentioned "propagate changes in mirror page tables to
the external pages" in the new comment, so all make sense to me now.
^ permalink raw reply
* Re: [RFC PATCH v5 19/45] KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page allocator
From: Sean Christopherson @ 2026-02-03 21:17 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: Kai Huang, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Xiaoyao Li, Yan Y Zhao, dave.hansen@linux.intel.com,
kas@kernel.org, mingo@redhat.com, binbin.wu@linux.intel.com,
pbonzini@redhat.com, Isaku Yamahata, ackerleytng@google.com,
linux-kernel@vger.kernel.org, sagis@google.com, tglx@kernel.org,
bp@alien8.de, Vishal Annapurve, x86@kernel.org
In-Reply-To: <c01b2f81e025dd38be90d3820260c488c7eb22ce.camel@intel.com>
On Tue, Feb 03, 2026, Rick P Edgecombe wrote:
> On Tue, 2026-02-03 at 12:12 -0800, Sean Christopherson wrote:
> > > E.g., I thought we might be able to use it to allocate a structure which has
> > > "pair of DPAMT pages" so it could be assigned to 'struct kvm_mmu_page'. But
> > > it seems you abandoned this idea. May I ask why? Just want to understand
> > > the reasoning here.
> >
> > Because that requires more complexity and there's no known use case, and I
> > don't see an obvious way for a use case to come along. All of the
> > motiviations for a custom allocation scheme that I can think of apply only to
> > full pages, or fit nicely in a kmem_cache.
> >
> > Specifically, the "cache" logic is already bifurcated between "kmem_cache' and
> > "page" usage. Further splitting the "page" case doesn't require modifications
> > to the "kmem_cache" case, whereas providing a fully generic solution would
> > require additional changes, e.g. to handle this code:
> >
> > page = (void *)__get_free_page(gfp_flags);
> > if (page && mc->init_value)
> > memset64(page, mc->init_value, PAGE_SIZE / sizeof(u64));
> >
> > It certainly wouldn't be much complexity, but this code is already a bit
> > awkward, so I don't think it makes sense to add support for something that
> > will probably never be used.
>
> The thing that the design needlessly works around is that we can rely on that
> there are only two DPAMT pages per 2MB range. We don't need the dynamic page
> count allocations.
>
> This means we don't need to pass around the list of pages that lets arch/x86
> take as many pages as it needs. We can maybe just pass in a struct like Kai was
> suggesting to the get/put helpers. So I was in the process of trying to morph
> this series in that direction to get rid of the complexity resulting from the
> dynamic assumption.
>
> This was what I had done in response to v4 discussions, so now retrofitting it
> into this new ops scheme. Care to warn me off of this before I have something to
> show?
That's largely orthogonal to this change. This change is about preparing the
DPAMT when S-EPT page is allocated versus being installed. The fact that DPAMT
requires at most two pages versus a more dynamic maximum is irrelevant.
The caches aren't about dynamic sizes (though they play nicely with them), they're
about:
(a) not having to deal with allocating under spinlock
(b) not having to free memory that goes unused (for a single page fault)
(c) batching allocations for performance reasons (with the caveat that I doubt
anyone has measured the performance impact in many, many years).
None of those talking points change at all if KVM needs to provide 2 pages versus
N pages. The max number of pages needed for page tables is pretty much the same
thing as DPAMT, just with a higher max (4/5 vs. 2). In both cases, the allocated
pages may or may not be consumed for any given fault.
For the leaf pages (including the hugepage splitting cases), which don't utilize
KVM's kvm_mmu_memory_cache, I wouldn't expect the KVM details to change all that
much. In fact, they shouldn't change at all, because tracking 2 pages versus N
pages in "struct tdx_pamt_cache" is a detail that is 100% buried in the TDX subsystem
(which was pretty much the entire goal of my design).
Though maybe I'm misunderstanding what you have in mind?
^ permalink raw reply
* Re: [RFC PATCH v5 19/45] KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page allocator
From: Edgecombe, Rick P @ 2026-02-03 20:33 UTC (permalink / raw)
To: seanjc@google.com, Huang, Kai
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
mingo@redhat.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
Yamahata, Isaku, ackerleytng@google.com,
linux-kernel@vger.kernel.org, sagis@google.com, tglx@kernel.org,
bp@alien8.de, Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aYJWvKagesT3FPfI@google.com>
On Tue, 2026-02-03 at 12:12 -0800, Sean Christopherson wrote:
> > E.g., I thought we might be able to use it to allocate a structure which has
> > "pair of DPAMT pages" so it could be assigned to 'struct kvm_mmu_page'. But
> > it seems you abandoned this idea. May I ask why? Just want to understand
> > the reasoning here.
>
> Because that requires more complexity and there's no known use case, and I
> don't see an obvious way for a use case to come along. All of the
> motiviations for a custom allocation scheme that I can think of apply only to
> full pages, or fit nicely in a kmem_cache.
>
> Specifically, the "cache" logic is already bifurcated between "kmem_cache' and
> "page" usage. Further splitting the "page" case doesn't require modifications
> to the "kmem_cache" case, whereas providing a fully generic solution would
> require additional changes, e.g. to handle this code:
>
> page = (void *)__get_free_page(gfp_flags);
> if (page && mc->init_value)
> memset64(page, mc->init_value, PAGE_SIZE / sizeof(u64));
>
> It certainly wouldn't be much complexity, but this code is already a bit
> awkward, so I don't think it makes sense to add support for something that
> will probably never be used.
The thing that the design needlessly works around is that we can rely on that
there are only two DPAMT pages per 2MB range. We don't need the dynamic page
count allocations.
This means we don't need to pass around the list of pages that lets arch/x86
take as many pages as it needs. We can maybe just pass in a struct like Kai was
suggesting to the get/put helpers. So I was in the process of trying to morph
this series in that direction to get rid of the complexity resulting from the
dynamic assumption.
This was what I had done in response to v4 discussions, so now retrofitting it
into this new ops scheme. Care to warn me off of this before I have something to
show?
^ permalink raw reply
* Re: [RFC PATCH v5 11/45] x86/tdx: Add helpers to check return status codes
From: Sean Christopherson @ 2026-02-03 20:32 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: Dave Hansen, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Kai Huang, Xiaoyao Li, Yan Y Zhao, dave.hansen@linux.intel.com,
kas@kernel.org, binbin.wu@linux.intel.com, mingo@redhat.com,
pbonzini@redhat.com, ackerleytng@google.com,
linux-kernel@vger.kernel.org, Isaku Yamahata, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Vishal Annapurve, x86@kernel.org
In-Reply-To: <8c5aca4bacb31475a510e6a109956e7fa4a63de5.camel@intel.com>
On Fri, Jan 30, 2026, Rick P Edgecombe wrote:
> On Thu, 2026-01-29 at 12:35 -0800, Sean Christopherson wrote:
> > On Thu, Jan 29, 2026, Dave Hansen wrote:
> > > On 1/28/26 17:14, Sean Christopherson wrote:
> > > ...
> > > > err = tdh_mng_vpflushdone(&kvm_tdx->td);
> > > > - if (err == TDX_FLUSHVP_NOT_DONE)
> > > > + if (IS_TDX_FLUSHVP_NOT_DONE(err))
> > > > goto out;
> > > > if (TDX_BUG_ON(err, TDH_MNG_VPFLUSHDONE, kvm)) {
> > >
> > > I really despise the non-csopeable, non-ctaggable, non-greppable names
> > > like this. Sometimes it's unavoidable. Is it really unavoidable here?
> > >
> > > Something like this is succinct enough and doesn't have any magic ##
> > > macro definitions:
> > >
> > > TDX_ERR_EQ(err, TDX_FLUSHVP_NOT_DONE)
>
> I like the editor friendliness. The only downside is that it puts the onus on
> the caller to make sure supported defines are passed into TDX_ERR_EQ().
Eh, that's easy enough to handle with a static_assert().
> Today there are a few special cases like IS_TDX_NON_RECOVERABLE().
Why bother with a wrapper for that one? It's a single bit, just test that bit.
For me, providing IS_TDX_NON_RECOVERABLE() is _more_ confusing, because it
suggests that there's a NON_RECOVERABLE error, when in fact (IIUC) it's more or
less a modifier.
> I don't know, I'm ok either way. I lean towards keeping it as in this patch
> because we already had an error code bit interpretation bug:
> https://lore.kernel.org/kvm/24d2f165-f854-4996-89cf-28d644c592a3@intel.com/
>
> So the centralization of bit interpretation seems like a real win.
>
> > FWIW, I have zero preference on this. I included the patch purely because it was
> > already there.
>
> Ha, actually we all had a long thread on this:
> https://lore.kernel.org/kvm/70484aa1b553ca250d893f80b2687b5d915e5309.camel@intel.com/
Oh, it's _that_ discussion :-)
What I meant was, "I don't have a strong preference between TDX_ERR_EQ() and
this patch". What I didn't like was tdx_operand_invalid(), because that reads
like a command and it's not at all clear that it's macro-like in behavior.
I'd vote for IS_TDX_ERR() over TDX_ERR_EQ(), but either works for me (as does
this patch).
> I see now that we closed it with you but never got Dave's final buy in.
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Sean Christopherson @ 2026-02-03 20:17 UTC (permalink / raw)
To: Kai Huang
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Rick P Edgecombe, ackerleytng@google.com,
sagis@google.com, Vishal Annapurve, linux-kernel@vger.kernel.org,
Yan Y Zhao, Xiaoyao Li, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Isaku Yamahata,
binbin.wu@linux.intel.com
In-Reply-To: <4fae16cdcc368d33f128c3a79c788b905b83ffe7.camel@intel.com>
On Tue, Feb 03, 2026, Kai Huang wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > u64 mirror_spte);
> > -
> > - /* Update external page tables for page table about to be freed. */
> > void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> > struct kvm_mmu_page *sp);
> > -
> > - /* Update external page table from spte getting removed, and flush TLB. */
>
> The above two comments are still useful to me.
>
> Not sure why do you want to remove them, especially in _this_ patch?
My intent was to replace the individual comments with a more generic comment for
all of the "external" hooks. For things like "and flush TLB", IMO those comments
belong at the call sites, not at this point. E.g. _KVM_ doesn't require a TLB
flush in all cases. And so for the definition of the hooks, I would prefer a more
generic comment, so that if there are details that matter to the usage, they are
documented there.
> > void (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> > u64 mirror_spte);
> >
> > +
>
> Unintentional change?
Ya.
>
> > bool (*has_wbinvd_exit)(void);
> >
> > u64 (*get_l2_tsc_offset)(struct kvm_vcpu *vcpu);
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 3911ac9bddfd..9b5a6861e2a4 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -6690,11 +6690,13 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> > vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
> > vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
> >
> > - vcpu->arch.mmu_shadow_page_cache.init_value =
> > - SHADOW_NONPRESENT_VALUE;
> > + vcpu->arch.mmu_shadow_page_cache.init_value = SHADOW_NONPRESENT_VALUE;
> > if (!vcpu->arch.mmu_shadow_page_cache.init_value)
> > vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
>
> Ditto. Not sure this adjustment is intentional?
Heh, I'm pretty sure it was intentional, but yeah, doesn't belong here.
^ permalink raw reply
* Re: [RFC PATCH v5 19/45] KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page allocator
From: Sean Christopherson @ 2026-02-03 20:12 UTC (permalink / raw)
To: Kai Huang
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Rick P Edgecombe, ackerleytng@google.com,
sagis@google.com, Vishal Annapurve, linux-kernel@vger.kernel.org,
Yan Y Zhao, Xiaoyao Li, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Isaku Yamahata,
binbin.wu@linux.intel.com
In-Reply-To: <de05853257e9cc66998101943f78a4b7e6e3d741.camel@intel.com>
On Tue, Feb 03, 2026, Kai Huang wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > Extend "struct kvm_mmu_memory_cache" to support a custom page allocator
> > so that x86's TDX can update per-page metadata on allocation and free().
> >
> > Name the allocator page_get() to align with __get_free_page(), e.g. to
> > communicate that it returns an "unsigned long", not a "struct page", and
> > to avoid collisions with macros, e.g. with alloc_page.
> >
> > Suggested-by: Kai Huang <kai.huang@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> I thought it could be more generic for allocating an object, but not just a
> page.
>
> E.g., I thought we might be able to use it to allocate a structure which has
> "pair of DPAMT pages" so it could be assigned to 'struct kvm_mmu_page'. But
> it seems you abandoned this idea. May I ask why? Just want to understand
> the reasoning here.
Because that requires more complexity and there's no known use case, and I don't
see an obvious way for a use case to come along. All of the motiviations for a
custom allocation scheme that I can think of apply only to full pages, or fit
nicely in a kmem_cache.
Specifically, the "cache" logic is already bifurcated between "kmem_cache' and
"page" usage. Further splitting the "page" case doesn't require modifications to
the "kmem_cache" case, whereas providing a fully generic solution would require
additional changes, e.g. to handle this code:
page = (void *)__get_free_page(gfp_flags);
if (page && mc->init_value)
memset64(page, mc->init_value, PAGE_SIZE / sizeof(u64));
It certainly wouldn't be much complexity, but this code is already a bit awkward,
so I don't think it makes sense to add support for something that will probably
never be used.
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Sean Christopherson @ 2026-02-03 20:06 UTC (permalink / raw)
To: Kai Huang
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Rick P Edgecombe, ackerleytng@google.com,
sagis@google.com, Vishal Annapurve, linux-kernel@vger.kernel.org,
Yan Y Zhao, Xiaoyao Li, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Isaku Yamahata,
binbin.wu@linux.intel.com
In-Reply-To: <1c4bdb3613ebaf65b5dcf9a2268b06fa0c5a6ef3.camel@intel.com>
On Tue, Feb 03, 2026, Kai Huang wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > Pass a pointer to iter->old_spte, not simply its value, when setting an
> > external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> > will be updated if the cmpxchg64 to freeze the mirror SPTE fails. The bug
> > is currently benign as TDX is mutualy exclusive with all paths that do
> > "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
> >
> > Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
>
> Btw, do we need to cc stable?
Probably not? The bug is benign until dirty logging comes along, and if someone
backports that support (if it ever manifests) to an older kernel, it's firmly
that person's responsibility to pick up dependencies like this.
^ permalink raw reply
* Re: [RFC PATCH v5 05/45] KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
From: Sean Christopherson @ 2026-02-03 20:05 UTC (permalink / raw)
To: Yan Zhao
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Vishal Annapurve, Ackerley Tng,
Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <aYHLlTPeo2fzh02y@yzhao56-desk.sh.intel.com>
On Tue, Feb 03, 2026, Yan Zhao wrote:
> On Wed, Jan 28, 2026 at 05:14:37PM -0800, Sean Christopherson wrote:
> > static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sptep,
> > gfn_t gfn, u64 *old_spte,
> > u64 new_spte, int level)
> > {
> > - bool was_present = is_shadow_present_pte(*old_spte);
> > - bool is_present = is_shadow_present_pte(new_spte);
> > - bool is_leaf = is_present && is_last_spte(new_spte, level);
> > - int ret = 0;
> > -
> > - KVM_BUG_ON(was_present, kvm);
> > + int ret;
> >
> > lockdep_assert_held(&kvm->mmu_lock);
> > +
> > + if (KVM_BUG_ON(is_shadow_present_pte(*old_spte), kvm))
> > + return -EIO;
> Why not move this check of is_shadow_present_pte() to tdx_sept_set_private_spte()
> as well?
The series gets there eventually, but as of this commit, @old_spte isn't plumbed
into tdx_sept_set_private_spte().
> Or also check !is_shadow_present_pte(new_spte) in TDP MMU?
Not sure I understand this suggestion.
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 5688c77616e3..30494f9ceb31 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1664,18 +1664,58 @@ static int tdx_mem_page_aug(struct kvm *kvm, gfn_t gfn,
> > return 0;
> > }
> >
> > +static struct page *tdx_spte_to_external_spt(struct kvm *kvm, gfn_t gfn,
> > + u64 new_spte, enum pg_level level)
> > +{
> > + struct kvm_mmu_page *sp = spte_to_child_sp(new_spte);
> > +
> > + if (KVM_BUG_ON(!sp->external_spt, kvm) ||
> > + KVM_BUG_ON(sp->role.level + 1 != level, kvm) ||
> > + KVM_BUG_ON(sp->gfn != gfn, kvm))
> > + return NULL;
> Could we remove the KVM_BUG_ON()s, and ...
>
> > + return virt_to_page(sp->external_spt);
> > +}
> > +
> > +static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn,
> > + enum pg_level level, u64 mirror_spte)
> > +{
> > + gpa_t gpa = gfn_to_gpa(gfn);
> > + u64 err, entry, level_state;
> > + struct page *external_spt;
> > +
> > + external_spt = tdx_spte_to_external_spt(kvm, gfn, mirror_spte, level);
> > + if (!external_spt)
> add a KVM_BUG_ON() here?
> It could save KVM_BUG_ON()s and have KVM_BUG_ON() match -EIO :)
We could, but I don't want to, because if we're going to bother with sanity checks,
I want the resulting WARNs to be precise. I.e. I want the WARN to capture *why*
tdx_spte_to_external_spt() failed, to make debug/triage easier.
> And as Rick also mentioned, better to remove external in external_spt, e.g.
> something like pt_page.
Yeah, maybe sept_spt?
> And mirror_spte --> new_spte?
Hmm, ya, I made that change later, but it can probably be shifted here.
> > - WARN_ON_ONCE(!is_shadow_present_pte(mirror_spte) ||
> > - (mirror_spte & VMX_EPT_RWX_MASK) != VMX_EPT_RWX_MASK);
> > + WARN_ON_ONCE((mirror_spte & VMX_EPT_RWX_MASK) != VMX_EPT_RWX_MASK);
> Also check this for tdx_sept_link_private_spt()?
Eh, we could, but I don't think it's necessary. make_nonleaf_spte() is hardcoded
to set full permissions (and I don't see that changing any time soon), whereas
leaf SPTE protections are much more dynamic.
^ permalink raw reply
* [PATCH v4 2/2] x86/sev: Add support to unaccept memory after hot-remove
From: Pratik R. Sampat @ 2026-02-03 17:49 UTC (permalink / raw)
To: linux-mm, linux-coco, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
In-Reply-To: <20260203174946.1198053-1-prsampat@amd.com>
Transition memory to the shared state during a hot-remove operation so
that it can be re-used by the hypervisor. This also applies when memory
is intended to be hotplugged back in later, as those pages will need to
be re-accepted after crossing the trust boundary.
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
arch/x86/coco/sev/core.c | 13 ++++++
arch/x86/include/asm/sev.h | 2 +
arch/x86/include/asm/unaccepted_memory.h | 15 ++++++
drivers/firmware/efi/unaccepted_memory.c | 59 ++++++++++++++++++++++++
include/linux/mm.h | 4 ++
mm/memory_hotplug.c | 2 +
6 files changed, 95 insertions(+)
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 9ae3b11754e6..63d8f44b76eb 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -703,6 +703,19 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
}
+void snp_unaccept_memory(phys_addr_t start, phys_addr_t end)
+{
+ unsigned long vaddr, npages;
+
+ if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+ return;
+
+ vaddr = (unsigned long)__va(start);
+ npages = (end - start) >> PAGE_SHIFT;
+
+ set_pages_state(vaddr, npages, SNP_PAGE_STATE_SHARED);
+}
+
static int vmgexit_ap_control(u64 event, struct sev_es_save_area *vmsa, u32 apic_id)
{
bool create = event != SVM_VMGEXIT_AP_DESTROY;
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 0e6c0940100f..3327de663793 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -514,6 +514,7 @@ bool snp_init(struct boot_params *bp);
void snp_dmi_setup(void);
int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call, struct svsm_attest_call *input);
void snp_accept_memory(phys_addr_t start, phys_addr_t end);
+void snp_unaccept_memory(phys_addr_t start, phys_addr_t end);
u64 snp_get_unsupported_features(u64 status);
u64 sev_get_status(void);
void sev_show_status(void);
@@ -623,6 +624,7 @@ static inline int snp_issue_svsm_attest_req(u64 call_id, struct svsm_call *call,
return -ENOTTY;
}
static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { }
+static inline void snp_unaccept_memory(phys_addr_t start, phys_addr_t end) { }
static inline u64 snp_get_unsupported_features(u64 status) { return 0; }
static inline u64 sev_get_status(void) { return 0; }
static inline void sev_show_status(void) { }
diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
index f5937e9866ac..91f01ad0ee03 100644
--- a/arch/x86/include/asm/unaccepted_memory.h
+++ b/arch/x86/include/asm/unaccepted_memory.h
@@ -18,6 +18,21 @@ static inline void arch_accept_memory(phys_addr_t start, phys_addr_t end)
}
}
+static inline void arch_unaccept_memory(phys_addr_t start, phys_addr_t end)
+{
+ /*
+ * TDX platforms do not require the guest to transition pages on remove
+ * rather expect the VMM to remove the unplugged memory from SEPT
+ */
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) {
+ return;
+ } else if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
+ snp_unaccept_memory(start, end);
+ } else {
+ panic("Cannot unaccept memory: unknown platform\n");
+ }
+}
+
static inline struct efi_unaccepted_memory *efi_get_unaccepted_table(void)
{
if (efi.unaccepted == EFI_INVALID_TABLE_ADDR)
diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index 359779133cb4..d11e7836200a 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -256,6 +256,65 @@ void accept_hotplug_memory(phys_addr_t start, unsigned long size)
spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
}
+/*
+ * Cold-plugged memory used with lazy acceptance may partially set pages to
+ * private. On removal, iterate through the bitmap to unaccept those ranges.
+ * For hotplug ranges beyond the bitmap, unaccept unconditionally.
+ */
+void unaccept_hotplug_memory(phys_addr_t start, unsigned long size)
+{
+ unsigned long range_start, range_end, bitrange_end;
+ phys_addr_t bitmap_end, end = start + size;
+ struct efi_unaccepted_memory *unaccepted;
+ u64 phys_base, unit_size;
+ unsigned long flags;
+
+ unaccepted = efi_get_unaccepted_table();
+ if (!unaccepted)
+ return;
+
+ phys_base = unaccepted->phys_base;
+ unit_size = unaccepted->unit_size;
+ bitmap_end = phys_base + unaccepted->size * unit_size * BITS_PER_BYTE;
+
+ /* Unaccept the entire hotplug range beyond the bitmap immediately */
+ if (start >= bitmap_end) {
+ arch_unaccept_memory(start, end);
+ return;
+ }
+
+ start = max(start, phys_base);
+ if (end < phys_base)
+ return;
+
+ /* Unaccept ranges when start is within the bitmap but end is beyond */
+ if (end > bitmap_end) {
+ arch_unaccept_memory(bitmap_end, end);
+ end = bitmap_end;
+ }
+
+ start -= phys_base;
+ end -= phys_base;
+
+ range_start = start / unit_size;
+ bitrange_end = DIV_ROUND_UP(end, unit_size);
+
+ /* Only unaccept memory that was previously accepted in the bitmap */
+ spin_lock_irqsave(&unaccepted_memory_lock, flags);
+ for_each_clear_bitrange_from(range_start, range_end, unaccepted->bitmap,
+ bitrange_end) {
+ unsigned long phys_start, phys_end;
+ unsigned long len = range_end - range_start;
+
+ phys_start = range_start * unit_size + phys_base;
+ phys_end = range_end * unit_size + phys_base;
+
+ arch_unaccept_memory(phys_start, phys_end);
+ bitmap_set(unaccepted->bitmap, range_start, len);
+ }
+ spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+}
+
#ifdef CONFIG_PROC_VMCORE
static bool unaccepted_memory_vmcore_pfn_is_ram(struct vmcore_cb *cb,
unsigned long pfn)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 2d3c1ea40606..49b194cddda7 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4505,6 +4505,7 @@ int set_anon_vma_name(unsigned long addr, unsigned long size,
bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size);
void accept_memory(phys_addr_t start, unsigned long size);
void accept_hotplug_memory(phys_addr_t start, unsigned long size);
+void unaccept_hotplug_memory(phys_addr_t start, unsigned long size);
#else
@@ -4522,6 +4523,9 @@ static inline void accept_hotplug_memory(phys_addr_t start, unsigned long size)
{
}
+static inline void unaccept_hotplug_memory(phys_addr_t start, unsigned long size)
+{
+}
#endif
static inline bool pfn_is_unaccepted_memory(unsigned long pfn)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 549ccfd190ee..21b87f2af930 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -2240,6 +2240,8 @@ static int try_remove_memory(u64 start, u64 size)
mem_hotplug_begin();
+ unaccept_hotplug_memory(start, size);
+
rc = memory_blocks_have_altmaps(start, size);
if (rc < 0) {
mem_hotplug_done();
--
2.52.0
^ permalink raw reply related
* [PATCH v4 1/2] mm/memory_hotplug: Add support to accept memory during hot-add
From: Pratik R. Sampat @ 2026-02-03 17:49 UTC (permalink / raw)
To: linux-mm, linux-coco, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
In-Reply-To: <20260203174946.1198053-1-prsampat@amd.com>
Confidential computing guests require memory to be accepted before use.
The unaccepted memory bitmap maintained by firmware does not track
most hotplugged memory ranges apart from system memory annotated to be
cold plugged at boot.
Explicitly validate and transition the newly added memory to a private
state, making it usable by the guest.
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
drivers/firmware/efi/unaccepted_memory.c | 47 ++++++++++++++++++++++++
include/linux/mm.h | 5 +++
mm/memory_hotplug.c | 2 +
3 files changed, 54 insertions(+)
diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index c2c067eff634..359779133cb4 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -209,6 +209,53 @@ bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size)
return ret;
}
+/*
+ * Unaccepted memory bitmap only covers initial boot memory and not the
+ * hotpluggable range that is part of SRAT parsing. However, some initial memory
+ * with the attribute EFI_MEMORY_HOT_PLUGGABLE can indicate boot time memory
+ * that can be hot-removed. Hence post acceptance, only for that range update
+ * the unaccepted bitmap to reflect this change.
+ */
+void accept_hotplug_memory(phys_addr_t start, unsigned long size)
+{
+ struct efi_unaccepted_memory *unaccepted;
+ unsigned long range_start, range_len;
+ phys_addr_t end = start + size;
+ u64 phys_base, unit_size;
+ unsigned long flags;
+
+ unaccepted = efi_get_unaccepted_table();
+ if (!unaccepted)
+ return;
+
+ /* Accept hotplug range unconditionally */
+ arch_accept_memory(start, end);
+
+ phys_base = unaccepted->phys_base;
+ unit_size = unaccepted->unit_size;
+
+ /* Only update bitmap for the region that is represented by it */
+ if (start >= phys_base + unaccepted->size * unit_size * BITS_PER_BYTE)
+ return;
+
+ start = max(start, phys_base);
+ if (end < phys_base)
+ return;
+
+ start -= phys_base;
+ end -= phys_base;
+
+ /* Make sure not to overrun the bitmap */
+ end = min(end, unaccepted->size * unit_size * BITS_PER_BYTE);
+
+ range_start = start / unit_size;
+ range_len = DIV_ROUND_UP(end, unit_size) - range_start;
+
+ spin_lock_irqsave(&unaccepted_memory_lock, flags);
+ bitmap_clear(unaccepted->bitmap, range_start, range_len);
+ spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+}
+
#ifdef CONFIG_PROC_VMCORE
static bool unaccepted_memory_vmcore_pfn_is_ram(struct vmcore_cb *cb,
unsigned long pfn)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 15076261d0c2..2d3c1ea40606 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4504,6 +4504,7 @@ int set_anon_vma_name(unsigned long addr, unsigned long size,
bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size);
void accept_memory(phys_addr_t start, unsigned long size);
+void accept_hotplug_memory(phys_addr_t start, unsigned long size);
#else
@@ -4517,6 +4518,10 @@ static inline void accept_memory(phys_addr_t start, unsigned long size)
{
}
+static inline void accept_hotplug_memory(phys_addr_t start, unsigned long size)
+{
+}
+
#endif
static inline bool pfn_is_unaccepted_memory(unsigned long pfn)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index a63ec679d861..549ccfd190ee 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1567,6 +1567,8 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
if (!strcmp(res->name, "System RAM"))
firmware_map_add_hotplug(start, start + size, "System RAM");
+ accept_hotplug_memory(start, size);
+
/* device_online() will take the lock when calling online_pages() */
mem_hotplug_done();
--
2.52.0
^ permalink raw reply related
* [PATCH v4 0/2] SEV-SNP Unaccepted Memory Hotplug
From: Pratik R. Sampat @ 2026-02-03 17:49 UTC (permalink / raw)
To: linux-mm, linux-coco, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
Guest memory hot-plug/remove via the QEMU monitor is used by virtual
machines to dynamically scale the memory capacity of a system with
virtually zero downtime to the guest. For confidential VMs, memory has
to be first accepted before it can be used. Add support to accept
memory that has been hot-added and revert back it's state for
hypervisors to be able to use the pages during hot-remove.
Usage (for SNP guests)
----------------------
Step1: Spawn a QEMU SNP guest with the additional parameter of slots and
maximum possible memory, along with the initial memory as below:
"-m X,slots=Y,maxmem=Z".
Step2: Once the guest is booted, launch the qemu monitor and hotplug
the memory as follows:
(qemu) object_add memory-backend-memfd,id=mem1,size=1G
(qemu) device_add pc-dimm,id=dimm1,memdev=mem1
Memory is accepted up-front when added to the guest.
If using auto-onlining by either:
a) echo online > /sys/devices/system/memory/auto_online_blocks, OR
b) enable CONFIG_MHP_DEFAULT_ONLINE_TYPE_* while compiling kernel
Memory should show up automatically.
Otherwise, memory can also be onlined by echoing 1 to the newly added
blocks in: /sys/devices/system/memory/memoryXX/online
Step3: memory can be hot-removed via the qemu monitor using:
(qemu) device_remove dimm1
(qemu) object_remove mem1
Tip: Enable the kvm_convert_memory event in QEMU to observe memory
conversions between private and shared during hotplug/remove.
The series is based on
git.kernel.org/pub/scm/virt/kvm/kvm.git next
Comments and feedback appreciated!
Changelog Patch v3..v4:
-----------------------
https://lore.kernel.org/all/20260128204105.508855-1-prsampat@amd.com/
1. Unconditionally accept all hotplug pages and set bitmap for the ones
that are in the bit range (Kiryl)
2. Fix implementation similarly for unacceptance and merge
unaccept_memory() implementation within unaccept_hotplug_memory()
3. Use max()/min() when clamping memory ranges to operate in the
bitmap (Andrew)
4. Fall through arch_unaccept_memory() for TDX platforms (Kiryl).
However, retain the panic() similar to arch_accept_memory() since
it is a can't-happen scenario for other archs.
Changelog Patch v2..v3:
-----------------------
https://lore.kernel.org/all/20260112202300.43546-1-prsampat@amd.com/
1. Account for cold-plugged memory at boot and introduce proper handling
of the unaccepted bitmap during both hotplug and remove. (Kiryl)
2. #include<asm/unaccepted_memory.h> within memory_hotplug caused build
failures on non-x86 archs (Andrew). Instead of introducing
#if-deffery to have arch agnostic fall throughs, create hotplug
specific helper functions since we now also need to take care of
managing the bitmaps due to 1. as well.
Changelog RFC..Patch v2:
------------------------
https://lore.kernel.org/all/20251125175753.1428857-1-prsampat@amd.com/
Based on feedback from the RFC, reworked the series to accept memory
upfront on hotplug. This is done for two reasons:
1. Avoids modifying the unaccepted bitmap. Extending the bitmap would
require either:
* Dynamically allocating the bitmap, which would need changes to EFI
struct definitions, or
* Pre-allocating a larger bitmap to accommodate hotpluggable memory.
This poses challenges since e820 is parsed before SRAT, which
contains the actual memory ranges information.
2. There are currently no known use-cases that would benefit from lazy
acceptance of hotplugged ranges which warrants this additional
complexity.
Pratik R. Sampat (2):
mm/memory_hotplug: Add support to accept memory during hot-add
x86/sev: Add support to unaccept memory after hot-remove
arch/x86/coco/sev/core.c | 13 +++
arch/x86/include/asm/sev.h | 2 +
arch/x86/include/asm/unaccepted_memory.h | 15 ++++
drivers/firmware/efi/unaccepted_memory.c | 106 +++++++++++++++++++++++
include/linux/mm.h | 9 ++
mm/memory_hotplug.c | 4 +
6 files changed, 149 insertions(+)
--
2.52.0
^ permalink raw reply
* SVSM Development Call February 4, 2026
From: Jörg Rödel @ 2026-02-03 17:06 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
Here is the call for agenda items for this weeks SVSM development call. Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.
We will use the LF Zoom instance. Details of the meeting can be found in our
governance repository at:
https://github.com/coconut-svsm/governance
The link to the COCONUT-SVSM calendar is:
https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week
The meeting will be recorded and the recording eventually published.
Regards,
Jörg
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Dave Hansen @ 2026-02-03 16:12 UTC (permalink / raw)
To: Sean Christopherson, Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, rick.p.edgecombe, kas, dave.hansen,
vishal.l.verma, Farrah Chen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <aYIXFmT-676oN6j0@google.com>
On 2/3/26 07:41, Sean Christopherson wrote:
>> It seems like a bug, or at least a P-SEAMLDR implementation issue the
>> needs to get fixed.
> Yeah, 'tis odd behavior. IMO, that's all the more reason the TDX subsystem should
> hide the quirk from the rest of the kernel.
>
> [*] https://lore.kernel.org/all/20251010220403.987927-1-seanjc@google.com
For now, I say treat it as a bug. Don't deal with it in the series.
If it truly is unfixable P-SEAMLDR behavior, then Intel can issue and
erratum for it and we can add (ugly) code to follow Sean's suggestion.
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Sean Christopherson @ 2026-02-03 15:41 UTC (permalink / raw)
To: Chao Gao
Cc: Dave Hansen, linux-coco, linux-kernel, kvm, x86, reinette.chatre,
ira.weiny, kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve,
paulmck, nik.borisov, zhenzhong.duan, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <aYHmUCLRYL+JX1ga@intel.com>
On Tue, Feb 03, 2026, Chao Gao wrote:
> >>> I'd be shocked if this is the one and only place in the whole kernel
> >>> that can unceremoniously zap VMX state.
> >>>
> >>> I'd *bet* that you don't really need to do the vmptrld and that KVM can
> >>> figure it out because it can vmptrld on demand anyway. Something along
> >>> the lines of:
> >>>
> >>> local_irq_disable();
> >>> list_for_each(handwaving...)
> >>> vmcs_clear();
> >>> ret = seamldr_prerr(fn, args);
> >>> local_irq_enable();
> >>>
> >>> Basically, zap this CPU's vmcs state and then make KVM reload it at some
> >>> later time.
> >>
> >> The idea is feasible. But just calling vmcs_clear() won't work. We need to
> >> reset all the tracking state associated with each VMCS. We should call
> >> vmclear_local_loaded_vmcss() instead, similar to what's done before VMXOFF.
> >>
> >>>
> >>> I'm sure Sean and Paolo will tell me if I'm crazy.
> >>
> >> To me, this approach needs more work since we need to either move
> >> vmclear_local_loaded_vmcss() to the kernel or allow KVM to register a callback.
> >>
> >> I don't think it's as straightforward as just doing the save/restore.
> >
> >Could you please just do me a favor and spend 20 minutes to see what
> >this looks like in practice and if the KVM folks hate it?
I hate it :-)
> Sure. KVM tracks the current VMCS and only executes vmptrld for a new VMCS if
> it differs from the current one. See arch/x86/kvm/vmx/vmx.c::vmx_vcpu_load_vmcs()
>
> prev = per_cpu(current_vmcs, cpu);
> if (prev != vmx->loaded_vmcs->vmcs) {
> per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
> vmcs_load(vmx->loaded_vmcs->vmcs);
> }
>
> By resetting current_vmcs to NULL during P-SEAMLDR calls, KVM is forced to do a
> vmptrld on the next VMCS load. So, we can implement seamldr_call() as:
>
> static int seamldr_call(u64 fn, struct tdx_module_args *args)
> {
> int ret;
>
> WARN_ON_ONCE(!is_seamldr_call(fn));
>
> /*
> * Serialize P-SEAMLDR calls since only a single CPU is allowed to
> * interact with P-SEAMLDR at a time.
> *
> * P-SEAMLDR calls invalidate the current VMCS. Exclude KVM access to
> * the VMCS by disabling interrupts. This is not safe against VMCS use
> * in NMIs, but there are none of those today.
> *
> * Set the per-CPU current_vmcs cache to NULL to force KVM to reload
> * the VMCS.
> */
> guard(raw_spinlock_irqsave)(&seamldr_lock);
> ret = seamcall_prerr(fn, args);
> this_cpu_write(current_vmcs, NULL);
>
> return ret;
> }
>
> This requires moving the per-CPU current_vmcs from KVM to the kernel, which
> should be trivial with Sean's VMXON series.
Trivial in code, but I am very strongly opposed to moving current_vmcs out of KVM.
As stated in the cover letter of the initial VMXON RFC[*]:
: Emphasis on "only", because leaving VMCS tracking and clearing in KVM is
: another key difference from Xin's series. The "light bulb" moment on that
: front is that TDX isn't a hypervisor, and isn't trying to be a hypervisor.
: Specifically, TDX should _never_ have it's own VMCSes (that are visible to the
: host; the TDX-Module has it's own VMCSes to do SEAMCALL/SEAMRET), and so there
: is simply no reason to move that functionality out of KVM.
TDX's "use" of a VMCS should be completely transparent to KVM, because otherwise
we are stepping over that line that says the TDX subsystem isn't a hypervisor.
I also really, really don't want to add a super special case rule to KVM's VMCS
tracking logic.
After reading through the rest of this discussion, I'm doubling down on that
stance, because I agree that this is decidely odd behavior.
Pulling in two other threads from this discussion:
On Wed, Jan 28, 2026 at 3:05 PM Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/23/26 06:55, Chao Gao wrote:
> > 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.
>
> That seems pretty mean.
>
> This is going to need a lot more justification for why this is an
> absolutely necessary requirement.
>
> KVM folks, are you OK with this?
As above, I'm definitely not ok with the current VMCS being zapped out from
underneath KVM. As to whether or not I'm ok with the P-SEAMLDR behavior, I would
say that's more of a question for you, as it will fall on the TDX subsytem to
workaround the bug/quirk.
On Fri, Jan 30, 2026 at 8:23 AM Dave Hansen <dave.hansen@intel.com> wrote:
> On 1/30/26 00:08, Chao Gao wrote:
> > AFAIK, this is a CPU implementation issue. The actual requirement is to
> > evict (flush and invalidate) all VMCSs __cached in SEAM mode__, but big
> > cores implement this by evicting the __entire__ VMCS cache. So, the
> > current VMCS is invalidated and cleared.
>
> But why is this a P-SEAMLDR thing and not a TDX module thing?
My guess is that it's because the P-SEAMLDR code loads and prepares the new TDX-
Module by constructing the VMCS used for SEAMCALL using direct writes to memory
(unless that TDX behavior has changed in the last few years). And so it needs
to ensure that in-memory representation is synchronized with the VMCS cache.
Hmm, but that doesn't make sense _if_ it really truly is SEAMRET that does the VMCS
cache invalidation, because flushing the VMCS cache would ovewrite the in-memory
state.
> It seems like a bug, or at least a P-SEAMLDR implementation issue the
> needs to get fixed.
Yeah, 'tis odd behavior. IMO, that's all the more reason the TDX subsystem should
hide the quirk from the rest of the kernel.
[*] https://lore.kernel.org/all/20251010220403.987927-1-seanjc@google.com
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-02-03 12:15 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <fedb3192-e68c-423c-93b2-a4dc2f964148@intel.com>
>>> I'd be shocked if this is the one and only place in the whole kernel
>>> that can unceremoniously zap VMX state.
>>>
>>> I'd *bet* that you don't really need to do the vmptrld and that KVM can
>>> figure it out because it can vmptrld on demand anyway. Something along
>>> the lines of:
>>>
>>> local_irq_disable();
>>> list_for_each(handwaving...)
>>> vmcs_clear();
>>> ret = seamldr_prerr(fn, args);
>>> local_irq_enable();
>>>
>>> Basically, zap this CPU's vmcs state and then make KVM reload it at some
>>> later time.
>>
>> The idea is feasible. But just calling vmcs_clear() won't work. We need to
>> reset all the tracking state associated with each VMCS. We should call
>> vmclear_local_loaded_vmcss() instead, similar to what's done before VMXOFF.
>>
>>>
>>> I'm sure Sean and Paolo will tell me if I'm crazy.
>>
>> To me, this approach needs more work since we need to either move
>> vmclear_local_loaded_vmcss() to the kernel or allow KVM to register a callback.
>>
>> I don't think it's as straightforward as just doing the save/restore.
>
>Could you please just do me a favor and spend 20 minutes to see what
>this looks like in practice and if the KVM folks hate it?
Sure. KVM tracks the current VMCS and only executes vmptrld for a new VMCS if
it differs from the current one. See arch/x86/kvm/vmx/vmx.c::vmx_vcpu_load_vmcs()
prev = per_cpu(current_vmcs, cpu);
if (prev != vmx->loaded_vmcs->vmcs) {
per_cpu(current_vmcs, cpu) = vmx->loaded_vmcs->vmcs;
vmcs_load(vmx->loaded_vmcs->vmcs);
}
By resetting current_vmcs to NULL during P-SEAMLDR calls, KVM is forced to do a
vmptrld on the next VMCS load. So, we can implement seamldr_call() as:
static int seamldr_call(u64 fn, struct tdx_module_args *args)
{
int ret;
WARN_ON_ONCE(!is_seamldr_call(fn));
/*
* Serialize P-SEAMLDR calls since only a single CPU is allowed to
* interact with P-SEAMLDR at a time.
*
* P-SEAMLDR calls invalidate the current VMCS. Exclude KVM access to
* the VMCS by disabling interrupts. This is not safe against VMCS use
* in NMIs, but there are none of those today.
*
* Set the per-CPU current_vmcs cache to NULL to force KVM to reload
* the VMCS.
*/
guard(raw_spinlock_irqsave)(&seamldr_lock);
ret = seamcall_prerr(fn, args);
this_cpu_write(current_vmcs, NULL);
return ret;
}
This requires moving the per-CPU current_vmcs from KVM to the kernel, which
should be trivial with Sean's VMXON series.
And I tested this. Without this_cpu_write(), vmread/vmwrite errors occur after
TDX Module updates. But with it, no errors.
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Huang, Kai @ 2026-02-03 11:16 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Edgecombe, Rick P, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-21-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Now that kvm_mmu_memory_cache supports custom page allocators, wire up the
> S-EPT cache to use tdx_{alloc,free}_control_page() (arguably S-EPT pages
> aren't "control" pages, but they're not guest pages either). Using the
> TDX APIs will make S-EPT pages naturally play nice with Dynamic PAMT, by
> virtue of adding/removing PAMT entries when S-EPT pages are allocated and
> freed, as opposed to when they are added/removed from the S-EPT tree.
>
> Inserting into the PAMT entries on allocation does mean KVM will create
> unnecessary PAMT entries, e.g. once a vCPU stops faulting in memory, the
> remaining pages in the MMU cache will go unused. But in practice, odds
> are very good the containing 2MiB page will have other in-use S-EPT pages,
> i.e. will create PAMT entries anyways. And _if_ creating PAMT entries on
> allocation is problematic for memory consumption, that can be resolved by
> tweaking KVM's cache size.
>
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Some nits below ..
[...]
> int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> u64 mirror_spte);
> -
> - /* Update external page tables for page table about to be freed. */
> void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> struct kvm_mmu_page *sp);
> -
> - /* Update external page table from spte getting removed, and flush TLB. */
The above two comments are still useful to me.
Not sure why do you want to remove them, especially in _this_ patch?
> void (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> u64 mirror_spte);
>
> +
Unintentional change?
> bool (*has_wbinvd_exit)(void);
>
> u64 (*get_l2_tsc_offset)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 3911ac9bddfd..9b5a6861e2a4 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6690,11 +6690,13 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
> vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
>
> - vcpu->arch.mmu_shadow_page_cache.init_value =
> - SHADOW_NONPRESENT_VALUE;
> + vcpu->arch.mmu_shadow_page_cache.init_value = SHADOW_NONPRESENT_VALUE;
> if (!vcpu->arch.mmu_shadow_page_cache.init_value)
> vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
Ditto. Not sure this adjustment is intentional?
^ 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