Linux Confidential Computing Development
 help / color / mirror / Atom feed
* Re: [PATCH 0/3] arm64/virt: Add Arm CCA measurement register support
From: Suzuki K Poulose @ 2026-04-14 10:10 UTC (permalink / raw)
  To: Jason Gunthorpe, Sami Mujawar, Dan Williams
  Cc: linux-arm-kernel, linux-kernel, catalin.marinas, will, thuth,
	steven.price, gshan, YeoReum.Yun, cedric.xing, Dan Williams,
	Dionna Glaze, Aneesh Kumar K . V, Alexey Kardashevskiy,
	linux-coco@lists.linux.dev
In-Reply-To: <20260413125925.GK3694781@ziepe.ca>

Cc: Dan, Cedric, Dionna, Aneesh, Alexey. linux-coco

Hi Jason,

On 13/04/2026 13:59, Jason Gunthorpe wrote:
> On Mon, Apr 13, 2026 at 09:49:54AM +0100, Sami Mujawar wrote:
>> This series adds support for Arm Confidential Compute Architecture (CCA)
>> measurement registers in the Linux kernel, enabling guest Realms to
>> access, extend, and expose measurement values for attestation and runtime
>> integrity tracking.
>>
>> The Realm Management Monitor (RMM) defines a set of measurement registers
>> consisting of a Realm Initial Measurement (RIM) and a number of Realm
>> Extensible Measurements (REMs). This series introduces the necessary
>> infrastructure to interact with these registers via the RSI interface
>> and exposes them to userspace through the TSM measurement framework.
>>
>> At a high level, the series includes:
>>   - Helper interfaces for reading and extending measurement
>>     registers via RSI
>>   - Definitions for Realm hash algorithms as defined by the
>>     RMM specification
>>   - Integration with the TSM measurement subsystem and sysfs
>>     exposure for userspace visibility and interaction
>>
>> After applying this series, measurement registers are exposed under:
>>      /sys/devices/virtual/misc/arm_cca_guest/measurements/
> 
> I'm surprised we get some random sysfs files? How does some more
> generic userspace figure out to use this vs a TPM or some other
> platform's version of it?

That is true. This is the infrastructure for exposing Runtime
Measurement registers (R/W) for use by the OS, complementing the
TSM_REPORTS (Read Only Platform measurements+Attestation Reports, e.g.
on CCA Attestation Report from RMM). Unlike the TSM reports,
this doesn't have a generic interface for userspace.


> I also think exposing PCRs as was done for TPM in sysfs was something
> of a mistake.. Allowing extension without logging is too low level and
> is very hard to build an entire attestation system around.
> 
> I really think we are missing a subsystem here, TPM has sort of been
> filling this role in a non-generic way, but we should have a
> common uAPI for platform measurement & attestation:

Agreed, such a subsystem would solve the below.

>   - Discover available measurements
>   - Report signed measurements, with ingesting a nonce
>   - Report measurement logs
>   - Extend measurements and udpate logs
>   - Report certificates used in signing
>   - General reporting of various kinds of attestation evidence
> 
> And it would be nice for the PCI devices and others to plug into the
> general framework as well instead of building a parallel TSM framework
> for handling evidence.

That makes sense and AFAIU, there are efforts in progress to expose
the Device measurements+Certificates in a different form. May be a good
idea to intervene early enough to see if we can find a common ground.

> 
> Isn't this also sort of incomplete?  Doesn't anything serious need
> signed measurements? Isnt't there alot more data that comes out of RMM
> than just a few measurement registers?
As mentioned above, this series adds the support for Runtime Extendible
Measurements (REM in CCA, RTMR on TDX). The RIM+Platform Attestation is 
already provided via the TSM_REPORT


Kind regards
Suzuki

> 
> Jason


^ permalink raw reply

* Re: [PATCH v2 05/31] x86/virt/tdx: Extend tdx_page_array to support IOMMU_MT
From: Xu Yilun @ 2026-04-14  9:57 UTC (permalink / raw)
  To: 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: <f38d0a080aee052937cb6721683d55155c657717.camel@intel.com>

On Wed, Apr 01, 2026 at 12:17:45AM +0000, Edgecombe, Rick P wrote:
> 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.

I'm drafting some changes and make the tdx_page_array look like:

  struct tdx_page_array {
	/* public: */
	unsigned int nr_pages;
	struct page **pages;

	/* private: */
	u64 *root;
	bool flush_on_free;
  };

  - I removed the page allocations for tdx_page_array kAPIs. Now the
    caller needs to allocate the struct page **pages and the page list,
    then create the tdx_page_array by providing these pages.

    struct tdx_page_array *tdx_page_array_create(struct page **pages,
						 unsigned int nr_pages)

    This also means tdx_page_array doesn't have to hold more than 512
    pages anymore, it now an exact descriptor for the TDX Module's
    definitions rather than a manager. It's a chunk of the required
    memory when we need more than 512 pages. This eliminates the need
    for 'offset' field and the slide window operations so make the
    helpers simpler.

  - I still keep the generic struct tdx_page_array to represent all
    kinds of object types (HPA_ARRAY_T, HPA_LIST_INFO, IOMMU_MT), and
    provide the tdx_page_array to SEAMCALL helpers as parameters. I
    think this structure is generally good enough to represent a list of
    pages, keeps type safety compared to a list of HPAs.

  - I still record both the page list (struct page **pages) and the HPA
    list (in u64 *root). struct page **pages works with kernel memory
    management (e.g. vmap) well while the populated root works with
    SEAMCALLs.

  - I'm not introducing more structures each for an object type, like 
    struct hpa_array, struct hpa_list_info, struct iommu_metadata. They
    are conceptually the same thing. The iommu_mt supports multi-order
    pages, hpa_array_t & hpa_list_info don't support. But their bit
    definitions don't conflict. I can use the same piece of code to
    populate their root page content.

  - Add a flush_on_free field to mark if a cache write back is needed on
    tdx_page_array_free(), then we don't need 2 free APIs.

I want to clean up my code, then post an incremental patch for preview.

Thanks.

^ permalink raw reply

* Re: [PATCH v7 13/22] x86/virt/seamldr: Install a new TDX module
From: Chao Gao @ 2026-04-14 10:19 UTC (permalink / raw)
  To: Edgecombe, Rick P
  Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
	Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
	seanjc@google.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
	Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
	mingo@redhat.com, Weiny, Ira, tony.lindgren@linux.intel.com,
	Annapurve, Vishal, sagis@google.com, hpa@zytor.com,
	tglx@kernel.org, paulmck@kernel.org, bp@alien8.de,
	yilun.xu@linux.intel.com, dan.j.williams@intel.com,
	x86@kernel.org
In-Reply-To: <2eec98d012b85113b4e6876d07eb6513618e2404.camel@intel.com>

