* Re: [PATCH v6 07/11] KVM: TDX: Allocate PAMT memory for TD and vCPU control structures
From: Yan Zhao @ 2026-07-09 3:18 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, kirill.shutemov@linux.intel.com, kas@kernel.org,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
In-Reply-To: <f686c26073761302b699c7374b668c933b0452e8.camel@intel.com>
On Thu, Jul 09, 2026 at 09:12:31AM +0800, Edgecombe, Rick P wrote:
> On Wed, 2026-07-08 at 17:13 +0800, Yan Zhao wrote:
> > On Mon, May 25, 2026 at 07:35:11PM -0700, Rick Edgecombe wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > >
> > > Use control page helpers for allocating and freeing TD control structures,
> > > such these operations can work for Dynamic PAMT.
> > >
> > > The TDX module tracks some state for each page of physical memory that it
> > > might use. It calls this state the PAMT. It includes separate state for
> > Nit: It records this state in the PAMT ?
>
> The text is trying to describe what PAMT is, so I'm not sure about this one.
Ok.
To me, PAMT stands for physical address metadata table, and the metadata the
table tracks is the "state" of a page.
Feel free to ignore the nit though :)
> > > each page size a physical page could be utilized at within the TDX module
> > > (1GB, 2MB, 4KB). In Dynamic PAMT, only the 4KB page size state is
> > > allocated dynamically. So the kernel must install PAMT backing for each 4KB
> > How about
> > "..., only the backend to hold the 4KB page size state is allocated
> > dynamically" ?
>
> How about:
> In Dynamic PAMT, only the 4KB page size backing is allocated dynamically.
LGTM.
> > > page before gifting it to the TDX module, and tear it down after the page
> > > is reclaimed.
> > How about
> > "So the kernel must check whether it is necessary to install/remove PAMT
> > backing for each 4KB page, and do so when necessary before gifting the page
> > to the TDX module or after it is reclaimed." ?
>
> The checking part is true, but is it needed to understand this patch? I think it
> just needs a background of what is happening. How about:
>
> So the kernel must install PAMT backing for any 4KB page being gifting to the
> TDX module, and tear down the backing when the associated gifted pages are
> reclaimed.
Hmm. I'm not sure if it would be confusing for people without sufficient
background, because the kernel only needs to install PAMT backing before the
gifting the first 4KB page.
How about:
So the kernel must ensure PAMT backing installed for any 4KB page being gifting
to the TDX module, and tear down the backing when all associated gifted pages
are reclaimed.
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Yan Zhao @ 2026-07-09 2:50 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, kirill.shutemov@linux.intel.com, kas@kernel.org,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
In-Reply-To: <9aa0694b9f0e2057808ed7c69ba6796a1d629bf7.camel@intel.com>
On Thu, Jul 09, 2026 at 08:33:40AM +0800, Edgecombe, Rick P wrote:
> Thanks for the review!
>
> On Wed, 2026-07-08 at 14:46 +0800, Yan Zhao wrote:
> > On Mon, May 25, 2026 at 07:35:09PM -0700, Rick Edgecombe wrote:
> > > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> > >
> > > tdx_pamt_get()/tdx_pamt_put() unconditionally add or remove Dynamic PAMT
> > > backing for the 2MB region covering the passed pfn. However, multiple
> > > callers can concurrently operate on 4KB pages that fall within the same
> > > 2MB region. When this happens only one Dynamic PAMT page pair needs to be
> > What "this" stands for is not clear and a comma is missing after "happens".
>
> Really this is not clear? The previous sentence is all about a senario being
> possible "multiple callers can concurrently operate on 4KB pages that fall
> within the same 2MB region". So I thought this would be clear.
Hmm, sorry for nitpicking.
But the previous sentence says "multiple callers can concurrently operate on 4KB
pages", where "operate" doesn't necessarily imply allocating pages. However, the
latter sentence assumes PAMT pages need to be installed. That's why I think "it"
is not clear.
> I could say "When that scenario happens..."? But trying to keep the word count
> down.
...
> > > +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> > How about
> > "Bump the refcount of the PAMT page pair for the given PFN and add the PAMT
> > page pair on the first reference." ?
>
> I think I see what you are getting at. How about this instead:
> Bump PAMT refcount for the given pfn and allocate PAMT backing if needed
Hmm, in this patch, "allocate PAMT backing" is always performed.
Only "add PAMT backing" is "if needed".
That's my reasoning. :)
And I can understand that "PAMT refcount" refers to the refcount of the PAMT
page pair. I just thought it would be clear to call this out explicitly.
No strong opinion though.
> > > static int tdx_pamt_get(kvm_pfn_t pfn)
> > > {
> > > struct page *pamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> > > + atomic_t *pamt_refcount;
> > > u64 tdx_status;
> > > int ret;
> > >
> > > @@ -2057,10 +2061,26 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > > if (ret)
> > > return ret;
> > >
> > > - tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > > - if (tdx_status != TDX_SUCCESS) {
> > > - ret = -EIO;
> > > - goto out_free;
> > > + pamt_refcount = tdx_find_pamt_refcount(pfn);
> > > +
> > > + scoped_guard(spinlock, &pamt_lock) {
> > > + /*
> > > + * If the pamt page is already added (i.e. refcount >= 1),
> > > + * then just increment the refcount.
> > > + */
> > > + if (atomic_read(pamt_refcount)) {
> > > + atomic_inc(pamt_refcount);
> > > + goto out_free;
> > > + }
> > > +
> > > + /* Try to add the pamt page and take the refcount 0->1. */
> > > + tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > > + if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS)) {
> > > + ret = -EIO;
> > > + goto out_free;
> > > + }
> > > +
> > > + atomic_set(pamt_refcount, 1);
> > > }
> > >
> > > return 0;
> > > @@ -2069,26 +2089,46 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > > return ret;
> > > }
> > >
> > > -/* Free PAMT memory for the given page */
> > > +/*
> > > + * Drop PAMT refcount for the given page and free PAMT memory if it is no
> > > + * longer needed.
> > How about:
> > "Drop the refcount of the PAMT page pair for the given PFN, and remove the
> > PAMT page pair if it is no longer needed." ?
>
> Matching the above:
>
> Drop PAMT refcount for the given pfn and free PAMT backing if needed
LGTM.
> > Another nit:
> > How about renaming the title from
> > "Handle concurrent callers in tdx_pamt_get/put()" to
> > "Handle concurrent calls to tdx_pamt_get/put()" ?
>
> I'm not seeing a benefit. Just because it's shorter?
I previously thought "callers in" sounded odd.
Feel free to ignore it if it is not :)
^ permalink raw reply
* Re: [PATCH 00/15] Device Evidence and Trust for PCI Security Protocol (TDISP)
From: Dan Williams (nvidia) @ 2026-07-09 2:45 UTC (permalink / raw)
To: Jason Gunthorpe, Dan Williams (nvidia)
Cc: linux-coco, linux-pci, driver-core, ankita, Aaron Tomlin,
Alexey Kardashevskiy, Alistair Francis, Aneesh Kumar K.V,
Arnd Bergmann, Bjorn Helgaas, Daniel Gomez, Danilo Krummrich,
Dexuan Cui, Donald Hunter, Greg Kroah-Hartman, Jakub Kicinski,
Luis Chamberlain, Lukas Wunner, Petr Pavlu, Rafael J. Wysocki,
Robin Murphy, Sami Tolvanen, Samuel Ortiz, Saravana Kannan,
Will Deacon, Xu Yilun
In-Reply-To: <20260708143153.GH118978@ziepe.ca>
Jason Gunthorpe wrote:
> On Tue, Jul 07, 2026 at 05:12:44PM -0700, Dan Williams (nvidia) wrote:
>
> > 1/ We previously discussed a use case to operate a device's private MMIO
> > while not allowing access to private memory (software encrypted NVME
> > with private MMIO [1]). Many of the following comments are based on
> > preserving this assumption so you can save some reading if we agree that
> > use case can be abandoned.
>
> force_dma_unencrypted() does not *prevent* device access to private
> memory and provides no security properties on its own. It's only
> purpose is to inform the DMA API what the HW restrictions are for
> doing DMA.
Right, to be clear, this mode's security properties come from never
asking the TSM to enable private DMA while the device is in RUN.
force_dma_unencrypted() has no security properties.
> The use case I had in mind relied on the vIOMMU to police access to
> the private memory. I think if you do T=1 and ADVERSARY you must have
> a vIOMMU or it is an illogical configuration.
Ok, the software encryption aspect of the suggestion was what threw me.
So we are saying, "no need, outside of defense in depth or data at rest
concerns, to include software encryption on top of an NVME device being
operated adversarially with T=1."
> So, IMHO there is no use case for a T=1 device to only use shared
> memory, I would abandon that combination.
>
> > 2/ The confirmation of the trust level and the enabling of DMA are
> > separated in time from setting the trust level and entering the RUN
> > state.
> >
> > All the archs separate the RUN step from the ENABLE DMA step, and the
> > implementation separates those steps in time.
>
> Yes, until the device is fully accepted the DMA has to be kept off. If
> you have to measure in the RUN state then we must have a second step
> after measurement completes, however we are measuring in LOCKED right?
Yes, measure while in LOCKED, but nothing strictly prevents taking
measurements after RUN.
> > echo tsm0 > $pdev/tsm/lock
> > cat $nonce | device-evidence dump $pdev
> > device-evidence validate $pdev $generation
>
> So the device is in LOCKED here, DMA remains off both because of
> LOCKED and becuase ENABLE DMA has not been done.
>
> > echo 1 > $pdev/tsm/accept
>
> And now it is RUN. So I don't see the issue with enabling DMA at the
> same time as gonig to RUN? (though defering it to driver probe would
> be a very nice touch as well)
It requires "accept" to consider the trust level. E.g. what does it mean
to do something like change requirements after accept?
echo full $pdev/trust
echo 1 > $pdev/tsm/accept
echo adversary > $pdev/trust
In that scenario this now adversary device may have been allowed to
operate without an enforcing vIOMMU, and needs to unlock the device to
correct that.
Delaying enable DMA means the implementation can say, "all trust and
security privileges are reconciled at device driver bind, and revoked at
unbind".
> With this API you can't set the devices to RUN and *then* collect the
> evidence, is that restriction OK?
The Enable DMA step requires evidence in the end. So it is more
implementation ergonomics driving whether "accept" includes Enable DMA
or defers to driver bind.
> > echo full > $pdev/trust
> > echo $pdev > $driver/bind
>
> Then we can set the trust and bind the driver, knowing that the T=1
> security property is in place.
Or, "accept" success tells you the full T=1 property is required for
"bind" to succeed.
> > The configuration window where $pdev/trust and $pdev/accept can be
> > dynamically changing should not be changing the force_dma_unencrypyted()
> > result if only because that value will mismatch the hardware state until
> > the next ->dma_configure() event.
>
> T=x cannot be changed while a driver is bound, we need to block
> that. Changing it will mess up all DMA and the DMA API won't work
> anymore.
>
> Thus force_dma_unencrypted() can only change when a driver is
> unbound.
>
> If a driver is unbound nothing ever reads force_dma_unencrypted() so
> it is safe to change it, thus we can just synchronize T=1 and
> force_dma_unencrypted() whenever T changes.
>
> The error case has to wait for the driver to unbind if userspace wants
> to go from ERROR->UNLOCKED. IMHO for RAS we are going to need a flow
> to logically go from ERROR->RUN, probably with the 'same device
> acceptance' I suggested earlier. In this case the DMAs will be halted,
> the devices gets back to T=1 and RUN, then the DMAs are turned on
> again. The driver remains in T=1 and force_dma_unencrypted() is always
> false.
Ideally this recovery does not need userspace round trips, but for that
the kernel needs SPDM transcript parsing. The measurement digest that
CCA provides is over the full previous transcript. A fresh nonce to
validate same device leads to the kernel need to validate the signature
with a cached key and compare the extracted measurement payload from the
previous transcript.
> > There are also buses and paravisors that may know that private-DMA is
> > enabled for a device by construction. In that case it is also a "trusted
> > to access" signal, and not a "required to access" signal.
>
> In this case they wire force_dma_unencrypted()=false.
Yeah, I just need a scheme where modular bus providers do not end up
compromising the private / unexported method of changing the flag that
force_dma_unencrypted() consumes.
> > My answer, given the concerns of drivers dangerously flipping the
> > force_dma_unencrypted() result at runtime was to place it in 'struct
> > device_private'
>
> Yes, this seems good idea in general
>
> > Otherwise it is confusing when 2 devices are at FULL trust, but
> > one has private memory access and the other does not. One is FULL the
> > other is FULL+.
>
> It is confusing when phrased like this, but this is also why I was
> suggesting below that a pure trust level may not be sufficient as we
> have quite a cross product of scenarios.
>
> If the user wants to operate a device without T=1 and without
> ADVERSARY then that's goofy, but technically the kernel doesn't really
> need to care? It can do it.
>
> IMHO a big question is how much do we want the kernel to nanny
> userspace by having options to double check its work?
>
> Either we trust the user to have checked all the policy conditions
> before writing /sys/trust, or we encode some policy conditions and
> tell the kernel so it can double-check prior to probe.
>
> At least if we omit the double check it can be fairly easy to add in
> later if it really was needed for some time of use reason.
As long as IOMMU presence can be enumerated prior to acceptance, T=1
always includes private DMA, and the T=1 status is tracked independent
of the trust level then yes, the cross product can be avoided.
> > The question is whether these requirements belong on a central device
> > trust mechanism bitmap or should be pushed out to other ABI.
>
> Yes, for sure, I don't have a good sense what is better.
>
> > For
> > example, if the presence of the secure IOMMU is enumerated
> > after the device is in the LOCKED state then userspace policy can know
> > what it is getting into without needing to tell the kernel.
>
> Yes, I think for almost all of this we could push it out to userspace
> and have knobs all over. Like IOMMU already has a knob to switch to
> DMA mode, we could add an IOMMU specific trust level to set ADVERSARY
> also if we go in this direction.
>
> But, I also do really like the idea of a central knob that was fairly
> general and simple DISABLE/ADVERSARY/FULL which all sorts of things
> can hang off. Including IOMMU and the drivers themselves that may
> change operation in ADVERSARY mode.
Yes, even if some of the ancillary requirements move to other ABI the
central question of DISABLE/ADVERSARY/FULL stays common.
> That means they need simple general definitions for what they are, and
> I really like this ADVERSARY naming vs trust as it makes it very clear
> what the device disposition intention is.
I will do the "device_adversarial()" conversion throughout.
'struct device' grows some "request" policy for trust and T=1 that can
be changed by modules etc. 'struct device_private' grows the operational
trust level stable under device_lock() and a new flag to reflect T=1.
^ permalink raw reply
* Re: [PATCH v6 09/11] KVM: TDX: Get/put PAMT pages when (un)mapping private memory
From: Edgecombe, Rick P @ 2026-07-09 1:22 UTC (permalink / raw)
To: Gao, Chao
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, kirill.shutemov@linux.intel.com,
x86@kernel.org
In-Reply-To: <ak4Lv+CQIG1P3k/f@intel.com>
On Wed, 2026-07-08 at 16:35 +0800, Chao Gao wrote:
> On Mon, May 25, 2026 at 07:35:13PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > Add Dynamic PAMT support to KVM's S-EPT MMU by "getting" a PAMT page when
> > adding guest memory (PAGE.ADD or PAGE.AUG), and "putting" the page when
> > removing guest memory (PAGE.REMOVE).
> >
> > To access the per-vCPU PAMT caches without plumbing @vcpu throughout the
> > TDP MMU, begrudgingly use kvm_get_running_vcpu() to get the vCPU, and bug
> > the VM if KVM attempts to set an S-EPT leaf without an active vCPU. KVM
> > only supports creating _new_ mappings in page (pre)fault paths, all of
> > which require an active vCPU.
> >
> > The PAMT memory holds metadata for TDX-protected memory. With Dynamic
> > PAMT, PAMT_4K is allocated on demand. The kernel supplies the TDX module
> > with a few pages that cover 2M of host physical memory.
> >
> > Releases are balanced via tdx_pamt_put(): every control-page free goes
> > through tdx_free_control_page(), and guest data pages are put directly on
> > the successful tdh_mem_page_remove() path and in the
> > tdx_mem_page_add/aug() error path.
> >
> > Assisted-by: Sashiko:claude-opus-4-6 GitHub Copilot:claude-opus-4-6 Claude:claude-opus-4-7
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Co-developed-by: Sean Christopherson <seanjc@google.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > ---
> > v6:
> > - Don't have topup op take a min param (Yan, Sean)
>
> The topup_external_cache x86 op still takes int min_nr_spts. Did you
> forget to remove it, or am I misreading this changelog?
There was some discussion on v5 to drop the "min" param, but it got added back
in later discussion and renamed min_nr_spts:
https://lore.kernel.org/lkml/aYprxnSHKHUtk7pt@google.com/#t
^ permalink raw reply
* Re: [PATCH v6 07/11] KVM: TDX: Allocate PAMT memory for TD and vCPU control structures
From: Edgecombe, Rick P @ 2026-07-09 1:12 UTC (permalink / raw)
To: Zhao, Yan Y
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, kirill.shutemov@linux.intel.com, kas@kernel.org,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
In-Reply-To: <ak4UyN4fnIZasWM8@yzhao56-desk.sh.intel.com>
On Wed, 2026-07-08 at 17:13 +0800, Yan Zhao wrote:
> On Mon, May 25, 2026 at 07:35:11PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > Use control page helpers for allocating and freeing TD control structures,
> > such these operations can work for Dynamic PAMT.
> >
> > The TDX module tracks some state for each page of physical memory that it
> > might use. It calls this state the PAMT. It includes separate state for
> Nit: It records this state in the PAMT ?
The text is trying to describe what PAMT is, so I'm not sure about this one.
>
> > each page size a physical page could be utilized at within the TDX module
> > (1GB, 2MB, 4KB). In Dynamic PAMT, only the 4KB page size state is
> > allocated dynamically. So the kernel must install PAMT backing for each 4KB
> How about
> "..., only the backend to hold the 4KB page size state is allocated
> dynamically" ?
How about:
In Dynamic PAMT, only the 4KB page size backing is allocated dynamically.
>
> > page before gifting it to the TDX module, and tear it down after the page
> > is reclaimed.
> How about
> "So the kernel must check whether it is necessary to install/remove PAMT
> backing for each 4KB page, and do so when necessary before gifting the page
> to the TDX module or after it is reclaimed." ?
The checking part is true, but is it needed to understand this patch? I think it
just needs a background of what is happening. How about:
So the kernel must install PAMT backing for any 4KB page being gifting to the
TDX module, and tear down the backing when the associated gifted pages are
reclaimed.
>
> > TD-scoped control pages (TDR, TDCS) and vCPU-scoped control pages (TDVPR,
> > TDCX) are all handed to the TDX module at 4KB page size and are therefore
> > subject to this requirement. Replace the raw alloc_page()/__free_page()
> > calls for these pages with tdx_alloc/free_control_page().
> >
> > Switching between special Dynamic PAMT operations or normal page
> > alloc/free operations is handled internally in
> > tdx_alloc/free_control_page(). So don't check for Dynamic PAMT around these
> > calls. Just call them unconditionally. Similarly, drop the NULL checks
> > before freeing, as tdx_free_control_page() handles NULL internally.
> >
> > No functional change intended when Dynamic PAMT is not in use.
>
> Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>
>
^ permalink raw reply
* Re: [PATCH v6 06/11] x86/virt/tdx: Optimize tdx_pamt_get/put()
From: Edgecombe, Rick P @ 2026-07-09 0:44 UTC (permalink / raw)
To: Zhao, Yan Y
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, kirill.shutemov@linux.intel.com, kas@kernel.org,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
In-Reply-To: <ak4OUC9fg6CJ/06Y@yzhao56-desk.sh.intel.com>
On Wed, 2026-07-08 at 16:46 +0800, Yan Zhao wrote:
> The optimization LGTM.
Thanks for checking it.
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Edgecombe, Rick P @ 2026-07-09 0:33 UTC (permalink / raw)
To: Zhao, Yan Y
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, kirill.shutemov@linux.intel.com, kas@kernel.org,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
In-Reply-To: <ak3ySL89u5+1oCZB@yzhao56-desk.sh.intel.com>
Thanks for the review!
On Wed, 2026-07-08 at 14:46 +0800, Yan Zhao wrote:
> On Mon, May 25, 2026 at 07:35:09PM -0700, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > tdx_pamt_get()/tdx_pamt_put() unconditionally add or remove Dynamic PAMT
> > backing for the 2MB region covering the passed pfn. However, multiple
> > callers can concurrently operate on 4KB pages that fall within the same
> > 2MB region. When this happens only one Dynamic PAMT page pair needs to be
> What "this" stands for is not clear and a comma is missing after "happens".
Really this is not clear? The previous sentence is all about a senario being
possible "multiple callers can concurrently operate on 4KB pages that fall
within the same 2MB region". So I thought this would be clear.
I could say "When that scenario happens..."? But trying to keep the word count
down.
>
> > installed to cover the 2MB range. And when one page is freed, the Dynamic
> > PAMT backing cannot be freed until all pages in the range are no longer in
> > use. Make the helpers handle these races internally.
> >
> > Use the per-2MB refcounts from previous changes to track how many 4KB
> > pages are in use within each region. Gate the actual Dynamic PAMT add and
> > remove on refcount transitions (0->1 and 1->0). Serialize the refcount
> > check and SEAMCALL with a global spinlock so the read-decide-act sequence
> > is atomic. This also avoids TDX module BUSY errors, as Dynamic PAMT add
> > and remove SEAMCALLs take an internal TDX module locks at 2MB granularity,
> "an internal TDX module locks" --> "internal TDX module locks"?
Yep, thanks.
>
> > so simultaneous attempts on the same region would conflict.
> How about:
> "This also avoids TDX module BUSY errors, as Dynamic PAMT add
> and remove SEAMCALLs take internal TDX module locks for the 2MB ranges of
> the specified PFN and the PAMT page pair PFNs, so simultaneous attempts on
> the same 2MB ranges of the PFNs would conflict." ?
Hmm, yea.
>
>
> > The lock is global and heavyweight. Use simple conditional logic to keep
> > correctness obvious. This will be optimized in a later change.
> >
> > Assisted-by: GitHub Copilot:claude-opus-4-6 Claude:claude-opus-4-7
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Co-developed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > ---
> > v6:
> > - Split from "x86/virt/tdx: Add tdx_alloc/free_control_page() helpers"
> > - Return 0 instead of ret to be clearer (Binbin)
> > - Clarify log (Nikolay)
> > - Justify why the patch is not optimized in response to comments by
> > (Nikolay)
> > - Move tdx_find_pamt_refcount() to faciliate patch re-order
> > - Adjustments from dropping error helper patches
> > - Log tweaks
> > ---
> > arch/x86/virt/vmx/tdx/tdx.c | 72 ++++++++++++++++++++++++++++---------
> > 1 file changed, 56 insertions(+), 16 deletions(-)
> >
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 6658a6be6697c..50333eb96efa6 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -2043,10 +2043,14 @@ static u64 tdh_phymem_pamt_remove(kvm_pfn_t pfn, struct page **pamt_pages)
> > return 0;
> > }
> >
> > -/* Allocate PAMT memory for the given page */
> > +/* Serializes adding/removing PAMT memory */
> > +static DEFINE_SPINLOCK(pamt_lock);
> > +
> > +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> How about
> "Bump the refcount of the PAMT page pair for the given PFN and add the PAMT
> page pair on the first reference." ?
I think I see what you are getting at. How about this instead:
Bump PAMT refcount for the given pfn and allocate PAMT backing if needed
>
> > static int tdx_pamt_get(kvm_pfn_t pfn)
> > {
> > struct page *pamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT];
> > + atomic_t *pamt_refcount;
> > u64 tdx_status;
> > int ret;
> >
> > @@ -2057,10 +2061,26 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > if (ret)
> > return ret;
> >
> > - tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > - if (tdx_status != TDX_SUCCESS) {
> > - ret = -EIO;
> > - goto out_free;
> > + pamt_refcount = tdx_find_pamt_refcount(pfn);
> > +
> > + scoped_guard(spinlock, &pamt_lock) {
> > + /*
> > + * If the pamt page is already added (i.e. refcount >= 1),
> > + * then just increment the refcount.
> > + */
> > + if (atomic_read(pamt_refcount)) {
> > + atomic_inc(pamt_refcount);
> > + goto out_free;
> > + }
> > +
> > + /* Try to add the pamt page and take the refcount 0->1. */
> > + tdx_status = tdh_phymem_pamt_add(pfn, pamt_pages);
> > + if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS)) {
> > + ret = -EIO;
> > + goto out_free;
> > + }
> > +
> > + atomic_set(pamt_refcount, 1);
> > }
> >
> > return 0;
> > @@ -2069,26 +2089,46 @@ static int tdx_pamt_get(kvm_pfn_t pfn)
> > return ret;
> > }
> >
> > -/* Free PAMT memory for the given page */
> > +/*
> > + * Drop PAMT refcount for the given page and free PAMT memory if it is no
> > + * longer needed.
> How about:
> "Drop the refcount of the PAMT page pair for the given PFN, and remove the
> PAMT page pair if it is no longer needed." ?
Matching the above:
Drop PAMT refcount for the given pfn and free PAMT backing if needed
>
> > + */
> > static void tdx_pamt_put(kvm_pfn_t pfn)
> > {
> > struct page *pamt_pages[TDX_DPAMT_ENTRY_PAGE_CNT] = {};
> > + atomic_t *pamt_refcount;
> > u64 tdx_status;
> >
> > if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > return;
> >
> > - tdx_status = tdh_phymem_pamt_remove(pfn, pamt_pages);
> > + pamt_refcount = tdx_find_pamt_refcount(pfn);
> >
> > - /*
> > - * Don't free pamt_pages as it could hold garbage when
> > - * tdh_phymem_pamt_remove() fails. Don't panic/BUG_ON(), as
> > - * there is no risk of data corruption, but do yell loudly as
> > - * failure indicates a kernel bug, memory is being leaked, and
> > - * the dangling PAMT entry may cause future operations to fail.
> > - */
> > - if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS))
> > - return;
> > + scoped_guard(spinlock, &pamt_lock) {
> > + /*
> > + * If the there are more than 1 references on the pamt page,
> s/If the/If
> s/references/reference
Yep, thanks.
>
> > + * don't remove it yet. Just decrement the refcount.
> > + */
> > + if (atomic_read(pamt_refcount) > 1) {
> > + atomic_dec(pamt_refcount);
> > + return;
> > + }
> > +
> > + /* Try to remove the pamt page and take the refcount 1->0. */
> > + tdx_status = tdh_phymem_pamt_remove(pfn, pamt_pages);
> > +
> > + /*
> > + * Don't free pamt_pages as it could hold garbage when
> > + * tdh_phymem_pamt_remove() fails. Don't panic/BUG_ON(), as
> > + * there is no risk of data corruption, but do yell loudly as
> > + * failure indicates a kernel bug, memory is being leaked, and
> > + * the dangling PAMT entry may cause future operations to fail.
> > + */
> > + if (WARN_ON_ONCE(tdx_status != TDX_SUCCESS))
> > + return;
> > +
> > + atomic_set(pamt_refcount, 0);
> > + }
> >
> > free_pamt_array(pamt_pages);
> > }
>
> Another nit:
> How about renaming the title from
> "Handle concurrent callers in tdx_pamt_get/put()" to
> "Handle concurrent calls to tdx_pamt_get/put()" ?
I'm not seeing a benefit. Just because it's shorter?
>
>
> Reviewed-by: Yan Zhao <yan.y.zhao@intel.com>
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Edgecombe, Rick P @ 2026-07-08 23:38 UTC (permalink / raw)
To: seanjc@google.com, Fang, Peter
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Hansen, Dave, dave.hansen@linux.intel.com,
baolu.lu@linux.intel.com, Hunter, Adrian, kas@kernel.org,
linux-kernel@vger.kernel.org, Xu, Yilun,
tony.lindgren@linux.intel.com, Mehta, Sohil, djbw@kernel.org,
Duan, Zhenzhong, Maloor, Kishen, yilun.xu@linux.intel.com,
x86@kernel.org
In-Reply-To: <ak7PHTGEuxAZCvXw@google.com>
On Wed, 2026-07-08 at 15:28 -0700, Sean Christopherson wrote:
> > I'll just add that this design decision comes from the fact that we're
> > looking at different kinds of future attestation stuff. Some of it is
> > quite complex (like post-quantum crypto). Some of it may even require
> > more complicated interactions with the S3M. Hiding these details allows
> > all this crypto stuff to look like just another TDX module release.
>
> IMO, that's not a good thing. Burying more code and more complexity in what
> is supposed to be a minimal TCB doesn't seem like a "win". I can certainly
> appreciate that interacting with the S3M microcontroller isn't something we
> want in KVM, but that's why drivers/ exists.
This is an interesting take and makes me think even more that we need to be
socializing the TDX arch stuff a lot more.
>
> > The host kernel shouldn't be burdened with attestation intricacies,
>
> Oh, but the kernel has to be, at least to the point where the kernel has
> enough information to guarantee backwards compatibility, enumerate choices,
> etc. If you're saying that the TDX module will always generate reports that
> are compatible across TDX Module updates, then we probably don't have anything
> to worry about, but it seems unlikely everything can Just Work while also
> updating the core crypto algorithms and whatnot.
There are absolutely some ABI sensitivity here that need to be considered, and
unfortunately the analysis from the internal debates did not make into the
public presentation of the feature. In fact the whole DICE part of this series
is missing a lot of needed context.
I think I mentioned that we want to come to talk about migration in PUCK, it
might help to bring this one in too. Despite being small, it actually took quite
a bit for us to wrap our heads around it. Would you want to try that? Or we
could fill out the logs and coverletter in v3. Or we could just try to slowly
walk through it all here. I was thinking a call may shorten the loops.
^ permalink raw reply
* Re: [PATCH v6 04/11] x86/virt/tdx: Allocate ref counts for Dynamic PAMT memory
From: Edgecombe, Rick P @ 2026-07-08 23:21 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, pbonzini@redhat.com,
linux-kernel@vger.kernel.org, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, Annapurve, Vishal,
tglx@kernel.org, Mehta, Sohil, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <6ab50a8c-d502-4d51-b037-3af6c4b4083e@intel.com>
On Wed, 2026-07-08 at 13:49 -0700, Sohil Mehta wrote:
> On 5/25/2026 7:35 PM, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > The PAMT memory holds metadata for all possible TDX protected memory. Each
> > physical address range is covered by PAMT entries at three levels (1GB,
> > 2MB, 4KB). With Dynamic PAMT, the 4KB range of PAMT is allocated on
> > demand. The kernel supplies the TDX module with page pairs to store the
> > 4KB entries, which cover 2MB of host physical memory. The kernel must
> > provide this page pair before using pages from the range for TDX. If this
> > is not done, SEAMCALLs that give the pages to be protected by the TDX module
> > will fail.
> >
> > Allocate reference counters for every 2MB range to track TDX memory usage.
> > This can be used to handle concurrent get/put callers, in order to
> > accurately determine when the dynamic 4KB level of Dynamic PAMT needs to
> > be allocated and when it can be freed.
> >
> > This allocation will currently consume 2 MB for every 1 TB of address
> > space from 0 to max_pfn. The allocation size will depend on how the RAM is
> > physically laid out. In a worst case scenario where the entire 52-bit
> > address space is covered this would be 8GB. Then the DPAMT refcount
> > allocations could hypothetically cause the savings from Dynamic PAMT to go
> > negative on exotic platforms with sparse, small amounts of memory.
> >
>
> ...
>
> > +/*
> > + * On a machine with Dynamic PAMT, the kernel maintains a reference counter
> > + * for every 2M range.
>
> Commit log says every 2MB range.
Yea, I'll unify to MB, TB, KB.
>
> The counter indicates how many users there are for
> > + * the PAMT memory of the 2M range. The kernel allocates PAMT refcounts at
> > + * initialization.
> > + */
> > +static atomic_t *pamt_refcounts;
> > +
> > /* All TDX-usable memory regions. Protected by mem_hotplug_lock. */
> > static LIST_HEAD(tdx_memlist);
> >
> > @@ -254,6 +263,43 @@ static struct syscore tdx_syscore = {
> > .ops = &tdx_syscore_ops,
> > };
> >
> > +/*
> > + * Allocate PAMT reference counters for all physical memory.
> > + *
> > + * It consumes 2MiB for every 1TiB of physical memory.
>
> Commit log says 2MB and 1TB. I would make both consistent.
Yea, different authors for some of this text.
>
> > + */
> > +static int init_pamt_refcounts(void)
> > +{
> > + size_t size = DIV_ROUND_UP(max_pfn, PTRS_PER_PTE) * sizeof(*pamt_refcounts);
> > +
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return 0;
> > +
> > + pamt_refcounts = __vmalloc(size, GFP_KERNEL | __GFP_ZERO);
>
> vzalloc()?
Totally.
>
> > + if (!pamt_refcounts)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static void free_pamt_refcounts(void)
> > +{
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return;
> > +
> > + vfree(pamt_refcounts);
> > + pamt_refcounts = NULL;
> > +}
> > +
> > +/* Find PAMT refcount for a given physical address */
>
> This comment is probably not that useful. The function name is
> descriptive by itself.
I'll delete it.
>
> > +static atomic_t * __maybe_unused tdx_find_pamt_refcount(unsigned long pfn)
> > +{
> > + /* Find which PMD a PFN is in. */
> > + unsigned long index = pfn >> (PMD_SHIFT - PAGE_SHIFT);
> > +
> > + return &pamt_refcounts[index];
> > +}
> > +
^ permalink raw reply
* Re: [PATCH v6 03/11] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Edgecombe, Rick P @ 2026-07-08 22:54 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, pbonzini@redhat.com,
linux-kernel@vger.kernel.org, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, Annapurve, Vishal,
tglx@kernel.org, Mehta, Sohil, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <7d39a92a-0555-41cb-91dd-96dace318dcf@intel.com>
On Tue, 2026-07-07 at 20:50 -0700, Sohil Mehta wrote:
> How about?
>
> x86/virt/tdx: Add tdx_{alloc,free}_control_page() helpers
Sure.
>
> On 5/25/2026 7:35 PM, Rick Edgecombe wrote:
> > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> >
> > Add helpers to use when allocating or preparing pages that are handed to
> > the TDX-Module for use as control/S-EPT pages, and thus need Dynamic PAMT
> > adjustments.
> >
> > The TDX module tracks some state for each page of physical memory that it
> > might use. It calls this state the PAMT. It includes separate state for
> > each page size a physical page could be utilized at within the TDX module
> > (1GB, 2MB, 4KB). In Dynamic PAMT, only the 4KB page size state is
> > allocated dynamically. So for pages that TDX will use as 2MB physically
> > contiguous pages, Dynamic PAMT backing is not needed.
>
> I lost the continuation in the last sentence. Why does it only talk
> about 2MB if only 4KB is dynamically allocated. What about 1GB?
>
> (Probably due to my lack of TDX knowledge)
> Similarly, why do these functions only refer to 2MB only and not 1GB?
>
> pamt_2mb_arg(), tdh_phymem_pamt_add(), tdh_phymem_pamt_remove().
We only need to dynamically allocate the 4k level of the PAMT. This is the
detail we discussed in patch 2. There it was necessary to explain why we are
changing the 4K level of the PAMT to be a bitmap thing, and not the others. Here
I think it is useful to understand why the helpers are all for 4KB page size of
host memory. I think it's clearer actually with just the last line removed:
The TDX module tracks some state for each page of physical memory that it
might use. It calls this state the PAMT. It includes separate state for
each page size a physical page could be utilized at within the TDX module
(1GB, 2MB, 4KB). In Dynamic PAMT, only the 4KB page size state is
allocated dynamically.
KVM will need to hand pages to the TDX module that it will use at 4KB
granularity. So these pages will need Dynamic PAMT backing added before
they are used by the TDX module, and removed afterwards.
And, I'll address the trimmed style comments.
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Sean Christopherson @ 2026-07-08 22:28 UTC (permalink / raw)
To: Peter Fang
Cc: Dave Hansen, Rick P Edgecombe, djbw@kernel.org,
kvm@vger.kernel.org, linux-coco@lists.linux.dev, Xiaoyao Li,
dave.hansen@linux.intel.com, baolu.lu@linux.intel.com,
Adrian Hunter, kas@kernel.org, tony.lindgren@linux.intel.com,
Yilun Xu, linux-kernel@vger.kernel.org, Sohil Mehta,
Zhenzhong Duan, Kishen Maloor, yilun.xu@linux.intel.com,
x86@kernel.org
In-Reply-To: <20260708220915.GI2169894@pedri>
On Wed, Jul 08, 2026, Peter Fang wrote:
> On Wed, Jul 08, 2026 at 01:47:29PM -0700, Dave Hansen wrote:
> > On 7/6/26 10:57, Sean Christopherson wrote:
> > > What is "the S3M" though? Is it a separate chip a la AMD's PSP/ASP? Is it a
> > > per-package thing? Per-core?
> > I'll give you my rough software guy mental model of what it is: Each
> > package has its own S3M. They are microcontrollers which are discrete
> > from the CPU cores. Each S3M gets some CPU physical address space routed
> > over to it.
> >
> > > How is it accessed, and what are the "rules" for for those
> > > accesses? What types of latencies are we looking at?
> >
> > As far as I know, the latency for one round trip to/from S3M is on the
> > order of a "real" device. It has a physical address and when the OS
> > wants to talk to it, those addresses are mapped with ioremap(). It's
> > similar to any modern I/O device control plane. Note, though, that for
> > TDX, there's no ioremap() because the I/O is hidden in the TDX module.
> >
> > The real overhead comes because the I/O window is essentially 4 bytes
> > wide (IIRC) and all the data that comes in and out of it has to be
> > squeezed through that window. It reminds me of a UART, but with a
> > slightly more arcane interface.
> >
> > For TDX, though, the craziness is mostly hidden in the TDX module.
>
> I'll just add that this design decision comes from the fact that we're
> looking at different kinds of future attestation stuff. Some of it is
> quite complex (like post-quantum crypto). Some of it may even require
> more complicated interactions with the S3M. Hiding these details allows
> all this crypto stuff to look like just another TDX module release.
IMO, that's not a good thing. Burying more code and more complexity in what is
supposed to be a minimal TCB doesn't seem like a "win". I can certainly
appreciate that interacting with the S3M microcontroller isn't something we want
in KVM, but that's why drivers/ exists.
> The host kernel shouldn't be burdened with attestation intricacies,
Oh, but the kernel has to be, at least to the point where the kernel has enough
information to guarantee backwards compatibility, enumerate choices, etc. If
you're saying that the TDX module will always generate reports that are compatible
across TDX Module updates, then we probably don't have anything to worry about,
but it seems unlikely everything can Just Work while also updating the core crypto
algorithms and whatnot.
> and the TDX module should just make sure the new module "works".
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Peter Fang @ 2026-07-08 22:09 UTC (permalink / raw)
To: Dave Hansen
Cc: Sean Christopherson, Rick P Edgecombe, djbw@kernel.org,
kvm@vger.kernel.org, linux-coco@lists.linux.dev, Xiaoyao Li,
dave.hansen@linux.intel.com, baolu.lu@linux.intel.com,
Adrian Hunter, kas@kernel.org, tony.lindgren@linux.intel.com,
Yilun Xu, linux-kernel@vger.kernel.org, Sohil Mehta,
Zhenzhong Duan, Kishen Maloor, yilun.xu@linux.intel.com,
x86@kernel.org
In-Reply-To: <08c9bf2f-75be-4244-b99c-153ec1f604ca@intel.com>
On Wed, Jul 08, 2026 at 01:47:29PM -0700, Dave Hansen wrote:
> On 7/6/26 10:57, Sean Christopherson wrote:
> > What is "the S3M" though? Is it a separate chip a la AMD's PSP/ASP? Is it a
> > per-package thing? Per-core?
> I'll give you my rough software guy mental model of what it is: Each
> package has its own S3M. They are microcontrollers which are discrete
> from the CPU cores. Each S3M gets some CPU physical address space routed
> over to it.
>
> > How is it accessed, and what are the "rules" for for those
> > accesses? What types of latencies are we looking at?
>
> As far as I know, the latency for one round trip to/from S3M is on the
> order of a "real" device. It has a physical address and when the OS
> wants to talk to it, those addresses are mapped with ioremap(). It's
> similar to any modern I/O device control plane. Note, though, that for
> TDX, there's no ioremap() because the I/O is hidden in the TDX module.
>
> The real overhead comes because the I/O window is essentially 4 bytes
> wide (IIRC) and all the data that comes in and out of it has to be
> squeezed through that window. It reminds me of a UART, but with a
> slightly more arcane interface.
>
> For TDX, though, the craziness is mostly hidden in the TDX module.
I'll just add that this design decision comes from the fact that we're
looking at different kinds of future attestation stuff. Some of it is
quite complex (like post-quantum crypto). Some of it may even require
more complicated interactions with the S3M. Hiding these details allows
all this crypto stuff to look like just another TDX module release. The
host kernel shouldn't be burdened with attestation intricacies, and the
TDX module should just make sure the new module "works".
>
> > What else uses the S3M? Do we have to worry about contending with
> > non-TDX usage?
> There _are_ different users of S3M. But each of them should get their
> own I/O address and S3M firmware has to handle talking to those
> different users at the same time. The TDX I/O window is owned
> exclusively by the TDX module.
>
> So, while S3M has and long and growing list of jobs, the random software
> (like the host kernel) poking at one I/O window doesn't have to know
> about the other piece of software (the TDX module) poking at another.
>
> I'm sure I got a detail or two wrong in there, so folks that know this
> better: please correct me. But I think that's a halfway-decent 10,000ft
> view.
This is pretty spot on based on my understanding. Thanks Dave :)
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Sean Christopherson @ 2026-07-08 21:37 UTC (permalink / raw)
To: Xu Yilun
Cc: x86, kvm, linux-coco, linux-kernel, djbw, kas, rick.p.edgecombe,
yilun.xu, xiaoyao.li, sohil.mehta, adrian.hunter, kishen.maloor,
tony.lindgren, peter.fang, baolu.lu, zhenzhong.duan, dave.hansen,
dave.hansen
In-Reply-To: <20260618081355.3253581-17-yilun.xu@linux.intel.com>
On Thu, Jun 18, 2026, Xu Yilun wrote:
> void __init tdx_early_init(void);
> diff --git a/arch/x86/kvm/vmx/tdx.h b/arch/x86/kvm/vmx/tdx.h
> index ac8323a68b16..5e4b3aee0577 100644
> --- a/arch/x86/kvm/vmx/tdx.h
> +++ b/arch/x86/kvm/vmx/tdx.h
> @@ -47,6 +47,12 @@ struct kvm_tdx {
> * Set/unset is protected with kvm->mmu_lock.
> */
> bool wait_for_sept_zap;
> +
> + /*
> + * Whether to get the quote directly in kernel, without exiting to
> + * userspace.
> + */
> + bool get_quote_in_kernel;
Why is this cached per-VM? IIUC, tdx_quote_enabled() is constant after initial
TDX bringup.
And is keying off tdx_quote_enabled() and only tdx_quote_enabled() backwards
compatible? How do we know that cutting userspace out of the loop wont' break
anything?
> };
>
> /* TDX module vCPU states */
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 9f7c39e0d4b5..20558b0185b6 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1538,11 +1538,133 @@ static int tdx_get_quote_user(struct kvm_vcpu *vcpu, u64 gpa, u64 size)
> return 0;
> }
>
> +static bool write_quote_status_to_guest(struct kvm_vcpu *vcpu, u64 status,
> + gpa_t gpa)
> +{
> + if (kvm_vcpu_write_guest(vcpu,
> + gpa + offsetof(struct tdx_quote_req, status),
> + &status, sizeof(status)))
> + return false;
> +
> + return true;
> +}
This is a completely useless wrapper. Actually, it's worse than useless, as it
inverts the return value from the widely used kvm_vcpu_write_guest() API.
> +
> +static bool write_quote_to_guest(struct kvm_vcpu *vcpu, void *quote_data,
> + u32 quote_len, gpa_t gpa)
> +{
> + if (kvm_vcpu_write_guest(vcpu,
> + gpa + TDX_QUOTE_REQ_HDR_SIZE,
This is unnecessarily confusing. I had to look at the definition of
TDX_QUOTE_REQ_HDR_SIZE to understand that this just writing to tdx_quote_req.data.
I might not care if this was the only write, but then the very next statement
writes to a field that isn't obviously in the header.
> + quote_data, quote_len))
> + return false;
> +
> + if (kvm_vcpu_write_guest(vcpu,
> + gpa + offsetof(struct tdx_quote_req, out_len),
> + "e_len, sizeof(quote_len)))
> + return false;
> +
> + return true;
> +}
So is this. There's literally one caller.
> +
> +static u64 get_quote_kernel(struct kvm_vcpu *vcpu, struct tdx_quote_req *req,
> + gpa_t req_gpa, size_t total_len)
> +{
> + struct tdx_td *td = &to_kvm_tdx(vcpu->kvm)->td;
> +
> + /* Only support version 1 as defined in the GHCI spec */
> + if (req->version != 1)
> + return TDX_QUOTE_STATUS_ERROR;
> +
> + /* Header + input data must fit in the page read from guest memory */
> + if ((size_t)req->in_len + TDX_QUOTE_REQ_HDR_SIZE > PAGE_SIZE)
> + return TDX_QUOTE_STATUS_ERROR;
> +
> + /* Caller owns the requested quote */
> + void *quote_data __free(kvfree) =
> + tdx_quote_generate(td, req->data, req->in_len, &req->out_len);
Am I the only person that finds the __free() stuff to be difficult to follow?
I'm sure there are flows where it's a big net positive, but IMO this isn't one.
If the *unnecesary* layers of single-use helpers are stripped away, I suspect
we'll naturally end up with a goto sequence that is more obvious that using __free().
> +
> + if (!quote_data)
> + return TDX_QUOTE_STATUS_UNAVAILABLE;
> +
> + if ((size_t)req->out_len + TDX_QUOTE_REQ_HDR_SIZE > total_len)
Can we really not pre-check the size?
> + return TDX_QUOTE_STATUS_ERROR;
> +
> + if (!write_quote_to_guest(vcpu, quote_data, req->out_len, req_gpa))
> + return TDX_QUOTE_STATUS_ERROR;
> +
> + return TDX_QUOTE_STATUS_SUCCESS;
> +}
> +
> +static u64 tdx_get_quote_check_args(struct kvm_vcpu *vcpu, u64 gpa, u64 size)
Adding single use helpers does not make the code easier to read.
> +{
> + gfn_t gfn_start, gfn_end;
> + u64 end;
> +
> + if (!size)
> + return TDVMCALL_STATUS_INVALID_OPERAND;
> +
> + if (!PAGE_ALIGNED(gpa) || !PAGE_ALIGNED(size))
> + return TDVMCALL_STATUS_ALIGN_ERROR;
> +
> + if (check_add_overflow(gpa, size, &end))
> + return TDVMCALL_STATUS_INVALID_OPERAND;
> +
> + gfn_start = gpa_to_gfn(gpa);
> + gfn_end = gpa_to_gfn(end);
> +
> + /*
> + * Reject if the guest didn't explicitly convert its quote pages to
> + * shared.
> + */
> + if (!kvm_range_has_memory_attributes(vcpu->kvm, gfn_start, gfn_end,
> + KVM_MEMORY_ATTRIBUTE_PRIVATE, 0))
TOCTOU?
> + return TDVMCALL_STATUS_INVALID_OPERAND;
> +
> + return TDVMCALL_STATUS_SUCCESS;
> +}
^ permalink raw reply
* Re: [PATCH v6 02/11] x86/virt/tdx: Allocate page bitmap for Dynamic PAMT
From: Sohil Mehta @ 2026-07-08 21:20 UTC (permalink / raw)
To: Edgecombe, Rick P, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Huang, Kai, Hansen, Dave, Zhao, Yan Y,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
nik.borisov@suse.com, linux-doc@vger.kernel.org, hpa@zytor.com,
Annapurve, Vishal, tglx@kernel.org, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: binbin.wu@linux.intel.com
In-Reply-To: <64f1cebe15f013fb8ca11c8133d416e97c2ba001.camel@intel.com>
On 7/8/2026 1:50 PM, Edgecombe, Rick P wrote:
> I tweaked it like this, seem reasonable?
>
> /*
> * The kernel supports using TDX without Dynamic PAMT, so
> * avoid reporting failure if it's not supported. Don't try
> * to support buggy TDX modules that advertise dynamic PAMT
> * but don't expose the metadata.
> */
Looks good.
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Sean Christopherson @ 2026-07-08 21:16 UTC (permalink / raw)
To: Dave Hansen
Cc: Peter Fang, Rick P Edgecombe, djbw@kernel.org,
kvm@vger.kernel.org, linux-coco@lists.linux.dev, Xiaoyao Li,
dave.hansen@linux.intel.com, baolu.lu@linux.intel.com,
Adrian Hunter, kas@kernel.org, tony.lindgren@linux.intel.com,
Yilun Xu, linux-kernel@vger.kernel.org, Sohil Mehta,
Zhenzhong Duan, Kishen Maloor, yilun.xu@linux.intel.com,
x86@kernel.org
In-Reply-To: <08c9bf2f-75be-4244-b99c-153ec1f604ca@intel.com>
On Wed, Jul 08, 2026, Dave Hansen wrote:
> On 7/6/26 10:57, Sean Christopherson wrote:
> > What is "the S3M" though? Is it a separate chip a la AMD's PSP/ASP? Is it a
> > per-package thing? Per-core?
> I'll give you my rough software guy mental model of what it is: Each
> package has its own S3M. They are microcontrollers which are discrete
> from the CPU cores. Each S3M gets some CPU physical address space routed
> over to it.
>
> > How is it accessed, and what are the "rules" for for those
> > accesses? What types of latencies are we looking at?
>
> As far as I know, the latency for one round trip to/from S3M is on the
> order of a "real" device. It has a physical address and when the OS
> wants to talk to it, those addresses are mapped with ioremap(). It's
> similar to any modern I/O device control plane. Note, though, that for
> TDX, there's no ioremap() because the I/O is hidden in the TDX module.
>
> The real overhead comes because the I/O window is essentially 4 bytes
> wide (IIRC) and all the data that comes in and out of it has to be
> squeezed through that window. It reminds me of a UART, but with a
> slightly more arcane interface.
>
> For TDX, though, the craziness is mostly hidden in the TDX module.
>
> > What else uses the S3M? Do we have to worry about contending with
> > non-TDX usage?
> There _are_ different users of S3M. But each of them should get their
> own I/O address and S3M firmware has to handle talking to those
> different users at the same time. The TDX I/O window is owned
> exclusively by the TDX module.
>
> So, while S3M has and long and growing list of jobs, the random software
> (like the host kernel) poking at one I/O window doesn't have to know
> about the other piece of software (the TDX module) poking at another.
>
> I'm sure I got a detail or two wrong in there, so folks that know this
> better: please correct me. But I think that's a halfway-decent 10,000ft
> view.
Thank you, very helpful!
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Dave Hansen @ 2026-07-08 21:13 UTC (permalink / raw)
To: Sohil Mehta, Edgecombe, Rick P, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Huang, Kai, Zhao, Yan Y,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
nik.borisov@suse.com, linux-doc@vger.kernel.org, hpa@zytor.com,
tglx@kernel.org, Annapurve, Vishal, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <3af17868-6ab2-4a22-907f-e994a1b18ccd@intel.com>
On 7/8/26 14:11, Sohil Mehta wrote:
> Instead of "Leave the atomic_t in place", would it be more appropriate
> to say "Use an (unnecessary) atomic_t to enable future ..."?
That reads fine to me.
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Sohil Mehta @ 2026-07-08 21:11 UTC (permalink / raw)
To: Edgecombe, Rick P, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Huang, Kai, Hansen, Dave, Zhao, Yan Y,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
nik.borisov@suse.com, linux-doc@vger.kernel.org, hpa@zytor.com,
tglx@kernel.org, Annapurve, Vishal, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <eab9a2e601883deaffb70cfe2fbdbbc95daacc28.camel@intel.com>
>> I'd just add a changelog blurb to handwave it away for the moment:
>>
>> The pamt_refcount[]s are atomic_t's. They do not strictly need
>> to be because all access is protected by pamt_lock. The overhead
>> of an atomic_t in this situation is minuscule compared to the
>> global lock. Leave the (unnecessary) atomic_t in place to enable
>> future optimization with minimal churn.
>>
The atomic_t is introduced primarily by this patch (and slightly by the
previous one).
Instead of "Leave the atomic_t in place", would it be more appropriate
to say "Use an (unnecessary) atomic_t to enable future ..."?
>> Does that work for everybody?
>
Yes, this works.
> Works for me, thanks.
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Edgecombe, Rick P @ 2026-07-08 21:03 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, Gao, Chao, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <1d60cebf-4764-4785-bf01-8ca59ab2cc7d@intel.com>
On Wed, 2026-07-08 at 12:01 -0700, Dave Hansen wrote:
> On 5/25/26 19:35, Rick Edgecombe wrote:
> > + scoped_guard(spinlock, &pamt_lock) {
> > + /*
> > + * If the pamt page is already added (i.e. refcount >= 1),
> > + * then just increment the refcount.
> > + */
> > + if (atomic_read(pamt_refcount)) {
> > + atomic_inc(pamt_refcount);
> > + goto out_free;
> > + }
>
> I think this pattern is a good fit for atomic_inc_not_zero().
Sure. The intention was to make this one as obviously correct as possible and I
thought to leverage the most familiar concepts of read/inc/if.
>
> I think it's also fair to say that using an atomic_t here is unnecessary
> because of the spinlock protecting all accesses. But I also understand
> that it's here because you want to optimize this path to avoid the
> spinlock in the future.
>
> This is not a hot enough path to care about an atomic_t vs. int. In
> practice, the performance is going to be bottlenecked on the big
> spinlock, *not* the atomic_t overhead.
>
> I'd just add a changelog blurb to handwave it away for the moment:
>
> The pamt_refcount[]s are atomic_t's. They do not strictly need
> to be because all access is protected by pamt_lock. The overhead
> of an atomic_t in this situation is minuscule compared to the
> global lock. Leave the (unnecessary) atomic_t in place to enable
> future optimization with minimal churn.
>
> Does that work for everybody?
Works for me, thanks.
^ permalink raw reply
* Re: [PATCH v6 02/11] x86/virt/tdx: Allocate page bitmap for Dynamic PAMT
From: Edgecombe, Rick P @ 2026-07-08 20:50 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
pbonzini@redhat.com, nik.borisov@suse.com,
linux-doc@vger.kernel.org, hpa@zytor.com, Annapurve, Vishal,
tglx@kernel.org, Mehta, Sohil, bp@alien8.de, Gao, Chao,
x86@kernel.org
Cc: binbin.wu@linux.intel.com
In-Reply-To: <a058a3e9-711b-4034-b9a6-28444aef3b5a@intel.com>
On Tue, 2026-07-07 at 20:35 -0700, Sohil Mehta wrote:
> > It why-ifys the comment more, but only gently moves from the auto-generated
> > patterns.
>
> Sure, that's fine if you want to continue the style in this file.
>
> BTW, do you also want to clarify why we fail the init if
> get_tdx_sys_info_tdmr_dpamt() fails?
>
> Is it because:
>
> If reading the DPAMT metadata fails, something is really wrong with the
> TDX module. It is better to fail the initialization in that case.
The reason why it doesn't try to handle it by falling back to normal PAMT is
just to reduce complexity. It's not to protect the kernel or anything.
Supporting broken TDX modules is just not worth the extra possible states we'd
have to consider.
I tweaked it like this, seem reasonable?
/*
* The kernel supports using TDX without Dynamic PAMT, so
* avoid reporting failure if it's not supported. Don't try
* to support buggy TDX modules that advertise dynamic PAMT
* but don't expose the metadata.
*/
^ permalink raw reply
* Re: [PATCH v6 04/11] x86/virt/tdx: Allocate ref counts for Dynamic PAMT memory
From: Sohil Mehta @ 2026-07-08 20:49 UTC (permalink / raw)
To: Rick Edgecombe, bp, dave.hansen, hpa, kas, kvm, linux-coco,
linux-doc, linux-kernel, mingo, nik.borisov, pbonzini, seanjc,
tglx, vannapurve, x86, chao.gao, yan.y.zhao, kai.huang
Cc: Kirill A. Shutemov
In-Reply-To: <20260526023515.288829-5-rick.p.edgecombe@intel.com>
On 5/25/2026 7:35 PM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> The PAMT memory holds metadata for all possible TDX protected memory. Each
> physical address range is covered by PAMT entries at three levels (1GB,
> 2MB, 4KB). With Dynamic PAMT, the 4KB range of PAMT is allocated on
> demand. The kernel supplies the TDX module with page pairs to store the
> 4KB entries, which cover 2MB of host physical memory. The kernel must
> provide this page pair before using pages from the range for TDX. If this
> is not done, SEAMCALLs that give the pages to be protected by the TDX module
> will fail.
>
> Allocate reference counters for every 2MB range to track TDX memory usage.
> This can be used to handle concurrent get/put callers, in order to
> accurately determine when the dynamic 4KB level of Dynamic PAMT needs to
> be allocated and when it can be freed.
>
> This allocation will currently consume 2 MB for every 1 TB of address
> space from 0 to max_pfn. The allocation size will depend on how the RAM is
> physically laid out. In a worst case scenario where the entire 52-bit
> address space is covered this would be 8GB. Then the DPAMT refcount
> allocations could hypothetically cause the savings from Dynamic PAMT to go
> negative on exotic platforms with sparse, small amounts of memory.
>
...
> +/*
> + * On a machine with Dynamic PAMT, the kernel maintains a reference counter
> + * for every 2M range.
Commit log says every 2MB range.
The counter indicates how many users there are for
> + * the PAMT memory of the 2M range. The kernel allocates PAMT refcounts at
> + * initialization.
> + */
> +static atomic_t *pamt_refcounts;
> +
> /* All TDX-usable memory regions. Protected by mem_hotplug_lock. */
> static LIST_HEAD(tdx_memlist);
>
> @@ -254,6 +263,43 @@ static struct syscore tdx_syscore = {
> .ops = &tdx_syscore_ops,
> };
>
> +/*
> + * Allocate PAMT reference counters for all physical memory.
> + *
> + * It consumes 2MiB for every 1TiB of physical memory.
Commit log says 2MB and 1TB. I would make both consistent.
> + */
> +static int init_pamt_refcounts(void)
> +{
> + size_t size = DIV_ROUND_UP(max_pfn, PTRS_PER_PTE) * sizeof(*pamt_refcounts);
> +
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return 0;
> +
> + pamt_refcounts = __vmalloc(size, GFP_KERNEL | __GFP_ZERO);
vzalloc()?
> + if (!pamt_refcounts)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +static void free_pamt_refcounts(void)
> +{
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return;
> +
> + vfree(pamt_refcounts);
> + pamt_refcounts = NULL;
> +}
> +
> +/* Find PAMT refcount for a given physical address */
This comment is probably not that useful. The function name is
descriptive by itself.
> +static atomic_t * __maybe_unused tdx_find_pamt_refcount(unsigned long pfn)
> +{
> + /* Find which PMD a PFN is in. */
> + unsigned long index = pfn >> (PMD_SHIFT - PAGE_SHIFT);
> +
> + return &pamt_refcounts[index];
> +}
> +
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Dave Hansen @ 2026-07-08 20:47 UTC (permalink / raw)
To: Sean Christopherson, Peter Fang
Cc: Rick P Edgecombe, djbw@kernel.org, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Xiaoyao Li,
dave.hansen@linux.intel.com, baolu.lu@linux.intel.com,
Adrian Hunter, kas@kernel.org, tony.lindgren@linux.intel.com,
Yilun Xu, linux-kernel@vger.kernel.org, Sohil Mehta,
Zhenzhong Duan, Kishen Maloor, yilun.xu@linux.intel.com,
x86@kernel.org
In-Reply-To: <akvsgVjwb00P1EuA@google.com>
On 7/6/26 10:57, Sean Christopherson wrote:
> What is "the S3M" though? Is it a separate chip a la AMD's PSP/ASP? Is it a
> per-package thing? Per-core?
I'll give you my rough software guy mental model of what it is: Each
package has its own S3M. They are microcontrollers which are discrete
from the CPU cores. Each S3M gets some CPU physical address space routed
over to it.
> How is it accessed, and what are the "rules" for for those
> accesses? What types of latencies are we looking at?
As far as I know, the latency for one round trip to/from S3M is on the
order of a "real" device. It has a physical address and when the OS
wants to talk to it, those addresses are mapped with ioremap(). It's
similar to any modern I/O device control plane. Note, though, that for
TDX, there's no ioremap() because the I/O is hidden in the TDX module.
The real overhead comes because the I/O window is essentially 4 bytes
wide (IIRC) and all the data that comes in and out of it has to be
squeezed through that window. It reminds me of a UART, but with a
slightly more arcane interface.
For TDX, though, the craziness is mostly hidden in the TDX module.
> What else uses the S3M? Do we have to worry about contending with
> non-TDX usage?
There _are_ different users of S3M. But each of them should get their
own I/O address and S3M firmware has to handle talking to those
different users at the same time. The TDX I/O window is owned
exclusively by the TDX module.
So, while S3M has and long and growing list of jobs, the random software
(like the host kernel) poking at one I/O window doesn't have to know
about the other piece of software (the TDX module) poking at another.
I'm sure I got a detail or two wrong in there, so folks that know this
better: please correct me. But I think that's a halfway-decent 10,000ft
view.
^ permalink raw reply
* Re: SVSM Development Call July 8th, 2026
From: Jörg Rödel @ 2026-07-08 20:10 UTC (permalink / raw)
To: coconut-svsm, linux-coco
In-Reply-To: <ak0nGPsnLf6Rn4Kx@8bytes.org>
Meeting Minutes for the calls on July 1st and 8th are now posted:
https://github.com/coconut-svsm/governance/pull/115
Have fun!
-Joerg
^ permalink raw reply
* Re: [PATCH v6 05/11] x86/virt/tdx: Handle concurrent callers in tdx_pamt_get/put()
From: Dave Hansen @ 2026-07-08 19:01 UTC (permalink / raw)
To: Rick Edgecombe, bp, hpa, kas, kvm, linux-coco, linux-doc,
linux-kernel, mingo, nik.borisov, pbonzini, seanjc, tglx,
vannapurve, x86, chao.gao, yan.y.zhao, kai.huang
Cc: Kirill A. Shutemov
In-Reply-To: <20260526023515.288829-6-rick.p.edgecombe@intel.com>
On 5/25/26 19:35, Rick Edgecombe wrote:
> + scoped_guard(spinlock, &pamt_lock) {
> + /*
> + * If the pamt page is already added (i.e. refcount >= 1),
> + * then just increment the refcount.
> + */
> + if (atomic_read(pamt_refcount)) {
> + atomic_inc(pamt_refcount);
> + goto out_free;
> + }
I think this pattern is a good fit for atomic_inc_not_zero().
I think it's also fair to say that using an atomic_t here is unnecessary
because of the spinlock protecting all accesses. But I also understand
that it's here because you want to optimize this path to avoid the
spinlock in the future.
This is not a hot enough path to care about an atomic_t vs. int. In
practice, the performance is going to be bottlenecked on the big
spinlock, *not* the atomic_t overhead.
I'd just add a changelog blurb to handwave it away for the moment:
The pamt_refcount[]s are atomic_t's. They do not strictly need
to be because all access is protected by pamt_lock. The overhead
of an atomic_t in this situation is minuscule compared to the
global lock. Leave the (unnecessary) atomic_t in place to enable
future optimization with minimal churn.
Does that work for everybody?
^ permalink raw reply
* Re: [PATCH v6 07/11] KVM: TDX: Allocate PAMT memory for TD and vCPU control structures
From: Dave Hansen @ 2026-07-08 18:30 UTC (permalink / raw)
To: Sean Christopherson, Rick Edgecombe
Cc: bp, hpa, kas, kvm, linux-coco, linux-doc, linux-kernel, mingo,
nik.borisov, pbonzini, tglx, vannapurve, x86, chao.gao,
yan.y.zhao, kai.huang, Kirill A. Shutemov
In-Reply-To: <akw-dgBE_dycts0o@google.com>
On 7/6/26 16:47, Sean Christopherson wrote:
>> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>> [sean: handle alloc+free+reclaim in one patch]
>> Co-developed-by: Sean Christopherson <seanjc@google.com>
> Where did this come from? I don't think me squashing two patches together
> warrants a Co-developed-by.
FWIW, I think the pendulum has swung way too far in terms of people
trying to tell the whole tortured story of their patches via tags. I
prefer some prose which is less formal *and* more descriptive than tags.
Sounds like Sean and I are mostly on the same page here too, which is a
bonus.
^ permalink raw reply
* Re: [PATCH 06/15] PCI/TSM: Add device evidence support
From: Dan Williams (nvidia) @ 2026-07-08 18:25 UTC (permalink / raw)
To: Alexey Kardashevskiy, Dan Williams, linux-coco
Cc: linux-pci, driver-core, ankita, Bjorn Helgaas, Alistair Francis,
Lukas Wunner, Xu Yilun, Aneesh Kumar K.V (Arm)
In-Reply-To: <e42b9ef3-a177-4e38-8aad-c3f4061f1b64@amd.com>
Alexey Kardashevskiy wrote:
> On 6/7/26 08:08, Dan Williams wrote:
> > Register the PCI Trusted Execution Environment Security Manager (TSM)
> > framework with the device-evidence netlink ABI. The security operations
> > that PCI core coordinates with a device security manager (DSM) through a
> > platform TEE security manager (TSM), builds upon the SPDM protocol.
> >
> > A TSM owns an SPDM session and publishes the corresponding evidence through
> > TSM firwmware ABIs. A low level TSM driver is responsible for creating a
> > 'struct device_evidence' context and carrying out refresh_evidence()
>
>
> a nit: so it is really refresh_measurements() as it won't refresh the report? may be call it so?
Sure.
> > requests for regenerating measurement transcripts with a nonce.
[..]
> > diff --git a/include/linux/pci-tsm.h b/include/linux/pci-tsm.h
> > index a6435aba03f9..8bc16029d31e 100644
> > --- a/include/linux/pci-tsm.h
> > +++ b/include/linux/pci-tsm.h
> > @@ -3,7 +3,10 @@
> > #define __PCI_TSM_H
> > #include <linux/mutex.h>
> > #include <linux/pci.h>
> > +#include <linux/rwsem.h>
>
> then this file is missing extern struct rw_semaphore pci_tsm_rwsem;
This include was a mistake. pci_tsm_rwsem is private to
drivers/pci/tsm/, so I will remove this include. This is the only shared
data structure between drivers/pci/tsm/core.c and
drivers/pci/tsm/evidence.c. It could go go in a
drivers/pci/tsm/private.h, but given just the one declaration have
evidence.c do it directly.
> > #include <linux/sockptr.h>
> > +#include <linux/tsm.h>
> > +#include <linux/device/evidence.h>
> >
> > struct pci_tsm;
> > struct tsm_dev;
> > @@ -18,6 +21,7 @@ enum pci_tsm_req_scope;
> > * @devsec_ops: Lock, unlock, and interrogate the security state of the
> > * function via the platform TSM (typically virtual function
> > * operations).
> > + * @refresh_evidence: Common operation to regenerate attestation objects
> > *
> > * This operations are mutually exclusive either a tsm_dev instance
> > * manages physical link properties or it manages function security
> > @@ -75,6 +79,9 @@ struct pci_tsm_ops {
> > struct pci_dev *pdev);
> > void (*unlock)(struct pci_tsm *tsm);
> > );
> > +
> > + int (*refresh_evidence)(struct pci_tsm *tsm, const void *nonce,
> > + size_t nonce_len);
> > };
> >
> > /**
[..]
> > @@ -216,6 +226,10 @@ void pci_tsm_tdi_constructor(struct pci_dev *pdev, struct pci_tdi *tdi,
> > ssize_t pci_tsm_guest_req(struct pci_dev *pdev, enum pci_tsm_req_scope scope,
> > sockptr_t req_in, size_t in_len, sockptr_t req_out,
> > size_t out_len, u64 *tsm_code);
> > +static inline const struct pci_tsm_ops *to_pci_tsm_ops(struct pci_tsm *tsm)
> > +{
> > + return tsm->tsm_dev->pci_ops;
> > +}
> > #else
> > static inline int pci_tsm_register(struct tsm_dev *tsm_dev)
> > {
> > diff --git a/drivers/pci/tsm.c b/drivers/pci/tsm/core.c
> > similarity index 99%
> > rename from drivers/pci/tsm.c
> > rename to drivers/pci/tsm/core.c
>
> This move + related makefile changes + exposing pci_tsm_rwsem are better be in a separate patch. Thanks,
I'll do a prep patch.
^ 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