* Re: [PATCH v7 03/22] iommu/dma: Check atomic pool allocation result directly
From: Jason Gunthorpe @ 2026-07-13 17:57 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: iommu, linux-arm-kernel, linux-kernel, linux-coco, Robin Murphy,
Marek Szyprowski, Will Deacon, Marc Zyngier, Steven Price,
Suzuki K Poulose, Catalin Marinas, Jiri Pirko, Mostafa Saleh,
Petr Tesarik, Alexey Kardashevskiy, 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, Michael Kelley
In-Reply-To: <20260701054926.825925-4-aneesh.kumar@kernel.org>
On Wed, Jul 01, 2026 at 11:19:07AM +0530, Aneesh Kumar K.V (Arm) wrote:
> The non-blocking, non-coherent allocation path uses dma_alloc_from_pool(),
> which returns the allocated page and fills cpu_addr only on success.
>
> Do not rely on cpu_addr to detect allocation failure in this path. Check
> the returned page directly before using it for the IOMMU mapping.
>
> Fixes: 9420139f516d ("dma-pool: fix coherent pool allocations for IOMMU mappings")
> Tested-by: Michael Kelley <mhklinux@outlook.com>
> Tested-by: Mostafa Saleh <smostafa@google.com>
> Reviewed-by: Petr Tesarik <ptesarik@suse.com>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
> drivers/iommu/dma-iommu.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [PATCH v7 11/22] dma-pool: track decrypted atomic pools and select them via attrs
From: Jason Gunthorpe @ 2026-07-13 17:56 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: iommu, linux-arm-kernel, linux-kernel, linux-coco, Robin Murphy,
Marek Szyprowski, Will Deacon, Marc Zyngier, Steven Price,
Suzuki K Poulose, Catalin Marinas, Jiri Pirko, Mostafa Saleh,
Petr Tesarik, Alexey Kardashevskiy, 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, Jiri Pirko,
Michael Kelley
In-Reply-To: <20260701054926.825925-12-aneesh.kumar@kernel.org>
On Wed, Jul 01, 2026 at 11:19:15AM +0530, Aneesh Kumar K.V (Arm) wrote:
> -static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> +static int atomic_pool_expand(struct dma_gen_pool *dma_pool, size_t pool_size,
> gfp_t gfp)
> {
> unsigned int order;
> @@ -114,14 +120,17 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> * Memory in the atomic DMA pools must be unencrypted, the pools do not
> * shrink so no re-encryption occurs in dma_direct_free().
> */
> - ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> - 1 << order);
> - if (ret) {
> - leak_pages = true;
> - goto remove_mapping;
> + if (dma_pool->cc_shared) {
> + ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> + 1 << order);
> + if (ret) {
> + leak_pages = true;
> + goto remove_mapping;
> + }
> }
> - ret = gen_pool_add_virt(pool, (unsigned long)addr, page_to_phys(page),
> - pool_size, NUMA_NO_NODE);
> +
> + ret = gen_pool_add_virt(dma_pool->pool, (unsigned long)addr,
> + page_to_phys(page), pool_size, NUMA_NO_NODE);
> if (ret)
> goto encrypt_mapping;
>
> @@ -129,12 +138,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> return 0;
>
> encrypt_mapping:
> - ret = set_memory_encrypted((unsigned long)page_to_virt(page),
> - 1 << order);
> - if (WARN_ON_ONCE(ret)) {
> - /* Decrypt succeeded but encrypt failed, purposely leak */
> + if (dma_pool->cc_shared &&
> + set_memory_encrypted((unsigned long)page_to_virt(page), 1 << order))
> leak_pages = true;
> - }
> +
Was it intentional to remove the WARN_ON and comment ?
Jason
^ permalink raw reply
* Re: [PATCH v7 02/22] dma-pool: fix page leak in atomic_pool_expand() cleanup
From: Jason Gunthorpe @ 2026-07-13 17:54 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: iommu, linux-arm-kernel, linux-kernel, linux-coco, Robin Murphy,
Marek Szyprowski, Will Deacon, Marc Zyngier, Steven Price,
Suzuki K Poulose, Catalin Marinas, Jiri Pirko, Mostafa Saleh,
Petr Tesarik, Alexey Kardashevskiy, 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, Michael Kelley
In-Reply-To: <20260701054926.825925-3-aneesh.kumar@kernel.org>
On Wed, Jul 01, 2026 at 11:19:06AM +0530, Aneesh Kumar K.V (Arm) wrote:
> @@ -115,8 +116,10 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> */
> ret = set_memory_decrypted((unsigned long)page_to_virt(page),
> 1 << order);
> - if (ret)
> + if (ret) {
> + leak_pages = true;
> goto remove_mapping;
> + }
Truely these _set_memory_decrypted() things are an insane API. So a if
it fails to decrypt it can be in any messy state?
> @@ -130,14 +133,15 @@ static int atomic_pool_expand(struct gen_pool *pool, size_t pool_size,
> 1 << order);
> if (WARN_ON_ONCE(ret)) {
> /* Decrypt succeeded but encrypt failed, purposely leak */
> - goto out;
> + leak_pages = true;
At least this one makes some sense..
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [PATCH 2/2] virt: tdx-guest: Allocate Quote buffer dynamically
From: Kuppuswamy Sathyanarayanan @ 2026-07-13 17:27 UTC (permalink / raw)
To: Peter Fang, Dave Hansen, Kiryl Shutsemau, Rick Edgecombe
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, linux-kernel, linux-coco, kvm
In-Reply-To: <20260612110853.3188196-3-peter.fang@intel.com>
Hi Peter,
On 6/12/2026 4:08 AM, Peter Fang wrote:
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> The TDX attestation driver currently uses a fixed 128 KB Quote buffer
> shared with the host VMM. This may be too small for Quotes using schemes
> such as post-quantum cryptography (PQC), where certificate chains can
> increase the Quote size to several megabytes.
>
> Allocate the Quote buffer based on the size reported by the TDX module
> instead of always reserving a fixed-size buffer. This avoids wasting
> memory on platforms that do not require larger Quotes. Older platforms
> fall back to the default 128 KB buffer.
>
> Because the Quote buffer must be physically contiguous, its size is
> bound by the buddy allocator's maximum page order (4 MB), which should
> be sufficient for current attestation needs.
>
> struct tdx_quote_buf has a trailing flexible array, so use offsetof()
> instead of sizeof() to calculate the header size.
>
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Assisted-by: Claude:claude-opus-4-7
> Assisted-by: GitHub Copilot:gpt-5.4
> Signed-off-by: Peter Fang <peter.fang@intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> drivers/virt/coco/tdx-guest/tdx-guest.c | 52 ++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index a9ecc46df187..162fb47f3fae 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -163,7 +163,7 @@ static void tdx_mr_deinit(const struct attribute_group *mr_grp)
> * DICE-based attestation uses layered evidence that requires
> * larger Quote size (~100K).
> */
> -#define GET_QUOTE_BUF_SIZE SZ_128K
> +#define GET_QUOTE_DEFAULT_BUF_SIZE SZ_128K
>
> #define GET_QUOTE_CMD_VER 1
>
> @@ -171,7 +171,7 @@ static void tdx_mr_deinit(const struct attribute_group *mr_grp)
> #define GET_QUOTE_SUCCESS 0
> #define GET_QUOTE_IN_FLIGHT 0xffffffffffffffff
>
> -#define TDX_QUOTE_MAX_LEN (GET_QUOTE_BUF_SIZE - sizeof(struct tdx_quote_buf))
> +#define TDX_QUOTE_BUF_LEN(n) (offsetof(struct tdx_quote_buf, data) + (n))
>
> /* struct tdx_quote_buf: Format of Quote request buffer.
> * @version: Quote format version, filled by TD.
> @@ -192,8 +192,9 @@ struct tdx_quote_buf {
> u8 data[];
> };
>
> -/* Quote data buffer */
> +/* Quote data buffer and size */
> static void *quote_data;
> +static size_t quote_data_size;
>
> /* Lock to streamline quote requests */
> static DEFINE_MUTEX(quote_lock);
> @@ -210,9 +211,8 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
> USER_SOCKPTR(req->tdreport));
> }
>
> -static void free_quote_buf(void *buf)
> +static void free_quote_buf(void *buf, size_t len)
> {
> - size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> unsigned int count = len >> PAGE_SHIFT;
>
> if (set_memory_encrypted((unsigned long)buf, count)) {
> @@ -223,19 +223,43 @@ static void free_quote_buf(void *buf)
> free_pages_exact(buf, len);
> }
>
> -static void *alloc_quote_buf(void)
> +static size_t get_quote_buf_size(void)
> {
> - size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> - unsigned int count = len >> PAGE_SHIFT;
> + size_t buf_sz = GET_QUOTE_DEFAULT_BUF_SIZE;
> + u32 quote_sz;
> +
> + quote_sz = tdx_get_max_quote_size();
> +
> + if (quote_sz)
> + /* Reported size does not include GetQuote header */
> + buf_sz = TDX_QUOTE_BUF_LEN(quote_sz);
> +
> + return PAGE_ALIGN(buf_sz);
> +}
> +
> +static void *alloc_quote_buf(size_t *buflen)
> +{
> + unsigned int count;
> + size_t len;
> void *addr;
>
> + len = get_quote_buf_size();
> +
> + /*
> + * This fails if the requested size exceeds the buddy allocator's
> + * maximum order (order-10, 4MB).
> + */
> addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
> if (!addr)
> return NULL;
>
> + count = len >> PAGE_SHIFT;
> +
> if (set_memory_decrypted((unsigned long)addr, count))
> return NULL;
>
> + *buflen = len;
> +
> return addr;
> }
>
> @@ -286,7 +310,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
> if (desc->inblob_len != TDX_REPORTDATA_LEN)
> return -EINVAL;
>
> - memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
> + memset(quote_data, 0, quote_data_size);
>
> /* Update Quote buffer header */
> quote_buf->version = GET_QUOTE_CMD_VER;
> @@ -297,7 +321,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
> if (ret)
> return ret;
>
> - err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
> + err = tdx_hcall_get_quote(quote_data, quote_data_size);
> if (err) {
> pr_err("GetQuote hypercall failed, status:%llx\n", err);
> return -EIO;
> @@ -316,7 +340,7 @@ static int tdx_report_new_locked(struct tsm_report *report, void *data)
>
> out_len = READ_ONCE(quote_buf->out_len);
>
> - if (out_len > TDX_QUOTE_MAX_LEN)
> + if (TDX_QUOTE_BUF_LEN(out_len) > quote_data_size)
> return -EFBIG;
Nit: I think this check will be more readable if you can rename
quote_data_size to quote_buf_size (since it holds total buffer
size).
>
> buf = kvmemdup(quote_buf->data, out_len, GFP_KERNEL);
> @@ -418,7 +442,7 @@ static int __init tdx_guest_init(void)
> if (ret)
> goto deinit_mr;
>
> - quote_data = alloc_quote_buf();
> + quote_data = alloc_quote_buf("e_data_size);
> if (!quote_data) {
> pr_err("Failed to allocate Quote buffer\n");
> ret = -ENOMEM;
> @@ -432,7 +456,7 @@ static int __init tdx_guest_init(void)
> return 0;
>
> free_quote:
> - free_quote_buf(quote_data);
> + free_quote_buf(quote_data, quote_data_size);
> free_misc:
> misc_deregister(&tdx_misc_dev);
> deinit_mr:
> @@ -445,7 +469,7 @@ module_init(tdx_guest_init);
> static void __exit tdx_guest_exit(void)
> {
> tsm_report_unregister(&tdx_tsm_ops);
> - free_quote_buf(quote_data);
> + free_quote_buf(quote_data, quote_data_size);
> misc_deregister(&tdx_misc_dev);
> tdx_mr_deinit(tdx_attr_groups[0]);
> }
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH 1/2] x86/tdx: Add helper to query maximum TD Quote size
From: Kuppuswamy Sathyanarayanan @ 2026-07-13 16:01 UTC (permalink / raw)
To: Peter Fang, Dave Hansen, Kiryl Shutsemau, Rick Edgecombe
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, linux-kernel, linux-coco, kvm
In-Reply-To: <20260612110853.3188196-2-peter.fang@intel.com>
On 6/12/2026 4:08 AM, Peter Fang wrote:
> TDX attestation blob ("TD Quote") sizes can grow with newer
> cryptographic schemes, so guests can no longer rely on a fixed-size
> buffer for the Quote.
>
> Newer TDX modules report the maximum TD Quote size via a TD-scope
> metadata field. Add a helper to query it instead of exposing tdg_vm_rd()
> directly, as it can read arbitrary metadata fields.
>
> Thanks to Xu Yilun for suggesting this.
I'm not sure what the original suggestion was. Perhaps a link to the discussion
(or Xu Yilun's email) would be helpful for context.
>
> Assisted-by: Claude:claude-opus-4-7
> Assisted-by: GitHub Copilot:gpt-5.4
> Signed-off-by: Peter Fang <peter.fang@intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> arch/x86/coco/tdx/tdx.c | 19 +++++++++++++++++++
> arch/x86/include/asm/shared/tdx.h | 1 +
> arch/x86/include/asm/tdx.h | 2 ++
> 3 files changed, 22 insertions(+)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 186915a17c50..88c66c46e70a 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -197,6 +197,25 @@ u64 tdx_hcall_get_quote(u8 *buf, size_t size)
> }
> EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
>
> +/**
> + * tdx_get_max_quote_size() - Get the maximum TD Quote size
> + *
> + * Read the maximum size of a TD Quote from a 4-byte TD metadata field. The TDX
> + * guest driver uses it to size the buffer for Quote retrieval. Older TDX
> + * modules do not support this field and return an error.
> + *
> + * Return: Maximum Quote size in bytes on success, or 0 on failure.
> + */
> +u32 tdx_get_max_quote_size(void)
> +{
> + u64 val, ret;
> +
> + ret = tdg_vm_rd(TDCS_QUOTE_MAX_SIZE, &val);
> +
> + return ret ? 0 : (u32)val;
> +}
> +EXPORT_SYMBOL_GPL(tdx_get_max_quote_size);
> +
> static void __noreturn tdx_panic(const char *msg)
> {
> struct tdx_module_args args = {
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 049638e3da74..2880f493a8e5 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -49,6 +49,7 @@
> /* TDX TD-Scope Metadata. To be used by TDG.VM.WR and TDG.VM.RD */
> #define TDCS_CONFIG_FLAGS 0x1110000300000016
> #define TDCS_TD_CTLS 0x1110000300000017
> +#define TDCS_QUOTE_MAX_SIZE 0x9010000200000008
> #define TDCS_NOTIFY_ENABLES 0x9100000000000010
> #define TDCS_TOPOLOGY_ENUM_CONFIGURED 0x9100000000000019
>
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index a149740b24e8..ac39674c9479 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -72,6 +72,8 @@ int tdx_mcall_extend_rtmr(u8 index, u8 *data);
>
> u64 tdx_hcall_get_quote(u8 *buf, size_t size);
>
> +u32 tdx_get_max_quote_size(void);
> +
> void __init tdx_dump_attributes(u64 td_attr);
> void __init tdx_dump_td_ctls(u64 td_ctls);
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED
From: Jason Gunthorpe @ 2026-07-13 13:56 UTC (permalink / raw)
To: 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, Alexey Kardashevskiy, 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, Jiri Pirko,
Michael Kelley
In-Reply-To: <yq5atsq6hdaa.fsf@kernel.org>
On Sat, Jul 11, 2026 at 12:39:49AM +0530, Aneesh Kumar K.V wrote:
> > But if you really want to it should be
> > cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) *only* and get rid of the wrong
> > force_dma_unencrypted().
> >
>
> Ok, I will update the changes to so we preserve the previous behaviour
> for SME?
>
> if (attrs & DMA_ATTR_MMIO) {
> /*
> * For host memory encryption treat MMIO memory as shared
> */
> if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
> attrs |= DMA_ATTR_CC_SHARED;
> }
I'm OK with this, with the expectation we should aim to remove it when
fixing the callers.
> > But IMHO, I'd rather this series treat ATTR_MMIO as private MMIO and
> > ATTR_MMIO|CC_SHARED as shared MMIO and that's the right and correct
> > thing for the DMA API.
> >
>
> If you think the rest of the series is ready for upstream, could you
> please ack it so it can be picked up for the next merge window? I'll
> repost v8, rebased on top of the pKVM topic branch.
Yeah
Jason
^ permalink raw reply
* [PATCH v6 3/3] x86/tdx: Fix zero-extension for 32-bit port I/O
From: Kiryl Shutsemau @ 2026-07-13 13:37 UTC (permalink / raw)
To: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86
Cc: Sean Christopherson, Paolo Bonzini, Kuppuswamy Sathyanarayanan,
Kai Huang, Xiaoyao Li, Rick Edgecombe, Binbin Wu, David Laight,
Andi Kleen, Dan Williams, Borys Tsyrulnikov, kvm, linux-coco,
linux-kernel, stable, Kiryl Shutsemau (Meta)
In-Reply-To: <20260713133753.223947-1-kirill@shutemov.name>
From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
According to x86 architecture rules, 32-bit operations zero-extend the
result to 64 bits. The current implementation of handle_in() only masks
the lower 32 bits, which preserves the upper 32 bits of RAX when a
32-bit port IN instruction is emulated.
Use insn_assign_reg() to write the result back into RAX with proper
partial-register-write semantics: 1- and 2-byte forms leave the upper
bits untouched, the 4-byte form zero-extends to the full register.
Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Cc: stable@vger.kernel.org
---
arch/x86/coco/tdx/tdx.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index b8bbd715fb62..f904a636d449 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -694,8 +694,8 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
.r13 = PORT_READ,
.r14 = port,
};
- u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
bool success;
+ u64 val;
/*
* Emulate the I/O read via hypercall. More info about ABI can be found
@@ -703,11 +703,9 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
* "TDG.VP.VMCALL<Instruction.IO>".
*/
success = !__tdx_hypercall(&args);
+ val = success ? args.r11 : 0;
- /* Update part of the register affected by the emulated instruction */
- regs->ax &= ~mask;
- if (success)
- regs->ax |= args.r11 & mask;
+ insn_assign_reg(®s->ax, val, size);
return success;
}
--
2.54.0
^ permalink raw reply related
* [PATCH v6 2/3] x86/insn-eval: Move assign_register() out of KVM as insn_assign_reg()
From: Kiryl Shutsemau @ 2026-07-13 13:37 UTC (permalink / raw)
To: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86
Cc: Sean Christopherson, Paolo Bonzini, Kuppuswamy Sathyanarayanan,
Kai Huang, Xiaoyao Li, Rick Edgecombe, Binbin Wu, David Laight,
Andi Kleen, Dan Williams, Borys Tsyrulnikov, kvm, linux-coco,
linux-kernel, stable, Kiryl Shutsemau (Meta)
In-Reply-To: <20260713133753.223947-1-kirill@shutemov.name>
From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
KVM's instruction emulator has a small helper, assign_register(), that
writes a value into a register following the x86 rules for writes to
general-purpose registers: an 8- or 16-bit write leaves the rest of the
register untouched, a 32-bit write zero-extends the result to 64 bits,
and a 64-bit write replaces the whole register.
The TDX guest #VE handler needs the same logic for port I/O emulation
to get 32-bit zero-extension right. Rather than add a third copy of
the same switch, move the helper verbatim to <asm/insn-eval.h>, rename
it to insn_assign_reg(), and route KVM's callers through it.
Add <asm/insn.h> to the header's includes so it builds standalone in
callers that have not pulled it in transitively.
No functional change.
Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Acked-by: Sean Christopherson <seanjc@google.com>
Cc: stable@vger.kernel.org # prerequisite for the following 32-bit port I/O zero-extension fix
---
arch/x86/include/asm/insn-eval.h | 36 ++++++++++++++++++++++++++++++++
arch/x86/kvm/emulate.c | 26 ++++-------------------
2 files changed, 40 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/insn-eval.h b/arch/x86/include/asm/insn-eval.h
index 4733e9064ee5..ae05647a0afb 100644
--- a/arch/x86/include/asm/insn-eval.h
+++ b/arch/x86/include/asm/insn-eval.h
@@ -9,6 +9,7 @@
#include <linux/compiler.h>
#include <linux/bug.h>
#include <linux/err.h>
+#include <asm/insn.h>
#include <asm/ptrace.h>
#define INSN_CODE_SEG_ADDR_SZ(params) ((params >> 4) & 0xf)
@@ -46,4 +47,39 @@ enum insn_mmio_type insn_decode_mmio(struct insn *insn, int *bytes);
bool insn_is_nop(struct insn *insn);
+/*
+ * Write @val into *@reg following the x86 rules for writes to
+ * general-purpose registers (Intel SDM Vol. 1, "General-Purpose
+ * Registers in 64-Bit Mode"): an 8- or 16-bit write leaves the rest of
+ * the register untouched, a 32-bit write zero-extends the result into
+ * the upper 32 bits, and a 64-bit write replaces the whole register.
+ *
+ * @bytes is the width of the write, not a property of the instruction:
+ * an instruction that, say, sign-extends a 32-bit immediate into a
+ * 64-bit register does a 64-bit write here.
+ *
+ * @reg need not be 8-byte aligned: KVM's instruction emulator offsets
+ * the pointer by one byte to address the high-byte registers (AH, CH,
+ * DH, BH). Use narrow stores for the sub-word cases so the access
+ * width matches @bytes and the adjacent bytes are left alone.
+ */
+static inline void insn_assign_reg(unsigned long *reg, u64 val, int bytes)
+{
+ switch (bytes) {
+ case 1:
+ *(u8 *)reg = (u8)val;
+ break;
+ case 2:
+ *(u16 *)reg = (u16)val;
+ break;
+ case 4:
+ /* A 32-bit write zero-extends into the upper 32 bits. */
+ *reg = (u32)val;
+ break;
+ case 8:
+ *reg = val;
+ break;
+ }
+}
+
#endif /* _ASM_X86_INSN_EVAL_H */
diff --git a/arch/x86/kvm/emulate.c b/arch/x86/kvm/emulate.c
index b566ab5c7515..c6dcb5ac48af 100644
--- a/arch/x86/kvm/emulate.c
+++ b/arch/x86/kvm/emulate.c
@@ -24,6 +24,7 @@
#include "kvm_emulate.h"
#include <linux/stringify.h>
#include <asm/debugreg.h>
+#include <asm/insn-eval.h>
#include <asm/nospec-branch.h>
#include <asm/ibt.h>
#include <asm/text-patching.h>
@@ -439,25 +440,6 @@ static void assign_masked(ulong *dest, ulong src, ulong mask)
*dest = (*dest & ~mask) | (src & mask);
}
-static void assign_register(unsigned long *reg, u64 val, int bytes)
-{
- /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
- switch (bytes) {
- case 1:
- *(u8 *)reg = (u8)val;
- break;
- case 2:
- *(u16 *)reg = (u16)val;
- break;
- case 4:
- *reg = (u32)val;
- break; /* 64b: zero-extend */
- case 8:
- *reg = val;
- break;
- }
-}
-
static inline unsigned long ad_mask(struct x86_emulate_ctxt *ctxt)
{
return (1UL << (ctxt->ad_bytes << 3)) - 1;
@@ -505,7 +487,7 @@ register_address_increment(struct x86_emulate_ctxt *ctxt, int reg, int inc)
{
ulong *preg = reg_rmw(ctxt, reg);
- assign_register(preg, *preg + inc, ctxt->ad_bytes);
+ insn_assign_reg(preg, *preg + inc, ctxt->ad_bytes);
}
static void rsp_increment(struct x86_emulate_ctxt *ctxt, int inc)
@@ -1767,7 +1749,7 @@ static int load_segment_descriptor(struct x86_emulate_ctxt *ctxt,
static void write_register_operand(struct operand *op)
{
- return assign_register(op->addr.reg, op->val, op->bytes);
+ return insn_assign_reg(op->addr.reg, op->val, op->bytes);
}
static int writeback(struct x86_emulate_ctxt *ctxt, struct operand *op)
@@ -2008,7 +1990,7 @@ static int em_popa(struct x86_emulate_ctxt *ctxt)
rc = emulate_pop(ctxt, &val, ctxt->op_bytes);
if (rc != X86EMUL_CONTINUE)
break;
- assign_register(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
+ insn_assign_reg(reg_rmw(ctxt, reg), val, ctxt->op_bytes);
--reg;
}
return rc;
--
2.54.0
^ permalink raw reply related
* [PATCH v6 1/3] x86/tdx: Fix off-by-one in port I/O handling
From: Kiryl Shutsemau @ 2026-07-13 13:37 UTC (permalink / raw)
To: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86
Cc: Sean Christopherson, Paolo Bonzini, Kuppuswamy Sathyanarayanan,
Kai Huang, Xiaoyao Li, Rick Edgecombe, Binbin Wu, David Laight,
Andi Kleen, Dan Williams, Borys Tsyrulnikov, kvm, linux-coco,
linux-kernel, stable, Kiryl Shutsemau (Meta)
In-Reply-To: <20260713133753.223947-1-kirill@shutemov.name>
From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
handle_in() and handle_out() in arch/x86/coco/tdx/tdx.c use:
u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
GENMASK(h, l) includes bit h. For size=1 (INB), this produces
GENMASK(8, 0) = 0x1FF (9 bits) instead of GENMASK(7, 0) = 0xFF (8
bits). The mask is one bit too wide for all I/O sizes.
Fix the mask calculation.
Fixes: 03149948832a ("x86/tdx: Port I/O: Add runtime hypercalls")
Reported-by: Borys Tsyrulnikov <tsyrulnikov.borys@gmail.com>
Link: https://lore.kernel.org/all/CAKw_Dz96rfSQc6Rn+9QBcUFHhmkK+9zu+P=bxowfZwxrATCBRg@mail.gmail.com/
Signed-off-by: Kiryl Shutsemau (Meta) <kas@kernel.org>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: stable@vger.kernel.org
---
arch/x86/coco/tdx/tdx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 29b6f1ed59ec..b8bbd715fb62 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -694,7 +694,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
.r13 = PORT_READ,
.r14 = port,
};
- u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+ u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
bool success;
/*
@@ -714,7 +714,7 @@ static bool handle_in(struct pt_regs *regs, int size, int port)
static bool handle_out(struct pt_regs *regs, int size, int port)
{
- u64 mask = GENMASK(BITS_PER_BYTE * size, 0);
+ u64 mask = GENMASK(BITS_PER_BYTE * size - 1, 0);
/*
* Emulate the I/O write via hypercall. More info about ABI can be found
--
2.54.0
^ permalink raw reply related
* [PATCH v6 0/3] x86/tdx: Fix port I/O handling bugs
From: Kiryl Shutsemau @ 2026-07-13 13:37 UTC (permalink / raw)
To: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86
Cc: Sean Christopherson, Paolo Bonzini, Kuppuswamy Sathyanarayanan,
Kai Huang, Xiaoyao Li, Rick Edgecombe, Binbin Wu, David Laight,
Andi Kleen, Dan Williams, Borys Tsyrulnikov, kvm, linux-coco,
linux-kernel, stable, Kiryl Shutsemau (Meta)
From: "Kiryl Shutsemau (Meta)" <kas@kernel.org>
Three fixes for emulated port I/O in the TDX guest #VE handler.
Patch 1 fixes an off-by-one in the GENMASK() used by handle_in() and
handle_out(): the mask was one bit too wide for every I/O size.
Patch 3 fixes 32-bit port IN to zero-extend into RAX, per x86
semantics, instead of preserving the upper 32 bits. To avoid
open-coding the partial-register-write rules, patch 2 first moves KVM's
assign_register() into <asm/insn-eval.h> as insn_assign_reg() so both
KVM and the #VE handler can share it.
Changes since v5:
- Patch 2: reword the shortlog and comment; no functional change
(Sean, David Laight). Collect Acked-by from Sean.
- Patches 1 and 3 unchanged.
v5: https://lore.kernel.org/all/20260701110547.764083-1-kirill@shutemov.name/
v4: https://lore.kernel.org/all/cover.1780584300.git.kas@kernel.org/
Kiryl Shutsemau (Meta) (3):
x86/tdx: Fix off-by-one in port I/O handling
x86/insn-eval: Move assign_register() out of KVM as insn_assign_reg()
x86/tdx: Fix zero-extension for 32-bit port I/O
arch/x86/coco/tdx/tdx.c | 10 ++++-----
arch/x86/include/asm/insn-eval.h | 36 ++++++++++++++++++++++++++++++++
arch/x86/kvm/emulate.c | 26 ++++-------------------
3 files changed, 44 insertions(+), 28 deletions(-)
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
--
2.54.0
^ permalink raw reply
* Re: [PATCH v5 3/5] iommufd/viommu: Keep a reference to the KVM file
From: Souradeep Chakrabarti @ 2026-07-13 13:18 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: linux-coco, iommu, linux-kernel, kvm, Alexey Kardashevskiy,
Bjorn Helgaas, Dan Williams, Jason Gunthorpe, Joerg Roedel,
Jonathan Cameron, Kevin Tian, Nicolin Chen, Samuel Ortiz,
Steven Price, Suzuki K Poulose, Will Deacon, Xu Yilun,
Shameer Kolothum, Paolo Bonzini, Tony Krowiak, Halil Pasic,
Jason Herne, Harald Freudenberger, Holger Dengler, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Alex Williamson, Matthew Rosato, Farhan Ali,
Eric Farman, linux-s390
In-Reply-To: <20260525154816.1029642-4-aneesh.kumar@kernel.org>
On Mon, May 25, 2026 at 09:18:14PM +0530, Aneesh Kumar K.V (Arm) wrote:
> From: Nicolin Chen <nicolinc@nvidia.com>
>
> The TSM vDevice operations need access to the KVM associated with the
> device's vIOMMU. Save the device's KVM file in the iommufd_viommu when the
> vIOMMU is allocated, and take a file reference so it remains valid for the
> lifetime of the vIOMMU.
>
> Release the reference when the vIOMMU is destroyed.
>
> Based on an original patch by Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>
> [nicolinc: hold kvm's users_count]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> [aneesh.kumar: Switch to use kvm_file]
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
> drivers/iommu/iommufd/viommu.c | 5 +++++
> include/linux/iommufd.h | 1 +
> 2 files changed, 6 insertions(+)
>
> diff --git a/drivers/iommu/iommufd/viommu.c b/drivers/iommu/iommufd/viommu.c
> index 4081deda9b33..bf5d58d55939 100644
> --- a/drivers/iommu/iommufd/viommu.c
> +++ b/drivers/iommu/iommufd/viommu.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /* Copyright (c) 2024, NVIDIA CORPORATION & AFFILIATES
> */
> +#include <linux/file.h>
> #include "iommufd_private.h"
>
> void iommufd_viommu_destroy(struct iommufd_object *obj)
> @@ -11,6 +12,8 @@ void iommufd_viommu_destroy(struct iommufd_object *obj)
> if (viommu->ops && viommu->ops->destroy)
> viommu->ops->destroy(viommu);
> refcount_dec(&viommu->hwpt->common.obj.users);
> + if (viommu->kvm_file)
> + fput(viommu->kvm_file);
> xa_destroy(&viommu->vdevs);
> }
>
> @@ -76,6 +79,8 @@ int iommufd_viommu_alloc_ioctl(struct iommufd_ucmd *ucmd)
> }
>
> xa_init(&viommu->vdevs);
> + if (idev->kvm_file)
> + viommu->kvm_file = get_file(idev->kvm_file);
There is now no uapi path at all for a non-KVM VMM to associate its VM
with a viommu. Can we (re)introduce an explicit, neutral
association, that accepts a KVM fd or another VMM's VM fd, in addition
to the implicit VFIO path?
> viommu->type = cmd->type;
> viommu->ictx = ucmd->ictx;
> viommu->hwpt = hwpt_paging;
> diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> index 0a0bb4abfbd2..3267717f676d 100644
> --- a/include/linux/iommufd.h
> +++ b/include/linux/iommufd.h
> @@ -103,6 +103,7 @@ struct iommufd_viommu {
> struct iommufd_ctx *ictx;
> struct iommu_device *iommu_dev;
> struct iommufd_hwpt_paging *hwpt;
> + struct file *kvm_file;
>
> const struct iommufd_viommu_ops *ops;
>
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v5 2/5] iommufd/device: Associate KVM file pointer with iommufd_device
From: Souradeep Chakrabarti @ 2026-07-13 13:14 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: linux-coco, iommu, linux-kernel, kvm, Alexey Kardashevskiy,
Bjorn Helgaas, Dan Williams, Jason Gunthorpe, Joerg Roedel,
Jonathan Cameron, Kevin Tian, Nicolin Chen, Samuel Ortiz,
Steven Price, Suzuki K Poulose, Will Deacon, Xu Yilun,
Shameer Kolothum, Paolo Bonzini, Tony Krowiak, Halil Pasic,
Jason Herne, Harald Freudenberger, Holger Dengler, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Alex Williamson, Matthew Rosato, Farhan Ali,
Eric Farman, linux-s390, Jason Gunthorpe
In-Reply-To: <20260525154816.1029642-3-aneesh.kumar@kernel.org>
On Mon, May 25, 2026 at 09:18:13PM +0530, Aneesh Kumar K.V (Arm) wrote:
> From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
>
> TSM vDevice support needs access to the KVM associated with a VFIO device
> after the device has been bound to iommufd.
>
> Extend iommufd_device_bind() to accept the device's KVM file and store it
> in the iommufd_device. The KVM file reference is owned by VFIO and is
> already held for the duration of the device open path.
>
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> [nicolinc: fix build error in iommufd_test_mock_domain()]
> Signed-off-by: Nicolin Chen <nicolinc@nvidia.com>
> [aneesh.kumar: Switch to use kvm_file]
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
> drivers/iommu/iommufd/device.c | 7 ++++++-
> drivers/iommu/iommufd/iommufd_private.h | 2 ++
> drivers/iommu/iommufd/selftest.c | 2 +-
> drivers/vfio/iommufd.c | 3 ++-
> include/linux/iommufd.h | 4 +++-
> 5 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
> index 170a7005f0bc..718abdc0e627 100644
> --- a/drivers/iommu/iommufd/device.c
> +++ b/drivers/iommu/iommufd/device.c
> @@ -203,6 +203,7 @@ void iommufd_device_destroy(struct iommufd_object *obj)
> * iommufd_device_bind - Bind a physical device to an iommu fd
> * @ictx: iommufd file descriptor
> * @dev: Pointer to a physical device struct
> + * @kvm_file: VM file if device belongs to a KVM VM
> * @id: Output ID number to return to userspace for this device
> *
> * A successful bind establishes an ownership over the device and returns
> @@ -216,7 +217,9 @@ void iommufd_device_destroy(struct iommufd_object *obj)
> * The caller must undo this with iommufd_device_unbind()
> */
> struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> - struct device *dev, u32 *id)
> + struct device *dev,
> + struct file *kvm_file,
> + u32 *id)
could it be a neutrally-named VM-file handle (e.g. vm_file)?
> {
> struct iommufd_device *idev;
> struct iommufd_group *igroup;
> @@ -266,6 +269,8 @@ struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> if (!iommufd_selftest_is_mock_dev(dev))
> iommufd_ctx_get(ictx);
> idev->dev = dev;
For a non-KVM VMM a registrable "which VM-file type" hook (a
file_is_mshv_partition() analogue) rather than an implicit "whatever
VFIO set" contract will be helpful, so the type can be validated where
it matters.
> + /* reference is already taken in vfio_df_ioctl_bind_iommufd() */
> + idev->kvm_file = kvm_file;
> idev->enforce_cache_coherency =
> device_iommu_capable(dev, IOMMU_CAP_ENFORCE_CACHE_COHERENCY);
> /* The calling driver is a user until iommufd_device_unbind() */
> diff --git a/drivers/iommu/iommufd/iommufd_private.h b/drivers/iommu/iommufd/iommufd_private.h
> index 6ac1965199e9..44eb026c206d 100644
> --- a/drivers/iommu/iommufd/iommufd_private.h
> +++ b/drivers/iommu/iommufd/iommufd_private.h
> @@ -488,6 +488,8 @@ struct iommufd_device {
> struct list_head group_item;
> /* always the physical device */
> struct device *dev;
> + /* ..and the VM file if available */
> + struct file *kvm_file;
> bool enforce_cache_coherency;
> struct iommufd_vdevice *vdev;
> bool destroying;
> diff --git a/drivers/iommu/iommufd/selftest.c b/drivers/iommu/iommufd/selftest.c
> index af07c642a526..a193390f9d07 100644
> --- a/drivers/iommu/iommufd/selftest.c
> +++ b/drivers/iommu/iommufd/selftest.c
> @@ -1069,7 +1069,7 @@ static int iommufd_test_mock_domain(struct iommufd_ucmd *ucmd,
> goto out_sobj;
> }
>
> - idev = iommufd_device_bind(ucmd->ictx, &sobj->idev.mock_dev->dev,
> + idev = iommufd_device_bind(ucmd->ictx, &sobj->idev.mock_dev->dev, NULL,
> &idev_id);
> if (IS_ERR(idev)) {
> rc = PTR_ERR(idev);
> diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> index a38d262c6028..d2d0bd9382a1 100644
> --- a/drivers/vfio/iommufd.c
> +++ b/drivers/vfio/iommufd.c
> @@ -119,7 +119,8 @@ int vfio_iommufd_physical_bind(struct vfio_device *vdev,
> {
> struct iommufd_device *idev;
>
> - idev = iommufd_device_bind(ictx, vdev->dev, out_device_id);
> + idev = iommufd_device_bind(ictx, vdev->dev, vdev->kvm_file,
> + out_device_id);
> if (IS_ERR(idev))
> return PTR_ERR(idev);
> vdev->iommufd_device = idev;
> diff --git a/include/linux/iommufd.h b/include/linux/iommufd.h
> index 6e7efe83bc5d..0a0bb4abfbd2 100644
> --- a/include/linux/iommufd.h
> +++ b/include/linux/iommufd.h
> @@ -59,7 +59,9 @@ struct iommufd_object {
> };
>
> struct iommufd_device *iommufd_device_bind(struct iommufd_ctx *ictx,
> - struct device *dev, u32 *id);
> + struct device *dev,
> + struct file *kvm_file,
> + u32 *id);
> void iommufd_device_unbind(struct iommufd_device *idev);
>
> int iommufd_device_attach(struct iommufd_device *idev, ioasid_t pasid,
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v5 1/5] vfio: cache KVM VM file references instead of raw struct kvm pointers
From: Souradeep Chakrabarti @ 2026-07-13 12:58 UTC (permalink / raw)
To: Aneesh Kumar K.V (Arm)
Cc: linux-coco, iommu, linux-kernel, kvm, Alexey Kardashevskiy,
Bjorn Helgaas, Dan Williams, Jason Gunthorpe, Joerg Roedel,
Jonathan Cameron, Kevin Tian, Nicolin Chen, Samuel Ortiz,
Steven Price, Suzuki K Poulose, Will Deacon, Xu Yilun,
Shameer Kolothum, Paolo Bonzini, Tony Krowiak, Halil Pasic,
Jason Herne, Harald Freudenberger, Holger Dengler, Heiko Carstens,
Vasily Gorbik, Alexander Gordeev, Christian Borntraeger,
Sven Schnelle, Alex Williamson, Matthew Rosato, Farhan Ali,
Eric Farman, linux-s390
In-Reply-To: <20260525154816.1029642-2-aneesh.kumar@kernel.org>
On Mon, May 25, 2026 at 09:18:12PM +0530, Aneesh Kumar K.V (Arm) wrote:
> VFIO currently records struct kvm pointers on vfio_group, vfio_device_file
> and the opened vfio_device. Switch VFIO to track the VM's struct file
> instead, so VFIO and iommufd can use normal file references for VM lifetime
> instead of depending on KVM's internal struct kvm refcounting.
>
> KVM_CREATE_DEVICE binds the KVM VM lifetime to the KVM device fd lifetime.
> For KVM_DEV_TYPE_VFIO, the KVM VFIO device fd also takes references to each
> VFIO file added through KVM_DEV_VFIO_FILE_ADD. The KVM VFIO device fd
> therefore owns both the internal KVM reference and the VFIO file references
> in kvf->file.
>
> KVM_DEV_VFIO_FILE_ADD further installs the VM file association into the
> VFIO file. VFIO converts the struct kvm pointer to a VM file reference with
> get_file_active(&kvm->_file), because the KVM device fd can keep struct kvm
> alive after the original VM fd is already in final release.
>
> The association intentionally pins the VM file until KVM_DEV_VFIO_FILE_DEL
> or until the KVM VFIO device fd is released. This gives VFIO/iommufd a
> stable VM file reference source without taking a dependency on KVM's struct
> kvm lifetime. The KVM VFIO device release path clears the VFIO-side
> association before dropping its VFIO file references.
>
> When a VFIO device is opened or bound, VFIO takes an additional reference
> from the associated VM file and stores it in vfio_device::kvm_file for
> driver and iommufd use. That open-time reference is released from
> vfio_device_put_kvm() when the VFIO device is closed or unbound.
>
can the "VM file" that VFIO caches be abstracted from "the KVM VM file"
to "a VM file the VMM registered"?
> This gives the ownership model:
>
> - KVM device fd pins struct kvm through kvm->users_count
> - KVM VFIO device fd pins VFIO files through kvf->file
> - VFIO group/device-file state pins the VM file while associated with KVM
> - vfio_device::kvm_file pins the VM file during active VFIO device use
>
> Signed-off-by: Aneesh Kumar K.V (Arm) <aneesh.kumar@kernel.org>
> ---
> drivers/s390/crypto/vfio_ap_ops.c | 5 +-
> drivers/vfio/device_cdev.c | 10 ++--
> drivers/vfio/group.c | 14 +++---
> drivers/vfio/pci/vfio_pci_zdev.c | 7 +--
> drivers/vfio/vfio.h | 16 ++++--
> drivers/vfio/vfio_main.c | 81 ++++++++++++++++---------------
> include/linux/kvm_host.h | 3 ++
> include/linux/vfio.h | 17 ++++++-
> virt/kvm/kvm_main.c | 2 +
> 9 files changed, 91 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/s390/crypto/vfio_ap_ops.c b/drivers/s390/crypto/vfio_ap_ops.c
> index 44b3a1dcc1b3..05996a8fd860 100644
> --- a/drivers/s390/crypto/vfio_ap_ops.c
> +++ b/drivers/s390/crypto/vfio_ap_ops.c
> @@ -2054,11 +2054,12 @@ static int vfio_ap_mdev_open_device(struct vfio_device *vdev)
> {
> struct ap_matrix_mdev *matrix_mdev =
> container_of(vdev, struct ap_matrix_mdev, vdev);
> + struct kvm *kvm = vfio_device_get_kvm(vdev);
>
> - if (!vdev->kvm)
> + if (!kvm)
> return -EINVAL;
>
> - return vfio_ap_mdev_set_kvm(matrix_mdev, vdev->kvm);
> + return vfio_ap_mdev_set_kvm(matrix_mdev, kvm);
> }
>
> static void vfio_ap_mdev_close_device(struct vfio_device *vdev)
> diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> index 54abf312cf04..ca75ab8eb7bd 100644
> --- a/drivers/vfio/device_cdev.c
> +++ b/drivers/vfio/device_cdev.c
> @@ -56,7 +56,7 @@ int vfio_device_fops_cdev_open(struct inode *inode, struct file *filep)
> static void vfio_df_get_kvm_safe(struct vfio_device_file *df)
> {
> spin_lock(&df->kvm_ref_lock);
> - vfio_device_get_kvm_safe(df->device, df->kvm);
> + vfio_device_get_kvm_safe(df->device, df->kvm_file);
> spin_unlock(&df->kvm_ref_lock);
> }
>
> @@ -133,10 +133,10 @@ long vfio_df_ioctl_bind_iommufd(struct vfio_device_file *df,
> }
>
> /*
> - * Before the device open, get the KVM pointer currently
> - * associated with the device file (if there is) and obtain
> - * a reference. This reference is held until device closed.
> - * Save the pointer in the device for use by drivers.
> + * Before the device open, get the VM struct file currently
> + * associated with the device file (if there is one) and obtain a
> + * reference. This reference is held until the device is closed.
> + * Save the file in the device for use by drivers.
> */
> vfio_df_get_kvm_safe(df);
>
> diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> index b2299e5bc6df..8950cfb9405d 100644
> --- a/drivers/vfio/group.c
> +++ b/drivers/vfio/group.c
> @@ -163,7 +163,7 @@ static int vfio_group_ioctl_set_container(struct vfio_group *group,
> static void vfio_device_group_get_kvm_safe(struct vfio_device *device)
> {
> spin_lock(&device->group->kvm_ref_lock);
> - vfio_device_get_kvm_safe(device, device->group->kvm);
> + vfio_device_get_kvm_safe(device, device->group->kvm_file);
> spin_unlock(&device->group->kvm_ref_lock);
> }
>
> @@ -181,10 +181,10 @@ static int vfio_df_group_open(struct vfio_device_file *df)
> mutex_lock(&device->dev_set->lock);
>
> /*
> - * Before the first device open, get the KVM pointer currently
> - * associated with the group (if there is one) and obtain a reference
> - * now that will be held until the open_count reaches 0 again. Save
> - * the pointer in the device for use by drivers.
> + * Before the first device open, get the VM struct file currently
> + * associated with the group (if there is one) and obtain a
> + * reference now that will be held until the open_count reaches 0
> + * again. Save the file in the device for use by drivers.
> */
> if (device->open_count == 0)
> vfio_device_group_get_kvm_safe(device);
> @@ -862,9 +862,7 @@ bool vfio_group_enforced_coherent(struct vfio_group *group)
>
> void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> {
> - spin_lock(&group->kvm_ref_lock);
> - group->kvm = kvm;
> - spin_unlock(&group->kvm_ref_lock);
> + vfio_kvm_file_replace(&group->kvm_file, &group->kvm_ref_lock, kvm);
> }
>
> /**
> diff --git a/drivers/vfio/pci/vfio_pci_zdev.c b/drivers/vfio/pci/vfio_pci_zdev.c
> index 0990fdb146b7..a9d8e6aa3839 100644
> --- a/drivers/vfio/pci/vfio_pci_zdev.c
> +++ b/drivers/vfio/pci/vfio_pci_zdev.c
> @@ -144,15 +144,16 @@ int vfio_pci_info_zdev_add_caps(struct vfio_pci_core_device *vdev,
> int vfio_pci_zdev_open_device(struct vfio_pci_core_device *vdev)
> {
> struct zpci_dev *zdev = to_zpci(vdev->pdev);
> + struct kvm *kvm = vfio_device_get_kvm(&vdev->vdev);
>
> if (!zdev)
> return -ENODEV;
>
> - if (!vdev->vdev.kvm)
> + if (!kvm)
> return 0;
>
> if (zpci_kvm_hook.kvm_register)
> - return zpci_kvm_hook.kvm_register(zdev, vdev->vdev.kvm);
> + return zpci_kvm_hook.kvm_register(zdev, kvm);
>
> return -ENOENT;
> }
> @@ -161,7 +162,7 @@ void vfio_pci_zdev_close_device(struct vfio_pci_core_device *vdev)
> {
> struct zpci_dev *zdev = to_zpci(vdev->pdev);
>
> - if (!zdev || !vdev->vdev.kvm)
> + if (!zdev || !vfio_device_get_kvm(&vdev->vdev))
> return;
>
> if (zpci_kvm_hook.kvm_unregister)
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index e4b72e79b7e3..41032104eb36 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -22,8 +22,8 @@ struct vfio_device_file {
>
> u8 access_granted;
> u32 devid; /* only valid when iommufd is valid */
> - spinlock_t kvm_ref_lock; /* protect kvm field */
> - struct kvm *kvm;
> + spinlock_t kvm_ref_lock; /* protect kvm_file */
> + struct file *kvm_file;
> struct iommufd_ctx *iommufd; /* protected by struct vfio_device_set::lock */
> };
>
> @@ -88,7 +88,7 @@ struct vfio_group {
> #endif
> enum vfio_group_type type;
> struct mutex group_lock;
> - struct kvm *kvm;
> + struct file *kvm_file;
> struct file *opened_file;
> struct iommufd_ctx *iommufd;
> spinlock_t kvm_ref_lock;
> @@ -434,11 +434,17 @@ static inline void vfio_virqfd_exit(void)
> #endif
>
> #if IS_ENABLED(CONFIG_KVM)
> -void vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm);
> +void vfio_kvm_file_replace(struct file **dst, spinlock_t *lock, struct kvm *kvm);
> +void vfio_device_get_kvm_safe(struct vfio_device *device, struct file *kvm_file);
> void vfio_device_put_kvm(struct vfio_device *device);
> #else
> +static inline void vfio_kvm_file_replace(struct file **dst,
> + spinlock_t *lock, struct kvm *kvm)
> +{
> +}
> +
> static inline void vfio_device_get_kvm_safe(struct vfio_device *device,
> - struct kvm *kvm)
> + struct file *kvm_file)
> {
> }
>
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index 6222376ab6ab..88c85a7b98c0 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -442,55 +442,61 @@ void vfio_unregister_group_dev(struct vfio_device *device)
> EXPORT_SYMBOL_GPL(vfio_unregister_group_dev);
>
> #if IS_ENABLED(CONFIG_KVM)
> -void vfio_device_get_kvm_safe(struct vfio_device *device, struct kvm *kvm)
> +void vfio_kvm_file_replace(struct file **dst, spinlock_t *lock, struct kvm *kvm)
> {
> - void (*pfn)(struct kvm *kvm);
> - bool (*fn)(struct kvm *kvm);
> - bool ret;
> + struct file *old_kvm_file, *new_kvm_file = NULL;
>
> - lockdep_assert_held(&device->dev_set->lock);
> + /*
> + * @kvm can outlive the VM fd and its final __fput(). Only take a
> + * new reference if the VM file is still active.
> + */
> + if (kvm)
> + new_kvm_file = get_file_active(&kvm->_file);
>
> - if (!kvm)
> - return;
> + spin_lock(lock);
> + old_kvm_file = *dst;
> + *dst = new_kvm_file;
> + spin_unlock(lock);
>
> - pfn = symbol_get(kvm_put_kvm);
> - if (WARN_ON(!pfn))
> - return;
> + if (old_kvm_file)
> + fput(old_kvm_file);
> +}
>
> - fn = symbol_get(kvm_get_kvm_safe);
> - if (WARN_ON(!fn)) {
> - symbol_put(kvm_put_kvm);
> - return;
> - }
> +void vfio_device_get_kvm_safe(struct vfio_device *device, struct file *kvm_file)
> +{
> + lockdep_assert_held(&device->dev_set->lock);
>
> - ret = fn(kvm);
> - symbol_put(kvm_get_kvm_safe);
> - if (!ret) {
> - symbol_put(kvm_put_kvm);
> - return;
> - }
> + /*
> + * Take a VM file reference if the KVM fd is still active.
> + */
> + if (kvm_file)
> + kvm_file = get_file(kvm_file);
>
> - device->put_kvm = pfn;
> - device->kvm = kvm;
> + device->kvm_file = kvm_file;
> }
>
> void vfio_device_put_kvm(struct vfio_device *device)
> {
> + struct file *kvm_file;
> +
> lockdep_assert_held(&device->dev_set->lock);
>
> - if (!device->kvm)
> + kvm_file = device->kvm_file;
> + if (!kvm_file)
> return;
>
> - if (WARN_ON(!device->put_kvm))
> - goto clear;
> + device->kvm_file = NULL;
> + fput(kvm_file);
> +}
>
> - device->put_kvm(device->kvm);
> - device->put_kvm = NULL;
> - symbol_put(kvm_put_kvm);
> +struct kvm *vfio_device_get_kvm(struct vfio_device *device)
> +{
> + if (!device->kvm_file)
> + return NULL;
>
> -clear:
> - device->kvm = NULL;
> + return device->kvm_file->private_data;
> }
> +EXPORT_SYMBOL_GPL(vfio_device_get_kvm);
> #endif
>
> /* true if the vfio_device has open_device() called but not close_device() */
> @@ -1518,13 +1524,10 @@ static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> struct vfio_device_file *df = file->private_data;
>
> /*
> - * The kvm is first recorded in the vfio_device_file, and will
> - * be propagated to vfio_device::kvm when the file is bound to
> - * iommufd successfully in the vfio device cdev path.
> + * Cache the VM file reference associated with this VFIO file so it
> + * can be pinned into vfio_device while the device is open.
> */
> - spin_lock(&df->kvm_ref_lock);
> - df->kvm = kvm;
> - spin_unlock(&df->kvm_ref_lock);
> + vfio_kvm_file_replace(&df->kvm_file, &df->kvm_ref_lock, kvm);
> }
>
> /**
> @@ -1532,8 +1535,8 @@ static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> * @file: VFIO group file or VFIO device file
> * @kvm: KVM to link
> *
> - * When a VFIO device is first opened the KVM will be available in
> - * device->kvm if one was associated with the file.
> + * When a VFIO device is first opened, VFIO caches a VM file reference if
> + * one was associated with the file.
> */
> void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> {
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 4c14aee1fb06..31afac5fb0ea 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -45,6 +45,8 @@
> #include <asm/kvm_host.h>
> #include <linux/kvm_dirty_ring.h>
>
> +struct file;
> +
> #ifndef KVM_MAX_VCPU_IDS
> #define KVM_MAX_VCPU_IDS KVM_MAX_VCPUS
> #endif
> @@ -861,6 +863,7 @@ struct kvm {
> struct srcu_struct srcu;
> struct srcu_struct irq_srcu;
> pid_t userspace_pid;
> + struct file __rcu *_file;
> bool override_halt_poll_ns;
> unsigned int max_halt_poll_ns;
> u32 dirty_ring_size;
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index 31b826efba00..bca1d00f7845 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -22,8 +22,22 @@ struct kvm;
> struct iommufd_ctx;
> struct iommufd_device;
> struct iommufd_access;
> +struct vfio_device;
> struct vfio_info_cap;
>
> +#if IS_ENABLED(CONFIG_KVM)
> +/*
> + * Return the KVM associated with @vdev's kvm_file. The returned pointer
> + * is valid only while VFIO device open holds the kvm_file reference.
> + */
> +struct kvm *vfio_device_get_kvm(struct vfio_device *vdev);
> +#else
> +static inline struct kvm *vfio_device_get_kvm(struct vfio_device *vdev)
> +{
> + return NULL;
> +}
> +#endif
> +
> /*
> * VFIO devices can be placed in a set, this allows all devices to share this
> * structure and the VFIO core will provide a lock that is held around
> @@ -54,7 +68,7 @@ struct vfio_device {
> struct list_head dev_set_list;
> unsigned int migration_flags;
> u8 precopy_info_v2;
> - struct kvm *kvm;
> + struct file *kvm_file;
>
> /* Members below here are private, not for driver use */
> unsigned int index;
> @@ -66,7 +80,6 @@ struct vfio_device {
> unsigned int open_count;
> struct completion comp;
> struct iommufd_access *iommufd_access;
> - void (*put_kvm)(struct kvm *kvm);
> struct inode *inode;
> #if IS_ENABLED(CONFIG_IOMMUFD)
> struct iommufd_device *iommufd_device;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 89489996fbc1..011819c5c47c 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1351,6 +1351,7 @@ static int kvm_vm_release(struct inode *inode, struct file *filp)
>
> kvm_irqfd_release(kvm);
>
> + RCU_INIT_POINTER(kvm->_file, NULL);
> kvm_put_kvm(kvm);
> return 0;
> }
> @@ -5500,6 +5501,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type)
> r = PTR_ERR(file);
> goto put_kvm;
> }
> + rcu_assign_pointer(kvm->_file, file);
>
> /*
> * Don't call kvm_put_kvm anymore at this point; file->f_op is
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support
From: Kiryl Shutsemau @ 2026-07-13 11:38 UTC (permalink / raw)
To: Xu Yilun
Cc: x86, linux-kernel, rick.p.edgecombe, dave.hansen, dave.hansen,
yilun.xu, chao.gao, djbw, linux-coco, peter.fang, xiaoyao.li
In-Reply-To: <alSt/TgbDQgNveoS@yilunxu-OptiPlex-7050>
On Mon, Jul 13, 2026 at 05:21:01PM +0800, Xu Yilun wrote:
> > The commit message covers the mechanics, but is silent on compatibility
> > with older TDX modules. That context matters for the design choice, so
> > regardless of where the implementation discussion lands, I think it
> > needs to be spelled out here.
> >
> > Who is responsible for picking a version the module supports?
> >
> > TDH.VP.INIT can hardcode version 1 only because KVM already refuses to
> > enable TDX on modules without TOPOLOGY_ENUM.
> >
> > The TDH.SYS.UPDATE user from [1] is the opposite case: it has to pick
> > version 0 or 1 at runtime depending on whether add-on features are
> > configured, to keep working on modules that don't support them.
> >
> > The second point is the actual argument for a version field in struct
> > tdx_module_args rather than encoding the version in the leaf defines:
> > the version is not always a compile-time property of the call site.
>
> That's good point. I'll add the TDX module compatibility argument in
> changelog:
>
> One concern is the compatibility with older TDX modules which don't
> recognize the new SEAMCALLs. The kernel should decide which SEAMCALL
> version to use at runtime, selecting the minimum version number for the
> required functionality. It can't overwrite the function number with a
> new value at compile time.
There are two policies, not one: make the new version a hard
requirement at init time as we did with TDH.VP.INIT, or fall back to
version 0 at runtime like your TDH.SYS.UPDATE user does. Both are valid,
and the changelog should not claim runtime selection is the rule.
BTW, we might eventually switch TDH.SYS.UPDATE to hardcoded v1 once we
stop caring about older modules.
> > Without TDH.SYS.UPDATE context, the patch seems pointless.
>
> mm.. It's true one target is to address the compatibility with old
> module. But is the other concern valid?
>
> Another concern is the obscure usage of the 'fn' parameter for seamcall
> wrappers. An existing caller for TDH.VP.INIT packs the version into the
> 'fn' to match the low-level TDX ABI RAX layout. The RAX layout
> interprets some bits differently, such as INTERRUPT_MODE, SEAMLDR flag,
> which are not a good fit for the function number definition.
>
> Is this a blocker for you as an independent cleanup patch?
Not a blocker. But respinning now seems premature: the shape of v3
depends entirely on where the RAX composition discussion lands --
args.version with FIELD_MODIFY(), a version macro in 'fn', or Dave's
rax-in-the-struct. I'd wait for that to settle first.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v2 08/17] x86/virt/tdx: Prepare Quote buffer during extension bringup
From: Peter Fang @ 2026-07-13 10:19 UTC (permalink / raw)
To: Nikolay Borisov
Cc: Xu Yilun, x86, kvm, linux-coco, linux-kernel, djbw, kas,
rick.p.edgecombe, yilun.xu, xiaoyao.li, sohil.mehta,
adrian.hunter, kishen.maloor, tony.lindgren, baolu.lu,
zhenzhong.duan, dave.hansen, dave.hansen, seanjc
In-Reply-To: <d13c4073-b248-40ac-aa28-1c0dfc6b9043@suse.com>
On Wed, Jul 08, 2026 at 10:52:07AM +0300, Nikolay Borisov wrote:
>
>
> On 6/18/26 11:13, Xu Yilun wrote:
> > From: Peter Fang <peter.fang@intel.com>
> >
> > During TDX attestation, the TDX guest asks the host to generate a
> > signed, verifiable structure (a "Quote"). With the Quoting extension,
> > the TDX module returns the Quote in pages that the host shares via an
> > Extension-SEAMCALL.
>
> nit: Just say the host provides the pages to the TDX module, no need to
> specify if it's via an ext seamcall or some otherway. One can infer this
> from the code itself.
Good point. Thanks for the simplification.
>
> >
> > The SEAMCALL accepts the host buffer pages as a linked list of 4KB
> > "HPA_LINKED_LIST" nodes. Each entry holds the physical address of a 4KB
> > data page, except for the last entry, which points to the next node. The
> > TDX module reports the required Quote buffer size through a global
> > metadata field. See [1] for details.
>
> That HPA_LINKED_LIST is basically a linked list of arrays, each node into
> the linked list being the array, and each entry of that array being the HPA
> witht he last being a pointer to the next HPA_LINKED_LIST, so in a way a 2
> level data structure. Can you be more explicit and just say something along
> the lines of :
>
> "SEAMCALL accepts host buffer pages arranged in a custom data structure,
> consisting of nodes containing HPAs, arranged linearly, and each node is
> linked to the next one via the last entry"
Oh this is much easier to read. Thanks for the suggestion! I can
incorporate this.
>
> > +
> > +#define HPAS_PER_NODE (PAGE_SIZE / sizeof(u64))
> > +
> > +/*
> > + * Pass the quote buffer to the TDX module as an HPA linked list, where each
> > + * node holds 4KB page HPAs and the last entry points to the next node.
> > + */
> > +static __init int tdx_quote_create_buf(unsigned int npages,
> > + struct tdx_quote_data *qdata)
> > +{
> > + unsigned int nnodes;
> > + u64 *hpas;
> > + void *qbuf;
> > + int i, j;
> > +
> > + if (!npages)
> > + return -EINVAL;
> > +
> > + /*
> > + * Each node holds up to (HPAS_PER_NODE - 1) 4KB page HPAs.
> > + * The last entry of the node points to the next node.
> > + */
> > + nnodes = DIV_ROUND_UP(npages, HPAS_PER_NODE - 1);
> nit: It's somewhat arbitrary but num_nodes seems more explicit, saving 3
> chars is not worth it.
Sure, I can make this change.
> > +
> > + hpas = vmalloc_array(nnodes, PAGE_SIZE);
>
> hpas is basically the starting node, so name it "nodes". Also I'm slightly
> confused why you use vmalloc to allocate a contiguous address space when the
> nodes are linked in a list? I.e you keep referring to list in the changelog
> and the code seems to be working in an array chunks?
Hmm. I used the vmalloc family because I wanted to keep using hpas[i]
without switching to the next node's va. It felt a bit simpler to just
keep doing "i++". Maybe that actually hurts readability? I can replace
that part of the for loop with something like:
if (i == HPAS_PER_NODE - 1) {
next_node = kmalloc(PAGE_SIZE, GFP_KERNEL);
hpas[i] = virt_to_phys(next_node);
hpas = next_node;
i = 0;
}
And "hpas[i]" feels a bit closer to what the code does than "nodes[i]"?
It's filling this page with a bunch of HPAs.
>
> > + if (!hpas)
> > + return -ENOMEM;
> > +
> > + /*
> > + * ~0ULL is the list terminator for HPA_LINKED_LIST.
> > + *
> > + * Pre-fill the last node with 0xff bytes so that unused entries are
> > + * terminators. Overwrite populated entries later.
> > + */
> > + memset((u8 *)hpas + (nnodes - 1) * PAGE_SIZE, 0xff, PAGE_SIZE);
> > +
> > + qbuf = vcalloc(npages, PAGE_SIZE);
> > + if (!qbuf)
> > + goto out_nomem;
>
> 'qbuf' is rather arbitrary here. You simply pre-allocate a bunch of pages
> which you then initialize into the nodes. Can't this allocation be moved
> inside the loop itself, of course it will increase the number of allocs
> happening but this is during initialization so it's not performance
> critical, yes?
I see your point. It’s a bit subtle here. In patch 11, there is a
kvmemdup() that depends on this qbuf being virtually contiguous. Based
on feedback from other folks, I think I might change this and use
runtime quote buffers allocated with kmalloc(). That would also avoid
the TLB flushing penalty of vfree(). The free path would be a bit more
complicated, but that should be okay.
>
> > +
> > + /* Populate the linked list */
> > + for (i = 0, j = 0; j < npages; i++) {
>
> nit: This feedback was given before, but both variables can be defined in
> this loop.
Great suggestion. I can do that. Thanks.
>
>
^ permalink raw reply
* Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support
From: Kiryl Shutsemau @ 2026-07-13 9:40 UTC (permalink / raw)
To: Dave Hansen
Cc: Xu Yilun, x86, linux-kernel, rick.p.edgecombe, dave.hansen,
yilun.xu, chao.gao, djbw, linux-coco, peter.fang, xiaoyao.li
In-Reply-To: <f8ef7a94-7824-4d1f-899a-60a0fbd2e2f7@intel.com>
On Fri, Jul 10, 2026 at 09:48:22AM -0700, Dave Hansen wrote:
> On 7/10/26 09:21, Kiryl Shutsemau wrote:
> > The version is not a register operand, it is part of the function
> > number.
>
> I'm not sure why that's important.
>
> Sure, the arg structure is all full registers now. But it's not even all
> the registers because RAX isn't there. So let's say we made the data
> structure smarter:
>
> struct tdx_module_args {
> union {
> u64 rax;
> struct {
> u16 function_nr;
> u8 version
> u8 padding0;
> u32 flags;
> };
> };
> u64 rcx;
> u64 rdx;
> ...
> }
This union doesn't match the ABI it is supposed to represent.
RAX is:
15:0 Leaf number
23:16 Version number
24 INTERRUPT_MODE
62:25 Reserved
63 P-SEAMLDR select
In your layout INTERRUPT_MODE ends up somewhere in padding0 and the
P-SEAMLDR bit is BIT(31) of 'flags'. Nothing lines up with the spec.
To describe the format properly the struct would need bitfields, and we
generally discourage bitfields for ABI-defined layouts. Without
bitfields it degrades into masks and shifts on a u64 -- which is
exactly what composing 'fn' with a macro is. The union doesn't add
anything on top of that, it only hides where the bits are.
>
> All of the:
>
> struct tdx_module_args args = {};
>
> instances would default to version=0. It would take a one-liner to get
> bit 63 set:
>
> static int seamldr_call(u64 fn, struct tdx_module_args *args)
> {
> ...
> + args->flags |= SEAMLDR_BIT;
> guard(raw_spinlock)(&seamldr_lock);
> return seamcall_prerr(fn, args);
> }
>
> The truth of the matter is that 'fn' *IS* the RAX from the TDX ABI.
> We're carrying it around the kernel in that format, and it just doesn't
> work very well.
>
> The alternative is to carry the logical pieces of RAX around the kernel
> and them assemble RAX out of them in one (or very few) places. *Not* to
> build RAX in the TDX module ABI early far from the TDX module ABI layer
> itself.
I don't see what delaying the assembly buys us.
The SEAMCALL helper is where we decide what we want from the call: the
leaf, the version, the operands. Nothing below it adds information --
sc_retry() and __seamcall_dirty_cache() only retry on entropy failure
and track cache state. There is no layer further down that is in a
better position to compose RAX than the helper itself.
And it is not "far from the TDX module ABI layer". The tdh_*() helpers
are the C representation of the ABI functions. They are the ABI layer.
We also don't carry the pieces "around the kernel" today. The
composition already happens in exactly one place -- a macro next to the
leaf defines. Call sites don't open-code shifts.
That said, I'm okay with RAX living in the struct as a plain u64
instead of a separate 'fn' argument, if you prefer the structure to
carry all the registers. But its composition should stay early, in the
helper, where the decisions about the call are made:
args.rax = TDH_VP_INIT | SEAMCALL_LEAF_VER(1);
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support
From: Xu Yilun @ 2026-07-13 9:21 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: x86, linux-kernel, rick.p.edgecombe, dave.hansen, dave.hansen,
yilun.xu, chao.gao, djbw, linux-coco, peter.fang, xiaoyao.li
In-Reply-To: <alEUAMgx7HMegOmX@thinkstation>
> The commit message covers the mechanics, but is silent on compatibility
> with older TDX modules. That context matters for the design choice, so
> regardless of where the implementation discussion lands, I think it
> needs to be spelled out here.
>
> Who is responsible for picking a version the module supports?
>
> TDH.VP.INIT can hardcode version 1 only because KVM already refuses to
> enable TDX on modules without TOPOLOGY_ENUM.
>
> The TDH.SYS.UPDATE user from [1] is the opposite case: it has to pick
> version 0 or 1 at runtime depending on whether add-on features are
> configured, to keep working on modules that don't support them.
>
> The second point is the actual argument for a version field in struct
> tdx_module_args rather than encoding the version in the leaf defines:
> the version is not always a compile-time property of the call site.
That's good point. I'll add the TDX module compatibility argument in
changelog:
One concern is the compatibility with older TDX modules which don't
recognize the new SEAMCALLs. The kernel should decide which SEAMCALL
version to use at runtime, selecting the minimum version number for the
required functionality. It can't overwrite the function number with a
new value at compile time.
>
> Without TDH.SYS.UPDATE context, the patch seems pointless.
mm.. It's true one target is to address the compatibility with old
module. But is the other concern valid?
Another concern is the obscure usage of the 'fn' parameter for seamcall
wrappers. An existing caller for TDH.VP.INIT packs the version into the
'fn' to match the low-level TDX ABI RAX layout. The RAX layout
interprets some bits differently, such as INTERRUPT_MODE, SEAMLDR flag,
which are not a good fit for the function number definition.
Is this a blocker for you as an independent cleanup patch?
Thanks,
Yilun
>
> > Link: https://lore.kernel.org/kvm/4f4b0f29-424b-45ed-8cfd-c77da2ea390f@intel.com/ # [1]
^ permalink raw reply
* Re: [RFC PATCH 10/30] vfio/pci: Export vfio dma-buf specific info for importers
From: Jason Gunthorpe @ 2026-07-12 22:19 UTC (permalink / raw)
To: Ackerley Tng
Cc: Xu Yilun, kvm, sumit.semwal, christian.koenig, pbonzini, seanjc,
alex.williamson, dan.j.williams, aik, linux-coco, dri-devel,
linux-media, linaro-mm-sig, vivek.kasireddy, yilun.xu,
linux-kernel, lukas, yan.y.zhao, daniel.vetter, leon, baolu.lu,
zhenzhong.duan, tao1.su, linux-pci, zhiw, simona.vetter,
shameerali.kolothum.thodi, aneesh.kumar, iommu, kevin.tian
In-Reply-To: <CAEvNRgFpJWQ5M5sQhGpQUV3GbBq9N+MQhhaxdxa=D8ky94SCsw@mail.gmail.com>
On Sat, Jul 11, 2026 at 06:01:31PM -0700, Ackerley Tng wrote:
> In the course of a CoCo guest's operation, will the guest need to
> convert between private/shared MMIO? Will the guest need some pages
> shared and others private? If these are required operations, guest_memfd
> already provides the tracking and is going to have a conversion ioctl
> very soon. Instead of further extending dmabuf to track more things, how
> about letting guest_memfd track it?
Use another FD type was sort of my fallback if we couldn't get DMABUF
into something workable. I'm kind of surprised to see guestmemfd
proposed as the other FD, but I don't know much about its insides.
If VFIO can create one and fill it with MMIO physical addresses then
maybe it is OK?
Jason
^ permalink raw reply
* Re: [RFC PATCH 10/30] vfio/pci: Export vfio dma-buf specific info for importers
From: Ackerley Tng @ 2026-07-12 1:01 UTC (permalink / raw)
To: Jason Gunthorpe, Xu Yilun
Cc: kvm, sumit.semwal, christian.koenig, pbonzini, seanjc,
alex.williamson, dan.j.williams, aik, linux-coco, dri-devel,
linux-media, linaro-mm-sig, vivek.kasireddy, yilun.xu,
linux-kernel, lukas, yan.y.zhao, daniel.vetter, leon, baolu.lu,
zhenzhong.duan, tao1.su, linux-pci, zhiw, simona.vetter,
shameerali.kolothum.thodi, aneesh.kumar, iommu, kevin.tian
In-Reply-To: <20250602133009.GC233377@nvidia.com>
Jason Gunthorpe <jgg@nvidia.com> writes:
> On Thu, May 29, 2025 at 01:34:53PM +0800, Xu Yilun wrote:
>> Export vfio dma-buf specific info by attaching vfio_dma_buf_data in
>> struct dma_buf::priv. Provide a helper vfio_dma_buf_get_data() for
>> importers to fetch these data. Exporters identify VFIO dma-buf by
>> successfully getting these data.
>>
>> VFIO dma-buf supports disabling host access to these exported MMIO
>> regions when the device is converted to private. Exporters like KVM
>> need to identify this type of dma-buf to decide if it is good to use.
>> KVM only allows host unaccessible MMIO regions been mapped in private
>> roots.
>>
>> Export struct kvm * handler attached to the vfio device. This
>> allows KVM to do another sanity check. MMIO should only be assigned to
>> a CoCo VM if its owner device is already assigned to the same VM.
>
> This doesn't seem right, it should be encapsulated into the standard
> DMABUF API in some way.
>
I'd like to propose an alternative. I've been working on guest_memfd and
new to the world of IO, please help me along! :)
It seems like using dmabufs are used a little awkwardly here. IIUC
dmabufs were originally meant to expose memory of one device to another
device, mostly meant to share memory. Dmabufs do expose MMIO too, for
device to device communications. Without virtualization, userspace MMIO
would be done by mmap()-ing a VFIO fd and having the userspace program
write to the userspace addresses.
Before CoCo, device passthrough (MMIO) is mostly handled by mmap()-ing a
VFIO fd and setting up the userspace address in a KVM memslot for the
guest.
With CoCo, is the problem we're solving that we want KVM to know what
pfns to set up in stage 2 page tables, but not via userspace addresses?
guest_memfd already does that for regular host memory, tracks the
private/shared-ness of the memory, tracks which struct kvm the memory
belongs to.
guest_memfd functions as KVM's bridge to host memory. KVM already can
ask guest_memfd for the pfn to map into stage 2 page tables, and already
asks guest_memfd for the shared/private state of the memory. guest_memfd
already also blocks the host from faulting guest private memory
(mmap()-ing is always allowed).
Instead of using dmabuf as the intermediary between the MMIO PFNs and
KVM, why not use guest_memfd?
What if we make guest_memfd accept a VFIO fd, or a dmabuf fd?
guest_memfd can then take the mmap() calls from userspace and .fault()
from mm, and then forward them to VFIO or dmabuf. This way, VFIO/dmabuf
can stick to their original functions, and the changes to VFIO/dmabuf
would probably revolve around disabling access.
Disabling access would probably involve some of these:
+ When guest_memfd receives the fd, it could return error for existing
mappings, or perhaps it could just force-unmap.
+ 1 extra flag or field to indicate that guest_memfd is controlling this
file, so that if userspace tries to take some actions with the
original VFIO or dmabuf fd, the request should be blocked.
+ Perhaps just close the original fd, like dup2(oldfd, newfd) closes
newfd?
I'm about to restart work on guest_memfd HugeTLB and I'm thinking about
a similar approach for guest_memfd HugeTLB, where perhaps the interface
could be that userspace will give guest_memfd a HugeTLB fd at creation
time, and then the original HugeTLB fd would be rendered unusable in the
same way as above, perhaps like with the S_IMMUTABLE inode flag, but
also blocking reads, and not userspace-modifiable.
In the course of a CoCo guest's operation, will the guest need to
convert between private/shared MMIO? Will the guest need some pages
shared and others private? If these are required operations, guest_memfd
already provides the tracking and is going to have a conversion ioctl
very soon. Instead of further extending dmabuf to track more things, how
about letting guest_memfd track it?
^ permalink raw reply
* Re: [PATCH 01/15] netlink: specs: Introduce multi-message blobs for SPDM
From: Dan Williams (nvidia) @ 2026-07-11 1:43 UTC (permalink / raw)
To: Donald Hunter, Dan Williams
Cc: linux-coco, linux-pci, driver-core, ankita, Alistair Francis,
Lukas Wunner, Jakub Kicinski
In-Reply-To: <m2y0flbuoa.fsf@gmail.com>
Donald Hunter wrote:
> Dan Williams <djbw@kernel.org> writes:
>
> > The SPDM, Security Protocol and Data Model, underpins PCI device security
> > and other use cases. It defines objects that allow for verification of
> > device identity and configuration. These objects can be large in size 16MB.
> > Netlink is otherwise suitable to define the operations, with optional
> > parameters, and notifications for working with these objects. For example,
> > operations like "regenerate evidence with nonce", "mark evidence
> > validated", and "broadcast evidence / security state change events".
> >
> > A netlink 'blob' is introduced as a way to teach YNL that one instance of a
> > attribute may span multiple messages. It enables netlink to convey all the
> > data needed for verification and manipulation of SPDM transported evidence.
> >
> > The schema change to allows YNL to infer that an attribute may span
>
> change to allows -> change allows
ack.
> > multiple messages and interrogate its length to preallocate an
> > appropriately sized receive buffer.
> >
> > The design direction to extend the netlink schema for a "multi-message
> > object receive" case was the result of this discussion [1].
> >
> > Cc: Alistair Francis <alistair.francis@wdc.com>
> > Cc: Lukas Wunner <lukas@wunner.de>
> > Cc: Jakub Kicinski <kuba@kernel.org>
> > Cc: Donald Hunter <donald.hunter@gmail.com>
> > Link: http://lore.kernel.org/20260318170014.6650d2bf@kernel.org [1]
> > Signed-off-by: Dan Williams <djbw@kernel.org>
> > ---
> > Documentation/netlink/genetlink-legacy.yaml | 6 ++++++
> > Documentation/netlink/genetlink.yaml | 7 +++++++
> > Documentation/netlink/netlink-raw.yaml | 7 +++++++
>
> I suggest you drop the changes to genetlink-legacy and netlink-raw
> because I don't think we want to support the blob functionality for
> legacy families.
Done.
> Can you also update Documentation/userspace-api/netlink/specs.rst to
> describe the multi-attr / bloblen API behaviour.
Added:
@@ -239,6 +239,16 @@ Boolean property signifying that the attribute may be present multiple times.
Allowing an attribute to repeat is the recommended way of implementing arrays
(no extra nesting).
+bloblen (multi-message binary blob)
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+String property specified on a ``type: binary`` ``multi-attr: true``
+attribute to indicate that this single byte array may span multiple
+messages. The string names an attribute that indicates the final size of
+the blob when all messages are received. This differs from other ``type:
+binary`` ``multi-attr: true`` instances without bloblen where each
+message is an attribute boundary.
+
byte-order
~~~~~~~~~~
^ permalink raw reply
* Re: [PATCH v2 16/17] KVM: TDX: Add in-kernel Quote generation
From: Peter Fang @ 2026-07-10 22:59 UTC (permalink / raw)
To: Sean Christopherson
Cc: Xu Yilun, x86, kvm, linux-coco, linux-kernel, djbw, kas,
rick.p.edgecombe, yilun.xu, xiaoyao.li, sohil.mehta,
adrian.hunter, kishen.maloor, tony.lindgren, baolu.lu,
zhenzhong.duan, dave.hansen, dave.hansen
In-Reply-To: <alEGIrWhQvzf2wAV@google.com>
On Fri, Jul 10, 2026 at 07:48:02AM -0700, Sean Christopherson wrote:
> On Fri, Jul 10, 2026, Peter Fang wrote:
> > On Wed, Jul 08, 2026 at 02:37:55PM -0700, Sean Christopherson wrote:
> > > On Thu, Jun 18, 2026, Xu Yilun wrote:
> > > 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?
> >
> > Yeah this needs explaining:
> >
> > The guest-observable difference between the two paths is that it sees
> > quotes in different formats (the TDX module uses newer formats of
> > course). And an old guest must be able to continue using its
> > TDVMCALL<GetQuote> interface for quotes. The guest sends the quote to an
> > external verifier to authenticate itself. The verifier (part of the
> > cloud infrastructure) must know how to parse this format. So in other
> > words, it's the verifier that must maintain backward compatibility if it
> > wants to continue accepting old quotes. And it needs to be updated to
> > support newer formats as well.
>
> Uh, no, that's not how KVM's guest/host ABI works. Updating the host kernel and
> breaking existing guest is not acceptable.
Yeah I agree there's ABI sensitivity here. I can provide a bit more
context here:
- When you get a quote you don't check it yourself. You pass it to a
verifier service which does the check. The verifier checks if the
platform is genuine. It has platform specific knowledge for each new
platform. So with a new platform these services need to be updated to
know about the new platform's quote, or attestation will fail for
these guests.
- There may be platforms that support TDX but have SGX off for various
reasons. Meaning SGX based attestation won't work and they will only
support this new type of quote.
- SGX based attestation is very complicated for users. It will likely be
deprecated long term. This new type of quote simplifies a lot of
steps.
A risk could be that someone makes a quote verifying service that
supports a new DICE capable platform halfway (SGX only), then the user
does a kernel upgrade and sees their verifications start failing. But
these halfway services would probably come to life after the kernel
support is upstream since DICE capable platforms are not available yet.
So not sure if that's a major concern for regression.
I should call out this is not a zero-risk ABI change and the risks
should have been enumerated. The approach this revision takes is to have
less uABI on the assumption that it will not impact any real users. So
we need to consider whether the simplicity is worth it vs going to
userspace and back. Is it worth considering to have less uABI? Or would
you rather punt to userspace? BTW, the quote SEAMCALL does it for a
specifc TD. So putting this new ABI in the TDX host service would
require adding KVM's TD life cycle knowledge. So the "and back" part
probably would fit best as a new KVM TDX ioctl.
>
> > > > +{
> > > > + 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?
> >
> > I struggled a bit with this actually... I can't just grab
> > kvm->slots_lock here, and dropping kvm->srcu seems bad because the quote
> > will be written to guest memory later. I settled on the idea that the
> > guest should make sure these pages are converted to shared first. If it
> > tries to convert them back to private after this check, before this
> > TDVMCALL returns, then it's kind of a self-inflicted race and I don't
> > see KVM doing any self damage in this case. Not sure if this makes sense
> > to you? Or maybe I should just drop this check and go straight to
> > kvm_vcpu_read_guest()?
>
> Yes, drop it and rely on uaccess to do the right thing.
Got it, thanks Sean.
^ permalink raw reply
* Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED
From: Aneesh Kumar K.V @ 2026-07-10 19:09 UTC (permalink / raw)
To: Jason Gunthorpe
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, Alexey Kardashevskiy, 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, Jiri Pirko,
Michael Kelley
In-Reply-To: <20260710161945.GO118978@ziepe.ca>
Jason Gunthorpe <jgg@ziepe.ca> writes:
> On Fri, Jul 10, 2026 at 10:52:49AM +0530, Aneesh Kumar K.V wrote:
>> >> > /*
>> >> > * For host memory encryption and device requiring unencrypted DMA,
>> >> > * MMIO memory is treated as shared by default.
>> >> > */
>> >> > if (attrs & DMA_ATTR_MMIO) {
>> >> > if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) || force_dma_unencrypted(dev))
>> >> > attrs |= DMA_ATTR_CC_SHARED;
>> >> > }
>> >>
>> >> Yes, I think it does the trick, preserves the current semantics for AMD.
>> >> I guess you could use a single 'if' for all checks (up to you).
>> >
>> > Please don't change it, MMIO P2P is broken on CC systems today and it
>> > should stay broken. Passing DMA_ATTR_MMIO with DMA_ATTR_CC_SHARED is
>> > an error that we need to correct in the drivers not make work in the
>> > core code.
>> >
>>
>> But the above changes are intended to handle HOST_MEM_ENCRYPT. In v7, we
>> had the following diff:
>
> To follow how the rest of the decrypted/encrypted stuff works the MMIO
> has to be flaged with CC_SHARED for HOST_MEM_ENCRYPT too, just like
> the PTEs.
>
>> @@ -88,37 +88,40 @@ static inline dma_addr_t dma_direct_map_phys(struct device *dev,
>> {
>> dma_addr_t dma_addr;
>>
>> + /*
>> + * For a device requiring unencrypted DMA, MMIO memory is treated
>> + * as shared by default.
>> + */
>> + if (force_dma_unencrypted(dev) && (attrs & DMA_ATTR_MMIO))
>> + attrs |= DMA_ATTR_CC_SHARED;
>
> force_dma_unencrypted() says nothing about the properties of the
> address passed in, this was nonsense :\
>
>> As we discussed [1], that can come in a later patch. In the meantime, adding
>> the HOST_MEM_ENCRYPT check preserves the previous behavior for SME.
>
> It never worked. When we added ATTR_MMIO it started to have a chance
> to work but prior to that it was always broken anyhow. I don't see
> there is much merit in preserving the narrow window when we
> inadvertantly had a half working ATTR_MMIO.
>
> But if you really want to it should be
> cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) *only* and get rid of the wrong
> force_dma_unencrypted().
>
Ok, I will update the changes to so we preserve the previous behaviour
for SME?
if (attrs & DMA_ATTR_MMIO) {
/*
* For host memory encryption treat MMIO memory as shared
*/
if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT))
attrs |= DMA_ATTR_CC_SHARED;
}
Do let me know if you feel strongly that the above change should not be
included and that the SME P2P case should instead be handled in a future
patch.
>
> But IMHO, I'd rather this series treat ATTR_MMIO as private MMIO and
> ATTR_MMIO|CC_SHARED as shared MMIO and that's the right and correct
> thing for the DMA API.
>
If you think the rest of the series is ready for upstream, could you
please ack it so it can be picked up for the next merge window? I'll
repost v8, rebased on top of the pKVM topic branch.
-aneesh
^ permalink raw reply
* Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support
From: Dave Hansen @ 2026-07-10 16:48 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: Xu Yilun, x86, linux-kernel, rick.p.edgecombe, dave.hansen,
yilun.xu, chao.gao, djbw, linux-coco, peter.fang, xiaoyao.li
In-Reply-To: <alEXflESyo6ndDTY@thinkstation>
On 7/10/26 09:21, Kiryl Shutsemau wrote:
> The version is not a register operand, it is part of the function
> number.
I'm not sure why that's important.
Sure, the arg structure is all full registers now. But it's not even all
the registers because RAX isn't there. So let's say we made the data
structure smarter:
struct tdx_module_args {
union {
u64 rax;
struct {
u16 function_nr;
u8 version
u8 padding0;
u32 flags;
};
};
u64 rcx;
u64 rdx;
...
}
All of the:
struct tdx_module_args args = {};
instances would default to version=0. It would take a one-liner to get
bit 63 set:
static int seamldr_call(u64 fn, struct tdx_module_args *args)
{
...
+ args->flags |= SEAMLDR_BIT;
guard(raw_spinlock)(&seamldr_lock);
return seamcall_prerr(fn, args);
}
The truth of the matter is that 'fn' *IS* the RAX from the TDX ABI.
We're carrying it around the kernel in that format, and it just doesn't
work very well.
The alternative is to carry the logical pieces of RAX around the kernel
and them assemble RAX out of them in one (or very few) places. *Not* to
build RAX in the TDX module ABI early far from the TDX module ABI layer
itself.
^ permalink raw reply
* Re: [PATCH v2] x86/virt/tdx: Formalize SEAMCALL version encoding support
From: Kiryl Shutsemau @ 2026-07-10 16:21 UTC (permalink / raw)
To: Dave Hansen
Cc: Xu Yilun, x86, linux-kernel, rick.p.edgecombe, dave.hansen,
yilun.xu, chao.gao, djbw, linux-coco, peter.fang, xiaoyao.li
In-Reply-To: <e8aaca8c-6d88-490b-a3ae-c63a9e9a8b90@intel.com>
On Wed, Jul 08, 2026 at 11:03:14AM -0700, Dave Hansen wrote:
> On 7/8/26 10:03, Xu Yilun wrote:
> > +/*
> > + * SEAMCALL leaf:
> > + *
> > + * Bit 15:0 Leaf number
> > + * Bit 23:16 Version number
> > + */
> > +#define SEAMCALL_VERSION_MASK GENMASK_U64(23, 16)
> > +
> > static __always_inline u64 __seamcall_dirty_cache(sc_func_t func, u64 fn,
> > struct tdx_module_args *args)
> > {
> > @@ -39,6 +48,7 @@ static __always_inline u64 __seamcall_dirty_cache(sc_func_t func, u64 fn,
> > */
> > this_cpu_write(cache_state_incoherent, true);
> >
> > + FIELD_MODIFY(SEAMCALL_VERSION_MASK, &fn, args->version);
> > return func(fn, args);
> > }
>
> This is really looking fragmented and inconsistent.
>
> What if someone *does* set the version bits in 'fn'? Also, if the "leaf
> number" is just 16 bits, why is it a u64 in the API?
>
> Additionally, look at this:
>
> > /*
> > * Used in __tdcall*() to gather the input/output registers' values of the
> > * TDCALL instruction when requesting services from the TDX module. This is a
> > * software only structure and not part of the TDX module/VMM ABI
> > */
> > struct tdx_module_args {
>
> "version" doesn't fit this comment, does it? It's not a register.
>
> If "fn" is just the 16-bit leaf number it should be a u16 everywhere.
> Then there's no worry about the version number leaking in there
> somewhere. The type can't even _carry_ the version number.
>
> As a general rule, I dislike doing things in assembly that can be done
> in C. But, in this case, we have some pretty darn simple assembly doing
> a pretty simple job: marshaling data out of 'tdx_module_args' and in to
> registers.
Ughh.. I think it is totally wrong direction.
The version is not a register operand, it is part of the function
number. And RAX carries more than leaf+version: bit 24 is
INTERRUPT_MODE (enumerated by TDX_FEATURES0 bit 62) and bit 63 selects
P-SEAMLDR. If the version gets a struct field marshaled in assembly,
do we add fields for these bits too? Composing 'fn' as u64 in the
caller is the honest representation of the ABI.
Rather than pushing the version all the way down to assembly, let's
look if we can salvage what was proposed initially[1].
I know you didn't like it, but the problem there was that the version
was invisible: TDH_SYS_UPDATE was silently v1 and the old one got
renamed to TDH_SYS_UPDATE_V0. With the version spelled out at the call
site, that objection goes away:
int tdx_module_run_update(void)
{
struct tdx_module_args args = {};
u64 seamcall_fn = TDH_SYS_UPDATE;
int ret;
if (tdx_addon_feature0) {
seamcall_fn |= SEAMCALL_LEAF_VER(1);
args.r9 = tdx_addon_feature0;
}
ret = seamcall_prerr(seamcall_fn, &args);
if (ret)
return ret;
...
}
And tdh_vp_init() stays what it is in mainline today, just with the
macro instead of the open-coded shift:
/* apicid requires version == 1. */
return seamcall(TDH_VP_INIT | SEAMCALL_LEAF_VER(1), &args);
This also addresses all three complaints above at once: there is no
FIELD_MODIFY() to silently overwrite version bits in 'fn' as it is
exactly what the caller wrote, tdx_module_args stays a pure register
structure, and the assembly is untouched.
[1] https://lore.kernel.org/kvm/20260618081355.3253581-3-yilun.xu@linux.intel.com/
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v7 16/22] dma-direct: make dma_direct_map_phys() honor DMA_ATTR_CC_SHARED
From: Jason Gunthorpe @ 2026-07-10 16:19 UTC (permalink / raw)
To: 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, Alexey Kardashevskiy, 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, Jiri Pirko,
Michael Kelley
In-Reply-To: <yq5ajyr3h106.fsf@kernel.org>
On Fri, Jul 10, 2026 at 10:52:49AM +0530, Aneesh Kumar K.V wrote:
> >> > /*
> >> > * For host memory encryption and device requiring unencrypted DMA,
> >> > * MMIO memory is treated as shared by default.
> >> > */
> >> > if (attrs & DMA_ATTR_MMIO) {
> >> > if (cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) || force_dma_unencrypted(dev))
> >> > attrs |= DMA_ATTR_CC_SHARED;
> >> > }
> >>
> >> Yes, I think it does the trick, preserves the current semantics for AMD.
> >> I guess you could use a single 'if' for all checks (up to you).
> >
> > Please don't change it, MMIO P2P is broken on CC systems today and it
> > should stay broken. Passing DMA_ATTR_MMIO with DMA_ATTR_CC_SHARED is
> > an error that we need to correct in the drivers not make work in the
> > core code.
> >
>
> But the above changes are intended to handle HOST_MEM_ENCRYPT. In v7, we
> had the following diff:
To follow how the rest of the decrypted/encrypted stuff works the MMIO
has to be flaged with CC_SHARED for HOST_MEM_ENCRYPT too, just like
the PTEs.
> @@ -88,37 +88,40 @@ static inline dma_addr_t dma_direct_map_phys(struct device *dev,
> {
> dma_addr_t dma_addr;
>
> + /*
> + * For a device requiring unencrypted DMA, MMIO memory is treated
> + * as shared by default.
> + */
> + if (force_dma_unencrypted(dev) && (attrs & DMA_ATTR_MMIO))
> + attrs |= DMA_ATTR_CC_SHARED;
force_dma_unencrypted() says nothing about the properties of the
address passed in, this was nonsense :\
> As we discussed [1], that can come in a later patch. In the meantime, adding
> the HOST_MEM_ENCRYPT check preserves the previous behavior for SME.
It never worked. When we added ATTR_MMIO it started to have a chance
to work but prior to that it was always broken anyhow. I don't see
there is much merit in preserving the narrow window when we
inadvertantly had a half working ATTR_MMIO.
But if you really want to it should be
cc_platform_has(CC_ATTR_HOST_MEM_ENCRYPT) *only* and get rid of the wrong
force_dma_unencrypted().
But IMHO, I'd rather this series treat ATTR_MMIO as private MMIO and
ATTR_MMIO|CC_SHARED as shared MMIO and that's the right and correct
thing for the DMA API.
Jason
^ 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