* Re: [PATCH v6 00/11] Dynamic PAMT
From: Edgecombe, Rick P @ 2026-07-06 21:01 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
In-Reply-To: <20260526023515.288829-1-rick.p.edgecombe@intel.com>
Sean,
On Mon, 2026-05-25 at 19:35 -0700, Rick Edgecombe wrote:
> KVM: TDX: Allocate PAMT memory for TD and vCPU control structures
> KVM: TDX: Get/put PAMT pages when (un)mapping private memory
Would you be willing to take a look at these two patches that need ack's from
the KVM side? I'm hoping to make the next version of this the last one. So it
would be great to get any remaining comments before then.
Thanks.
^ permalink raw reply
* Re: [PATCH 00/15] Device Evidence and Trust for PCI Security Protocol (TDISP)
From: Dan Williams (nvidia) @ 2026-07-06 20:55 UTC (permalink / raw)
To: Jason Gunthorpe, Dan Williams
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: <20260706125140.GB107792@ziepe.ca>
Jason Gunthorpe wrote:
> On Sun, Jul 05, 2026 at 03:08:04PM -0700, Dan Williams wrote:
> > * NONE: no usage of the device unless the trust is explicitly overridden
> > by user policy specified via a driver flag, module flag, or uapi (TBD).
> >
> > * ADVERSARY: needs acknowledgement from the bus and IOMMU / DMA layers
> > that the device is limited to strict IOMMU translation behavior. Drivers
> > can use this as a signal to limit functionality. This designation
> > implies follow-on IOMMU and bus enabling work for features like
> > arranging for the device to attach to a blocked IOMMU domain when
> > detached from a driver.
> >
> > * AUTO: typical / historical Linux driver model.
> >
> > * TCB: a trust level that only exists in Confidential Computing
> > environments. When acked by the IOMMU / DMA layer it enables the device
> > to issue direct-DMA to private/encrypted addresses or otherwise attach to
> > a secure vIOMMU within the TCB.
>
> I'm not sure I entirely like this one, certainly it needs to be
> possible to have both T=1 and ADVERSARY together.
T=1 and ADVERSARY are independent for link encryption and private MMIO.
In other words the device is placed into the TDISP RUN state independent
of its trust level. Downstream accesses to the device must have T=1, and
its upstream accesses will have T=1, but with force_dma_unencrypted() ==
true.
The only combination missing is T=1 with force_dma_unencrypted() == false &&
VIOMMU enforcing IOMMU_DOMAIN_DMA.
> Arguably the T=0/1 decision is much more like link encryption, it
> effects the transport of the DMA into the CPU. That it also impacts
> how the VIOMMU works is the only thing that make it sort of
> trust-like in this model.
>
> I'd also argue this list is missing "FULL" trust, which is the
> historical Linux behavior for a normal device. AUTO should be
> selecting between FULL/ADVERSARY based on things like the ACPI/etc as
> it does today.
Two notes,
1/ that is effectively how the UNSET level behaves. If the
bus has not set ADVERSARY before device_add() then the default behavior
is the AUTO level. Where AUTO means all of the automatic privileges a
device can be offered without needing any other coordination.
2/ The ambiguity and conflict occurs at ->dma_configure() time when the
bus and IOMMU layer want to reject the device's access to some privilege
by failing. When FULL is defined as !ADVERSARY then it is difficult to
describe the semantics when FULL trust honors rejections to private DMA
and when it falls back to shared operation.
The above more points to a need to have an explicit trust level for
adversarial private memory access. The address spaces are distinct
assets with different levels of trust.
UNSET: bus picks initial level, or leaves it to the device_core().
NONE:
ADVERSARY: Device can be in T=0, or T=1 mode (UNLOCKED, or RUN).
AUTO: Could rename this to be FULL or ALL or DEFAULT, I still keep
coming back to the "AUTO" name because the privileges are not
uniform based on the IOMMU / DMA topology and device capability.
Again, the TDISP state is independent. The TSM driver does
not get called to gatekeep and verify access in this mode.
TCB_ADVERSARY: or PRIVATE_ADVERSARY. Device can access private platform
resources iff an enforcing IOMMU is present.
TCB: or PRIVATE_FULL, automatically enable all access privileges
including private memory access.
Direct-dma to private memory without a Secure VIOMMU present
requires "TCB".
> In a CC VM if we have a T=0 device we probably want to operate it with
> ADVERSARY (there is no T=0 VIOMMU so this is equivilent to FULL)
>
> For a T=1 device we need to have the choice of FULL or ADVERSARY. When
> a VIOMMU is present ADVERSARY will further restrict the T=1 traffic as
> a defense in depth.
Yes, a mode that can setup IOMMU_DOMAIN_DMA for private memory
makes sense. It needs a new trust level to distinguish if the
device is trusted to access one or both address spaces.
> So, it is really necessary to have "TCB" here?
I think it is important to distinguish that the CC case adds another
class of memory to the trust decision.
> If the trust level is reduced to just be a command to the kernel how
> it should operate the device then it would be up to userspace to
> confirm things like T=1 before setting the trust.
This discussion gets strained for me when T=1 is used to mean both
"device is in TDISP RUN (with link encryption and private MMIO)" and
"device is in TDISP RUN + force_dma_unencrypted() == false".
As implemented, userspace can confirm the former before setting the
trust. The latter only gets confirmed / denied at ->dma_configure()
time, because that is the point at which driver policy has been applied
to set the operational trust level.
Otherwise, full bi-directional T=1 before setting the trust would
require an IOMMU to be blocking the device until that final confirmation
point. Given that is not always available the proposal is to defer
acknowledging the trust level with the TSM until ->dma_configure().
As far as userspace is concerned it has enabled T=1 at the device and
validated device evidence. Any failures to operate the device from that
point are either the TSM finding fault with the evidence the CC VM is
relying upon, or, in the new TCB_ADVERSARY case, the IOMMU layer says,
"no IOMMU present to enforce IOMMU_DOMAIN_DMA".
> The kernel would have to set the trust to NONE when security sensitive
> changes are detected.
>
> If we still want a kernel-side policy gate, eg kernel will not
> progress unless T=1 is met, then I think that would be better as an
> independent pair'd policy field, ie
> trust=FULL when policy=T=1, LINK_ENCRYPTION=1, etc
>
> As bundling the two into "TCB" is probably going to turn problematic
> as we have more and more combinations of policy conditions.
Agree that "TCB" alone is insufficient per above. However, a new
"trust when/if" ABI does not appear to scale.
Userspace policy simply needs to contend with the mechanisms that
"Device Core, BUS, IOMMU or TSM driver may fail to enable a device at a
given trust level".
NONE: Device core rejects device operation
ADVERSARY: reject device operation if an IOMMU to set IOMMU_DOMAIN_DMA
not available (not in current patches)
AUTO: no rejections, but no private memory access either
TCB_ADVERSARY: reject device operation if IOMMU_DOMAIN_DMA not
available, or TSM rejects the evidence used to enable
private memory access.
TCB: reject device operation if TSM rejects chosen evidence
Overrides and bypasses for those rejection decisions belong with
their respective drivers. For example, if a paravisor is handling
TDISP then there is no TSM driver to reject TCB operation.
Similarly ->dma_configure() when no IOMMU is present is a nop
today. New enabling could turn to enforcing, or userspace policy
otherwise understands the risks.
^ permalink raw reply
* Re: [PATCH v6 10/11] x86/virt/tdx: Enable Dynamic PAMT
From: Edgecombe, Rick P @ 2026-07-06 20:48 UTC (permalink / raw)
To: kas@kernel.org, Gao, Chao
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, linux-kernel@vger.kernel.org,
seanjc@google.com, mingo@redhat.com, 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: <6d87002a7484cd44687c78d60319bea283a78562.camel@intel.com>
On Tue, 2026-06-30 at 18:20 -0700, Rick Edgecombe wrote:
> Looking... Actually there are some dynamic supported ones. Ok, I'll ask.
Thanks for this suggestion. Looks like we can drop the check.
^ 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-06 20:47 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, 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: <0c31fcdc-048c-4ff2-9e89-1ba112815c84@linux.intel.com>
On Fri, 2026-07-03 at 11:15 +0800, Binbin Wu wrote:
> > @@ -1669,16 +1683,29 @@ static struct page *tdx_spte_to_sept_pt(struct kvm
> > *kvm, gfn_t gfn,
> > static int tdx_sept_map_nonleaf_spte(struct kvm *kvm, gfn_t gfn,
> > enum pg_level level, u64 new_spte)
> > {
> > + struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
> > + struct vcpu_tdx *tdx = to_tdx(vcpu);
>
> Nit:
> Is it better to move this after checking vcpu is not NULL?
> Although tdx is not dereferenced in between, if vcpu is NULL,
> it means container_of() does arithmetic to a NULL pointer.
Personally I'm on the fence. I'm going to leave it, because Sean did it that
way:
https://lore.kernel.org/kvm/20260129011517.3545883-23-seanjc@google.com/
^ 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-06 20:27 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, 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: <1694d6c2-2aaa-4277-ba83-97c4a3007417@linux.intel.com>
On Thu, 2026-07-02 at 15:39 +0800, Binbin Wu wrote:
> On 5/26/2026 10:35 AM, 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
> > 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,
> > so simultaneous attempts on the same region would conflict.
> >
> > 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>
>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
>
> One nit below.
>
> [...]
>
> >
> > @@ -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);
>
> tdx_find_pamt_refcount() is tagged as __maybe_unused in the previous patch.
> I think the tag should be removed.
Oh yea, that was the intention. Will fix.
^ permalink raw reply
* Re: [PATCH v6 04/11] x86/virt/tdx: Allocate ref counts for Dynamic PAMT memory
From: Edgecombe, Rick P @ 2026-07-06 20:26 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: <akusIp6UQW/pIDc3@intel.com>
On Mon, 2026-07-06 at 21:22 +0800, Chao Gao wrote:
> > +/*
> > + * Allocate PAMT reference counters for all physical memory.
> > + *
> > + * It consumes 2MiB for every 1TiB of physical memory.
> > + */
> > +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);
> > + 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;
> > +}
>
> Both functions are only called during boot. They can be annotated with __init.
Yep, thanks.
>
> Reviewed-by: Chao Gao <chao.gao@intel.com>
^ permalink raw reply
* Re: [PATCH v6 04/11] x86/virt/tdx: Allocate ref counts for Dynamic PAMT memory
From: Edgecombe, Rick P @ 2026-07-06 20:26 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Hansen, Dave, Zhao, Yan Y, 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: <ca22e5c5-9742-44fa-9c47-e4134e832d49@linux.intel.com>
On Thu, 2026-07-02 at 15:20 +0800, Binbin Wu wrote:
> On 5/26/2026 10:35 AM, 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.
>
> Nit:
> Do you think it make sense to print the memory size allocated for
> DPAMT refcounts in the kernel log?
I think, no? Because it should normally be "small" on the scale of the system.
We have to draw the line somewhere.
>
> >
> > Future changes could reduce this refcount overhead to be only allocating
> > refcounts for physical ranges that contain memory that TDX can use.
> > However, this is left for future work.
> >
> > Assisted-by: Sashiko:claude-opus-4-6 GitHub Copilot:claude-opus-4-6 Sashiko:claude-opus-4-6
> > 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>
>
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Thanks, and for the others.
^ permalink raw reply
* Re: [PATCH v6 03/11] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Edgecombe, Rick P @ 2026-07-06 20:23 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: <akugEqMk29Q9DJUY@intel.com>
On Mon, 2026-07-06 at 20:31 +0800, Chao Gao wrote:
> On Mon, May 25, 2026 at 07:35:07PM -0700, 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
>
> For the whole series: s/TDX-Module/TDX module, to match the existing
> convention.
Doh, I actually changed the whole series to TDX-module before realizing that
most of the references are "TDX module". So I thought to standardize on that
going forward. I missed this one when changing back.
>
> > +/*
> > + * Calculate the arg needed for operating on the DPAMT backing for
> > + * a given 4KB page.
> > + */
> > +static u64 pamt_2mb_arg(kvm_pfn_t pfn)
> > +{
> > + unsigned long hpa_2mb = ALIGN_DOWN(pfn << PAGE_SHIFT, PMD_SIZE);
>
> The changelog and the comment don't explain why the pfn is aligned down to a
> 2MB boundary. And ...
>
> > +
> > + return hpa_2mb | TDX_PS_2M;
> > +}
> > +
> > +/* Add PAMT backing for the given page. */
>
> ... this adds PAMT backing for the whole 2MB region containing the given page,
> not just the given page itself. Is it worth clarifying in the comment?
Yea that is a good comment.
>
> <snip>
>
> > +/*
> > + * Return a page that can be gifted to the TDX-Module for use as a "control"
> > + * page, i.e. pages that are used for control and S-EPT structures for a given
> > + * TDX guest, and bound to said guest's HKID and thus obtain TDX protections,
> > + * including PAMT tracking.
> > + */
>
> IIUC, S-EPT structures are not allocated through this helper. Why mention it?
Oh, yea that is removed with v6. I'll update it.
>
> > +struct page *tdx_alloc_control_page(void)
> > +{
> > + struct page *page;
> > +
> > + page = alloc_page(GFP_KERNEL_ACCOUNT);
> > + if (!page)
> > + return NULL;
> > +
> > + if (tdx_pamt_get(page_to_pfn(page))) {
> > + __free_page(page);
> > + return NULL;
> > + }
> > +
> > + return page;
> > +}
> > +EXPORT_SYMBOL_FOR_KVM(tdx_alloc_control_page);
> > +
> > +/*
> > + * Free a page that was gifted to the TDX-Module for use as a control/S-EPT
> > + * page. After this, the page is no longer protected by TDX.
> > + */
>
> Ditto.
>
> > +void tdx_free_control_page(struct page *page)
> > +{
> > + if (!page)
> > + return;
> > +
> > + tdx_pamt_put(page_to_pfn(page));
> > + __free_page(page);
> > +}
> > +EXPORT_SYMBOL_FOR_KVM(tdx_free_control_page);
>
> With above nits fixed,
>
> Reviewed-by: Chao Gao <chao.gao@intel.com>
^ permalink raw reply
* Re: [PATCH v6 01/11] x86/virt/tdx: Simplify tdmr_get_pamt_sz()
From: Edgecombe, Rick P @ 2026-07-06 20:18 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, binbin.wu@linux.intel.com,
x86@kernel.org
In-Reply-To: <akdNPjf3zt6jPX1m@intel.com>
On Fri, 2026-07-03 at 13:48 +0800, Chao Gao wrote:
> On Mon, May 25, 2026 at 07:35:05PM -0700, Rick Edgecombe wrote:
> > Since the loop that iterates over it is gone, further simplify the code by
> > dropping the array of intermediate size and base storage. Just store the
> > values to their final locations.
>
> > Accept the small complication of having
> > to clear tdmr->pamt_4k_base in the error path, so that tdmr_do_pamt_func()
> > will not try to operate on the TDMR struct when attempting to free it.
>
> The clearing of tdmr->pamt_4k_base was dropped, so this section is a bit
> stale. Apart from this nit,
Oh, good point.
>
> Reviewed-by: Chao Gao <chao.gao@intel.com>
Thanks, and for the others.
^ permalink raw reply
* Re: [PATCH v2 16/31] coco/tdx-host: Support Link TSM for TDX host
From: Ackerley Tng @ 2026-07-06 20:12 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-17-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
>
> [...snip...]
>
> +static struct pci_tsm_ops tdx_tsm_link_ops = {
> + .probe = tdx_tsm_link_probe,
> + .remove = tdx_tsm_link_remove,
> + .connect = tdx_tsm_link_connect,
Since .connect is called from pci_tsm_connect(), which is in turn called
from connect_store, is the plan that the VMM should write to
/sys/bus/pci/devices/.../tsm/connect
to set up SPDM and IDE before starting the guest?
> + .disconnect = tdx_tsm_link_disconnect,
> +};
> +
>
> [...snip...]
>
^ permalink raw reply
* Re: [PATCH v2 30/31] coco/tdx-host: Implement IDE stream setup/teardown
From: Ackerley Tng @ 2026-07-06 20:05 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-31-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
>
> [...snip...]
>
> static int tdx_tsm_link_connect(struct pci_dev *pdev)
> {
> struct tdx_tsm_link *tlink = to_tdx_tsm_link(pdev->tsm);
> @@ -362,7 +579,15 @@ static int tdx_tsm_link_connect(struct pci_dev *pdev)
> return PTR_ERR(tlink_spdm);
> }
>
> + struct tdx_tsm_link *tlink_ide __free(tdx_ide_stream_teardown) =
> + tdx_ide_stream_setup(tlink);
> + if (IS_ERR(tlink_ide)) {
> + pci_err(pdev, "fail to setup ide stream\n");
> + return PTR_ERR(tlink_ide);
> + }
> +
> retain_and_null_ptr(tlink_spdm);
> + retain_and_null_ptr(tlink_ide);
>
> return 0;
> }
Would it be possible to define .connect = tdx_tsm_link_connect in the
patch that sets up spdm, and then update tdx_tsm_link_connect in this
patch?
I think it would help in showing how the spdm is associated with
tdx_tsm_link_ops.
Would it also be possible to bring the definition of .probe in
tdx_tsm_link_ops till after this patch too?
> @@ -371,6 +596,7 @@ static void tdx_tsm_link_disconnect(struct pci_dev *pdev)
> {
> struct tdx_tsm_link *tlink = to_tdx_tsm_link(pdev->tsm);
>
> + tdx_ide_stream_teardown(tlink);
> tdx_spdm_session_teardown(tlink);
> }
>
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v6 00/20] dma-mapping: Use DMA_ATTR_CC_SHARED through direct, pool and swiotlb paths
From: Jason Gunthorpe @ 2026-07-06 19:52 UTC (permalink / raw)
To: Tom Lendacky
Cc: Alexey Kardashevskiy, Aneesh Kumar K.V, Catalin Marinas, iommu,
linux-arm-kernel, linux-kernel, linux-coco, Robin Murphy,
Marek Szyprowski, Will Deacon, Marc Zyngier, Steven Price,
Suzuki K Poulose, Jiri Pirko, Mostafa Saleh, Petr Tesarik,
Dan Williams, Xu Yilun, linuxppc-dev, linux-s390,
Madhavan Srinivasan, Michael Ellerman, Nicholas Piggin,
Christophe Leroy (CS GROUP), Alexander Gordeev, Gerald Schaefer,
Heiko Carstens, Vasily Gorbik, Christian Borntraeger,
Sven Schnelle, x86
In-Reply-To: <d7afa22d-5a41-491f-b945-289ab78080e7@amd.com>
On Mon, Jul 06, 2026 at 01:45:33PM -0500, Tom Lendacky wrote:
> In that situation, with SME enabled on the host, the SWIOTLB must be
> mapped by the kernel without the encryption bit set and DMA addresses
> must be provided to devices without the encryption bit set. This is
> because if SWIOTLB is mapped encrypted, requiring the encryption bit be
> part of the DMA address, any device that cannot perform DMA at the
> address width where the encryption bit exists will fail to DMA properly.
Ideally there would be two SWIOTLBs and "decrypted" aka low address,
one is only used if the device requires it.
Alternatively we don't support this questionable security choice and
using swiotlb with an incapable device just fails. Use the iommu, or
don't force swiotlb.
Jason
^ permalink raw reply
* Re: [PATCH v2 28/31] coco/tdx-host: Parse ACPI KEYP table to init IDE for PCI host bridges
From: Ackerley Tng @ 2026-07-06 19:46 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-29-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
> Parse the KEYP Key Configuration Units (KCU), to decide the max IDE
> streams supported for each host bridge.
>
> The KEYP table points to a number of KCU structures that each associates
> with a list of root ports (RP) via segment, bus, and devfn. Sanity check
> the KEYP table, ensure all RPs listed for each KCU are included in one
> host bridge. Then extact the max IDE streams supported to
> pci_host_bridge via pci_ide_set_nr_streams().
>
> Co-developed-by: Dave Jiang <dave.jiang@intel.com>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
> drivers/virt/coco/tdx-host/tdx-host.c | 111 ++++++++++++++++++++++++++
> 1 file changed, 111 insertions(+)
>
> diff --git a/drivers/virt/coco/tdx-host/tdx-host.c b/drivers/virt/coco/tdx-host/tdx-host.c
> index 4d127b7c2591..d5072a68b81a 100644
> --- a/drivers/virt/coco/tdx-host/tdx-host.c
> +++ b/drivers/virt/coco/tdx-host/tdx-host.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2025 Intel Corporation
> */
>
> +#include <linux/acpi.h>
> #include <linux/bitfield.h>
> #include <linux/device/faux.h>
> #include <linux/dmar.h>
> @@ -12,6 +13,7 @@
> #include <linux/mod_devicetable.h>
> #include <linux/pci.h>
> #include <linux/pci-doe.h>
> +#include <linux/pci-ide.h>
> #include <linux/pci-tsm.h>
> #include <linux/tsm.h>
> #include <linux/vmalloc.h>
> @@ -474,6 +476,111 @@ static void unregister_link_tsm(void *link)
> tsm_unregister(link);
> }
>
> +#define KCU_STR_CAP_NUM_STREAMS GENMASK(8, 0)
> +
> +/* The bus_end is inclusive */
> +struct keyp_hb_info {
> + /* input */
> + u16 segment;
> + u8 bus_start;
> + u8 bus_end;
Would you consider using bus_last instead of bus_end to indicate that
bus_last is inclusive? Or would it be better to say nr_buses to avoid
the inclusive/exclusive problem?
> + /* output */
> + u8 nr_ide_streams;
> +};
> +
> +static bool keyp_info_match(struct acpi_keyp_rp_info *rp,
> + struct keyp_hb_info *hb)
> +{
> + return rp->segment == hb->segment && rp->bus >= hb->bus_start &&
> + rp->bus <= hb->bus_end;
> +}
> +
> +static int keyp_config_unit_handler(union acpi_subtable_headers *header,
> + void *arg, const unsigned long end)
> +{
> + struct acpi_keyp_config_unit *acpi_cu =
> + (struct acpi_keyp_config_unit *)&header->keyp;
> + struct keyp_hb_info *hb_info = arg;
> + int rp_size, rp_count, i;
> + void __iomem *addr;
> + bool match = false;
> + u32 cap;
> +
> + rp_size = acpi_cu->header.length - sizeof(*acpi_cu);
> + if (rp_size % sizeof(struct acpi_keyp_rp_info))
> + return -EINVAL;
> +
> + rp_count = rp_size / sizeof(struct acpi_keyp_rp_info);
> + if (!rp_count || rp_count != acpi_cu->root_port_count)
> + return -EINVAL;
> +
> + for (i = 0; i < rp_count; i++) {
> + struct acpi_keyp_rp_info *rp_info = &acpi_cu->rp_info[i];
> +
> + if (i == 0) {
> + match = keyp_info_match(rp_info, hb_info);
> + /* The host bridge already matches another KCU */
> + if (match && hb_info->nr_ide_streams)
> + return -EINVAL;
> +
> + continue;
> + }
> +
> + if (match ^ keyp_info_match(rp_info, hb_info))
> + return -EINVAL;
> + }
> +
> + if (!match)
> + return 0;
> +
> + addr = ioremap(acpi_cu->register_base_address, sizeof(cap));
> + if (!addr)
> + return -ENOMEM;
> + cap = ioread32(addr);
> + iounmap(addr);
> +
> + hb_info->nr_ide_streams = FIELD_GET(KCU_STR_CAP_NUM_STREAMS, cap) + 1;
> +
> + return 0;
> +}
> +
> +static u8 keyp_find_nr_ide_stream(u16 segment, u8 bus_start, u8 bus_end)
> +{
> + struct keyp_hb_info hb_info = {
> + .segment = segment,
> + .bus_start = bus_start,
> + .bus_end = bus_end,
> + };
> + int rc;
> +
> + rc = acpi_table_parse_keyp(ACPI_KEYP_TYPE_CONFIG_UNIT,
> + keyp_config_unit_handler, &hb_info);
> + if (rc < 0)
> + return 0;
> +
> + return hb_info.nr_ide_streams;
> +}
> +
> +static void keyp_setup_nr_ide_stream(struct pci_bus *bus)
> +{
> + struct pci_host_bridge *hb = pci_find_host_bridge(bus);
> + u8 nr_ide_streams;
> +
> + nr_ide_streams = keyp_find_nr_ide_stream(pci_domain_nr(bus),
> + bus->busn_res.start,
> + bus->busn_res.end);
> +
> + pci_ide_set_nr_streams(hb, nr_ide_streams);
> +}
> +
> +static void tdx_setup_nr_ide_stream(void)
Was wondering, is nr in the name meant as an abbreviation for number?
Why not just tdx_setup_ide_streams(), since this doesn't really set up a
specific number of streams.
Same for the naming of keyp_setup_nr_ide_stream().
> +{
> + struct pci_bus *bus = NULL;
> +
> + while ((bus = pci_find_next_bus(bus)))
> + keyp_setup_nr_ide_stream(bus);
> +}
> +
> static DEFINE_XARRAY(tlink_iommu_xa);
>
> static void tdx_iommu_clear(u64 iommu_id, struct tdx_page_array *iommu_mt)
> @@ -569,6 +676,8 @@ static int __maybe_unused tdx_connect_init(struct device *dev)
> if (ret)
> return ret;
>
> + tdx_setup_nr_ide_stream();
> +
> link = tsm_register(dev, &tdx_tsm_link_ops);
> if (IS_ERR(link))
> return dev_err_probe(dev, PTR_ERR(link),
> @@ -612,5 +721,7 @@ static void __exit tdx_host_exit(void)
> }
> module_exit(tdx_host_exit);
>
> +MODULE_IMPORT_NS("ACPI");
> +MODULE_IMPORT_NS("PCI_IDE");
> MODULE_DESCRIPTION("TDX Host Services");
> MODULE_LICENSE("GPL");
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v2 26/31] mm: Add __free() support for __free_page()
From: Ackerley Tng @ 2026-07-06 19:31 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-27-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
> Allow for the declaration of struct page * variables that trigger
> __free_page() when they go out of scope.
>
> A example usage would be in the following patch:
>
> static struct pci_tsm *tdx_tsm_link_pf0_probe(...)
> {
> ...
>
> struct page *in_msg_page __free(__free_page) =
> alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (!in_msg_page)
> return NULL;
>
> struct page *out_msg_page __free(__free_page) =
> alloc_page(GFP_KERNEL | __GFP_ZERO);
> if (!out_msg_page)
> return NULL;
>
> ...
>
> tlink->in_msg = no_free_ptr(in_msg_page);
> tlink->out_msg = no_free_ptr(out_msg_page);
>
> ...
> }
>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
> ---
> include/linux/gfp.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/linux/gfp.h b/include/linux/gfp.h
> index 51ef13ed756e..d37e5564234e 100644
> --- a/include/linux/gfp.h
> +++ b/include/linux/gfp.h
> @@ -391,6 +391,7 @@ extern void free_pages_nolock(struct page *page, unsigned int order);
> extern void free_pages(unsigned long addr, unsigned int order);
>
> #define __free_page(page) __free_pages((page), 0)
> +DEFINE_FREE(__free_page, struct page *, if (_T) __free_page(_T))
> #define free_page(addr) free_pages((addr), 0)
>
> void page_alloc_init_cpuhp(void);
> --
> 2.25.1
This is nice to have, independent of this series. Perhaps it should be
sent separately, and some existing function could be refactored to use
this, maybe __tdx_td_init?
Reviewed-by: Ackerley Tng <ackerleytng@google.com>
^ permalink raw reply
* Re: [PATCH v2 23/31] coco/tdx-host: Setup all trusted IOMMUs on TDX Connect init
From: Ackerley Tng @ 2026-07-06 19:21 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-24-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
> Setup all trusted IOMMUs on TDX Connect initialization and clear all on
> TDX Connect removal.
>
> Trusted IOMMU setup is the pre-condition for all following TDX Connect
> operations such as SPDM/IDE setup. It is more of a platform
> configuration than a standalone IOMMU configuration, so put the
> implementation in tdx-host driver.
>
I was wondering about hotplug. There's dmar_device_hotplug(). Does it
make sense to do TDH.IOMMU.SETUP from there?
For boot, is the platform-level configuration due to dependencies? Is it
because TDX is set up after IOMMUs and hence TDH.IOMMU.SETUP is done
from the creation of the faux device?
> There is no dedicated way to enumerate which IOMMU devices support
> trusted operations. The host has to call TDH.IOMMU.SETUP on all IOMMU
> devices and tell their trusted capability by the return value.
>
>
> [...snip...]
>
> @@ -119,6 +120,82 @@ static void unregister_link_tsm(void *link)
> tsm_unregister(link);
> }
>
> +static DEFINE_XARRAY(tlink_iommu_xa);
> +
> +static void tdx_iommu_clear(u64 iommu_id, struct tdx_page_array *iommu_mt)
> +{
> + u64 r;
> +
> + r = tdh_iommu_clear(iommu_id, iommu_mt);
> + if (r) {
> + pr_err("fail to clear tdx iommu 0x%llx\n", r);
> + goto leak;
> + }
> +
> + if (tdx_page_array_ctrl_release(iommu_mt, iommu_mt->nr_pages,
> + virt_to_phys(iommu_mt->root))) {
> + pr_err("fail to release iommu_mt pages\n");
> + goto leak;
> + }
> +
> + return;
> +
> +leak:
> + tdx_page_array_ctrl_leak(iommu_mt);
> +}
> +
> +static int tdx_iommu_enable_one(struct dmar_drhd_unit *drhd)
> +{
> + unsigned int nr_pages = tdx_sysinfo->connect.iommu_mt_page_count;
> + u64 r, iommu_id;
> + int ret;
> +
> + struct tdx_page_array *iommu_mt __free(tdx_page_array_free) =
> + tdx_page_array_create_iommu_mt(1, nr_pages);
> + if (!iommu_mt)
> + return -ENOMEM;
> +
> + r = tdh_iommu_setup(drhd->reg_base_addr, iommu_mt, &iommu_id);
> + /* This drhd doesn't support tdx mode, skip. */
> + if ((r & TDX_SEAMCALL_STATUS_MASK) == TDX_OPERAND_INVALID)
Extra space here before ==.
Is there a chance that TDX_OPERAND_INVALID might be returned for some
other reason other than that the drhd doesn't support TDX? I'm concerned
that some drhd would be silently skipped for some other error.
> + return 0;
> +
> + if (r) {
> + pr_err("fail to enable tdx mode for DRHD[0x%llx]\n",
> + drhd->reg_base_addr);
> + return -EFAULT;
> + }
> +
> + ret = xa_insert(&tlink_iommu_xa, (unsigned long)iommu_id,
> + no_free_ptr(iommu_mt), GFP_KERNEL);
> + if (ret) {
> + tdx_iommu_clear(iommu_id, iommu_mt);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void tdx_iommu_disable_all(void *data)
> +{
> + struct tdx_page_array *iommu_mt;
> + unsigned long iommu_id;
> +
> + xa_for_each(&tlink_iommu_xa, iommu_id, iommu_mt)
> + tdx_iommu_clear(iommu_id, iommu_mt);
> +}
> +
Is there an equivalent teardown operation? Like on kexec, if TDX is torn
down, would tdx_iommu_disable_all() need to be called to undo
everything, so that after TDX is started again tdx_iommu_enable_all()
would be set up again?
For hot-unplug, would the TDX IOMMU config need to be disabled?
> +static int tdx_iommu_enable_all(void)
> +{
> + int ret;
> +
> + ret = do_for_each_drhd_unit(tdx_iommu_enable_one);
> + if (ret)
> + tdx_iommu_disable_all(NULL);
> +
> + return ret;
> +}
> +
> static int __maybe_unused tdx_connect_init(struct device *dev)
> {
> struct tsm_dev *link;
> @@ -130,6 +207,14 @@ static int __maybe_unused tdx_connect_init(struct device *dev)
> if (!(tdx_sysinfo->features.tdx_features0 & TDX_FEATURES0_TDXCONNECT))
> return 0;
>
> + ret = tdx_iommu_enable_all();
> + if (ret)
> + return dev_err_probe(dev, ret, "Enable tdx iommu failed\n");
> +
> + ret = devm_add_action_or_reset(dev, tdx_iommu_disable_all, NULL);
> + if (ret)
> + return ret;
> +
> link = tsm_register(dev, &tdx_tsm_link_ops);
> if (IS_ERR(link))
> return dev_err_probe(dev, PTR_ERR(link),
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v6 00/20] dma-mapping: Use DMA_ATTR_CC_SHARED through direct, pool and swiotlb paths
From: Tom Lendacky @ 2026-07-06 18:45 UTC (permalink / raw)
To: Alexey Kardashevskiy, Jason Gunthorpe, Aneesh Kumar K.V
Cc: Catalin Marinas, iommu, linux-arm-kernel, linux-kernel,
linux-coco, Robin Murphy, Marek Szyprowski, Will Deacon,
Marc Zyngier, Steven Price, Suzuki K Poulose, Jiri Pirko,
Mostafa Saleh, Petr Tesarik, Dan Williams, Xu Yilun, linuxppc-dev,
linux-s390, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy (CS GROUP), Alexander Gordeev,
Gerald Schaefer, Heiko Carstens, Vasily Gorbik,
Christian Borntraeger, Sven Schnelle, x86
In-Reply-To: <fd135adc-a8a4-48e5-b649-2a29789b22d8@amd.com>
On 7/1/26 20:04, Alexey Kardashevskiy wrote:
> On 1/7/26 03:42, Jason Gunthorpe wrote:
>> On Mon, Jun 29, 2026 at 12:16:30PM +0530, Aneesh Kumar K.V wrote:
>>>>> Thinking about this more, I guess we should mark the swiotlb as
>>>>> cc_shared only with CC_ATTR_GUEST_MEM_ENCRYPT instead of
>>>>> CC_ATTR_MEM_ENCRYPT as we have below.
>>>>
>>>> The name cc_shared should be used for GUEST scenarios only.
>>>>
>>>> I guess there is some merit in keeping swiotlb using "decrypted" to
>>>> mean it usinig pgprot_decrypted and set_memory_decyped() which AMD
>>>> gives meaning to on both host and guest.
>>>
>>> Are you suggesting to change the struct io_tlb_mem::cc_shared back to
>>> struct io_tlb_mem::unencrypted?.
>>
>> Yes
>>
>>>> IDK what AMD should do on the host by default. I guess it should setup
>>>> a swiotlb pool of low dma addrs "unencrypted", but not "cc_shared"?
>>>>
>>>
>>> If by low DMA address you mean using an address with the C-bit
>>> cleared.
>>
>> Yes
>>
>>> The current code already does this and uses the swiotlb pool correctly
>>> on SME.
>>
>> Well, through the force_dma_unencrypted() hack...
>>
>>> The challenge arises when we want to force SWIOTLB
>>> bouncing even for devices that can handle encrypted DMA addresses (more
>>> on that below). For such a config force_dma_uencrypted(dev) will return
>>> false and swiotlb will be marked cc_shared/decrypted = true; This trip
>>> the new check we added.
>>
>> Yes, because cc_shared (guest) and unencrypted (host) are very
>> different things and we've mixed them:
>>
>>> if (unlikely(mem->cc_shared != force_dma_unencrypted(dev)))
>>
>> I'm aruging force_dma_unencrypted should mean cc_shared and be
>> guest_only, but the SME hack breaks this.
>>
>>> We can also do
>>>
>>> if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
>>> /* swiotlb pool is incorrect for this device */
>>> if (unlikely(mem->cc_shared != force_dma_unencrypted(dev)))
>>> return (phys_addr_t)DMA_MAPPING_ERROR;
>>>
>>> /* Force attrs to match the kind of memory in the pool */
>>> if (mem->cc_shared)
>>> *attrs |= DMA_ATTR_CC_SHARED;
>>> else
>>> *attrs &= ~DMA_ATTR_CC_SHARED;
>>> } else {
>>> /*
>>> * Host memory encryption where device requires an
>>> * unencrypted dma_addr_t due to dma mask limit
>>> */
>>> if (force_dma_unencrypted(dev))
>>> *attrs |= DMA_ATTR_CC_SHARED;
>>> else
>>> *attrs &= ~DMA_ATTR_CC_SHARED;
>>> }
>>
>> If we do this I would like to split the force_dma_.. functions into
>> guest and host, ie force_dma_cc_shared() and force_host_decrypted()
>
> imho force_dma_unencrypted() should not look at the mask at all (the
> mask should tell the DMA layer to use swiotlb, encrypted or not),
> instead, when we set up swiotlb - we could make it unencrypted if
> iommu=pt, otherwise encrypted (although this means IOMMU and defeats the
> purpose of swiotlb). But at least this patchset has enough plumbing to
> have swiotlb encrypted, right?
>
>> To make it clear there are two very different things here.
>>
>>> Here I see value in having DMA_ATTR_UNENCRYPTED. The question is do we
>>> need to split this into two flags and introduce the resulting code
>>> duplication.
>>
>> The external flag name should be DMA_ATTR_CC_SHARED and only used on
>> CC guest. Internally that turns into using set_memory_decrypted()
>> which works on guest and host for AMD. I don't know how to make the
>> host only case clearer and still keep the code efficient..
>>
>>>> The dma api has to detect, after the driver sets the dma limit, that
>>>> none of system memory is usable when:
>>>> - The direct path is being used
>>>> - phys to dma for 0 is outside the dma limit
>>>>
>>>> Then it should assume the arch has setup a swiotlb pool for it to use
>>>> to fix the high memory problem.
>>>>
>>>> Similar hackery would be needed in the dma alloc path to know that
>>>> decrypted can be used to fix the high memory problem like for GUEST.
>>>>
>>>> I guess some 'dev_cannot_reach_memory(dev)' sort of test in a
>>>> few key places? Setup with a static branch to be a nop on everything
>>>> but AMD, compiled out on every other arch.
>>>>
>>>
>>> If we are not able to reach the memory because of the memory encryption
>>> bit, then isn't dev_cannot_reach_memory(dev) the same as
>>> force_dma_unencrypted(dev)? If so, that is how it is already done.
>>
>> Sort of yes, but it is properly named to its purpose and not confused
>> with what should be a guest-only function.
>>
>>> x86/dma: Disable forced SWIOTLB bouncing for SME IOMMU passthrough
>>
>> Maybe as a crutch to get this series merged..
> feels okay but I do not really know the true meaning of "swiotlb=force"
> so adding Tom to the thread. Thanks,
With swiotlb=force all DMA would be bounce buffered through SWIOTLB. My
understanding (which could be completely wrong) for this setting is that
it is used mainly to validate a driver implementation (race conditions
between device memory access and driver memory access) and validate the
SWIOTLB implementation itself. I'm not sure why you would want to run
with swiotlb=force other than that.
In that situation, with SME enabled on the host, the SWIOTLB must be
mapped by the kernel without the encryption bit set and DMA addresses
must be provided to devices without the encryption bit set. This is
because if SWIOTLB is mapped encrypted, requiring the encryption bit be
part of the DMA address, any device that cannot perform DMA at the
address width where the encryption bit exists will fail to DMA properly.
Thanks,
Tom
>
>
^ permalink raw reply
* Re: [PATCH v2 22/31] iommu/vt-d: Export a helper to do function for each dmar_drhd_unit
From: Ackerley Tng @ 2026-07-06 18:35 UTC (permalink / raw)
To: Xu Yilun, linux-coco, linux-pci, dan.j.williams, x86
Cc: chao.gao, dave.jiang, baolu.lu, yilun.xu, zhenzhong.duan, kvm,
rick.p.edgecombe, dave.hansen, kas, xiaoyao.li, vishal.l.verma,
linux-kernel
In-Reply-To: <20260327160132.2946114-23-yilun.xu@linux.intel.com>
Xu Yilun <yilun.xu@linux.intel.com> writes:
> Enable the tdx-host module to get VTBAR address for every IOMMU device.
> The VTBAR address is for TDX Module to identify the IOMMU device and
> setup its trusted configuraion.
>
I think this commit message doesn't match the commit subject or the
change, unless I'm misunderstanding something
> Suggested-by: Lu Baolu <baolu.lu@linux.intel.com>
> Signed-off-by: Xu Yilun <yilun.xu@linux.intel.com>
> ---
> include/linux/dmar.h | 2 ++
> drivers/iommu/intel/dmar.c | 16 ++++++++++++++++
> 2 files changed, 18 insertions(+)
>
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index 692b2b445761..cd8d9f440975 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -86,6 +86,8 @@ extern struct list_head dmar_drhd_units;
> dmar_rcu_check()) \
> if (i=drhd->iommu, 0) {} else
>
> +int do_for_each_drhd_unit(int (*fn)(struct dmar_drhd_unit *));
> +
> static inline bool dmar_rcu_check(void)
> {
> return rwsem_is_locked(&dmar_global_lock) ||
> diff --git a/drivers/iommu/intel/dmar.c b/drivers/iommu/intel/dmar.c
> index 4f9571eee1d4..eea9ba691f99 100644
> --- a/drivers/iommu/intel/dmar.c
> +++ b/drivers/iommu/intel/dmar.c
> @@ -2452,3 +2452,19 @@ bool dmar_platform_optin(void)
> return ret;
> }
> EXPORT_SYMBOL_GPL(dmar_platform_optin);
> +
> +int do_for_each_drhd_unit(int (*fn)(struct dmar_drhd_unit *))
> +{
> + struct dmar_drhd_unit *drhd;
> + int ret;
> +
> + guard(rwsem_read)(&dmar_global_lock);
> +
> + for_each_drhd_unit(drhd) {
> + ret = fn(drhd);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(do_for_each_drhd_unit);
> --
> 2.25.1
^ permalink raw reply
* Re: [PATCH v8 13/46] KVM: guest_memfd: Add base support for KVM_SET_MEMORY_ATTRIBUTES2
From: Ackerley Tng @ 2026-07-06 18:17 UTC (permalink / raw)
To: Suzuki K Poulose, aik, andrew.jones, binbin.wu, brauner,
chao.p.peng, david, jmattson, jthoughton, michael.roth, oupton,
pankaj.gupta, qperret, rick.p.edgecombe, rientjes, shivankg,
steven.price, tabba, willy, wyihan, yan.y.zhao, forkloop,
pratyush, aneesh.kumar, liam, Paolo Bonzini, Sean Christopherson,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Steven Rostedt, Masami Hiramatsu,
Mathieu Desnoyers, Jonathan Corbet, Shuah Khan, Shuah Khan,
Vishal Annapurve, Andrew Morton, Chris Li, Kairui Song,
Kemeng Shi, Nhat Pham, Barry Song, Axel Rasmussen, Yuanchu Xie,
Wei Xu, Youngjun Park, Qi Zheng, Shakeel Butt, Kiryl Shutsemau,
Baoquan He, Jason Gunthorpe, Vlastimil Babka
Cc: kvm, linux-kernel, linux-trace-kernel, linux-doc, linux-kselftest,
linux-mm, linux-coco
In-Reply-To: <114e2488-97ed-4740-a8e8-1edd991f26c5@arm.com>
Suzuki K Poulose <suzuki.poulose@arm.com> writes:
>
> [...snip...]
>
>> +static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
>> + size_t nr_pages, uint64_t attrs)
>> +{
>> + struct address_space *mapping = inode->i_mapping;
>> + struct gmem_inode *gi = GMEM_I(inode);
>> + pgoff_t end = start + nr_pages;
>> + struct maple_tree *mt;
>> + struct ma_state mas;
>> + int r;
>> +
>> + mt = &gi->attributes;
>> +
>> + filemap_invalidate_lock(mapping);
>> +
>> + mas_init(&mas, mt, start);
>> + r = kvm_gmem_mas_preallocate(&mas, attrs, start, nr_pages);
>> + if (r)
>> + goto out;
>> +
>> + /*
>> + * From this point on guest_memfd has performed necessary
>> + * checks and can proceed to do guest-breaking changes.
>> + */
>> +
>> + kvm_gmem_invalidate_start(inode, start, end);
>
> I added support for Arm CCA KVM patches with the inplace conversion and
> I am hitting the following issue.
>
> 1. I am supporting INIT_SHARED + MMAP flags.
> 2. VMM creates the Gmem_fd with both the flags above.
> 3. Uses the shared gmem-mmap to load the initial payloads (kernel, dtb).
> 4. At the VM finalization time, Populate the loaded regions one by one
> by
> a) copying the images to a temparory buffer - Since CCA can't really
> load the contents in-place.
Sounds good :). I see that you blocked this in the kernel by returning
-EOPNOTSUPP if (!src_page) [0].
> b) Set the "region" to Private in the gmem_fd (via
> SET_MEMORY_ATTRIBUTES2)
> c) Invoke CCA backend to populate the private memory via
> ioctl(KVM_ARM_RMI_POPULATE,..) [0]
>
This flow sounds right.
> [0]
> https://lore.kernel.org/all/20260513131757.116630-27-steven.price@arm.com/
>
>
> 5. Additionally, VMM can mark the entire RAM to be private before the VM
> starts running, again via SET_MEMORY_ATTRIBUTES2. On CCA, this
> action is measured and doesn't require the Host to "commit" memory to
> the VM.
> Instead the host can lazily donate memory on a fault.
>
For both TDX and SNP, the host can also lazily donate memory,
guest_memfd supports this.
> But step (5) triggers the invalidation of both private and shared
> mappings of the gmem area, from the kvm_gmem_invalidate_start()
> above.
>
> This is because, the entire DRAM now has, some portions PRIVATE (the
> loaded regions) and the rest are SHARED (from the Gmem_fd creation).
> Thus, kvm_gmem_get_invalidate_filter(Dram_start, Dram_end) causes the
> invalidation of both "PRIVATE" and "SHARED" regions, which results
> in the destruction of the already loaded data and things go south.
>
This destruction will happen for TDX as well. I think we managed to get
around this because we didn't apply conversion on the already-private
ranges.
IIUC on SNP, zapping pages in the stage 2 page tables doesn't destroy
the data, so that's probably why it has been fine for SNP.
> When we know that the kvm_gmem_invalidate_xx is triggered by a
> conversion, we don't need to invalidate the existing pages that
> are in the requested state. i.e., the following patch on top of
> this series does the trick for me :
>
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index a97fcac34a0e..62e0427a49f4 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -250,16 +250,23 @@ static void __kvm_gmem_invalidate_start(struct
> gmem_file *f, pgoff_t start,
> KVM_MMU_UNLOCK(kvm);
> }
>
> +static void kvm_gmem_invalidate_start_filter(struct inode *inode,
> pgoff_t start,
> + pgoff_t end,
> + enum kvm_gfn_range_filter
> attr_filter)
> +{
> + struct gmem_file *f;
> +
> + kvm_gmem_for_each_file(f, inode)
> + __kvm_gmem_invalidate_start(f, start, end, attr_filter);
> +}
> +
> static void kvm_gmem_invalidate_start(struct inode *inode, pgoff_t start,
> pgoff_t end)
> {
> enum kvm_gfn_range_filter attr_filter;
> - struct gmem_file *f;
> -
> attr_filter = kvm_gmem_get_invalidate_filter(inode, start, end);
>
> - kvm_gmem_for_each_file(f, inode)
> - __kvm_gmem_invalidate_start(f, start, end, attr_filter);
> + kvm_gmem_invalidate_start_filter(inode, start, end, attr_filter);
> }
>
> static void __kvm_gmem_invalidate_end(struct gmem_file *f, pgoff_t start,
> @@ -724,9 +731,14 @@ static int __kvm_gmem_set_attributes(struct inode
> *inode, pgoff_t start,
> /*
> * From this point on guest_memfd has performed necessary
> * checks and can proceed to do guest-breaking changes.
> + * Also, we don't have to invalidate the regions that
> + * may already be in the requested state. Hence, we could
> + * explicitly filter the invalidations to the opposite
> + * state.
> */
>
> - kvm_gmem_invalidate_start(inode, start, end);
> + kvm_gmem_invalidate_start_filter(inode, start, end,
> + to_private ? KVM_FILTER_SHARED :
> KVM_FILTER_PRIVATE);
>
I think this makes sense. Thanks for catching this.
> if (!to_private)
> kvm_gmem_invalidate(inode, start, end);
>
>
> Thoughts ?
>
> Suzuki
>
>
>>
>> [...snip...]
>>
^ permalink raw reply
* Re: [PATCH v5 01/51] x86/apic: Provide helpers to set local APIC timer period in hz and khz
From: Sean Christopherson @ 2026-07-06 18:00 UTC (permalink / raw)
To: Michael Kelley
Cc: Jonathan Corbet, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86@kernel.org, Kiryl Shutsemau,
Rick Edgecombe, K. Y. Srinivasan, Haiyang Zhang, Wei Liu,
Dexuan Cui, Long Li, Ajay Kaher, Alexey Makhalov, Jan Kiszka,
Andy Lutomirski, Peter Zijlstra, Juergen Gross, Daniel Lezcano,
John Stultz, Shuah Khan, H. Peter Anvin, Vitaly Kuznetsov,
Broadcom internal kernel review list, Boris Ostrovsky,
Stephen Boyd, linux-doc@vger.kernel.org, kvm@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-coco@lists.linux.dev,
linux-hyperv@vger.kernel.org, virtualization@lists.linux.dev,
xen-devel@lists.xenproject.org, Tom Lendacky, Nikunj A Dadhania,
David Woodhouse, David Woodhouse, Thomas Gleixner
In-Reply-To: <SN6PR02MB41578D3C34AB283B892C27A4D4F52@SN6PR02MB4157.namprd02.prod.outlook.com>
On Thu, Jul 02, 2026, Michael Kelley wrote:
> > @@ -796,6 +796,16 @@ bool __init apic_needs_pit(void)
> > return lapic_timer_period == 0;
> > }
> >
> > +void apic_set_timer_period_khz(u64 period_khz, const char *source)
> > +{
> > + lapic_timer_period = mul_u64_u32_div(period_khz, 1000, HZ);
> > +}
> > +
> > +void apic_set_timer_period_hz(u64 period_hz, const char *source)
> > +{
> > + lapic_timer_period = div_u64(period_hz, HZ);
> > +}
>
> A string "source" argument is passed in, but not used. Is there an
> envisioned future use? Also, this function doesn't output a pr_info()
> message like the existing Hyper-V and VMware code does.
It was a complete goof on my part (Sashiko also pointed out the oddity[*]). I
fully intended to log a message and provide equivalent Hyper-V/VMware behavior,
and totally spaced it.
[*] https://lore.kernel.org/all/20260701194621.4BD691F000E9@smtp.kernel.org
> I don't know that the message is all that useful, though I do remember one
> case where I was debugging some clock/timer issue when I looked at it.
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Sean Christopherson @ 2026-07-06 17:57 UTC (permalink / raw)
To: Peter Fang
Cc: Rick P Edgecombe, djbw@kernel.org, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Xiaoyao Li, Dave Hansen,
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: <20260704054342.GB2169894@pedri>
On Fri, Jul 03, 2026, Peter Fang wrote:
> On Wed, Jul 01, 2026 at 11:45:53AM -0700, Edgecombe, Rick P wrote:
> > On Wed, 2026-07-01 at 10:25 -0700, Sean Christopherson wrote:
> > > > > That is a good question. The answer is partly historical reasons, but I
> > > > > think the pros/cons don’t really move the needle too much.
> > > > >
> > > > > The main benefit of doing it with the host in the loop is that the guest
> > > > > side TDVMCALL quoting interface can stay the same. There is also a wrinkle
> > > > > in that there is a limited HW resource involved in the quoting,
> > >
> > > What is this magical resource?
> >
> > It's a HW crypto thing. I'll let Peter explain more.
>
> It's called S3M (Secured Startup Services). There was once a public
> document about it, but it was removed for some reason. It basically
> provides a number of boot and security services through dedicated HW.
> Attestation is one of them. This is part of our plan to develop new
> attestation schemes without using SGX.
>
> There was a lot of internal debate about how this HW should be managed.
> We decided to let the TDX module handle it and sort of hide it from the
> host kernel and only expose it as SEAMCALLs. It however impacted how the
> quoting SEAMCALLs look, so apologies for not calling out these factors.
>
> >
> > >
> > > > > so we want to do these operations one at a time. Having a mutex on the
> > > > > host is the KISS way of accomplishing some level of fairness for DOS
> > > > > prevention.
> > >
> > > At the risk of unintentionally causing effecitvely DoS by introducing a
> > > system-wide lock.
> >
> > The DoS that people are interested in trying to prevent is a TD getting starved
> > of quotes forever. So the lock is supposed to give some eventual fairness.
> > Eventually the guest gets its quote even if it has to wait.
> >
> > If instead contention throws a BUSY code to the guest, the guest spins trying.
> > That wouldn't have the guarantees.
> >
> > So I think the two options are: have some lock in the TDX module, or have a lock
> > of some sort in the host. I think actually a waiting lock in the TDX module is
> > possible. But I think there are tradeoffs besides where the extra code is. If
> > it's in Linux we get a lot more control, lockdep, etc.
> >
> > BTW the "historical reasons" part I mentioned involves a past effort to create
> > configurable host controlled policies around managing these resources. So part
> > of the simplification is doing a simple eventually fair global lock instead of
> > something more complicated.
>
> Yeah my bad for not including a lot more background. There were several
> design aspects that influenced this implementation. I'll do a better job
> of explaining them in the next version, but basically:
>
> 1. Performance requirements: For this first implementation, the
> priority is to not starve a legit guest from getting a quote, even
> when there's a malicious guest DoSing the host. A mutex is sort of
> a "fair enough queue" for this problem. A guest's authenticity can
> only be verified after the quote is generated.
>
> 2. HW limitation: The TDX module implements different attestation
> schemes, some of which use the S3M during TDH.QUOTE.GET. This
What is "the S3M" though? Is it a separate chip a la AMD's PSP/ASP? Is it a
per-package thing? Per-core? How is it accessed, and what are the "rules" for
for those accesses? What types of latencies are we looking at? What else uses
the S3M? Do we have to worry about contending with non-TDX usage?
> limits the amount of concurrency this SEAMCALL supports and is also
> part of why we chose a mutex for TDH.QUOTE.GET. (The shared buffer
> in core TDX doesn't seem great, so I will improve it.)
>
> 3. Host policies: Preserving the GetQuote GHCI also allows the host to
> participate in the quoting process, e.g. add host-controlled
> policies or fetch host-side quote information. It's a bit similar
> to SEV-SNP's KVM_EXIT_SNP_REQ_CERTS [1]. This series does not
> implement host policies though.
>
>
> Ultimately, there are ways to push more of this into the module, and the
> current design is kind of a tradeoff between the complexities of the
> work needed in the guest, the host, and the TDX module. Like Rick said,
> there were past versions that required way more work in the host and the
> guest, such as managing the S3M, new GHCI calls, etc.
>
> [1] Commit fa9893fadbc2 ("KVM: Introduce KVM_EXIT_SNP_REQ_CERTS for SNP
> certificate-fetching"
>
> >
^ permalink raw reply
* Re: [PATCH] KVM: x86/tdx: Do not print error message on non-present feature
From: Edgecombe, Rick P @ 2026-07-06 16:54 UTC (permalink / raw)
To: jirislaby@kernel.org, seanjc@google.com
Cc: Gao, Chao, x86@kernel.org, bp@alien8.de, kas@kernel.org,
dave.hansen@linux.intel.com, mingo@redhat.com,
dan.j.williams@intel.com, hpa@zytor.com, tglx@kernel.org,
linux-coco@lists.linux.dev, linux-kernel@vger.kernel.org,
kvm@vger.kernel.org
In-Reply-To: <20260702043204.81741-1-jirislaby@kernel.org>
On Thu, 2026-07-02 at 06:32 +0200, Jiri Slaby (SUSE) wrote:
> Before commit 165e77353831 below, the kernel did not print anything when
> X86_FEATURE_TDX_HOST_PLATFORM was not present in a cpu.
This isn't quite correct. It would print something if KVM tried to enable it,
which is controlled by a module param.
>
> After the commit, this is emitted to the log, with the error level:
> virt/tdx: TDX not supported by the host platform
>
> This is disturbing as it is not an error running CPUs without the
> feature.
Agree. If the kernel is now trying to enabling TDX automatically, it is not
error print worthy if it fails because TDX is not supported.
But after the below change, something is still printed if the kernel param is
used. Can we be a little more specific?
>
> Drop the error message completely to revert to the state before
> 165e77353831.
'state' is a little ambiguous here. Sounds like a full revert.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Fixes: 165e77353831 ("KVM: x86/tdx: Do VMXON and TDX-Module initialization during subsys init")
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Chao Gao <chao.gao@intel.com>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Sean Christopherson <seanjc@google.com>
>
> --
>
> Cc: Thomas Gleixner <tglx@kernel.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: x86@kernel.org
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Kiryl Shutsemau <kas@kernel.org>
> Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-coco@lists.linux.dev
> Cc: kvm@vger.kernel.org
> ---
> arch/x86/virt/vmx/tdx/tdx.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 42df8ea464c4..bdfc3bf12a9c 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -1228,10 +1228,8 @@ static __init int tdx_enable(void)
> enum cpuhp_state state;
> int ret;
>
> - if (!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM)) {
> - pr_err("TDX not supported by the host platform\n");
> + if (!cpu_feature_enabled(X86_FEATURE_TDX_HOST_PLATFORM))
> return -ENODEV;
> - }
Seems like a reasonable change to me, but the log is a bit misleading.
>
> if (!cpu_feature_enabled(X86_FEATURE_XSAVE)) {
> pr_err("XSAVE is required for TDX\n");
^ permalink raw reply
* Re: [PATCH v14 26/44] arm64: RMI: Allow populating initial contents
From: Ackerley Tng @ 2026-07-06 16:43 UTC (permalink / raw)
To: Steven Price, kvm, kvmarm
Cc: Catalin Marinas, Marc Zyngier, Will Deacon, James Morse,
Oliver Upton, Suzuki K Poulose, Zenghui Yu, linux-arm-kernel,
linux-kernel, Joey Gouly, Alexandru Elisei, Christoffer Dall,
Fuad Tabba, linux-coco, Ganapatrao Kulkarni, Gavin Shan,
Shanker Donthineni, Alper Gun, Aneesh Kumar K . V, Emi Kisanuki,
Vishal Annapurve, WeiLin.Chang, Lorenzo.Pieralisi2
In-Reply-To: <20260513131757.116630-27-steven.price@arm.com>
Steven Price <steven.price@arm.com> writes:
>
> [...snip...]
>
> +
> +static int populate_region_cb(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> + struct page *src_page, void *opaque)
> +{
> + unsigned long data_flags = *(unsigned long *)opaque;
> + phys_addr_t ipa = gfn_to_gpa(gfn);
> +
> + if (!src_page)
> + return -EOPNOTSUPP;
Came here from [0]!
Sean recently moved this check to before kvm_gmem_populate() was called
and returned -EINVAL [1]. I think the rationale would also apply for
CCA.
[0] https://lore.kernel.org/all/114e2488-97ed-4740-a8e8-1edd991f26c5@arm.com/
[1] https://lore.kernel.org/all/20260630213711.479692-3-seanjc@google.com/
> +
> + return realm_data_map_init(kvm, ipa, pfn, page_to_pfn(src_page),
> + data_flags);
> +}
> +
>
> [...snip...]
>
^ permalink raw reply
* Re: [PATCH v7 1/6] firmware: smccc: Add an Arm SMCCC bus
From: Sudeep Holla @ 2026-07-06 15:09 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-coco, linux-arm-kernel, linux-kernel, Sudeep Holla,
Catalin Marinas, Greg KH, Jeremy Linton, Jonathan Cameron,
Lorenzo Pieralisi, Mark Rutland, Will Deacon, Steven Price,
Suzuki K Poulose, Andre Przywara
In-Reply-To: <yq5a4iici3u3.fsf@kernel.org>
On Mon, Jul 06, 2026 at 08:05:00PM +0530, Aneesh Kumar K.V wrote:
> Sudeep Holla <sudeep.holla@kernel.org> writes:
>
> > On Thu, Jun 11, 2026 at 06:34:24PM +0530, Aneesh Kumar K.V (Arm) wrote:
>
> ...
>
> >> +#define to_arm_smccc_driver(d) \
> >> + container_of_const(d, struct arm_smccc_driver, driver)
> >> +
> >> +int arm_smccc_driver_register(struct arm_smccc_driver *driver,
> >> + struct module *owner, const char *mod_name);
> >> +void arm_smccc_driver_unregister(struct arm_smccc_driver *driver);
> >> +struct arm_smccc_device *arm_smccc_device_register(const char *name);
> >> +void arm_smccc_device_unregister(struct arm_smccc_device *smcc_dev);
> >> +
> >
> > I may be overthinking but what will happen if HAVE_ARM_SMCCC_DISCOVERY=n
> > and some driver is compiled using this header ? It should be fine if it
> > fails to compile, just thinking out loud if we need to handle that are not.
> > As long as all the drivers using these depends on HAVE_ARM_SMCCC_DISCOVERY
> > it should be fine I think.
>
> This will result in a build failure. The driver should either select
> HAVE_ARM_SMCCC_DISCOVERY or depend on HAVE_ARM_SMCCC_DISCOVERY.
>
That should be fine for now.
> >
> >> +#define arm_smccc_register(driver) \
> >> + arm_smccc_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
> >> +#define arm_smccc_unregister(driver) \
> >> + arm_smccc_driver_unregister(driver)
> >> +
> >> +#define module_arm_smccc_driver(__arm_smccc_driver) \
> >> + module_driver(__arm_smccc_driver, arm_smccc_register, \
> >> + arm_smccc_unregister)
> >> +
> >> +extern const struct bus_type arm_smccc_bus_type;
> >> +
> >> +#endif /* __LINUX_ARM_SMCCC_BUS_H */
> >> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> >> index 23ff24080dfd..c9cee8c5a0b2 100644
> >> --- a/include/linux/mod_devicetable.h
> >> +++ b/include/linux/mod_devicetable.h
> >
> > This file seems to be reworked recently, so you need to rebase it moving
> > smccc specific changes to separate file I think.
> >
>
> Do we need to add a separate file? I was able to rebase the series on
> 7.2-rc1
>
Then could be something changed between -rc1 to -rc2, I couldn't apply the
patch.
[...]
> >> +
> >> +/**
> >> + * struct arm_smccc_device_id - Arm SMCCC bus device identifier
> >> + * @name: SMCCC device name
> >> + * @driver_data: driver data
> >> + */
> >> +struct arm_smccc_device_id {
> >> + char name[ARM_SMCCC_NAME_SIZE];
> >> + kernel_ulong_t driver_data;
> >
> > Can't find any users of the above driver_data ?
> > Is it for future ? If so, can you add details on how it is supposed to be used
> > if you don't want to drop it.
> >
>
> That would depend on the driver, wouldn’t it? I was comparing this with
> struct auxiliary_device_id, and very few drivers use driver_data;
> mei_gsc_probe() is one example.
>
Indeed, but I don't see any attempt to use it currently which is fine but
I also fail how this can be used in future in SMCCC context.
IIUC driver_data is a per-match-entry cookie. It is useful when one driver
supports multiple IDs and wants each ID to select slightly different
behavior.
The issues as I see are:
1. No driver is using it in this series.
2. The SMCCC bus match code finds the matching table entry, but then
throws it away. It is not passed to probe for example.
> I am not sure there is a generic rule here. IIUC, it provides a place
> where a driver can add driver-specific data that can be used during the
> probe routine.
>
How is that possible unless we either add a function like
| const struct arm_smccc_device_id *
| arm_smccc_get_device_id(struct arm_smccc_device *sdev);
to get the pointer to the id_table or that driver_data from the probe.
Or change probe to receive the matched ID/driver_data:
int (*probe)(struct arm_smccc_device *sdev,
const struct arm_smccc_device_id *id);
Please drop it if you have no plans to use it or see any future usage.
--
Regards,
Sudeep
^ permalink raw reply
* Re: [PATCH v5 02/10] irqchip/gic-v3-its: Align shared ITS allocations to the CoCo shared granule size
From: Marc Zyngier @ 2026-07-06 14:44 UTC (permalink / raw)
To: Aneesh Kumar K.V
Cc: linux-coco, kvmarm, linux-arm-kernel, linux-kernel, iommu,
Catalin Marinas, Jason Gunthorpe, Marek Szyprowski, Robin Murphy,
Steven Price, Suzuki K Poulose, Thomas Gleixner, Will Deacon
In-Reply-To: <yq5a7bn8i69y.fsf@kernel.org>
On Mon, 06 Jul 2026 14:42:17 +0100,
Aneesh Kumar K.V <aneesh.kumar@kernel.org> wrote:
>
> Marc Zyngier <maz@kernel.org> writes:
>
> > On Mon, 06 Jul 2026 07:04:24 +0100,
> > "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
> >>
> >> ITS tables allocated by the coco guest are shared with the hypervisor.
> >> These allocations must satisfy the host shared-buffer granule size so that
> >> the full converted range is safe for host access.
> >>
> >> Allocate ITS pages using a size rounded up to the shared granule size and
> >> use the same allocation order when encrypting, decrypting and freeing the
> >> memory. Also grow the ITT cache in shared-granule sized chunks instead of
> >> assuming PAGE_SIZE is sufficient.
> >>
> >> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> >> ---
> >> drivers/irqchip/irq-gic-v3-its.c | 17 +++++++++++------
> >> 1 file changed, 11 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> >> index b57d81ad33a0..5d6d38aa0dae 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -213,16 +213,17 @@ static gfp_t gfp_flags_quirk;
> >> static struct page *its_alloc_pages_node(int node, gfp_t gfp,
> >> unsigned int order)
> >> {
> >> + unsigned int alloc_order;
> >> struct page *page;
> >> int ret = 0;
> >>
> >> - page = alloc_pages_node(node, gfp | gfp_flags_quirk, order);
> >> -
> >> + alloc_order = get_order(mem_cc_align_to_shared_granule(PAGE_SIZE << order));
> >> + page = alloc_pages_node(node, gfp | gfp_flags_quirk, alloc_order);
> >> if (!page)
> >> return NULL;
> >>
> >> ret = set_memory_decrypted((unsigned long)page_address(page),
> >> - 1 << order);
> >> + 1 << alloc_order);
> >> /*
> >> * If set_memory_decrypted() fails then we don't know what state the
> >> * page is in, so we can't free it. Instead we leak it.
> >> @@ -241,13 +242,16 @@ static struct page *its_alloc_pages(gfp_t gfp, unsigned int order)
> >>
> >> static void its_free_pages(void *addr, unsigned int order)
> >> {
> >> + int alloc_order;
> >> +
> >> + alloc_order = get_order(mem_cc_align_to_shared_granule(PAGE_SIZE << order));
> >> /*
> >> * If the memory cannot be encrypted again then we must leak the pages.
> >> * set_memory_encrypted() will already have WARNed.
> >> */
> >> - if (set_memory_encrypted((unsigned long)addr, 1 << order))
> >> + if (set_memory_encrypted((unsigned long)addr, 1 << alloc_order))
> >> return;
> >> - free_pages((unsigned long)addr, order);
> >> + free_pages((unsigned long)addr, alloc_order);
> >
> > The comments I had in [1] are still applicable.
> >
> > M.
> >
> > [1] https://lore.kernel.org/all/86zf2ozrb8.wl-maz@kernel.org/
> >
>
> I renamed it to alloc_order to make it clear that it refers to the
> allocation order rather than the requested order. If you feel strongly
> about not introducing another variable, I'll drop the new variable.
I do. At the very least, to avoid you changing the type of the order
in its_free_pages(). These things do matter.
M.
--
Without deviation from the norm, progress is not possible.
^ permalink raw reply
* Re: [PATCH v7 1/6] firmware: smccc: Add an Arm SMCCC bus
From: Aneesh Kumar K.V @ 2026-07-06 14:35 UTC (permalink / raw)
To: Sudeep Holla
Cc: linux-coco, linux-arm-kernel, linux-kernel, Sudeep Holla,
Catalin Marinas, Greg KH, Jeremy Linton, Jonathan Cameron,
Lorenzo Pieralisi, Mark Rutland, Will Deacon, Steven Price,
Suzuki K Poulose, Andre Przywara
In-Reply-To: <20260706-belligerent-excellent-saluki-a4e431@sudeepholla>
Sudeep Holla <sudeep.holla@kernel.org> writes:
> On Thu, Jun 11, 2026 at 06:34:24PM +0530, Aneesh Kumar K.V (Arm) wrote:
...
>> +static int arm_smccc_bus_match(struct device *dev,
>> + const struct device_driver *drv)
>> +{
>> + const struct arm_smccc_device_id *id_table;
>> + struct arm_smccc_device *smccc_dev = to_arm_smccc_device(dev);
>> +
>> + id_table = to_arm_smccc_driver(drv)->id_table;
>> + if (!id_table)
>> + return 0;
>> +
>
> Any reason for not having this check when registering the driver also so
> that it becomes a must to register ? I remember vaguely adding this as fix to
> FF-A driver recently.
>
I will add
int arm_smccc_driver_register(struct arm_smccc_driver *driver,
struct module *owner, const char *mod_name)
{
if (!driver->probe || !driver->id_table)
return -EINVAL;
>
>> + while (id_table->name[0]) {
>> + if (!strcmp(smccc_dev->name, id_table->name))
>> + return 1;
>> + id_table++;
>> + }
>> +
>> + return 0;
>> +}
>> +
...
>> +struct arm_smccc_device *arm_smccc_device_register(const char *name)
>> +{
>> + struct arm_smccc_device *smccc_dev;
>> + int id, ret;
>> +
>> + id = ida_alloc_min(&arm_smccc_bus_id, 1, GFP_KERNEL);
>> + if (id < 0)
>> + return ERR_PTR(id);
>> +
>> + smccc_dev = kzalloc_obj(*smccc_dev);
>> + if (!smccc_dev) {
>> + ida_free(&arm_smccc_bus_id, id);
>> + return ERR_PTR(-ENOMEM);
>> + }
>> +
>> + smccc_dev->id = id;
>> + if (strscpy(smccc_dev->name, name) < 0) {
>
> Since this is exported symbol and one can call arm_smccc_device_register(ptr)
> where ptr = NULL, won't it blow up then ? IIUC strscpy() handles NULL dst
> but doesn't check for NULL src.
>
Will add the NULL check.
>> + kfree(smccc_dev);
>> + ida_free(&arm_smccc_bus_id, id);
>> + return ERR_PTR(-EINVAL);
>> + }
>> + smccc_dev->dev.bus = &arm_smccc_bus_type;
>> + smccc_dev->dev.release = arm_smccc_release_device;
>> +
>> + ret = dev_set_name(&smccc_dev->dev, "%s-%d", smccc_dev->name, id);
>> + if (ret) {
>> + kfree(smccc_dev);
>> + ida_free(&arm_smccc_bus_id, id);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + ret = device_register(&smccc_dev->dev);
>> + if (ret) {
>> + put_device(&smccc_dev->dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + return smccc_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(arm_smccc_device_register);
...
>> +#define to_arm_smccc_driver(d) \
>> + container_of_const(d, struct arm_smccc_driver, driver)
>> +
>> +int arm_smccc_driver_register(struct arm_smccc_driver *driver,
>> + struct module *owner, const char *mod_name);
>> +void arm_smccc_driver_unregister(struct arm_smccc_driver *driver);
>> +struct arm_smccc_device *arm_smccc_device_register(const char *name);
>> +void arm_smccc_device_unregister(struct arm_smccc_device *smcc_dev);
>> +
>
> I may be overthinking but what will happen if HAVE_ARM_SMCCC_DISCOVERY=n
> and some driver is compiled using this header ? It should be fine if it
> fails to compile, just thinking out loud if we need to handle that are not.
> As long as all the drivers using these depends on HAVE_ARM_SMCCC_DISCOVERY
> it should be fine I think.
This will result in a build failure. The driver should either select
HAVE_ARM_SMCCC_DISCOVERY or depend on HAVE_ARM_SMCCC_DISCOVERY.
>
>> +#define arm_smccc_register(driver) \
>> + arm_smccc_driver_register(driver, THIS_MODULE, KBUILD_MODNAME)
>> +#define arm_smccc_unregister(driver) \
>> + arm_smccc_driver_unregister(driver)
>> +
>> +#define module_arm_smccc_driver(__arm_smccc_driver) \
>> + module_driver(__arm_smccc_driver, arm_smccc_register, \
>> + arm_smccc_unregister)
>> +
>> +extern const struct bus_type arm_smccc_bus_type;
>> +
>> +#endif /* __LINUX_ARM_SMCCC_BUS_H */
>> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
>> index 23ff24080dfd..c9cee8c5a0b2 100644
>> --- a/include/linux/mod_devicetable.h
>> +++ b/include/linux/mod_devicetable.h
>
> This file seems to be reworked recently, so you need to rebase it moving
> smccc specific changes to separate file I think.
>
Do we need to add a separate file? I was able to rebase the series on
7.2-rc1
modified include/linux/mod_devicetable.h
@@ -879,6 +879,19 @@ struct auxiliary_device_id {
kernel_ulong_t driver_data;
};
+#define ARM_SMCCC_NAME_SIZE 40
+#define ARM_SMCCC_MODULE_PREFIX "arm_smccc:"
+
+/**
+ * struct arm_smccc_device_id - Arm SMCCC bus device identifier
+ * @name: SMCCC device name
+ * @driver_data: driver data
+ */
+struct arm_smccc_device_id {
+ char name[ARM_SMCCC_NAME_SIZE];
+ kernel_ulong_t driver_data;
+};
+
/* Surface System Aggregator Module */
#define SSAM_MATCH_TARGET 0x1
>> @@ -876,6 +876,19 @@ struct auxiliary_device_id {
>> kernel_ulong_t driver_data;
>> };
>>
>> +#define ARM_SMCCC_NAME_SIZE 40
>> +#define ARM_SMCCC_MODULE_PREFIX "arm_smccc:"
>> +
>> +/**
>> + * struct arm_smccc_device_id - Arm SMCCC bus device identifier
>> + * @name: SMCCC device name
>> + * @driver_data: driver data
>> + */
>> +struct arm_smccc_device_id {
>> + char name[ARM_SMCCC_NAME_SIZE];
>> + kernel_ulong_t driver_data;
>
> Can't find any users of the above driver_data ?
> Is it for future ? If so, can you add details on how it is supposed to be used
> if you don't want to drop it.
>
That would depend on the driver, wouldn’t it? I was comparing this with
struct auxiliary_device_id, and very few drivers use driver_data;
mei_gsc_probe() is one example.
I am not sure there is a generic rule here. IIUC, it provides a place
where a driver can add driver-specific data that can be used during the
probe routine.
>> +};
>> +
>> /* Surface System Aggregator Module */
>>
>> #define SSAM_MATCH_TARGET 0x1
>> diff --git a/scripts/mod/devicetable-offsets.c b/scripts/mod/devicetable-offsets.c
>> index b4178c42d08f..a485011ff137 100644
>> --- a/scripts/mod/devicetable-offsets.c
>> +++ b/scripts/mod/devicetable-offsets.c
>> @@ -254,6 +254,9 @@ int main(void)
>> DEVID(auxiliary_device_id);
>> DEVID_FIELD(auxiliary_device_id, name);
>>
>> + DEVID(arm_smccc_device_id);
>> + DEVID_FIELD(arm_smccc_device_id, name);
>> +
>> DEVID(ssam_device_id);
>> DEVID_FIELD(ssam_device_id, match_flags);
>> DEVID_FIELD(ssam_device_id, domain);
-aneesh
^ 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