* 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
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Andre Przywara @ 2026-01-29 17:26 UTC (permalink / raw)
To: Vedashree Vidwans, Salman Nabi, sudeep.holla, mark.rutland,
lpieralisi
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar
In-Reply-To: <bf1addf9-5e28-46c6-8943-6a359630e533@nvidia.com>
Hi Vedashree,
many thanks for having a look!
On 1/28/26 00:01, Vedashree Vidwans wrote:
> Hello,
>
> On 1/19/26 04:27, Salman Nabi wrote:
>> The Arm Live Firmware Activation (LFA) is a specification [1] to describe
>> activating firmware components without a reboot. Those components
>> (like TF-A's BL31, EDK-II, TF-RMM, secure paylods) would be updated the
>> usual way: via fwupd, FF-A or other secure storage methods, or via some
[ ... ]
>> drivers/firmware/smccc/Kconfig | 8 +
>> drivers/firmware/smccc/Makefile | 1 +
>> drivers/firmware/smccc/lfa_fw.c | 668 ++++++++++++++++++++++++++++++++
>> 3 files changed, 677 insertions(+)
>> create mode 100644 drivers/firmware/smccc/lfa_fw.c
>>
[ ... ]
>> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/
>> lfa_fw.c
>> new file mode 100644
>> index 000000000000..ce54049b7190
>> --- /dev/null
>> +++ b/drivers/firmware/smccc/lfa_fw.c
>> @@ -0,0 +1,668 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (C) 2025 Arm Limited
>> + */
[ ... ]
>> +
>> +static int call_lfa_activate(void *data)
>> +{
>> + struct image_props *attrs = data;
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> +
>> + reg.a0 = LFA_1_0_FN_ACTIVATE;
>> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
>> + /*
>> + * As we do not support updates requiring a CPU reset (yet),
>> + * we pass 0 in reg.a3 and reg.a4, holding the entry point and
>> context
>> + * ID respectively.
>> + * cpu_rendezvous_forced is set by the administrator, via sysfs,
>> + * cpu_rendezvous is dictated by each firmware component.
>> + */
>> + reg.a2 = !(attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous);
>> +
>> + for (;;) {
>> + arm_smccc_1_2_invoke(®, ®);
>> +
>> + if ((long)reg.a0 < 0) {
>> + pr_err("ACTIVATE for image %s failed: %s\n",
>> + attrs->image_name, lfa_error_strings[-reg.a0]);
>> + return reg.a0;
>> + }
>> + if (!(reg.a1 & LFA_ACTIVATE_CALL_AGAIN))
>> + break; /* ACTIVATE successful */
>> + }
> The implementation uses same 'struct arm_smccc_1_2_regs reg' as
> input and output for arm_smccc_1_2_invoke(). Here, reg.a0 (function ID),
> reg.a1 (fw_seq_id) and reg.a2 (cpu rendezvous) are initialized once
> before the loop and arm_smccc_1_2_invoke() overwrites the whole register
> set on every iteration. That means inputs (a0, a1, a2) can be clobbered
> between loop iterations unless reassigned each time.
> Suggestion: Re-initialize input members of reg on each loop iteration or
> use a separate 'struct arm_smccc_1_2_regs' for output to avoid input
> corruption.
Ah, that's a good point and indeed a bug, thanks for spotting this! Will
fix it.
>> +
>> + return reg.a0;
>> +}
>> +
>> +static int activate_fw_image(struct image_props *attrs)
>> +{
>> + int ret;
>> +
>> + mutex_lock(&lfa_lock);
>> + if (attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous)
>> + ret = stop_machine(call_lfa_activate, attrs, cpu_online_mask);
>> + else
>> + ret = call_lfa_activate(attrs);
>> +
>> + if (ret != 0) {
>> + mutex_unlock(&lfa_lock);
>> + return lfa_cancel(attrs);
>> + }
>> +
>> + /*
>> + * Invalidate fw_seq_ids (-1) for all images as the seq_ids and the
>> + * number of firmware images in the LFA agent may change after a
>> + * successful activation attempt. Negate all image flags as well.
>> + */
>> + attrs = NULL;
>> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
>> + set_image_flags(attrs, -1, 0b1000, 0, 0);
>> + }
>> +
>> + update_fw_images_tree();
>> +
>> + /*
>> + * Removing non-valid image directories at the end of an activation.
>> + * We can't remove the sysfs attributes while in the respective
>> + * _store() handler, so have to postpone the list removal to a
>> + * workqueue.
>> + */
>> + INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
>> + queue_work(fw_images_update_wq, &fw_images_update_work);
>> + mutex_unlock(&lfa_lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int prime_fw_image(struct image_props *attrs)
>> +{
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> + int ret;
>> +
>> + mutex_lock(&lfa_lock);
>> + /* Avoid SMC calls on invalid firmware images */
>> + if (attrs->fw_seq_id == -1) {
>> + pr_err("Arm LFA: Invalid firmware sequence id\n");
>> + mutex_unlock(&lfa_lock);
>> +
>> + return -ENODEV;
>> + }
>> +
>> + if (attrs->may_reset_cpu) {
>> + pr_err("CPU reset not supported by kernel driver\n");
>> + mutex_unlock(&lfa_lock);
>> +
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * LFA_PRIME/ACTIVATE will return 1 in reg.a1 if the firmware
>> + * priming/activation is still in progress. In that case
>> + * LFA_PRIME/ACTIVATE will need to be called again.
>> + * reg.a1 will become 0 once the prime/activate process completes.
>> + */
>> + reg.a0 = LFA_1_0_FN_PRIME;
>> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
>> + for (;;) {
>> + arm_smccc_1_2_invoke(®, ®);
>> +
>> + if ((long)reg.a0 < 0) {
>> + pr_err("LFA_PRIME for image %s failed: %s\n",
>> + attrs->image_name, lfa_error_strings[-reg.a0]);
>> + mutex_unlock(&lfa_lock);
>> +
>> + return reg.a0;
>> + }
>> + if (!(reg.a1 & LFA_PRIME_CALL_AGAIN)) {
>> + ret = 0;
>> + break; /* PRIME successful */
>> + }
>> + }
> Similar comment to call_lfa_activate(). Suggestion to either re-assign
> 'struct arm_smccc_1_2_regs' input values on each loop iteration or use a
> separate 'struct arm_smccc_1_2_regs' for output to avoid input
> corruption between loop iterations. This matches the intended
> 'CALL_AGAIN' protocal while keeping the inputs stable across retries.
Indeed, good catch.
>> +
>> + mutex_unlock(&lfa_lock);
>> + return ret;
> The introduction of separate 'ret' cariable does not appear necessary
> for functional correctness. The SMCCC status is conveyed via reg.a0 on
> each iteration, so returning reg.a0 should preserve existing behavior.
> If 'ret' must be kept, consider initializing it to 0 at declaration
> time. That avoids setting ret = 0 inside 'PRIME successful' path and
> leads to simpler control flow.
Right, looks like a leftover from a previous version, it's indeed not
needed.
>> +}
>> +
>> +static ssize_t name_show(struct kobject *kobj, struct kobj_attribute
>> *attr,
>> + char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_NAME]);
>> +
>> + return sysfs_emit(buf, "%s\n", attrs->image_name);
>> +}
>> +
>> +static ssize_t activation_capable_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_ACT_CAPABLE]);
>> +
>> + return sysfs_emit(buf, "%d\n", attrs->activation_capable);
>> +}
>> +
>> +static ssize_t activation_pending_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_ACT_PENDING]);
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> +
>> + /*
>> + * Activation pending status can change anytime thus we need to
>> update
>> + * and return its current value
>> + */
>> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
>> + reg.a1 = attrs->fw_seq_id;
>> + arm_smccc_1_2_invoke(®, ®);
>> + if (reg.a0 == LFA_SUCCESS)
>> + attrs->activation_pending = !!(reg.a3 & BIT(1));
>> +
>> + return sysfs_emit(buf, "%d\n", attrs->activation_pending);
>> +}
>> +
>> +static ssize_t may_reset_cpu_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_MAY_RESET_CPU]);
>> +
>> + return sysfs_emit(buf, "%d\n", attrs->may_reset_cpu);
>> +}
>> +
>> +static ssize_t cpu_rendezvous_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_CPU_RENDEZVOUS]);
>> +
>> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous);
>> +}
>> +
>> +static ssize_t force_cpu_rendezvous_store(struct kobject *kobj,
>> + struct kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
>> + int ret;
>> +
>> + ret = kstrtobool(buf, &attrs->cpu_rendezvous_forced);
>> + if (ret)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t force_cpu_rendezvous_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
>> +
>> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous_forced);
>> +}
>> +
>> +static ssize_t current_version_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_CURRENT_VERSION]);
>> + u32 maj, min;
>> +
>> + maj = attrs->current_version >> 32;
>> + min = attrs->current_version & 0xffffffff;
>> + return sysfs_emit(buf, "%u.%u\n", maj, min);
>> +}
>> +
>> +static ssize_t pending_version_show(struct kobject *kobj,
>> + struct kobj_attribute *attr, char *buf)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_ACT_PENDING]);
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> + u32 maj, min;
>> +
>> + /*
>> + * Similar to activation pending, this value can change following an
>> + * update, we need to retrieve fresh info instead of stale
>> information.
>> + */
>> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
>> + reg.a1 = attrs->fw_seq_id;
>> + arm_smccc_1_2_invoke(®, ®);
>> + if (reg.a0 == LFA_SUCCESS) {
>> + if (reg.a5 != 0 && attrs->activation_pending)
>> + {
>> + attrs->pending_version = reg.a5;
>> + maj = reg.a5 >> 32;
>> + min = reg.a5 & 0xffffffff;
>> + }
>> + }
>> +
>> + return sysfs_emit(buf, "%u.%u\n", maj, min);
>> +}
>> +
>> +static ssize_t activate_store(struct kobject *kobj, struct
>> kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_ACTIVATE]);
>> + int ret;
>> +
>> + ret = prime_fw_image(attrs);
>> + if (ret) {
>> + pr_err("Firmware prime failed: %s\n",
>> + lfa_error_strings[-ret]);
>> + return -ECANCELED;
>> + }
>> +
>> + ret = activate_fw_image(attrs);
>> + if (ret) {
>> + pr_err("Firmware activation failed: %s\n",
>> + lfa_error_strings[-ret]);
>> + return -ECANCELED;
>> + }
>> +
>> + pr_info("Firmware activation succeeded\n");
>> +
>> + return count;
>> +}
>> +
>> +static ssize_t cancel_store(struct kobject *kobj, struct
>> kobj_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct image_props *attrs = container_of(attr, struct image_props,
>> + image_attrs[LFA_ATTR_CANCEL]);
>> + int ret;
>> +
>> + ret = lfa_cancel(attrs);
>> + if (ret != 0)
>> + return ret;
>> +
>> + return count;
>> +}
>> +
>> +static struct kobj_attribute image_attrs_group[LFA_ATTR_NR_IMAGES] = {
>> + [LFA_ATTR_NAME] = __ATTR_RO(name),
>> + [LFA_ATTR_CURRENT_VERSION] = __ATTR_RO(current_version),
>> + [LFA_ATTR_PENDING_VERSION] = __ATTR_RO(pending_version),
>> + [LFA_ATTR_ACT_CAPABLE] = __ATTR_RO(activation_capable),
>> + [LFA_ATTR_ACT_PENDING] = __ATTR_RO(activation_pending),
>> + [LFA_ATTR_MAY_RESET_CPU] = __ATTR_RO(may_reset_cpu),
>> + [LFA_ATTR_CPU_RENDEZVOUS] = __ATTR_RO(cpu_rendezvous),
>> + [LFA_ATTR_FORCE_CPU_RENDEZVOUS] =
>> __ATTR_RW(force_cpu_rendezvous),
>> + [LFA_ATTR_ACTIVATE] = __ATTR_WO(activate),
>> + [LFA_ATTR_CANCEL] = __ATTR_WO(cancel)
>> +};
>> +
>> +static void clean_fw_images_tree(void)
>> +{
>> + struct image_props *attrs, *tmp;
>> +
>> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node)
>> + delete_fw_image_node(attrs);
>> +}
>> +
>> +static int update_fw_image_node(char *fw_uuid, int seq_id,
>> + u32 image_flags, u64 reg_current_ver,
>> + u64 reg_pending_ver)
>> +{
>> + const char *image_name = "(unknown)";
>> + struct image_props *attrs;
>> + int ret;
>> +
>> + /*
>> + * If a fw_image is already in the images list then we just update
>> + * its flags and seq_id instead of trying to recreate it.
>> + */
>> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
>> + if (!strcmp(attrs->image_dir->name, fw_uuid)) {
>> + set_image_flags(attrs, seq_id, image_flags,
>> + reg_current_ver, reg_pending_ver);
>> + return 0;
>> + }
>> + }
>> +
>> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
>> + if (!attrs)
>> + return -ENOMEM;
>> +
>> + for (int i = 0; i < ARRAY_SIZE(fw_images_uuids); i++) {
>> + if (!strcmp(fw_images_uuids[i].uuid, fw_uuid))
>> + image_name = fw_images_uuids[i].name;
>> + }
> I would recommend using fw_uuid as the image_name when UUID is not
> found in fw_images_uuids[], currently the driver assigns 'unknown'
> in such case.
> There is a valid possibility that platform-specific FW images, not
> listed in fw_images_uuids[], are used by LFA agent for live FW
> activation. In such scenarios, falling back to 'unknown' would lose
> important information especially when errors surface in
> call_lfa_activate(). Using UUID directly would indicate which
> image failed or behaved unexpectedly.
Well, I think if you want to identify an image clearly, you always have
to use the UUID, as shown by the directory name. The "name" sysfs file
is there just for convenience, to make this easier for *users* when
dealing with well-known firmware image. As you rightly said, we can
never guarantee that the kernel knows a certain UUID, and it wouldn't be
necessary for proper operation at all.
So I was expecting this name to be only used by reporting scripts or
such. But indeed some "unknown" string is a bit fragile as a placeholder
name, I was wondering if we should just provide an empty string in this
case? This would allow scripts to detect this special case reliably and
provide their own rendering then.
Does this make sense?
Cheers,
Andre
> Thank you,
> Veda
>> +
>> + attrs->image_dir = kobject_create_and_add(fw_uuid, lfa_dir);
>> + if (!attrs->image_dir)
>> + return -ENOMEM;
>> +
>> + INIT_LIST_HEAD(&attrs->image_node);
>> + attrs->image_name = image_name;
>> + attrs->cpu_rendezvous_forced = 1;
>> + set_image_flags(attrs, seq_id, image_flags, reg_current_ver,
>> + reg_pending_ver);
>> +
>> + /*
>> + * The attributes for each sysfs file are constant (handler
>> functions,
>> + * name and permissions are the same within each directory), but we
>> + * need a per-directory copy regardless, to get a unique handle
>> + * for each directory, so that container_of can do its magic.
>> + * Also this requires an explicit sysfs_attr_init(), since it's a
>> new
>> + * copy, to make LOCKDEP happy.
>> + */
>> + memcpy(attrs->image_attrs, image_attrs_group,
>> + sizeof(attrs->image_attrs));
>> + for (int i = 0; i < LFA_ATTR_NR_IMAGES; i++) {
>> + struct attribute *attr = &attrs->image_attrs[i].attr;
>> +
>> + sysfs_attr_init(attr);
>> + ret = sysfs_create_file(attrs->image_dir, attr);
>> + if (ret) {
>> + pr_err("creating sysfs file for uuid %s: %d\n",
>> + fw_uuid, ret);
>> + clean_fw_images_tree();
>> +
>> + return ret;
>> + }
>> + }
>> + list_add(&attrs->image_node, &lfa_fw_images);
>> +
>> + return ret;
>> +}
>> +
>> +static int update_fw_images_tree(void)
>> +{
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> + struct uuid_regs image_uuid;
>> + char image_id_str[40];
>> + int ret, num_of_components;
>> +
>> + num_of_components = get_nr_lfa_components();
>> + if (num_of_components <= 0) {
>> + pr_err("Error getting number of LFA components\n");
>> + return -ENODEV;
>> + }
>> +
>> + for (int i = 0; i < num_of_components; i++) {
>> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
>> + reg.a1 = i; /* fw_seq_id under consideration */
>> + arm_smccc_1_2_invoke(®, ®);
>> + if (reg.a0 == LFA_SUCCESS) {
>> + image_uuid.uuid_lo = reg.a1;
>> + image_uuid.uuid_hi = reg.a2;
>> +
>> + snprintf(image_id_str, sizeof(image_id_str), "%pUb",
>> + &image_uuid);
>> + ret = update_fw_image_node(image_id_str, i,
>> + reg.a3, reg.a4, reg.a5);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init lfa_init(void)
>> +{
>> + struct arm_smccc_1_2_regs reg = { 0 };
>> + int err;
>> +
>> + reg.a0 = LFA_1_0_FN_GET_VERSION;
>> + arm_smccc_1_2_invoke(®, ®);
>> + if (reg.a0 == -LFA_NOT_SUPPORTED) {
>> + pr_info("Live Firmware activation: no firmware agent found\n");
>> + return -ENODEV;
>> + }
>> +
>> + fw_images_update_wq = alloc_workqueue("fw_images_update_wq",
>> + WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
>> + if (!fw_images_update_wq) {
>> + pr_err("Live Firmware Activation: Failed to allocate
>> workqueue.\n");
>> +
>> + return -ENOMEM;
>> + }
>> +
>> + pr_info("Live Firmware Activation: detected v%ld.%ld\n",
>> + reg.a0 >> 16, reg.a0 & 0xffff);
>> +
>> + lfa_dir = kobject_create_and_add("lfa", firmware_kobj);
>> + if (!lfa_dir)
>> + return -ENOMEM;
>> +
>> + mutex_lock(&lfa_lock);
>> + err = update_fw_images_tree();
>> + if (err != 0)
>> + kobject_put(lfa_dir);
>> +
>> + mutex_unlock(&lfa_lock);
>> + return err;
>> +}
>> +module_init(lfa_init);
>> +
>> +static void __exit lfa_exit(void)
>> +{
>> + flush_workqueue(fw_images_update_wq);
>> + destroy_workqueue(fw_images_update_wq);
>> +
>> + mutex_lock(&lfa_lock);
>> + clean_fw_images_tree();
>> + mutex_unlock(&lfa_lock);
>> +
>> + kobject_put(lfa_dir);
>> +}
>> +module_exit(lfa_exit);
>> +
>> +MODULE_DESCRIPTION("ARM Live Firmware Activation (LFA)");
>> +MODULE_LICENSE("GPL");
>
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2026-01-29 17:18 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: <aXq1qPYTR8vpJfc9@google.com>
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.
Here was the end of that thread:
https://lore.kernel.org/kvm/da3701ea-08ea-45c9-94a8-355205a45f8e@intel.com/
This simplifies the whole thing, no union, no worse case allocations,
etc. I'm just getting back and going through mail so will check out
your full solution. (Thanks!) But from the below I think the fixed
array size code will be better still.
>
> It's also relying on the developer to correctly copy+paste the same
> register in multiple locations: ~5 depending on how you want to
> count.
>
> static u64 *dpamt_args_array_ptr_r12(struct tdx_module_array_args
> *args)
> #1
> {
> WARN_ON_ONCE(tdx_dpamt_entry_pages() > MAX_TDX_ARGS(r12));
> #2
>
> return &args->args_array[TDX_ARG_INDEX(r12)];
> #3
>
>
> u64 guest_memory_pamt_page[MAX_TDX_ARGS(r12)];
> #4
>
>
> u64 *args_array = dpamt_args_array_ptr_r12(&args);
> #5
Yea it could probably use another DEFINE or two to make it less error
prone. Vanilla DPAMT has 4 instances of rdx.
>
> After all of that boilerplate, the caller _still_ has to do the
> actual memcpy(), and for me at least, all of the above makes it
> _harder_ to understand what the code is doing.
>
> Drop the struct+union overlay and just provide a helper with wrappers
> to copy to/from a tdx_module_args structure. It's far from
> bulletproof, but it at least avoids an immediate buffer overflow, and
> defers to the kernel owner with respect to handling uninitialized
> stack data.
>
> /*
> * 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))
> return;
>
> /* Copy PAMT page PA's to/from the struct per the TDX ABI.
> */
> if (copy_to_regs)
> memcpy(reg, pamt_pa_array, size);
> else
> memcpy(pamt_pa_array, reg, size);
> }
>
> #define dpamt_copy_from_regs(dst, args, reg) \
> dpamt_copy_regs_array(args, &(args)->reg, dst, false)
>
> #define dpamt_copy_to_regs(args, reg, src) \
> dpamt_copy_regs_array(args, &(args)->reg, src, true)
>
> As far as the on-stack allocations go, why bother being precise?
> Except for paranoid setups which explicitly initialize the stack,
> "allocating" ~48 unused bytes is literally free. Not to mention the
> cost relative to the latency of a SEAMCALL is in the noise.
>
> /*
> * When declaring PAMT arrays on the stack, use the maximum
> theoretical number
> * of entries that can be squeezed into a SEAMCALL, as stack
> allocations are
> * practically free, i.e. any wasted space is a non-issue.
> */
> #define MAX_NR_DPAMT_ARGS (sizeof(struct tdx_module_args) /
> sizeof(u64))
>
>
> With that, callers don't have to regurgitate the same register
> multiple times, and we don't need a new wrapper for every variation
> of SEAMCALL.
> E.g.
>
>
> u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
>
> ...
>
> bool dpamt = tdx_supports_dynamic_pamt(&tdx_sysinfo) &&
> level == PG_LEVEL_2M;
> u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
> struct tdx_module_args args = {
> .rcx = gpa | pg_level_to_tdx_sept_level(level),
> .rdx = tdx_tdr_pa(td),
> .r8 = page_to_phys(new_sp),
> };
> u64 ret;
>
> if (!tdx_supports_demote_nointerrupt(&tdx_sysinfo))
> return TDX_SW_ERROR;
>
> if (dpamt) {
> if (alloc_pamt_array(pamt_pa_array, pamt_cache))
> return TDX_SW_ERROR;
>
> dpamt_copy_to_regs(&args, r12, pamt_pa_array);
> }
>
> Which to me is easier to read and much more intuitive than:
>
>
> u64 guest_memory_pamt_page[MAX_TDX_ARGS(r12)];
> struct tdx_module_array_args args = {
> .args.rcx = gpa | pg_level_to_tdx_sept_level(level),
> .args.rdx = tdx_tdr_pa(td),
> .args.r8 = PFN_PHYS(page_to_pfn(new_sp)),
> };
> struct tdx_module_array_args retry_args;
> int i = 0;
> u64 ret;
>
> if (dpamt) {
> u64 *args_array = dpamt_args_array_ptr_r12(&args);
>
> if (alloc_pamt_array(guest_memory_pamt_page,
> pamt_cache))
> return TDX_SW_ERROR;
>
> /*
> * Copy PAMT page PAs of the guest memory into the
> struct per the
> * TDX ABI
> */
> memcpy(args_array, guest_memory_pamt_page,
> tdx_dpamt_entry_pages() *
> sizeof(*args_array));
> }
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.
Dave didn't like the solution and suggested the union actually:
https://lore.kernel.org/kvm/355ad607-52ed-42cc-9a48-63aaa49f4c68@intel.com/#t
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.
But I think it's all moot because the fixed size-2 solution doesn't
need union or array copying. They can be just normal tdx_module_args
args.
^ permalink raw reply
* Re: [RFC PATCH v5 00/45] TDX: Dynamic PAMT + S-EPT Hugepage
From: Dave Hansen @ 2026-01-29 17:17 UTC (permalink / raw)
To: Konrad Rzeszutek Wilk, Sean Christopherson
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: <aXuVR0kq_K1TYwlR@char.us.oracle.com>
On 1/29/26 09:13, Konrad Rzeszutek Wilk wrote:
> On Wed, Jan 28, 2026 at 05:14:32PM -0800, Sean Christopherson wrote:
>> This is a combined series of Dynamic PAMT (from Rick), and S-EPT hugepage
>> support (from Yan). Except for some last minute tweaks to the DPAMT array
>> args stuff, a version of this based on a Google-internal kernel has been
>> moderately well tested (thanks Vishal!). But overall it's still firmly RFC
>> as I have deliberately NOT addressed others feedback from v4 of DPAMT and v3
> What does PAMT stand for? Is there a design document somewhere?
It's all in here (I hope):
https://lore.kernel.org/kvm/20250918232224.2202592-1-rick.p.edgecombe@intel.com/
^ permalink raw reply
* Re: [RFC PATCH v5 00/45] TDX: Dynamic PAMT + S-EPT Hugepage
From: Konrad Rzeszutek Wilk @ 2026-01-29 17:13 UTC (permalink / raw)
To: Sean Christopherson
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: <20260129011517.3545883-1-seanjc@google.com>
On Wed, Jan 28, 2026 at 05:14:32PM -0800, Sean Christopherson wrote:
> This is a combined series of Dynamic PAMT (from Rick), and S-EPT hugepage
> support (from Yan). Except for some last minute tweaks to the DPAMT array
> args stuff, a version of this based on a Google-internal kernel has been
> moderately well tested (thanks Vishal!). But overall it's still firmly RFC
> as I have deliberately NOT addressed others feedback from v4 of DPAMT and v3
What does PAMT stand for? Is there a design document somewhere?
> of S-EPT hugepage (mostly lack of cycles), and there's at least one patch in
> here that shouldn't be merged as-is (the quick-and-dirty switch from struct
> page to raw pfns).
>
> My immediate goal is to solidify the designs for DPAMT and S-EPT hugepage.
> Given the substantial design changes I am proposing, posting an end-to-end
> RFC seemed like a much better method than trying to communicate my thoughts
> piecemeal.
>
> As for landing these series, I think the fastest overall approach would be
> to land patches 1-4 asap (tangentially related cleanups and fixes), agree
Should they be split out as non-RFC then?
> on a design (hopefully), and then hand control back to Rick and Yan to polish
> their respective series for merge.
>
> I also want to land the VMXON series[*] before DPAMT, because there's a nasty
> wart where KVM wires up a DPAMT-specific hook even if DPAMT is disabled,
> because KVM's ordering needs to set the vendor hooks before tdx_sysinfo is
> ready. Decoupling VMXON from KVM solves that problem, because it lets the
> TDX subsystem parse sysinfo before TDX is loaded.
>
> Beyond that dependency, I am comfortable landing both DPAMT and S-EPT hugepage
> support without any other prereqs, i.e. without an in-tree way to light up
> the S-EPT hugepage code due to lack of hugepage support in guest_memfd.
Can there be test-cases? Or simple code posted for QEMU which is the
tool that 99% of kernel engineers use?
> Outside of the guest_memfd arch hook for in-place conversion, S-EPT hugepage
> support doesn't have any direction dependencies/conflicts with guest_memfd
> hugepage or in-place conversion support (which is great, because it means we
> didn't totally botch the design!). E.g. Vishal's been able to test this code
> precisely because it applies relatively cleanly on an internal branch with a
> whole pile of guest_memfd changes.
>
> Applies on kvm-x86 next (specifically kvm-x86-next-2026.01.23).
>
> [*] https://lore.kernel.org/all/20251206011054.494190-1-seanjc@google.com
>
> P.S. I apologize if I clobbered any of the Author attribution or SoBs. I
> was moving patches around and synchronizing between an internal tree
> and this upstream version, so things may have gotten a bit wonky.
>
> Isaku Yamahata (1):
> KVM: x86/tdp_mmu: Alloc external_spt page for mirror page table
> splitting
>
> Kiryl Shutsemau (12):
> x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
> x86/tdx: Add helpers to check return status codes
> x86/virt/tdx: Allocate page bitmap for Dynamic PAMT
> x86/virt/tdx: Allocate reference counters for PAMT memory
> x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
> x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
> x86/virt/tdx: Optimize tdx_alloc/free_control_page() helpers
> KVM: TDX: Allocate PAMT memory for TD and vCPU control structures
> KVM: TDX: Get/put PAMT pages when (un)mapping private memory
> x86/virt/tdx: Enable Dynamic PAMT
> Documentation/x86: Add documentation for TDX's Dynamic PAMT
> x86/virt/tdx: Get/Put DPAMT page pair if and only if mapping size is
> 4KB
>
> Rick Edgecombe (3):
> x86/virt/tdx: Simplify tdmr_get_pamt_sz()
> x86/tdx: Add APIs to support get/put of DPAMT entries from KVM, under
> spinlock
> KVM: x86/mmu: Prevent hugepage promotion for mirror roots in fault
> path
>
> Sean Christopherson (22):
> x86/tdx: Use pg_level in TDX APIs, not the TDX-Module's 0-based level
> KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE
> "fails"
> KVM: TDX: Account all non-transient page allocations for per-TD
> structures
> KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
> KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use
> .set_external_spte() for all
> KVM: x86/mmu: Fold set_external_spte_present() into its sole caller
> KVM: x86/mmu: Plumb the SPTE _pointer_ into the TDP MMU's
> handle_changed_spte()
> KVM: x86/mmu: Propagate mirror SPTE removal to S-EPT in
> handle_changed_spte()
> KVM: x86: Rework .free_external_spt() into .reclaim_external_sp()
> KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page
> allocator
> KVM: x86/mmu: Allocate/free S-EPT pages using
> tdx_{alloc,free}_control_page()
> *** DO NOT MERGE *** x86/virt/tdx: Don't assume guest memory is backed
> by struct page
> x86/virt/tdx: Extend "reset page" quirk to support huge pages
> KVM: x86/mmu: Plumb the old_spte into kvm_x86_ops.set_external_spte()
> KVM: TDX: Hoist tdx_sept_remove_private_spte() above
> set_private_spte()
> KVM: TDX: Handle removal of leaf SPTEs in .set_private_spte()
> KVM: TDX: Add helper to handle mapping leaf SPTE into S-EPT
> KVM: TDX: Move S-EPT page demotion TODO to tdx_sept_set_private_spte()
> KVM: x86/mmu: Add Dynamic PAMT support in TDP MMU for vCPU-induced
> page split
> KVM: guest_memfd: Add helpers to get start/end gfns give
> gmem+slot+pgoff
> *** DO NOT MERGE *** KVM: guest_memfd: Add pre-zap arch hook for
> shared<=>private conversion
> KVM: x86/mmu: Add support for splitting S-EPT hugepages on conversion
>
> Xiaoyao Li (1):
> x86/virt/tdx: Add API to demote a 2MB mapping to 512 4KB mappings
>
> Yan Zhao (6):
> x86/virt/tdx: Enhance tdh_mem_page_aug() to support huge pages
> x86/virt/tdx: Enhance tdh_phymem_page_wbinvd_hkid() to invalidate huge
> pages
> KVM: TDX: Add core support for splitting/demoting 2MiB S-EPT to 4KiB
> KVM: x86: Introduce hugepage_set_guest_inhibit()
> KVM: TDX: Honor the guest's accept level contained in an EPT violation
> KVM: TDX: Turn on PG_LEVEL_2M
>
> Documentation/arch/x86/tdx.rst | 21 +
> arch/x86/coco/tdx/tdx.c | 10 +-
> arch/x86/include/asm/kvm-x86-ops.h | 9 +-
> arch/x86/include/asm/kvm_host.h | 36 +-
> arch/x86/include/asm/shared/tdx.h | 1 +
> arch/x86/include/asm/shared/tdx_errno.h | 104 +++
> arch/x86/include/asm/tdx.h | 127 ++--
> arch/x86/include/asm/tdx_global_metadata.h | 1 +
> arch/x86/kvm/Kconfig | 1 +
> arch/x86/kvm/mmu.h | 4 +
> arch/x86/kvm/mmu/mmu.c | 34 +-
> arch/x86/kvm/mmu/mmu_internal.h | 11 -
> arch/x86/kvm/mmu/tdp_mmu.c | 315 ++++----
> arch/x86/kvm/mmu/tdp_mmu.h | 2 +
> arch/x86/kvm/vmx/tdx.c | 468 +++++++++---
> arch/x86/kvm/vmx/tdx.h | 5 +-
> arch/x86/kvm/vmx/tdx_arch.h | 3 +
> arch/x86/kvm/vmx/tdx_errno.h | 40 -
> arch/x86/virt/vmx/tdx/tdx.c | 762 +++++++++++++++++---
> arch/x86/virt/vmx/tdx/tdx.h | 6 +-
> arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 7 +
> include/linux/kvm_host.h | 5 +
> include/linux/kvm_types.h | 2 +
> virt/kvm/Kconfig | 4 +
> virt/kvm/guest_memfd.c | 71 +-
> virt/kvm/kvm_main.c | 7 +-
> 26 files changed, 1576 insertions(+), 480 deletions(-)
> create mode 100644 arch/x86/include/asm/shared/tdx_errno.h
> delete mode 100644 arch/x86/kvm/vmx/tdx_errno.h
>
>
> base-commit: e81f7c908e1664233974b9f20beead78cde6343a
> --
> 2.53.0.rc1.217.geba53bf80e-goog
>
>
^ permalink raw reply
* Re: [PATCH v3 06/26] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs
From: Dave Hansen @ 2026-01-29 16:59 UTC (permalink / raw)
To: 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, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <aXt0+lRvpvf5knKP@intel.com>
On 1/29/26 06:55, Chao Gao wrote:
> On Wed, Jan 28, 2026 at 03:03:14PM -0800, Dave Hansen wrote:
...
> Thanks. This is much clearer than my version.
>
> One tiny nit: NP-SEAMLDR isn't SEAM mode software. It is an authenticated code
> module (ACM).
Ahhh, thanks for the correction!
>> Then do this part:
>>
>>> P-SEAMLDR SEAMCALLs differ from SEAMCALLs of the TDX module in terms of
>>> error codes and the handling of the current VMCS.
>> Except I don't even know how the TDX module handles the current VMCS.
>> That probably needs to be in there. Or, it should be brought up in the
>> patch itself that implements this. Or, uplifted to the cover letter.
>
> My logic was:
>
> 1. The kernel communicates with P-SEAMLDR via SEAMCALL, just like with the TDX
> Module.
> 2. But P-SEAMLDR SEAMCALLs and TDX Module SEAMCALLs are slightly different.
>
> So we need some tweaks to the low-level helpers to add separate wrappers for
> P-SEAMLDR SEAMCALLs.
>
> To me, without mentioning #2, these tweaks in this patch (for separate wrappers
> in the next patch) aren't justified.
My objection is that you talk about the VMCS handling in here but
there's no actual VMCS handling. This is the changelog for patch 06/26,
not 07/26.
Don't talk about the VMCS handling here. When you do, make sure you give
enough background.
>>> static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>>> struct tdx_module_args *args)
>>> {
>>> + u64 retry_code = TDX_RND_NO_ENTROPY;
>>> int retry = RDRAND_RETRY_LOOPS;
>>> u64 ret;
>>>
>>> + if (unlikely(is_seamldr_call(fn)))
>>> + retry_code = SEAMLDR_RND_NO_ENTROPY;
>>
>> (un)likey() has two uses:
>>
>> 1. It's in performance critical code and compilers have been
>> demonstrated to be generating bad code.
>> 2. It's in code where it's not obvious what the fast path is
>> and (un)likey() makes the code more readable.
>>
>> Which one is this?
>
> I think #2 although I am happy to drop "unlikely".
But why does it *MATTER*? Is it important to understand that SEAMLDR
calls are rarer than TDX module calls?
^ permalink raw reply
* Re: [PATCH v3 06/26] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs
From: Dave Hansen @ 2026-01-29 16:08 UTC (permalink / raw)
To: Xu Yilun
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: <aXssgOZJteoaJUOz@yilunxu-OptiPlex-7050>
On 1/29/26 01:46, Xu Yilun wrote:
...
> Then with all the doc fixes, we only need minor code change:
>
> @@ -127,7 +127,8 @@ static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
> preempt_disable();
> ret = __seamcall_dirty_cache(func, fn, args);
> preempt_enable();
> - } while (ret == TDX_RND_NO_ENTROPY && --retry);
> + } while ((ret == TDX_RND_NO_ENTROPY ||
> + ret == SEAMLDR_RND_NO_ENTROPY) && --retry);
That's better than what was there in the past, but I do think even this
is pretty silly.
I mean, we (Intel) control all the components. These errors are for the
same dang thing. The people who wrote both components probably sit next
to each other. :)
I think I'd be a bit less grumpy if there was _anything_ else that
demanded a retry. So, let's try to extract the guarantee that the error
spaces are at least unified, in that a TDX module will never return
SEAMLDR_RND_NO_ENTROPY to mean something else and a SEAMLDR will never
return TDX_RND_NO_ENTROPY. Then, maybe talk them into doing a unified
thing from here on out.
But, for now, drop this patch. We'll just assume the P-SEAMLDR doesn't
have "no entropy" errors until this is sorted.
^ permalink raw reply
* Re: [PATCH v3 03/26] x86/virt/tdx: Move low level SEAMCALL helpers out of <asm/tdx.h>
From: Dave Hansen @ 2026-01-29 16:03 UTC (permalink / raw)
To: Chao Gao
Cc: Binbin Wu, 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <aXtoZTRlCMSAIeda@intel.com>
On 1/29/26 06:02, Chao Gao wrote:'
...
> But this sounds like the copyright notice should be kept.
>
> Do you mean the copyright notices from the original files should be carried
> over to the new file?
>
> This patch extracts code from arch/x86/include/asm/tdx.h and
> arch/x86/virt/vmx/tdx/tdx.c. They have:
>
> Copyright (C) 2021-2022 Intel Corporation
> Copyright(c) 2023 Intel Corporation.
>
> So for the new file, the copyright notice should be
>
> Copyright (C) 2021-2023 Intel Corporation
> ?
The most straightforward thing to do is to copy the gunk from the
original file:
Copyright (C) 2021-2023 Intel Corporation
... which is as of today the "official" Intel way of doing it with the
"(C)" just like that.
along with:
/* SPDX-License-Identifier: GPL-2.0 */
and a note in the changelog about what you did. There's no need to do
any more than that because that's what git is for.
^ permalink raw reply
* Re: [RFC PATCH v5 44/45] KVM: x86/mmu: Add support for splitting S-EPT hugepages on conversion
From: Sean Christopherson @ 2026-01-29 15:39 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-45-seanjc@google.com>
On Wed, Jan 28, 2026, Sean Christopherson wrote:
> #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT
> +static int __tdp_mmu_split_mirror_huge_pages(struct kvm *kvm,
> + struct kvm_mmu_page *root,
> + gfn_t gfn, int target_level)
> +{
> + gfn_t end = gfn + KVM_PAGES_PER_HPAGE(target_level + 1);
> +
> + return tdp_mmu_split_huge_pages_root(kvm, root, gfn, end, target_level, false);
> +}
> +
> +static int tdp_mmu_split_mirror_huge_pages(struct kvm *kvm,
> + struct kvm_mmu_page *root,
> + gfn_t start, gfn_t end, int level)
> +{
> +
> + gfn_t head = gfn_round_for_level(start, level + 1);
> + gfn_t tail = gfn_round_for_level(end, level + 1);
> + int r;
> +
> + if (head != start) {
> + r = __tdp_mmu_split_mirror_huge_pages(kvm, root, head, level);
> + if (r)
> + return r;
> + }
> +
> + if (tail != end && (head != tail || head == start)) {
> + r = __tdp_mmu_split_mirror_huge_pages(kvm, root, tail, level);
> + if (r)
> + return r;
> + }
> +
> + return 0;
> +}
> +
> int kvm_arch_gmem_convert(struct kvm *kvm, gfn_t start, gfn_t end,
> bool to_private)
> {
> + struct kvm_mmu_page *root;
> + int r;
> +
> + /*
> + * When converting from private=>shared, KVM must first split potential
> + * hugepages, as KVM mustn't overzap private mappings for TDX guests,
> + * i.e. must zap _exactly_ [start, end). Split potential hugepages at
> + * the head and tail of the to-be-converted (and thus zapped) range so
> + * that KVM doesn't overzap due to dropping a hugepage that doesn't
> + * fall wholly inside the range.
> + */
> + if (to_private || !kvm_has_mirrored_tdp(kvm))
> + return 0;
> +
> + /*
> + * Acquire the external cache lock, a.k.a. the Dynamic PAMT lock, to
> + * protect the per-VM cache of pre-allocate pages used to populate the
> + * Dynamic PAMT when splitting S-EPT huge pages.
> + */
> + guard(mutex)(&kvm->arch.tdp_mmu_external_cache_lock);
> +
> + guard(write_lock)(&kvm->mmu_lock);
> +
> + /*
> + * TODO: Also split from PG_LEVEL_1G => PG_LEVEL_2M when KVM supports
> + * 1GiB S-EPT pages.
> + */
> + __for_each_tdp_mmu_root_yield_safe(kvm, root, 0, KVM_MIRROR_ROOTS) {
> + r = tdp_mmu_split_mirror_huge_pages(kvm, root, start, end, PG_LEVEL_4K);
> + if (r)
This needs to call kvm_tdp_mmu_put_root() on failure. But if we instead add
kvm_tdp_mmu_mirrors_split_huge_pages() for use in handling mismatched ACCEPT,
this code goes away.
And then the bulk of this code can live in tdx.c instead of tdp_mmu.c, and the
pamt mutex can live in kvm_tdx instead of kvm_arch.
Compile tested only...
---
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 22 Jan 2026 07:36:47 -0800
Subject: [PATCH] KVM: x86/mmu: Add support for splitting S-EPT hugepages on
conversion
Add support for splitting S-EPT hugepages in preparation for converting a
subset of a hugepage to be shared, as KVM must precisely zap/remove S-EPT
entries to avoid clobbering guest memory (the lifetime of guest private
memory is tied to the S-EPT). I.e. KVM needs to first split a hugepage so
that only the to-be-converted small pages can be zapped.
To avoid unnecessary work, e.g. if only the tail/end page of massive region
isn't aligned to the conversion, explicitly detect unaligned head and tail
pages relative to the max page size support by KVM, i.e. head/tail pages
that will undergo partial conversion.
To support splitting an S-EPT hugepage without a vCPU, add a per-VM PAMT
cache, along with a mutex to guard the cache. Using a mutex, e.g. versus
a spinlock, is important at it allows KVM to allocate memory *without*
dropping the lock, i.e. so that the PAMT cache can be topped-up as needed
without needed to juggle arch.tdp_mmu_external_cache_lock.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/include/asm/kvm-x86-ops.h | 1 +
arch/x86/include/asm/kvm_host.h | 3 +-
arch/x86/kvm/mmu/mmu.c | 2 +-
arch/x86/kvm/mmu/tdp_mmu.c | 7 ++-
arch/x86/kvm/vmx/tdx.c | 96 ++++++++++++++++++++++++++++--
arch/x86/kvm/vmx/tdx.h | 3 +
arch/x86/kvm/x86.c | 2 +-
7 files changed, 102 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 3ca56fe6b951..6083fb07cd3b 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -149,6 +149,7 @@ KVM_X86_OP_OPTIONAL(alloc_apic_backing_page)
KVM_X86_OP_OPTIONAL_RET0(gmem_prepare)
KVM_X86_OP_OPTIONAL_RET0(gmem_max_mapping_level)
KVM_X86_OP_OPTIONAL(gmem_invalidate)
+KVM_X86_OP_OPTIONAL_RET0(gmem_convert)
#undef KVM_X86_OP
#undef KVM_X86_OP_OPTIONAL
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 385f1cf32d70..cd3e7dc6ab9b 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1861,7 +1861,7 @@ struct kvm_x86_ops {
u64 new_spte, enum pg_level level);
void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
struct kvm_mmu_page *sp);
- int (*topup_external_cache)(struct kvm_vcpu *vcpu, int min);
+ int (*topup_external_cache)(struct kvm *kvm, struct kvm_vcpu *vcpu, int min);
bool (*has_wbinvd_exit)(void);
@@ -1950,6 +1950,7 @@ struct kvm_x86_ops {
void *(*alloc_apic_backing_page)(struct kvm_vcpu *vcpu);
int (*gmem_prepare)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, int max_order);
void (*gmem_invalidate)(kvm_pfn_t start, kvm_pfn_t end);
+ int (*gmem_convert)(struct kvm *kvm, gfn_t start, gfn_t end, bool to_private);
int (*gmem_max_mapping_level)(struct kvm *kvm, kvm_pfn_t pfn, bool is_private);
};
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c2765bfc8492..62bf6bec2df2 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -606,7 +606,7 @@ static int mmu_topup_memory_caches(struct kvm_vcpu *vcpu, bool maybe_indirect)
if (r)
return r;
- r = kvm_x86_call(topup_external_cache)(vcpu, PT64_ROOT_MAX_LEVEL);
+ r = kvm_x86_call(topup_external_cache)(vcpu->kvm, vcpu, PT64_ROOT_MAX_LEVEL);
if (r)
return r;
}
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index a45d8ee91481..a32192c35099 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1447,7 +1447,8 @@ bool kvm_tdp_mmu_wrprot_slot(struct kvm *kvm,
return spte_set;
}
-static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct tdp_iter *iter)
+static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
+ struct tdp_iter *iter)
{
struct kvm_mmu_page *sp;
@@ -1464,7 +1465,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct tdp_iter *iter)
if (!sp->external_spt)
goto err_external_spt;
- if (kvm_x86_call(topup_external_cache)(kvm_get_running_vcpu(), 1))
+ if (kvm_x86_call(topup_external_cache)(kvm, kvm_get_running_vcpu(), 1))
goto err_external_split;
}
@@ -1556,7 +1557,7 @@ static int tdp_mmu_split_huge_pages_root(struct kvm *kvm,
else
write_unlock(&kvm->mmu_lock);
- sp = tdp_mmu_alloc_sp_for_split(&iter);
+ sp = tdp_mmu_alloc_sp_for_split(kvm, &iter);
if (shared)
read_lock(&kvm->mmu_lock);
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9f2ef46f87b0..c4050d94fb4d 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -607,6 +607,8 @@ void tdx_vm_destroy(struct kvm *kvm)
{
struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
+ tdx_free_pamt_cache(&kvm_tdx->pamt_cache);
+
tdx_reclaim_td_control_pages(kvm);
kvm_tdx->state = TD_STATE_UNINITIALIZED;
@@ -629,6 +631,8 @@ int tdx_vm_init(struct kvm *kvm)
{
struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
+ tdx_init_pamt_cache(&kvm_tdx->pamt_cache);
+
kvm->arch.has_protected_state = true;
/*
* TDX Module doesn't allow the hypervisor to modify the EOI-bitmap,
@@ -1285,6 +1289,66 @@ static int tdx_map_gpa(struct kvm_vcpu *vcpu)
return 1;
}
+static int __tdx_sept_split_huge_pages(struct kvm *kvm, gfn_t gfn, int target_level)
+{
+ gfn_t end = gfn + KVM_PAGES_PER_HPAGE(target_level + 1);
+
+ return kvm_tdp_mmu_mirrors_split_huge_pages(kvm, gfn, end, target_level);
+}
+
+static int tdx_sept_split_huge_pages(struct kvm *kvm, gfn_t start, gfn_t end,
+ int target_level)
+{
+
+ gfn_t head = gfn_round_for_level(start, target_level + 1);
+ gfn_t tail = gfn_round_for_level(end, target_level + 1);
+ int r;
+
+ if (head != start) {
+ r = __tdx_sept_split_huge_pages(kvm, head, target_level);
+ if (r)
+ return r;
+ }
+
+ if (tail != end && (head != tail || head == start)) {
+ r = __tdx_sept_split_huge_pages(kvm, tail, target_level);
+ if (r)
+ return r;
+ }
+
+ return 0;
+}
+
+static int tdx_gmem_convert(struct kvm *kvm, gfn_t start, gfn_t end,
+ bool to_private)
+{
+ /*
+ * When converting from private=>shared, KVM must first split potential
+ * hugepages, as KVM mustn't overzap private mappings for TDX guests,
+ * i.e. must zap _exactly_ [start, end). Split potential hugepages at
+ * the head and tail of the to-be-converted (and thus zapped) range so
+ * that KVM doesn't overzap due to dropping a hugepage that doesn't
+ * fall wholly inside the range.
+ */
+ if (to_private || !kvm_has_mirrored_tdp(kvm))
+ return 0;
+
+ /*
+ * Acquire the external cache lock, a.k.a. the Dynamic PAMT lock, to
+ * protect the per-VM cache of pre-allocate pages used to populate the
+ * Dynamic PAMT when splitting S-EPT huge pages.
+ */
+ guard(mutex)(&to_kvm_tdx(kvm)->pamt_cache_lock);
+
+ guard(write_lock)(&kvm->mmu_lock);
+
+ /*
+ * TODO: Also split from PG_LEVEL_1G => PG_LEVEL_2M when KVM supports
+ * 1GiB S-EPT pages.
+ */
+ return tdx_sept_split_huge_pages(kvm, start, end, PG_LEVEL_4K);
+}
+
static int tdx_report_fatal_error(struct kvm_vcpu *vcpu)
{
struct vcpu_tdx *tdx = to_tdx(vcpu);
@@ -1621,15 +1685,32 @@ void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
td_vmcs_write64(to_tdx(vcpu), SHARED_EPT_POINTER, root_hpa);
}
-static int tdx_topup_external_pamt_cache(struct kvm_vcpu *vcpu, int min)
+static struct tdx_pamt_cache *tdx_get_pamt_cache(struct kvm *kvm,
+ struct kvm_vcpu *vcpu)
{
+ if (KVM_BUG_ON(vcpu && vcpu->kvm != kvm, kvm))
+ return NULL;
+
+ if (vcpu)
+ return &to_tdx(vcpu)->pamt_cache;
+
+ lockdep_assert_held(&to_kvm_tdx(kvm)->pamt_cache_lock);
+ return &to_kvm_tdx(kvm)->pamt_cache;
+}
+
+static int tdx_topup_external_pamt_cache(struct kvm *kvm,
+ struct kvm_vcpu *vcpu, int min)
+{
+ struct tdx_pamt_cache *pamt_cache;
+
if (!tdx_supports_dynamic_pamt(tdx_sysinfo))
return 0;
- if (WARN_ON_ONCE(!vcpu))
+ pamt_cache = tdx_get_pamt_cache(kvm, vcpu);
+ if (!pamt_cache)
return -EIO;
- return tdx_topup_pamt_cache(&to_tdx(vcpu)->pamt_cache, min);
+ return tdx_topup_pamt_cache(pamt_cache, min);
}
static int tdx_mem_page_add(struct kvm *kvm, gfn_t gfn, enum pg_level level,
@@ -1792,8 +1873,8 @@ static struct page *tdx_spte_to_external_spt(struct kvm *kvm, gfn_t gfn,
static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
u64 new_spte, enum pg_level level)
{
- struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
+ struct tdx_pamt_cache *pamt_cache;
gpa_t gpa = gfn_to_gpa(gfn);
u64 err, entry, level_state;
struct page *external_spt;
@@ -1804,7 +1885,8 @@ static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
if (!external_spt)
return -EIO;
- if (KVM_BUG_ON(!vcpu || vcpu->kvm != kvm, kvm))
+ pamt_cache = tdx_get_pamt_cache(kvm, kvm_get_running_vcpu());
+ if (!pamt_cache)
return -EIO;
err = tdh_do_no_vcpus(tdh_mem_range_block, kvm, &kvm_tdx->td, gpa,
@@ -1816,7 +1898,7 @@ static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
err = tdh_do_no_vcpus(tdh_mem_page_demote, kvm, &kvm_tdx->td, gpa,
level, spte_to_pfn(old_spte), external_spt,
- &to_tdx(vcpu)->pamt_cache, &entry, &level_state);
+ pamt_cache, &entry, &level_state);
if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_DEMOTE, entry, level_state, kvm))
return -EIO;
@@ -3776,6 +3858,8 @@ void __init tdx_hardware_setup(void)
vt_x86_ops.set_external_spte = tdx_sept_set_private_spte;
vt_x86_ops.reclaim_external_sp = tdx_sept_reclaim_private_sp;
+ vt_x86_ops.gmem_convert = tdx_gmem_convert;
+
/*
* FIXME: Wire up the PAMT hook iff DPAMT is supported, once VMXON is
* moved out of KVM and tdx_bringup() is folded into here.
diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
index f444fc84d93b..2bb4604a64ca 100644
--- a/arch/x86/kvm/vmx/tdx.h
+++ b/arch/x86/kvm/vmx/tdx.h
@@ -48,6 +48,9 @@ struct kvm_tdx {
* Set/unset is protected with kvm->mmu_lock.
*/
bool wait_for_sept_zap;
+
+ struct tdx_pamt_cache pamt_cache;
+ struct mutex pamt_cache_lock;
};
/* TDX module vCPU states */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c80cc60e7862..c3d71ba9a1dc 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -14061,7 +14061,7 @@ void kvm_arch_gmem_invalidate(kvm_pfn_t start, kvm_pfn_t end)
int kvm_arch_gmem_convert(struct kvm *kvm, gfn_t start, gfn_t end,
bool to_private)
{
- return 0;
+ return kvm_x86_call(gmem_convert)(kvm, start, end, to_private);
}
#endif
#endif
base-commit: b2791d61e9774d8575525816e864d2e09ee9090a
--
^ permalink raw reply related
* Re: [RFC PATCH v5 41/45] KVM: TDX: Honor the guest's accept level contained in an EPT violation
From: Sean Christopherson @ 2026-01-29 15:32 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-42-seanjc@google.com>
On Wed, Jan 28, 2026, Sean Christopherson wrote:
> +int kvm_tdp_mmu_split_huge_pages(struct kvm_vcpu *vcpu, gfn_t start, gfn_t end,
> + int target_level)
> +{
> + struct kvm_mmu_page *root = root_to_sp(vcpu->arch.mmu->root.hpa);
This is wrong, mmu->root.hpa is the shared root, not the mirror root. Dittof for
the sanity check in tdx_handle_mismatched_accept().
Rather than operate on the vCPU's root, I think it makes sense to add an API to
operate on all mirror roots. In practice, there can only be one valid mirror
root, so KVM isn't actually doing more work. Then TDX can reuse that API for
splitting the head+tail pages when preparing for a partial shared=>private
conversion.
Slotted in before this patch:
---
From: Sean Christopherson <seanjc@google.com>
Date: Thu, 29 Jan 2026 15:21:30 +0000
Subject: [PATCH] KVM: x86/mmu: Add a TDP MMU API to split hugepages for mirror
roots
Add an exported API to split hugepages in mirror roots for a given gfn
range. TDX will use the API to split hugepages in preparation for
partially converting a hugepage from private to shared, and for splitting
a hugepage to match the guest's ACCEPT level.
For all intents and purposes, no functional change intended.
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/mmu/tdp_mmu.c | 39 ++++++++++++++++++++++++++++----------
arch/x86/kvm/mmu/tdp_mmu.h | 2 ++
2 files changed, 31 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index e32034bfca5a..a45d8ee91481 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -1597,6 +1597,26 @@ static int tdp_mmu_split_huge_pages_root(struct kvm *kvm,
return 0;
}
+static int tdp_mmu_split_huge_pages(struct kvm *kvm, int as_id,
+ enum kvm_tdp_mmu_root_types type,
+ gfn_t start, gfn_t end,
+ int target_level, bool shared)
+{
+ struct kvm_mmu_page *root;
+ int r;
+
+ kvm_lockdep_assert_mmu_lock_held(kvm, shared);
+
+ __for_each_tdp_mmu_root_yield_safe(kvm, root, as_id, type) {
+ r = tdp_mmu_split_huge_pages_root(kvm, root, start, end,
+ target_level, shared);
+ if (r) {
+ kvm_tdp_mmu_put_root(kvm, root);
+ return r;
+ }
+ }
+ return 0;
+}
/*
* Try to split all huge pages mapped by the TDP MMU down to the target level.
@@ -1606,18 +1626,17 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
gfn_t start, gfn_t end,
int target_level, bool shared)
{
- struct kvm_mmu_page *root;
- int r = 0;
+ tdp_mmu_split_huge_pages(kvm, slot->as_id, KVM_VALID_ROOTS, start, end,
+ target_level, shared);
+}
- kvm_lockdep_assert_mmu_lock_held(kvm, shared);
- for_each_valid_tdp_mmu_root_yield_safe(kvm, root, slot->as_id) {
- r = tdp_mmu_split_huge_pages_root(kvm, root, start, end, target_level, shared);
- if (r) {
- kvm_tdp_mmu_put_root(kvm, root);
- break;
- }
- }
+int kvm_tdp_mmu_mirrors_split_huge_pages(struct kvm *kvm, gfn_t start,
+ gfn_t end, int target_level)
+{
+ return tdp_mmu_split_huge_pages(kvm, 0, KVM_MIRROR_ROOTS, start, end,
+ target_level, false);
}
+EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_tdp_mmu_mirrors_split_huge_pages);
static bool tdp_mmu_need_write_protect(struct kvm *kvm, struct kvm_mmu_page *sp)
{
diff --git a/arch/x86/kvm/mmu/tdp_mmu.h b/arch/x86/kvm/mmu/tdp_mmu.h
index bd62977c9199..a6919de10ca2 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.h
+++ b/arch/x86/kvm/mmu/tdp_mmu.h
@@ -97,6 +97,8 @@ void kvm_tdp_mmu_try_split_huge_pages(struct kvm *kvm,
const struct kvm_memory_slot *slot,
gfn_t start, gfn_t end,
int target_level, bool shared);
+int kvm_tdp_mmu_mirrors_split_huge_pages(struct kvm *kvm, gfn_t start,
+ gfn_t end, int target_level);
static inline void kvm_tdp_mmu_walk_lockless_begin(void)
{
base-commit: 86c3bb72bf5c6201636529ee4609334b0887c6e3
--
^ permalink raw reply related
* Re: [PATCH v3 06/26] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-01-29 14:55 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: <e2245231-ee39-40aa-bfdc-e43419fa30f4@intel.com>
On Wed, Jan 28, 2026 at 03:03:14PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> P-SEAMLDR is another component alongside the TDX module within the
>> protected SEAM range. P-SEAMLDR can update the TDX module at runtime.
>> Software can talk with P-SEAMLDR via SEAMCALLs with the bit 63 of RAX
>> (leaf number) set to 1 (a.k.a P-SEAMLDR SEAMCALLs).
>
>This text kinda bugs me. It's OK, but needs improvement.
>
>First, don't explain the ABI in the changelog. Nobody cares that it's
>bit 63.
>
>
>Background:
>
> The TDX architecture uses the "SEAMCALL" instruction to
> communicate with SEAM mode software. Right now, the only SEAM
> mode software that the kernel communicates with is the TDX
> module. But, there are actually some components that run in SEAM
> mode but that are separate from the TDX module: that SEAM
> loaders. Right now, the only component that communicates with
> them is the BIOS which loads the TDX module itself at boot. But,
> to support updating the TDX module, the kernel now needs to be
> able to talk to one of the the SEAM loaders: the Persistent
> loader or "P-SEAMLDR".
Thanks. This is much clearer than my version.
One tiny nit: NP-SEAMLDR isn't SEAM mode software. It is an authenticated code
module (ACM).
>
>Then do this part:
>
>> P-SEAMLDR SEAMCALLs differ from SEAMCALLs of the TDX module in terms of
>> error codes and the handling of the current VMCS.
>Except I don't even know how the TDX module handles the current VMCS.
>That probably needs to be in there. Or, it should be brought up in the
>patch itself that implements this. Or, uplifted to the cover letter.
My logic was:
1. The kernel communicates with P-SEAMLDR via SEAMCALL, just like with the TDX
Module.
2. But P-SEAMLDR SEAMCALLs and TDX Module SEAMCALLs are slightly different.
So we need some tweaks to the low-level helpers to add separate wrappers for
P-SEAMLDR SEAMCALLs.
To me, without mentioning #2, these tweaks in this patch (for separate wrappers
in the next patch) aren't justified.
<snip>
>> static __always_inline u64 sc_retry(sc_func_t func, u64 fn,
>> struct tdx_module_args *args)
>> {
>> + u64 retry_code = TDX_RND_NO_ENTROPY;
>> int retry = RDRAND_RETRY_LOOPS;
>> u64 ret;
>>
>> + if (unlikely(is_seamldr_call(fn)))
>> + retry_code = SEAMLDR_RND_NO_ENTROPY;
>
>(un)likey() has two uses:
>
>1. It's in performance critical code and compilers have been
> demonstrated to be generating bad code.
>2. It's in code where it's not obvious what the fast path is
> and (un)likey() makes the code more readable.
>
>Which one is this?
I think #2 although I am happy to drop "unlikely".
>
>Second, this is nitpicky, but I'd rather this be:
>
> if (is_seamldr_call(fn))
> retry_code = SEAMLDR_RND_NO_ENTROPY;
> else
> retry_code = TDX_RND_NO_ENTROPY;
Will do.
<snip>
>> +static inline void seamldr_err(u64 fn, u64 err, struct tdx_module_args *args)
>> +{
>> + /*
>> + * Note: P-SEAMLDR leaf numbers are printed in hex as they have
>> + * bit 63 set, making them hard to read and understand if printed
>> + * in decimal
>> + */
>> + pr_err("P-SEAMLDR (%llx) failed: %#016llx\n", fn, err);
>> +}
>
>Oh, lovely.
>
>Didn't you just propose changing the module SEAMCALL leaf numbers in
>decimal? Isn't it a little crazy to do one in decimal and the other in hex?
Yes, that's crazy. I'll just reuse seamcall_err(), so leaf numbers will be
printed in hex for both the TDX Module and P-SEAMLDR
>
>I'd really rather just see the TDX documentation changed.
I'll submit a request for TDX documentation to display leaf numbers in both hex
and decimal.
>
>But, honestly, I'd probably just leave the thing in hex, drop this hunk,
>and go thwack someone that writes TDX module documentation instead.
>
>> static __always_inline int sc_retry_prerr(sc_func_t func,
>> sc_err_func_t err_func,
>> u64 fn, struct tdx_module_args *args)
>> @@ -96,4 +119,7 @@ static __always_inline int sc_retry_prerr(sc_func_t func,
>> #define seamcall_prerr_ret(__fn, __args) \
>> sc_retry_prerr(__seamcall_ret, seamcall_err_ret, (__fn), (__args))
>>
>> +#define seamldr_prerr(__fn, __args) \
>> + sc_retry_prerr(__seamcall, seamldr_err, (__fn), (__args))
This can be dropped if we don't need to add seamldr_err().
^ permalink raw reply
* Re: [PATCH v3 05/26] coco/tdx-host: Expose TDX Module version
From: Chao Gao @ 2026-01-29 14:07 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
In-Reply-To: <d82dd253-aeea-49c5-a21b-44864bd78f25@intel.com>
On Wed, Jan 28, 2026 at 09:01:35AM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>...
>> This approach follows the pattern used by microcode updates and
>> other CoCo implementations:
>>
>> 1. AMD has a PCI device for the PSP for SEV which provides an
>> existing place to hang their equivalent metadata.
>>
>> 2. ARM CCA will likely have a faux device (although it isn't obvious
>> if they have a need to export version information there) [1]
>>
>> 3. Microcode revisions are exposed as CPU device attributes
>
>I kinda disagree with the idea that this follows existing patterns. It
>uses a *NEW* pattern.
>
>AMD doesn't use a faux device because they *HAVE* a PCI device in their
>architecture. TDX doesn't have a PCI device in its hardware architecture.
>
>ARM CCA doesn't exist in the tree.
>
>CPU microcode doesn't use a faux device. For good reason. The microcode
>version is *actually* per-cpu. It can differ between CPU cores. The TDX
>module version is not per-cpu. There's one and only one global module.
>This is the reason that we need a global, unique device for TDX.
>
>I'm not saying that being new is a bad thing. But let's not pretend this
>is following any kind of existing pattern. Let's explain *why* it needs
>to be different.
Thanks. I understand your point. The pattern I was referring to is: using a
device (PCI device, virtual device, or faux device) and exposing
versions/metadata as device attributes.
You're right if we look at the details, they're not exactly the same pattern.
I'll revise the changelog to make this clearer.
^ permalink raw reply
* Re: [PATCH v3 03/26] x86/virt/tdx: Move low level SEAMCALL helpers out of <asm/tdx.h>
From: Chao Gao @ 2026-01-29 14:02 UTC (permalink / raw)
To: Dave Hansen
Cc: Binbin Wu, 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <1bd9bdcb-6dca-4496-945c-526abee46059@intel.com>
On Wed, Jan 28, 2026 at 08:31:26AM -0800, Dave Hansen wrote:
>On 1/28/26 04:42, Chao Gao wrote:
>>>> diff --git a/arch/x86/virt/vmx/tdx/seamcall.h b/arch/x86/virt/vmx/tdx/seamcall.h
>>>> new file mode 100644
>>>> index 000000000000..0912e03fabfe
>>>> --- /dev/null
>>>> +++ b/arch/x86/virt/vmx/tdx/seamcall.h
>>>> @@ -0,0 +1,99 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/* Copyright (C) 2025 Intel Corporation */
>>> Should this be updated to 2026?
>> Yes. And I may drop the copyright notice if it is not necessary.
>
>No.
Sorry. I am a bit confused..
>
>The copyright is to document the timing of a creative action. Moving
>code is not a creative action.
This sounds like we don't need to add copyright notices for moving code.
>
>If you want to remove it, do it in another patch. If you move code, just
>_move_ _the_ _code_. You can _maybe_ clean up whitespace if you want to
>along the way. But that's it. Don't muck with it unless you have a
>reason. A *good* reason.
But this sounds like the copyright notice should be kept.
Do you mean the copyright notices from the original files should be carried
over to the new file?
This patch extracts code from arch/x86/include/asm/tdx.h and
arch/x86/virt/vmx/tdx/tdx.c. They have:
Copyright (C) 2021-2022 Intel Corporation
Copyright(c) 2023 Intel Corporation.
So for the new file, the copyright notice should be
Copyright (C) 2021-2023 Intel Corporation
?
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-01-29 10:51 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: <e7acf7ed-103b-46aa-a1f6-35bb6292d30f@amd.com>
On Wed, Jan 28, 2026 at 06:38:29PM -0600, Kim Phillips wrote:
> For that last paragraph, how about:
>
> "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."
Better.
> 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?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v3 2/2] x86/sev: Add support to unaccept memory after hot-remove
From: Kiryl Shutsemau @ 2026-01-29 10:40 UTC (permalink / raw)
To: Dave Hansen
Cc: Pratik R. Sampat, linux-mm, linux-coco, x86, linux-kernel, tglx,
mingo, bp, dave.hansen, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth
In-Reply-To: <16aa84a9-83ce-4748-bc73-ccf5cb6ce376@intel.com>
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.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ 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