* Re: [PATCH v3 09/26] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Dave Hansen @ 2026-01-30 16:02 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
In-Reply-To: <aXzD5nOW0NhCHG7+@intel.com>
On 1/30/26 06:44, Chao Gao wrote:
>>> +What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
>>> +Contact: linux-coco@lists.linux.dev
>>> +Description: (RO) Report the number of remaining updates that can be performed.
>>> + The CPU keeps track of TCB versions for each TDX Module that
>>> + has been loaded. Since this tracking database has finite
>>> + capacity, there's a maximum number of Module updates that can
>>> + be performed.
>>
>> Is it really the CPU? Or some SEAM software construct?
>
> It is the CPU. The CPU provides the database and gives instructions to
> P-SEAMLDR for adding records or cleaning up the entire database.
Either way, it's an implementation detail that doesn't need to be
litigated in the OS ABI docs.
TDX maintains a log about each TDX module which has been loaded.
This log has a finite size which limits the number of TDX module
updates which can be performed.
Report the number of updates remaining.
>>> +#ifdef CONFIG_INTEL_TDX_MODULE_UPDATE
...
> /facepalm. Sorry for missing these important considerations.
>
> I overlooked a critical constraint: only one CPU can call P-SEAMLDR at a time;
> any second CPU gets VMFailInvalid. Patch 19 adds a lock for SEAMLDR.INSTALL
> serialization, but we actually need to serialize all P-SEAMLDR calls or handle
> VMFailInvalid with retries.
>
> I will make the following changes to see how they look:
>
> 1. Move the lock from patch 19 to seamldr_call() to serialize all P-SEAMLDR calls
Ack, yes, this is obviously required.
> 2. Cache seamldr_info and only update it after successful updates
> 3. Make seamldr_get_info() return cached data instead of calling P-SEAMLDR every time
To be honest, I'm not sure we need a cache. Why don't we just make the
permissions 400 and keep the info structure on the stack?
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Dave Hansen @ 2026-01-30 16:06 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: <aXy4WgLd5ncrmje5@intel.com>
On 1/30/26 05:55, Chao Gao wrote:
...
>>> invoking SEAMLDR.INFO SEAMCALL in preparation for exposing P-SEAMLDR
>>> version and other necessary information to userspace.
>>
>> I also want to know what spec you are getting this out of.
>
> Will add a link in the changelog.
Remember, as a general rule, links go stale. Document titles and Intel
document numbers stay valid for *much* longer.
>>> + 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];
>>
>> Why not label this:
>>
>> u32 acm_x2apicid: /* unused by kernel */
>>
>> ?
>
> Will do. Probably because I thought the kernel would never use it.
It just makes me think that I'm looking at different documentation for
this data structure than you are. It literally costs nothing to give it
a real name. Maybe 5 bytes of code or something.
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Dave Hansen @ 2026-01-30 16:18 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: <aXywVcqbXodADg4a@intel.com>
On 1/30/26 05:21, Chao Gao wrote:
...
>>> + /*
>>> + * SEAMRET from P-SEAMLDR invalidates the current VMCS. Save/restore
>>> + * the VMCS across P-SEAMLDR SEAMCALLs to avoid clobbering KVM state.
>>> + * Disable interrupts as KVM is allowed to do VMREAD/VMWRITE in IRQ
>>> + * context (but not NMI context).
>>> + */
>>
>> I think you mean:
>>
>> WARN_ON(in_nmi());
>
> This function only disables interrupts, not NMIs. Kirill questioned whether any
> KVM operations might execute from NMI context and do VMREAD/VMWRITE. If such
> operations exist and an NMI interrupts seamldr_call(), they could encounter
> an invalid current VMCS.
>
> The problematic scenario is:
>
> seamldr_call() KVM code in NMI handler
>
> 1. vmptrst // save current-vmcs
> 2. seamcall // clobber current-vmcs
> 3. // NMI handler start
> call into some KVM code and do vmread/vmwrite
> // consume __invalid__ current-vmcs
> // NMI handler end
> 4. vmptrld // restore current-vmcs
>
> The comment clarifies that KVM doesn't do VMREAD/VMWRITE during NMI handling.
How about something like:
P-SEAMLDR calls invalidate the current VMCS. It must be saved
and restored around the call. Exclude KVM access to the VMCS
by disabling interrupts. This is not safe against VMCS use in
NMIs, but there are none of those today.
Ideally, you'd also pair that with _some_ checks in the KVM code that
use lockdep or warnings to reiterate that NMI access to the VMCS is not OK.
>>> + local_irq_save(flags);
>>> +
>>> + asm goto("1: vmptrst %0\n\t"
>>> + _ASM_EXTABLE(1b, %l[error])
>>> + : "=m" (vmcs) : : "cc" : error);
>>
>> I'd much rather this be wrapped up in a helper function. We shouldn't
>> have to look at the horrors of inline assembly like this.
>>
>> But this *REALLY* wants the KVM folks to look at it. One argument is
>> that with the inline assembly this is nice and self-contained. The other
>> argument is that this completely ignores all existing KVM infrastructure
>> and is parallel VMCS management.
>
> Exactly. Sean suggested this approach [*]. He prefers inline assembly rather than
> adding new, inferior wrappers
>
> *: https://lore.kernel.org/linux-coco/aHEYtGgA3aIQ7A3y@google.com/
Get his explicit reviews on the patch, please.
Also, I 100% object to inline assembly in the main flow. Please at least
make a wrapper for these and stick them in:
arch/x86/include/asm/special_insns.h
so the inline assembly spew is hidden from view.
>> I'd be shocked if this is the one and only place in the whole kernel
>> that can unceremoniously zap VMX state.
>>
>> I'd *bet* that you don't really need to do the vmptrld and that KVM can
>> figure it out because it can vmptrld on demand anyway. Something along
>> the lines of:
>>
>> local_irq_disable();
>> list_for_each(handwaving...)
>> vmcs_clear();
>> ret = seamldr_prerr(fn, args);
>> local_irq_enable();
>>
>> Basically, zap this CPU's vmcs state and then make KVM reload it at some
>> later time.
>
> The idea is feasible. But just calling vmcs_clear() won't work. We need to
> reset all the tracking state associated with each VMCS. We should call
> vmclear_local_loaded_vmcss() instead, similar to what's done before VMXOFF.
>
>>
>> I'm sure Sean and Paolo will tell me if I'm crazy.
>
> To me, this approach needs more work since we need to either move
> vmclear_local_loaded_vmcss() to the kernel or allow KVM to register a callback.
>
> I don't think it's as straightforward as just doing the save/restore.
Could you please just do me a favor and spend 20 minutes to see what
this looks like in practice and if the KVM folks hate it?
>>> diff --git a/drivers/virt/coco/tdx-host/Kconfig b/drivers/virt/coco/tdx-host/Kconfig
>>> index e58bad148a35..6a9199e6c2c6 100644
>>> --- a/drivers/virt/coco/tdx-host/Kconfig
>>> +++ b/drivers/virt/coco/tdx-host/Kconfig
>>> @@ -8,3 +8,13 @@ config TDX_HOST_SERVICES
>>>
>>> Say y or m if enabling support for confidential virtual machine
>>> support (CONFIG_INTEL_TDX_HOST). The module is called tdx_host.ko
>>> +
>>> +config INTEL_TDX_MODULE_UPDATE
>>> + bool "Intel TDX module runtime update"
>>> + depends on TDX_HOST_SERVICES
>>> + help
>>> + This enables the kernel to support TDX module runtime update. This
>>> + allows the admin to update the TDX module to another compatible
>>> + version without the need to terminate running TDX guests.
>>
>> ... as opposed to the method that the kernel has to update the module
>> without terminating guests? ;)
>
> I will reduce this to:
>
> This enables the kernel to update the TDX Module to another compatible
> version.
I guess I'll be explicit: Remove this Kconfig prompt.
I think you should remove INTEL_TDX_MODULE_UPDATE entirely. But I'll
settle for:
config INTEL_TDX_MODULE_UPDATE
bool
default TDX_HOST_SERVICES
so that users don't have to see it. Don't bother users with it. Period.
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Dave Hansen @ 2026-01-30 16:23 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: <aXxm+ezJvwUQ4sfD@intel.com>
On 1/30/26 00:08, Chao Gao wrote:
> 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.
But why is this a P-SEAMLDR thing and not a TDX module thing?
It seems like a bug, or at least a P-SEAMLDR implementation issue the
needs to get fixed.
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Dave Hansen @ 2026-01-30 16:35 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: <aXwtILdwb/KMX9uH@yilunxu-OptiPlex-7050>
On 1/29/26 20:01, Xu Yilun wrote:
>> I'd also prefer a
>>
>> BUILD_BUG_ON(sizeof(struct seamldr_info) != 2048);
> ^
> BUILD_BUG_ON(sizeof(struct seamldr_info) != 256); is it?
Whatever the documentation says. I might have been looking at the
seamldr_seaminfo.
>> 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?
I'm pretty sure __packed is used all over the place.
I'd be shocked if access to a __packed structure generated different
code than a non-packed one for the same layout. But it wouldn't be the
first time I was shocked by a compiler.
I think you might be confusing the fact that access to unaligned data
can really stink on some architectures. The code generation for *that*
can be garbage. But not on x86 really and not for data that's already
naturally aligned.
Plus, *this* data structure is far, far from being performance sensitive
anyway. So it doubly or triply doesn't matter here.
If nothing else, __packed is a good indicator that WYSIWYG for structure
layout because it's an ABI. I honestly don't see a lot of downsides.
^ 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-30 17:32 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: <aXwJIkFJw_4mRl70@google.com>
On Thu, 2026-01-29 at 17:28 -0800, Sean Christopherson wrote:
>
> Hmm, that's probably doable, but definitely in a separate patch.
> E.g. something
> like:
I think it would be a good change. But after more consideration, I
think the original patch is good on its own. Better to turn a bug into
a deterministic thing, than an opportunity to consume stack. Seems to
be what you intended.
Another idea would be to have a variant that returns an error instead
of 0 so the callers can have there error logic triggered, but it's all
incremental value on top of this.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: 6.18 CVM guest kernel boot issues with non-UEFI bootloader
From: Tom Lendacky @ 2026-01-30 18:49 UTC (permalink / raw)
To: Kevin Hui, linux-coco, Ard Biesheuvel
Cc: youngjaelee, hvolkmer, chrisboltz, tzn, andrisaar, grobler
In-Reply-To: <8d47d224-0ce0-417f-8c12-3ab9d6da8a59@meta.com>
Adding Ard
On 1/28/26 21:57, Kevin Hui wrote:
> Hello,
>
> We’ve been using a non-UEFI bootloader to launch SEV-SNP CVMs and noticed that
> there’s an issue with booting the newest 6.18 guest kernel with
> the stage0 bootloader (https://github.com/project-oak/oak/tree/main/stage0_bin).
> The guest kernel boots successfully with 6.17 and below, but fails with 6.18. We
> were able to reproduce this with host kernels 6.13.2 and 6.18.3 on Milan/Genoa
> hosts.
>
> We traced the commit that started causing boot issues to
> 68a501d7fd82454525797971c6a0005ceeb93153 and noticed that for some reason the
> variable snp_vmpl was non-zero, even though our stack doesn’t run SVSM. This
> triggers the sev_es_terminate() call and subsequently crashes the CVM. We
> noticed that the commit removes a supposedly redundant rmpadjust() check, but
> from our observations it seems that the failed rmpadjust() short-circuited the
> check and avoided the underlying issue.
>
> I was chatting with Tom about this, and taking a deeper look at the issue, we
> suspect that the BSS is cleared after the sev_enable() call in
> arch/x86/boot/compressed/head_64.S, and that because of this snp_vmpl contains
> random junk and is not zeroed. When coming through UEFI, it seems that the BSS
> is cleared via drivers/firmware/efi/libstub/x86-stub.c, but in a non-UEFI
> bootloader there is no call to startup_64 and so this path is never invoked,
> leaving whatever random data was in bss to remain.
>
> Perhaps the proper fix for this is to put the variables that are set as part of
> sev_enable() into .data so that both non-UEFI and UEFI bootloaders will have the
> same treatment from the kernel, but I would love to hear everyone’s thoughts on
> this.
>
> Thanks,
> Kevin
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Vedashree Vidwans @ 2026-01-30 19:29 UTC (permalink / raw)
To: Andre Przywara, Salman Nabi, sudeep.holla, mark.rutland,
lpieralisi
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar
In-Reply-To: <8e4b3940-bea0-4c75-80ec-8c5a87cd24a5@arm.com>
Hi Andre,
On 1/29/26 09:26, Andre Przywara wrote:
> 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?
Thank you for teh clarification, this helps. I agree that UUID is the
canonical identifier and that 'name' sysfs file is mainly for user
convenience.
One concern I have is around error reporting. For example, if
call_lfa_activate() fails, the kernel prints:
ACTIVATE for image <name_string> failed: LFA_BUSY
If the 'name_string' is "unknown" or an empty string, the error message
doesn't indicate which firmware component failed activation, especially
on system with multiple LFA-capable FW images.
In such cases, having meaningful fallback identifier is helpful for
debugging. Using UUID in the error log would help identify which FW
image encountered the failure. This avoids ambiguity and makes error
triage much easier, especially when platform-specific firmware images
are not listed in fw_images_uuids[] but are still valid LFA components.
So eitehr of the following would solve the issue:
1. Use fw_uuid as the fallback image_name
2. Update error print in call_lfa_activate() and prime_fw_image() to use
UUID instead of image_name.
Best,
Veda
>
> 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 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Andre Przywara @ 2026-01-30 20:31 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: <67ffa9f9-64b4-4015-b374-c7ab545c5200@nvidia.com>
Hi,
On 1/30/26 20:29, Vedashree Vidwans wrote:
> Hi Andre,
>
> On 1/29/26 09:26, Andre Przywara wrote:
>> 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
>>
[ .... ]
>>>> +
>>>> + 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?
> Thank you for teh clarification, this helps. I agree that UUID is the
> canonical identifier and that 'name' sysfs file is mainly for user
> convenience.
> One concern I have is around error reporting. For example, if
> call_lfa_activate() fails, the kernel prints:
> ACTIVATE for image <name_string> failed: LFA_BUSY
>
> If the 'name_string' is "unknown" or an empty string, the error message
> doesn't indicate which firmware component failed activation, especially
> on system with multiple LFA-capable FW images.
Yes, that's a good point.
> In such cases, having meaningful fallback identifier is helpful for
> debugging. Using UUID in the error log would help identify which FW
> image encountered the failure. This avoids ambiguity and makes error
> triage much easier, especially when platform-specific firmware images
> are not listed in fw_images_uuids[] but are still valid LFA components.
> So eitehr of the following would solve the issue:
> 1. Use fw_uuid as the fallback image_name
> 2. Update error print in call_lfa_activate() and prime_fw_image() to use
> UUID instead of image_name.
I'd say number 2 is better. And if we leave the name char* as NULL or an
empty string, then this is also easy to detect in the code, and we can
print the UUID instead. If we do this more than once, then this could be
nicely hidden in a function.
Thanks,
Andre
> Best,
> Veda
>>
>> 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: 6.18 CVM guest kernel boot issues with non-UEFI bootloader
From: Ard Biesheuvel @ 2026-01-30 20:49 UTC (permalink / raw)
To: Tom Lendacky, Kevin Hui, linux-coco
Cc: youngjaelee, hvolkmer, chrisboltz, tzn, andrisaar, grobler
In-Reply-To: <6bb2c25f-e51b-4d90-a67e-100bff1af838@amd.com>
Hi Tom Kevin,
On Fri, 30 Jan 2026, at 19:49, Tom Lendacky wrote:
> Adding Ard
>
> On 1/28/26 21:57, Kevin Hui wrote:
>> Hello,
>>
>> We’ve been using a non-UEFI bootloader to launch SEV-SNP CVMs and noticed that
>> there’s an issue with booting the newest 6.18 guest kernel with
>> the stage0 bootloader (https://github.com/project-oak/oak/tree/main/stage0_bin).
>> The guest kernel boots successfully with 6.17 and below, but fails with 6.18. We
>> were able to reproduce this with host kernels 6.13.2 and 6.18.3 on Milan/Genoa
>> hosts.
>>
>> We traced the commit that started causing boot issues to
>> 68a501d7fd82454525797971c6a0005ceeb93153 and noticed that for some reason the
>> variable snp_vmpl was non-zero, even though our stack doesn’t run SVSM. This
>> triggers the sev_es_terminate() call and subsequently crashes the CVM. We
>> noticed that the commit removes a supposedly redundant rmpadjust() check, but
>> from our observations it seems that the failed rmpadjust() short-circuited the
>> check and avoided the underlying issue.
>>
>> I was chatting with Tom about this, and taking a deeper look at the issue, we
>> suspect that the BSS is cleared after the sev_enable() call in
>> arch/x86/boot/compressed/head_64.S, and that because of this snp_vmpl contains
>> random junk and is not zeroed. When coming through UEFI, it seems that the BSS
>> is cleared via drivers/firmware/efi/libstub/x86-stub.c, but in a non-UEFI
>> bootloader there is no call to startup_64 and so this path is never invoked,
>> leaving whatever random data was in bss to remain.
>>
If there is no call to startup_64(), where does the call to sev_enable() originate from?
>> Perhaps the proper fix for this is to put the variables that are set as part of
>> sev_enable() into .data so that both non-UEFI and UEFI bootloaders will have the
>> same treatment from the kernel, but I would love to hear everyone’s thoughts on
>> this.
>>
AIUI, the root cause of the issue is that C code is being called (sev_enable()) before BSS has been cleared, right?
If so, I don't think moving variables around is the right solution here: instead, BSS should be cleared before calling C code. And by the looks of it, this is not even SEV-specific, given that load_stage1_idt() is also a C function.
^ permalink raw reply
* Re: 6.18 CVM guest kernel boot issues with non-UEFI bootloader
From: Tom Lendacky @ 2026-01-30 22:05 UTC (permalink / raw)
To: Ard Biesheuvel, Kevin Hui, linux-coco
Cc: youngjaelee, hvolkmer, chrisboltz, tzn, andrisaar, grobler
In-Reply-To: <e634d3f1-a58b-4303-9066-0dabc96d825f@app.fastmail.com>
On 1/30/26 14:49, Ard Biesheuvel wrote:
> Hi Tom Kevin,
>
> On Fri, 30 Jan 2026, at 19:49, Tom Lendacky wrote:
>> Adding Ard
>>
>> On 1/28/26 21:57, Kevin Hui wrote:
>>> Hello,
>>>
>>> We’ve been using a non-UEFI bootloader to launch SEV-SNP CVMs and noticed that
>>> there’s an issue with booting the newest 6.18 guest kernel with
>>> the stage0 bootloader (https://github.com/project-oak/oak/tree/main/stage0_bin).
>>> The guest kernel boots successfully with 6.17 and below, but fails with 6.18. We
>>> were able to reproduce this with host kernels 6.13.2 and 6.18.3 on Milan/Genoa
>>> hosts.
>>>
>>> We traced the commit that started causing boot issues to
>>> 68a501d7fd82454525797971c6a0005ceeb93153 and noticed that for some reason the
>>> variable snp_vmpl was non-zero, even though our stack doesn’t run SVSM. This
>>> triggers the sev_es_terminate() call and subsequently crashes the CVM. We
>>> noticed that the commit removes a supposedly redundant rmpadjust() check, but
>>> from our observations it seems that the failed rmpadjust() short-circuited the
>>> check and avoided the underlying issue.
>>>
>>> I was chatting with Tom about this, and taking a deeper look at the issue, we
>>> suspect that the BSS is cleared after the sev_enable() call in
>>> arch/x86/boot/compressed/head_64.S, and that because of this snp_vmpl contains
>>> random junk and is not zeroed. When coming through UEFI, it seems that the BSS
>>> is cleared via drivers/firmware/efi/libstub/x86-stub.c, but in a non-UEFI
>>> bootloader there is no call to startup_64 and so this path is never invoked,
>>> leaving whatever random data was in bss to remain.
>>>
>
> If there is no call to startup_64(), where does the call to sev_enable() originate from?
There is a call to startup_64().
>
>>> Perhaps the proper fix for this is to put the variables that are set as part of
>>> sev_enable() into .data so that both non-UEFI and UEFI bootloaders will have the
>>> same treatment from the kernel, but I would love to hear everyone’s thoughts on
>>> this.
>>>
>
> AIUI, the root cause of the issue is that C code is being called (sev_enable()) before BSS has been cleared, right?
Right, but the issue also is that the BSS gets cleared after relocation,
too. So I don't think using BSS is correct, because it would have to be
cleared before the call to sev_enable(). But the call to clear the BSS
after relocation would have to stay because to, otherwise the guest crashes.
>
> If so, I don't think moving variables around is the right solution here: instead, BSS should be cleared before calling C code. And by the looks of it, this is not even SEV-specific, given that load_stage1_idt() is also a C function.
Because of how things are called and because of the relocation, I think
putting the variables in .data is probably the right thing to do. I've
noticed a few other files under arch/x86/boot/compressed that have
variables defined with __section(".data") probably for the same reason.
The load_stage1_idt() function only works with variables on a stack or in
.data (boot_idt), which is probably why that works ok.
Thanks,
Tom
>
^ permalink raw reply
* Re: 6.18 CVM guest kernel boot issues with non-UEFI bootloader
From: Ard Biesheuvel @ 2026-01-30 22:12 UTC (permalink / raw)
To: Tom Lendacky, Kevin Hui, linux-coco
Cc: youngjaelee, hvolkmer, chrisboltz, tzn, andrisaar, grobler
In-Reply-To: <3dd58daf-6b7c-4887-b510-9c4fb7de0469@amd.com>
On Fri, 30 Jan 2026, at 23:05, Tom Lendacky wrote:
> On 1/30/26 14:49, Ard Biesheuvel wrote:
>> Hi Tom Kevin,
>>
>> On Fri, 30 Jan 2026, at 19:49, Tom Lendacky wrote:
>>> Adding Ard
>>>
>>> On 1/28/26 21:57, Kevin Hui wrote:
...
>>>> Perhaps the proper fix for this is to put the variables that are set as part of
>>>> sev_enable() into .data so that both non-UEFI and UEFI bootloaders will have the
>>>> same treatment from the kernel, but I would love to hear everyone’s thoughts on
>>>> this.
>>>>
>>
>> AIUI, the root cause of the issue is that C code is being called (sev_enable()) before BSS has been cleared, right?
>
> Right, but the issue also is that the BSS gets cleared after relocation,
> too. So I don't think using BSS is correct, because it would have to be
> cleared before the call to sev_enable(). But the call to clear the BSS
> after relocation would have to stay because to, otherwise the guest crashes.
>
Yeah, and relocating the image should migrate the contents of BSS too. So I agree that this is tricky. I hate the decompressor ...
>>
>> If so, I don't think moving variables around is the right solution here: instead, BSS should be cleared before calling C code. And by the looks of it, this is not even SEV-specific, given that load_stage1_idt() is also a C function.
>
> Because of how things are called and because of the relocation, I think
> putting the variables in .data is probably the right thing to do. I've
> noticed a few other files under arch/x86/boot/compressed that have
> variables defined with __section(".data") probably for the same reason.
>
> The load_stage1_idt() function only works with variables on a stack or in
> .data (boot_idt), which is probably why that works ok.
>
Indeed. In fact, creating any global state before relocation is rather tricky because of how the image moves itself around. But I guess it is unavoidable here.
^ permalink raw reply
* Re: [RFC PATCH v5 05/45] KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
From: Edgecombe, Rick P @ 2026-01-30 23:55 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-6-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Drop the dedicated .link_external_spt() for linking non-leaf S-EPT
> pages, and instead funnel everything through .set_external_spte().
> Using separate hooks doesn't help prevent TDP MMU details from
> bleeding into TDX, and vice versa; to the contrary, dedicated
> callbacks will result in _more_ pollution when hugepage support is
> added, e.g. will require the TDP MMU to know details about the
> splitting rules for TDX that aren't all that relevant to
> the TDP MMU.
>
> Ideally, KVM would provide a single pair of hooks to set S-EPT
> entries, one hook for setting SPTEs under write-lock and another for
> settings SPTEs under read-lock (e.g. to ensure the entire operation
> is "atomic", to allow for failure, etc.). Sadly, TDX's requirement
> that all child S-EPT entries are removed before the parent makes that
> impractical: the TDP MMU deliberately prunes non-leaf SPTEs and
> _then_ processes its children, thus making it quite important for the
> TDP MMU to differentiate between zapping leaf and non-leaf S-EPT
> entries.
>
> However, that's the _only_ case that's truly special, and even that
> case could be shoehorned into a single hook; it's just wouldn't be a
> net positive.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
It has better handling of the external_spt == NULL case too, by
actually returning an error, but one naming nit below to take or leave.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 1 -
> arch/x86/include/asm/kvm_host.h | 3 --
> arch/x86/kvm/mmu/tdp_mmu.c | 37 +++---------------
> arch/x86/kvm/vmx/tdx.c | 61 ++++++++++++++++++++--------
> --
> 4 files changed, 48 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h
> b/arch/x86/include/asm/kvm-x86-ops.h
> index c18a033bee7e..57eb1f4832ae 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -94,7 +94,6 @@ 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_RET0(link_external_spt)
> KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> KVM_X86_OP_OPTIONAL_RET0(free_external_spt)
> KVM_X86_OP_OPTIONAL(remove_external_spte)
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index e441f270f354..d12ca0f8a348 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1853,9 +1853,6 @@ struct kvm_x86_ops {
> void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
> int root_level);
>
> - /* Update external mapping with page table link. */
> - int (*link_external_spt)(struct kvm *kvm, gfn_t gfn, enum
> pg_level level,
> - void *external_spt);
> /* Update the external page table from spte getting set. */
> int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum
> pg_level level,
> u64 mirror_spte);
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 0feda295859a..56ad056e6042 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -495,31 +495,17 @@ static void handle_removed_pt(struct kvm *kvm,
> tdp_ptep_t pt, bool shared)
> call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> }
>
> -static void *get_external_spt(gfn_t gfn, u64 new_spte, int level)
> -{
> - if (is_shadow_present_pte(new_spte) &&
> !is_last_spte(new_spte, level)) {
> - struct kvm_mmu_page *sp =
> spte_to_child_sp(new_spte);
> -
> - WARN_ON_ONCE(sp->role.level + 1 != level);
> - WARN_ON_ONCE(sp->gfn != gfn);
> - return sp->external_spt;
> - }
> -
> - return NULL;
> -}
> -
> static int __must_check set_external_spte_present(struct kvm *kvm,
> tdp_ptep_t sptep,
> gfn_t gfn, u64
> *old_spte,
> u64 new_spte, int
> level)
> {
> - bool was_present = is_shadow_present_pte(*old_spte);
> - bool is_present = is_shadow_present_pte(new_spte);
> - bool is_leaf = is_present && is_last_spte(new_spte, level);
> - int ret = 0;
> -
> - KVM_BUG_ON(was_present, kvm);
> + int ret;
>
> lockdep_assert_held(&kvm->mmu_lock);
> +
> + if (KVM_BUG_ON(is_shadow_present_pte(*old_spte), kvm))
> + return -EIO;
> +
> /*
> * We need to lock out other updates to the SPTE until the
> external
> * page table has been modified. Use FROZEN_SPTE similar to
> @@ -528,18 +514,7 @@ static int __must_check
> set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
> if (!try_cmpxchg64(rcu_dereference(sptep), old_spte,
> FROZEN_SPTE))
> return -EBUSY;
>
> - /*
> - * Use different call to either set up middle level
> - * external page table, or leaf.
> - */
> - if (is_leaf) {
> - ret = kvm_x86_call(set_external_spte)(kvm, gfn,
> level, new_spte);
> - } else {
> - void *external_spt = get_external_spt(gfn, new_spte,
> level);
> -
> - KVM_BUG_ON(!external_spt, kvm);
> - ret = kvm_x86_call(link_external_spt)(kvm, gfn,
> level, external_spt);
> - }
> + ret = kvm_x86_call(set_external_spte)(kvm, gfn, level,
> new_spte);
> if (ret)
> __kvm_tdp_mmu_write_spte(sptep, *old_spte);
> else
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 5688c77616e3..30494f9ceb31 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1664,18 +1664,58 @@ static int tdx_mem_page_aug(struct kvm *kvm,
> gfn_t gfn,
> return 0;
> }
>
> +static struct page *tdx_spte_to_external_spt(struct kvm *kvm, gfn_t
> gfn,
> + u64 new_spte, enum
> pg_level level)
> +{
> + struct kvm_mmu_page *sp = spte_to_child_sp(new_spte);
> +
> + if (KVM_BUG_ON(!sp->external_spt, kvm) ||
> + KVM_BUG_ON(sp->role.level + 1 != level, kvm) ||
> + KVM_BUG_ON(sp->gfn != gfn, kvm))
> + return NULL;
> +
> + return virt_to_page(sp->external_spt);
> +}
> +
> +static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn,
> + enum pg_level level, u64
> mirror_spte)
> +{
> + gpa_t gpa = gfn_to_gpa(gfn);
> + u64 err, entry, level_state;
> + struct page *external_spt;
> +
> + external_spt = tdx_spte_to_external_spt(kvm, gfn,
> mirror_spte, level);
The "external" abstraction wraps the "S-EPT" knowledge and naming (for
maybe increasingly dubious reasons), but in the TDX code, inside the
abstraction, it uses the sept naming. so I might have called it
sept_pt.
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Trilok Soni @ 2026-01-31 1:35 UTC (permalink / raw)
To: Salman Nabi, vvidwans, andre.przywara, sudeep.holla, mark.rutland,
lpieralisi
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar
In-Reply-To: <20260119122729.287522-2-salman.nabi@arm.com>
On 1/19/2026 4:27 AM, 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
> IMPDEF Out-Of-Bound method. The user can then activate this new firmware,
> at system runtime, without requiring a reboot.
> The specification covers the SMCCC interface to list and query available
> components and eventually trigger the activation.
>
> Add a new directory under /sys/firmware to present firmware components
> capable of live activation. Each of them is a directory under lfa/,
> and is identified via its GUID. The activation will be triggered by echoing
> "1" into the "activate" file:
> ==========================================
> /sys/firmware/lfa # ls -l . 6c*
> .:
> total 0
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 47d4086d-4cfe-9846-9b95-2950cbbd5a00
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 6c0762a6-12f2-4b56-92cb-ba8f633606d9
> drwxr-xr-x 2 0 0 0 Jan 19 11:33 d6d0eea7-fcea-d54b-9782-9934f234b6e4
Can you please explain or add a note on why we don't have name of the firmware
as the directory name and why you have selected GUID as top-level
directory name?
---Trilok Soni
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Xu Yilun @ 2026-02-02 0:16 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: <1b1a2fe7-d225-414c-9055-8ad06938a0bf@intel.com>
> If nothing else, __packed is a good indicator that WYSIWYG for structure
> layout because it's an ABI. I honestly don't see a lot of downsides.
OK. So on x86 I can use it without worry. Thanks.
^ permalink raw reply
* Re: [PATCH v3 11/26] x86/virt/seamldr: Block TDX Module updates if any CPU is offline
From: Xu Yilun @ 2026-02-02 0:31 UTC (permalink / raw)
To: Chao Gao
Cc: 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-12-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:19AM -0800, Chao Gao wrote:
> P-SEAMLDR requires every CPU to call the SEAMLDR.INSTALL SEAMCALL during
> updates. So, every CPU should be online.
>
> Check if all CPUs are online and abort the update if any CPU is offline at
> the very beginning. Without this check, P-SEAMLDR will report failure at a
> later phase where the old TDX module is gone and TDs have to be killed.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request
From: Xu Yilun @ 2026-02-02 3:08 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-14-chao.gao@intel.com>
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> + int module_size, sig_size;
> + const void *sig, *module;
You need to firstly check if size is big enough for the header before
offset into it.
if (size < sizeof(struct tdx_blob))
return XXX;
^ permalink raw reply
* Re: [PATCH v3 14/26] x86/virt/seamldr: Introduce skeleton for TDX Module updates
From: Xu Yilun @ 2026-02-02 6:01 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-15-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:22AM -0800, Chao Gao wrote:
> The P-SEAMLDR requires that no TDX Module SEAMCALLs are invoked during a
> runtime TDX Module update.
>
> But currently, TDX Module SEAMCALLs are invoked in various contexts and in
> parallel across CPUs. Additionally, considering the need to force all vCPUs
> out of guest mode, no single lock primitive, except for stop_machine(), can
> meet this requirement.
>
> Perform TDX Module updates within stop_machine() as it achieves the
> P-SEAMLDR requirements and is an existing well understood mechanism.
>
> TDX Module updates consist of several steps: shutting down the old
> module, installing the new module, and initializing the new one and etc.
> Some steps must be executed on a single CPU, others serially across all
> CPUs, and some can be performed concurrently on all CPUs and there are
> ordering requirements between steps. So, all CPUs need to perform the work
> in a step-locked manner.
>
> In preparation for adding concrete steps for TDX Module updates,
> establish the framework by mimicking multi_cpu_stop(). Specifically, use a
> global state machine to control the work done on each CPU and require all
> CPUs to acknowledge completion before proceeding to the next stage.
>
> Potential alternative to stop_machine()
> =======================================
> An alternative approach is to lock all KVM entry points and kick all
> vCPUs. Here, KVM entry points refer to KVM VM/vCPU ioctl entry points,
> implemented in KVM common code (virt/kvm). Adding a locking mechanism
> there would affect all architectures. And to lock only TDX vCPUs, new
> logic would be needed to identify TDX vCPUs, which the common code
> currently lacks. This would add significant complexity and maintenance
> overhead for a TDX-specific use case.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 15/26] x86/virt/seamldr: Abort updates if errors occurred midway
From: Xu Yilun @ 2026-02-02 6:08 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-16-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:23AM -0800, Chao Gao wrote:
> The TDX Module update process has multiple stages, each of which may
> encounter failures.
>
> The current state machine of updates proceeds to the next stage
> regardless of errors. But continuing updates when errors occur midway
> is pointless.
>
> If a CPU encounters an error, abort the update by setting a flag and
> exiting the execution loop. Note that this CPU doesn't acknowledge the
> current stage. This will keep all other CPUs in the current stage until
> they see the flag and exit the loop as well.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 16/26] x86/virt/seamldr: Shut down the current TDX module
From: Xu Yilun @ 2026-02-02 6:31 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-17-chao.gao@intel.com>
> +static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff)
> +{
> + int ret = 0;
> + u64 val;
> +
> + if (tdx_supports_runtime_update(&tdx_sysinfo) &&
> + !(ret = read_sys_metadata_field(0x8900000100000000, &val)))
> + sysinfo_handoff->module_hv = val;
> +
> + return ret;
> +}
> +
> static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> {
> int ret = 0;
> @@ -115,6 +127,7 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> ret = ret ?: get_tdx_sys_info_tdmr(&sysinfo->tdmr);
> ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl);
> ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf);
> + ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff);
Do all fields in sysinfo_handoff optional or just module_hv, if the
former, is it better:
if (tdx_supports_runtime_update(&tdx_sysinfo)
ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff);
>
> return ret;
> }
> --
> 2.47.3
>
^ permalink raw reply
* Re: [PATCH v3 18/26] x86/virt/seamldr: Log TDX Module update failures
From: Xu Yilun @ 2026-02-02 7:11 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-19-chao.gao@intel.com>
> +static void print_update_failure_message(void)
> +{
> + static atomic_t printed = ATOMIC_INIT(0);
> +
> + if (atomic_inc_return(&printed) == 1)
> + pr_err("update failed, SEAMCALLs will report failure until TDs killed\n");
> +}
Not sure why it can't be just pr_err_once()?
> +
> /*
> * See multi_cpu_stop() from where this multi-cpu state-machine was
> * adopted, and the rationale for touch_nmi_watchdog()
> @@ -289,10 +297,13 @@ static int do_seamldr_install_module(void *params)
> break;
> }
>
> - if (ret)
> + if (ret) {
> atomic_inc(&tdp_data.failed);
> - else
> + if (curstate > TDP_SHUTDOWN)
> + print_update_failure_message();
> + } else {
> ack_state();
> + }
> } else {
> touch_nmi_watchdog();
> rcu_momentary_eqs();
> --
> 2.47.3
>
^ permalink raw reply
* Re: [PATCH v3 20/26] x86/virt/seamldr: Do TDX per-CPU initialization after updates
From: Xu Yilun @ 2026-02-02 7:32 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-21-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:28AM -0800, Chao Gao wrote:
> After installing the new TDX module, each CPU should be initialized
> again to make the CPU ready to run any other SEAMCALLs. So, call
> tdx_cpu_enable() on all CPUs.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 22/26] x86/virt/tdx: Update tdx_sysinfo and check features post-update
From: Xu Yilun @ 2026-02-02 7:33 UTC (permalink / raw)
To: Chao Gao
Cc: 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: <20260123145645.90444-23-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:30AM -0800, Chao Gao wrote:
> tdx_sysinfo contains all metadata of the active TDX module, including
> versions, supported features, and TDMR/TDCS/TDVPS information. These
> elements may change over updates. Blindly refreshing the entire tdx_sysinfo
> could disrupt running software, as it may subtly rely on the previous state
> unless proven otherwise.
>
> Adopt a conservative approach, like microcode updates, by only refreshing
> version information that does not affect functionality, while ignoring
> all other changes. This is acceptable as new modules are required to
> maintain backward compatibility.
>
> Any updates to metadata beyond versions should be justified and reviewed on
> a case-by-case basis.
>
> Note that preallocating a tdx_sys_info buffer before updates is to avoid
> having to handle -ENOMEM when updating tdx_sysinfo after a successful
> update.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 23/26] x86/virt/tdx: Enable TDX Module runtime updates
From: Xu Yilun @ 2026-02-02 7:41 UTC (permalink / raw)
To: Chao Gao
Cc: 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-24-chao.gao@intel.com>
> static inline bool tdx_supports_runtime_update(const struct tdx_sys_info *sysinfo)
> {
> - return false; /* To be enabled when kernel is ready */
> + return sysinfo->features.tdx_features0 & TDX_FEATURES0_TD_PRESERVING;
Frankly speak, I don't know why we need endless one-line wrappers like
this, we've already exposed all details of the tdx_sys_info to its
users.
But I'm still OK with them.
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* [PATCH RESEND] KVM: SEV: Track SNP launch state and disallow invalid userspace interactions
From: Jethro Beekman @ 2026-02-02 10:11 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, kvm,
linux-kernel, linux-coco
Calling any of the SNP_LAUNCH_ ioctls after SNP_LAUNCH_FINISH results in a
kernel page fault due to RMP violation. Track SNP launch state and exit early.
vCPUs created after SNP_LAUNCH_FINISH won't have a guest VMSA automatically
created during SNP_LAUNCH_FINISH by converting the kernel-allocated VMSA. Don't
allocate a VMSA page, so that the vCPU is in a state similar to what it would
be after SNP AP destroy. This ensures pre_sev_run() prevents the vCPU from
running even if userspace makes the vCPU runnable.
Signed-off-by: Jethro Beekman <jethro@fortanix.com>
---
arch/x86/kvm/svm/sev.c | 43 ++++++++++++++++++++++++++----------------
arch/x86/kvm/svm/svm.h | 1 +
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f59c65abe3cf..cdaca10b8773 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2205,6 +2205,9 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (!sev_snp_guest(kvm))
return -ENOTTY;
+ if (sev->snp_finished)
+ return -EINVAL;
+
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
return -EFAULT;
@@ -2369,7 +2372,7 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
void __user *src;
int ret = 0;
- if (!sev_snp_guest(kvm) || !sev->snp_context)
+ if (!sev_snp_guest(kvm) || !sev->snp_context || sev->snp_finished)
return -EINVAL;
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
@@ -2502,7 +2505,7 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (!sev_snp_guest(kvm))
return -ENOTTY;
- if (!sev->snp_context)
+ if (!sev->snp_context || sev->snp_finished)
return -EINVAL;
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
@@ -2548,13 +2551,15 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
data->gctx_paddr = __psp_pa(sev->snp_context);
ret = sev_issue_cmd(kvm, SEV_CMD_SNP_LAUNCH_FINISH, data, &argp->error);
- /*
- * Now that there will be no more SNP_LAUNCH_UPDATE ioctls, private pages
- * can be given to the guest simply by marking the RMP entry as private.
- * This can happen on first access and also with KVM_PRE_FAULT_MEMORY.
- */
- if (!ret)
+ if (!ret) {
+ sev->snp_finished = true;
+ /*
+ * Now that there will be no more SNP_LAUNCH_UPDATE ioctls, private pages
+ * can be given to the guest simply by marking the RMP entry as private.
+ * This can happen on first access and also with KVM_PRE_FAULT_MEMORY.
+ */
kvm->arch.pre_fault_allowed = true;
+ }
kfree(id_auth);
@@ -3253,6 +3258,9 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
svm = to_svm(vcpu);
+ if (!svm->sev_es.vmsa)
+ goto skip_vmsa_free;
+
/*
* If it's an SNP guest, then the VMSA was marked in the RMP table as
* a guest-owned page. Transition the page to hypervisor state before
@@ -4653,6 +4661,7 @@ void sev_init_vmcb(struct vcpu_svm *svm, bool init_event)
int sev_vcpu_create(struct kvm_vcpu *vcpu)
{
+ struct kvm_sev_info *sev = to_kvm_sev_info(vcpu->kvm);
struct vcpu_svm *svm = to_svm(vcpu);
struct page *vmsa_page;
@@ -4661,15 +4670,17 @@ int sev_vcpu_create(struct kvm_vcpu *vcpu)
if (!sev_es_guest(vcpu->kvm))
return 0;
- /*
- * SEV-ES guests require a separate (from the VMCB) VMSA page used to
- * contain the encrypted register state of the guest.
- */
- vmsa_page = snp_safe_alloc_page();
- if (!vmsa_page)
- return -ENOMEM;
+ if (!sev->snp_finished) {
+ /*
+ * SEV-ES guests require a separate (from the VMCB) VMSA page used to
+ * contain the encrypted register state of the guest.
+ */
+ vmsa_page = snp_safe_alloc_page();
+ if (!vmsa_page)
+ return -ENOMEM;
- svm->sev_es.vmsa = page_address(vmsa_page);
+ svm->sev_es.vmsa = page_address(vmsa_page);
+ }
vcpu->arch.guest_tsc_protected = snp_is_secure_tsc_enabled(vcpu->kvm);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 01be93a53d07..59c328c13b2a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -96,6 +96,7 @@ struct kvm_sev_info {
bool active; /* SEV enabled guest */
bool es_active; /* SEV-ES enabled guest */
bool need_init; /* waiting for SEV_INIT2 */
+ bool snp_finished; /* SNP guest measurement has been finalized */
unsigned int asid; /* ASID used for this guest */
unsigned int handle; /* SEV firmware handle */
int fd; /* SEV device fd */
--
2.43.0
^ permalink raw reply related
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