>> +static int seamldr_install(const struct seamldr_params *params)
>> +{
>> +	struct tdx_module_args args = { .rcx = __pa(params) };
>
>In an earlier patch you have a wrapper as:
>struct tdx_module_args args = = {};
>args.rxx = foo;
>
>Why the style difference? It would be good to standardize, but the existing code
>isn't standardized. What do you think about going with this style through the
>series for the one arg ones?

Sure, happy to standardize. To confirm, you prefer this style?

	struct tdx_module_args args = { .rcx = __pa(params) };

it looks more common than the other way.

^ permalink raw reply

* Re: [PATCH v7 22/22] x86/virt/seamldr: Log TDX module update failures
From: Chao Gao @ 2026-04-14 10:25 UTC (permalink / raw)
  To: Edgecombe, Rick P
  Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
	Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
	seanjc@google.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
	Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
	mingo@redhat.com, Weiny, Ira, tony.lindgren@linux.intel.com,
	Annapurve, Vishal, sagis@google.com, hpa@zytor.com,
	tglx@kernel.org, paulmck@kernel.org, bp@alien8.de,
	yilun.xu@linux.intel.com, dan.j.williams@intel.com,
	x86@kernel.org
In-Reply-To: <e4cdab7d83bfc1c7fe3b5759a987c71b922585b7.camel@intel.com>

On Tue, Apr 14, 2026 at 04:04:05AM +0800, Edgecombe, Rick P wrote:
>On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
>> Currently, there is no way to restore a TDX module from shutdown state to
>> running state. This means if errors occur after a successful module
>> shutdown, they are unrecoverable since the old module is gone but the new
>> module isn't installed. All subsequent SEAMCALLs to the TDX module will
>> fail, so TDs will be killed due to SEAMCALL failures.
>> 
>> Log a message to clarify that SEAMCALL errors are expected in this
>> scenario. This ensures that after update failures, the first message in
>> dmesg explains the situation rather than showing confusing call traces from
>> various code paths.
>
>Why is this patch at the end? Is it not valid initial behavior?

It's not strictly required for the update to function. It just improves
debuggability by explaining expected SEAMCALL errors after a failure.

I placed it last so maintainers can easily drop it if they consider it
non-essential, similar to the guidance in *.

*: https://lore.kernel.org/kvm/63082cd1-15ab-4aaf-83ad-f72d94b9bb8e@intel.com/

^ permalink raw reply

* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Chao Gao @ 2026-04-14 11:20 UTC (permalink / raw)
  To: Edgecombe, Rick P
  Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
	Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
	seanjc@google.com, binbin.wu@linux.intel.com, pbonzini@redhat.com,
	Chatre, Reinette, Verma, Vishal L, nik.borisov@suse.com,
	mingo@redhat.com, Weiny, Ira, tony.lindgren@linux.intel.com,
	Annapurve, Vishal, sagis@google.com, hpa@zytor.com,
	tglx@kernel.org, paulmck@kernel.org, bp@alien8.de,
	yilun.xu@linux.intel.com, dan.j.williams@intel.com,
	x86@kernel.org
In-Reply-To: <fbcce1bd98ff8a7ff106f7583e3b576b170358cf.camel@intel.com>

On Tue, Apr 14, 2026 at 03:08:33AM +0800, Edgecombe, Rick P wrote:
>On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
>> TDX module updates require userspace to select the appropriate module
>> to load. Expose necessary information to facilitate this decision. Two
>> values are needed:
>> 
>> - P-SEAMLDR version: for compatibility checks between TDX module and
>> 		     P-SEAMLDR
>> - num_remaining_updates: indicates how many updates can be performed
>
>Can you explain how all of these overlap?
> - TDX module supports module update
> - SEAMLDR supports NUM_REMAINING_UPDATES info
> - SEAMLDR supports VERSION info
>
>If the TDX module supports module update, do we know the SEAMLDR supports this
>other stuff somehow? It might be worth a comment the reasoning.

VERSION and NUM_REMAINING_UPDATES are always available for any P-SEAMLDR. They
don't depend on TDX module's update support.

>
>> 
>> 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).
>
>Since this is later handled with the errata check, what is the point being made
>here?

I will drop it. Dave also questioned mentioning the erratum here.

^ permalink raw reply

* Re: [PATCH 0/3] arm64/virt: Add Arm CCA measurement register support
From: Jason Gunthorpe @ 2026-04-14 12:29 UTC (permalink / raw)
  To: Suzuki K Poulose
  Cc: Sami Mujawar, Dan Williams, linux-arm-kernel, linux-kernel,
	catalin.marinas, will, thuth, steven.price, gshan, YeoReum.Yun,
	cedric.xing, Dan Williams, Dionna Glaze, Aneesh Kumar K . V,
	Alexey Kardashevskiy, linux-coco@lists.linux.dev
In-Reply-To: <514ddb27-137b-4223-84fe-2152737db3a6@arm.com>

On Tue, Apr 14, 2026 at 11:10:51AM +0100, Suzuki K Poulose wrote:

> > Isn't this also sort of incomplete?  Doesn't anything serious need
> > signed measurements? Isnt't there alot more data that comes out of RMM
> > than just a few measurement registers?
> As mentioned above, this series adds the support for Runtime Extendible
> Measurements (REM in CCA, RTMR on TDX). The RIM+Platform Attestation is
> already provided via the TSM_REPORT

Okay, but what actual use is this?

Extendable measrements with no log
Measurement read back without signature

What is the use case? What do you imagine any userspace will do with
this? Put it in the cover letter.

I don't think the raw rmm calls are sufficiently developed to be
usable directly by userspace. They are less capable than TPM and even
TPM has a lot of software around it to make it useful.

Jason

^ permalink raw reply

* Re: [PATCH v2 1/6] KVM: x86: Add dedicated storage for guest RIP
From: Xiaoyao Li @ 2026-04-14 12:31 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
  Cc: kvm, x86, linux-coco, linux-kernel, Chang S . Bae
In-Reply-To: <20260409224236.2021562-2-seanjc@google.com>

On 4/10/2026 6:42 AM, Sean Christopherson wrote:
> Add kvm_vcpu_arch.rip to track guest RIP instead of including it in the
> generic regs[] array.  Decoupling RIP from regs[] will allow using a
> *completely* arbitrary index for RIP, as opposed to the mostly-arbitrary
> index that is currently used.  That in turn will allow using indices
> 16-31 to track R16-R31 that are coming with APX.

Even leave RIP in regs[], what is the problem by just allocating the 
index 16-31 to R16-R31 and making RIP the index 32? (I think I need go 
read the APX discussion to better understand the reason)

> Note, although RIP can used for addressing, it does NOT have an
                          ^
missing a 'be'

> architecturally defined index, and so can't be reached via flows like
> get_vmx_mem_address() where KVM "blindly" reads a general purpose register
> given the SIB information reported by hardware.  For RIP-relative
> addressing, hardware reports the full "offset" in vmcs.EXIT_QUALIFICATION.
> 
> Note #2, keep the available/dirty tracking as RSP is context switched

s/RSP/RIP

> through the VMCS, i.e. needs to be cached for VMX.
> 
> Opportunistically rename NR_VCPU_REGS to NR_VCPU_GENERAL_PURPOSE_REGS to
> better capture what it tracks, and so that KVM can slot in R16-R13 without

s/R16-R13/R16-R31

> running into weirdness where KVM's definition of "EXREG" doesn't line up
> with APX's definition of "extended reg".
> 
> No functional change intended.
> 

^ permalink raw reply

* Re: [PATCH 0/3] arm64/virt: Add Arm CCA measurement register support
From: Suzuki K Poulose @ 2026-04-14 13:26 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Sami Mujawar, Dan Williams, linux-arm-kernel, linux-kernel,
	catalin.marinas, will, thuth, steven.price, gshan, YeoReum.Yun,
	cedric.xing, Dan Williams, Dionna Glaze, Aneesh Kumar K . V,
	Alexey Kardashevskiy, linux-coco@lists.linux.dev
In-Reply-To: <20260414122950.GW3694781@ziepe.ca>

On 14/04/2026 13:29, Jason Gunthorpe wrote:
> On Tue, Apr 14, 2026 at 11:10:51AM +0100, Suzuki K Poulose wrote:
> 
>>> Isn't this also sort of incomplete?  Doesn't anything serious need
>>> signed measurements? Isnt't there alot more data that comes out of RMM
>>> than just a few measurement registers?
>> As mentioned above, this series adds the support for Runtime Extendible
>> Measurements (REM in CCA, RTMR on TDX). The RIM+Platform Attestation is
>> already provided via the TSM_REPORT
> 
> Okay, but what actual use is this?
> 

Good point. This REMs are planned to be used for 
EFI_CC_MEASUREMENT_PROTOCOL as described below:

https://github.com/tianocore/edk2/issues/11383

At the moment they are exposed as raw, similar to the Intel TDX RTMRs.
This may eventually need to be connected to IMA subsystem.

> Extendable measrements with no log
> Measurement read back without signature
> 
> What is the use case? What do you imagine any userspace will do with
> this? Put it in the cover letter.

Agreed.

> 
> I don't think the raw rmm calls are sufficiently developed to be
> usable directly by userspace. They are less capable than TPM and even
> TPM has a lot of software around it to make it useful.

See above.

Kind regards
Suzuki

> 
> Jason


^ permalink raw reply

* Re: [PATCH 0/3] arm64/virt: Add Arm CCA measurement register support
From: Jason Gunthorpe @ 2026-04-14 13:35 UTC (permalink / raw)
  To: Suzuki K Poulose
  Cc: Sami Mujawar, Dan Williams, linux-arm-kernel, linux-kernel,
	catalin.marinas, will, thuth, steven.price, gshan, YeoReum.Yun,
	cedric.xing, Dan Williams, Dionna Glaze, Aneesh Kumar K . V,
	Alexey Kardashevskiy, linux-coco@lists.linux.dev
