* 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
* Re: [PATCH v3 07/26] x86/virt/seamldr: Introduce a wrapper for P-SEAMLDR SEAMCALLs
From: Chao Gao @ 2026-01-30 8:08 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <04f9a748-fe61-439c-af80-59c498970580@intel.com>
On Wed, Jan 28, 2026 at 03:04:55PM -0800, Dave Hansen wrote:
>On 1/23/26 06:55, Chao Gao wrote:
>> SEAMRET from the P-SEAMLDR clears the current VMCS structure pointed
>> to by the current-VMCS pointer. A VMM that invokes the P-SEAMLDR
>> using SEAMCALL must reload the current-VMCS, if required, using the
>> VMPTRLD instruction.
>
>That seems pretty mean.
>
>This is going to need a lot more justification for why this is an
>absolutely necessary requirement.
AFAIK, this is a CPU implementation issue. The actual requirement is to
evict (flush and invalidate) all VMCSs __cached in SEAM mode__, but big
cores implement this by evicting the __entire__ VMCS cache. So, the
current VMCS is invalidated and cleared.
>
>KVM folks, are you OK with this?
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Xu Yilun @ 2026-01-30 4:01 UTC (permalink / raw)
To: Dave Hansen
Cc: Chao Gao, linux-coco, linux-kernel, kvm, x86, reinette.chatre,
ira.weiny, kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <b2e2fd5e-8aff-4eda-a648-9ae9f8234d25@intel.com>
> I'd also prefer a
>
> BUILD_BUG_ON(sizeof(struct seamldr_info) != 2048);
^
BUILD_BUG_ON(sizeof(struct seamldr_info) != 256); is it?
>
> just as a sanity check. It doesn't cost anything and it makes sure that
> as you muck around with reserved fields and padding that there's at
> least one check making sure it's OK.
And I recently received a comments that "never __packed for naturally
aligned structures cause it leads to bad generated code and hurts
performance", but I really want to highlight nearby it is for a
formatted binary blob, so:
struct seamldr_info {
u32 version;
u32 attributes;
u32 vendor_id;
u32 build_date;
u16 build_num;
u16 minor_version;
u16 major_version;
u16 update_version;
u8 reserved0[4];
u32 num_remaining_updates;
u8 reserved1[224];
}; //delete __packed here
static_assert(sizeof(struct seamldr_info) == 256);
Is it better?
^ permalink raw reply
* Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Sean Christopherson @ 2026-01-30 1:30 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Yan Zhao, Vishal Annapurve,
Ackerley Tng, Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-17-seanjc@google.com>
On Wed, Jan 28, 2026, Sean Christopherson wrote:
> +/*
> + * For SEAMCALLs that pass a bundle of pages, the TDX spec treats the registers
> + * like an array, as they are ordered in the struct. The effective array size
> + * is (obviously) limited by the number or registers, relative to the starting
> + * register. Fill the register array at a given starting register, with sanity
> + * checks to avoid overflowing the args structure.
> + */
> +static void dpamt_copy_regs_array(struct tdx_module_args *args, void *reg,
> + u64 *pamt_pa_array, bool copy_to_regs)
> +{
> + int size = tdx_dpamt_entry_pages() * sizeof(*pamt_pa_array);
> +
> + if (WARN_ON_ONCE(reg + size > (void *)args) + sizeof(*args))
The above closing ')' after args is misplaced, this should be
if (WARN_ON_ONCE(reg + size > (void *)args + sizeof(*args)))
I'm still in disbelief that I managed to end up with such a horrid bug that
compiled without any warnings. *sigh*
^ permalink raw reply
* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Sean Christopherson @ 2026-01-30 1:28 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Kai Huang, ackerleytng@google.com,
sagis@google.com, Vishal Annapurve, linux-kernel@vger.kernel.org,
Yan Y Zhao, Xiaoyao Li, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Isaku Yamahata,
binbin.wu@linux.intel.com
In-Reply-To: <f9f65b0fad57db12e21d2168d02bac036615fb7f.camel@intel.com>
On Thu, Jan 29, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > Define kvm_x86_ops .link_external_spt(), .set_external_spte(), and
> > .free_external_spt() as RET0 static calls so that an unexpected call to a
> > a default operation doesn't consume garbage.
> >
> > Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> > Fixes: 94faba8999b9 ("KVM: x86/tdp_mmu: Propagate tearing down mirror page tables")
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
>
> We don't want to crash unnecessarily, but do we want to get some sort of notice?
Hmm, that's probably doable, but definitely in a separate patch. E.g. something
like:
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 6083fb07cd3b..270149f84bb4 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -3,6 +3,13 @@
BUILD_BUG_ON(1)
#endif
+#ifndef KVM_X86_OP_OPTIONAL
+#define KVM_X86_OP_OPTIONAL KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_WARN KVM_X86_OP
+#define KVM_X86_OP_OPTIONAL_RET0_WARN KVM_X86_OP
+#endif
+
/*
* KVM_X86_OP() and KVM_X86_OP_OPTIONAL() are used to help generate
* both DECLARE/DEFINE_STATIC_CALL() invocations and
@@ -94,11 +101,11 @@ KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
KVM_X86_OP(load_mmu_pgd)
-KVM_X86_OP_OPTIONAL(alloc_external_sp)
-KVM_X86_OP_OPTIONAL(free_external_sp)
-KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
-KVM_X86_OP_OPTIONAL(reclaim_external_sp)
-KVM_X86_OP_OPTIONAL_RET0(topup_external_cache)
+KVM_X86_OP_OPTIONAL_WARN(alloc_external_sp)
+KVM_X86_OP_OPTIONAL_WARN(free_external_sp)
+KVM_X86_OP_OPTIONAL_RET0_WARN(set_external_spte)
+KVM_X86_OP_OPTIONAL_WARN(reclaim_external_sp)
+KVM_X86_OP_OPTIONAL_RET0_WARN(topup_external_cache)
KVM_X86_OP(has_wbinvd_exit)
KVM_X86_OP(get_l2_tsc_offset)
KVM_X86_OP(get_l2_tsc_multiplier)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index cd3e7dc6ab9b..663c9943c0dd 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2004,8 +2004,6 @@ extern struct kvm_x86_ops kvm_x86_ops;
#define KVM_X86_OP(func) \
DECLARE_STATIC_CALL(kvm_x86_##func, *(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_OPTIONAL KVM_X86_OP
-#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
#include <asm/kvm-x86-ops.h>
int kvm_x86_vendor_init(struct kvm_x86_init_ops *ops);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index c3d71ba9a1dc..1748f44c81c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -143,8 +143,6 @@ struct kvm_x86_ops kvm_x86_ops __read_mostly;
#define KVM_X86_OP(func) \
DEFINE_STATIC_CALL_NULL(kvm_x86_##func, \
*(((struct kvm_x86_ops *)0)->func));
-#define KVM_X86_OP_OPTIONAL KVM_X86_OP
-#define KVM_X86_OP_OPTIONAL_RET0 KVM_X86_OP
#include <asm/kvm-x86-ops.h>
EXPORT_STATIC_CALL_GPL(kvm_x86_get_cs_db_l_bits);
EXPORT_STATIC_CALL_GPL(kvm_x86_cache_reg);
@@ -9965,6 +9963,17 @@ static struct notifier_block pvclock_gtod_notifier = {
};
#endif
+static void kvm_static_call_warn(void)
+{
+ WARN_ON_ONCE(1);
+}
+
+static long kvm_static_call_warn_return0(void)
+{
+ WARN_ON_ONCE(1);
+ return 0;
+}
+
static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
{
memcpy(&kvm_x86_ops, ops->runtime_ops, sizeof(kvm_x86_ops));
@@ -9977,6 +9986,12 @@ static inline void kvm_ops_update(struct kvm_x86_init_ops *ops)
#define KVM_X86_OP_OPTIONAL_RET0(func) \
static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
(void *)__static_call_return0);
+#define KVM_X86_OP_OPTIONAL_WARN(func) \
+ static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
+ (void *)kvm_static_call_warn);
+#define KVM_X86_OP_OPTIONAL_RET0_WARN(func) \
+ static_call_update(kvm_x86_##func, (void *)kvm_x86_ops.func ? : \
+ (void *)kvm_static_call_warn_return0);
#include <asm/kvm-x86-ops.h>
#undef __KVM_X86_OP
^ permalink raw reply related
* Re: [RFC PATCH v5 11/45] x86/tdx: Add helpers to check return status codes
From: Edgecombe, Rick P @ 2026-01-30 0:36 UTC (permalink / raw)
To: Hansen, Dave, seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, dave.hansen@linux.intel.com,
kas@kernel.org, binbin.wu@linux.intel.com, mingo@redhat.com,
pbonzini@redhat.com, ackerleytng@google.com,
linux-kernel@vger.kernel.org, Yamahata, Isaku, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aXvEgD69vDTPj4z5@google.com>
On Thu, 2026-01-29 at 12:35 -0800, Sean Christopherson wrote:
> On Thu, Jan 29, 2026, Dave Hansen wrote:
> > On 1/28/26 17:14, Sean Christopherson wrote:
> > ...
> > > err = tdh_mng_vpflushdone(&kvm_tdx->td);
> > > - if (err == TDX_FLUSHVP_NOT_DONE)
> > > + if (IS_TDX_FLUSHVP_NOT_DONE(err))
> > > goto out;
> > > if (TDX_BUG_ON(err, TDH_MNG_VPFLUSHDONE, kvm)) {
> >
> > I really despise the non-csopeable, non-ctaggable, non-greppable names
> > like this. Sometimes it's unavoidable. Is it really unavoidable here?
> >
> > Something like this is succinct enough and doesn't have any magic ##
> > macro definitions:
> >
> > TDX_ERR_EQ(err, TDX_FLUSHVP_NOT_DONE)
I like the editor friendliness. The only downside is that it puts the onus on
the caller to make sure supported defines are passed into TDX_ERR_EQ(). Today
there are a few special cases like IS_TDX_NON_RECOVERABLE().
I don't know, I'm ok either way. I lean towards keeping it as in this patch
because we already had an error code bit interpretation bug:
https://lore.kernel.org/kvm/24d2f165-f854-4996-89cf-28d644c592a3@intel.com/
So the centralization of bit interpretation seems like a real win.
>
> FWIW, I have zero preference on this. I included the patch purely because it was
> already there.
Ha, actually we all had a long thread on this:
https://lore.kernel.org/kvm/70484aa1b553ca250d893f80b2687b5d915e5309.camel@intel.com/
I see now that we closed it with you but never got Dave's final buy in.
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Edgecombe, Rick P @ 2026-01-29 22:48 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, dave.hansen@linux.intel.com,
linux-kernel@vger.kernel.org, kas@kernel.org, mingo@redhat.com,
pbonzini@redhat.com, binbin.wu@linux.intel.com,
ackerleytng@google.com, Yamahata, Isaku, sagis@google.com,
tglx@kernel.org, bp@alien8.de, Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aXvd2z8HmLFd50io@google.com>
On Thu, 2026-01-29 at 14:23 -0800, Sean Christopherson wrote:
> No, the bug is if the cmpxchg64 fails. On failure, the current mismatching value
> is stored in the "old" param. KVM relies on the iter->old_spte holding the
> current value when restarting an operation without re-reading the SPTE from memory.
Ah, I see. Sorry. Just went and refreshed up on the difference between
cmpxchg64() and try_cmpxchg64(). I see now that the log is accurate since it
refers to the behavior of the instruction, but specifying try_cmpxchg64() might
be a little clearer since cmpxchg() doesn't automatically update the 'old'
passed in. In either case:
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Kim Phillips @ 2026-01-29 22:32 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, kvm, linux-coco, x86, Sean Christopherson,
Paolo Bonzini, K Prateek Nayak, Nikunj A Dadhania, Tom Lendacky,
Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <20260129105116.GBaXs7pBF-k4x_5_W1@fat_crate.local>
On 1/29/26 4:51 AM, Borislav Petkov wrote:
> On Wed, Jan 28, 2026 at 06:38:29PM -0600, Kim Phillips wrote:
>> SNP_FEATURES_PRESENT is for the non-trivial variety: Its bits get set as
>> part of the patchseries that add the explicit guest support *code*.
> Yes, and I'm asking why can't SNP_FEATURES_PRESENT contain *all* SNP features?
Not *all* SNP features are implemented in all guest kernel versions, and,
well, for those that don't require explicit guest code support, perhaps it's
because they aren't necessarily well defined and validated in all hardware
versions...
Kim
^ permalink raw reply
* Re: [PATCH kernel 2/2] crypto/ccp: Allow multiple streams on the same root bridge
From: Alexey Kardashevskiy @ 2026-01-29 22:29 UTC (permalink / raw)
To: dan.j.williams, linux-crypto
Cc: linux-kernel, Ashish Kalra, Tom Lendacky, John Allen, Herbert Xu,
David S. Miller, x86, linux-coco, Pratik R . Sampat
In-Reply-To: <6978626886a50_1d33100c1@dwillia2-mobl4.notmuch>
On 27/1/26 17:59, dan.j.williams@intel.com wrote:
> Alexey Kardashevskiy wrote:
>> On 24/1/26 09:59, dan.j.williams@intel.com wrote:
>>> Alexey Kardashevskiy wrote:
>>>> IDE stream IDs are responsibility of a platform and in some cases
>>>> TSM allocates the numbers. AMD SEV TIO though leaves it to the host
>>>> OS. Mistakenly stream ID is hard coded to be the same as a traffic
>>>> class.
>>>
>>> I scratched my head at this comment, but now realize that you are
>>> saying the existing code used the local @tc, not that the hardware
>>> stream ID is in any way related to traffic class, right?
>>
>> When I did that in the first place, I also wanted to try different
>> traffic classes so I just took a shortcut here.
>>
>>> It would help to detail what the end user visible effects of this
>>> bug are. The TSM framework does not allow for multiple streams per
>>> PF, so I wonder what scenario is being fixed?
>>
>> There is no way in the current upstream code to specify this TC so the
>> only visible effect is that 2 devices under the same bridge can work
>> now, previously the second device would fail to allocate a stream.
>>
>>> Lastly, are you expecting tsm.git#fixes to pick this up? I am
>>> assuming that this goes through crypto.git and tsm.git can just stay
>>> focused on core fixes.
>>
>> I was kinda hoping that Tom acks these (as he did) and you could take
>> them. Thanks,
>
> Ok, so can you refresh the changelog to call out the user visible
> effects? Something like:
>
> ---
> With SEV-TIO the low-level TSM driver is responsible for allocating a
> Stream ID. The Stream ID needs to be unique within each IDE partner
> port. Fix the Stream ID selection to reuse the host bridge stream
> resource id which is a pool of 256 ids per host bridge on AMD platforms.
> Otherwise, only one device per-host bridge can establish Selective
> Stream IDE.
> ---
>
> Send a v2, and I will pick it up.
Please squash it in the v1, if possible.
Acked-by: Alexey Kardashevskiy <aik@amd.com>
thanks!
ps sorry missed that on time, I do suck at multitasking :(
--
Alexey
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Sean Christopherson @ 2026-01-29 22:23 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: x86@kernel.org, dave.hansen@linux.intel.com, kas@kernel.org,
bp@alien8.de, mingo@redhat.com, pbonzini@redhat.com,
tglx@kernel.org, Kai Huang, ackerleytng@google.com,
sagis@google.com, Vishal Annapurve, linux-kernel@vger.kernel.org,
Yan Y Zhao, Xiaoyao Li, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Isaku Yamahata,
binbin.wu@linux.intel.com
In-Reply-To: <fbaeb0d2f4658efd4c7bb61ac0ba2919c8226a36.camel@intel.com>
On Thu, Jan 29, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> > Pass a pointer to iter->old_spte, not simply its value, when setting an
> > external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> > will be updated if the cmpxchg64 to freeze the mirror SPTE fails.
> >
>
> Might be being dense here, but is the bug that if cmpxchg64 *succeeds* and
> set_external_spte() fails? Then old_spte is not updated and the local retry will
> expect the wrong old_spte.
No, the bug is if the cmpxchg64 fails. On failure, the current mismatching value
is stored in the "old" param. KVM relies on the iter->old_spte holding the
current value when restarting an operation without re-reading the SPTE from memory.
E.g. in __tdp_mmu_zap_root(), if tdp_mmu_set_spte_atomic() fails, iter->old_spte
*must* hold the current in-memroy value, otherwise the loop will hang because it
will re-attempt cmpxchg64 using the stale iter->old_spte.
static void __tdp_mmu_zap_root(struct kvm *kvm, struct kvm_mmu_page *root,
bool shared, int zap_level)
{
struct tdp_iter iter;
for_each_tdp_pte_min_level_all(iter, root, zap_level) {
retry:
if (tdp_mmu_iter_cond_resched(kvm, &iter, false, shared))
continue;
if (!is_shadow_present_pte(iter.old_spte))
continue;
if (iter.level > zap_level)
continue;
if (!shared)
tdp_mmu_iter_set_spte(kvm, &iter, SHADOW_NONPRESENT_VALUE);
else if (tdp_mmu_set_spte_atomic(kvm, &iter, SHADOW_NONPRESENT_VALUE))
goto retry;
}
}
> > The bug
> > is currently benign as TDX is mutualy exclusive with all paths that do
> > "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
>
>
^ permalink raw reply
* Re: [PATCH v1 08/14] x86: make CONFIG_EFI_STUB unconditional
From: H. Peter Anvin @ 2026-01-29 22:22 UTC (permalink / raw)
To: Simon Glass
Cc: ubizjak, linux-kernel, akpm, bp, dave.hansen, kas, kees,
linux-coco, mingo, nathan, peterz, pmladek, rick.p.edgecombe,
tglx, x86
In-Reply-To: <CAFLszTiQ_BAMGJPJfAV4TXiELvZn12+wX2txBuzMcQ_nw2kzOg@mail.gmail.com>
On January 29, 2026 2:13:13 PM PST, Simon Glass <sjg@chromium.org> wrote:
>Hi Peter,
>
>On Tue, 27 Jan 2026 at 16:22, H. Peter Anvin <hpa@zytor.com> wrote:
>>
>> On January 26, 2026 7:14:27 PM PST, Simon Glass <sjg@chromium.org> wrote:
>> >Hi Peter,
>> >
>> >On Tue, 27 Jan 2026 at 15:55, H. Peter Anvin <hpa@zytor.com> wrote:
>> >>
>> >> On January 26, 2026 5:44:43 PM PST, Simon Glass <sjg@chromium.org> wrote:
>> >> >Hi Peter,
>> >> >
>> >> >On Tue, 27 Jan 2026 at 11:21, H. Peter Anvin <hpa@zytor.com> wrote:
>> >> >>
>> >> >> On 2026-01-26 13:19, Simon Glass wrote:
>> >> >> >>
>> >> >> >> Including the EFI stub doesn't mean using EFI to boot is required.
>> >> >> >
>> >> >> > Yes, understood, but it adds bloat. More importantly it will lead to
>> >> >> > people assuming that the stub is always used and thus unwittingly blur
>> >> >> > the boundary between the stub and the kernel itself.
>> >> >> >
>> >> >> > What is the actual need for this?
>> >> >> >
>> >> >>
>> >> >> I would argue that the opposite is more likely: someone inadvertently builds a
>> >> >> kernel without the stub, the bootloader goes down a legacy support path and
>> >> >> things seems to work... except for some platform subtleties.
>> >> >>
>> >> >> The bloat is there, but it is small and is only in the on-disk kernel image;
>> >> >> it is zero at runtime.
>> >> >>
>> >> >> As such, I don't think this option is a particularly good idea anymore. If
>> >> >> necessary, it could be hidden behind an EXPERT option, but I first wanted to
>> >> >> see who if anyone actually cares in a meaningful way to maintain this option.
>> >> >> Every option, after all, adds maintenance burden.
>> >> >>
>> >> >> Note that the BIOS stub is unconditionally compiled and included, and that has
>> >> >> not been an issue.
>> >> >
>> >> >What is the maintenance burden here? I could potentially take that on,
>> >> >but I would first want to understand what is involved.
>> >> >
>> >> >The use of the word 'legacy' worries me too. Is this patch a step
>> >> >towards removing the non-EFI path?
>> >> >
>> >> >Regards,
>> >> >Simon
>> >
>> >(joining the threads)
>> >
>> >> Bypassing the firmware stub (BIOS or EFI) is really only appropriate for special user cases, like kexec, because it removes the ability for the kernel to deal with system issues at an early point.
>> >
>> >Does this dealing happen in the EFI stub, or later? Are you referring
>> >to ACPI fix-ups or something else?
>> >
>> >>
>> >> However, kexec needs it, and it's not going to go away. However, that doesn't mean we should encourage this in cases which doesn't need it (no matter what the Grub maintainers tell you.)
>> >>
>> >> Now, if you are using KVM without EFI you are probably doing BIOS boot (regardless of if you know it or not), entering via the BIOS firmware stub.
>> >
>> >I am thinking of the 64-bit entry point to the kernel. everything
>> >being laid out in memory ready to go.
>> >
>> >>
>> >> I just realized you are the U-boot maintainer, so I'm assuming you are thinking of the case where there is no UEFI or BIOS firmware. In that case, just like as for kexec, the current entry point will continue to work, of course.
>> >>
>> >> What we don't want is having to suffer being on a BIOS or EFI system but not being able to leverage it for the benefit of the kernel. The kernel image is much easier to upgrade.
>> >
>> >More generally I am thinking about a simple and clean API for the
>> >kernel that doesn't involve having to provide 30K lines of firmware
>> >code just to boot. The BIOS entry point (if that is what it is called)
>> >is quite close to this ideal, even though I know it has shortcomings.
>> >
>> >Regards,
>> >Simon
>>
>> Yes, I understand.
>>
>> The 32/64-bit entrypoints aren't going away; it would be impossible to do so.
>>
>
>OK, so 'depend on EXPORT' seems good to me.
>
>> The general rule is: do things as late in the boot process as possible, but no later.
>>
>
>Just on this point, I wonder how we should define 'late', in the
>context of EFI. For example, if the kernel stub reads a file, meaning
>it calls back into Tianocore, it is using both early and late code.
>
>Regards,
>Simon
"Early" in this context is before ExitBootServices().
^ permalink raw reply
* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Edgecombe, Rick P @ 2026-01-29 22:20 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Huang, Kai, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-5-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Define kvm_x86_ops .link_external_spt(), .set_external_spte(), and
> .free_external_spt() as RET0 static calls so that an unexpected call to a
> a default operation doesn't consume garbage.
>
> Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> Fixes: 94faba8999b9 ("KVM: x86/tdp_mmu: Propagate tearing down mirror page tables")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
We don't want to crash unnecessarily, but do we want to get some sort of notice?
^ permalink raw reply
* Re: [RFC PATCH v5 03/45] KVM: TDX: Account all non-transient page allocations for per-TD structures
From: Edgecombe, Rick P @ 2026-01-29 22:15 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Huang, Kai, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-4-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Account all non-transient allocations associated with a single TD (or its
> vCPUs), as KVM's ABI is that allocations that are active for the lifetime
> of a VM are accounted. Leave temporary allocations, i.e. allocations that
> are freed within a single function/ioctl, unaccounted, to again align with
> KVM's existing behavior, e.g. see commit dd103407ca31 ("KVM: X86: Remove
> unnecessary GFP_KERNEL_ACCOUNT for temporary variables").
>
> Fixes: 8d032b683c29 ("KVM: TDX: create/destroy VM structure")
> Fixes: a50f673f25e0 ("KVM: TDX: Do TDX specific vcpu initialization")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
^ permalink raw reply
* Re: [PATCH v1 08/14] x86: make CONFIG_EFI_STUB unconditional
From: Simon Glass @ 2026-01-29 22:13 UTC (permalink / raw)
To: H. Peter Anvin
Cc: ubizjak, linux-kernel, akpm, bp, dave.hansen, kas, kees,
linux-coco, mingo, nathan, peterz, pmladek, rick.p.edgecombe,
tglx, x86
In-Reply-To: <36C4A641-A124-4082-884D-90AE22B10275@zytor.com>
Hi Peter,
On Tue, 27 Jan 2026 at 16:22, H. Peter Anvin <hpa@zytor.com> wrote:
>
> On January 26, 2026 7:14:27 PM PST, Simon Glass <sjg@chromium.org> wrote:
> >Hi Peter,
> >
> >On Tue, 27 Jan 2026 at 15:55, H. Peter Anvin <hpa@zytor.com> wrote:
> >>
> >> On January 26, 2026 5:44:43 PM PST, Simon Glass <sjg@chromium.org> wrote:
> >> >Hi Peter,
> >> >
> >> >On Tue, 27 Jan 2026 at 11:21, H. Peter Anvin <hpa@zytor.com> wrote:
> >> >>
> >> >> On 2026-01-26 13:19, Simon Glass wrote:
> >> >> >>
> >> >> >> Including the EFI stub doesn't mean using EFI to boot is required.
> >> >> >
> >> >> > Yes, understood, but it adds bloat. More importantly it will lead to
> >> >> > people assuming that the stub is always used and thus unwittingly blur
> >> >> > the boundary between the stub and the kernel itself.
> >> >> >
> >> >> > What is the actual need for this?
> >> >> >
> >> >>
> >> >> I would argue that the opposite is more likely: someone inadvertently builds a
> >> >> kernel without the stub, the bootloader goes down a legacy support path and
> >> >> things seems to work... except for some platform subtleties.
> >> >>
> >> >> The bloat is there, but it is small and is only in the on-disk kernel image;
> >> >> it is zero at runtime.
> >> >>
> >> >> As such, I don't think this option is a particularly good idea anymore. If
> >> >> necessary, it could be hidden behind an EXPERT option, but I first wanted to
> >> >> see who if anyone actually cares in a meaningful way to maintain this option.
> >> >> Every option, after all, adds maintenance burden.
> >> >>
> >> >> Note that the BIOS stub is unconditionally compiled and included, and that has
> >> >> not been an issue.
> >> >
> >> >What is the maintenance burden here? I could potentially take that on,
> >> >but I would first want to understand what is involved.
> >> >
> >> >The use of the word 'legacy' worries me too. Is this patch a step
> >> >towards removing the non-EFI path?
> >> >
> >> >Regards,
> >> >Simon
> >
> >(joining the threads)
> >
> >> Bypassing the firmware stub (BIOS or EFI) is really only appropriate for special user cases, like kexec, because it removes the ability for the kernel to deal with system issues at an early point.
> >
> >Does this dealing happen in the EFI stub, or later? Are you referring
> >to ACPI fix-ups or something else?
> >
> >>
> >> However, kexec needs it, and it's not going to go away. However, that doesn't mean we should encourage this in cases which doesn't need it (no matter what the Grub maintainers tell you.)
> >>
> >> Now, if you are using KVM without EFI you are probably doing BIOS boot (regardless of if you know it or not), entering via the BIOS firmware stub.
> >
> >I am thinking of the 64-bit entry point to the kernel. everything
> >being laid out in memory ready to go.
> >
> >>
> >> I just realized you are the U-boot maintainer, so I'm assuming you are thinking of the case where there is no UEFI or BIOS firmware. In that case, just like as for kexec, the current entry point will continue to work, of course.
> >>
> >> What we don't want is having to suffer being on a BIOS or EFI system but not being able to leverage it for the benefit of the kernel. The kernel image is much easier to upgrade.
> >
> >More generally I am thinking about a simple and clean API for the
> >kernel that doesn't involve having to provide 30K lines of firmware
> >code just to boot. The BIOS entry point (if that is what it is called)
> >is quite close to this ideal, even though I know it has shortcomings.
> >
> >Regards,
> >Simon
>
> Yes, I understand.
>
> The 32/64-bit entrypoints aren't going away; it would be impossible to do so.
>
OK, so 'depend on EXPORT' seems good to me.
> The general rule is: do things as late in the boot process as possible, but no later.
>
Just on this point, I wonder how we should define 'late', in the
context of EFI. For example, if the kernel stub reads a file, meaning
it calls back into Tianocore, it is using both early and late code.
Regards,
Simon
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Edgecombe, Rick P @ 2026-01-29 22:10 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Huang, Kai, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-3-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Pass a pointer to iter->old_spte, not simply its value, when setting an
> external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> will be updated if the cmpxchg64 to freeze the mirror SPTE fails.
>
Might be being dense here, but is the bug that if cmpxchg64 *succeeds* and
set_external_spte() fails? Then old_spte is not updated and the local retry will
expect the wrong old_spte.
> The bug
> is currently benign as TDX is mutualy exclusive with all paths that do
> "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
^ permalink raw reply
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