* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-01-30 12:32 UTC (permalink / raw)
To: Kim Phillips
Cc: linux-kernel, kvm, linux-coco, x86, Sean Christopherson,
Paolo Bonzini, K Prateek Nayak, Nikunj A Dadhania, Tom Lendacky,
Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <f42e878a-d56f-413d-87e1-19acdc6de690@amd.com>
On Thu, Jan 29, 2026 at 04:32:49PM -0600, Kim Phillips wrote:
> Not *all* SNP features are implemented in all guest kernel versions, and,
> well, for those that don't require explicit guest code support, perhaps it's
> because they aren't necessarily well defined and validated in all hardware
> versions...
Ok, can you add *this* feature to SNP_FEATURES_PRESENT? If not, why not?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-01-30 8:08 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: <04f9a748-fe61-439c-af80-59c498970580@intel.com>
On Wed, Jan 28, 2026 at 03:04:55PM -0800, Dave Hansen 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.
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.
>
>KVM folks, are you OK with this?
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Xu Yilun @ 2026-01-30 4:01 UTC (permalink / raw)
To: Dave Hansen
Cc: Chao Gao, linux-coco, linux-kernel, kvm, x86, reinette.chatre,
ira.weiny, kai.huang, dan.j.williams, 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: <b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@intel.com>
> I'd also prefer a
>
> BUILD_BUG_ON(sizeof(struct seamldr_info) != 2048);
^
BUILD_BUG_ON(sizeof(struct seamldr_info) != 256); is it?
>
> just as a sanity check. It doesn't cost anything and it makes sure that
> as you muck around with reserved fields and padding that there's at
> least one check making sure it's OK.
And I recently received a comments that "never __packed for naturally
aligned structures cause it leads to bad generated code and hurts
performance", but I really want to highlight nearby it is for a
formatted binary blob, so:
struct seamldr_info {
u32 version;
u32 attributes;
u32 vendor_id;
u32 build_date;
u16 build_num;
u16 minor_version;
u16 major_version;
u16 update_version;
u8 reserved0[4];
u32 num_remaining_updates;
u8 reserved1[224];
}; //delete __packed here
static_assert(sizeof(struct seamldr_info) == 256);
Is it better?
^ permalink raw reply
* Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Sean Christopherson @ 2026-01-30 1:30 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Yan Zhao, Vishal Annapurve,
Ackerley Tng, Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-17-seanjc@google.com>
On Wed, Jan 28, 2026, Sean Christopherson wrote:
> +/*
> + * For SEAMCALLs that pass a bundle of pages, the TDX spec treats the registers
> + * like an array, as they are ordered in the struct. The effective array size
> + * is (obviously) limited by the number or registers, relative to the starting
> + * register. Fill the register array at a given starting register, with sanity
> + * checks to avoid overflowing the args structure.
> + */
> +static void dpamt_copy_regs_array(struct tdx_module_args *args, void *reg,
> + u64 *pamt_pa_array, bool copy_to_regs)
> +{
> + int size = tdx_dpamt_entry_pages() * sizeof(*pamt_pa_array);
> +
> + if (WARN_ON_ONCE(reg + size > (void *)args) + sizeof(*args))
The above closing ')' after args is misplaced, this should be
if (WARN_ON_ONCE(reg + size > (void *)args + sizeof(*args)))
I'm still in disbelief that I managed to end up with such a horrid bug that
compiled without any warnings. *sigh*
^ permalink raw reply
* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Sean Christopherson @ 2026-01-30 1:28 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Kai Huang, 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: <f9f65b0fad57db12e21d2168d02bac036615fb7f.camel@intel.com>
On Thu, Jan 29, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > Define kvm_x86_ops .link_external_spt(), .set_external_spte(), and
> > .free_external_spt() as RET0 static calls so that an unexpected call to a
> > a default operation doesn't consume garbage.
> >
> > Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> > Fixes: 94faba8999b9 ("KVM: x86/tdp_mmu: Propagate tearing down mirror page tables")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> We don't want to crash unnecessarily, but do we want to get some sort of notice?
Hmm, that's probably doable, but definitely in a separate patch. E.g. something
like:
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 6083fb07cd3b..270149f84bb4 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -3,6 +3,13 @@
BUILD_BUG_ON(1)
#endif
+#ifndef KVM_X86_OP_OPTIONAL
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_WARN KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0_WARN KVM_X86_OP
+#endif
+
/*
* KVM_X86_OP() and KVM_X86_OP_OPTIONAL() are used to help generate
* both DECLARE/DEFINE_STATIC_CALL() invocations and
@@ -94,11 +101,11 @@ KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
KVM_X86_OP(load_mmu_pgd)
-KVM_X86_OP_OPTIONAL(alloc_external_sp)
-KVM_X86_OP_OPTIONAL(free_external_sp)
-KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
-KVM_X86_OP_OPTIONAL(reclaim_external_sp)
-KVM_X86_OP_OPTIONAL_RET0(topup_external_cache)
+KVM_X86_OP_OPTIONAL_WARN(alloc_external_sp)
+KVM_X86_OP_OPTIONAL_WARN(free_external_sp)
+KVM_X86_OP_OPTIONAL_RET0_WARN(set_external_spte)
+KVM_X86_OP_OPTIONAL_WARN(reclaim_external_sp)
+KVM_X86_OP_OPTIONAL_RET0_WARN(topup_external_cache)
KVM_X86_OP(has_wbinvd_exit)
KVM_X86_OP(get_l2_tsc_offset)
KVM_X86_OP(get_l2_tsc_multiplier)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index cd3e7dc6ab9b..663c9943c0dd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2004,8 +2004,6 @@ extern struct kvm_x86_ops kvm_x86_ops;
#define KVM_X86_OP(func) \
DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_OPTIONAL KVM_X86_OP
-#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
#include <asm/kvm-x86-ops.h>
int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c3d71ba9a1dc..1748f44c81c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -143,8 +143,6 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
#define KVM_X86_OP(func) \
DEFINE_STATIC_CALL_NULL(kvm_x86_##func, \
*(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_OPTIONAL KVM_X86_OP
-#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
#include <asm/kvm-x86-ops.h>
EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
@@ -9965,6 +9963,17 @@ static struct notifier_block pvclock_gtod_notifier = {
};
#endif
+static void kvm_static_call_warn(void)
+{
+ WARN_ON_ONCE(1);
+}
+
+static long kvm_static_call_warn_return0(void)
+{
+ WARN_ON_ONCE(1);
+ return 0;
+}
+
static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
{
memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
@@ -9977,6 +9986,12 @@ static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
#define KVM_X86_OP_OPTIONAL_RET0(func) \
static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
(void *)__static_call_return0);
+#define KVM_X86_OP_OPTIONAL_WARN(func) \
+ static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
+ (void *)kvm_static_call_warn);
+#define KVM_X86_OP_OPTIONAL_RET0_WARN(func) \
+ static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
+ (void *)kvm_static_call_warn_return0);
#include <asm/kvm-x86-ops.h>
#undef __KVM_X86_OP
^ permalink raw reply related
* Re: [RFC PATCH v5 11/45] x86/tdx: Add helpers to check return status codes
From: Edgecombe, Rick P @ 2026-01-30 0:36 UTC (permalink / raw)
To: Hansen, Dave, seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, 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, Yamahata, Isaku, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aXvEgD69vDTPj4z5@google.com>
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(). Today
there are a few special cases like IS_TDX_NON_RECOVERABLE().
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/
I see now that we closed it with you but never got Dave's final buy in.
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Edgecombe, Rick P @ 2026-01-29 22:48 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, 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, Yamahata, Isaku, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aXvd2z8HmLFd50io@google.com>
On Thu, 2026-01-29 at 14:23 -0800, Sean Christopherson wrote:
> No, the bug is if the cmpxchg64 fails. On failure, the current mismatching value
> is stored in the "old" param. KVM relies on the iter->old_spte holding the
> current value when restarting an operation without re-reading the SPTE from memory.
Ah, I see. Sorry. Just went and refreshed up on the difference between
cmpxchg64() and try_cmpxchg64(). I see now that the log is accurate since it
refers to the behavior of the instruction, but specifying try_cmpxchg64() might
be a little clearer since cmpxchg() doesn't automatically update the 'old'
passed in. In either case:
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Kim Phillips @ 2026-01-29 22:32 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, kvm, linux-coco, x86, Sean Christopherson,
Paolo Bonzini, K Prateek Nayak, Nikunj A Dadhania, Tom Lendacky,
Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <20260129105116.GBaXs7pBF-k4x_5_W1@fat_crate.local>
On 1/29/26 4:51 AM, Borislav Petkov wrote:
> On Wed, Jan 28, 2026 at 06:38:29PM -0600, Kim Phillips wrote:
>> SNP_FEATURES_PRESENT is for the non-trivial variety: Its bits get set as
>> part of the patchseries that add the explicit guest support *code*.
> Yes, and I'm asking why can't SNP_FEATURES_PRESENT contain *all* SNP features?
Not *all* SNP features are implemented in all guest kernel versions, and,
well, for those that don't require explicit guest code support, perhaps it's
because they aren't necessarily well defined and validated in all hardware
versions...
Kim
^ permalink raw reply
* Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
From: Alexey Kardashevskiy @ 2026-01-29 22:29 UTC (permalink / raw)
To: dan.j.williams, linux-crypto
Cc: linux-kernel, Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
David S. Miller, x86, linux-coco, Pratik R . Sampat
In-Reply-To: <6978626886a50_1d33100c1@dwillia2-mobl4.notmuch>
On 27/1/26 17:59, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
>> On 24/1/26 09:59, dan.j.williams@intel.com wrote:
>>> Alexey Kardashevskiy wrote:
>>>> IDE stream IDs are responsibility of a platform and in some cases
>>>> TSM allocates the numbers. AMD SEV TIO though leaves it to the host
>>>> OS. Mistakenly stream ID is hard coded to be the same as a traffic
>>>> class.
>>>
>>> I scratched my head at this comment, but now realize that you are
>>> saying the existing code used the local @tc, not that the hardware
>>> stream ID is in any way related to traffic class, right?
>>
>> When I did that in the first place, I also wanted to try different
>> traffic classes so I just took a shortcut here.
>>
>>> It would help to detail what the end user visible effects of this
>>> bug are. The TSM framework does not allow for multiple streams per
>>> PF, so I wonder what scenario is being fixed?
>>
>> There is no way in the current upstream code to specify this TC so the
>> only visible effect is that 2 devices under the same bridge can work
>> now, previously the second device would fail to allocate a stream.
>>
>>> Lastly, are you expecting tsm.git#fixes to pick this up? I am
>>> assuming that this goes through crypto.git and tsm.git can just stay
>>> focused on core fixes.
>>
>> I was kinda hoping that Tom acks these (as he did) and you could take
>> them. Thanks,
>
> Ok, so can you refresh the changelog to call out the user visible
> effects? Something like:
>
> ---
> With SEV-TIO the low-level TSM driver is responsible for allocating a
> Stream ID. The Stream ID needs to be unique within each IDE partner
> port. Fix the Stream ID selection to reuse the host bridge stream
> resource id which is a pool of 256 ids per host bridge on AMD platforms.
> Otherwise, only one device per-host bridge can establish Selective
> Stream IDE.
> ---
>
> Send a v2, and I will pick it up.
Please squash it in the v1, if possible.
Acked-by: Alexey Kardashevskiy <aik@amd.com>
thanks!
ps sorry missed that on time, I do suck at multitasking :(
--
Alexey
^ 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-01-29 22:23 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Kai Huang, 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: <fbaeb0d2f4658efd4c7bb61ac0ba2919c8226a36.camel@intel.com>
On Thu, Jan 29, 2026, Rick P Edgecombe 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.
> >
>
> Might be being dense here, but is the bug that if cmpxchg64 *succeeds* and
> set_external_spte() fails? Then old_spte is not updated and the local retry will
> expect the wrong old_spte.
No, the bug is if the cmpxchg64 fails. On failure, the current mismatching value
is stored in the "old" param. KVM relies on the iter->old_spte holding the
current value when restarting an operation without re-reading the SPTE from memory.
E.g. in __tdp_mmu_zap_root(), if tdp_mmu_set_spte_atomic() fails, iter->old_spte
*must* hold the current in-memroy value, otherwise the loop will hang because it
will re-attempt cmpxchg64 using the stale iter->old_spte.
static void __tdp_mmu_zap_root(struct kvm *kvm, struct kvm_mmu_page *root,
bool shared, int zap_level)
{
struct tdp_iter iter;
for_each_tdp_pte_min_level_all(iter, root, zap_level) {
retry:
if (tdp_mmu_iter_cond_resched(kvm, &iter, false, shared))
continue;
if (!is_shadow_present_pte(iter.old_spte))
continue;
if (iter.level > zap_level)
continue;
if (!shared)
tdp_mmu_iter_set_spte(kvm, &iter, SHADOW_NONPRESENT_VALUE);
else if (tdp_mmu_set_spte_atomic(kvm, &iter, SHADOW_NONPRESENT_VALUE))
goto retry;
}
}
> > 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().
>
>
^ permalink raw reply
* Re: [PATCH v1 08/14] x86: make CONFIG_EFI_STUB unconditional
From: H. Peter Anvin @ 2026-01-29 22:22 UTC (permalink / raw)
To: Simon Glass
Cc: ubizjak, linux-kernel, akpm, bp, dave.hansen, kas, kees,
linux-coco, mingo, nathan, peterz, pmladek, rick.p.edgecombe,
tglx, x86
In-Reply-To: <CAFLszTiQ_BAMGJPJfAV4TXiELvZn12+wX2txBuzMcQ_nw2kzOg@mail.gmail.com>
On January 29, 2026 2:13:13 PM PST, Simon Glass <sjg@chromium.org> wrote:
>Hi Peter,
>
>On Tue, 27 Jan 2026 at 16:22, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> On January 26, 2026 7:14:27 PM PST, Simon Glass <sjg@chromium.org> wrote:
>> >Hi Peter,
>> >
>> >On Tue, 27 Jan 2026 at 15:55, H. Peter Anvin <hpa@zytor.com> wrote:
>> >>
>> >> On January 26, 2026 5:44:43 PM PST, Simon Glass <sjg@chromium.org> wrote:
>> >> >Hi Peter,
>> >> >
>> >> >On Tue, 27 Jan 2026 at 11:21, H. Peter Anvin <hpa@zytor.com> wrote:
>> >> >>
>> >> >> On 2026-01-26 13:19, Simon Glass wrote:
>> >> >> >>
>> >> >> >> Including the EFI stub doesn't mean using EFI to boot is required.
>> >> >> >
>> >> >> > Yes, understood, but it adds bloat. More importantly it will lead to
>> >> >> > people assuming that the stub is always used and thus unwittingly blur
>> >> >> > the boundary between the stub and the kernel itself.
>> >> >> >
>> >> >> > What is the actual need for this?
>> >> >> >
>> >> >>
>> >> >> I would argue that the opposite is more likely: someone inadvertently builds a
>> >> >> kernel without the stub, the bootloader goes down a legacy support path and
>> >> >> things seems to work... except for some platform subtleties.
>> >> >>
>> >> >> The bloat is there, but it is small and is only in the on-disk kernel image;
>> >> >> it is zero at runtime.
>> >> >>
>> >> >> As such, I don't think this option is a particularly good idea anymore. If
>> >> >> necessary, it could be hidden behind an EXPERT option, but I first wanted to
>> >> >> see who if anyone actually cares in a meaningful way to maintain this option.
>> >> >> Every option, after all, adds maintenance burden.
>> >> >>
>> >> >> Note that the BIOS stub is unconditionally compiled and included, and that has
>> >> >> not been an issue.
>> >> >
>> >> >What is the maintenance burden here? I could potentially take that on,
>> >> >but I would first want to understand what is involved.
>> >> >
>> >> >The use of the word 'legacy' worries me too. Is this patch a step
>> >> >towards removing the non-EFI path?
>> >> >
>> >> >Regards,
>> >> >Simon
>> >
>> >(joining the threads)
>> >
>> >> Bypassing the firmware stub (BIOS or EFI) is really only appropriate for special user cases, like kexec, because it removes the ability for the kernel to deal with system issues at an early point.
>> >
>> >Does this dealing happen in the EFI stub, or later? Are you referring
>> >to ACPI fix-ups or something else?
>> >
>> >>
>> >> However, kexec needs it, and it's not going to go away. However, that doesn't mean we should encourage this in cases which doesn't need it (no matter what the Grub maintainers tell you.)
>> >>
>> >> Now, if you are using KVM without EFI you are probably doing BIOS boot (regardless of if you know it or not), entering via the BIOS firmware stub.
>> >
>> >I am thinking of the 64-bit entry point to the kernel. everything
>> >being laid out in memory ready to go.
>> >
>> >>
>> >> I just realized you are the U-boot maintainer, so I'm assuming you are thinking of the case where there is no UEFI or BIOS firmware. In that case, just like as for kexec, the current entry point will continue to work, of course.
>> >>
>> >> What we don't want is having to suffer being on a BIOS or EFI system but not being able to leverage it for the benefit of the kernel. The kernel image is much easier to upgrade.
>> >
>> >More generally I am thinking about a simple and clean API for the
>> >kernel that doesn't involve having to provide 30K lines of firmware
>> >code just to boot. The BIOS entry point (if that is what it is called)
>> >is quite close to this ideal, even though I know it has shortcomings.
>> >
>> >Regards,
>> >Simon
>>
>> Yes, I understand.
>>
>> The 32/64-bit entrypoints aren't going away; it would be impossible to do so.
>>
>
>OK, so 'depend on EXPORT' seems good to me.
>
>> The general rule is: do things as late in the boot process as possible, but no later.
>>
>
>Just on this point, I wonder how we should define 'late', in the
>context of EFI. For example, if the kernel stub reads a file, meaning
>it calls back into Tianocore, it is using both early and late code.
>
>Regards,
>Simon
"Early" in this context is before ExitBootServices().
^ permalink raw reply
* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Edgecombe, Rick P @ 2026-01-29 22:20 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: Huang, Kai, 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-5-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Define kvm_x86_ops .link_external_spt(), .set_external_spte(), and
> .free_external_spt() as RET0 static calls so that an unexpected call to a
> a default operation doesn't consume garbage.
>
> Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> Fixes: 94faba8999b9 ("KVM: x86/tdp_mmu: Propagate tearing down mirror page tables")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
We don't want to crash unnecessarily, but do we want to get some sort of notice?
^ permalink raw reply
* Re: [RFC PATCH v5 03/45] KVM: TDX: Account all non-transient page allocations for per-TD structures
From: Edgecombe, Rick P @ 2026-01-29 22:15 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: Huang, Kai, 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-4-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Account all non-transient allocations associated with a single TD (or its
> vCPUs), as KVM's ABI is that allocations that are active for the lifetime
> of a VM are accounted. Leave temporary allocations, i.e. allocations that
> are freed within a single function/ioctl, unaccounted, to again align with
> KVM's existing behavior, e.g. see commit dd103407ca31 ("KVM: X86: Remove
> unnecessary GFP_KERNEL_ACCOUNT for temporary variables").
>
> Fixes: 8d032b683c29 ("KVM: TDX: create/destroy VM structure")
> Fixes: a50f673f25e0 ("KVM: TDX: Do TDX specific vcpu initialization")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v1 08/14] x86: make CONFIG_EFI_STUB unconditional
From: Simon Glass @ 2026-01-29 22:13 UTC (permalink / raw)
To: H. Peter Anvin
Cc: ubizjak, linux-kernel, akpm, bp, dave.hansen, kas, kees,
linux-coco, mingo, nathan, peterz, pmladek, rick.p.edgecombe,
tglx, x86
In-Reply-To: <36C4A641-A124-4082-884D-90AE22B10275@zytor.com>
Hi Peter,
On Tue, 27 Jan 2026 at 16:22, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On January 26, 2026 7:14:27 PM PST, Simon Glass <sjg@chromium.org> wrote:
> >Hi Peter,
> >
> >On Tue, 27 Jan 2026 at 15:55, H. Peter Anvin <hpa@zytor.com> wrote:
> >>
> >> On January 26, 2026 5:44:43 PM PST, Simon Glass <sjg@chromium.org> wrote:
> >> >Hi Peter,
> >> >
> >> >On Tue, 27 Jan 2026 at 11:21, H. Peter Anvin <hpa@zytor.com> wrote:
> >> >>
> >> >> On 2026-01-26 13:19, Simon Glass wrote:
> >> >> >>
> >> >> >> Including the EFI stub doesn't mean using EFI to boot is required.
> >> >> >
> >> >> > Yes, understood, but it adds bloat. More importantly it will lead to
> >> >> > people assuming that the stub is always used and thus unwittingly blur
> >> >> > the boundary between the stub and the kernel itself.
> >> >> >
> >> >> > What is the actual need for this?
> >> >> >
> >> >>
> >> >> I would argue that the opposite is more likely: someone inadvertently builds a
> >> >> kernel without the stub, the bootloader goes down a legacy support path and
> >> >> things seems to work... except for some platform subtleties.
> >> >>
> >> >> The bloat is there, but it is small and is only in the on-disk kernel image;
> >> >> it is zero at runtime.
> >> >>
> >> >> As such, I don't think this option is a particularly good idea anymore. If
> >> >> necessary, it could be hidden behind an EXPERT option, but I first wanted to
> >> >> see who if anyone actually cares in a meaningful way to maintain this option.
> >> >> Every option, after all, adds maintenance burden.
> >> >>
> >> >> Note that the BIOS stub is unconditionally compiled and included, and that has
> >> >> not been an issue.
> >> >
> >> >What is the maintenance burden here? I could potentially take that on,
> >> >but I would first want to understand what is involved.
> >> >
> >> >The use of the word 'legacy' worries me too. Is this patch a step
> >> >towards removing the non-EFI path?
> >> >
> >> >Regards,
> >> >Simon
> >
> >(joining the threads)
> >
> >> Bypassing the firmware stub (BIOS or EFI) is really only appropriate for special user cases, like kexec, because it removes the ability for the kernel to deal with system issues at an early point.
> >
> >Does this dealing happen in the EFI stub, or later? Are you referring
> >to ACPI fix-ups or something else?
> >
> >>
> >> However, kexec needs it, and it's not going to go away. However, that doesn't mean we should encourage this in cases which doesn't need it (no matter what the Grub maintainers tell you.)
> >>
> >> Now, if you are using KVM without EFI you are probably doing BIOS boot (regardless of if you know it or not), entering via the BIOS firmware stub.
> >
> >I am thinking of the 64-bit entry point to the kernel. everything
> >being laid out in memory ready to go.
> >
> >>
> >> I just realized you are the U-boot maintainer, so I'm assuming you are thinking of the case where there is no UEFI or BIOS firmware. In that case, just like as for kexec, the current entry point will continue to work, of course.
> >>
> >> What we don't want is having to suffer being on a BIOS or EFI system but not being able to leverage it for the benefit of the kernel. The kernel image is much easier to upgrade.
> >
> >More generally I am thinking about a simple and clean API for the
> >kernel that doesn't involve having to provide 30K lines of firmware
> >code just to boot. The BIOS entry point (if that is what it is called)
> >is quite close to this ideal, even though I know it has shortcomings.
> >
> >Regards,
> >Simon
>
> Yes, I understand.
>
> The 32/64-bit entrypoints aren't going away; it would be impossible to do so.
>
OK, so 'depend on EXPORT' seems good to me.
> The general rule is: do things as late in the boot process as possible, but no later.
>
Just on this point, I wonder how we should define 'late', in the
context of EFI. For example, if the kernel stub reads a file, meaning
it calls back into Tianocore, it is using both early and late code.
Regards,
Simon
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Edgecombe, Rick P @ 2026-01-29 22:10 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: Huang, Kai, 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-3-seanjc@google.com>
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.
>
Might be being dense here, but is the bug that if cmpxchg64 *succeeds* and
set_external_spte() fails? Then old_spte is not updated and the local retry will
expect the wrong old_spte.
> 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().
^ permalink raw reply
* Re: [RFC PATCH v5 11/45] x86/tdx: Add helpers to check return status codes
From: Sean Christopherson @ 2026-01-29 20:35 UTC (permalink / raw)
To: Dave Hansen
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Yan Zhao, Vishal Annapurve,
Ackerley Tng, Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <cd7c140a-247b-44a7-80cc-80fd177d22bb@intel.com>
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)
FWIW, I have zero preference on this. I included the patch purely because it was
already there.
> But, honestly, if I were trying to push a 45-patch series, I probably
> wouldn't tangle this up as part of it. It's not _that_ desperately in
> need of munging it a quarter of the way into this series.
For sure. The 45 patches are definitely not intended to land as one. I posted
the mega-series to propose an end-to-end design for DPAMT + S-EPT hugepage support.
I don't have the bandwidth or brainpower to hash out a KVM design in two different
series.
^ permalink raw reply
* Re: [PATCH v3 2/2] x86/sev: Add support to unaccept memory after hot-remove
From: Pratik R. Sampat @ 2026-01-29 19:32 UTC (permalink / raw)
To: Dave Hansen, Kiryl Shutsemau
Cc: linux-mm, linux-coco, x86, linux-kernel, tglx, mingo, bp,
dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <b7cbc2f4-6fef-44c2-a0f4-2d6895c0fd74@intel.com>
On 1/29/26 11:39 AM, Dave Hansen wrote:
> On 1/29/26 09:32, Pratik R. Sampat wrote:
>> In that case a fall through for TDX (with a comment explaining why) and
>> panic for rest may be the way to go?
>
> No. panic() is an absolute last resort. It's almost never the way to go.
>
> What else can we do to ensure we never reach this code if the platform
> doesn't support memory un-acceptance?
The panic() here similar to its existing arch_accept_memory() counterpart is
mostly to guard against a cant-happen scenario (unless Kiryl had a different
intention writing the initial hook). It is called from functions that compile
this in only if CONFIG_UNACCEPTED_MEMORY is enabled. TDX and SNP are the only
two users of it today.
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2026-01-29 19:12 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, binbin.wu@linux.intel.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <aXuweFnbPhoG4Jbk@google.com>
On Thu, 2026-01-29 at 11:09 -0800, Sean Christopherson wrote:
> What you proposed is fundamentally quite different than what I'm
> proposing. I'm not complaining about the union, I'm complaining
> about providing a helper to grab a pointer to the middle of a struct
> and then open coding memcpy() calls using that pointer. I find that
> _extremely_ difficult to grok, because it does a poor job of
> capturing the intent (copy these values to this sequence of
> registers).
>
> The decouple pointer+memcpy() approach also bleeds gory details about
> how PAMT pages are passed in multiple args throughout all SEAMCALL
> APIs that have such args. I want the APIs to not have to care about
> the underlying mechanics of how PAMT pages are copied from an array
> to registers, e.g. so that readers of the code can focus on the
> semantics of the SEAMCALL and the pieces of logic that are
> unique to each SEAMCALL.
>
> In other words, I see the necessity for a union as being a symptom of
> the flawed approach.
Ah ok, fair enough.
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Sean Christopherson @ 2026-01-29 19:09 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Kai Huang,
Xiaoyao Li, Dave Hansen, Yan Y Zhao, Binbin Wu, kas@kernel.org,
binbin.wu@linux.intel.com, mingo@redhat.com, pbonzini@redhat.com,
tglx@linutronix.de, Isaku Yamahata, linux-kernel@vger.kernel.org,
Vishal Annapurve, Chao Gao, bp@alien8.de, x86@kernel.org
In-Reply-To: <9096e7a47742f4a46a7f400aac467ac78e1dfe50.camel@intel.com>
On Thu, Jan 29, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-01-28 at 17:19 -0800, Sean Christopherson wrote:
> > Honestly, the entire scheme is a mess. Four days of staring at this
> > and I finally undertand what the code is doing. The whole "struct
> > tdx_module_array_args" union is completely unnecessary, the resulting
> > args.args crud is ugly, having a pile of duplicate accessors is
> > brittle, the code obfuscates a simple concept, and the end result
> > doesn't provide any actual protection since the kernel will happily
> > overflow the buffer after the WARN.
>
> The original sin for this, as was spotted by Nikilay in v3, is actually
> that it turns out that the whole variable length thing was intended to
> give the TDX module flexibility *if* it wanted to increase it in the
> future. As in it's not required today. Worse, whether it would actually
> grow in the specific way the code assumes is not covered in the spec.
> Apparently it was based on some past internal discussions. So the
> agreement on v3 was to just support the fixed two page size in the
> spec.
Heh, I was _this_ close to suggesting we add a compile-time assert on the incoming
size of the array (and effective regs array), with a constant max size supported
by the kernel. It wouldn't eliminate the array shenanigans, but it would let us
make them more or less bombproof.
> Yea it could probably use another DEFINE or two to make it less error
> prone. Vanilla DPAMT has 4 instances of rdx.
For me, it's not just a syntax problem. It's the approach of getting a pointer
to the middle of structure and then doing a memcpy() at a later point in time.
More below.
> What you have here is close to what I had done when I first took this
> series. But it ran afoul of FORTIFY_SOUCE and required some horrible
> casting to trick it. I wonder if this code will hit that issue too.
AFAICT, FORTIFY_SOURCE doesn't complain.
> Dave didn't like the solution and suggested the union actually:
> https://lore.kernel.org/kvm/355ad607-52ed-42cc-9a48-63aaa49f4c68@intel.com/#t
What you proposed is fundamentally quite different than what I'm proposing. I'm
not complaining about the union, I'm complaining about providing a helper to grab
a pointer to the middle of a struct and then open coding memcpy() calls using
that pointer. I find that _extremely_ difficult to grok, because it does a poor
job of capturing the intent (copy these values to this sequence of registers).
The decouple pointer+memcpy() approach also bleeds gory details about how PAMT
pages are passed in multiple args throughout all SEAMCALL APIs that have such
args. I want the APIs to not have to care about the underlying mechanics of how
PAMT pages are copied from an array to registers, e.g. so that readers of the
code can focus on the semantics of the SEAMCALL and the pieces of logic that are
unique to each SEAMCALL.
In other words, I see the necessity for a union as being a symptom of the flawed
approach.
> I'm aware of your tendency to dislike union based solutions. But since
> this was purely contained to tip, I went with Dave's preference.
^ permalink raw reply
* Re: [RFC PATCH v5 11/45] x86/tdx: Add helpers to check return status codes
From: Dave Hansen @ 2026-01-29 18:58 UTC (permalink / raw)
To: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, Kiryl Shutsemau, Paolo Bonzini
Cc: linux-kernel, linux-coco, kvm, Kai Huang, Rick Edgecombe,
Yan Zhao, Vishal Annapurve, Ackerley Tng, Sagi Shahar, Binbin Wu,
Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-12-seanjc@google.com>
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)
But, honestly, if I were trying to push a 45-patch series, I probably
wouldn't tangle this up as part of it. It's not _that_ desperately in
need of munging it a quarter of the way into this series.
^ permalink raw reply
* Re: [RFC PATCH v5 10/45] x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
From: Dave Hansen @ 2026-01-29 18:13 UTC (permalink / raw)
To: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, Kiryl Shutsemau, Paolo Bonzini
Cc: linux-kernel, linux-coco, kvm, Kai Huang, Rick Edgecombe,
Yan Zhao, Vishal Annapurve, Ackerley Tng, Sagi Shahar, Binbin Wu,
Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-11-seanjc@google.com>
On 1/28/26 17:14, Sean Christopherson wrote:
...
> "asm/shared" is used for sharing TDX code between the early compressed
> code and the normal kernel code. While the compressed code for the guest
> doesn't use these error code header definitions today, it does make the
> types of calls that return the values they define. So place the defines in
> "shared" location so that it can, but leave such cleanups for future
> changes.
This is beating around the bush a bit. Should this read:
Place the new header is in "asm/shared". It doesn't need to be
there, but Google's kernel fork has early compressed use of
these things and mainline will too soon.
or what? Can we be more direct, please?
^ permalink raw reply
* Re: [PATCH v3 2/2] x86/sev: Add support to unaccept memory after hot-remove
From: Dave Hansen @ 2026-01-29 17:39 UTC (permalink / raw)
To: Pratik R. Sampat, Kiryl Shutsemau
Cc: linux-mm, linux-coco, x86, linux-kernel, tglx, mingo, bp,
dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <097a4980-9d10-40d2-9667-d07bf19e7b86@amd.com>
On 1/29/26 09:32, Pratik R. Sampat wrote:
> In that case a fall through for TDX (with a comment explaining why) and
> panic for rest may be the way to go?
No. panic() is an absolute last resort. It's almost never the way to go.
What else can we do to ensure we never reach this code if the platform
doesn't support memory un-acceptance?
^ permalink raw reply
* Re: [RFC PATCH v5 01/45] x86/tdx: Use pg_level in TDX APIs, not the TDX-Module's 0-based level
From: Dave Hansen @ 2026-01-29 17:37 UTC (permalink / raw)
To: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, Kiryl Shutsemau, Paolo Bonzini
Cc: linux-kernel, linux-coco, kvm, Kai Huang, Rick Edgecombe,
Yan Zhao, Vishal Annapurve, Ackerley Tng, Sagi Shahar, Binbin Wu,
Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-2-seanjc@google.com>
On 1/28/26 17:14, Sean Christopherson wrote:
> Rework the TDX APIs to take the kernel's 1-based pg_level enum, not the
> TDX-Module's 0-based level. The APIs are _kernel_ APIs, not TDX-Module
> APIs, and the kernel (and KVM) uses "enum pg_level" literally everywhere.
>
> Using "enum pg_level" eliminates ambiguity when looking at the APIs (it's
> NOT clear that "int level" refers to the TDX-Module's level), and will
> allow for using existing helpers like page_level_size() when support for
> hugepages is added to the S-EPT APIs.
Yup, totally the right thing to do: push the TDX-isms as deep in the
code as possible. pg_level_to_tdx_sept_level() is a bit wordy, but I can
live with it:
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 2/2] x86/sev: Add support to unaccept memory after hot-remove
From: Pratik R. Sampat @ 2026-01-29 17:32 UTC (permalink / raw)
To: Kiryl Shutsemau, Dave Hansen
Cc: linux-mm, linux-coco, x86, linux-kernel, tglx, mingo, bp,
dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <aXs4DTXCbI_c0JPO@thinkstation>
On 1/29/26 4:40 AM, Kiryl Shutsemau wrote:
> On Wed, Jan 28, 2026 at 01:15:06PM -0800, Dave Hansen wrote:
>> On 1/28/26 12:41, Pratik R. Sampat wrote:
>>> +static inline void arch_unaccept_memory(phys_addr_t start, phys_addr_t end)
>>> +{
>>> + if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP)) {
>>> + snp_unaccept_memory(start, end);
>>> + } else {
>>> + panic("Cannot unaccept memory: unknown platform\n");
>>> + }
>>> +}
>>
>> This panic() is pretty nasty.
>>
>> Can't we just disable memory hotplug up front if it's:
>>
>> !cc_platform_has(CC_ATTR_GUEST_SEV_SNP)
>>
>> ?
>
> I don't understand SEV-SNP situation, but I don't think we need to do
> anything on unplug for TDX. We should expect the unplugged memory to be
> removed from SEPT. If VMM doesn't do this, it is effectively DoS and we
> don't protect against DoS in CoCo.
>
> Converting the memory to shared will do no good for us.
In that case a fall through for TDX (with a comment explaining why) and
panic for rest may be the way to go?
>
^ permalink raw reply
* Re: [PATCH v3 1/2] mm/memory_hotplug: Add support to accept memory during hot-add
From: Pratik R. Sampat @ 2026-01-29 17:32 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: linux-mm, linux-coco, x86, linux-kernel, tglx, mingo, bp,
dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <aXs09b7Q8wmHMFNQ@thinkstation>
Hi Kiryl,
On 1/29/26 4:35 AM, Kiryl Shutsemau wrote:
> On Wed, Jan 28, 2026 at 02:41:04PM -0600, Pratik R. Sampat wrote:
>> 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 | 18 ++++++++++++++++++
>> include/linux/mm.h | 5 +++++
>> mm/memory_hotplug.c | 2 ++
>> 3 files changed, 25 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
>> index c2c067eff634..5a4c8b0f56c8 100644
>> --- a/drivers/firmware/efi/unaccepted_memory.c
>> +++ b/drivers/firmware/efi/unaccepted_memory.c
>> @@ -209,6 +209,24 @@ 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, handle acceptance in accordance with the
>> + * unaccepted bitmap. Otherwise, perform the state change for the memory range
>> + * up-front.
>> + */
>> +void accept_hotplug_memory(phys_addr_t start, unsigned long size)
>> +{
>> + if (range_contains_unaccepted_memory(start, size)) {
>> + accept_memory(start, size);
>> + return;
>> + }
>
> No. This is buggy. The memory has to be accepted regardless of state in
> the bitmap. If the memory is ever unplugged the bitmap state is not
> relevant.
>
> So, accept it unconditionally and mark the memory accepted in the
> bitmap.
I see. This makes sense for acceptance since device_del would always fully
unplug it.
I still might need to keep a version of bitmap handling in unaccept considering
the case where partially accepted (lazy) memory is removed.
For SNP, pvalidate is not an idempotent operation and we must only rescind
the state for the bits that were previously accepted.
Also, now that I stare at the unaccept_hotplug_memory implementation, I realize
calling range_contains_unaccepted_memory() is plain wrong. I should rather be
looking at the ranges and handling the bitmap + unacceptance.
I'll be sure to clean that up in the next iteration as well.
Thanks,
--Pratik
>
>> +
>> + arch_accept_memory(start, start + size);
>> +}
>> +
>> #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
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