In-Reply-To: <a1831d3d-af31-4fa7-b107-f4001841f051@arm.com>

On Tue, Apr 14, 2026 at 02:26:58PM +0100, Suzuki K Poulose wrote:
> On 14/04/2026 13:29, Jason Gunthorpe wrote:
> > On Tue, Apr 14, 2026 at 11:10:51AM +0100, Suzuki K Poulose wrote:
> > 
> > > > Isn't this also sort of incomplete?  Doesn't anything serious need
> > > > signed measurements? Isnt't there alot more data that comes out of RMM
> > > > than just a few measurement registers?
> > > As mentioned above, this series adds the support for Runtime Extendible
> > > Measurements (REM in CCA, RTMR on TDX). The RIM+Platform Attestation is
> > > already provided via the TSM_REPORT
> > 
> > Okay, but what actual use is this?
> > 
> 
> Good point. This REMs are planned to be used for EFI_CC_MEASUREMENT_PROTOCOL
> as described below:
> 
> https://github.com/tianocore/edk2/issues/11383

So this is tying it to the same FW event log that TPM uses.

I think that strengthens my point this should all be uninform. TPM
drivers are directly exposing the event log today, but I guess that
needs generalization if non-TPM drivers are going to present it as
well.

How do you imagine getting and manipulating the EFI event log to use
with this?

Jason

^ permalink raw reply

* Re: [PATCH v2 1/6] KVM: x86: Add dedicated storage for guest RIP
From: Chang S. Bae @ 2026-04-14 13:59 UTC (permalink / raw)
  To: Xiaoyao Li, Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau
  Cc: kvm, x86, linux-coco, linux-kernel
In-Reply-To: <20b82b65-b156-4a2c-8094-b86dccfb3025@intel.com>

On 4/14/2026 5:31 AM, Xiaoyao Li wrote:
> Even leave RIP in regs[], what is the problem by just allocating the 
> index 16-31 to R16-R31 and making RIP the index 32?

But why?

Even though the array isn't explicitly labeled as GPRs, that's 
effectively how it's being used, and RIP isn't part of that set.

I don't think there is any benefit of leaving it in regs[]. Instead, It 
can be stored like that simple, period.

Thanks,
Chang

^ permalink raw reply

* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Sean Christopherson @ 2026-04-14 14:04 UTC (permalink / raw)
  To: Xiaoyao Li
  Cc: Kai Huang, Chang Seok Bae, kvm@vger.kernel.org,
	pbonzini@redhat.com, kas@kernel.org, linux-kernel@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org
In-Reply-To: <95a931f8-42cc-4834-953c-30c9167bfdc1@intel.com>

On Tue, Apr 14, 2026, Xiaoyao Li wrote:
> On 4/14/2026 7:03 AM, Huang, Kai wrote:
> > > Because VMX and SVM make all GRPs available immediately, except
> > > for RSP, KVM ignores avail/dirty for GPRs.  I.e. "fixing" TDX will just shift the
> > > "bugs" elsewhere.
> > Just want to understand:
> > 
> > I thought the fix could be we simply remove the wrong GPRs from the list.
> > Not sure how fixing TDX will shift bugs elsewhere?
> 
> I'm curious too.

What I'm saying is that, _if_ there are bugs where KVM uses a register that isn't
available, then modifying TDX's list won't actually fix anything (without more
changes), it will just change which code is technically buggy (hence all the quotes
above).

