* Re: [PATCH v3 01/26] x86/virt/tdx: Print SEAMCALL leaf numbers in decimal
From: Dave Hansen @ 2026-01-28 16:26 UTC (permalink / raw)
To: Chao Gao, linux-coco, linux-kernel, kvm, x86
Cc: 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,
Kirill A. Shutemov, Farrah Chen, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-2-chao.gao@intel.com>
On 1/23/26 06:55, Chao Gao wrote:
> Both TDX spec and kernel defines SEAMCALL leaf numbers as decimal. Printing
> them in hex makes no sense. Correct it.
This patch has zero to do with "Runtime TDX Module update support". Why
is it in this series?
^ permalink raw reply
* Re: [PATCH v3 02/26] x86/virt/tdx: Use %# prefix for hex values in SEAMCALL error messages
From: Dave Hansen @ 2026-01-28 15:18 UTC (permalink / raw)
To: Chao Gao, linux-coco, linux-kernel, kvm, x86
Cc: reinette.chatre, ira.weiny, kai.huang, dan.j.williams, yilun.xu,
sagis, vannapurve, paulmck, nik.borisov, zhenzhong.duan, seanjc,
rick.p.edgecombe, kas, dave.hansen, vishal.l.verma,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-3-chao.gao@intel.com>
On 1/23/26 06:55, Chao Gao wrote:
> "%#" format specifier automatically adds the "0x" prefix and has one less
> character than "0x%".
IMNHO, this is needless bikeshedding about existing code. It's only
function is going to be to distract reviewers from the parts of the set
that actually need review.
If this matters (it doesn't), send it as a separate patch, please.
^ permalink raw reply
* Re: [PATCH v3 03/26] x86/virt/tdx: Move low level SEAMCALL helpers out of <asm/tdx.h>
From: Chao Gao @ 2026-01-28 12:42 UTC (permalink / raw)
To: Binbin Wu
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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <f8329aaf-7074-4bcc-b05b-b50a639cc970@linux.intel.com>
On Wed, Jan 28, 2026 at 09:37:05AM +0800, Binbin Wu wrote:
>
>
>On 1/23/2026 10:55 PM, Chao Gao wrote:
>> From: Kai Huang <kai.huang@intel.com>
>>
>> TDX host core code implements three seamcall*() helpers to make SEAMCALL
>> to the TDX module. Currently, they are implemented in <asm/tdx.h> and
>> are exposed to other kernel code which includes <asm/tdx.h>.
>>
>> However, other than the TDX host core, seamcall*() are not expected to
>> be used by other kernel code directly. For instance, for all SEAMCALLs
>> that are used by KVM, the TDX host core exports a wrapper function for
>> each of them.
>>
>> Move seamcall*() and related code out of <asm/tdx.h> and make them only
>> visible to TDX host core.
>>
>> Since TDX host core tdx.c is already very heavy, don't put low level
>> seamcall*() code there but to a new dedicated "seamcall.h". Also,
>> currently tdx.c has seamcall_prerr*() helpers which additionally print
>> error message when calling seamcall*() fails. Move them to "seamcall.h"
>> as well. In such way all low level SEAMCALL helpers are in a dedicated
>> place, which is much more readable.
>>
>> Signed-off-by: Kai Huang <kai.huang@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
>
>One question below.
>
>[...]
>
>> diff --git a/arch/x86/virt/vmx/tdx/seamcall.h b/arch/x86/virt/vmx/tdx/seamcall.h
>> new file mode 100644
>> index 000000000000..0912e03fabfe
>> --- /dev/null
>> +++ b/arch/x86/virt/vmx/tdx/seamcall.h
>> @@ -0,0 +1,99 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2025 Intel Corporation */
>
>Should this be updated to 2026?
Yes. And I may drop the copyright notice if it is not necessary.
According to [1][2], it seems to be optional or even discouraged.
[1]: https://lwn.net/Articles/912355/
[2]: https://www.linuxfoundation.org/blog/blog/copyright-notices-in-open-source-software-projects
^ permalink raw reply
* Re: [PATCH v3 02/26] x86/virt/tdx: Use %# prefix for hex values in SEAMCALL error messages
From: Chao Gao @ 2026-01-28 12:16 UTC (permalink / raw)
To: Binbin Wu
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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <35fa8047-7506-4a78-b493-732160c3d25c@linux.intel.com>
On Wed, Jan 28, 2026 at 09:34:03AM +0800, Binbin Wu wrote:
>
>
>On 1/23/2026 10:55 PM, Chao Gao wrote:
>> "%#" format specifier automatically adds the "0x" prefix and has one less
>> character than "0x%".
>>
>> For conciseness, replace "0x%" with "%#" when printing hexadecimal values
>> in SEAMCALL error messages.
>>
>> Suggested-by: Dan Williams <dan.j.williams@intel.com>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>
>Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
>
>> ---
>> "0x%" is also used to print TDMR ranges. I didn't convert them to reduce
>> code churn, but if they should be converted for consistency, I'm happy
>> to do that.
>
>Generally, is there any preference for coding in Linux kernel about
>"0x%" VS. "%#"? Or developers just make their own choices?
There seems to be no clear guidance on "0x%x" vs. "%#x".
If anyone has strong objections to this change, I can definitely drop it. I
included this patch because Dan suggested it during his review, though I'm not
sure how strongly he feels about it.
I searched lore and found the example below where "%#x" is preferred in another
subsystem:
https://lore.kernel.org/all/20251202231352.GF1712166@ZenIV/
^ permalink raw reply
* Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-01-28 11:28 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: <fc3e72ec4443afd79ccade31e9e0036e645e567b.camel@intel.com>
On Tue, Jan 27, 2026 at 11:21:06AM +0800, Huang, Kai wrote:
>
>> +/*
>> + * Allocate and populate a seamldr_params.
>> + * Note that both @module and @sig should be vmalloc'd memory.
>> + */
>> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
>> + const void *sig, unsigned int sig_size)
>> +{
>> + struct seamldr_params *params;
>> + const u8 *ptr;
>> + int i;
>> +
>> + BUILD_BUG_ON(sizeof(struct seamldr_params) != SZ_4K);
>> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (!IS_ALIGNED(module_size, SZ_4K) || sig_size != SZ_4K ||
>> + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
>> + !IS_ALIGNED((unsigned long)sig, SZ_4K))
>> + return ERR_PTR(-EINVAL);
>> +
>
>Based on the the blob format link below, we have
>
>struct tdx_blob
>{
> ...
> _u64 sigstruct[256]; // 2KB sigstruct,intel_tdx_module.so.sigstruct
> _u64 reserved2[256]; // Reserved space
> ...
>}
>
>So it's clear SIGSTRUCT is just 2KB and the second half 2KB is "reserved
>space".
>
>Why is the "reserved space" treated as part of SIGSTRUCT here?
Good question. Because the space is reserved for sigstruct expansion.
The __current__ SEAMLDR ABI accepts one 4KB page, but all __existing__
sigstructs are only 2KB. so, tdx_blob currently defines a 2KB sigstruct field
followed by 2KB of reserved space. We anticipate that sigstructs will
eventually exceed 4KB, so we added reserved3[N*512] to accommodate future
growth.
You're right. The current tdx_blob definition doesn't clearly indicate that
reserved2/3 are actually part of the sigstruct.
Does this revised tdx_blob definition make that clearer and better align with
this patch? The idea is to make tdx_blob generic enough to clearly represent:
a 4KB header, followed by 4KB-aligned sigstruct, followed by the TDX Module
binary. Current SEAMLDR ABI details or current sigstruct sizes are irrelevant.
struct tdx_blob
{
_u16 version; // Version number
_u16 checksum; // Checksum of the entire blob should be zero
_u32 offset_of_module; // Offset of the module binary intel_tdx_module.bin in bytes
_u8 signature[8]; // Must be "TDX-BLOB"
_u32 length; // The length in bytes of the entire blob
_u32 reserved0; // Reserved space
_u64 reserved1[509]; // Reserved space
_u64 sigstruct[512 + N*512]; // sigstruct, 4KB aligned
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_u8 module[]; // intel_tdx_module.bin, 4KB aligned, to the end of the file
}
>
>> +
>> +/*
>> + * Intel TDX Module blob. Its format is defined at:
>> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
>> + */
>> +struct tdx_blob {
>> + u16 version;
>> + u16 checksum;
>> + u32 offset_of_module;
>> + u8 signature[8];
>> + u32 len;
>> + u32 resv1;
>> + u64 resv2[509];
>
>Nit: Perhaps s/resv/rsvd ?
>
Sure. Will do.
>"#grep rsvd arch/x86 -Rn" gave me a bunch of results but "#grep resv" gave
>me much less (and part of the results were 'resvd' and 'resv_xx' instead of
>plain 'resv').
>
>> + u8 data[];
>> +} __packed;
>
>For this structure, I need to click the link and open it in a browser to
>understand where is the sigstruct and module, and ...
>
>> +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;
>> +
>> + if (blob->version != 0x100) {
>> + pr_err("unsupported blob version: %x\n", blob->version);
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + if (blob->resv1 || memchr_inv(blob->resv2, 0, sizeof(blob->resv2))) {
>> + pr_err("non-zero reserved fields\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + /* Split the given blob into a sigstruct and a module */
>> + sig = blob->data;
>> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
>> + module = data + blob->offset_of_module;
>> + module_size = size - blob->offset_of_module;
>> +
>
>... to see whether this code makes sense.
>
>I understand the
>
> ...
> u64 rsvd[N*512];
> u8 module[];
>
>is painful to be declared explicitly in 'struct tdx_blob' because IIUC we
>cannot put two flexible array members at the end of the structure.
Yes.
>
>But I think if we add 'sigstruct' to the 'struct tdx_blob', e.g.,
>
>struct tdx_blob {
> u16 version;
> ...
> u64 rsvd2[509];
> u64 sigstruct[256];
> u64 rsvd3[256];
> u64 data;
>} __packed;
>
>.. we can just use
>
> sig = blob->sigstruct;
> sig_size = 2K (or 4K I don't quite follow);
>
>which is clearer to read IMHO?
The problem is hard-coding the sigstruct size to 2KB/4KB. This will soon no
longer hold.
But
sig = blob->data;
sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
doesn't make that assumption, making it more future-proof.
>
>> + return alloc_seamldr_params(module, module_size, sig, sig_size);
>> +}
>> +
>
>
>
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Binbin Wu @ 2026-01-28 6:50 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: <20260123145645.90444-9-chao.gao@intel.com>
On 1/23/2026 10:55 PM, 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
> invoking SEAMLDR.INFO SEAMCALL in preparation for exposing P-SEAMLDR
> version and other necessary information to userspace.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
One nit below.
[...]
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> index b99d73f7bb08..6a83ae405fac 100644
> --- a/arch/x86/virt/vmx/tdx/seamldr.c
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -9,9 +9,16 @@
> #include <linux/irqflags.h>
> #include <linux/types.h>
>
> +#include <asm/seamldr.h>
> +
> #include "seamcall.h"
>
> -static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> +/* P-SEAMLDR SEAMCALL leaf function */
> +#define P_SEAMLDR_INFO 0x8000000000000000
> +
> +static struct seamldr_info seamldr_info __aligned(256);
> +
> +static inline int seamldr_call(u64 fn, struct tdx_module_args *args)
No need to tag the local function with inline.
> {
> unsigned long flags;
> u64 vmcs;
> @@ -54,3 +61,11 @@ static __maybe_unused int seamldr_call(u64 fn, struct tdx_module_args *args)
> WARN_ONCE(1, "Failed to save/restore the current VMCS");
> return -EIO;
> }
^ permalink raw reply
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Binbin Wu @ 2026-01-28 6:38 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: <20260123145645.90444-8-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> Software needs to talk with P-SEAMLDR via P-SEAMLDR SEAMCALLs. So, add a
> wrapper for P-SEAMLDR SEAMCALLs.
>
> Save and restore the current VMCS using VMPTRST and VMPTRLD instructions
> to avoid breaking KVM. Doing so is because P-SEAMLDR SEAMCALLs would
> invalidate the current VMCS as documented in Intel® Trust Domain CPU
> Architectural Extensions (May 2021 edition) Chapter 2.3 [1]:
>
> 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.
>
> Disable interrupts to prevent KVM code from interfering with P-SEAMLDR
> SEAMCALLs. For example, if a vCPU is scheduled before the current VMCS is
> restored, it may encounter an invalid current VMCS, causing its VMX
> instruction to fail. Additionally, if KVM sends IPIs to invalidate a
> current VMCS and the invalidation occurs right after the current VMCS is
> saved, that VMCS will be reloaded after P-SEAMLDR SEAMCALLs, leading to
> unexpected behavior.
>
> NMIs are not a problem, as the only scenario where instructions relying on
> the current-VMCS are used is during guest PMI handling in KVM. This occurs
> immediately after VM exits with IRQ and NMI disabled, ensuring no
> interference with P-SEAMLDR SEAMCALLs.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> Link: https://cdrdv2.intel.com/v1/dl/getContent/733582 # [1]
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Two nits below.
> ---
> v2:
> - don't create a new, inferior framework to save/restore VMCS
> - use human-friendly language, just "current VMCS" rather than
> SDM term "current-VMCS pointer"
> - don't mix guard() with goto
> ---
> arch/x86/virt/vmx/tdx/Makefile | 1 +
> arch/x86/virt/vmx/tdx/seamldr.c | 56 ++++++++++++++++++++++++++++++
> drivers/virt/coco/tdx-host/Kconfig | 10 ++++++
> 3 files changed, 67 insertions(+)
> create mode 100644 arch/x86/virt/vmx/tdx/seamldr.c
>
> diff --git a/arch/x86/virt/vmx/tdx/Makefile b/arch/x86/virt/vmx/tdx/Makefile
> index 90da47eb85ee..26aea3531c36 100644
> --- a/arch/x86/virt/vmx/tdx/Makefile
> +++ b/arch/x86/virt/vmx/tdx/Makefile
> @@ -1,2 +1,3 @@
> # SPDX-License-Identifier: GPL-2.0-only
> obj-y += seamcall.o tdx.o
> +obj-$(CONFIG_INTEL_TDX_MODULE_UPDATE) += seamldr.o
> diff --git a/arch/x86/virt/vmx/tdx/seamldr.c b/arch/x86/virt/vmx/tdx/seamldr.c
> new file mode 100644
> index 000000000000..b99d73f7bb08
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/seamldr.c
> @@ -0,0 +1,56 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright(c) 2025 Intel Corporation.
Update to 2026?
> + *
> + * Intel TDX module runtime update
> + */
> +#define pr_fmt(fmt) "seamldr: " fmt
> +
> +#include <linux/irqflags.h>
> +#include <linux/types.h>
> +
> +#include "seamcall.h"
> +
> +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;
> +
> + /*
> + * 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).
> + */
> + local_irq_save(flags);
> +
> + asm goto("1: vmptrst %0\n\t"
> + _ASM_EXTABLE(1b, %l[error])
> + : "=m" (vmcs) : : "cc" : error);
> +
> + ret = seamldr_prerr(fn, args);
> +
> + /*
> + * Restore the current VMCS pointer. VMPTSTR "returns" all ones if the
s/VMPTSTR/VMPTRST
^ permalink raw reply
* Re: [PATCH v3 06/26] x86/virt/tdx: Prepare to support P-SEAMLDR SEAMCALLs
From: Binbin Wu @ 2026-01-28 5:58 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: <20260123145645.90444-7-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> P-SEAMLDR is another component alongside the TDX module within the
> protected SEAM range. P-SEAMLDR can update the TDX module at runtime.
> Software can talk with P-SEAMLDR via SEAMCALLs with the bit 63 of RAX
> (leaf number) set to 1 (a.k.a P-SEAMLDR SEAMCALLs).
>
> P-SEAMLDR SEAMCALLs differ from SEAMCALLs of the TDX module in terms of
> error codes and the handling of the current VMCS.
>
> In preparation for adding support for P-SEAMLDR SEAMCALLs, do the two
> following changes to SEAMCALL low-level helpers:
>
> 1) Tweak sc_retry() to retry on "lack of entropy" errors reported by
> P-SEAMLDR because it uses a different error code.
>
> 2) Add seamldr_err() to log error messages on P-SEAMLDR SEAMCALL failures.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request
From: Huang, Kai @ 2026-01-28 4:03 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: 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: <20260123145645.90444-14-chao.gao@intel.com>
> +/*
> + * 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.
> + */
> +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
> + const void *sig, unsigned int sig_size)
> +{
>
[...]
> + ptr = module;
> + for (i = 0; i < params->num_module_pages; i++) {
> + params->mod_pages_pa_list[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
> + ptr += SZ_4K;
> + }
> +
> + return params;
> +}
>
[...]
> +/*
> + * Verify that the checksum of the entire blob is zero. The checksum is
> + * calculated by summing up all 16-bit words, with carry bits dropped.
> + */
> +static bool verify_checksum(const struct tdx_blob *blob)
> +{
> + u32 size = blob->len;
> + u16 checksum = 0;
> + const u16 *p;
> + int i;
> +
> + /* Handle the last byte if the size is odd */
> + if (size % 2) {
> + checksum += *((const u8 *)blob + size - 1);
> + size--;
> + }
> +
> + p = (const u16 *)blob;
> + for (i = 0; i < size; i += 2) {
> + checksum += *p;
> + p++;
> + }
> +
> + return !checksum;
> +}
> +
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
>
[...]
> + 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?
If so a comment would be helpful.
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.
^ permalink raw reply
* Re: [PATCH v3 05/26] coco/tdx-host: Expose TDX Module version
From: Binbin Wu @ 2026-01-28 3:48 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
In-Reply-To: <20260123145645.90444-6-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> For TDX Module updates, userspace needs to select compatible update
> versions based on the current module version. This design delegates
> module selection complexity to userspace because TDX Module update
> policies are complex and version series are platform-specific.
>
> For example, the 1.5.x series is for certain platform generations, while
> the 2.0.x series is intended for others. And TDX Module 1.5.x may be
> updated to 1.5.y but not to 1.5.y+1.
>
> Expose the TDX Module version to userspace via sysfs to aid module
> selection. Since the TDX faux device will drive module updates, expose
> the version as its attribute.
>
> This approach follows the pattern used by microcode updates and other
> CoCo implementations:
>
> 1. AMD has a PCI device for the PSP for SEV which provides an existing
> place to hang their equivalent metadata.
>
> 2. ARM CCA will likely have a faux device (although it isn't obvious if
> they have a need to export version information there) [1]
>
> 3. Microcode revisions are exposed as CPU device attributes
>
> One bonus of exposing TDX Module version via sysfs is: TDX Module
> version information remains available even after dmesg logs are cleared.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Link: https://lore.kernel.org/all/2025073035-bulginess-rematch-b92e@gregkh/ # [1]
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 10/26] coco/tdx-host: Implement FW_UPLOAD sysfs ABI for TDX Module updates
From: Huang, Kai @ 2026-01-28 3:30 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: 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: <20260123145645.90444-11-chao.gao@intel.com>
> 2. TDX Module Updates complete synchronously within .write(), meaning
> .poll_complete() is only called after successful updates and therefore
> always returns success
Nit:
Why "updates" instead of "update"? Is there multiple updates possible
within .write()?
[...]
>
>
> +struct tdx_fw_upload_status {
> + bool cancel_request;
> +};
> +
> +struct fw_upload *tdx_fwl;
Can 'tdx_fwl' be static?
[...]
>
> +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;
> +
> + if (!IS_ENABLED(CONFIG_INTEL_TDX_MODULE_UPDATE))
> + return;
> +
> + if (!tdx_supports_runtime_update(tdx_sysinfo))
> + pr_info("Current TDX Module cannot be updated. Consider BIOS updates\n");
What's the point of continuing if runtime update is not supported?
> +
> + tdx_fwl = firmware_upload_register(THIS_MODULE, dev, "seamldr_upload",
> + &tdx_fw_ops, &tdx_fw_upload_status);
> + ret = PTR_ERR_OR_ZERO(tdx_fwl);
> + if (ret)
> + pr_err("failed to register module uploader %d\n", ret);
> +}
> +
> +static void seamldr_deinit(void)
> +{
> + if (tdx_fwl)
> + firmware_upload_unregister(tdx_fwl);
> +}
> +
> +static int tdx_host_probe(struct faux_device *fdev)
> +{
> + seamldr_init(&fdev->dev);
IMHO you need a comment to explain why seamldr_init() doesn't return error
and tdx_host_probe() already returns success?
> + return 0;
> +}
> +
> +static void tdx_host_remove(struct faux_device *fdev)
> +{
> + seamldr_deinit();
> +}
> +
> +static struct faux_device_ops tdx_host_ops = {
> + .probe = tdx_host_probe,
> + .remove = tdx_host_remove,
> +};
> +
> static struct faux_device *fdev;
>
> static int __init tdx_host_init(void)
> @@ -107,7 +229,7 @@ static int __init tdx_host_init(void)
> if (!x86_match_cpu(tdx_host_ids) || !tdx_get_sysinfo())
> return -ENODEV;
>
> - fdev = faux_device_create_with_groups(KBUILD_MODNAME, NULL, NULL, tdx_host_groups);
> + fdev = faux_device_create_with_groups(KBUILD_MODNAME, NULL, &tdx_host_ops, tdx_host_groups);
> if (!fdev)
> return -ENODEV;
>
^ permalink raw reply
* Re: [PATCH v3 04/26] coco/tdx-host: Introduce a "tdx_host" device
From: Binbin Wu @ 2026-01-28 3:24 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, Jonathan Cameron, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-5-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
[...]
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index df1cfaf26c65..f7691f64fbe3 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -17,5 +17,7 @@ source "drivers/virt/coco/arm-cca-guest/Kconfig"
> source "drivers/virt/coco/guest/Kconfig"
> endif
>
> +source "drivers/virt/coco/tdx-host/Kconfig"
> +
> config TSM
> bool
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index cb52021912b3..b323b0ae4f82 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -6,6 +6,7 @@ obj-$(CONFIG_EFI_SECRET) += efi_secret/
> obj-$(CONFIG_ARM_PKVM_GUEST) += pkvm-guest/
> obj-$(CONFIG_SEV_GUEST) += sev-guest/
> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
> +obj-$(CONFIG_INTEL_TDX_HOST) += tdx-host/
IIUC, the folder name "tdx-host" here stands for TDX host services?
Should it use CONFIG_TDX_HOST_SERVICES here?
> obj-$(CONFIG_ARM_CCA_GUEST) += arm-cca-guest/
> obj-$(CONFIG_TSM) += tsm-core.o
> obj-$(CONFIG_TSM_GUEST) += guest/
> diff --git a/drivers/virt/coco/tdx-host/Kconfig b/drivers/virt/coco/tdx-host/Kconfig
> new file mode 100644
> index 000000000000..e58bad148a35
> --- /dev/null
> +++ b/drivers/virt/coco/tdx-host/Kconfig
> @@ -0,0 +1,10 @@
> +config TDX_HOST_SERVICES
> + tristate "TDX Host Services Driver"
> + depends on INTEL_TDX_HOST
> + default m
> + help
> + Enable access to TDX host services like module update and
> + extensions (e.g. TDX Connect).
> +
> + Say y or m if enabling support for confidential virtual machine
> + support (CONFIG_INTEL_TDX_HOST). The module is called tdx_host.ko
> diff --git a/drivers/virt/coco/tdx-host/Makefile b/drivers/virt/coco/tdx-host/Makefile
> new file mode 100644
> index 000000000000..e61e749a8dff
> --- /dev/null
> +++ b/drivers/virt/coco/tdx-host/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_TDX_HOST_SERVICES) += tdx-host.o
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> new file mode 100644
> index 000000000000..c77885392b09
> --- /dev/null
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -0,0 +1,43 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * TDX host user interface driver
> + *
> + * Copyright (C) 2025 Intel Corporation
Nit:
Update the year to 2026?
^ permalink raw reply
* Re: [PATCH v3 09/26] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Huang, Kai @ 2026-01-28 3:07 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: Edgecombe, Rick P, seanjc@google.com, Duan, Zhenzhong,
yilun.xu@linux.intel.com, sagis@google.com, Chatre, Reinette,
kas@kernel.org, Williams, Dan J, paulmck@kernel.org, Weiny, Ira,
Annapurve, Vishal, nik.borisov@suse.com,
dave.hansen@linux.intel.com, Verma, Vishal L, Chen, Farrah
In-Reply-To: <20260123145645.90444-10-chao.gao@intel.com>
On Fri, 2026-01-23 at 06:55 -0800, Chao Gao wrote:
> -ATTRIBUTE_GROUPS(tdx_host);
> +
> +struct attribute_group tdx_host_group = {
> + .attrs = tdx_host_attrs,
> +};
> +
This 'tdx_host_group' can be static?
^ permalink raw reply
* Re: [PATCH v3 03/26] x86/virt/tdx: Move low level SEAMCALL helpers out of <asm/tdx.h>
From: Binbin Wu @ 2026-01-28 1:37 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-4-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> From: Kai Huang <kai.huang@intel.com>
>
> TDX host core code implements three seamcall*() helpers to make SEAMCALL
> to the TDX module. Currently, they are implemented in <asm/tdx.h> and
> are exposed to other kernel code which includes <asm/tdx.h>.
>
> However, other than the TDX host core, seamcall*() are not expected to
> be used by other kernel code directly. For instance, for all SEAMCALLs
> that are used by KVM, the TDX host core exports a wrapper function for
> each of them.
>
> Move seamcall*() and related code out of <asm/tdx.h> and make them only
> visible to TDX host core.
>
> Since TDX host core tdx.c is already very heavy, don't put low level
> seamcall*() code there but to a new dedicated "seamcall.h". Also,
> currently tdx.c has seamcall_prerr*() helpers which additionally print
> error message when calling seamcall*() fails. Move them to "seamcall.h"
> as well. In such way all low level SEAMCALL helpers are in a dedicated
> place, which is much more readable.
>
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
One question below.
[...]
> diff --git a/arch/x86/virt/vmx/tdx/seamcall.h b/arch/x86/virt/vmx/tdx/seamcall.h
> new file mode 100644
> index 000000000000..0912e03fabfe
> --- /dev/null
> +++ b/arch/x86/virt/vmx/tdx/seamcall.h
> @@ -0,0 +1,99 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2025 Intel Corporation */
Should this be updated to 2026?
^ permalink raw reply
* Re: [PATCH v3 02/26] x86/virt/tdx: Use %# prefix for hex values in SEAMCALL error messages
From: Binbin Wu @ 2026-01-28 1:34 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, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-3-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> "%#" format specifier automatically adds the "0x" prefix and has one less
> character than "0x%".
>
> For conciseness, replace "0x%" with "%#" when printing hexadecimal values
> in SEAMCALL error messages.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> "0x%" is also used to print TDMR ranges. I didn't convert them to reduce
> code churn, but if they should be converted for consistency, I'm happy
> to do that.
Generally, is there any preference for coding in Linux kernel about
"0x%" VS. "%#"? Or developers just make their own choices?
>
> v2: new
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index dbc7cb08ca53..2218bb42af40 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -63,16 +63,16 @@ typedef void (*sc_err_func_t)(u64 fn, u64 err, struct tdx_module_args *args);
>
> static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
> {
> - pr_err("SEAMCALL (%llu) failed: 0x%016llx\n", fn, err);
> + pr_err("SEAMCALL (%llu) failed: %#016llx\n", fn, err);
> }
>
> static inline void seamcall_err_ret(u64 fn, u64 err,
> struct tdx_module_args *args)
> {
> seamcall_err(fn, err, args);
> - pr_err("RCX 0x%016llx RDX 0x%016llx R08 0x%016llx\n",
> + pr_err("RCX %#016llx RDX %#016llx R08 %#016llx\n",
> args->rcx, args->rdx, args->r8);
> - pr_err("R09 0x%016llx R10 0x%016llx R11 0x%016llx\n",
> + pr_err("R09 %#016llx R10 %#016llx R11 %#016llx\n",
> args->r9, args->r10, args->r11);
> }
>
^ permalink raw reply
* Re: [PATCH v3 01/26] x86/virt/tdx: Print SEAMCALL leaf numbers in decimal
From: Binbin Wu @ 2026-01-28 1:28 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, Kirill A. Shutemov, Farrah Chen,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-2-chao.gao@intel.com>
On 1/23/2026 10:55 PM, Chao Gao wrote:
> Both TDX spec and kernel defines SEAMCALL leaf numbers as decimal. Printing
> them in hex makes no sense. Correct it.
>
> Suggested-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> Reviewed-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> v2:
> - print leaf numbers with %llu
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 5ce4ebe99774..dbc7cb08ca53 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -63,7 +63,7 @@ typedef void (*sc_err_func_t)(u64 fn, u64 err, struct tdx_module_args *args);
>
> static inline void seamcall_err(u64 fn, u64 err, struct tdx_module_args *args)
> {
> - pr_err("SEAMCALL (0x%016llx) failed: 0x%016llx\n", fn, err);
> + pr_err("SEAMCALL (%llu) failed: 0x%016llx\n", fn, err);
> }
>
> static inline void seamcall_err_ret(u64 fn, u64 err,
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Vedashree Vidwans @ 2026-01-27 23:01 UTC (permalink / raw)
To: Salman Nabi, 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>
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
> 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
>
> 6c0762a6-12f2-4b56-92cb-ba8f633606d9:
> total 0
> --w------- 1 0 0 4096 Jan 19 11:33 activate
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_capable
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_pending
> --w------- 1 0 0 4096 Jan 19 11:33 cancel
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 current_version
> -rw-r--r-- 1 0 0 4096 Jan 19 11:33 force_cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 may_reset_cpu
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 name
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 pending_version
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # grep . *
> grep: activate: Permission denied
> activation_capable:1
> activation_pending:1
> grep: cancel: Permission denied
> cpu_rendezvous:1
> current_version:0.0
> force_cpu_rendezvous:1
> may_reset_cpu:0
> name:TF-RMM
> pending_version:0.0
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # echo 1 > activate
> [ 2825.797871] Arm LFA: firmware activation succeeded.
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 #
> ==========================================
>
> [1] https://developer.arm.com/documentation/den0147/latest/
>
> Signed-off-by: Salman Nabi <salman.nabi@arm.com>
> ---
> 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/Kconfig b/drivers/firmware/smccc/Kconfig
> index 15e7466179a6..ff7ca49486b0 100644
> --- a/drivers/firmware/smccc/Kconfig
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -23,3 +23,11 @@ config ARM_SMCCC_SOC_ID
> help
> Include support for the SoC bus on the ARM SMCCC firmware based
> platforms providing some sysfs information about the SoC variant.
> +
> +config ARM_LFA
> + tristate "Arm Live Firmware activation support"
> + depends on HAVE_ARM_SMCCC_DISCOVERY
> + default y
> + help
> + Include support for triggering Live Firmware Activation, which
> + allows to upgrade certain firmware components without a reboot.
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 40d19144a860..a6dd01558a94 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -2,3 +2,4 @@
> #
> obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o kvm_guest.o
> obj-$(CONFIG_ARM_SMCCC_SOC_ID) += soc_id.o
> +obj-$(CONFIG_ARM_LFA) += lfa_fw.o
> 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
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/stop_machine.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/psci.h>
> +#include <uapi/linux/psci.h>
> +#include <linux/uuid.h>
> +#include <linux/array_size.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "Arm LFA: " fmt
> +
> +/* LFA v1.0b0 specification */
> +#define LFA_1_0_FN_BASE 0xc40002e0
> +#define LFA_1_0_FN(n) (LFA_1_0_FN_BASE + (n))
> +
> +#define LFA_1_0_FN_GET_VERSION LFA_1_0_FN(0)
> +#define LFA_1_0_FN_CHECK_FEATURE LFA_1_0_FN(1)
> +#define LFA_1_0_FN_GET_INFO LFA_1_0_FN(2)
> +#define LFA_1_0_FN_GET_INVENTORY LFA_1_0_FN(3)
> +#define LFA_1_0_FN_PRIME LFA_1_0_FN(4)
> +#define LFA_1_0_FN_ACTIVATE LFA_1_0_FN(5)
> +#define LFA_1_0_FN_CANCEL LFA_1_0_FN(6)
> +
> +/* CALL_AGAIN flags (returned by SMC) */
> +#define LFA_PRIME_CALL_AGAIN BIT(0)
> +#define LFA_ACTIVATE_CALL_AGAIN BIT(0)
> +
> +/* LFA return values */
> +#define LFA_SUCCESS 0
> +#define LFA_NOT_SUPPORTED 1
> +#define LFA_BUSY 2
> +#define LFA_AUTH_ERROR 3
> +#define LFA_NO_MEMORY 4
> +#define LFA_CRITICAL_ERROR 5
> +#define LFA_DEVICE_ERROR 6
> +#define LFA_WRONG_STATE 7
> +#define LFA_INVALID_PARAMETERS 8
> +#define LFA_COMPONENT_WRONG_STATE 9
> +#define LFA_INVALID_ADDRESS 10
> +#define LFA_ACTIVATION_FAILED 11
> +
> +#define LFA_ERROR_STRING(name) \
> + [name] = #name
> +
> +static const char * const lfa_error_strings[] = {
> + LFA_ERROR_STRING(LFA_SUCCESS),
> + LFA_ERROR_STRING(LFA_NOT_SUPPORTED),
> + LFA_ERROR_STRING(LFA_BUSY),
> + LFA_ERROR_STRING(LFA_AUTH_ERROR),
> + LFA_ERROR_STRING(LFA_NO_MEMORY),
> + LFA_ERROR_STRING(LFA_CRITICAL_ERROR),
> + LFA_ERROR_STRING(LFA_DEVICE_ERROR),
> + LFA_ERROR_STRING(LFA_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_PARAMETERS),
> + LFA_ERROR_STRING(LFA_COMPONENT_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_ADDRESS),
> + LFA_ERROR_STRING(LFA_ACTIVATION_FAILED)
> +};
> +
> +enum image_attr_names {
> + LFA_ATTR_NAME,
> + LFA_ATTR_CURRENT_VERSION,
> + LFA_ATTR_PENDING_VERSION,
> + LFA_ATTR_ACT_CAPABLE,
> + LFA_ATTR_ACT_PENDING,
> + LFA_ATTR_MAY_RESET_CPU,
> + LFA_ATTR_CPU_RENDEZVOUS,
> + LFA_ATTR_FORCE_CPU_RENDEZVOUS,
> + LFA_ATTR_ACTIVATE,
> + LFA_ATTR_CANCEL,
> + LFA_ATTR_NR_IMAGES
> +};
> +
> +struct image_props {
> + struct list_head image_node;
> + const char *image_name;
> + int fw_seq_id;
> + u64 current_version;
> + u64 pending_version;
> + bool activation_capable;
> + bool activation_pending;
> + bool may_reset_cpu;
> + bool cpu_rendezvous;
> + bool cpu_rendezvous_forced;
> + struct kobject *image_dir;
> + struct kobj_attribute image_attrs[LFA_ATTR_NR_IMAGES];
> +};
> +static LIST_HEAD(lfa_fw_images);
> +
> +/* A UUID split over two 64-bit registers */
> +struct uuid_regs {
> + u64 uuid_lo;
> + u64 uuid_hi;
> +};
> +
> +static const struct fw_image_uuid {
> + const char *name;
> + const char *uuid;
> +} fw_images_uuids[] = {
> + {
> + .name = "TF-A BL31 runtime",
> + .uuid = "47d4086d-4cfe-9846-9b95-2950cbbd5a00",
> + },
> + {
> + .name = "BL33 non-secure payload",
> + .uuid = "d6d0eea7-fcea-d54b-9782-9934f234b6e4",
> + },
> + {
> + .name = "TF-RMM",
> + .uuid = "6c0762a6-12f2-4b56-92cb-ba8f633606d9",
> + },
> +};
> +
> +static struct kobject *lfa_dir;
> +static DEFINE_MUTEX(lfa_lock);
> +static struct workqueue_struct *fw_images_update_wq;
> +static struct work_struct fw_images_update_work;
> +
> +static int update_fw_images_tree(void);
> +
> +static void delete_fw_image_node(struct image_props *attrs)
> +{
> + int i;
> +
> + for (i = 0; i < LFA_ATTR_NR_IMAGES; i++)
> + sysfs_remove_file(attrs->image_dir, &attrs->image_attrs[i].attr);
> +
> + kobject_put(attrs->image_dir);
> + list_del(&attrs->image_node);
> + kfree(attrs);
> +}
> +
> +static void remove_invalid_fw_images(struct work_struct *work)
> +{
> + struct image_props *attrs, *tmp;
> +
> + mutex_lock(&lfa_lock);
> +
> + /*
> + * Remove firmware images including directories that are no longer
> + * present in the LFA agent after updating the existing ones.
> + */
> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node) {
> + if (attrs->fw_seq_id == -1)
> + delete_fw_image_node(attrs);
> + }
> +
> + mutex_unlock(&lfa_lock);
> +}
> +
> +static void set_image_flags(struct image_props *attrs, int seq_id,
> + u32 image_flags, u64 reg_current_ver,
> + u64 reg_pending_ver)
> +{
> + attrs->fw_seq_id = seq_id;
> + attrs->current_version = reg_current_ver;
> + attrs->pending_version = reg_pending_ver;
> + attrs->activation_capable = !!(image_flags & BIT(0));
> + attrs->activation_pending = !!(image_flags & BIT(1));
> + attrs->may_reset_cpu = !!(image_flags & BIT(2));
> + /* cpu_rendezvous_optional bit has inverse logic in the spec */
> + attrs->cpu_rendezvous = !(image_flags & BIT(3));
> +}
> +
> +static unsigned long get_nr_lfa_components(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_GET_INFO;
> + reg.a1 = 0; /* lfa_info_selector = 0 */
> +
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 != LFA_SUCCESS)
> + return reg.a0;
> +
> + return reg.a1;
> +}
> +
> +static int lfa_cancel(void *data)
> +{
> + struct image_props *attrs = data;
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_CANCEL;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> +
> + /*
> + * When firmware activation is called with "skip_cpu_rendezvous=1",
> + * LFA_CANCEL can fail with LFA_BUSY if the activation could not be
> + * cancelled.
> + */
> + if (reg.a0 == LFA_SUCCESS) {
> + pr_info("Activation cancelled for image %s\n",
> + attrs->image_name);
> + } else {
> + pr_err("Firmware activation could not be cancelled: %s\n",
> + lfa_error_strings[-reg.a0]);
> + return -EINVAL;
> + }
> +
> + return reg.a0;
> +}
> +
> +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.
> +
> + 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.
> +
> + 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.
> +}
> +
> +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.
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 2/2] KVM: SEV: Add support for IBPB-on-Entry
From: Kim Phillips @ 2026-01-27 20:56 UTC (permalink / raw)
To: Nikunj A. Dadhania, linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak, Tom Lendacky,
Michael Roth, Borislav Petkov, Borislav Petkov, Naveen Rao,
David Kaplan
In-Reply-To: <4dea11f9-6034-489b-acaf-9a150818d1a1@amd.com>
On 1/27/26 12:38 AM, Nikunj A. Dadhania wrote:
Hi Nikunj,
> On 1/27/2026 4:12 AM, Kim Phillips wrote:
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index ea515cf41168..8a6d25db0c00 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -3165,8 +3165,15 @@ void __init sev_hardware_setup(void)
>> cpu_feature_enabled(X86_FEATURE_NO_NESTED_DATA_BP))
>> sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
>>
>> - if (sev_snp_enabled && tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
>> + if (!sev_snp_enabled)
>> + return;
>> + /* the following feature bit checks are SNP specific */
>> +
>
> The early return seems to split up the SNP features unnecessarily.
>
> Keeping everything under `if (sev_snp_enabled)` is cleaner IMO -
> it's clear that these features belong together. Plus, when
> someone adds the next SNP feature, they won't have to think about
> whether it goes before or after the return. The comment about
> "SNP specific" features becomes redundant as well.
The SNP 'togetherness' semantics are maintained whether under an
'if (sev_snp_enabled)' body, or after an 'if (!sev_snp_enabled) return;'.
Only SNP-specific things are being done in the trailing part of the function,
so it naturally lends itself to do the early return. It makes it more
readable by eliminating the unnecessary indentation created by an
'if (sev_snp_enabled)' body.
Meanwhile, I agree with your comments on the first patch in the series.
Thanks for your review,
Kim
^ permalink raw reply
* Re: [PATCH v3 26/26] coco/tdx-host: Set and document TDX Module update expectations
From: dan.j.williams @ 2026-01-27 17:23 UTC (permalink / raw)
To: Chao Gao, dan.j.williams
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, yilun.xu, sagis, vannapurve, paulmck, nik.borisov,
zhenzhong.duan, seanjc, rick.p.edgecombe, kas, dave.hansen,
vishal.l.verma
In-Reply-To: <aXis76vQhWi3RvEB@intel.com>
Chao Gao wrote:
[..]
> >So, remove "compat_capable" ABI. Amend the "error" ABI documentation
> >with the details for avoiding failures and the risk of running updates
> >on configurations that support update but not collision avoidance.
>
> Got it. I will modify this patch as follows:
Overall, looks good to me. You can add:
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
...after a few additional fixups below:
> diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> index a3f155977016..0a68e68375fa 100644
> --- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> +++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
> @@ -29,3 +29,57 @@ Description: (RO) Report the number of remaining updates that can be performed.
> 4.2 "SEAMLDR.INSTALL" for more information. The documentation is
> available at:
> https://cdrdv2-public.intel.com/739045/intel-tdx-seamldr-interface-specification.pdf
> +
> +What: /sys/devices/faux/tdx_host/firmware/seamldr_upload
> +Contact: linux-coco@lists.linux.dev
> +Description: (Directory) The seamldr_upload directory implements the
> + fw_upload sysfs ABI, see
> + Documentation/ABI/testing/sysfs-class-firmware for the general
> + description of the attributes @data, @cancel, @error, @loading,
> + @remaining_size, and @status. This ABI facilitates "Compatible
> + TDX Module Updates". A compatible update is one that meets the
> + following criteria:
> +
> + Does not interrupt or interfere with any current TDX
> + operation or TD VM.
> +
> + Does not invalidate any previously consumed Module metadata
> + values outside of the TEE_TCB_SVN_2 field (updated Security
> + Version Number) in TD Quotes.
> +
> + Does not require validation of new Module metadata fields. By
> + implication, new Module features and capabilities are only
> + available by installing the Module at reboot (BIOS or EFI
> + helper loaded).
> +
> + See tdx_host/firmware/seamldr_upload/error for more details.
> +
> +What: /sys/devices/faux/tdx_host/firmware/seamldr_upload/error
> +Contact: linux-coco@lists.linux.dev
> +Description: (RO) See Documentation/ABI/testing/sysfs-class-firmware for
> + baseline expectations for this file. The <ERROR> part in the
> + <STATUS>:<ERROR> format can be:
> +
> + "device-busy": Compatibility checks failed or not all CPUs
> + are online
> + "flash-wearout": the number of updates reached the limit.
> + "read-write-error": Memory allocation failed.
> + "hw-error": Cannot communicate with P-SEAMLDR or TDX Module
> + "firmware-invalid": The TDX Module to be installed is invalid
> + or other unexpected errors occurred.
> +
> + "hw-error" or "firmware-invalid" may be fatal, causing all TDs
> + and the TDX Module to be lost and preventing further TDX
> + operations. This occurs when /sys/devices/faux/tdx_host/version
> + becomes unreadable after update failures.
I would specify the exact unambiguous errno value that gets returned on
read when the version become indeterminate, like ENXIO.
> + and the (previous) TDX Module stay running.
> +
> + On certain earlier TDX Module versions, incompatible updates may
> + not trigger "device-busy" errors but instead cause TD
> + attestation failures.
I would just leave this out. It bitrots quickly and does not provide
any actionable information. This is not the kernel's responsibility...
> +
> + See version_select_and_load.py [1] documentation for how to
> + detect compatible updates and whether the current platform
> + components catch errors or let them leak and cause potential TD
> + attestation failures.
> + [1]: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/version_select_and_load.py
...that detail about what happens when compat detection is missing
belongs in the tooling documentation. That documentation does not exist
yet, so this link needs to be replaced with a pointer to documentation
before this goes upstream. I am assuming that we want to create an
actual package that distributions can pick up as project? It might be
worth going through the exercise of packaging the binaries and the tool
as an rpm or deb to get that work bootstrapped.
"version_select_and_load" probably wants a better name like "tdxctl" or
similar.
Note that a tdxctl project would also attract features related to TDX
Connect to wrap common flows around the tdx_host device sysfs ABIs.
^ permalink raw reply
* SVSM Development Call January 28, 2026
From: Jörg Rödel @ 2026-01-27 17:06 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
Here is the call for agenda items for this weeks SVSM development call. Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.
We will use the LF Zoom instance. Details of the meeting can be found in our
governance repository at:
https://github.com/coconut-svsm/governance
The link to the COCONUT-SVSM calendar is:
https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week
The meeting will be recorded and the recording eventually published.
Regards,
Jörg
^ permalink raw reply
* Re: [PATCH v3 26/26] coco/tdx-host: Set and document TDX Module update expectations
From: Chao Gao @ 2026-01-27 12:17 UTC (permalink / raw)
To: dan.j.williams
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, yilun.xu, sagis, vannapurve, paulmck, nik.borisov,
zhenzhong.duan, seanjc, rick.p.edgecombe, kas, dave.hansen,
vishal.l.verma
In-Reply-To: <6977e73a7a121_30951002f@dwillia2-mobl4.notmuch>
On Mon, Jan 26, 2026 at 02:14:18PM -0800, dan.j.williams@intel.com wrote:
>Chao Gao wrote:
>> In rare cases, TDX Module updates may cause TD management operations to
>> fail if they occur during phases of the TD lifecycle that are sensitive
>> to update compatibility.
>
>No. The TDX Module wants to be able to claim that some updates are
>compatible when they are not. If Linux takes on additional exclusions it
>modestly increases the scope of changes that can be included in an
>update. It is not possible to claim "rare" if module updates routinely
>include that problematic scope.
>
>> But not all combinations of P-SEAMLDR, kernel, and TDX Module have the
>> capability to detect and prevent said incompatibilities. Completely
>> disabling TDX Module updates on platforms without the capability would
>> be overkill, as these incompatibility cases are rare and can be
>> addressed by userspace through coordinated scheduling of updates and TD
>> management operations.
>
>"Completely disabling" is not the tradeoff. The tradeoff is whether or
>not the TDX Module meets Linux compatible update requirements or not.
>
>> To set clear expectations for TDX Module updates, expose the capability
>> to detect and prevent these incompatibility cases via sysfs and
>> document the compatibility criteria and indications when those criteria
>> are violated.
>
>Linux derives no benefit from a "compat_capable" kernel ABI. Yes, the
>internals must export the error condition on collision. I am not
>debating that nor revisiting the decision of pre-update-fail, vs
>post-collision-notify. However, if the module violates the Linux
>expectations that is the module's issue to document or preclude. The
>fact that the compatibility contract is ambiguous to the kernel is a
>feature. It puts the onus squarely on module updates to be documented
>(or tools updated to understand) as meeting or violating Linux
>compatibility expectations.
>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> v3:
>> - new, based on a reference patch from Dan Williams
>
>One of the details that is missing is the protocol (module documentation
>or tooling) to determine ahead of time if an update is compatible. That
>obviates the need for "compat_capable" ABI which serves no long term
>purpose. Specifically, the expectation is "run non-compatible updates at
>your own operational risk".
Agreed. We need to add metadata like crypto library version or equivalent
abstraction to the mapping file. This enables userspace to determine whether
module updates meet Linux compatibility requirements. I'll submit a request
for this metadata.
And actually, userspace can already determine if the TDX module supports
"collision avoidance" by reading the "tdx_features0" field from the mapping
file [1].
[1]: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/mapping_file.json
>
>So, remove "compat_capable" ABI. Amend the "error" ABI documentation
>with the details for avoiding failures and the risk of running updates
>on configurations that support update but not collision avoidance.
Got it. I will modify this patch as follows:
diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
index a3f155977016..0a68e68375fa 100644
--- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
+++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
@@ -29,3 +29,57 @@ Description: (RO) Report the number of remaining updates that can be performed.
4.2 "SEAMLDR.INSTALL" for more information. The documentation is
available at:
https://cdrdv2-public.intel.com/739045/intel-tdx-seamldr-interface-specification.pdf
+
+What: /sys/devices/faux/tdx_host/firmware/seamldr_upload
+Contact: linux-coco@lists.linux.dev
+Description: (Directory) The seamldr_upload directory implements the
+ fw_upload sysfs ABI, see
+ Documentation/ABI/testing/sysfs-class-firmware for the general
+ description of the attributes @data, @cancel, @error, @loading,
+ @remaining_size, and @status. This ABI facilitates "Compatible
+ TDX Module Updates". A compatible update is one that meets the
+ following criteria:
+
+ Does not interrupt or interfere with any current TDX
+ operation or TD VM.
+
+ Does not invalidate any previously consumed Module metadata
+ values outside of the TEE_TCB_SVN_2 field (updated Security
+ Version Number) in TD Quotes.
+
+ Does not require validation of new Module metadata fields. By
+ implication, new Module features and capabilities are only
+ available by installing the Module at reboot (BIOS or EFI
+ helper loaded).
+
+ See tdx_host/firmware/seamldr_upload/error for more details.
+
+What: /sys/devices/faux/tdx_host/firmware/seamldr_upload/error
+Contact: linux-coco@lists.linux.dev
+Description: (RO) See Documentation/ABI/testing/sysfs-class-firmware for
+ baseline expectations for this file. The <ERROR> part in the
+ <STATUS>:<ERROR> format can be:
+
+ "device-busy": Compatibility checks failed or not all CPUs
+ are online
+ "flash-wearout": the number of updates reached the limit.
+ "read-write-error": Memory allocation failed.
+ "hw-error": Cannot communicate with P-SEAMLDR or TDX Module
+ "firmware-invalid": The TDX Module to be installed is invalid
+ or other unexpected errors occurred.
+
+ "hw-error" or "firmware-invalid" may be fatal, causing all TDs
+ and the TDX Module to be lost and preventing further TDX
+ operations. This occurs when /sys/devices/faux/tdx_host/version
+ becomes unreadable after update failures. For other errors, TDs
+ and the (previous) TDX Module stay running.
+
+ On certain earlier TDX Module versions, incompatible updates may
+ not trigger "device-busy" errors but instead cause TD
+ attestation failures.
+
+ See version_select_and_load.py [1] documentation for how to
+ detect compatible updates and whether the current platform
+ components catch errors or let them leak and cause potential TD
+ attestation failures.
+ [1]: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/version_select_and_load.py
^ permalink raw reply related
* Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
From: dan.j.williams @ 2026-01-27 6:59 UTC (permalink / raw)
To: Alexey Kardashevskiy, dan.j.williams, linux-crypto
Cc: linux-kernel, Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
David S. Miller, x86, linux-coco, Pratik R . Sampat
In-Reply-To: <196ef0d2-93ac-4872-831b-e803e02b5d95@amd.com>
Alexey Kardashevskiy wrote:
> On 24/1/26 09:59, dan.j.williams@intel.com wrote:
> > Alexey Kardashevskiy wrote:
> >> IDE stream IDs are responsibility of a platform and in some cases
> >> TSM allocates the numbers. AMD SEV TIO though leaves it to the host
> >> OS. Mistakenly stream ID is hard coded to be the same as a traffic
> >> class.
> >
> > I scratched my head at this comment, but now realize that you are
> > saying the existing code used the local @tc, not that the hardware
> > stream ID is in any way related to traffic class, right?
>
> When I did that in the first place, I also wanted to try different
> traffic classes so I just took a shortcut here.
>
> > It would help to detail what the end user visible effects of this
> > bug are. The TSM framework does not allow for multiple streams per
> > PF, so I wonder what scenario is being fixed?
>
> There is no way in the current upstream code to specify this TC so the
> only visible effect is that 2 devices under the same bridge can work
> now, previously the second device would fail to allocate a stream.
>
> > Lastly, are you expecting tsm.git#fixes to pick this up? I am
> > assuming that this goes through crypto.git and tsm.git can just stay
> > focused on core fixes.
>
> I was kinda hoping that Tom acks these (as he did) and you could take
> them. Thanks,
Ok, so can you refresh the changelog to call out the user visible
effects? Something like:
---
With SEV-TIO the low-level TSM driver is responsible for allocating a
Stream ID. The Stream ID needs to be unique within each IDE partner
port. Fix the Stream ID selection to reuse the host bridge stream
resource id which is a pool of 256 ids per host bridge on AMD platforms.
Otherwise, only one device per-host bridge can establish Selective
Stream IDE.
---
Send a v2, and I will pick it up.
^ permalink raw reply
* Re: [PATCH 2/2] KVM: SEV: Add support for IBPB-on-Entry
From: Nikunj A. Dadhania @ 2026-01-27 6:38 UTC (permalink / raw)
To: Kim Phillips, linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak, Tom Lendacky,
Michael Roth, Borislav Petkov, Borislav Petkov, Naveen Rao,
David Kaplan
In-Reply-To: <20260126224205.1442196-3-kim.phillips@amd.com>
On 1/27/2026 4:12 AM, Kim Phillips wrote:
> AMD EPYC 5th generation and above processors support IBPB-on-Entry
> for SNP guests. By invoking an Indirect Branch Prediction Barrier
> (IBPB) on VMRUN, old indirect branch predictions are prevented
> from influencing indirect branches within the guest.
>
> SNP guests may choose to enable IBPB-on-Entry by setting
> SEV_FEATURES bit 21 (IbpbOnEntry).
>
> Host support for IBPB on Entry is indicated by CPUID
> Fn8000_001F[IbpbOnEntry], bit 31.
>
> If supported, indicate support for IBPB on Entry in
> sev_supported_vmsa_features bit 23 (IbpbOnEntry).
>
> For more info, refer to page 615, Section 15.36.17 "Side-Channel
> Protection", AMD64 Architecture Programmer's Manual Volume 2: System
> Programming Part 2, Pub. 24593 Rev. 3.42 - March 2024 (see Link).
>
> Link: https://bugzilla.kernel.org/attachment.cgi?id=306250
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
> ---
> arch/x86/include/asm/cpufeatures.h | 1 +
> arch/x86/include/asm/svm.h | 1 +
> arch/x86/kvm/svm/sev.c | 9 ++++++++-
> 3 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
> index c01fdde465de..3ce5dff36f78 100644
> --- a/arch/x86/include/asm/cpufeatures.h
> +++ b/arch/x86/include/asm/cpufeatures.h
> @@ -459,6 +459,7 @@
> #define X86_FEATURE_ALLOWED_SEV_FEATURES (19*32+27) /* Allowed SEV Features */
> #define X86_FEATURE_SVSM (19*32+28) /* "svsm" SVSM present */
> #define X86_FEATURE_HV_INUSE_WR_ALLOWED (19*32+30) /* Allow Write to in-use hypervisor-owned pages */
> +#define X86_FEATURE_IBPB_ON_ENTRY (19*32+31) /* SEV-SNP IBPB on VM Entry */
>
> /* AMD-defined Extended Feature 2 EAX, CPUID level 0x80000021 (EAX), word 20 */
> #define X86_FEATURE_NO_NESTED_DATA_BP (20*32+ 0) /* No Nested Data Breakpoints */
> diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h
> index edde36097ddc..eebc65ec948f 100644
> --- a/arch/x86/include/asm/svm.h
> +++ b/arch/x86/include/asm/svm.h
> @@ -306,6 +306,7 @@ static_assert((X2AVIC_4K_MAX_PHYSICAL_ID & AVIC_PHYSICAL_MAX_INDEX_MASK) == X2AV
> #define SVM_SEV_FEAT_ALTERNATE_INJECTION BIT(4)
> #define SVM_SEV_FEAT_DEBUG_SWAP BIT(5)
> #define SVM_SEV_FEAT_SECURE_TSC BIT(9)
> +#define SVM_SEV_FEAT_IBPB_ON_ENTRY BIT(21)
>
> #define VMCB_ALLOWED_SEV_FEATURES_VALID BIT_ULL(63)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index ea515cf41168..8a6d25db0c00 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3165,8 +3165,15 @@ void __init sev_hardware_setup(void)
> cpu_feature_enabled(X86_FEATURE_NO_NESTED_DATA_BP))
> sev_supported_vmsa_features |= SVM_SEV_FEAT_DEBUG_SWAP;
>
> - if (sev_snp_enabled && tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> + if (!sev_snp_enabled)
> + return;
> + /* the following feature bit checks are SNP specific */
> +
The early return seems to split up the SNP features unnecessarily.
Keeping everything under `if (sev_snp_enabled)` is cleaner IMO -
it's clear that these features belong together. Plus, when
someone adds the next SNP feature, they won't have to think about
whether it goes before or after the return. The comment about
"SNP specific" features becomes redundant as well.
> + if (tsc_khz && cpu_feature_enabled(X86_FEATURE_SNP_SECURE_TSC))
> sev_supported_vmsa_features |= SVM_SEV_FEAT_SECURE_TSC;
> +
> + if (cpu_feature_enabled(X86_FEATURE_IBPB_ON_ENTRY))
> + sev_supported_vmsa_features |= SVM_SEV_FEAT_IBPB_ON_ENTRY;
Regards,
Nikunj
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Nikunj A. Dadhania @ 2026-01-27 6:19 UTC (permalink / raw)
To: Kim Phillips, linux-kernel, kvm, linux-coco, x86
Cc: Sean Christopherson, Paolo Bonzini, K Prateek Nayak, Tom Lendacky,
Michael Roth, Borislav Petkov, Borislav Petkov, Naveen Rao,
David Kaplan, stable
In-Reply-To: <20260126224205.1442196-2-kim.phillips@amd.com>
On 1/27/2026 4:12 AM, Kim Phillips wrote:
> KVM: SEV: IBPB-on-Entry guest support
The subject line should have the prefix "x86/sev" instead
of "KVM: SEV". The below subject line would be more appropriate:
x86/sev: Allow IBPB-on-Entry feature for SNP guests
> The SEV-SNP IBPB-on-Entry feature does not require a guest-side
> implementation. The feature was added in Zen5 h/w, after the first
> SNP Zen implementation, and thus was not accounted for when the
> initial set of SNP features were added to the kernel.
>
> In its abundant precaution, commit 8c29f0165405 ("x86/sev: Add SEV-SNP
> guest feature negotiation support") included SEV_STATUS' IBPB-on-Entry
> bit as a reserved bit, thereby masking guests from using the feature.
>
> Unmask the bit, to allow guests to take advantage of the feature on
> hypervisor kernel versions that support it: Amend the SEV_STATUS MSR
> SNP_RESERVED_MASK to exclude bit 23 (IbpbOnEntry).
>
> Fixes: 8c29f0165405 ("x86/sev: Add SEV-SNP guest feature negotiation support")> Cc: Nikunj A Dadhania <nikunj@amd.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> CC: Borislav Petkov (AMD) <bp@alien8.de>
> CC: Michael Roth <michael.roth@amd.com>
> Cc: stable@kernel.org
> Signed-off-by: Kim Phillips <kim.phillips@amd.com>
Apart from the above comments:
Reviewed-by: Nikunj A Dadhania <nikunj@amd.com>
> ---
> arch/x86/boot/compressed/sev.c | 1 +
> arch/x86/coco/sev/core.c | 1 +
> arch/x86/include/asm/msr-index.h | 5 ++++-
> 3 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/boot/compressed/sev.c b/arch/x86/boot/compressed/sev.c
> index c8c1464b3a56..2b639703b8dd 100644
> --- a/arch/x86/boot/compressed/sev.c
> +++ b/arch/x86/boot/compressed/sev.c
> @@ -188,6 +188,7 @@ bool sev_es_check_ghcb_fault(unsigned long address)
> MSR_AMD64_SNP_RESERVED_BIT13 | \
> MSR_AMD64_SNP_RESERVED_BIT15 | \
> MSR_AMD64_SNP_SECURE_AVIC | \
> + MSR_AMD64_SNP_RESERVED_BITS19_22 | \
> MSR_AMD64_SNP_RESERVED_MASK)
>
> #ifdef CONFIG_AMD_SECURE_AVIC
> diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
> index 9ae3b11754e6..13f608117411 100644
> --- a/arch/x86/coco/sev/core.c
> +++ b/arch/x86/coco/sev/core.c
> @@ -122,6 +122,7 @@ static const char * const sev_status_feat_names[] = {
> [MSR_AMD64_SNP_VMSA_REG_PROT_BIT] = "VMSARegProt",
> [MSR_AMD64_SNP_SMT_PROT_BIT] = "SMTProt",
> [MSR_AMD64_SNP_SECURE_AVIC_BIT] = "SecureAVIC",
> + [MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT] = "IBPBOnEntry",
> };
>
> /*
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index 4d3566bb1a93..9016a6b00bc7 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -735,7 +735,10 @@
> #define MSR_AMD64_SNP_SMT_PROT BIT_ULL(MSR_AMD64_SNP_SMT_PROT_BIT)
> #define MSR_AMD64_SNP_SECURE_AVIC_BIT 18
> #define MSR_AMD64_SNP_SECURE_AVIC BIT_ULL(MSR_AMD64_SNP_SECURE_AVIC_BIT)
> -#define MSR_AMD64_SNP_RESV_BIT 19
> +#define MSR_AMD64_SNP_RESERVED_BITS19_22 GENMASK_ULL(22, 19)
> +#define MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT 23
> +#define MSR_AMD64_SNP_IBPB_ON_ENTRY BIT_ULL(MSR_AMD64_SNP_IBPB_ON_ENTRY_BIT)
> +#define MSR_AMD64_SNP_RESV_BIT 24
> #define MSR_AMD64_SNP_RESERVED_MASK GENMASK_ULL(63, MSR_AMD64_SNP_RESV_BIT)
> #define MSR_AMD64_SAVIC_CONTROL 0xc0010138
> #define MSR_AMD64_SAVIC_EN_BIT 0
^ permalink raw reply
* Re: [PATCH v3 24/26] x86/virt/seamldr: Extend sigstruct to 16KB
From: Huang, Kai @ 2026-01-27 3:58 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Gao, Chao, x86@kernel.org
Cc: 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: <20260123145645.90444-25-chao.gao@intel.com>
On Fri, 2026-01-23 at 06:55 -0800, Gao, Chao wrote:
> Currently, each TDX Module has a 4KB sigstruct that is passed to the
> P-SEAMLDR during module updates to authenticate the TDX Module binary.
>
> Future TDX Module versions will pack additional information into the
> sigstruct, which will exceed the current 4KB size limit.
>
> To accommodate this, the sigstruct is being extended to support up to
> 16KB.
>
[...]
> Update seamldr_params and tdx-blob structures to handle the larger
> sigstruct size.
Nit: there's no update to 'struct tdx_blob' in this patch.
Btw, is there any spec/doc that mentions this publicly?
E.g., we do need some update to the TDX module blob doc, right?
^ 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