* 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 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: [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: 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: 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 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: [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: [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: 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: [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: [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: [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 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 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 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 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-01-30 15:45 UTC (permalink / raw)
To: Tom Lendacky
Cc: Kim Phillips, linux-kernel, kvm, linux-coco, x86,
Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <2295adbc-835f-4a84-934b-b7aba65137a8@amd.com>
On Fri, Jan 30, 2026 at 08:56:07AM -0600, Tom Lendacky wrote:
> It can be added. Any of the features added to SNP_FEATURES_PRESENT that
> aren't set in the SNP_FEATURES_IMPL_REQ bitmap are really a no-op. The
> SNP_FEATURES_PRESENT bitmap is meant to contain whatever bits are set in
> SNP_FEATURES_IMPL_REQ when an implementation has been implemented for the
> guest.
>
> But, yeah, we could add all the bits that aren't set in
> SNP_FEATURES_IMPL_REQ to SNP_FEATURES_PRESENT if it makes it clearer.
Right, that's the question. SNP_FEATURES_PRESENT is used in the masking
operation to get the unsupported features.
But when we say a SNP feature is present, then, even if it doesn't need guest
implementation, that feature is still present nonetheless.
So our nomenclature is kinda imprecise here.
I'd say, we can always rename SNP_FEATURES_PRESENT to denote what it is there
for, i.e., the narrower functionality of the masking.
Or, if we want to gather there *all* features that are present, then we can
start adding them...
> If we do that, it should probably be a separate patch (?) that also
> rewords the comment above SNP_FEATURES_PRESENT
... yes, as a separate patch.
Question is, what do we really wanna do here?
Does it make sense and is it useful to have SNP_FEATURES_PRESENT contain *all*
guest SNP features...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-01-30 14:56 UTC (permalink / raw)
To: Huang, Kai
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, x86@kernel.org,
dave.hansen@linux.intel.com, kas@kernel.org, seanjc@google.com,
Chatre, Reinette, Weiny, Ira, tglx@linutronix.de, Verma, Vishal L,
nik.borisov@suse.com, mingo@redhat.com, hpa@zytor.com,
sagis@google.com, Chen, Farrah, Duan, Zhenzhong,
Edgecombe, Rick P, paulmck@kernel.org, Annapurve, Vishal,
yilun.xu@linux.intel.com, Williams, Dan J, bp@alien8.de
In-Reply-To: <e229343797319f6d316432055bb52aaa637d5d6f.camel@intel.com>
On Wed, Jan 28, 2026 at 12:03:25PM +0800, Huang, Kai wrote:
>
>> +/*
>> + * Allocate and populate a seamldr_params.
>> + * Note that both @module and @sig should be vmalloc'd memory.
>
>Nit:
>
>How about actually using is_vmalloc_addr() to check in the code rather than
>documenting in the comment?
>
>I see you have already checked the overall 'data' buffer is vmalloc()'ed in
>seamldr_install_module() so the 'module' and 'sig' (part of 'data') must be
>too. But since is_vmalloc_addr() is cheap so I think it's also fine to do
>the check here. We can also WARN() so it can be used to catch bug.
Kai,
Thanks a lot.
Looks good to me. I think WARN() is always better than comments.
>> + if (!verify_checksum(blob)) {
>> + pr_err("invalid checksum\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + return alloc_seamldr_params(module, module_size, sig, sig_size);
>> +}
>
>It's weird that we have do verify checksum manually, because hardware
>normally catches that.
>
>I suppose this is because we want to catch as many errors as possible before
>actually asking P-SEAMLDR to do module update, since in order to do which we
>have to shutdown the existing module first and there's no returning point
>once we reach that?
Yes. Exactly.
>
>If so a comment would be helpful.
Will do.
>
>Also, it's also weird that you have to write code for checksum on your own.
>I guess the kernel should already have some library code for that.
>
>I checked and it _seems_ the code in lib/checksum.c could be used?
>
>I am not expert though, but I think we should use kernel lib code when we
>can.
Good point. After a quick review, lib/checksum.c uses a different algorithm
than tdx_blob's checksum. It adds the carry bit to the checksum, while tdx_blob
drops the carry bit.
*sigh* when I designed the checksum algorithm, I wasn't aware of lib/checksum.c.
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Tom Lendacky @ 2026-01-30 14:56 UTC (permalink / raw)
To: Borislav Petkov, Kim Phillips
Cc: linux-kernel, kvm, linux-coco, x86, Sean Christopherson,
Paolo Bonzini, K Prateek Nayak, Nikunj A Dadhania, Michael Roth,
Naveen Rao, David Kaplan, stable
In-Reply-To: <20260130123252.GAaXyk9DJEAiQeDyeh@fat_crate.local>
On 1/30/26 06:32, Borislav Petkov wrote:
> On Thu, Jan 29, 2026 at 04:32:49PM -0600, Kim Phillips wrote:
>> Not *all* SNP features are implemented in all guest kernel versions, and,
>> well, for those that don't require explicit guest code support, perhaps it's
>> because they aren't necessarily well defined and validated in all hardware
>> versions...
>
> Ok, can you add *this* feature to SNP_FEATURES_PRESENT? If not, why not?
It can be added. Any of the features added to SNP_FEATURES_PRESENT that
aren't set in the SNP_FEATURES_IMPL_REQ bitmap are really a no-op. The
SNP_FEATURES_PRESENT bitmap is meant to contain whatever bits are set in
SNP_FEATURES_IMPL_REQ when an implementation has been implemented for the
guest.
But, yeah, we could add all the bits that aren't set in
SNP_FEATURES_IMPL_REQ to SNP_FEATURES_PRESENT if it makes it clearer.
If we do that, it should probably be a separate patch (?) that also
rewords the comment above SNP_FEATURES_PRESENT
Thanks,
Tom
>
^ permalink raw reply
* Re: [PATCH v3 09/26] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-01-30 14:44 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
In-Reply-To: <9fb1bbf3-0623-447e-86d7-d48ef20fb42c@intel.com>
>> +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.
<snip>
>> +#ifdef CONFIG_INTEL_TDX_MODULE_UPDATE
>> +static ssize_t seamldr_version_show(struct device *dev, struct device_attribute *attr,
>> + char *buf)
>> +{
>> + const struct seamldr_info *info = seamldr_get_info();
>
>Uhh... seamldr_get_info() calls down into the SEAMLDR. It happily zaps
>the VMCS and this is surely a slow thing. This also has 0444 permissions
>which means *ANYONE* can call this. Constantly. As fast as they can make
>a few syscalls.
>
>Right?
You are absolutely right.
>
>Are there any concerns about making SEAMLDR calls? Are there any
>system-wide performance implications? How long of an interrupt-blocking
>blip is there for this?
>
>Also, what's the locking around seamldr_get_info()? It writes into a
>global, shared structure. I guess you disabled interrupts so it's
>preempt safe at least. <sigh>
>
>I guess it won't change *that* much. But, sheesh, it seems like an
>awfully bad idea to have lots of CPUs writing into a common data
>structure all at the same time.
/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
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
^ permalink raw reply
* Re: [PATCH v3 10/26] coco/tdx-host: Implement FW_UPLOAD sysfs ABI for TDX Module updates
From: Xu Yilun @ 2026-01-30 14:07 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-11-chao.gao@intel.com>
> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
> + u32 offset, u32 size, u32 *written)
> +{
> + struct tdx_fw_upload_status *status = fwl->dd_handle;
> + int ret;
> +
> + if (status->cancel_request) {
> + status->cancel_request = false;
> + return FW_UPLOAD_ERR_CANCELED;
We don't allow partial write, we stop_machine while writing, so we
cannot possibly cancel the update in progress, so we only check the
cancel_request once before first write. That means cancel is useless for
our case. Is it better we delete all the cancel logic &
struct tdx_fw_upload_status?
> + }
> +
> + /*
> + * tdx_fw_write() always processes all data on the first call with
> + * offset == 0. Since it never returns partial success (it either
> + * succeeds completely or fails), there is no subsequent call with
> + * non-zero offsets.
> + */
> + WARN_ON_ONCE(offset);
> + ret = seamldr_install_module(data, size);
...
> +static void tdx_fw_cancel(struct fw_upload *fwl)
> +{
Unfortunately fw_upload core doesn't allow .cancel unimplemented, leave
it as a dummy stub is OK, since this callback just request cancel,
doesn't care whether the cancel succeeds or fails in the end.
If you agree, add some comments in this function.
> +}
> +
> +static const struct fw_upload_ops tdx_fw_ops = {
> + .prepare = tdx_fw_prepare,
> + .write = tdx_fw_write,
> + .poll_complete = tdx_fw_poll_complete,
> + .cancel = tdx_fw_cancel,
> +};
> +
> +static void seamldr_init(struct device *dev)
> +{
> + const struct tdx_sys_info *tdx_sysinfo = tdx_get_sysinfo();
> + int ret;
> +
> + if (WARN_ON_ONCE(!tdx_sysinfo))
> + return;
We already does tdx_get_sysinfo() on module_init, is it better we have
a global tdx_sysinfo pointer in this driver, so that we don't have to
retrieve it again and again.
^ permalink raw reply
* Re: [PATCH v3 24/26] x86/virt/seamldr: Extend sigstruct to 16KB
From: Chao Gao @ 2026-01-30 14:25 UTC (permalink / raw)
To: Huang, Kai
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, x86@kernel.org,
dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
seanjc@google.com, Weiny, Ira, Chatre, Reinette,
tglx@linutronix.de, nik.borisov@suse.com, Verma, Vishal L,
hpa@zytor.com, sagis@google.com, Annapurve, Vishal,
Duan, Zhenzhong, Edgecombe, Rick P, paulmck@kernel.org,
bp@alien8.de, yilun.xu@linux.intel.com, Williams, Dan J
In-Reply-To: <c9c648536ed4cd242ce5d7de87cafe352503839f.camel@intel.com>
>Let's move the discussion here (from patch 13 -- sorry about that):
>
>IIRC this patch just simply re-purposes couple of reserved space in
>SEAMLDR_PARAMS (which is part of P-SEAMLDR ABI) w/o enumeration, explicit
>opt-in whatever. The code change here doesn't even bump up its version.
>
>IIUC, if this code run on an old platform where SEAMLDR.INSTALL still only
>works with 4K SIGSTRUCT, the SEAMLDR.INSTALL will only see part of the
>SIGSTRUCT thus will likely fail.
>
>How can we know whether a given 'struct tdx_blob' can work on an platform or
>not? Or am I missing anything?
Good question.
This is actually userspace's responsibility. The kernel exposes P-SEAMLDR
version to userspace, and for each module, the mapping file [*] lists the
module's minimum P-SEAMLDR version requirements. This allows userspace to
determine whether the existing P-SEAMLDR can load a specific TDX blob.
If the kernel cannot load a module using the current P-SEAMLDR, that's
userspace's fault.
*: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/mapping_file.json
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Chao Gao @ 2026-01-30 13: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: <b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@intel.com>
On Wed, Jan 28, 2026 at 03:54:38PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> P-SEAMLDR returns its information e.g., version and supported features, in
>> response to the SEAMLDR.INFO SEAMCALL.
>>
>> This information is useful for userspace. For example, the admin can decide
>> which TDX module versions are compatible with the P-SEAMLDR according to
>> the P-SEAMLDR version.
>>
>> Add and export seamldr_get_info() which retrieves P-SEAMLDR information by
>
>I don't need to know what the function name is. That's in the code.
>
Hi Dave,
Thank you for the thorough review.
I will go through the following patches to ensure they don't have the same
issues you have pointed out.
>> 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.
>
>I think it's also worth calling out that there are SEAMLDR calls for both:
>
> SEAMLDR_INFO
>and
> SEAMLDR_SEAMINFO
>
>Which is astonishingly confusing. Please have mercy on folks that are
>looking through the docs for the first time and explain this.
Sorry about this. Will do.
>
>> diff --git a/arch/x86/include/asm/seamldr.h b/arch/x86/include/asm/seamldr.h
>> new file mode 100644
>> index 000000000000..d1e9f6e16e8d
>> --- /dev/null
>> +++ b/arch/x86/include/asm/seamldr.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef _ASM_X86_SEAMLDR_H
>> +#define _ASM_X86_SEAMLDR_H
>> +
>> +#include <linux/types.h>
>> +
>> +struct seamldr_info {
>
>/*
> * This called the "SEAMLDR_INFO" data structure and is defined
> * in "SEAM Loader (SEAMLDR) Interface Specification".
> */
Will do.
>
>
>> + 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.
<snip>
>> +const struct seamldr_info *seamldr_get_info(void)
>> +{
>> + struct tdx_module_args args = { .rcx = __pa(&seamldr_info) };
>> +
>> + return seamldr_call(P_SEAMLDR_INFO, &args) ? NULL : &seamldr_info;
>> +}
>> +EXPORT_SYMBOL_FOR_MODULES(seamldr_get_info, "tdx-host");
>
>I'd also prefer a
>
> BUILD_BUG_ON(sizeof(struct seamldr_info) != 2048);
>
>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.
ok.
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Chao Gao @ 2026-01-30 13:30 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: <7e981cd1-cfd5-4bc3-a87d-0a187c510dc2@intel.com>
On Wed, Jan 28, 2026 at 03:57:30PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> +static struct seamldr_info seamldr_info __aligned(256);
>
>I also wonder if this should be __read_mostly or even read-only after
>boot. Is it ever modified?
This should be __read_mostly. num_remaining_updates changes after successful
updates.
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-01-30 13:21 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: <301f8156-bafe-440a-8628-3bf8fae74464@intel.com>
On Wed, Jan 28, 2026 at 03:36:49PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>...
>> +static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
>> +{
>> + unsigned long flags;
>> + u64 vmcs;
>> + int ret;
>> +
>> + if (!is_seamldr_call(fn))
>> + return -EINVAL;
>
>Why is this here? We shouldn't be silently papering over kernel bugs.
>This is a WARN_ON() at *best*, but it also begs the question of how a
>non-SEAMLDR call even got here.
Only SEAMLDR calls can get here. I will make it a WARN_ON_ONCE().
>
>> + /*
>> + * 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.
>
>> + 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/
>
>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.
>
>> 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.
>
>> + If unsure, say N.
>
>Let's call this:
>
> config
>INTEL_TDX_ONLY_DISABLE_THIS_IF_YOU_HATE_SECURITY_AND_IF_YOU_DO_WHY_ARE_YOU_RUNNING_TDX?
>
>Can we have question marks in config symbol names? ;)
>
>But, seriously, what the heck? Who would disable security updates for
>their confidential computing infrastructure? Is this some kind of
>intelligence test for our users so that if someone disables it we can
>just laugh at them?
Looks like I failed that test! ;) I'll change it to default to 'y' and
recommend 'Y' if unsure.
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-01-30 12:32 UTC (permalink / raw)
To: Kim Phillips
Cc: linux-kernel, kvm, linux-coco, x86, Sean Christopherson,
Paolo Bonzini, K Prateek Nayak, Nikunj A Dadhania, Tom Lendacky,
Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <f42e878a-d56f-413d-87e1-19acdc6de690@amd.com>
On Thu, Jan 29, 2026 at 04:32:49PM -0600, Kim Phillips wrote:
> Not *all* SNP features are implemented in all guest kernel versions, and,
> well, for those that don't require explicit guest code support, perhaps it's
> because they aren't necessarily well defined and validated in all hardware
> versions...
Ok, can you add *this* feature to SNP_FEATURES_PRESENT? If not, why not?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
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