> > > More importantly, because the TDX-Module*requires* RCX (the GPR that holds the
> > > mask of registers to expose to the VMM) to be hidden on TDVMCALL, KVM*can't*
> > > do any kind of meaningful "available" tracking.
> > > 
> > Hmm I think RCX conveys the shared GPRs and VMM can read.  Per "Table 5.323:
> > TDH.VP.ENTER Output Operands Format #5 Definition: On TDCALL(TDG.VP.VMCALL)
> > Following a TD Entry":
> > 
> >    RCX   ...
> > 	Bit(s) Name         Description
> > 
> > 	31:0   PARAMS_MASK  Value as passed into TDCALL(TDG.VP.VMCALL) by
> > 			    the guest TD: indicates which part of the guest
> > 			    TD GPR and XMM state is passed as-is to the
> > VMM
> > 			    and back. For details, see the description of
> > 			    TDG.VP.VMCALL in 5.5.26.
> > 
> > I think the problem is, as said previously, currently KVM TDX code uses
> > KVM's existing infrastructure to emulate MSR, KVM hypercall etc,  but
> > TDVMCALL has a different ABI, thus there's a mismatch here.
> 
> I once had patch for it internally.
> 
> It adds back the available check for GPRs when accessing instead of assuming
> they are always available. For normal VMX and SVM, all the GPRs are still
> always available. But for TDX, only EXIT_INFO_1 and EXIT_INFO_2 are always
> marked available, while others need to be explicitly set case by case.
> 
> The good thing is it makes TDX safer that KVM won't consume invalid data
> silently for TDX. But it adds additional overhead of checking the
> unnecessary register availability for VMX and SVM case.
> 
> -----------------------------&<-------------------------------------
> From: Xiaoyao Li <xiaoyao.li@intel.com>
> Date: Tue, 11 Mar 2025 07:13:29 -0400
> Subject: [PATCH] KVM: x86: Add available check for GPRs
> 
> Since commit de3cd117ed2f ("KVM: x86: Omit caching logic for
> always-available GPRs"), KVM doesn't check the availability of GPRs
> except RSP and RIP when accessing them, because they are always
> available.
> 
> However, it's not true when it comes to TDX. The GPRs are not available
> after TD vcpu exits actually.

> And it relies on KVM manually sets the
> GPRs value when needed, e.g.
> 
>  - setting rax, rbx, rcx, rdx, rsi, for hypercall emulation in
>    tdx_emulate_tdvmall();
> 
>  - setting rax, rcx and rdx before MSR write emulation;
> 
> Add the available check of GPRs read, and WARN_ON_ONCE() when unavailable.
> It can help capture the cases of undesired GPRs consumption by TDX.

Sorry, but NAK.  I am strongly against adding any code to the GPR accessors/mutators
just for TDX.  It's a _lot_ of code.  From commit de3cd117ed2f ("KVM: x86: Omit
caching logic for always-available GPRs"):

    E.g. on x86_64, kvm_emulate_cpuid() is reduced from 342 to 182 bytes and
    kvm_emulate_hypercall() from 1362 to 1143, with the total size of KVM
    dropping by ~1000 bytes.  With CONFIG_RETPOLINE=y, the numbers are even
    more pronounced, e.g.: 353->182, 1418->1172 and well over 2000 bytes.

Note that updating only the "available" masks is wrong, as TDX needs to marshall
written registers back to their correct location.

In the end, the available/dirty tracking isn't about hardening against bugs, it's
about deferring expensive VMREAD and VMWRITE (and guest memory) operations until
action is required.

We could bury sanity checks behind a Kconfig of some kind, but I genuinely don't
see much value in doing so.  These emulation flows are very static (all register
usage is hardcoded), and so it's very much a "get it right once" sort of thing,
i.e. the odds of a runtime check finding a bug after initial development are
basically zero.

An alternative for TDX would be to avoid bouncing through GPRs in the first place,
e.g. by reworking __kvm_emulate_rdmsr() to not access any registers.  But I'm
probably opposed to even that, because I doubt the end result would be an overall
net positive for KVM.  We'd end up with duplicate code, harder to read common
code (because of the new abstractions), and likely without meaningfully moving
the needle in terms of finding/preventing bugs.  KVM still needs to get operands
to/from the right parameters, though only difference is that for TDX, the parameters
would be very "direct".

^ permalink raw reply

* Re: [PATCH v2 1/6] KVM: x86: Add dedicated storage for guest RIP
From: Sean Christopherson @ 2026-04-14 15:37 UTC (permalink / raw)
  To: Chang S. Bae
  Cc: Xiaoyao Li, Paolo Bonzini, Kiryl Shutsemau, kvm, x86, linux-coco,
	linux-kernel
In-Reply-To: <87e767b6-0324-44e6-92cb-f933002dec43@intel.com>

On Tue, Apr 14, 2026, Chang S. Bae wrote:
> On 4/14/2026 5:31 AM, Xiaoyao Li wrote:
> > Even leave RIP in regs[], what is the problem by just allocating the
> > index 16-31 to R16-R31 and making RIP the index 32?
> 
> But why?
> 
> Even though the array isn't explicitly labeled as GPRs, that's effectively
> how it's being used, and RIP isn't part of that set.
> 
> I don't think there is any benefit of leaving it in regs[]. 

+1.  Chang's earlier argument that RIP isn't a proper GPR swayed me over, e.g. RIP
doesn't have an architectural index.

Keeping RIP in regs[] saves one line of code in arch/x86/include/asm/kvm_host.h,
at the cost of making the code less readable (IMO) and incorrectly suggesting that
RIP can be accessed like other regs[].

^ permalink raw reply

* Re: [PATCH v2 5/6] KVM: x86: Track available/dirty register masks as "unsigned long" values
From: Sean Christopherson @ 2026-04-14 15:48 UTC (permalink / raw)
  To: Kai Huang
  Cc: Chang Seok Bae, kvm@vger.kernel.org, pbonzini@redhat.com,
	kas@kernel.org, linux-kernel@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org
In-Reply-To: <d6f05e5fa781ccb465d27a4fb1c7c1ac1e9e95ff.camel@intel.com>

On Mon, Apr 13, 2026, Kai Huang wrote:
> On Mon, 2026-04-13 at 07:54 -0700, Sean Christopherson wrote:
> > More importantly, because the TDX-Module *requires* RCX (the GPR that holds the
> > mask of registers to expose to the VMM) to be hidden on TDVMCALL, KVM *can't*
> > do any kind of meaningful "available" tracking.  
> > 
> 
> Hmm I think RCX conveys the shared GPRs and VMM can read.  Per "Table 5.323:
> TDH.VP.ENTER Output Operands Format #5 Definition: On TDCALL(TDG.VP.VMCALL)
> Following a TD Entry":
> 
>   RCX   ...
> 	Bit(s) Name         Description
> 
> 	31:0   PARAMS_MASK  Value as passed into TDCALL(TDG.VP.VMCALL) by
> 			    the guest TD: indicates which part of the guest
> 			    TD GPR and XMM state is passed as-is to the
> VMM 
> 			    and back. For details, see the description of
> 			    TDG.VP.VMCALL in 5.5.26.

The problem is that bit 1 in RCX is required to be '0'.  I.e. the guest *can't*
expose RCX to the VMM.  From the spec:

  15:0    GPR Mask Controls the transfer of GPR values:
  Bit 0:  RAX (must be 0)
  Bit 1:  RCX (must be 0)

And the code:

  api_error_type tdg_vp_vmcall(uint64_t controller_value)
  {
    api_error_type retval = TDX_OPERAND_INVALID;
    tdx_module_local_t* tdx_local_data_ptr = get_local_data();

    tdvmcall_control_t control = { .raw = controller_value };

    // Bits 0, 1 and 4 and 63:32 of RCX must be 0
    if (((control.gpr_select & (uint16_t)(BIT(0) | BIT(1) | BIT(4))) != 0) ||  <==== sadness
         (control.reserved != 0))
    {
        retval = api_error_with_operand_id(TDX_OPERAND_INVALID, OPERAND_ID_RCX);
        TDX_ERROR("Unsupported bits in GPR_SELECT field = 0x%x\n", control.gpr_select)
        goto EXIT_FAILURE;
    }

Oh, dagnabbit.  The spec also says:

  The value of RCX itself is always passed to the host VMM.

and then in code:

    td_exit_qual.gpr_select = control.gpr_select;
    td_exit_qual.xmm_select = control.xmm_select;

    tdx_local_data_ptr->vmm_regs.rcx = td_exit_qual.raw;

    // RAX is not copied, RCX filled above, start from RDX

I don't get why TDX requires bit 1 to be 0, but whatever.

So I was wrong, KVM can (and should!) validate the registers coming from the
guest.  If we want to harden TDX, that's the obvious first step.

^ permalink raw reply

* SVSM Development Call April 15, 2026
From: Jörg Rödel @ 2026-04-14 16:15 UTC (permalink / raw)
  To: coconut-svsm, linux-coco

Hi,

Here is the call for agenda items for this weeks SVSM development call.  Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.

We will use the LF Zoom instance. Details of the meeting  can be found in our
governance repository at:

	https://github.com/coconut-svsm/governance

The link to the COCONUT-SVSM calendar is:

	https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week

The meeting will be recorded and the recording eventually published.

Regards,

	Jörg

^ permalink raw reply

* Re: [PATCH v7 06/22] coco/tdx-host: Expose P-SEAMLDR information via sysfs
From: Edgecombe, Rick P @ 2026-04-14 17:02 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4jFpCYbW5laTbr@intel.com>

On Tue, 2026-04-14 at 19:20 +0800, Chao Gao wrote:
> > Can you explain how all of these overlap?
> > - TDX module supports module update
> > - SEAMLDR supports NUM_REMAINING_UPDATES info
> > - SEAMLDR supports VERSION info
> > 
> > If the TDX module supports module update, do we know the SEAMLDR supports
> > this other stuff somehow? It might be worth a comment the reasoning.
> 
> VERSION and NUM_REMAINING_UPDATES are always available for any P-SEAMLDR. They
> don't depend on TDX module's update support.

The old seamldr's don't support them though, right? At least the docs I'm
looking at show NUM_REMAINING_UPDATES as a change in the spec. Oh this update is
from 2022. So quite old now.

^ permalink raw reply

* Re: [PATCH v7 07/22] coco/tdx-host: Implement firmware upload sysfs ABI for TDX module updates
From: Edgecombe, Rick P @ 2026-04-14 17:04 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4N6giqSJcnZ/o6@intel.com>

On Tue, 2026-04-14 at 17:50 +0800, Chao Gao wrote:
> > It's hard to review whether these error codes match because the function
> > doesn't
> > return them yet. Why isn't this patch just done later in the series after
> > everything is in place?
> 
> Good point.
> 
> The series is ordered top-down (user interface first, implementation details
> later). I could either move this patch later, or add each error mapping in the
> patch that introduces the corresponding return value. I'd prefer the latter to
> keep the top-down ordering. Would that work for you?

Either way sounds ok to me.

^ permalink raw reply

* Re: [PATCH v7 11/22] x86/virt/seamldr: Shut down the current TDX module
From: Edgecombe, Rick P @ 2026-04-14 17:34 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4SZdlybWkLtLEm@intel.com>

On Tue, 2026-04-14 at 18:09 +0800, Chao Gao wrote:
> Retrieve the module's handoff version from TDX global metadata and add an
> update step to shut down the module. Module shutdown has global effect, so
> it only needs to run on one CPU.

Sure.

^ permalink raw reply

* Re: [PATCH v7 13/22] x86/virt/seamldr: Install a new TDX module
From: Edgecombe, Rick P @ 2026-04-14 17:35 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4UsiCjY6V+Ox5D@intel.com>

On Tue, 2026-04-14 at 18:19 +0800, Chao Gao wrote:
> > Why the style difference? It would be good to standardize, but the existing
> > code
> > isn't standardized. What do you think about going with this style through
> > the
> > series for the one arg ones?
> 
> Sure, happy to standardize. To confirm, you prefer this style?
> 
> 	struct tdx_module_args args = { .rcx = __pa(params) };
> 
> it looks more common than the other way.

They both seem fine to me. And, yes, this is probably in the nit category. But
if it looks sloppy it can add doubts to the code for the final review.

^ permalink raw reply

* Re: [PATCH v7 08/22] x86/virt/seamldr: Allocate and populate a module update request
From: Edgecombe, Rick P @ 2026-04-14 17:37 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4MWR0DyOaI0C50@intel.com>

On Tue, 2026-04-14 at 17:43 +0800, Chao Gao wrote:
> On Sat, Apr 11, 2026 at 09:14:36AM +0800, Edgecombe, Rick P wrote:
> > On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> > > P-SEAMLDR uses the SEAMLDR_PARAMS structure to describe TDX module
> > > update requests. This structure contains physical addresses pointing to
> > > the module binary and its signature file (or sigstruct), along with an
> > > update scenario field.
> > > 
> > > TDX modules are distributed in the tdx_blob format defined in
> > > blob_structure.txt from the "Intel TDX module Binaries Repository". A
> > > tdx_blob contains a header, sigstruct, and module binary. This is also the
> > > format supplied by the userspace to the kernel.
> > > 
> > > Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure
> > > accordingly. This structure will be passed to P-SEAMLDR to initiate the
> > > update.
> > 
> > The thing that confused me about this earlier was the exact reason why we are
> > checking all the fields. We discussed that we need to check the fields that
> > kernel processes, but we don't need to double check data that the TDX module
> > processes.
> > 
> > Should we explain it?
> 
> Yes. how about adding the following in the changelog:
> 
> Parse the tdx_blob format and populate a SEAMLDR_PARAMS structure. The
> header is consumed solely by the kernel to extract the sigstruct and
> module, so validate it before processing. The sigstruct and module are
> passed to and validated by P-SEAMLDR, so don't duplicate any validation
> in the kernel.

Looks good.

> 
> > And how it explains the checks below?
> 
> 
> > > +static void free_seamldr_params(struct seamldr_params *params)
> > > +{
> > > +	free_page((unsigned long)params);
> > > +}
> > 
> > Do we really need this helper? This doesn't work?
> 
> No. This works. Will do.
> 
> > 
> > DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> > 	    if (!IS_ERR_OR_NULL(_T)) free_page((unsigned long)_T))
> > 
> > 
> > > +
> > > +static struct seamldr_params *alloc_seamldr_params(const void *module, unsigned int module_size,
> > > +						   const void *sig, unsigned int sig_size)
> > > +{
> > > +	struct seamldr_params *params;
> > > +	const u8 *ptr;
> > > +	int i;
> > > +
> > > +	if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K)
> > > +		return ERR_PTR(-EINVAL);
> > 
> > I don't know if it's worth that much, but we could do a MIN thing here to
> > protect the loop, and lose the conditionals. If userspace passes a blob that is
> > out of spec they can deal with the module error, no?
> 
> I think we all agree: we need to do something to protect the loop. So, the
> question is just how.
> 
> looks like you prefer:
> 
> 	module_size = MIN(module_size, SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K);
> 	sig_size = MIN(sig_size, SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K);
> 
> But I think the MIN approach actually requires more justification than a plain
> bounds check.
> 
> With MIN, the reasoning chain is: we don't want conditionals -> userspace
> can deal with the module error -> and that error won't be fatal to the
> system (or even if it is, it's admin-initiated). That's a lot of assumptions
> to validate.
> 
> With a bounds check and early return, the reasoning is simply: the input is out
> of range -> reject it as invalid.

Hmm, I'm convinced. I think a lot of tip code elides conditionals. I'm not sure
which they would prefer.

I guess the problem I see is that all these conditionals seem like a lot for
something that is mostly just getting passed through. So they need to come
across better somehow.

> 
> > 
> > > +
> > > +	/*
> > > +	 * Check that input buffers satisfy P-SEAMLDR's size and alignment
> > > +	 * constraints so they can be passed directly to P-SEAMLDR without
> > > +	 * relocation or copy.
> > > +	 */
> > > +	if (!IS_ALIGNED(module_size, SZ_4K) || !IS_ALIGNED(sig_size, SZ_4K) ||
> > > +	    !IS_ALIGNED((unsigned long)module, SZ_4K) ||
> > > +	    !IS_ALIGNED((unsigned long)sig, SZ_4K))
> > > +		return ERR_PTR(-EINVAL);
> > 
> > I thought you are going to reduce this checking to just to reject invalid input
> > that the kernel processes.
> > 
> > What happens if we don't check this?
> 
> If the blob->offset_of_module or blob->size is misaligned, the loops
> silently drop the unaligned portion. P-SEAMLDR will then reject the update
> after module shutdown and TDs will be killed. Since this is admin-initiated
> with an intentionally invalid blob, the consequence is acceptable.

Seems like we could drop it then.

> 
> > The vmallocs are all going to be page
> > aligned anyway. But even still, does it mess up the below loops somehow in a way
> > that hurts anything?
> 
> I agree addresses/sizes are page-aligned for valid blobs, but they are not
> guaranteed (e.g., if offset_of_module is misaligned) for all inputs. I
> removed the check once in v6 and Sashiko questioned it, so I thought the
> assumption isn't obviously correct to everyone.

Haha, as impressed as I am with AI code review... are we already at the point of
adding comments to help the AI not get confused?

> 
> Without the check, we'd need something like:
> 
> 	/*
> 	 * Assume sig/module base addresses and sizes are page-aligned.
> 	 * If violated, P-SEAMLDR will reject the update.
> 	 */
> 
> It's a few lines of straightforward validation vs. a comment that requires
> readers to verify the assumption chain (vmalloc internals, userspace
> contract, P-SEAMLDR behavior). Not sure the trade is worth it.
> 
> If you think the comment is sufficient, I'm fine dropping the checks.

The checks seem like too much. Any ideas on how to contain it more? It seems
like you are making the case for each one. But if we delete them all... does it
seem like a better tradeoff?

> 
> > 
> > I might be confused, but it seems different then we discussed.
> > 
> > > +
> > > +	params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> > > +	if (!params)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	/*
> > > +	 * Only use version 1 when required (sigstruct > 4KB) for backward
> > > +	 * compatibility with P-SEAMLDR that lacks version 1 support.
> > > +	 */
> > > +	if (sig_size > SZ_4K)
> > > +		params->version = 1;
> > > +	else
> > > +		params->version = 0;
> > 
> > I'm a bit confused by this part. What does it mean to support old P-SEAMLDRs?
> 
> All current P-SEAMLDRs only support version 0. Version 1 is a planned
> extension but not yet implemented by any P-SEAMLDR. Always requiring
> version 1 just saves us a conditional but disables updates for all existing
> P-SEAMLDRs for no reason. I think it is not worthwhile.

Sorry, I still don't understand. Why does the kernel need to set this bit? It's
an opt-in for a format change? How many old TDX modules are we talking?

I think, connecting to the discussion in the later patches, that we should try
to keep the initial implementation as small as possible. Everything we add makes
things take a little bit longer. And while we spin new versions the kernel has
zero support. So I see that this is another non-critical enhancement.

> 
> > But also could it be:
> > 
> > params->version = sig_size > SZ_4K;
> 
> ok with me. I just wanted to make it match with "version 1" in the
> comment right above.
> 
> > > +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> > > +{
> > > +	const struct tdx_blob *blob = (const void *)data;
> > > +	int module_size, sig_size;
> > > +	const void *sig, *module;
> > > +
> > > +	/*
> > > +	 * Ensure the size is valid otherwise reading any field from the
> > > +	 * blob may overflow.
> > > +	 */
> > > +	if (size <= sizeof(struct tdx_blob) || size <= blob->offset_of_module)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (blob->version != TDX_BLOB_VERSION_1)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	/* Split the blob into a sigstruct and a module. */
> > > +	sig		= blob->data;
> > > +	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);
> > > +	module		= data + blob->offset_of_module;
> > > +	module_size	= size - blob->offset_of_module;
> > 
> > Did you consider just passing the tdx_blob into alloc_seamldr_params()?
> > Basically, this function checks the blob fields, then alloc_seamldr_params()
> > turns blob into  struct seamldr_params without checks. The way it is, the work
> > seems kind of spread around two functions with various checks.
> 
> Fine with merging them. 
> 

I wasn't suggesting to merge them. I was suggesting to have them each do a
dedicated thing.

> How about:
> 
> static struct seamldr_params *alloc_seamldr_params(const u8 *data, u32 size)
> {
> 	const struct tdx_blob *blob = (const void *)data;
> 	struct seamldr_params *params;
> 	int module_size, sig_size;
> 	const void *sig, *module;
> 	const u8 *ptr;
> 	int i;
> 
> 	/*
> 	 * Ensure the size is valid otherwise reading any field from the
> 	 * blob may overflow.
> 	 */
> 	if (size <= sizeof(struct tdx_blob))
> 		return ERR_PTR(-EINVAL);
> 
> 	if (blob->version != TDX_BLOB_VERSION_1)
> 		return ERR_PTR(-EINVAL);
> 	
> 	if (blob->length != size)
> 		return ERR_PTR(-EINVAL);
> 
> 	if (memcmp(blob->signature, "TDX-BLOB", 8))
> 		return ERR_PTR(-EINVAL);
> 
> 	if (blob->reserved0 || memchr_inv(blob->reserved1, 0, sizeof(blob->reserved1)))
> 		return ERR_PTR(-EINVAL);
> 
> 	/* Split the blob into a sigstruct and a module. */
> 	sig		= blob->data;
> 	sig_size	= blob->offset_of_module - sizeof(struct tdx_blob);
> 	module		= data + blob->offset_of_module;
> 	module_size	= size - blob->offset_of_module;
> 
> 	if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K || module_size <= 0)
> 		return ERR_PTR(-EINVAL);
> 
> 	if (sig_size > SEAMLDR_MAX_NR_SIG_4KB_PAGES * SZ_4K || sig_size <= 0)
> 		return ERR_PTR(-EINVAL);
> 
> 	params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> 	if (!params)
> 		return ERR_PTR(-ENOMEM);
> 
> 	/*
> 	 * Only use version 1 when required (sigstruct > 4KB) for backward
> 	 * compatibility with P-SEAMLDR that lacks version 1 support.
> 	 */
> 	params->version = sig_size > SZ_4K;
> 	params->scenario = SEAMLDR_SCENARIO_UPDATE;
> 
> 	ptr = sig;
> 	/*
> 	 * Assume sig/module base addresses and sizes are page-aligned.
> 	 * If violated, P-SEAMLDR will reject the update.
> 	 */
> 	for (i = 0; i < sig_size / SZ_4K; i++) {
> 		params->sigstruct_pa[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
> 		ptr += SZ_4K;
> 	}
> 
> 	params->num_module_pages = module_size / SZ_4K;
> 
> 	ptr = module;
> 	for (i = 0; i < params->num_module_pages; i++) {
> 		params->mod_pages_pa_list[i] = vmalloc_to_pfn(ptr) << PAGE_SHIFT;
> 		ptr += SZ_4K;
> 	}
> 
> 	return params;
> }


^ permalink raw reply

* Re: [PATCH v7 22/22] x86/virt/seamldr: Log TDX module update failures
From: Edgecombe, Rick P @ 2026-04-14 17:39 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4WCJdJ2MrGvkWX@intel.com>

On Tue, 2026-04-14 at 18:25 +0800, Chao Gao wrote:
> On Tue, Apr 14, 2026 at 04:04:05AM +0800, Edgecombe, Rick P wrote:
> > On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> > > Currently, there is no way to restore a TDX module from shutdown state to
> > > running state. This means if errors occur after a successful module
> > > shutdown, they are unrecoverable since the old module is gone but the new
> > > module isn't installed. All subsequent SEAMCALLs to the TDX module will
> > > fail, so TDs will be killed due to SEAMCALL failures.
> > > 
> > > Log a message to clarify that SEAMCALL errors are expected in this
> > > scenario. This ensures that after update failures, the first message in
> > > dmesg explains the situation rather than showing confusing call traces from
> > > various code paths.
> > 
> > Why is this patch at the end? Is it not valid initial behavior?
> 
> It's not strictly required for the update to function. It just improves
> debuggability by explaining expected SEAMCALL errors after a failure.
> 
> I placed it last so maintainers can easily drop it if they consider it
> non-essential, similar to the guidance in *.
> 
> *: https://lore.kernel.org/kvm/63082cd1-15ab-4aaf-83ad-f72d94b9bb8e@intel.com/

Dave says:
   If you don't need it to "turn the lights on", I say kick it out.
   
To me that says to drop non critical pieces. Not put it last with no explanation
that it is optional...

^ permalink raw reply

* Re: [PATCH v7 10/22] x86/virt/seamldr: Abort updates if errors occurred midway
From: Edgecombe, Rick P @ 2026-04-14 17:41 UTC (permalink / raw)
  To: Gao, Chao
  Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
	Huang, Kai, x86@kernel.org, Zhao, Yan Y,
	dave.hansen@linux.intel.com, kas@kernel.org, mingo@redhat.com,
	Weiny, Ira, pbonzini@redhat.com, Chatre, Reinette,
	Verma, Vishal L, nik.borisov@suse.com, seanjc@google.com,
	linux-kernel@vger.kernel.org, binbin.wu@linux.intel.com,
	Annapurve, Vishal, hpa@zytor.com, sagis@google.com,
	tony.lindgren@linux.intel.com, paulmck@kernel.org,
	tglx@kernel.org, yilun.xu@linux.intel.com,
	dan.j.williams@intel.com, bp@alien8.de
In-Reply-To: <ad4P9HZEOnC3wUH3@intel.com>

On Tue, 2026-04-14 at 17:59 +0800, Chao Gao wrote:
> The main point is correctness, not saving time.
> 
> If shutdown fails midway, the update is still recoverable — TDs can continue
> running. But if we proceed to seamldr.install anyway, it becomes destructive.
> Aborting early on shutdown failure preserves recoverability (this is needed to
> handle races between updates and TD build/migration).
> 
> If seamldr.install itself fails, it's already destructive, so aborting early
> there just saves time. But using the same abort mechanism for both keeps the
> error handling uniform.

If it's non-required for "turning the lights on" it seems aligned with Dave's
suggestion you highlighted to drop it from the series.

^ permalink raw reply

* Re: [PATCH v7 17/22] x86/virt/tdx: Avoid updates during update-sensitive operations
From: Edgecombe, Rick P @ 2026-04-14 19:58 UTC (permalink / raw)
  To: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org, Gao, Chao
  Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
	kas@kernel.org, Chatre, Reinette, seanjc@google.com,
	pbonzini@redhat.com, binbin.wu@linux.intel.com, Verma, Vishal L,
	nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
	tony.lindgren@linux.intel.com, Annapurve, Vishal,
	sagis@google.com, djbw@kernel.org, tglx@kernel.org,
	paulmck@kernel.org, hpa@zytor.com, bp@alien8.de,
	yilun.xu@linux.intel.com, x86@kernel.org
In-Reply-To: <20260331124214.117808-18-chao.gao@intel.com>

On Tue, 2026-03-31 at 05:41 -0700, Chao Gao wrote:
> A runtime TDX module update can conflict with TD lifecycle operations that
> are update-sensitive.
> 
> Today, update-sensitive operations include:
> 
> - TD build: TD measurement is accumulated across multiple
>   TDH.MEM.PAGE.ADD, TDH.MR.EXTEND, and TDH.MR.FINALIZE calls.

How exactly does this work? There is a new error I see:
TDX_INCOMPATIBLE_MRTD_CONTEXT. It gets returned by those seamcalls sometimes.

I think(?)... TDX_INCOMPATIBLE_MRTD_CONTEXT gets returned by those seamcalls if
the problematic type of tdx module update happens between TDH.MNG.INIT and
TDH.MR.FINALIZE. So if the compat flag is not passed into shutdown... then KVM
gets some new surprise error codes to handle deep in the MMU.

> 
> - TD migration: intermediate crypto state is saved/restored across
>   interrupted/resumed TDH.EXPORT.STATE.* and TDH.IMPORT.STATE.* flows.

We don't need to consider migration for this change.

> 
> If an update races TD build, for example, TD measurement can become
> incorrect and attestation can fail.
> 
> The TDX architecture exposes two approaches:
> 
> 1) Avoid updates during update-sensitive operations.
> 2) Detect incompatibility after update and recover.
> 
> Post-update detection (option #2) is not a good fit: as discussed in [1],
> future module behavior may expand update-sensitive operations in ways that
> make KVM ABIs unstable and will break userspace.
> 
> "Do nothing" is also not preferred: while it keeps kernel code simple, it
> lets the issue leak into the broader stack, where both detection and
> recovery require significantly more effort.

This subject has had a lot of debate (as linked below in the log), but the way
this is written leaves a lot of questions. "do nothing" is not an option it
says, but the code does just that when UPDATE_COMPAT_SENSITIVE is not supported.

> 
> So, use option #1. Specifically, request "avoid update-sensitive" behavior
> during TDX module shutdown and map the resulting failure to -EBUSY so
> userspace can distinguish an update race from other failures.
> 
> When the "avoid update-sensitive" feature isn't supported, proceed with
> updates. If a race occurs between module update and update-sensitive
> operations, failures happen at a later stage (e.g., incorrect TD
> measurements in attestation reports for TD build). Effectively, this
> means "let userspace update at their own risk".
> 

Above it says we can't just do nothing, we need the flag. And then this argues
that we can do nothing because we can rely on userspace to deal with the
issue... This log is maybe just trying to put a brave face on an imperfect
compromise?

So, while I don't want to re-open the debate, I'm not sure the patch
justification is going to pass scrutiny as is.

In the link [2], Dan says "Do not make Linux carry short lived one-off
complexity", and also "Do not include logic to disable updates, document the
expectation in the tool."

It seems this does not exclude the option to just to always pass the compat
flag. Basically assume that the TDX module will always support
UPDATE_COMPAT_SENSITIVE if it supports TDX module updates. Which I guess we
should expect should eventually be true.

In [2] Dan was also against checking the UPDATE_COMPAT_SENSITIVE feature0 bit to
gate the feature.


For the record, I don't like allowing the update without the compat bit set, and
my concern has nothing to do with userspace roles and responsibilities. Instead
it's because we are over budget on complexity for handling SEAMCALL errors
within KVM and this makes things worse to keep track of.

tdh_mem_page_add() does a KVM_BUG_ON() if it sees a non-busy error. Imagine
working on this code and considering if it is a valid KVM_BUG_ON()? After this
patch, the answer is...well sometimes. It depends on the previous modules
specific feature0 bits, an understanding on admins expectations, and the
behavior of some far away code in arch/x86. Gah.

Actually, the diff Dan objected to was checking and printing a specific helpful
error. Maybe he does not mind much more simply checking an extra bit in
tdx_supports_runtime_update()? Otherwise, I'd think to just unconditionally pass
UPDATE_COMPAT_SENSITIVE without checking for support. Essentially mandate that
it is always supported if TDX module update is supported.

>  Userspace can check if
> the feature is supported or not. The alternative of blocking updates
> entirely is rejected [2] as it introduces permanent kernel complexity to
> accommodate limitations in early TDX module releases that userspace can
> handle.
> 
> Note: this implementation is based on a reference patch by Vishal [3].
> Note2: moving "NO_RBP_MOD" is just to centralize bit definitions.
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Reviewed-by: Tony Lindgren <tony.lindgren@linux.intel.com>
> Link: https://lore.kernel.org/linux-coco/aQIbM5m09G0FYTzE@google.com/ # [1]
> Link: https://lore.kernel.org/kvm/699fe97dc212f_2f4a100b@dwillia2-mobl4.notmuch/ # [2]
> Link: https://lore.kernel.org/linux-coco/CAGtprH_oR44Vx9Z0cfxvq5-QbyLmy_+Gn3tWm3wzHPmC1nC0eg@mail.gmail.com/ # [3]
> ---



^ permalink raw reply

* Re: [PATCH v13 00/48] arm64: Support for Arm CCA in KVM
From: Alper Gun @ 2026-04-14 21:40 UTC (permalink / raw)
  To: Steven Price
  Cc: kvm, kvmarm, 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, Aneesh Kumar K . V, Emi Kisanuki,
	Vishal Annapurve
In-Reply-To: <20260318155413.793430-1-steven.price@arm.com>

On Wed, Mar 18, 2026 at 8:54 AM Steven Price <steven.price@arm.com> wrote:
>
> This series adds support for running protected VMs using KVM under the
> Arm Confidential Compute Architecture (CCA).
>
> New major version number! This now targets RMM v2.0-bet0[1]. And unlike
> for Linux this represents a significant change.
>
> RMM v2.0 brings with it the ability to configure the RMM to have the
> same page size as the host (so no more RMM_PAGE_SIZE and dealing with
> granules being different from host pages). It also introduces range
> based APIs for many operations which should be more efficient and
> simplifies the code in places.
>
> The handling of the GIC has changed, so the system registers are used to
> pass the GIC state rather than memory. This means fewer changes to the
> KVM code as it looks much like a normal VM in this respect.
>
> And of course the new uAPI introduced in the previous v12 posting is
> retained so that also remains simplified compared to earlier postings.
>
> The RMM support for v2.0 is still early and so this series includes a
> few hacks to ease the integration. Of note are that there are some RMM
> v1.0 SMCs added to paper over areas where the RMM implementation isn't
> quite ready for v2.0, and "SROs" (see below) are deferred to the final
> patch in the series.
>
> The PMU in RMM v2.0 requires more handling on the RMM-side (and
> therefore simplifies the implementation on Linux), but this isn't quite
> ready yet. The Linux side is implemented (but untested).
>
> PSCI still requires the VMM to provide the "target" REC for operations
> that affect another vCPU. This is likely to change in a future version
> of the specification. There's also a desire to force PSCI to be handled
> in the VMM for realm guests - this isn't implemented yet as I'm waiting
> for the dust to settle on the RMM interface first.
>
> Stateful RMI Operations
> -----------------------
>
> The RMM v2.0 spec brings a new concept of Stateful RMI Operations (SROs)
> which allow the RMM to complete an operation over several SMC calls and
> requesting/returning memory to the host. This has the benefit of
> allowing interrupts to be handled in the middle of an operation (by
> returning to the host to handle the interrupt without completing the
> operation) and enables the RMM to dynamically allocate memory for
> internal tracking purposes. One example of this is RMI_REC_CREATE no
> longer needs "auxiliary granules" provided upfront but can request the
> memory needed during the RMI_REC_CREATE operation.
>
> There are a fairly large number of operations that are defined as SROs
> in the specification, but current both Linux and RMM only have support
> for RMI_REC_CREATE and RMI_REC_DESTROY. There a number of TODOs/FIXMEs
> in the code where support is missing.
>
> Given the early stage support for this, the SRO handling is all confined
> to the final patch. This patch can be dropped to return to a pre-SRO
> state (albeit a mixture of RMM v1.0 and v2.0 APIs) for testing purposes.
>
> A future posting will reorder the series to move the generic SRO support
> to an early patch and will implement the proper support for this in all
> RMI SMCs.
>
> One aspect of SROs which is not yet well captured is that in some
> circumstances the Linux kernel will need to call an SRO call in a
> context where memory allocation is restricted (e.g. because a spinlock
> is held). In this case the intention is that the SRO will be cancelled,
> the spinlock dropped so the memory allocation can be completed, and then
> the SRO restarted (obviously after rechecking the state that the
> spinlock was protecting). For this reason the code stores the memory
> allocations within a struct rmi_sro_state object - see the final patch
> for more details.
>
> This series is based on v7.0-rc1. It is also available as a git
> repository:
>
> https://gitlab.arm.com/linux-arm/linux-cca cca-host/v13
>
>

Hi Steven,

I have a question regarding host kexec and kdump scenarios, and
whether there is any plan to make them work in this initial series.

Intel TDX and AMD SEV-SNP both have a firmware shutdown command that
is invoked during the kexec or panic code paths to safely bypass
hardware memory protections and boot into the new kernel. As far as
I know, there is no similar global teardown command available for
the RMM.

What is the roadmap for supporting both general kexec and
more specifically kdump (panic) scenarios with CCA?

Thanks,
Alper

^ permalink raw reply

* Re: [PATCH v7 17/22] x86/virt/tdx: Avoid updates during update-sensitive operations
From: Dan Williams @ 2026-04-14 21:43 UTC (permalink / raw)
  To: Edgecombe, Rick P, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
	Gao, Chao
  Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
	kas@kernel.org, Chatre, Reinette, seanjc@google.com,
	pbonzini@redhat.com, binbin.wu@linux.intel.com, Verma, Vishal L,
	nik.borisov@suse.com, mingo@redhat.com, Weiny, Ira,
	tony.lindgren@linux.intel.com, Annapurve, Vishal,
	sagis@google.com, djbw@kernel.org, tglx@kernel.org,
	paulmck@kernel.org, hpa@zytor.com, bp@alien8.de,
	yilun.xu@linux.intel.com, x86@kernel.org
In-Reply-To: <af7732a08fcce4a21ff14b2038acbb85200c6f34.camel@intel.com>

Edgecombe, Rick P wrote:
[..]
> > If an update races TD build, for example, TD measurement can become
> > incorrect and attestation can fail.
> > 
> > The TDX architecture exposes two approaches:
> > 
> > 1) Avoid updates during update-sensitive operations.
> > 2) Detect incompatibility after update and recover.
> > 
> > Post-update detection (option #2) is not a good fit: as discussed in [1],
> > future module behavior may expand update-sensitive operations in ways that
> > make KVM ABIs unstable and will break userspace.
> > 
> > "Do nothing" is also not preferred: while it keeps kernel code simple, it
> > lets the issue leak into the broader stack, where both detection and
> > recovery require significantly more effort.
> 
> This subject has had a lot of debate (as linked below in the log), but the way
> this is written leaves a lot of questions. "do nothing" is not an option it
> says, but the code does just that when UPDATE_COMPAT_SENSITIVE is not supported.

Right, but that is not the kernel's problem. If you run updates on
a module that does not support conflict detection, you get to keep the
pieces.

Like other cases of Linux not wanting to deal with the eccentricities of
early modules, just have userspace know about a minimum module version
where this protocol exists and accept the risk otherwise. No pressing
need to burden the kernel with carrying worry for early modules.

> > So, use option #1. Specifically, request "avoid update-sensitive" behavior
> > during TDX module shutdown and map the resulting failure to -EBUSY so
> > userspace can distinguish an update race from other failures.
> > 
> > When the "avoid update-sensitive" feature isn't supported, proceed with
> > updates. If a race occurs between module update and update-sensitive
> > operations, failures happen at a later stage (e.g., incorrect TD
> > measurements in attestation reports for TD build). Effectively, this
> > means "let userspace update at their own risk".
> > 
> 
> Above it says we can't just do nothing, we need the flag. And then this argues
> that we can do nothing because we can rely on userspace to deal with the
> issue... This log is maybe just trying to put a brave face on an imperfect
> compromise?

Not really, the log could be simplified to just say "module versions < X
are not safe for runtime update". In general if the kernel had a minimum
supported module version concept it could collect all of these early
deprecation conditions under one check.

> So, while I don't want to re-open the debate, I'm not sure the patch
> justification is going to pass scrutiny as is.
> 
> In the link [2], Dan says "Do not make Linux carry short lived one-off
> complexity", and also "Do not include logic to disable updates, document the
> expectation in the tool."
> 
> It seems this does not exclude the option to just to always pass the compat
> flag. Basically assume that the TDX module will always support
> UPDATE_COMPAT_SENSITIVE if it supports TDX module updates. Which I guess we
> should expect should eventually be true.

Right, assume a minimum module version.

> In [2] Dan was also against checking the UPDATE_COMPAT_SENSITIVE feature0 bit to
> gate the feature.

...because if it must be supported, why check?

> For the record, I don't like allowing the update without the compat bit set, and
> my concern has nothing to do with userspace roles and responsibilities. Instead
> it's because we are over budget on complexity for handling SEAMCALL errors
> within KVM and this makes things worse to keep track of.
> 
> tdh_mem_page_add() does a KVM_BUG_ON() if it sees a non-busy error. Imagine
> working on this code and considering if it is a valid KVM_BUG_ON()? After this
> patch, the answer is...well sometimes. It depends on the previous modules
> specific feature0 bits, an understanding on admins expectations, and the
> behavior of some far away code in arch/x86. Gah.

Why would it be variable? The user tried update on a module that the
kernel deemed unfit for update. "Doctor, it KVM_BUG_ON()s when I run
update".

> Actually, the diff Dan objected to was checking and printing a specific helpful
> error. Maybe he does not mind much more simply checking an extra bit in
> tdx_supports_runtime_update()? Otherwise, I'd think to just unconditionally pass
> UPDATE_COMPAT_SENSITIVE without checking for support. Essentially mandate that
> it is always supported if TDX module update is supported.

In the end all of the hand wringing we are doing is misplaced. The root
of the problem is the TDX Module claiming "runtime update supported" to
include these corner cases of "but we corrupt things if the update tries
to replace the crypto library at the wrong time".

That problem is also solvable by not classifying those updates as
runtime compatible, or defining a protocol to mitigate the collisions.

Neither of those was chosen. Instead the module implemented 2 additional
complications Linux chose one that fails update, other VMMs chose one
that maximizes updates at the cost of needing to restart TD
construction.

I think the changelog is a bit non-commital trying to be diplomatic
about the mess. Simply, Linux wanted the easy button, all runtime
updates are safe. Instead, module exported complexity and optionality.
KVM voted for one flavor of that optionality to accommodate the module
complexity.

The solution, make modules with the option KVM wants the min requirement
and move on.

^ permalink raw reply

* Re: [PATCH v7 17/22] x86/virt/tdx: Avoid updates during update-sensitive operations
From: Edgecombe, Rick P @ 2026-04-14 22:20 UTC (permalink / raw)
  To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, djbw@kernel.org,
	linux-kernel@vger.kernel.org, Gao, Chao
  Cc: Li, Xiaoyao, Huang, Kai, Zhao, Yan Y, dave.hansen@linux.intel.com,
	kas@kernel.org, seanjc@google.com, binbin.wu@linux.intel.com,
	Weiny, Ira, mingo@redhat.com, Verma, Vishal L,
	nik.borisov@suse.com, Chatre, Reinette, pbonzini@redhat.com,
	tony.lindgren@linux.intel.com, sagis@google.com,
	Annapurve, Vishal, hpa@zytor.com, tglx@kernel.org,
	paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
	x86@kernel.org
In-Reply-To: <69deb5032ff01_147c801004c@djbw-dev.notmuch>

On Tue, 2026-04-14 at 14:43 -0700, Dan Williams wrote:
> > tdh_mem_page_add() does a KVM_BUG_ON() if it sees a non-busy error. Imagine
> > working on this code and considering if it is a valid KVM_BUG_ON()? After
> > this
> > patch, the answer is...well sometimes. It depends on the previous modules
> > specific feature0 bits, an understanding on admins expectations, and the
> > behavior of some far away code in arch/x86. Gah.
> 
> Why would it be variable? The user tried update on a module that the
> kernel deemed unfit for update. "Doctor, it KVM_BUG_ON()s when I run
> update".

The objection is not an unprepared user having an issue. It's that this adds
burden to the TDX MMU developers who have to keep track of which errors are
valid and which are not. Doing that is maybe _the_ major challenge for
maintaining that code, that code is the trickiest of TDX KVM, and we have an
easy option here to not muddy it further.

> 
> > Actually, the diff Dan objected to was checking and printing a specific
> > helpful
> > error. Maybe he does not mind much more simply checking an extra bit in
> > tdx_supports_runtime_update()? Otherwise, I'd think to just unconditionally
> > pass
> > UPDATE_COMPAT_SENSITIVE without checking for support. Essentially mandate
> > that
> > it is always supported if TDX module update is supported.
> 
> In the end all of the hand wringing we are doing is misplaced. The root
> of the problem is the TDX Module claiming "runtime update supported" to
> include these corner cases of "but we corrupt things if the update tries
> to replace the crypto library at the wrong time".
> 
> That problem is also solvable by not classifying those updates as
> runtime compatible, or defining a protocol to mitigate the collisions.
> 
> Neither of those was chosen. Instead the module implemented 2 additional
> complications Linux chose one that fails update, other VMMs chose one
> that maximizes updates at the cost of needing to restart TD
> construction.

Totally, agree. We might be talking past each other. Let's not put burden on the
kernel developers when we can assume some TDX module behavior (Compat is always
supported) that we need, right?

> 
> I think the changelog is a bit non-commital trying to be diplomatic
> about the mess. Simply, Linux wanted the easy button, all runtime
> updates are safe. Instead, module exported complexity and optionality.
> KVM voted for one flavor of that optionality to accommodate the module
> complexity.

It doesn't pick a flavor. It tries to handle them both depending on TDX module
support. See:
 
+	if (tdx_sysinfo.features.tdx_features0 & TDX_FEATURES0_UPDATE_COMPAT)
+		args.rcx |= TDX_SYS_SHUTDOWN_AVOID_COMPAT_SENSITIVE;

So it sounds like you have no objection to only supporting the mode that is easy
(TDX_SYS_SHUTDOWN_AVOID_COMPAT_SENSITIVE). If we always pass this flag, then the
TDX MMU developers can have less details to keep in their heads.

I think Chao took this as an objection to unsupporting
!TDX_FEATURES0_UPDATE_COMPAT updates, when it really was an objection to the
kernel trying too hard to help the admin:
https://lore.kernel.org/kvm/69a0c3d24310_1cc5100d1@dwillia2-mobl4.notmuch/


> 
> The solution, make modules with the option KVM wants the min requirement
> and move on.


^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox