* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: H. Peter Anvin @ 2026-04-01 17:42 UTC (permalink / raw)
To: Dave Hansen, Sean Christopherson, Rick P Edgecombe
Cc: Vishal L Verma, Kai Huang, bp@alien8.de, x86@kernel.org,
kas@kernel.org, mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <830f1e46-0fb7-4756-827b-c8f46af24374@intel.com>
On April 1, 2026 8:03:02 AM PDT, Dave Hansen <dave.hansen@intel.com> wrote:
>On 3/31/26 16:04, Sean Christopherson wrote:
>> But unless the WBINVD is actually costly, why bother getting fancy?
>
>WBINVD might be the most expensive single instruction in the whole ISA.
>
>That said, I'd much rather have a potentially unnecessary WBINVD than
>miss one. The thing I'd be worried about would be something wonky like:
>
> 1. CPU offline does WBINVD
> 2. Some other TDX call gets made, dirties caches again
> 3. tdx_offline_cpu() skips WBINVD
>
>So, let's just do both for now: Do WBINVD in tdx_offline_cpu() and
>comment that it might be redundant with other things in the CPU offline
>procedure.
>
>This really needs to be solved with infrastructure and keeping data
>about the reasons for needing WBINVD, not relying on code ordering or
>fragile semantics.
It is, *by far*, the most expensive *uninterruptible* instruction in the ISA. REP string instructions can of course be arbitrarily long, but are interruptible and so don't really count.
Some MSRs used during very early (pre-OS) initialization might be even slower on some implementations, but that's not visible to Linux and no workload of any kind is running.
^ permalink raw reply
* Re: [PATCH v3 3/6] x86/sev: Add support to perform RMP optimizations asynchronously
From: Dave Hansen @ 2026-04-01 16:10 UTC (permalink / raw)
To: Kalra, Ashish, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <a1a5fa97-b067-4145-a4dc-fbb04fe9c720@amd.com>
On 4/1/26 08:47, Kalra, Ashish wrote:
> For programming the RMPOPT_BASE MSR performance is not really that
> important as it is for issuing the RMPOPT instruction on only thread
> per core, and as we are programming the RMPOPT_BASE MSRs on all CPUs/
> threads to the same (starting) physical address to support all RAM
> up-to 2TB for RMP optimizations, therefore, i don't think it is that
> critical to implement wrmsrq_on_cpumask() and instead we can
> continue to program the RMPOPT_BASE MSR on all CPUs (threads).
I don't mean to be a grammar pedant. But, man, that's hard to parse when
written as a single sentence.
I'm also not quite sure what the resistance is to going and adding the
precise function that is needed:
int wrmsrq_on_cpus(const struct cpumask *mask, u32 msr_no, u64 q)
{
int err;
struct msr_info rv;
memset(&rv, 0, sizeof(rv));
rv.msr_no = msr_no;
rv.reg.q = q;
err = smp_call_function_many(mask, __wrmsr_on_cpu, &rv, 1);
return err;
}
EXPORT_SYMBOL(wrmsrq_on_cpus);
It's just wrmsrq_on_cpu(), replace the 'cpu' with a cpumask and
s/_single/_many/. I think. Unless I'm missing something.
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Accept hotplugged memory before online
From: Edgecombe, Rick P @ 2026-04-01 15:49 UTC (permalink / raw)
To: pbonzini@redhat.com, prsampat@amd.com
Cc: dave.hansen@linux.intel.com, marcandre.lureau@redhat.com,
kas@kernel.org, bp@alien8.de, linux-kernel@vger.kernel.org,
mingo@redhat.com, x86@kernel.org, Qiang, Chenyi, tglx@kernel.org,
hpa@zytor.com, kvm@vger.kernel.org, linux-coco@lists.linux.dev
In-Reply-To: <49e8b24d836c1883e83ad72d1ab279f9e3eb7455.camel@intel.com>
On Wed, 2026-04-01 at 08:39 -0700, Edgecombe, Rick P wrote:
> > Would it not be better to have this information in the unaccepted
> > bitmap which we could explicitly query to accept/unaccept?
>
> It makes me think about shared memory too. Should the unplug event
> also signal the host to reset the memory to private? If the VMM is
> actually not adjusting the guest mapping for a unplug/re-plug then
> the memory would come back as shared.
>
> But it really starts to feel like work the host should be doing.
Although if memory was able to be unplugged, the Linux guest would have
reset the memory to private when it was done with the memory. But from
a interface perspective, it seems weird to require the guest to signal
this because private/shared state is ultimately controlled by the host.
Unlike accept state where the guest gets some say.
^ permalink raw reply
* Re: [PATCH v3 3/6] x86/sev: Add support to perform RMP optimizations asynchronously
From: Kalra, Ashish @ 2026-04-01 15:47 UTC (permalink / raw)
To: Dave Hansen, tglx, mingo, bp, dave.hansen, x86, hpa, seanjc,
peterz, thomas.lendacky, herbert, davem, ardb
Cc: pbonzini, aik, Michael.Roth, KPrateek.Nayak, Tycho.Andersen,
Nathan.Fontenot, jackyli, pgonda, rientjes, jacobhxu, xin,
pawan.kumar.gupta, babu.moger, dyoung, nikunj, john.allen, darwi,
linux-kernel, linux-crypto, kvm, linux-coco
In-Reply-To: <e2e38a91-ef8c-41d0-9373-0fdb6f246847@amd.com>
Hello Dave,
On 3/30/2026 7:46 PM, Kalra, Ashish wrote:
>
> On 3/30/2026 6:22 PM, Dave Hansen wrote:
>>
>>> static __init void configure_and_enable_rmpopt(void)
>>> {
>>> phys_addr_t pa_start = ALIGN_DOWN(PFN_PHYS(min_low_pfn), SZ_1G);
>>> @@ -499,6 +582,37 @@ static __init void configure_and_enable_rmpopt(void)
>>> */
>>> for_each_online_cpu(cpu)
>>> wrmsrq_on_cpu(cpu, MSR_AMD64_RMPOPT_BASE, rmpopt_base);
>>
>> What is the scope of MSR_AMD64_RMPOPT_BASE? Can you have it enabled on
>> one thread and not the other? Could they be different values both for
>> enabling and the rmpopt_base value?
>>
>> If it's not per-thread, then why is it being initialized for each thread?
>>
>
> Only one logical thread per core needs to set RMPOPT_BASE MSR as it is per-core,
> so i will use the "primary_threads_cpumask" here to use it for programming this
> MSR.
>
> Just another reason, to set the "primary_threads_cpumask" here in this function
> and then re-use it for the RMPOPT worker.
>
Coming back to this ...
For using the "primary_thread_cpumask" i will need to use something like
on_each_cpu_mask() similar to what i was doing in v2.
In v2, i was programming the RMPOPT_BASE MSR using on_each_cpu_mask(),
that required using a callback function to do the WRMSR:
+static void __configure_rmpopt(void *val)
+{
+ u64 rmpopt_base = ((u64)val & PUD_MASK) | MSR_AMD64_RMPOPT_ENABLE;
+
+ wrmsrq(MSR_AMD64_RMPOPT_BASE, rmpopt_base);
+}
+
+ on_each_cpu_mask(cpu_online_mask, __configure_rmpopt, (void *)pa_start, true);
But, that required using the (void *) casting, which you objected to and you
suggested the use of for_each_online_cpu() and wrmsrq_on_cpu(), and i has replied
that i need to do it (only) once on each thread per core, and that's why i may need
to use on_each_cpu_mask() and then you had suggested that if you *need* performance
then i can implement/add something like wrmsrq_on_cpumask().
For programming the RMPOPT_BASE MSR performance is not really that important as
it is for issuing the RMPOPT instruction on only thread per core, and as we are
programming the RMPOPT_BASE MSRs on all CPUs/threads to the same (starting) physical
address to support all RAM up-to 2TB for RMP optimizations, therefore, i don't
think it is that critical to implement wrmsrq_on_cpumask() and instead we can continue
to program the RMPOPT_BASE MSR on all CPUs (threads).
Thanks,
Ashish
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Accept hotplugged memory before online
From: Edgecombe, Rick P @ 2026-04-01 15:37 UTC (permalink / raw)
To: pbonzini@redhat.com, prsampat@amd.com
Cc: dave.hansen@linux.intel.com, marcandre.lureau@redhat.com,
kas@kernel.org, bp@alien8.de, linux-kernel@vger.kernel.org,
mingo@redhat.com, x86@kernel.org, Qiang, Chenyi, tglx@kernel.org,
hpa@zytor.com, kvm@vger.kernel.org, linux-coco@lists.linux.dev
In-Reply-To: <cab5371d-e0f4-42b7-bae9-2c7f981b26b2@amd.com>
On Mon, 2026-03-30 at 11:10 -0400, Pratik R. Sampat wrote:
> SNP likely has an analogous issue too.
> Failing to switch states on remove will cause that RMP entry to
> remain validated. A malicious hypervisor could then remap this GPA to
> another HPA which would put this in the Guest-Invalid state. On re-
> hotplug if we ignore errors suggested by Patch 1 (in our case that'd
> be PVALIDATE_FAIL_NOUPDATE error likely), we could have two RMP
> entries for the same GPA and both being validated. This is dangerous
> because hypervisor could swap these at will.
Oh, I was just wondering if we could just zero the page on accept
failure for the case of already accepted. Handle the issue internally
and actually go back to something like patch 1. Will it work for SNP?
>
> Would it not be better to have this information in the unaccepted
> bitmap which we could explicitly query to accept/unaccept?
It makes me think about shared memory too. Should the unplug event also
signal the host to reset the memory to private? If the VMM is actually
not adjusting the guest mapping for a unplug/re-plug then the memory
would come back as shared.
But it really starts to feel like work the host should be doing.
>
> For ACPI hardware-style hotplug I was working with the UEFI side on a
> POC to reflect SRAT hotplug windows in UEFI_UNACCEPTED_MEMORY using
> EFI_MEMORY_HOT_PLUGGABLE attribute and working to modify that spec.
> I’m less sure what this description for virtio-mem would look like
> and if it'd be possible to do this early-boot.
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Dave Hansen @ 2026-04-01 15:03 UTC (permalink / raw)
To: Sean Christopherson, Rick P Edgecombe
Cc: Vishal L Verma, Kai Huang, bp@alien8.de, x86@kernel.org,
kas@kernel.org, hpa@zytor.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, dave.hansen@linux.intel.com,
tglx@kernel.org, pbonzini@redhat.com, linux-coco@lists.linux.dev,
kvm@vger.kernel.org
In-Reply-To: <acxTBwaw6_xYSShf@google.com>
On 3/31/26 16:04, Sean Christopherson wrote:
> But unless the WBINVD is actually costly, why bother getting fancy?
WBINVD might be the most expensive single instruction in the whole ISA.
That said, I'd much rather have a potentially unnecessary WBINVD than
miss one. The thing I'd be worried about would be something wonky like:
1. CPU offline does WBINVD
2. Some other TDX call gets made, dirties caches again
3. tdx_offline_cpu() skips WBINVD
So, let's just do both for now: Do WBINVD in tdx_offline_cpu() and
comment that it might be redundant with other things in the CPU offline
procedure.
This really needs to be solved with infrastructure and keeping data
about the reasons for needing WBINVD, not relying on code ordering or
fragile semantics.
^ permalink raw reply
* Re: [PATCH v2 3/5] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Dave Hansen @ 2026-04-01 14:24 UTC (permalink / raw)
To: Edgecombe, Rick P, Verma, Vishal L, kas@kernel.org
Cc: seanjc@google.com, bp@alien8.de, x86@kernel.org, hpa@zytor.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <944b3fe5fa8955319030a7dfc0ea164bb0266e68.camel@intel.com>
On 3/31/26 14:36, Edgecombe, Rick P wrote:
> On Tue, 2026-03-31 at 18:22 +0000, Verma, Vishal L wrote:
>>> I guess the actual behaviour is dependant on the return code. It is
>>> obviously going to be the case for TDX_SUCCESS. And from the discussion,
>>> I guess that's true for TDX_SYS_BUSY and TDX_INTERRUPTED_RESUMABLE.
>>>
>>> What about other cases? The spec draft also lists TDX_SYS_NOT_READY and
>>> TDX_SYS_SHUTDOWN.
>> I think these are safe too - TDX_SYS_SHUTDOWN means the module has
>> already been shutdown, which this seamcall would've done, so things
>> should be in the same state either way.
>>
>> TDX_SYS_NOT_READY means the module hasn't been initialized yet. This
>> seamcall should just exit, and the module is already blocking any
>> seamcall that need the module to be initialized. The seamcalls to
>> initialize the module will be allowed, as they are after a sys_disable
>> call anyway.
> Should the seamcall return success in the case where it would return
> TDX_SYS_NOT_READY? It is in basically a reset state right? The errors we care
> about are actual errors (TDX_SW_ERROR), so it makes no difference to the code in
> the patch. But it might be a nicer API for the seamcall?
The problem is that the module doesn't have *a* reset state.
TDX_SYS_NOT_READY gets returned before the module is initialized and
initialization is a long, arduous process.
For instance, I believe the module stays "not ready" in the middle of
giving it PAMT memory and a keyID and all that jazz.
TDX_SYS_NOT_READY is a way of saying it can't easily *make* it to the
actual reset state that TDH.SYS.DISABLE wants it to be in.
It's arguable that the module should be made more resilient to stop
returning TDX_SYS_NOT_READY. But it's not as simple as just changing a
return code in the module.
I'm OK with it continuing to return TDX_SYS_NOT_READY for now. I think
it's a useful indicator. Maybe the kernel can't do much with it, but
it's a little window into what went wrong.
^ permalink raw reply
* Re: [PATCH v2 11/31] x86/virt/tdx: Make TDX Module initialize Extensions
From: Huang, Kai @ 2026-04-01 11:42 UTC (permalink / raw)
To: yilun.xu@linux.intel.com, Edgecombe, Rick P
Cc: Gao, Chao, Xu, Yilun, x86@kernel.org, kas@kernel.org,
baolu.lu@linux.intel.com, dave.hansen@linux.intel.com,
Li, Xiaoyao, Williams, Dan J, Jiang, Dave,
linux-pci@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Duan, Zhenzhong, Verma, Vishal L,
kvm@vger.kernel.org
In-Reply-To: <acvhD/XeCbB4ea0C@yilunxu-OptiPlex-7050>
On Tue, 2026-03-31 at 22:58 +0800, Xu Yilun wrote:
> > > + /*
> > > + * ext_required == 0 means no need to call TDH.EXT.INIT, the Extensions
> > > + * are already working.
> >
> > How does this scenario happen exactly? And why not check it above at the
> > beginning? Before the allocation, so it doesn't need to free.
> >
> > Is there a scenario where the memory needs to be given, but the extension is
> > already inited?
>
> mm.. you are right. It leads to something absurd.
>
> I checked with TDX Module team again. The correct understanding is:
>
> - TDX_FEATURES0_EXT bit shows Extensions is supported.
> - optional feature bits are selected on TDH_SYS_CONFIG
> - If one of the optional feature (e.g. TDX CONNECT) requires Extention,
> memory_pool_required_pages > 0 && ext_required == 1. Otherwise no
> need to initialize Extension.
>
> So yes, I should check memory_pool_required_pages && ext_required at the
> beginning.
My understanding is different:
Per spec, the 'EXT_REQUIRED' global metadata just means "Return true if the
TDH.EXT.INIT is required to be called", so I think, architecturally, it's
possible that one particular feature only requires additional memory pool
but doesn't explicitly need to call TDH.EXT.INIT. Or some feature may not
require any additional memory pool but needs TDH.EXT.INIT. Or require both
(such as TDX Connect I presume).
We can safely assume 2) and 3) are not required if no feature is configured
in 1) (backward compatibility). But when there is, I think we can just:
1) If 'MEMORY_POOL_REQUIRED_PAGES' is not zero, do TDH.EXT.MEM.ADD
2) If 'EXT_REQUIRED' is true, do TDH.EXT.INIT
^ permalink raw reply
* Re: [PATCH v13 12/48] arm64: RMI: Basic infrastructure for creating a realm.
From: Steven Price @ 2026-04-01 10:54 UTC (permalink / raw)
To: Wei-Lin Chang, kvm, kvmarm
Cc: Catalin Marinas, Marc Zyngier, Will Deacon, James Morse,
Oliver Upton, Suzuki K Poulose, Zenghui Yu, linux-arm-kernel,
linux-kernel, Joey Gouly, Alexandru Elisei, Christoffer Dall,
Fuad Tabba, linux-coco, Ganapatrao Kulkarni, Gavin Shan,
Shanker Donthineni, Alper Gun, Aneesh Kumar K . V, Emi Kisanuki,
Vishal Annapurve
In-Reply-To: <xpl4tvzm22ruavicwzgpcw7dsh5mtclhsp5tnkkyybgrxiwptj@f4tmkuyutitp>
On 21/03/2026 16:34, Wei-Lin Chang wrote:
> On Wed, Mar 18, 2026 at 03:53:36PM +0000, Steven Price wrote:
>> Introduce the skeleton functions for creating and destroying a realm.
>> The IPA size requested is checked against what the RMM supports.
>>
>> The actual work of constructing the realm will be added in future
>> patches.
>>
>> Signed-off-by: Steven Price <steven.price@arm.com>
>> ---
>> Changes since v12:
>> * Drop the RMM_PAGE_{SHIFT,SIZE} defines - the RMM is now configured to
>> be the same as the host's page size.
>> * Rework delegate/undelegate functions to use the new RMI range based
>> operations.
>> Changes since v11:
>> * Major rework to drop the realm configuration and make the
>> construction of realms implicit rather than driven by the VMM
>> directly.
>> * The code to create RDs, handle VMIDs etc is moved to later patches.
>> Changes since v10:
>> * Rename from RME to RMI.
>> * Move the stage2 cleanup to a later patch.
>> Changes since v9:
>> * Avoid walking the stage 2 page tables when destroying the realm -
>> the real ones are not accessible to the non-secure world, and the RMM
>> may leave junk in the physical pages when returning them.
>> * Fix an error path in realm_create_rd() to actually return an error value.
>> Changes since v8:
>> * Fix free_delegated_granule() to not call kvm_account_pgtable_pages();
>> a separate wrapper will be introduced in a later patch to deal with
>> RTTs.
>> * Minor code cleanups following review.
>> Changes since v7:
>> * Minor code cleanup following Gavin's review.
>> Changes since v6:
>> * Separate RMM RTT calculations from host PAGE_SIZE. This allows the
>> host page size to be larger than 4k while still communicating with an
>> RMM which uses 4k granules.
>> Changes since v5:
>> * Introduce free_delegated_granule() to replace many
>> undelegate/free_page() instances and centralise the comment on
>> leaking when the undelegate fails.
>> * Several other minor improvements suggested by reviews - thanks for
>> the feedback!
>> Changes since v2:
>> * Improved commit description.
>> * Improved return failures for rmi_check_version().
>> * Clear contents of PGD after it has been undelegated in case the RMM
>> left stale data.
>> * Minor changes to reflect changes in previous patches.
>> ---
>> arch/arm64/include/asm/kvm_emulate.h | 5 ++
>> arch/arm64/include/asm/kvm_rmi.h | 16 +++++
>> arch/arm64/kvm/arm.c | 12 ++++
>> arch/arm64/kvm/mmu.c | 11 +++-
>> arch/arm64/kvm/rmi.c | 88 ++++++++++++++++++++++++++++
>> 5 files changed, 129 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
>> index f38b50151ce8..39310d9b4e16 100644
>> --- a/arch/arm64/include/asm/kvm_emulate.h
>> +++ b/arch/arm64/include/asm/kvm_emulate.h
>> @@ -701,6 +701,11 @@ static inline enum realm_state kvm_realm_state(struct kvm *kvm)
>> return READ_ONCE(kvm->arch.realm.state);
>> }
>>
>> +static inline bool kvm_realm_is_created(struct kvm *kvm)
>> +{
>> + return kvm_is_realm(kvm) && kvm_realm_state(kvm) != REALM_STATE_NONE;
>> +}
>> +
>> static inline bool vcpu_is_rec(struct kvm_vcpu *vcpu)
>> {
>> return false;
>> diff --git a/arch/arm64/include/asm/kvm_rmi.h b/arch/arm64/include/asm/kvm_rmi.h
>> index 3506f50b05cd..0ada525af18f 100644
>> --- a/arch/arm64/include/asm/kvm_rmi.h
>> +++ b/arch/arm64/include/asm/kvm_rmi.h
>> @@ -6,6 +6,8 @@
>> #ifndef __ASM_KVM_RMI_H
>> #define __ASM_KVM_RMI_H
>>
>> +#include <asm/rmi_smc.h>
>> +
>> /**
>> * enum realm_state - State of a Realm
>> */
>> @@ -46,11 +48,25 @@ enum realm_state {
>> * struct realm - Additional per VM data for a Realm
>> *
>> * @state: The lifetime state machine for the realm
>> + * @rd: Kernel mapping of the Realm Descriptor (RD)
>> + * @params: Parameters for the RMI_REALM_CREATE command
>> + * @num_aux: The number of auxiliary pages required by the RMM
>> + * @ia_bits: Number of valid Input Address bits in the IPA
>> */
>> struct realm {
>> enum realm_state state;
>> +
>> + void *rd;
>> + struct realm_params *params;
>> +
>> + unsigned long num_aux;
>> + unsigned int ia_bits;
>> };
>>
>> void kvm_init_rmi(void);
>> +u32 kvm_realm_ipa_limit(void);
>> +
>> +int kvm_init_realm_vm(struct kvm *kvm);
>> +void kvm_destroy_realm(struct kvm *kvm);
>>
>> #endif /* __ASM_KVM_RMI_H */
>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>> index 274d7866efdc..9b17bdfaf0c2 100644
>> --- a/arch/arm64/kvm/arm.c
>> +++ b/arch/arm64/kvm/arm.c
>> @@ -253,6 +253,13 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>>
>> bitmap_zero(kvm->arch.vcpu_features, KVM_VCPU_MAX_FEATURES);
>>
>> + /* Initialise the realm bits after the generic bits are enabled */
>> + if (kvm_is_realm(kvm)) {
>> + ret = kvm_init_realm_vm(kvm);
>> + if (ret)
>> + goto err_free_cpumask;
>> + }
>> +
>> return 0;
>>
>> err_free_cpumask:
>> @@ -312,6 +319,8 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>> kvm_unshare_hyp(kvm, kvm + 1);
>>
>> kvm_arm_teardown_hypercalls(kvm);
>> + if (kvm_is_realm(kvm))
>> + kvm_destroy_realm(kvm);
>> }
>>
>> static bool kvm_has_full_ptr_auth(void)
>> @@ -473,6 +482,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>> else
>> r = kvm_supports_cacheable_pfnmap();
>> break;
>> + case KVM_CAP_ARM_RMI:
>> + r = static_key_enabled(&kvm_rmi_is_available);
>> + break;
>>
>> default:
>> r = 0;
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 070a01e53fcb..d6094b60c4ce 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -872,12 +872,16 @@ static struct kvm_pgtable_mm_ops kvm_s2_mm_ops = {
>> .icache_inval_pou = invalidate_icache_guest_page,
>> };
>>
>> -static int kvm_init_ipa_range(struct kvm_s2_mmu *mmu, unsigned long type)
>> +static int kvm_init_ipa_range(struct kvm *kvm,
>> + struct kvm_s2_mmu *mmu, unsigned long type)
>> {
>> u32 kvm_ipa_limit = get_kvm_ipa_limit();
>> u64 mmfr0, mmfr1;
>> u32 phys_shift;
>>
>> + if (kvm_is_realm(kvm))
>> + kvm_ipa_limit = kvm_realm_ipa_limit();
>> +
>> if (type & ~KVM_VM_TYPE_ARM_IPA_SIZE_MASK)
>> return -EINVAL;
>>
>> @@ -974,7 +978,7 @@ int kvm_init_stage2_mmu(struct kvm *kvm, struct kvm_s2_mmu *mmu, unsigned long t
>> return -EINVAL;
>> }
>>
>> - err = kvm_init_ipa_range(mmu, type);
>> + err = kvm_init_ipa_range(kvm, mmu, type);
>> if (err)
>> return err;
>>
>> @@ -1113,7 +1117,8 @@ void kvm_free_stage2_pgd(struct kvm_s2_mmu *mmu)
>> write_unlock(&kvm->mmu_lock);
>>
>> if (pgt) {
>> - kvm_stage2_destroy(pgt);
>> + if (!kvm_is_realm(kvm))
>> + kvm_stage2_destroy(pgt);
>
> Hi,
>
> Question:
> Since kvm_stage2_destroy() is only called for non-realm VMs, then where
> does the root level RTT pages get freed?
> After searching for a while I feel like it is missed, but I am not
> certain.
You're absolutely right. I do have a bug fix locally for this:
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 9cfb8c434aa5..3e55c1ff046c 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -1148,6 +1148,8 @@ void kvm_free_stage2_pgd(struct kvm_s2_mmu *mmu)
if (pgt) {
if (!kvm_is_realm(kvm))
kvm_stage2_destroy(pgt);
+ else
+ kvm_pgtable_stage2_destroy_pgd(pgt);
kfree(pgt);
}
}
The issue here is that we don't want to talk the tables (because they
will have been scrubbed by the RMM), but I apparently forgot to add the
call to free the actual memory.
Thanks,
Steve
> Thanks,
> Wei-Lin Chang
>
>> kfree(pgt);
>> }
>> }
>> diff --git a/arch/arm64/kvm/rmi.c b/arch/arm64/kvm/rmi.c
>> index 80aedc85e94a..700b8c935d29 100644
>> --- a/arch/arm64/kvm/rmi.c
>> +++ b/arch/arm64/kvm/rmi.c
>> @@ -6,6 +6,8 @@
>> #include <linux/kvm_host.h>
>> #include <linux/memblock.h>
>>
>> +#include <asm/kvm_emulate.h>
>> +#include <asm/kvm_mmu.h>
>> #include <asm/kvm_pgtable.h>
>> #include <asm/rmi_cmds.h>
>> #include <asm/virt.h>
>> @@ -182,6 +184,92 @@ static int rmi_init_metadata(void)
>> return 0;
>> }
>>
>> +u32 kvm_realm_ipa_limit(void)
>> +{
>> + return u64_get_bits(rmm_feat_reg0, RMI_FEATURE_REGISTER_0_S2SZ);
>> +}
>> +
>> +static int undelegate_range(phys_addr_t phys, unsigned long size)
>> +{
>> + unsigned long ret;
>> + unsigned long top = phys + size;
>> + unsigned long out_top;
>> +
>> + while (phys < top) {
>> + ret = rmi_granule_range_undelegate(phys, top, &out_top);
>> + if (ret == RMI_SUCCESS)
>> + phys = out_top;
>> + else if (ret != RMI_BUSY && ret != RMI_BLOCKED)
>> + return ret;
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static int undelegate_page(phys_addr_t phys)
>> +{
>> + return undelegate_range(phys, PAGE_SIZE);
>> +}
>> +
>> +static int free_delegated_page(phys_addr_t phys)
>> +{
>> + if (WARN_ON(undelegate_page(phys))) {
>> + /* Undelegate failed: leak the page */
>> + return -EBUSY;
>> + }
>> +
>> + free_page((unsigned long)phys_to_virt(phys));
>> +
>> + return 0;
>> +}
>> +
>> +void kvm_destroy_realm(struct kvm *kvm)
>> +{
>> + struct realm *realm = &kvm->arch.realm;
>> + size_t pgd_size = kvm_pgtable_stage2_pgd_size(kvm->arch.mmu.vtcr);
>> +
>> + write_lock(&kvm->mmu_lock);
>> + kvm_stage2_unmap_range(&kvm->arch.mmu, 0,
>> + BIT(realm->ia_bits - 1), true);
>> + write_unlock(&kvm->mmu_lock);
>> +
>> + if (realm->params) {
>> + free_page((unsigned long)realm->params);
>> + realm->params = NULL;
>> + }
>> +
>> + if (!kvm_realm_is_created(kvm))
>> + return;
>> +
>> + WRITE_ONCE(realm->state, REALM_STATE_DYING);
>> +
>> + if (realm->rd) {
>> + phys_addr_t rd_phys = virt_to_phys(realm->rd);
>> +
>> + if (WARN_ON(rmi_realm_destroy(rd_phys)))
>> + return;
>> + free_delegated_page(rd_phys);
>> + realm->rd = NULL;
>> + }
>> +
>> + if (WARN_ON(undelegate_range(kvm->arch.mmu.pgd_phys, pgd_size)))
>> + return;
>> +
>> + WRITE_ONCE(realm->state, REALM_STATE_DEAD);
>> +
>> + /* Now that the Realm is destroyed, free the entry level RTTs */
>> + kvm_free_stage2_pgd(&kvm->arch.mmu);
>> +}
>> +
>> +int kvm_init_realm_vm(struct kvm *kvm)
>> +{
>> + kvm->arch.realm.params = (void *)get_zeroed_page(GFP_KERNEL);
>> +
>> + if (!kvm->arch.realm.params)
>> + return -ENOMEM;
>> + return 0;
>> +}
>> +
>> static int rmm_check_features(void)
>> {
>> if (kvm_lpa2_is_enabled() && !rmi_has_feature(RMI_FEATURE_REGISTER_0_LPA2)) {
>> --
>> 2.43.0
>>
^ permalink raw reply related
* Re: [PATCH v2 08/31] x86/virt/tdx: Configure TDX Module with optional TDX Connect feature
From: Huang, Kai @ 2026-04-01 10:13 UTC (permalink / raw)
To: Williams, Dan J, linux-pci@vger.kernel.org,
linux-coco@lists.linux.dev, yilun.xu@linux.intel.com,
x86@kernel.org
Cc: Gao, Chao, Edgecombe, Rick P, Xu, Yilun, Jiang, Dave,
dave.hansen@linux.intel.com, baolu.lu@linux.intel.com,
Duan, Zhenzhong, kas@kernel.org, Verma, Vishal L, Li, Xiaoyao,
kvm@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20260327160132.2946114-9-yilun.xu@linux.intel.com>
>
> TDX Module updates global metadata when optional features are enabled.
> Host should update the cached tdx_sysinfo to reflect these changes.
>
>
[...]
> static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
> {
> struct tdx_module_args args = {};
> + u64 seamcall_fn = TDH_SYS_CONFIG_V0;
> u64 *tdmr_pa_array;
> size_t array_sz;
> int i, ret;
> @@ -1377,7 +1378,15 @@ static int config_tdx_module(struct tdmr_info_list *tdmr_list, u64 global_keyid)
> args.rcx = __pa(tdmr_pa_array);
> args.rdx = tdmr_list->nr_consumed_tdmrs;
> args.r8 = global_keyid;
> - ret = seamcall_prerr(TDH_SYS_CONFIG, &args);
> +
> + if (tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_TDXCONNECT) {
> + args.r9 |= TDX_FEATURES0_TDXCONNECT;
> + args.r11 = ktime_get_real_seconds();
> + /* These parameters requires version >= 1 */
> + seamcall_fn = TDH_SYS_CONFIG;
> + }
> +
> + ret = seamcall_prerr(seamcall_fn, &args);
>
> /* Free the array as it is not required anymore. */
> kfree(tdmr_pa_array);
> @@ -1537,6 +1546,11 @@ static int init_tdx_module(void)
> if (ret)
> goto err_free_pamts;
>
> + /* configuration to tdx module may change tdx_sysinfo, update it */
> + ret = get_tdx_sys_info(&tdx_sysinfo);
> + if (ret)
> + goto err_reset_pamts;
> +
How about put this into config_tdx_module()?
In this way you can only update global metadata when there's new feature
being opted in, and at the meantime, avoid making init_tdx_module() more
complicated.
^ permalink raw reply
* Re: [PATCH v2 3/5] x86/virt/tdx: Add SEAMCALL wrapper for TDH.SYS.DISABLE
From: Kiryl Shutsemau @ 2026-04-01 9:26 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: Verma, Vishal L, seanjc@google.com, bp@alien8.de, x86@kernel.org,
hpa@zytor.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <944b3fe5fa8955319030a7dfc0ea164bb0266e68.camel@intel.com>
On Tue, Mar 31, 2026 at 09:36:03PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2026-03-31 at 18:22 +0000, Verma, Vishal L wrote:
> > >
> > > I guess the actual behaviour is dependant on the return code. It is
> > > obviously going to be the case for TDX_SUCCESS. And from the discussion,
> > > I guess that's true for TDX_SYS_BUSY and TDX_INTERRUPTED_RESUMABLE.
> > >
> > > What about other cases? The spec draft also lists TDX_SYS_NOT_READY and
> > > TDX_SYS_SHUTDOWN.
> >
> > I think these are safe too - TDX_SYS_SHUTDOWN means the module has
> > already been shutdown, which this seamcall would've done, so things
> > should be in the same state either way.
> >
> > TDX_SYS_NOT_READY means the module hasn't been initialized yet. This
> > seamcall should just exit, and the module is already blocking any
> > seamcall that need the module to be initialized. The seamcalls to
> > initialize the module will be allowed, as they are after a sys_disable
> > call anyway.
>
> Should the seamcall return success in the case where it would return
> TDX_SYS_NOT_READY? It is in basically a reset state right? The errors we care
> about are actual errors (TDX_SW_ERROR), so it makes no difference to the code in
> the patch. But it might be a nicer API for the seamcall?
I am not sure. TDX_SYS_NOT_READY can be useful as might indicate
mismatch of system state understanding between kernel and TDX module.
> > > I wounder if it can affect the kernel. Consider the case when kexec
> > > (crash kernel start) happens due to crash on TDX module.
> > >
> > > Will we be able to shutdown TDX module cleanly and make kexec safe?
> >
> > Hm -are the semantics for what happens if there is a crash in the
> > module defined?
I meant kernel crash around/before TDX module initialization. Sorry for
confusion.
> > I think Linux should expect that sys_disable should
> > either start doing its shutdown work, or exit with one of the other
> > defined exit statuses. Anything else would be considered a module bug.
>
> We often have the question come up about how much we should to guard against
> bugs in the TDX module. I tend to also think we should not do defensive
> programming, same as we do for the kernel. If it's easy to handle something or
> emit a warning it's nice, but otherwise the solution for such cases should be to
> fix the TDX module bug.
>
> But for the kdump case, we don't actually need sys disable to succeed. The kdump
> kernel will not load the TDX module.
AFAIK, it is possible to start a normal kernel after kdump is done with
kexec (requires memmap= tricks). And the normal kernel might want to use
TDX again.
Not sure if it is done in practice. I would rather go full reboot path
after crash.
> And as for the errata, this already needs a
> special situation to be a problem. But even if it happens, I'd think better to
> try to the kdump. Not sure what the fix would be for that scenario, even if we
> allowed for a large complexity budget. So best effort seems good.
>
> Does it seem reasonable?
I am probably too picky here. We want to start from make basic kexec
functionality to work for start.
Reviewed-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH 1/2] x86/tdx: Fix off-by-one in port I/O handling
From: Kiryl Shutsemau @ 2026-04-01 8:34 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H . Peter Anvin, Rick Edgecombe, Borys Tsyrulnikov, linux-kernel,
linux-coco, kvm, stable
In-Reply-To: <ee096f1e-b994-4d56-a78c-cb0e867ea047@linux.intel.com>
On Tue, Mar 31, 2026 at 02:57:32PM -0700, Kuppuswamy Sathyanarayanan wrote:
> Hi Kirill,
>
> On 3/31/2026 4:24 AM, Kiryl Shutsemau (Meta) wrote:
> > handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
> >
> > u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
> >
> > GENMASK(h, l) includes bit h. For size=1 (INB), this produces
> > GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
> > bits). The mask is one bit too wide for all I/O sizes.
> >
> > Fix the mask calculation.
> >
> > Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> > Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> > Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> > Cc: stable@vger.kernel.org
> > ---
>
> LGTM. Can you include a link to the bug report or related discussion in
> the commit log? It will help understand the impact of this issue.
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
It is relevant for both.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Chao Gao @ 2026-04-01 8:27 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <6bec316e-3216-425c-89e2-fcf32fd4e15a@intel.com>
On Tue, Mar 31, 2026 at 08:44:36AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> + ptr = sig;
>> + for (i = 0; i < sig_size / SZ_4K; i++) {
>> + /*
>> + * @sig is 4KB-aligned, but that does not imply PAGE_SIZE
>> + * alignment when PAGE_SIZE != SZ_4K. Always include the
>> + * in-page offset.
>> + */
>> + params->sigstruct_pa[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
>> + ((unsigned long)ptr & ~PAGE_MASK);
>> + ptr += SZ_4K;
>> + }
>
>There are a billion things the mainline kernel _could_ do. Like a 32-bit
>4/4 split or a non-4k PAGE_SIZE on x86. But the mainline kernel doesn't
>*do* this.
>
>Why add complexity to deal with something that doesn't exist?
The intent was to avoid sprinkling subtle assumptions that PAGE_SIZE == SZ_4K.
But I agree that on x86 that's not a realistic concern today. I will just
assume 4KB page size.
And I assume we don't need WARN_ON_ONCE(PAGE_SIZE != SZ_4K) since this is
unlikely to break soon and shouldn't be very hard to debug and fix if it
does.
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-04-01 7:49 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <01fc8946-eb84-46fa-9458-f345dd3f6033@intel.com>
On Tue, Mar 31, 2026 at 08:11:22AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +static enum fw_upload_err tdx_fw_poll_complete(struct fw_upload *fwl)
>> +{
>> + /*
>> + * TDX module updates are completed in the previous phase
>> + * (tdx_fw_write()). If any error occurred, the previous phase
>> + * would return an error code to abort the update process. In
>> + * other words, reaching this point means the update succeeded.
>> + */
>> + return FW_UPLOAD_ERR_NONE;
>> +}
>> +
>
>This will be the seventh 'fw_upload_ops' and the third that has this
>pattern of not needing a ->poll_complete() implementation. It seems like
>allowing a NULL ->poll_complete or having a common stub for this pattern
>would be worthwhile.
Thanks for this suggestion. allowing a NULL ->poll_complete looks good.
I will post a separate series for this. the core change would be:
From b0d02c337db9df040b702f57a28e960da04bd9e3 Mon Sep 17 00:00:00 2001
From: Chao Gao <chao.gao@intel.com>
Date: Tue, 31 Mar 2026 22:17:36 -0700
Subject: [PATCH] firmware_loader: Make fw_upload_ops::poll_complete() optional
mpfs-auto-update.c and thp7312.c implement poll_complete() as a trivial
stub that simply returns FW_UPLOAD_ERR_NONE, because their write() is
synchronous and the upload completes before fw_upload_ops::write() returns.
Make poll_complete() optional so that drivers with synchronous write()
don't need to duplicate this trivial stub. When the callback is not
provided, assume the upload has completed and simply return
FW_UPLOAD_ERR_NONE.
An alternative would be defining and exporting a common stub, but
exporting a stub seems unnecessary and differs from the optional
fw_upload_ops::cleanup() handling.
Suggested-by: Dave Hansen <dave.hansen@linux.intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Link: https://lore.kernel.org/kvm/01fc8946-eb84-46fa-9458-f345dd3f6033@intel.com/
---
drivers/base/firmware_loader/sysfs_upload.c | 12 +++++++++---
include/linux/firmware.h | 2 +-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_loader/sysfs_upload.c b/drivers/base/firmware_loader/sysfs_upload.c
index f59a7856934c..06788a6d7227 100644
--- a/drivers/base/firmware_loader/sysfs_upload.c
+++ b/drivers/base/firmware_loader/sysfs_upload.c
@@ -158,6 +158,13 @@ static void fw_upload_prog_complete(struct fw_upload_priv *fwlp)
mutex_unlock(&fwlp->lock);
}
+static enum fw_upload_err fw_upload_poll_complete(struct fw_upload_priv *fwlp)
+{
+ if (!fwlp->ops->poll_complete)
+ return FW_UPLOAD_ERR_NONE;
+ return fwlp->ops->poll_complete(fwlp->fw_upload);
+}
+
static void fw_upload_main(struct work_struct *work)
{
struct fw_upload_priv *fwlp;
@@ -197,7 +204,7 @@ static void fw_upload_main(struct work_struct *work)
}
fw_upload_update_progress(fwlp, FW_UPLOAD_PROG_PROGRAMMING);
- ret = fwlp->ops->poll_complete(fwl);
+ ret = fw_upload_poll_complete(fwlp);
if (ret != FW_UPLOAD_ERR_NONE)
fw_upload_set_error(fwlp, ret);
@@ -306,8 +313,7 @@ firmware_upload_register(struct module *module, struct device *parent,
if (!name || name[0] == '\0')
return ERR_PTR(-EINVAL);
- if (!ops || !ops->cancel || !ops->prepare ||
- !ops->write || !ops->poll_complete) {
+ if (!ops || !ops->cancel || !ops->prepare || !ops->write) {
dev_err(parent, "Attempt to register without all required ops\n");
return ERR_PTR(-EINVAL);
}
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index aae1b85ffc10..c320e8b3e708 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -57,7 +57,7 @@ struct fw_upload {
* size written or a negative error code. The write()
* op will be called repeatedly until all data is
* written.
- * @poll_complete: Required: Check for the completion of the
+ * @poll_complete: Optional: Check for the completion of the
* HW authentication/programming process.
* @cancel: Required: Request cancellation of update. This op
* is called from the context of a different kernel
--
2.47.3
>
>I also don't think you need to be that verbose. This would be fine:
>
> /*
> * The upload completed during tdx_fw_write().
> * Never poll for completion.
> */
Sure. thanks.
>
>
>
^ permalink raw reply related
* Re: [PATCH v2 03/31] x86/virt/tdx: Add tdx_page_array helpers for new TDX Module objects
From: Tony Lindgren @ 2026-04-01 7:25 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: yilun.xu@linux.intel.com, Gao, Chao, Xu, Yilun, x86@kernel.org,
kas@kernel.org, baolu.lu@linux.intel.com,
dave.hansen@linux.intel.com, Li, Xiaoyao, Williams, Dan J,
Jiang, Dave, linux-pci@vger.kernel.org,
linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
Duan, Zhenzhong, Verma, Vishal L, kvm@vger.kernel.org
In-Reply-To: <23b92a154c859450d106de6c9badbe284e3aa19f.camel@intel.com>
On Mon, Mar 30, 2026 at 11:25:08PM +0000, Edgecombe, Rick P wrote:
> On Mon, 2026-03-30 at 18:25 +0800, Xu Yilun wrote:
> > The caller is not aware of singleton mode. Actually, I'm trying to make
> > the tdx_page_array independent of HPA_ARRAY_T or HPA_LIST_INFO details
> > when allocating/populating, root page is still populated even not needed
> > for singleton mode. The differences only happen when collaping the struct
> > into u64 SEAMCALL parameters.
>
> It seems tdx_page_array combines two concepts. An array of pages, and the method
> that the pages get handed to the TDX module. What if we broke apart these
> concepts?
We could add an enum for the LIST_INFO type to intialized the tdx_page_array?
Then the code using the tdx_page_array could initialize the root page based on
the LIST_INFO type for the SEAMCALL.
Regards,
Tony
^ permalink raw reply
* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Chao Gao @ 2026-04-01 3:10 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-kernel, linux-coco, kvm, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <2efa9f58-af65-4e77-9667-f66dc48a61c7@intel.com>
On Tue, Mar 31, 2026 at 08:04:19AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +static enum fw_upload_err tdx_fw_write(struct fw_upload *fwl, const u8 *data,
>> + u32 offset, u32 size, u32 *written)
>> +{
>> + int ret;
>> +
>> + /*
>> + * 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);
>
>How would this non-zero offset be triggered? Wouldn't it take a bug in
>the firmware upload code?
Yes, a bug in firmware upload code or tdx_fw_write() returns partial success.
>If so, why bother? It's not like something bad
>happens here.
the comment and WARN() was added to explain why @offset isn't used at all
in this function. But if you feel this detail is obvious or doesn't help
readability, let's drop it.
^ permalink raw reply
* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-04-01 2:25 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <95126568-3031-4d0a-85c8-d3934b6fec3e@intel.com>
On Tue, Mar 31, 2026 at 07:58:14AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> +/*
>> + * Open-code DEVICE_ATTR_ADMIN_RO to specify a different 'show' function
>> + * for P-SEAMLDR version as version_show() is used for TDX module version.
>> + *
>> + * Admin-only readable as reading these attributes calls into P-SEAMLDR,
>> + * which may have potential performance and system impact.
>> + */
>> +static struct device_attribute dev_attr_seamldr_version =
>> + __ATTR(version, 0400, seamldr_version_show, NULL);
>> +static DEVICE_ATTR_ADMIN_RO(num_remaining_updates);
>
>I don't like that these are really *exactly* the same, except for the
>name but are defined so differently. I see three alternatives that are
>better:
>
>1. Open code *both* so they look the same
>2. Define a macro that both can use that has a seamldr_## prefix
>3. Move the code to a new file to avoid symbol conflicts.
>
>I'd honestly be fine with any of those.
>
>The other option is to just put the module information at the top
>directory so the sysfs name differentiates things.
>
>For real, how many things are going to be in the seamldr directory? If
>it's just two, is it worth having a directory?
Good question. There are two for this series, and I don't expect there will be
more soon.
The only reason to have a separate directory is that P-SEAMLDR is a different
component from the TDX module. But I think that's a bit weak. By renaming the
attribute to seamldr_version instead of version, there's no symbol conflict.
I will apply this incremental change:
diff --git a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
index f7221f2e5fec..1ace37b7f0e9 100644
--- a/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
+++ b/Documentation/ABI/testing/sysfs-devices-faux-tdx-host
@@ -5,14 +5,14 @@ Description: (RO) Report the version of the loaded TDX module. The TDX module
"y" is the minor version and "z" is the update version. Versions
are used for bug reporting, TDX module updates etc.
-What: /sys/devices/faux/tdx_host/seamldr/version
+What: /sys/devices/faux/tdx_host/seamldr_version
Contact: linux-coco@lists.linux.dev
Description: (RO) Report the version of the loaded SEAM loader. The SEAM
loader version is formatted as x.y.z, where "x" is the major
version, "y" is the minor version and "z" is the update version.
Versions are used for bug reporting and compatibility checks.
-What: /sys/devices/faux/tdx_host/seamldr/num_remaining_updates
+What: /sys/devices/faux/tdx_host/num_remaining_updates
Contact: linux-coco@lists.linux.dev
Description: (RO) Report the number of remaining updates. TDX maintains a
log about each TDX module that has been loaded. This log has
diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
index 5a672126f372..cd84108094cf 100644
--- a/drivers/virt/coco/tdx-host/tdx-host.c
+++ b/drivers/virt/coco/tdx-host/tdx-host.c
@@ -75,15 +75,7 @@ static ssize_t num_remaining_updates_show(struct device *dev,
return sysfs_emit(buf, "%u\n", info.num_remaining_updates);
}
-/*
- * Open-code DEVICE_ATTR_ADMIN_RO to specify a different 'show' function
- * for P-SEAMLDR version as version_show() is used for TDX module version.
- *
- * Admin-only readable as reading these attributes calls into P-SEAMLDR,
- * which may have potential performance and system impact.
- */
-static struct device_attribute dev_attr_seamldr_version =
- __ATTR(version, 0400, seamldr_version_show, NULL);
+static DEVICE_ATTR_ADMIN_RO(seamldr_version);
static DEVICE_ATTR_ADMIN_RO(num_remaining_updates);
static struct attribute *seamldr_attrs[] = {
@@ -105,7 +97,6 @@ static bool seamldr_group_visible(struct kobject *kobj)
DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(seamldr);
static const struct attribute_group seamldr_group = {
- .name = "seamldr",
.attrs = seamldr_attrs,
.is_visible = SYSFS_GROUP_VISIBLE(seamldr),
};
^ permalink raw reply related
* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-04-01 1:57 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, kvm, linux-kernel, binbin.wu, dan.j.williams,
dave.hansen, ira.weiny, kai.huang, kas, nik.borisov, paulmck,
pbonzini, reinette.chatre, rick.p.edgecombe, sagis, seanjc,
tony.lindgren, vannapurve, vishal.l.verma, yilun.xu, xiaoyao.li,
yan.y.zhao, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin
In-Reply-To: <114e44e8-b06f-4b45-b962-fab2e6c03b68@intel.com>
On Tue, Mar 31, 2026 at 07:58:02AM -0700, Dave Hansen wrote:
>On 3/31/26 05:41, Chao Gao wrote:
>> Expose them as tdx-host device attributes. Make seamldr attributes
>> visible only when the update feature is supported, as that's their sole
>> purpose. Unconditional exposure is also problematic because reading them
>> triggers P-SEAMLDR calls that break KVM on CPUs with a specific erratum
>> (to be enumerated and handled in a later patch).
>
>The erratum is irrelevant, IMNHO. Or it *should* be irrelevant.
OK. I'll drop the erratum mention from the commit message.
^ permalink raw reply
* Re: [PATCH v2 05/31] x86/virt/tdx: Extend tdx_page_array to support IOMMU_MT
From: Edgecombe, Rick P @ 2026-04-01 0:17 UTC (permalink / raw)
To: yilun.xu@linux.intel.com
Cc: Gao, Chao, Xu, Yilun, x86@kernel.org, kas@kernel.org,
baolu.lu@linux.intel.com, dave.hansen@linux.intel.com,
Li, Xiaoyao, Williams, Dan J, Jiang, Dave,
linux-pci@vger.kernel.org, linux-coco@lists.linux.dev,
linux-kernel@vger.kernel.org, Duan, Zhenzhong, Verma, Vishal L,
kvm@vger.kernel.org
In-Reply-To: <acvX1x5nDdGtZWyI@yilunxu-OptiPlex-7050>
On Tue, 2026-03-31 at 22:19 +0800, Xu Yilun wrote:
> > Consider the amount of tricks that are needed to coax the tdx_page_array to
> > populate the handoff page as needed. It adds 2 pages here, then subtracts
> > them
> > later in the callback. Then tweaks the pa in tdx_page_array_populate() to
> > add
> > the length...
>
> mm.. The tricky part is the specific memory requirement/allocation, the
> common part is the pa list contained in a root page. Maybe we only model
> the later, let the specific user does the memory allocation. Is that
> closer to your "break concepts apart" idea?
I haven't wrapped my head around this enough to suggest anything is definitely
the right approach.
But yes, the idea would be that the allocation of the list of pages to give to
the TDX module would be a separate allocation and set of management functions.
And the the allocation of the pages that are used to communicate the list of
pages (and in this case other args) with the module would be another set. So
each type of TDX module arg page format (IOMMU_MT, etc) would be separable, but
share the page list allocation part only. It looks like Nikolay was probing
along the same path. Not sure if he had the same solution in mind.
So for this:
1. Allocate a list or array of pages using a generic method.
2. Allocate these two IOMMU special pages.
3. Allocate memory needed for the seamcall (root pages)
Hand all three to the wrapper and have it shove them all through in the special
way it prefers.
Maybe... Can you write something about the similarities and differences with the
three types of lists in that series? Like in a compact form?
Also, how much of the earlier code duplication you wanted to avoid was the
leaking and special error handling stuff?
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Edgecombe, Rick P @ 2026-03-31 23:29 UTC (permalink / raw)
To: seanjc@google.com
Cc: pbonzini@redhat.com, bp@alien8.de, x86@kernel.org, kas@kernel.org,
hpa@zytor.com, linux-kernel@vger.kernel.org, Verma, Vishal L,
Huang, Kai, mingo@redhat.com, tglx@kernel.org,
linux-coco@lists.linux.dev, dave.hansen@linux.intel.com,
kvm@vger.kernel.org
In-Reply-To: <acxTBwaw6_xYSShf@google.com>
On Tue, 2026-03-31 at 16:04 -0700, Sean Christopherson wrote:
> > Oh, you acked it actually. But I was under the impression that after this
> > patch here, the splat wouldn't be triggered. So it inadvertently fixes it.
>
> Ah, that's why I was a bit confused. I was assuming tdx_shutdown_cpu() was a
> cpuhp callback, but it's actually an IPI callback.
>
> Hmm, isn't this patch wrong then? Ah, no, the changelog says:
>
> However, WBINVD is already generally done at CPU offline as matter of
> course. So don't bother adding TDX specific logic for this, and rely on the
> normal WBINVD to handle it.
>
> What's the "normal" WBINVD? At the very least, tdx_offline_cpu() should have
> a comment that explicitly calls out where that WBVIND is.
I guess we could add one in tdx_offline_cpu(). Seems reasonable.
> I assume you're
> referring to the wbinvd() calls in things like hlt_play_dead()?
Yea.
>
> But unless the WBINVD is actually costly, why bother getting fancy?
What is the suggestion to make it less fancy? Just put the wbinvd in
tdx_offline_cpu()? Yea that works too. Probably will get a comment either way.
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Sean Christopherson @ 2026-03-31 23:04 UTC (permalink / raw)
To: Rick P Edgecombe
Cc: Vishal L Verma, Kai Huang, bp@alien8.de, x86@kernel.org,
kas@kernel.org, hpa@zytor.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, dave.hansen@linux.intel.com,
tglx@kernel.org, pbonzini@redhat.com, linux-coco@lists.linux.dev,
kvm@vger.kernel.org
In-Reply-To: <6ad3ff51bbab85147b716de0b1f4e8b994b1998b.camel@intel.com>
On Tue, Mar 31, 2026, Rick P Edgecombe wrote:
> On Tue, 2026-03-31 at 12:22 -0700, Sean Christopherson wrote:
> > > -
> > > -#ifdef CONFIG_KEXEC_CORE
> > > -void tdx_cpu_flush_cache_for_kexec(void)
> > > -{
> > > - lockdep_assert_preemption_disabled();
> >
> > Is there a pre-existing bug here that gets propagate to tdx_shutdown_cpu()?
> > When called from kvm_offline_cpu(), preemption won't be fully disabled, but
> > per-CPU access are fine because the task is pinned to the target CPU.
> >
> > See https://lore.kernel.org/all/aUVx20ZRjOzKgKqy@google.com
>
> Yes. And actually it got hit during development of this series. This patch will
> conflict with the fix:
> https://lore.kernel.org/lkml/20260312100009.924136-1-kai.huang@intel.com/
>
> Oh, you acked it actually. But I was under the impression that after this patch
> here, the splat wouldn't be triggered. So it inadvertently fixes it.
Ah, that's why I was a bit confused. I was assuming tdx_shutdown_cpu() was a
cpuhp callback, but it's actually an IPI callback.
Hmm, isn't this patch wrong then? Ah, no, the changelog says:
However, WBINVD is already generally done at CPU offline as matter of course.
So don't bother adding TDX specific logic for this, and rely on the normal
WBINVD to handle it.
What's the "normal" WBINVD? At the very least, tdx_offline_cpu() should have a
comment that explicitly calls out where that WBVIND is. I assume you're referring
to the wbinvd() calls in things like hlt_play_dead()?
But unless the WBINVD is actually costly, why bother getting fancy?
> But that other patch is much more backport friendly.
^ permalink raw reply
* Re: [PATCH v2 00/16] fs,x86/resctrl: Add kernel-mode (e.g., PLZA) support to the resctrl subsystem
From: Reinette Chatre @ 2026-03-31 22:24 UTC (permalink / raw)
To: Babu Moger, corbet, tony.luck, Dave.Martin, james.morse, tglx,
mingo, bp, dave.hansen
Cc: skhan, x86, hpa, peterz, juri.lelli, vincent.guittot,
dietmar.eggemann, rostedt, bsegall, mgorman, vschneid, kas,
rick.p.edgecombe, akpm, pmladek, rdunlap, dapeng1.mi, kees, elver,
paulmck, lirongqing, safinaskar, fvdl, seanjc, pawan.kumar.gupta,
xin, tiala, Neeraj.Upadhyay, chang.seok.bae, thomas.lendacky,
elena.reshetova, linux-doc, linux-kernel, linux-coco, kvm,
eranian, peternewman
In-Reply-To: <30deeb5b-d2ec-4f85-aa4f-c21400df3486@amd.com>
Hi Babu,
On 3/30/26 11:46 AM, Babu Moger wrote:
> On 3/27/26 17:11, Reinette Chatre wrote:
>> On 3/26/26 10:12 AM, Babu Moger wrote:
>>> On 3/24/26 17:51, Reinette Chatre wrote:
>>>> On 3/12/26 1:36 PM, Babu Moger wrote:
>>>>> Tony suggested using global variables to store the kernel mode
>>>>> CLOSID and RMID. However, the kernel mode CLOSID and RMID are
>>>>> coming from rdtgroup structure with the new interface. Accessing
>>>>> them requires holding the associated lock, which would make the
>>>>> context switch path unnecessarily expensive. So, dropped the idea.
>>>>> https://lore.kernel.org/lkml/aXuxVSbk1GR2ttzF@agluck-desk3/
>>>>> Let me know if there are other ways to optimize this.
>>>> I do not see why the context switch path needs to be touched at all with this
>>>> implementation. Since PLZA only supports global assignment does it not mean that resctrl
>>>> only needs to update PQR_PLZA_ASSOC when user writes to info/kernel_mode and
>>>> info/kernel_mode_assignment?
>>> Each thread has an MSR to configure whether to associate privilege level zero execution with a separate COS and/or RMID, and the value of the COS and/or RMID. PLZA may be enabled or disabled on a per-thread basis. However, the COS and RMID association and configuration must be the same for all threads in the QOS Domain.
>> Based on previous comment in https://lore.kernel.org/lkml/abb049fa-3a3d-4601-9ae3-61eeb7fd8fcf@amd.com/
>> and this implementation all fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_en must be the
>> same for all CPUs on the system, not just per QoS domain. Could you please confirm?
>
> Sorry for the confusion. It is "per QoS domain".
>
> All the fields of PQR_PLZA_ASSOC except PQR_PLZA_ASSOC.plza_enmust be set to the same value for all HW threads in the QOS domain for consistent operation (Per-QosDomain).
Thank you for clarifying. To build on this, what would be best way for resctrl to interpret this?
As I see it all values in PQR_PLZA_ASSOC apply to *all* resources yet (theoretically?) every resource
can have domains that span different CPUs. There thus seem to be a built in assumption of what a "domain"
means for PQR_PLZA_ASSOC so it sounds to me as though, instead of saying that "PQR_PLZA_ASSOC needs
to be the same in QoS domain" it may be more accurate to, for example, say that "PQR_PLZA_ASSOC has L3 scope"?
This seems to be what this implementation does since it hardcodes PQR_PLZA_ASSOC scope to the L3
resource but that creates dependency to the L3 resource that would make PLZA unusable if, for example,
the user boots with "rdt=!l3cat" while wanting to use PLZA to manage MBA allocations when in kernel?
...
> Yes, I agree with your concerns. The goal here is to make the interface less disruptive while still addressing the different use cases.
I consider changing resctrl behavior when values are written to existing resctrl files
to be disruptive. This is something we explicitly discussed during v1 as something to
be avoided so this implementation that overloads the tasks file again is unexpected.
> Background: Customers have identified an issue with the QoS
> Bandwidth Control feature: when a CLOS is aggressively throttled
> and execution transitions into kernel mode, kernel operations are
> also subject to the same aggressive throttling.
>
> > Privilege-Level Zero Association (PLZA) allows a user to specify a
> COS and/or RMID to be used during execution at Privilege Level Zero.
> When PLZA is enabled on a hardware thread, any execution that enters
> Privilege Level Zero will have its transactions associated with the
> PLZA COS and/or RMID. Otherwise, the thread continues to use the COS
> and RMID specified by |PQR_ASSOC|. In other words, the hardware
> provides a dedicated COS and/or RMID specifically for kernel-mode
> execution.
ack.
>
> There are multiple ways this feature can be applied. For simplicity, the discussion below focuses only on CLOSID.
>
>
> 1. Global PLZA enablement
>
> PLZA can be configured as a global feature by setting |PQR_PLZA_ASSOC.closid = CLOSID| and |PQR_PLZA_ASSOC.plza_en = 1| on all threads in the system. A dedicated CLOSID is reserved for this purpose,
Also discussed during v1 is that there is no need to dedicate a CLOSID for this purpose.
There could be an "unthrottled" CLOSID to which all high priority user space tasks as
well as all kernel work of all tasks are assigned.
If user space chooses to dedicate a CLOSID for kernel work then that should supported and
interface can allow that, but there is no need for resctrl to enforce this.
> and all CPU threads use its allocations whenever they enter Privilege Level Zero. This CLOSID does not need to be associated with any resctrl group.
The CLOSID has to be associated with a resource group to be able to manage its
resource allocations, no?
> The user can explicitly enable or disable this feature.
ack.
> There is no context switch overhead but there is no flexibility with this approach.
Flexibility is subjective. As I understand this supports the only use case we learned about so far:
https://lore.kernel.org/lkml/CABPqkBSq=cgn-am4qorA_VN0vsbpbfDePSi7gubicpROB1=djw@mail.gmail.com/
> 2. Group based PLZA allocation : PLZA is managed via dedicated
> restctrl group. A separate resctrl group can be created
> specifically for PLZA, with a dedicated CLOSID used exclusively
> for kernel mode execution. This approach can be further divided
> into two association models:
So far this sounds like global allocation since both need a dedicated resource group.
Whether this group is dedicated to kernel work or shared between kernel and user space work
is up to the user. There is no motivation why CLOSID should ever be enforced to be
exclusive for kernel mode execution.
>
> i) CPU based association
> CPUs are assigned to the PLZA group, and PLZA is enabled only on
> those CPUs. This effectively creates a dedicated PLZA group. MSRs (|
> PQR_PLZA_ASSOC)| are programmed only when the user changes CPU
> assignments. This approach requires no changes to the context switch
> code and introduces no additional context switch overhead.
>
> ii) Task based association
> Tasks are explicitly assigned by the user to the PLZA group. Tasks
> need to be updated when user adds a new task. Also, this requires
> updates during task scheduling so that the MSRs (|PQR_PLZA_ASSOC)|
> are programmed on each context switch, which introduces additional
> context switch overhead.
As discussed during v1 any changes needed to support per task assignment would
need to be done with new files dedicated to this purpose. Do not overload the
existing resctrl tasks/cpus/cpus_list files.
> I tried to fit these requirements into the interface files in /sys/
> fs/resctrl/info/. I may have missed few things while trying to
> achieve it. As usual, I am open for the discussion and
> recommendations.
Many of these items were already discussed as part of v1 so I think we may be
talking past each other here. I tried to highlight the relevant points raised
during v1 discussion that I thought there already was agreement on.
The one new aspect is that I assumed this implementation will only be for
global configuration and assignment. It looks like you want to support both
global configuration and per-task assignment. In the original I did not consider
configuration and assignment to occur at different scope so we may need to come up
with new modes to distinguish. Consider the addition of two modes as below:
# cat info/kernel_mode
[inherit_ctrl_and_mon]
global_assign_ctrl_inherit_mon_set_all
global_assign_ctrl_assign_mon_set_all
global_assign_ctrl_inherit_mon_set_individual
global_assign_ctrl_assign_mon_set_individual
Above introduces a "set_all" and "set_individual" suffix to the original two
modes.
global_assign_ctrl_inherit_mon_set_all
global_assign_ctrl_assign_mon_set_all:
Above are the original two modes but makes it clear that when this mode is
activated _all_ tasks run with the assignment.
global_assign_ctrl_inherit_mon_set_individual
global_assign_ctrl_assign_mon_set_individual:
Above are two new modes. In this mode user space also assigns a resource
group globally but then needs to follow that up by activating every task
separately to run with this assignment.
One way in which this can be accomplished could be to have "kernel_mode_tasks",
"kernel_mode_cpus", and "kernel_mode_cpus_list" files become visible (or be
created) in the resource group found in info/kernel_mode_assignment. User
space interacts with the new files to set which tasks and/or CPUs run with
PLZA enabled.
Even so, as I understand global_assign_ctrl_inherit_mon_set_all and
global_assign_ctrl_assign_mon_set_all addresses the only known use case. Do you know
if there are use cases for global_assign_ctrl_inherit_mon_set_individual and
global_assign_ctrl_assign_mon_set_individual? The latter two adds significant
complexity to resctrl while I have not heard about any use case for it.
Reinette
^ permalink raw reply
* Re: [PATCH v2 2/5] x86/virt/tdx: Pull kexec cache flush logic into arch/x86
From: Edgecombe, Rick P @ 2026-03-31 22:21 UTC (permalink / raw)
To: Verma, Vishal L, seanjc@google.com
Cc: Huang, Kai, bp@alien8.de, x86@kernel.org, kas@kernel.org,
hpa@zytor.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
dave.hansen@linux.intel.com, tglx@kernel.org, pbonzini@redhat.com,
linux-coco@lists.linux.dev, kvm@vger.kernel.org
In-Reply-To: <acwe7kKyjfEQ0sIf@google.com>
On Tue, 2026-03-31 at 12:22 -0700, Sean Christopherson wrote:
> > -
> > -#ifdef CONFIG_KEXEC_CORE
> > -void tdx_cpu_flush_cache_for_kexec(void)
> > -{
> > - lockdep_assert_preemption_disabled();
>
> Is there a pre-existing bug here that gets propagate to tdx_shutdown_cpu()?
> When called from kvm_offline_cpu(), preemption won't be fully disabled, but
> per-CPU access are fine because the task is pinned to the target CPU.
>
> See https://lore.kernel.org/all/aUVx20ZRjOzKgKqy@google.com
Yes. And actually it got hit during development of this series. This patch will
conflict with the fix:
https://lore.kernel.org/lkml/20260312100009.924136-1-kai.huang@intel.com/
Oh, you acked it actually. But I was under the impression that after this patch
here, the splat wouldn't be triggered. So it inadvertently fixes it. But that
other patch is much more backport friendly.
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
From: Huang, Kai @ 2026-03-31 22:13 UTC (permalink / raw)
To: x86@kernel.org, mingo@redhat.com, kas@kernel.org, tglx@kernel.org,
bp@alien8.de, dave.hansen@linux.intel.com
Cc: Edgecombe, Rick P, hpa@zytor.com,
sathyanarayanan.kuppuswamy@linux.intel.com,
linux-kernel@vger.kernel.org, tsyrulnikov.borys@gmail.com,
kvm@vger.kernel.org, stable@vger.kernel.org,
linux-coco@lists.linux.dev
In-Reply-To: <20260331112430.71425-3-kas@kernel.org>
On Tue, 2026-03-31 at 12:24 +0100, Kiryl Shutsemau (Meta) wrote:
> According to x86 architecture rules, 32-bit operations zero-extend the
> result to 64 bits.
>
FWIW, the relevant part in the SDM seems to be:
Chapter 3.4.1.1 General-Purpose Registers in 64-Bit Mode
...
* 32-bit operands generate a 32-bit result, zero-extended to a 64 bit
result in the destination general-purpose register.
> The current implementation of handle_in() only masks
> the lower 32 bits, which preserves the upper 32 bits of RAX when a
> 32-bit port IN instruction is emulated.
>
> Update handle_in() to zero out the entire RAX register when the I/O size
> is 4 bytes to ensure correct zero-extension. For smaller sizes (1 or 2
> bytes), continue to preserve the unaffected upper bits.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
> arch/x86/coco/tdx/tdx.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 4d7f71d50122..b9b9a2d75119 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,8 +703,17 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
> */
> success = !__tdx_hypercall(&args);
>
> - /* Update part of the register affected by the emulated instruction */
> - regs->ax &= ~mask;
> + /*
> + * Update part of the register affected by the emulated instruction.
> + *
> + * 32-bit operands generate a 32-bit result, zero-extended to a 64-bit
> + * result.
> + */
> + if (size < 4)
> + regs->ax &= ~mask;
> + else
> + regs->ax = 0;
> +
> if (success)
> regs->ax |= args.r11 & mask;
>
^ permalink raw reply
* Re: [PATCH 2/2] x86/tdx: Fix zero-extension for 32-bit port I/O
From: Kuppuswamy Sathyanarayanan @ 2026-03-31 22:10 UTC (permalink / raw)
To: Kiryl Shutsemau (Meta), Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86
Cc: H . Peter Anvin, Rick Edgecombe, Borys Tsyrulnikov, linux-kernel,
linux-coco, kvm, stable
In-Reply-To: <20260331112430.71425-3-kas@kernel.org>
Hi Kiril,
On 3/31/2026 4:24 AM, Kiryl Shutsemau (Meta) wrote:
> According to x86 architecture rules, 32-bit operations zero-extend the
> result to 64 bits. The current implementation of handle_in() only masks
> the lower 32 bits, which preserves the upper 32 bits of RAX when a
> 32-bit port IN instruction is emulated.
>
> Update handle_in() to zero out the entire RAX register when the I/O size
> is 4 bytes to ensure correct zero-extension. For smaller sizes (1 or 2
> bytes), continue to preserve the unaffected upper bits.
>
> Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
> Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
> Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
> Cc: stable@vger.kernel.org
> ---
If you have bug or discussion link, please include it.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> arch/x86/coco/tdx/tdx.c | 13 +++++++++++--
> 1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 4d7f71d50122..b9b9a2d75119 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,8 +703,17 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
> */
> success = !__tdx_hypercall(&args);
>
> - /* Update part of the register affected by the emulated instruction */
> - regs->ax &= ~mask;
> + /*
> + * Update part of the register affected by the emulated instruction.
> + *
> + * 32-bit operands generate a 32-bit result, zero-extended to a 64-bit
> + * result.
> + */
> + if (size < 4)
> + regs->ax &= ~mask;
> + else
> + regs->ax = 0;
The logic would be more readable as:
if (size == 4)
regs->ax = 0;
else
regs->ax &= ~mask;
> +
> if (success)
> regs->ax |= args.r11 & mask;
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ 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