* [RFC PATCH 3/4] x86/sev: Introduce hotplug-aware SNP page state validation
From: Pratik R. Sampat @ 2025-11-25 17:57 UTC (permalink / raw)
To: linux-mm, linux-coco, linux-efi, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
In-Reply-To: <20251125175753.1428857-1-prsampat@amd.com>
When hot-removing memory in a SEV-SNP environment, pages must be set to
shared state so they can be reused by the hypervisor. This also applies
when memory is intended to be hotplugged back in later, as those pages
will need to be re-accepted after crossing the trust boundary.
However, memory can already be set to shared state externally. In such
cases, the pvalidate rescind operation will not change the validated bit
in the RMP table, setting the carry flag and causing the guest to
terminate.
Since memory hotplug is arguably unique, introduce a guest-maintained
memory state tracking structure that maintains a bitmap to track the
state (private vs shared) of all hotplugged memory supplemented with a
flag to indicate intent. This allows for memory that is already marked
as shared in the hotplug bitmap to avoid performing the pvalidate
rescind operation. Additionally, tracking page state changes from the
guest's perspective, enables the detection of inconsistencies if the
hypervisor changes states unexpectedly. For example, if the guest bitmap
reports memory as private but the hypervisor has already changed the RMP
state to shared, the guest detects this inconsistency when attempting to
share the memory and terminate rather than skipping over the pvalidate
rescind operation.
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
arch/x86/coco/sev/core.c | 104 +++++++++++++++++++++--
arch/x86/include/asm/sev.h | 32 +++++++
arch/x86/include/asm/unaccepted_memory.h | 13 +++
drivers/firmware/efi/unaccepted_memory.c | 2 +-
4 files changed, 143 insertions(+), 8 deletions(-)
diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c
index 14ef5908fb27..a5c9615a6e0c 100644
--- a/arch/x86/coco/sev/core.c
+++ b/arch/x86/coco/sev/core.c
@@ -46,6 +46,8 @@
#include <asm/cmdline.h>
#include <asm/msr.h>
+struct snp_hotplug_memory *snp_hp_mem;
+
/* AP INIT values as documented in the APM2 section "Processor Initialization State" */
#define AP_INIT_CS_LIMIT 0xffff
#define AP_INIT_DS_LIMIT 0xffff
@@ -453,9 +455,54 @@ static int vmgexit_psc(struct ghcb *ghcb, struct snp_psc_desc *desc)
return ret;
}
+static bool snp_hotplug_state_shared(unsigned long vaddr)
+{
+ phys_addr_t paddr = __pa(vaddr);
+ u64 hotplug_bit;
+
+ if (!snp_is_hotplug_memory(paddr))
+ return false;
+
+ hotplug_bit = (paddr - snp_hp_mem->phys_base) / snp_hp_mem->unit_size;
+
+ return !test_bit(hotplug_bit, snp_hp_mem->bitmap);
+}
+
+static void snp_set_hotplug_bit(unsigned long vaddr, bool private)
+{
+ phys_addr_t paddr = __pa(vaddr);
+ u64 hotplug_bit;
+
+ if (!snp_is_hotplug_memory(paddr))
+ return;
+
+ hotplug_bit = (paddr - snp_hp_mem->phys_base) / snp_hp_mem->unit_size;
+ if (private)
+ set_bit(hotplug_bit, snp_hp_mem->bitmap);
+ else
+ clear_bit(hotplug_bit, snp_hp_mem->bitmap);
+}
+
+static void set_hotplug_pages_state(struct snp_psc_desc *desc)
+{
+ struct psc_entry *e;
+ unsigned long vaddr;
+ bool op;
+ int i;
+
+ for (i = 0; i <= desc->hdr.end_entry; i++) {
+ e = &desc->entries[i];
+ vaddr = (unsigned long)pfn_to_kaddr(e->gfn);
+ op = e->operation == SNP_PAGE_STATE_PRIVATE;
+
+ snp_set_hotplug_bit(vaddr, op);
+ }
+}
+
static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long vaddr,
- unsigned long vaddr_end, int op)
+ unsigned long vaddr_end, int op, u8 psc_flags)
{
+ unsigned long vaddr_base;
struct ghcb_state state;
bool use_large_entry;
struct psc_hdr *hdr;
@@ -465,6 +512,7 @@ static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long
struct ghcb *ghcb;
int i;
+ vaddr_base = vaddr;
hdr = &data->hdr;
e = data->entries;
@@ -499,7 +547,8 @@ static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long
}
/* Page validation must be rescinded before changing to shared */
- if (op == SNP_PAGE_STATE_SHARED)
+ if (op == SNP_PAGE_STATE_SHARED &&
+ !(snp_hotplug_state_shared(vaddr_base) && (psc_flags & SNP_PSC_SHARED_TO_SHARED)))
pvalidate_pages(data);
local_irq_save(flags);
@@ -522,10 +571,12 @@ static unsigned long __set_pages_state(struct snp_psc_desc *data, unsigned long
if (op == SNP_PAGE_STATE_PRIVATE)
pvalidate_pages(data);
+ set_hotplug_pages_state(data);
+
return vaddr;
}
-static void set_pages_state(unsigned long vaddr, unsigned long npages, int op)
+static void set_pages_state(unsigned long vaddr, unsigned long npages, int op, u8 psc_flags)
{
struct snp_psc_desc desc;
unsigned long vaddr_end;
@@ -538,7 +589,7 @@ static void set_pages_state(unsigned long vaddr, unsigned long npages, int op)
vaddr_end = vaddr + (npages << PAGE_SHIFT);
while (vaddr < vaddr_end)
- vaddr = __set_pages_state(&desc, vaddr, vaddr_end, op);
+ vaddr = __set_pages_state(&desc, vaddr, vaddr_end, op, psc_flags);
}
void snp_set_memory_shared(unsigned long vaddr, unsigned long npages)
@@ -546,7 +597,7 @@ void snp_set_memory_shared(unsigned long vaddr, unsigned long npages)
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
return;
- set_pages_state(vaddr, npages, SNP_PAGE_STATE_SHARED);
+ set_pages_state(vaddr, npages, SNP_PAGE_STATE_SHARED, 0);
}
void snp_set_memory_private(unsigned long vaddr, unsigned long npages)
@@ -554,7 +605,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned long npages)
if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
return;
- set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
+ set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE, 0);
}
void snp_accept_memory(phys_addr_t start, phys_addr_t end)
@@ -567,7 +618,46 @@ void snp_accept_memory(phys_addr_t start, phys_addr_t end)
vaddr = (unsigned long)__va(start);
npages = (end - start) >> PAGE_SHIFT;
- set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE);
+ set_pages_state(vaddr, npages, SNP_PAGE_STATE_PRIVATE, 0);
+}
+
+int snp_extend_hotplug_memory_state_bitmap(phys_addr_t start,
+ unsigned long size,
+ uint64_t unit_size)
+{
+ u64 hp_mem_size = DIV_ROUND_UP(size, unit_size * BITS_PER_BYTE);
+
+ if (snp_hp_mem) {
+ u64 old_size = snp_hp_mem->size;
+ unsigned long *bitmap;
+
+ bitmap = krealloc(snp_hp_mem->bitmap, hp_mem_size, GFP_KERNEL);
+ if (!bitmap)
+ return -ENOMEM;
+
+ memset(bitmap + old_size, 0, hp_mem_size - old_size);
+ snp_hp_mem->size = hp_mem_size;
+ snp_hp_mem->bitmap = bitmap;
+
+ return 0;
+ }
+
+ snp_hp_mem = kzalloc(sizeof(*snp_hp_mem), GFP_KERNEL);
+ if (!snp_hp_mem)
+ return -ENOMEM;
+
+ snp_hp_mem->bitmap = kzalloc(hp_mem_size, GFP_KERNEL);
+ if (!snp_hp_mem->bitmap) {
+ kfree(snp_hp_mem);
+ return -ENOMEM;
+ }
+
+ snp_hp_mem->phys_base = start;
+ snp_hp_mem->phys_end = start + hp_mem_size;
+ snp_hp_mem->size = hp_mem_size;
+ snp_hp_mem->unit_size = unit_size;
+
+ return 0;
}
static int vmgexit_ap_control(u64 event, struct sev_es_save_area *vmsa, u32 apic_id)
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h
index 465b19fd1a2d..eb605892645c 100644
--- a/arch/x86/include/asm/sev.h
+++ b/arch/x86/include/asm/sev.h
@@ -464,6 +464,38 @@ static __always_inline void sev_es_nmi_complete(void)
extern int __init sev_es_efi_map_ghcbs_cas(pgd_t *pgd);
extern void sev_enable(struct boot_params *bp);
+#define SNP_PSC_SHARED_TO_SHARED 0x1
+
+struct snp_hotplug_memory {
+ u64 phys_base;
+ u64 phys_end;
+ u32 unit_size;
+ u64 size;
+ /* bitmap bit unset: shared, set: private */
+ unsigned long *bitmap;
+};
+
+extern struct snp_hotplug_memory *snp_hp_mem;
+
+#ifdef CONFIG_UNACCEPTED_MEMORY
+int snp_extend_hotplug_memory_state_bitmap(phys_addr_t start,
+ unsigned long size,
+ uint64_t unit_size);
+static inline bool snp_is_hotplug_memory(phys_addr_t paddr)
+{
+ return snp_hp_mem && paddr >= snp_hp_mem->phys_base && paddr < snp_hp_mem->phys_end;
+}
+#else /* !CONFIG_UNACCEPTED_MEMORY */
+static inline int snp_extend_hotplug_memory_state_bitmap(phys_addr_t start,
+ unsigned long size,
+ uint64_t unit_size)
+{
+ return 0;
+}
+
+static inline bool snp_is_hotplug_memory(phys_addr_t paddr) { return false; }
+#endif
+
/*
* RMPADJUST modifies the RMP permissions of a page of a lesser-
* privileged (numerically higher) VMPL.
diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
index 5da80e68d718..abdf5472de9e 100644
--- a/arch/x86/include/asm/unaccepted_memory.h
+++ b/arch/x86/include/asm/unaccepted_memory.h
@@ -33,4 +33,17 @@ static inline unsigned long *efi_get_unaccepted_bitmap(void)
return NULL;
return __va(unaccepted->bitmap);
}
+
+static inline int arch_set_unaccepted_mem_state(phys_addr_t start, unsigned long size)
+{
+ struct efi_unaccepted_memory *unaccepted = efi_get_unaccepted_table();
+
+ if (!unaccepted)
+ return -EIO;
+
+ if (cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
+ return snp_extend_hotplug_memory_state_bitmap(start, size, unaccepted->unit_size);
+
+ return 0;
+}
#endif
diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index 8537812346e2..6796042a64aa 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -281,7 +281,7 @@ static int extend_unaccepted_bitmap(phys_addr_t mem_range_start,
unacc_tbl->bitmap = (unsigned long *)__pa(new_bitmap);
spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
- return 0;
+ return arch_set_unaccepted_mem_state(mem_range_start, mem_range_size);
}
int accept_hotplug_memory(phys_addr_t mem_range_start, unsigned long mem_range_size)
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Pratik R. Sampat @ 2025-11-25 17:57 UTC (permalink / raw)
To: linux-mm, linux-coco, linux-efi, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
In-Reply-To: <20251125175753.1428857-1-prsampat@amd.com>
The unaccepted memory structure currently only supports accepting memory
present at boot time. The unaccepted table uses a fixed-size bitmap
reserved in memblock based on the initial memory layout, preventing
dynamic addition of memory ranges after boot. This causes guest
termination when memory is hot-added in a secure virtual machine due to
accessing pages that have not transitioned to private before use.
Extend the unaccepted memory framework to handle hotplugged memory by
dynamically managing the unaccepted bitmap. Allocate a new bitmap when
hotplugged ranges exceed the reserved bitmap capacity and switch to
kernel-managed allocation.
Hotplugged memory also follows the same acceptance policy using the
accept_memory=[eager|lazy] kernel parameter to accept memory either
up-front when added or before first use.
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
arch/x86/boot/compressed/efi.h | 1 +
.../firmware/efi/libstub/unaccepted_memory.c | 1 +
drivers/firmware/efi/unaccepted_memory.c | 83 +++++++++++++++++++
include/linux/efi.h | 1 +
include/linux/mm.h | 11 +++
mm/memory_hotplug.c | 7 ++
mm/page_alloc.c | 2 +
7 files changed, 106 insertions(+)
diff --git a/arch/x86/boot/compressed/efi.h b/arch/x86/boot/compressed/efi.h
index 4f7027f33def..a220a1966cae 100644
--- a/arch/x86/boot/compressed/efi.h
+++ b/arch/x86/boot/compressed/efi.h
@@ -102,6 +102,7 @@ struct efi_unaccepted_memory {
u32 unit_size;
u64 phys_base;
u64 size;
+ bool mem_reserved;
unsigned long *bitmap;
};
diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index c1370fc14555..b16bd61c12bf 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -83,6 +83,7 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
unaccepted_table->unit_size = EFI_UNACCEPTED_UNIT_SIZE;
unaccepted_table->phys_base = unaccepted_start;
unaccepted_table->size = bitmap_size;
+ unaccepted_table->mem_reserved = true;
memset(unaccepted_table->bitmap, 0, bitmap_size);
status = efi_bs_call(install_configuration_table,
diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index 4479aad258f8..8537812346e2 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -218,6 +218,89 @@ bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size)
return ret;
}
+static int extend_unaccepted_bitmap(phys_addr_t mem_range_start,
+ unsigned long mem_range_size)
+{
+ struct efi_unaccepted_memory *unacc_tbl;
+ unsigned long *old_bitmap, *new_bitmap;
+ phys_addr_t start, end, mem_range_end;
+ u64 phys_base, size, unit_size;
+ unsigned long flags;
+
+ unacc_tbl = efi_get_unaccepted_table();
+ if (!unacc_tbl || !unacc_tbl->unit_size)
+ return -EIO;
+
+ unit_size = unacc_tbl->unit_size;
+ phys_base = unacc_tbl->phys_base;
+
+ mem_range_end = round_up(mem_range_start + mem_range_size, unit_size);
+ size = DIV_ROUND_UP(mem_range_end - phys_base, unit_size * BITS_PER_BYTE);
+
+ /* Translate to offsets from the beginning of the bitmap */
+ start = mem_range_start - phys_base;
+ end = mem_range_end - phys_base;
+
+ old_bitmap = efi_get_unaccepted_bitmap();
+ if (!old_bitmap)
+ return -EIO;
+
+ /* If the bitmap is already large enough, just set the bits */
+ if (unacc_tbl->size >= size) {
+ spin_lock_irqsave(&unaccepted_memory_lock, flags);
+ bitmap_set(old_bitmap, start / unit_size, (end - start) / unit_size);
+ spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+
+ return 0;
+ }
+
+ /* Reserved memblocks cannot be extended so allocate a new bitmap */
+ if (unacc_tbl->mem_reserved) {
+ new_bitmap = kzalloc(size, GFP_KERNEL);
+ if (!new_bitmap)
+ return -ENOMEM;
+
+ spin_lock_irqsave(&unaccepted_memory_lock, flags);
+ memcpy(new_bitmap, old_bitmap, unacc_tbl->size);
+ unacc_tbl->mem_reserved = false;
+ free_reserved_area(old_bitmap, old_bitmap + unacc_tbl->size, -1, NULL);
+ spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+ } else {
+ new_bitmap = krealloc(old_bitmap, size, GFP_KERNEL);
+ if (!new_bitmap)
+ return -ENOMEM;
+
+ /* Zero the bitmap from the range it was extended from */
+ memset(new_bitmap + unacc_tbl->size, 0, size - unacc_tbl->size);
+ }
+
+ bitmap_set(new_bitmap, start / unit_size, (end - start) / unit_size);
+
+ spin_lock_irqsave(&unaccepted_memory_lock, flags);
+ unacc_tbl->size = size;
+ unacc_tbl->bitmap = (unsigned long *)__pa(new_bitmap);
+ spin_unlock_irqrestore(&unaccepted_memory_lock, flags);
+
+ return 0;
+}
+
+int accept_hotplug_memory(phys_addr_t mem_range_start, unsigned long mem_range_size)
+{
+ int ret;
+
+ if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY))
+ return 0;
+
+ ret = extend_unaccepted_bitmap(mem_range_start, mem_range_size);
+ if (ret)
+ return ret;
+
+ if (!mm_lazy_accept_enabled())
+ accept_memory(mem_range_start, mem_range_size);
+
+ return 0;
+}
+
#ifdef CONFIG_PROC_VMCORE
static bool unaccepted_memory_vmcore_pfn_is_ram(struct vmcore_cb *cb,
unsigned long pfn)
diff --git a/include/linux/efi.h b/include/linux/efi.h
index a74b393c54d8..1021eb78388f 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -545,6 +545,7 @@ struct efi_unaccepted_memory {
u32 unit_size;
u64 phys_base;
u64 size;
+ bool mem_reserved;
unsigned long *bitmap;
};
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 1ae97a0b8ec7..bb43876e6c47 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4077,6 +4077,9 @@ int set_anon_vma_name(unsigned long addr, unsigned long size,
bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size);
void accept_memory(phys_addr_t start, unsigned long size);
+int accept_hotplug_memory(phys_addr_t mem_range_start,
+ unsigned long mem_range_size);
+bool mm_lazy_accept_enabled(void);
#else
@@ -4090,6 +4093,14 @@ static inline void accept_memory(phys_addr_t start, unsigned long size)
{
}
+static inline int accept_hotplug_memory(phys_addr_t mem_range_start,
+ unsigned long mem_range_size)
+{
+ return 0;
+}
+
+static inline bool mm_lazy_accept_enabled(void) { return false; }
+
#endif
static inline bool pfn_is_unaccepted_memory(unsigned long pfn)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 74318c787715..bf8086682b66 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1581,6 +1581,13 @@ int add_memory_resource(int nid, struct resource *res, mhp_t mhp_flags)
if (!strcmp(res->name, "System RAM"))
firmware_map_add_hotplug(start, start + size, "System RAM");
+ ret = accept_hotplug_memory(start, size);
+ if (ret) {
+ remove_memory_block_devices(start, size);
+ arch_remove_memory(start, size, params.altmap);
+ goto error;
+ }
+
/* device_online() will take the lock when calling online_pages() */
mem_hotplug_done();
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d1d037f97c5f..d0c298dcaf9d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -7331,6 +7331,8 @@ bool has_managed_dma(void)
static bool lazy_accept = true;
+bool mm_lazy_accept_enabled(void) { return lazy_accept; }
+
static int __init accept_memory_parse(char *p)
{
if (!strcmp(p, "lazy")) {
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 1/4] efi/libstub: Decouple memory bitmap from the unaccepted table
From: Pratik R. Sampat @ 2025-11-25 17:57 UTC (permalink / raw)
To: linux-mm, linux-coco, linux-efi, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
In-Reply-To: <20251125175753.1428857-1-prsampat@amd.com>
Memory hotplug in secure environments requires the unaccepted memory
bitmap to grow as new memory is added. Currently, the bitmap is
implemented as a flexible array member at the end of struct
efi_unaccepted_memory, which is reserved by memblock at boot and cannot
be resized without reallocating the entire structure.
Replace the flexible array member with a pointer. This allows the bitmap
to be allocated and managed independently from the unaccepted memory
table, enabling dynamic growth to support memory hotplug.
Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
---
arch/x86/boot/compressed/efi.h | 2 +-
arch/x86/include/asm/unaccepted_memory.h | 9 +++++++++
.../firmware/efi/libstub/unaccepted_memory.c | 11 ++++++++++-
drivers/firmware/efi/unaccepted_memory.c | 19 ++++++++++++++-----
include/linux/efi.h | 2 +-
5 files changed, 35 insertions(+), 8 deletions(-)
diff --git a/arch/x86/boot/compressed/efi.h b/arch/x86/boot/compressed/efi.h
index b22300970f97..4f7027f33def 100644
--- a/arch/x86/boot/compressed/efi.h
+++ b/arch/x86/boot/compressed/efi.h
@@ -102,7 +102,7 @@ struct efi_unaccepted_memory {
u32 unit_size;
u64 phys_base;
u64 size;
- unsigned long bitmap[];
+ unsigned long *bitmap;
};
static inline int efi_guidcmp (efi_guid_t left, efi_guid_t right)
diff --git a/arch/x86/include/asm/unaccepted_memory.h b/arch/x86/include/asm/unaccepted_memory.h
index f5937e9866ac..5da80e68d718 100644
--- a/arch/x86/include/asm/unaccepted_memory.h
+++ b/arch/x86/include/asm/unaccepted_memory.h
@@ -24,4 +24,13 @@ static inline struct efi_unaccepted_memory *efi_get_unaccepted_table(void)
return NULL;
return __va(efi.unaccepted);
}
+
+static inline unsigned long *efi_get_unaccepted_bitmap(void)
+{
+ struct efi_unaccepted_memory *unaccepted = efi_get_unaccepted_table();
+
+ if (!unaccepted)
+ return NULL;
+ return __va(unaccepted->bitmap);
+}
#endif
diff --git a/drivers/firmware/efi/libstub/unaccepted_memory.c b/drivers/firmware/efi/libstub/unaccepted_memory.c
index 757dbe734a47..c1370fc14555 100644
--- a/drivers/firmware/efi/libstub/unaccepted_memory.c
+++ b/drivers/firmware/efi/libstub/unaccepted_memory.c
@@ -63,13 +63,22 @@ efi_status_t allocate_unaccepted_bitmap(__u32 nr_desc,
EFI_UNACCEPTED_UNIT_SIZE * BITS_PER_BYTE);
status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
- sizeof(*unaccepted_table) + bitmap_size,
+ sizeof(*unaccepted_table),
(void **)&unaccepted_table);
if (status != EFI_SUCCESS) {
efi_err("Failed to allocate unaccepted memory config table\n");
return status;
}
+ status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
+ bitmap_size,
+ (void **)&unaccepted_table->bitmap);
+ if (status != EFI_SUCCESS) {
+ efi_bs_call(free_pool, unaccepted_table);
+ efi_err("Failed to allocate unaccepted memory bitmap\n");
+ return status;
+ }
+
unaccepted_table->version = 1;
unaccepted_table->unit_size = EFI_UNACCEPTED_UNIT_SIZE;
unaccepted_table->phys_base = unaccepted_start;
diff --git a/drivers/firmware/efi/unaccepted_memory.c b/drivers/firmware/efi/unaccepted_memory.c
index c2c067eff634..4479aad258f8 100644
--- a/drivers/firmware/efi/unaccepted_memory.c
+++ b/drivers/firmware/efi/unaccepted_memory.c
@@ -36,7 +36,7 @@ void accept_memory(phys_addr_t start, unsigned long size)
unsigned long range_start, range_end;
struct accept_range range, *entry;
phys_addr_t end = start + size;
- unsigned long flags;
+ unsigned long flags, *bitmap;
u64 unit_size;
unaccepted = efi_get_unaccepted_table();
@@ -124,8 +124,12 @@ void accept_memory(phys_addr_t start, unsigned long size)
list_add(&range.list, &accepting_list);
range_start = range.start;
- for_each_set_bitrange_from(range_start, range_end, unaccepted->bitmap,
- range.end) {
+
+ bitmap = efi_get_unaccepted_bitmap();
+ if (!bitmap)
+ return;
+
+ for_each_set_bitrange_from(range_start, range_end, bitmap, range.end) {
unsigned long phys_start, phys_end;
unsigned long len = range_end - range_start;
@@ -147,7 +151,7 @@ void accept_memory(phys_addr_t start, unsigned long size)
arch_accept_memory(phys_start, phys_end);
spin_lock(&unaccepted_memory_lock);
- bitmap_clear(unaccepted->bitmap, range_start, len);
+ bitmap_clear(bitmap, range_start, len);
}
list_del(&range.list);
@@ -197,7 +201,12 @@ bool range_contains_unaccepted_memory(phys_addr_t start, unsigned long size)
spin_lock_irqsave(&unaccepted_memory_lock, flags);
while (start < end) {
- if (test_bit(start / unit_size, unaccepted->bitmap)) {
+ unsigned long *bitmap = efi_get_unaccepted_bitmap();
+
+ if (!bitmap)
+ break;
+
+ if (test_bit(start / unit_size, bitmap)) {
ret = true;
break;
}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index a98cc39e7aaa..a74b393c54d8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -545,7 +545,7 @@ struct efi_unaccepted_memory {
u32 unit_size;
u64 phys_base;
u64 size;
- unsigned long bitmap[];
+ unsigned long *bitmap;
};
/*
--
2.51.1
^ permalink raw reply related
* [RFC PATCH 0/4] SEV-SNP Unaccepted Memory Hotplug
From: Pratik R. Sampat @ 2025-11-25 17:57 UTC (permalink / raw)
To: linux-mm, linux-coco, linux-efi, x86, linux-kernel
Cc: tglx, mingo, bp, dave.hansen, kas, ardb, akpm, david, osalvador,
thomas.lendacky, michael.roth, prsampat
Guest memory hot-plug/remove via the QEMU monitor is used by virtual
machines to dynamically scale the memory capacity of a system with
virtually zero downtime to the guest. For confidential VMs, memory has
to be first accepted before it can be used.
The unaccepted memory feature provides a mechanism to accept memory
either up-front or right before it is needed. The unaccepted table that
tracks this information is allocated and memory block reserved at boot
time. For memory hotplug, this means the table cannot be updated to
track additional regions and accept them as the guest physical memory
grows.
This proof-of-concept series extends the unaccepted memory
infrastructure to support memory hotplug and hot-unplug on the SNP
platform. On a high-level, it does so by decoupling the memory bitmap
from the unaccepted table so that kernel can manage bitmap when memory
is added. For hot-remove, it reverts the page states so that the
hypervisor can reuse that memory. Hot-remove also presents a unique
scenario where the memory we attempt to share can already be in a shared
state set externally which can cause pvalidation on the platform to fail
since no updates were made to the validated bit. Handle this case by
tracking the state of hotplugged memory within the guest and disallow
pvalidate operations on the same state.
Usage (for SNP guests)
----------------------
Step1: Spawn a QEMU SNP guest with the additional parameter of slots and
maximum possible memory, along with the initial memory as below:
"-m X,slots=Y,maxmem=Z".
Use the "accept_memory=[eager|lazy]" kernel command-line parameter to
specify whether hotplugged memory should be accepted immediately upon
addition or only when first accessed. By default, lazy acceptance is
used.
Step2: Once the guest is booted, launch the qemu monitor and hotplug
the memory as follows:
(qemu) object_add memory-backend-memfd,id=mem1,size=1G
(qemu) device_add pc-dimm,id=dimm1,memdev=mem1
Step3: If using auto-onlining by either:
a) echo online > /sys/devices/system/memory/auto_online_blocks, OR
b) enable CONFIG_MHP_DEFAULT_ONLINE_TYPE_* while compiling kernel
Memory should show up automatically.
Otherwise, memory can also be onlined by echoing 1 to the newly added
blocks in: /sys/devices/system/memory/memoryXX/online
Step4: If accept_memory is set to eager, all memory is accepted
immediately. Otherwise, memory is accepted on access. For the latter,
acceptance can be triggered by simply running a program such as
stress-ng that requests enough memory to cover the newly allocated
hotplugged regions.
$ stress-ng --vm 1 --vm-bytes={X}G -t {T}s
Step5: memory can be hot-removed using the qemu monitor using:
(qemu) device_remove dimm1
(qemu) object_remove mem1
Tip: Enable the kvm_convert_memory event in QEMU to observe memory
conversions between private and shared during hotplug/remove.
The series is based on
git.kernel.org/pub/scm/virt/kvm/kvm.git next
Comments and feedback appreciated!
Pratik R. Sampat (4):
efi/libstub: Decouple memory bitmap from the unaccepted table
mm: Add support for unaccepted memory hotplug
x86/sev: Introduce hotplug-aware SNP page state validation
mm: Add support for unaccepted memory hot-remove
arch/x86/boot/compressed/efi.h | 3 +-
arch/x86/coco/sev/core.c | 127 +++++++++++++++-
arch/x86/include/asm/sev.h | 34 +++++
arch/x86/include/asm/unaccepted_memory.h | 31 ++++
.../firmware/efi/libstub/unaccepted_memory.c | 12 +-
drivers/firmware/efi/unaccepted_memory.c | 139 +++++++++++++++++-
include/linux/efi.h | 3 +-
include/linux/mm.h | 18 +++
mm/memory_hotplug.c | 9 ++
mm/page_alloc.c | 2 +
10 files changed, 363 insertions(+), 15 deletions(-)
--
2.51.1
^ permalink raw reply
* Re: [PATCH kernel v2 0/5] PCI/TSM: Enabling core infrastructure on
From: Joerg Roedel @ 2025-11-25 14:17 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linux-kernel, linux-crypto, Tom Lendacky, John Allen, Herbert Xu,
David S. Miller, Ashish Kalra, Suravee Suthikulpanit, Will Deacon,
Robin Murphy, Borislav Petkov (AMD), Kim Phillips,
Jerry Snitselaar, Vasant Hegde, Jason Gunthorpe, Gao Shiyuan,
Sean Christopherson, Nikunj A Dadhania, Michael Roth, Amit Shah,
Peter Gonda, iommu, linux-coco@lists.linux.dev, Dan Williams
In-Reply-To: <b56046b5-1fa2-404f-b99f-353ec8567621@amd.com>
Hey Alexey,
On Sat, Nov 22, 2025 at 02:35:03PM +1100, Alexey Kardashevskiy wrote:
> I should have cc'ed linux-coco@lists.linux.dev. And vim ate "AMD" from the
> subject line. Should I repost now? Thanks,
A full repost it not needed, imho. Just reply with linux-coco on CC for
awareness. Everyone on that list can then lookup the full thread on lore.
Just make sure to Cc linux-coco on the next version.
Regards,
Joerg
^ permalink raw reply
* Re: [PATCH v4 10/16] KVM: TDX: Allocate PAMT memory for vCPU control structures
From: Binbin Wu @ 2025-11-25 9:14 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-11-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> TDX vCPU control structures are provided to the TDX module at 4KB page
> size and require PAMT backing. This means for Dynamic PAMT they need to
> also have 4KB backings installed.
>
> Previous changes introduced tdx_alloc_page()/tdx_free_page() that can
> allocate a page and automatically handle the DPAMT maintenance. Use them
> for vCPU control structures instead of alloc_page()/__free_page().
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [update log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> v3:
> - Write log. Reame from “Allocate PAMT memory for TDH.VP.CREATE and
> TDH.VP.ADDCX”.
> - Remove new line damage
> ---
> arch/x86/kvm/vmx/tdx.c | 12 +++++-------
> 1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 8c4c1221e311..b6d7f4b5f40f 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2882,7 +2882,7 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> int ret, i;
> u64 err;
>
> - page = alloc_page(GFP_KERNEL);
> + page = tdx_alloc_page();
> if (!page)
> return -ENOMEM;
> tdx->vp.tdvpr_page = page;
> @@ -2902,7 +2902,7 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> }
>
> for (i = 0; i < kvm_tdx->td.tdcx_nr_pages; i++) {
> - page = alloc_page(GFP_KERNEL);
> + page = tdx_alloc_page();
> if (!page) {
> ret = -ENOMEM;
> goto free_tdcx;
> @@ -2924,7 +2924,7 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
> * method, but the rest are freed here.
> */
> for (; i < kvm_tdx->td.tdcx_nr_pages; i++) {
> - __free_page(tdx->vp.tdcx_pages[i]);
> + tdx_free_page(tdx->vp.tdcx_pages[i]);
> tdx->vp.tdcx_pages[i] = NULL;
> }
> return -EIO;
> @@ -2952,16 +2952,14 @@ static int tdx_td_vcpu_init(struct kvm_vcpu *vcpu, u64 vcpu_rcx)
>
> free_tdcx:
> for (i = 0; i < kvm_tdx->td.tdcx_nr_pages; i++) {
> - if (tdx->vp.tdcx_pages[i])
> - __free_page(tdx->vp.tdcx_pages[i]);
> + tdx_free_page(tdx->vp.tdcx_pages[i]);
> tdx->vp.tdcx_pages[i] = NULL;
> }
> kfree(tdx->vp.tdcx_pages);
> tdx->vp.tdcx_pages = NULL;
>
> free_tdvpr:
> - if (tdx->vp.tdvpr_page)
> - __free_page(tdx->vp.tdvpr_page);
> + tdx_free_page(tdx->vp.tdvpr_page);
> tdx->vp.tdvpr_page = NULL;
> tdx->vp.tdvpr_pa = 0;
>
^ permalink raw reply
* Re: [PATCH v4 09/16] KVM: TDX: Allocate PAMT memory for TD control structures
From: Binbin Wu @ 2025-11-25 9:11 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-10-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> TDX TD control structures are provided to the TDX module at 4KB page size
> and require PAMT backing. This means for Dynamic PAMT they need to also
> have 4KB backings installed.
>
> Previous changes introduced tdx_alloc_page()/tdx_free_page() that can
> allocate a page and automatically handle the DPAMT maintenance. Use them
> for vCPU control structures instead of alloc_page()/__free_page().
vCPU -> TD
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [update log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
One typo above.
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> ---
> v3:
> - Write log. Rename from “KVM: TDX: Allocate PAMT memory in __tdx_td_init()”
> ---
> arch/x86/kvm/vmx/tdx.c | 16 ++++++----------
> 1 file changed, 6 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 0eed334176b3..8c4c1221e311 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -2398,7 +2398,7 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
>
> atomic_inc(&nr_configured_hkid);
>
> - tdr_page = alloc_page(GFP_KERNEL);
> + tdr_page = tdx_alloc_page();
> if (!tdr_page)
> goto free_hkid;
>
> @@ -2411,7 +2411,7 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> goto free_tdr;
>
> for (i = 0; i < kvm_tdx->td.tdcs_nr_pages; i++) {
> - tdcs_pages[i] = alloc_page(GFP_KERNEL);
> + tdcs_pages[i] = tdx_alloc_page();
> if (!tdcs_pages[i])
> goto free_tdcs;
> }
> @@ -2529,10 +2529,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
> teardown:
> /* Only free pages not yet added, so start at 'i' */
> for (; i < kvm_tdx->td.tdcs_nr_pages; i++) {
> - if (tdcs_pages[i]) {
> - __free_page(tdcs_pages[i]);
> - tdcs_pages[i] = NULL;
> - }
> + tdx_free_page(tdcs_pages[i]);
> + tdcs_pages[i] = NULL;
> }
> if (!kvm_tdx->td.tdcs_pages)
> kfree(tdcs_pages);
> @@ -2548,15 +2546,13 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params,
>
> free_tdcs:
> for (i = 0; i < kvm_tdx->td.tdcs_nr_pages; i++) {
> - if (tdcs_pages[i])
> - __free_page(tdcs_pages[i]);
> + tdx_free_page(tdcs_pages[i]);
> }
> kfree(tdcs_pages);
> kvm_tdx->td.tdcs_pages = NULL;
>
> free_tdr:
> - if (tdr_page)
> - __free_page(tdr_page);
> + tdx_free_page(tdr_page);
> kvm_tdx->td.tdr_page = NULL;
>
> free_hkid:
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Binbin Wu @ 2025-11-25 8:09 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-8-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
[...]
>
> +/* Number PAMT pages to be provided to TDX module per 2M region of PA */
^ ^
of 2MB
> +static int tdx_dpamt_entry_pages(void)
> +{
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return 0;
> +
> + return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> +}
> +
> +/*
> + * The TDX spec treats the registers like an array, as they are ordered
> + * in the struct. The array size is limited by the number or registers,
> + * so define the max size it could be for worst case allocations and sanity
> + * checking.
> + */
> +#define MAX_TDX_ARG_SIZE(reg) (sizeof(struct tdx_module_args) - \
> + offsetof(struct tdx_module_args, reg))
This should be the maximum number of registers could be used to pass the
addresses of the pages (or other information), it needs to be divided by sizeof(u64).
Also, "SIZE" in the name could be confusing.
> +#define TDX_ARG_INDEX(reg) (offsetof(struct tdx_module_args, reg) / \
> + sizeof(u64))
> +
> +/*
> + * Treat struct the registers like an array that starts at RDX, per
> + * TDX spec. Do some sanitychecks, and return an indexable type.
sanitychecks -> sanity checks
> + */
[...]
> +/* Serializes adding/removing PAMT memory */
> +static DEFINE_SPINLOCK(pamt_lock);
> +
> +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> +int tdx_pamt_get(struct page *page)
> +{
> + u64 pamt_pa_array[MAX_TDX_ARG_SIZE(rdx)];
> + atomic_t *pamt_refcount;
> + u64 tdx_status;
> + int ret;
> +
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return 0;
> +
> + ret = alloc_pamt_array(pamt_pa_array);
> + if (ret)
> + goto out_free;
> +
> + pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> +
> + scoped_guard(spinlock, &pamt_lock) {
> + /*
> + * If the pamt page is already added (i.e. refcount >= 1),
> + * then just increment the refcount.
> + */
> + if (atomic_read(pamt_refcount)) {
> + atomic_inc(pamt_refcount);
So far, all atomic operations are inside the spinlock.
May be better to add some info in the change log that atomic is needed due to
the optimization in the later patch?
> + goto out_free;
> + }
> +
> + /* Try to add the pamt page and take the refcount 0->1. */
> +
> + tdx_status = tdh_phymem_pamt_add(page, pamt_pa_array);
> + if (!IS_TDX_SUCCESS(tdx_status)) {
> + pr_err("TDH_PHYMEM_PAMT_ADD failed: %#llx\n", tdx_status);
Can use pr_tdx_error().
Aslo, so for in this patch, when this SEAMCALL failed, does it indicate a bug?
> + goto out_free;
> + }
> +
> + atomic_inc(pamt_refcount);
> + }
> +
> + return ret;
Maybe just return 0 here since all error paths must be directed to out_free.
> +out_free:
> + /*
> + * pamt_pa_array is populated or zeroed up to tdx_dpamt_entry_pages()
> + * above. free_pamt_array() can handle either case.
> + */
> + free_pamt_array(pamt_pa_array);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tdx_pamt_get);
> +
>
[...]
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Binbin Wu @ 2025-11-25 3:15 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-7-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
[...]
> +
> +/* Unmap a page from the PAMT refcount vmalloc region */
> +static int pamt_refcount_depopulate(pte_t *pte, unsigned long addr, void *data)
> +{
> + struct page *page;
> + pte_t entry;
> +
> + spin_lock(&init_mm.page_table_lock);
> +
> + entry = ptep_get(pte);
> + /* refcount allocation is sparse, may not be populated */
Not sure this comment about "sparse" is accurate since this function is called via
apply_to_existing_page_range().
And the check for not present just for sanity check?
> + if (!pte_none(entry)) {
> + pte_clear(&init_mm, addr, pte);
> + page = pte_page(entry);
> + __free_page(page);
> + }
> +
> + spin_unlock(&init_mm.page_table_lock);
> +
> + return 0;
> +}
> +
> +/* Unmap all PAMT refcount pages and free vmalloc range */
> static void free_pamt_metadata(void)
> {
> + size_t size;
> +
> if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> return;
>
> + size = max_pfn / PTRS_PER_PTE * sizeof(*pamt_refcounts);
> + size = round_up(size, PAGE_SIZE);
> +
> + apply_to_existing_page_range(&init_mm,
> + (unsigned long)pamt_refcounts,
> + size, pamt_refcount_depopulate,
> + NULL);
> vfree(pamt_refcounts);
> pamt_refcounts = NULL;
> }
> @@ -288,10 +393,19 @@ static int build_tdx_memlist(struct list_head *tmb_list)
> ret = add_tdx_memblock(tmb_list, start_pfn, end_pfn, nid);
> if (ret)
> goto err;
> +
> + /* Allocated PAMT refcountes for the memblock */
> + ret = alloc_pamt_refcount(start_pfn, end_pfn);
> + if (ret)
> + goto err;
> }
>
> return 0;
> err:
> + /*
> + * Only free TDX memory blocks here, PAMT refcount pages
> + * will be freed in the init_tdx_module() error path.
> + */
> free_tdx_memlist(tmb_list);
> return ret;
> }
^ permalink raw reply
* Re: [PATCH 1/3] KVM: guest_memfd: Remove preparation tracking
From: Yan Zhao @ 2025-11-25 3:13 UTC (permalink / raw)
To: Michael Roth
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <20251121124314.36zlpzhwm5zglih2@amd.com>
On Fri, Nov 21, 2025 at 06:43:14AM -0600, Michael Roth wrote:
> On Thu, Nov 20, 2025 at 05:12:55PM +0800, Yan Zhao wrote:
> > On Thu, Nov 13, 2025 at 05:07:57PM -0600, Michael Roth wrote:
> > > @@ -797,19 +782,25 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> > > {
> > > pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > > struct folio *folio;
> > > - bool is_prepared = false;
> > > int r = 0;
> > >
> > > CLASS(gmem_get_file, file)(slot);
> > > if (!file)
> > > return -EFAULT;
> > >
> > > - folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
> > > + folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order);
> > > if (IS_ERR(folio))
> > > return PTR_ERR(folio);
> > >
> > > - if (!is_prepared)
> > > - r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> > > + if (!folio_test_uptodate(folio)) {
> > > + unsigned long i, nr_pages = folio_nr_pages(folio);
> > > +
> > > + for (i = 0; i < nr_pages; i++)
> > > + clear_highpage(folio_page(folio, i));
> > > + folio_mark_uptodate(folio);
> > Here, the entire folio is cleared only when the folio is not marked uptodate.
> > Then, please check my questions at the bottom
>
> Yes, in this patch at least where I tried to mirror the current logic. I
> would not be surprised if we need to rework things for inplace/hugepage
> support though, but decoupling 'preparation' from the uptodate flag is
> the main goal here.
Could you elaborate a little why the decoupling is needed if it's not for
hugepage?
> > > + }
> > > +
> > > + r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> > >
> > > folio_unlock(folio);
> > >
> > > @@ -852,7 +843,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > struct folio *folio;
> > > gfn_t gfn = start_gfn + i;
> > > pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > > - bool is_prepared = false;
> > > kvm_pfn_t pfn;
> > >
> > > if (signal_pending(current)) {
> > > @@ -860,19 +850,12 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > break;
> > > }
> > >
> > > - folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &is_prepared, &max_order);
> > > + folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
> > > if (IS_ERR(folio)) {
> > > ret = PTR_ERR(folio);
> > > break;
> > > }
> > >
> > > - if (is_prepared) {
> > > - folio_unlock(folio);
> > > - folio_put(folio);
> > > - ret = -EEXIST;
> > > - break;
> > > - }
> > > -
> > > folio_unlock(folio);
> > > WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > > (npages - i) < (1 << max_order));
> > TDX could hit this warning easily when npages == 1, max_order == 9.
>
> Yes, this will need to change to handle that. I don't think I had to
> change this for previous iterations of SNP hugepage support, but
> there are definitely cases where a sub-2M range might get populated
> even though it's backed by a 2M folio, so I'm not sure why I didn't
> hit it there.
>
> But I'm taking Sean's cue on touching as little of the existing
> hugepage logic as possible in this particular series so we can revisit
> the remaining changes with some better context.
Frankly, I don't understand why this patch 1 is required if we only want "moving
GUP out of post_populate()" to work for 4KB folios.
> >
> > > @@ -889,7 +872,7 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > p = src ? src + i * PAGE_SIZE : NULL;
> > > ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
> > > if (!ret)
> > > - kvm_gmem_mark_prepared(folio);
> > > + folio_mark_uptodate(folio);
> > As also asked in [1], why is the entire folio marked as uptodate here? Why does
> > kvm_gmem_get_pfn() clear all pages of a huge folio when the folio isn't marked
> > uptodate?
>
> Quoting your example from[1] for more context:
>
> > I also have a question about this patch:
> >
> > Suppose there's a 2MB huge folio A, where
> > A1 and A2 are 4KB pages belonging to folio A.
> >
> > (1) kvm_gmem_populate() invokes __kvm_gmem_get_pfn() and gets folio A.
> > It adds page A1 and invokes folio_mark_uptodate() on folio A.
>
> In SNP hugepage patchset you responded to, it would only mark A1 as
You mean code in
https://github.com/amdese/linux/commits/snp-inplace-conversion-rfc1 ?
> prepared/cleared. There was 4K-granularity tracking added to handle this.
I don't find the code that marks only A1 as "prepared/cleared".
Instead, I just found folio_mark_uptodate() is invoked by kvm_gmem_populate()
to mark the entire folio A as uptodate.
However, according to your statement below that "uptodate flag only tracks
whether a folio has been cleared", I don't follow why and where the entire folio
A would be cleared if kvm_gmem_populate() only adds page A1.
> There was an odd subtlety in that series though: it was defaulting to the
> folio_order() for the prep-tracking/post-populate, but it would then clamp
> it down based on the max order possible according whether that particular
> order was a homogenous range of KVM_MEMORY_ATTRIBUTE_PRIVATE. Which is not
> a great way to handle things, and I don't remember if I'd actually intended
> to implement it that way or not... that's probably why I never tripped over
> the WARN_ON() above, now that I think of it.
>
> But neither of these these apply to any current plans for hugepage support
> that I'm aware of, so probably not worth working through what that series
> did and look at this from a fresh perspective.
>
> >
> > (2) kvm_gmem_get_pfn() later faults in page A2.
> > As folio A is uptodate, clear_highpage() is not invoked on page A2.
> > kvm_gmem_prepare_folio() is invoked on the whole folio A.
> >
> > (2) could occur at least in TDX when only a part the 2MB page is added as guest
> > initial memory.
> >
> > My questions:
> > - Would (2) occur on SEV?
> > - If it does, is the lack of clear_highpage() on A2 a problem ?
> > - Is invoking gmem_prepare on page A1 a problem?
>
> Assuming this patch goes upstream in some form, we will now have the
> following major differences versus previous code:
>
> 1) uptodate flag only tracks whether a folio has been cleared
> 2) gmem always calls kvm_arch_gmem_prepare() via kvm_gmem_get_pfn() and
> the architecture can handle it's own tracking at whatever granularity
> it likes.
2) looks good to me.
> My hope is that 1) can similarly be done in such a way that gmem does not
> need to track things at sub-hugepage granularity and necessitate the need
> for some new data structure/state/flag to track sub-page status.
I actually don't understand what uptodate flag helps gmem to track.
Why can't clear_highpage() be done inside arch specific code? TDX doesn't need
this clearing after all.
> My understanding based on prior discussion in guest_memfd calls was that
> it would be okay to go ahead and clear the entire folio at initial allocation
> time, and basically never mess with it again. It was also my understanding
That's where I don't follow in this patch.
I don't see where the entire folio A is cleared if it's only partially mapped by
kvm_gmem_populate(). kvm_gmem_get_pfn() won't clear folio A either due to
kvm_gmem_populate() has set the uptodate flag.
> that for TDX it might even be optimal to completely skip clearing the folio
> if it is getting mapped into SecureEPT as a hugepage since the TDX module
> would handle that, but that maybe conversely after private->shared there
> would be some need to reclear... I'll try to find that discussion and
> refresh. Vishal I believe suggested some flags to provide more control over
> this behavior.
>
> >
> > It's possible (at least for TDX) that a huge folio is only partially populated
> > by kvm_gmem_populate(). Then kvm_gmem_get_pfn() faults in another part of the
> > huge folio. For example, in TDX, GFN 0x81f belongs to the init memory region,
> > while GFN 0x820 is faulted after TD is running. However, these two GFNs can
> > belong to the same folio of order 9.
>
> Would the above scheme of clearing the entire folio up front and not re-clearing
> at fault time work for this case?
This case doesn't affect TDX, because TDX clearing private pages internally in
SEAM APIs. So, as long as kvm_gmem_get_pfn() does not invoke clear_highpage()
after making a folio private, it works fine for TDX.
I was just trying to understand why SNP needs the clearing of entire folio in
kvm_gmem_get_pfn() while I don't see how the entire folio is cleared when it's
partially mapped in kvm_gmem_populate().
Also, I'm wondering if it would be better if SNP could move the clearing of
folio into something like kvm_arch_gmem_clear(), just as kvm_arch_gmem_prepare()
which is always invoked by kvm_gmem_get_pfn() and the architecture can handle
it's own tracking at whatever granularity.
> > Note: the current code should not impact TDX. I'm just asking out of curiosity:)
> >
> > [1] https://lore.kernel.org/all/aQ3uj4BZL6uFQzrD@yzhao56-desk.sh.intel.com/
> >
> >
^ permalink raw reply
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: Yan Zhao @ 2025-11-25 3:12 UTC (permalink / raw)
To: Ira Weiny
Cc: Michael Roth, kvm, linux-coco, linux-mm, linux-kernel,
thomas.lendacky, pbonzini, seanjc, vbabka, ashish.kalra,
liam.merwick, david, vannapurve, ackerleytng, aik
In-Reply-To: <69247f5fd9642_5cb63100e0@iweiny-mobl.notmuch>
On Mon, Nov 24, 2025 at 09:53:03AM -0600, Ira Weiny wrote:
> Yan Zhao wrote:
> > On Fri, Nov 21, 2025 at 07:01:44AM -0600, Michael Roth wrote:
> > > On Thu, Nov 20, 2025 at 05:11:48PM +0800, Yan Zhao wrote:
> > > > On Thu, Nov 13, 2025 at 05:07:59PM -0600, Michael Roth wrote:
>
> [snip]
>
> > > > > @@ -2284,14 +2285,21 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > goto err;
> > > > > }
> > > > >
> > > > > - if (src) {
> > > > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > > > + if (src_pages) {
> > > > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > > > >
> > > > > - if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> > > > > - ret = -EFAULT;
> > > > > - goto err;
> > > > > + memcpy(dst_vaddr, src_vaddr + src_offset, PAGE_SIZE - src_offset);
> > > > > + kunmap_local(src_vaddr);
> > > > > +
> > > > > + if (src_offset) {
> > > > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > > > +
> > > > > + memcpy(dst_vaddr + PAGE_SIZE - src_offset, src_vaddr, src_offset);
> > > > > + kunmap_local(src_vaddr);
> > > > IIUC, src_offset is the src's offset from the first page. e.g.,
> > > > src could be 0x7fea82684100, with src_offset=0x100, while npages could be 512.
> > > >
> > > > Then it looks like the two memcpy() calls here only work when npages == 1 ?
> > >
> > > src_offset ends up being the offset into the pair of src pages that we
> > > are using to fully populate a single dest page with each iteration. So
> > > if we start at src_offset, read a page worth of data, then we are now at
> > > src_offset in the next src page and the loop continues that way even if
> > > npages > 1.
> > >
> > > If src_offset is 0 we never have to bother with straddling 2 src pages so
> > > the 2nd memcpy is skipped on every iteration.
> > >
> > > That's the intent at least. Is there a flaw in the code/reasoning that I
> > > missed?
> > Oh, I got you. SNP expects a single src_offset applies for each src page.
> >
> > So if npages = 2, there're 4 memcpy() calls.
> >
> > src: |---------|---------|---------| (VA contiguous)
> > ^ ^ ^
> > | | |
> > dst: |---------|---------| (PA contiguous)
> >
>
> I'm not following the above diagram. Either src and dst are aligned and
Hmm, the src/dst legend in the above diagram just denotes source and target,
not the actual src user pointer.
> src_pages points to exactly one page. OR not aligned and src_pages points
> to 2 pages.
>
> src: |---------|---------| (VA contiguous)
> ^ ^
> | |
> dst: |---------| (PA contiguous)
>
> Regardless I think this is all bike shedding over a feature which I really
> don't think buys us much trying to allow the src to be missaligned.
>
> >
> > I previously incorrectly thought kvm_gmem_populate() should pass in src_offset
> > as 0 for the 2nd src page.
> >
> > Would you consider checking if params.uaddr is PAGE_ALIGNED() in
> > snp_launch_update() to simplify the design?
>
> I think this would help a lot... ATM I'm not even sure the algorithm
> works if order is not 0.
>
> [snip]
>
> >
> > > > Increasing GMEM_GUP_NPAGES to (1UL << PUD_ORDER) is probabaly not a good idea.
> > > >
> > > > Given both TDX/SNP map at 4KB granularity, why not just invoke post_populate()
> > > > per 4KB while removing the max_order from post_populate() parameters, as done
> > > > in Sean's sketch patch [1]?
> > >
> > > That's an option too, but SNP can make use of 2MB pages in the
> > > post-populate callback so I don't want to shut the door on that option
> > > just yet if it's not too much of a pain to work in. Given the guest BIOS
> > > lives primarily in 1 or 2 of these 2MB regions the benefits might be
> > > worthwhile, and SNP doesn't have a post-post-populate promotion path
> > > like TDX (at least, not one that would help much for guest boot times)
> > I see.
> >
> > So, what about below change?
>
> I'm not following what this change has to do with moving GUP out of the
> post_populate calls?
Without this change, TDX (and possibly SNP) would hit a warning when max_order>0.
(either GUP in 4KB granularity or this change can get rid of the warning).
Since this series already contains changes for 2MB pages (e.g., batched GUP to
allow SNP to map 2MB pages, and actually we don't need the change in patch 1
without considering huge pages), I don't see any reason to leave this change out
of tree.
Note: kvm_gmem_populate() already contains the logic of
while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
KVM_MEMORY_ATTRIBUTE_PRIVATE,
KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
if (!max_order)
goto put_folio_and_exit;
max_order--;
}
Also, the series is titled "Rework preparation/population flows in prep for
in-place conversion", so it's not just about "moving GUP out of the
post_populate", right? :)
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -878,11 +878,10 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > }
> >
> > folio_unlock(folio);
> > - WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > - (npages - i) < (1 << max_order));
> >
> > ret = -EINVAL;
> > - while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> > + while (!IS_ALIGNED(gfn, 1 << max_order) || (npages - i) < (1 << max_order) ||
> > + !kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> > KVM_MEMORY_ATTRIBUTE_PRIVATE,
> > KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> > if (!max_order)
> >
> >
> >
>
> [snip]
^ permalink raw reply
* Re: [PATCH v4 04/16] x86/virt/tdx: Allocate page bitmap for Dynamic PAMT
From: Binbin Wu @ 2025-11-25 1:50 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-5-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> The Physical Address Metadata Table (PAMT) holds TDX metadata for physical
> memory and must be allocated by the kernel during TDX module
> initialization.
>
> The exact size of the required PAMT memory is determined by the TDX module
> and may vary between TDX module versions. Currently it is approximately
> 0.4% of the system memory. This is a significant commitment, especially if
> it is not known upfront whether the machine will run any TDX guests.
>
> For normal PAMT, each memory region that the TDX module might use (TDMR)
> needs three separate PAMT allocations. One for each supported page size
> (1GB, 2MB, 4KB).
>
> At a high level, Dynamic PAMT still has the 1GB and 2MB levels allocated
> on TDX module initialization, but the 4KB level allocated dynamically at
> TD runtime. However, in the details, the TDX module still needs some per
> 4KB page data. The TDX module exposed how many bits per page need to be
> allocated (currently it is 1). The bits-per-page value can then be used to
> calculate the size to pass in place of the 4KB allocations in the TDMR,
> which TDX specs call "PAMT_PAGE_BITMAP".
>
> So in effect, Dynamic PAMT just needs a different (smaller) size
> allocation for the 4KB level part of the allocation. Although it is
> functionally something different, it is passed in the same way the 4KB page
> size PAMT allocation is.
>
> Begin to implement Dynamic PAMT in the kernel by reading the bits-per-page
> needed for Dynamic PAMT. Calculate the size needed for the bitmap,
> and use it instead of the 4KB size determined for normal PAMT, in the case
> of Dynamic PAMT. In doing so, reduce the static allocations to
> approximately 0.004%, a 100x improvement.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [Enhanced log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
One nit below.
[...]
> diff --git a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> index 13ad2663488b..00ab0e550636 100644
> --- a/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> +++ b/arch/x86/virt/vmx/tdx/tdx_global_metadata.c
> @@ -33,6 +33,13 @@ static int get_tdx_sys_info_tdmr(struct tdx_sys_info_tdmr *sysinfo_tdmr)
> sysinfo_tdmr->pamt_2m_entry_size = val;
> if (!ret && !(ret = read_sys_metadata_field(0x9100000100000012, &val)))
> sysinfo_tdmr->pamt_1g_entry_size = val;
> + /*
> + * Don't fail here if tdx_supports_dynamic_pamt() isn't supported. The
A bit weird to say "if tdx_supports_dynamic_pamt() isn't supported", how about
using "if dynamic PAMT isn't supported"?
> + * TDX code can fallback to normal PAMT if it's not supported.
> + */
> + if (!ret && tdx_supports_dynamic_pamt(&tdx_sysinfo) &&
> + !(ret = read_sys_metadata_field(0x9100000100000013, &val)))
> + sysinfo_tdmr->pamt_page_bitmap_entry_bits = val;
>
> return ret;
> }
^ permalink raw reply
* Re: [PATCH v4 03/16] x86/virt/tdx: Simplify tdmr_get_pamt_sz()
From: Binbin Wu @ 2025-11-25 1:27 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <f495fed769914a476bace0fd7eb58bebd933f6af.camel@intel.com>
On 11/25/2025 3:47 AM, Edgecombe, Rick P wrote:
> On Mon, 2025-11-24 at 17:26 +0800, Binbin Wu wrote:
>> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
> Thanks.
>
>> One nit below.
>>
>> [...]
>>> @@ -535,26 +518,18 @@ static int tdmr_set_up_pamt(struct tdmr_info *tdmr,
>>> * in overlapped TDMRs.
>>> */
>>> pamt = alloc_contig_pages(tdmr_pamt_size >> PAGE_SHIFT, GFP_KERNEL,
>>> - nid, &node_online_map);
>>> - if (!pamt)
>>> + nid, &node_online_map);
>>> + if (!pamt) {
>>> + /*
>>> + * tdmr->pamt_4k_base is zero so the
>>> + * error path will skip freeing.
>>> + */
>>> return -ENOMEM;
>> Nit:
>> Do you think it's OK to move the comment up so to avoid multiple lines of
>> comments as well as the curly braces?
> Yea, I think that is a good point. But I'm also thinking that this comment is
> not clear enough. There is no error path to speak of in this function, so maybe:
>
> /*
> * tdmr->pamt_4k_base is still zero so the error
> * path of the caller will skip freeing the pamt.
> */
>
> If you agree I will keep your RB.
Yes, please.
>> /* tdmr->pamt_4k_base is zero so the error path will skip freeing. */
>> if (!pamt)
>> return -ENOMEM;
^ permalink raw reply
* Re: [PATCH v4 00/16] TDX: Enable Dynamic PAMT
From: Sagi Shahar @ 2025-11-24 20:18 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-1-rick.p.edgecombe@intel.com>
On Thu, Nov 20, 2025 at 6:51 PM Rick Edgecombe
<rick.p.edgecombe@intel.com> wrote:
>
> Hi,
>
> This is 4th revision of Dynamic PAMT, which is a new feature that reduces
> the memory use of TDX. For background information, see the v3
> coverletter[0]. V4 mostly consists of incorporating the feedback from v3.
> Notably what it *doesn’t* change is the solution for pre-allocating DPAMT
> backing pages. (more info below in “Changes”)
>
> I think this series isn’t quite ready to ask for merging just yet. I’d
> appreciate another round of review especially looking for any issues in the
> refcount allocation/mapping and the pamt get/put lock-refcount dance. And
> hopefully collect some RBs.
>
> Sean/Paolo, we have mostly banished this all to TDX code except for “KVM:
> TDX: Add x86 ops for external spt cache” patch. That and “x86/virt/tdx:
> Add helpers to allow for pre-allocating pages” are probably the remaining
> possibly controversial parts of your domain. If you only have a short time
> to spend at this point, I’d point you at those two patches.
>
> Since most of the changes are in arch/x86, I’d think this feature could be
> a candidate for eventually merging through tip with Sean or Paolo’s ack.
> But it is currently based on kvm-x86/next in order to build on top of the
> post-populate cleanup series. Next time I’ll probably target tip if people
> think that is a good way forward.
>
> Changes
> =======
> There were two good suggestions around the pre-allocated pages solution
> last time, but neither ended up working out:
>
> 1. Dave suggested to use mempool_t instead of the linked list based
> structure, in order to not re-invent the wheel. This turned out to not
> quite fit. The problems were that there wasn’t really a “topup” mechanism,
> or an atomic fallback (which matches the kvm cache behavior). This results
> in very similar code being built around mempool that was built around the
> linked list. It was an overall harder to follow solution for not much code
> savings.
>
> I strongly considered going back to Kiryl’s original solution which passed
> a callback function pointer for allocating DPAMT pages, and an opaque
> void * that the callback could use to find the kvm_mmu_memory_cache. I
> thought that readability issues of passing the opaque void * between
> subsystems outweighed the small code duplication in the simple, familiar
> patterned linked list-based code. So I ended up leaving it.
>
> 2. Kai suggested (but later retracted the idea) that since the external
> page table cache was moved to TDX code, it could simply install DPAMT
> pages for the cache at topup time. Then the installation of DPAMT backing
> for S-EPT page tables could be done outside of the mmu_lock. It could also
> be argued that it makes the design simpler in a way, because the external
> page table cache acts like it did before. Anything in there could be simply
> used.
>
> At the time my argument against this was that whether a huge page would be
> installed (and thus, whether DPAMT backing was needed) for the guest
> private memory would not be known until later, so early install solution
> would need special late handling for TDX huge pages. After some internal
> discussions I at looked how we could simplify the series by punting on TDX
> huge pages needs.
>
> But it turns out that this other design was actually more complex and had
> more LOC than the previous solution. So it was dropped, and again, I went
> back to the original solution.
>
>
> I’m really starting to think that, while the overall solution here isn’t
> the most elegant, we might not have much more to squeeze from it. So
> design-wise, I think we should think about calling it done.
>
> Testing
> =======
> Based on kvm-x86/next (4531ff85d925). Testing was the usual, except I also
> tested with TDX modules that don't support DPAMT, and with the two
> optimization patches removed: “Improve PAMT refcounters allocation for
> sparse memory” and “x86/virt/tdx: Optimize tdx_alloc/free_page() helpers”.
>
> [0] https://lore.kernel.org/kvm/20250918232224.2202592-1-rick.p.edgecombe@intel.com/
>
>
> Kirill A. Shutemov (13):
> x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
> x86/tdx: Add helpers to check return status codes
> x86/virt/tdx: Allocate page bitmap for Dynamic PAMT
> x86/virt/tdx: Allocate reference counters for PAMT memory
> x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
> x86/virt/tdx: Add tdx_alloc/free_page() helpers
> x86/virt/tdx: Optimize tdx_alloc/free_page() helpers
> KVM: TDX: Allocate PAMT memory for TD control structures
> KVM: TDX: Allocate PAMT memory for vCPU control structures
> KVM: TDX: Handle PAMT allocation in fault path
> KVM: TDX: Reclaim PAMT memory
> x86/virt/tdx: Enable Dynamic PAMT
> Documentation/x86: Add documentation for TDX's Dynamic PAMT
>
> Rick Edgecombe (3):
> x86/virt/tdx: Simplify tdmr_get_pamt_sz()
> KVM: TDX: Add x86 ops for external spt cache
> x86/virt/tdx: Add helpers to allow for pre-allocating pages
>
> Documentation/arch/x86/tdx.rst | 21 +
> arch/x86/coco/tdx/tdx.c | 10 +-
> arch/x86/include/asm/kvm-x86-ops.h | 3 +
> arch/x86/include/asm/kvm_host.h | 14 +-
> arch/x86/include/asm/shared/tdx.h | 8 +
> arch/x86/include/asm/shared/tdx_errno.h | 104 ++++
> arch/x86/include/asm/tdx.h | 78 ++-
> arch/x86/include/asm/tdx_global_metadata.h | 1 +
> arch/x86/kvm/mmu/mmu.c | 6 +-
> arch/x86/kvm/mmu/mmu_internal.h | 2 +-
> arch/x86/kvm/vmx/tdx.c | 160 ++++--
> arch/x86/kvm/vmx/tdx.h | 3 +-
> arch/x86/kvm/vmx/tdx_errno.h | 40 --
> arch/x86/virt/vmx/tdx/tdx.c | 587 +++++++++++++++++---
> arch/x86/virt/vmx/tdx/tdx.h | 5 +-
> arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 7 +
> 16 files changed, 854 insertions(+), 195 deletions(-)
> create mode 100644 arch/x86/include/asm/shared/tdx_errno.h
> delete mode 100644 arch/x86/kvm/vmx/tdx_errno.h
>
> --
> 2.51.2
>
>
Tested-by: Sagi Shahar <sagis@google.com>
^ permalink raw reply
* Re: [PATCH v4 03/16] x86/virt/tdx: Simplify tdmr_get_pamt_sz()
From: Edgecombe, Rick P @ 2025-11-24 19:47 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <8eba534b-7fcf-43b2-a304-091993faef1c@linux.intel.com>
On Mon, 2025-11-24 at 17:26 +0800, Binbin Wu wrote:
> Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
Thanks.
>
> One nit below.
>
> [...]
> > @@ -535,26 +518,18 @@ static int tdmr_set_up_pamt(struct tdmr_info *tdmr,
> > * in overlapped TDMRs.
> > */
> > pamt = alloc_contig_pages(tdmr_pamt_size >> PAGE_SHIFT, GFP_KERNEL,
> > - nid, &node_online_map);
> > - if (!pamt)
> > + nid, &node_online_map);
> > + if (!pamt) {
> > + /*
> > + * tdmr->pamt_4k_base is zero so the
> > + * error path will skip freeing.
> > + */
> > return -ENOMEM;
> Nit:
> Do you think it's OK to move the comment up so to avoid multiple lines of
> comments as well as the curly braces?
Yea, I think that is a good point. But I'm also thinking that this comment is
not clear enough. There is no error path to speak of in this function, so maybe:
/*
* tdmr->pamt_4k_base is still zero so the error
* path of the caller will skip freeing the pamt.
*/
If you agree I will keep your RB.
>
> /* tdmr->pamt_4k_base is zero so the error path will skip freeing. */
> if (!pamt)
> return -ENOMEM;
^ permalink raw reply
* Re: [PATCH v4 02/16] x86/tdx: Add helpers to check return status codes
From: Edgecombe, Rick P @ 2025-11-24 19:31 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, kirill.shutemov@linux.intel.com,
tglx@linutronix.de, Annapurve, Vishal, Gao, Chao, bp@alien8.de,
x86@kernel.org
In-Reply-To: <86e2d5fa-4d68-4200-98d1-77113bc3c1da@linux.intel.com>
On Mon, 2025-11-24 at 16:56 +0800, Binbin Wu wrote:
> > +static inline u64 TDX_STATUS(u64 err)
> > +{
> > + return err & TDX_STATUS_MASK;
> > +}
>
> Should be tagged with __always_inline since it's used in noinstr range.
Nice, catch. Yes both should be done.
^ permalink raw reply
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: Ira Weiny @ 2025-11-24 15:53 UTC (permalink / raw)
To: Yan Zhao, Michael Roth
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <aSQmAuxGK7+MUfRW@yzhao56-desk.sh.intel.com>
Yan Zhao wrote:
> On Fri, Nov 21, 2025 at 07:01:44AM -0600, Michael Roth wrote:
> > On Thu, Nov 20, 2025 at 05:11:48PM +0800, Yan Zhao wrote:
> > > On Thu, Nov 13, 2025 at 05:07:59PM -0600, Michael Roth wrote:
[snip]
> > > > @@ -2284,14 +2285,21 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > goto err;
> > > > }
> > > >
> > > > - if (src) {
> > > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > > + if (src_pages) {
> > > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > > >
> > > > - if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> > > > - ret = -EFAULT;
> > > > - goto err;
> > > > + memcpy(dst_vaddr, src_vaddr + src_offset, PAGE_SIZE - src_offset);
> > > > + kunmap_local(src_vaddr);
> > > > +
> > > > + if (src_offset) {
> > > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > > +
> > > > + memcpy(dst_vaddr + PAGE_SIZE - src_offset, src_vaddr, src_offset);
> > > > + kunmap_local(src_vaddr);
> > > IIUC, src_offset is the src's offset from the first page. e.g.,
> > > src could be 0x7fea82684100, with src_offset=0x100, while npages could be 512.
> > >
> > > Then it looks like the two memcpy() calls here only work when npages == 1 ?
> >
> > src_offset ends up being the offset into the pair of src pages that we
> > are using to fully populate a single dest page with each iteration. So
> > if we start at src_offset, read a page worth of data, then we are now at
> > src_offset in the next src page and the loop continues that way even if
> > npages > 1.
> >
> > If src_offset is 0 we never have to bother with straddling 2 src pages so
> > the 2nd memcpy is skipped on every iteration.
> >
> > That's the intent at least. Is there a flaw in the code/reasoning that I
> > missed?
> Oh, I got you. SNP expects a single src_offset applies for each src page.
>
> So if npages = 2, there're 4 memcpy() calls.
>
> src: |---------|---------|---------| (VA contiguous)
> ^ ^ ^
> | | |
> dst: |---------|---------| (PA contiguous)
>
I'm not following the above diagram. Either src and dst are aligned and
src_pages points to exactly one page. OR not aligned and src_pages points
to 2 pages.
src: |---------|---------| (VA contiguous)
^ ^
| |
dst: |---------| (PA contiguous)
Regardless I think this is all bike shedding over a feature which I really
don't think buys us much trying to allow the src to be missaligned.
>
> I previously incorrectly thought kvm_gmem_populate() should pass in src_offset
> as 0 for the 2nd src page.
>
> Would you consider checking if params.uaddr is PAGE_ALIGNED() in
> snp_launch_update() to simplify the design?
I think this would help a lot... ATM I'm not even sure the algorithm
works if order is not 0.
[snip]
>
> > > Increasing GMEM_GUP_NPAGES to (1UL << PUD_ORDER) is probabaly not a good idea.
> > >
> > > Given both TDX/SNP map at 4KB granularity, why not just invoke post_populate()
> > > per 4KB while removing the max_order from post_populate() parameters, as done
> > > in Sean's sketch patch [1]?
> >
> > That's an option too, but SNP can make use of 2MB pages in the
> > post-populate callback so I don't want to shut the door on that option
> > just yet if it's not too much of a pain to work in. Given the guest BIOS
> > lives primarily in 1 or 2 of these 2MB regions the benefits might be
> > worthwhile, and SNP doesn't have a post-post-populate promotion path
> > like TDX (at least, not one that would help much for guest boot times)
> I see.
>
> So, what about below change?
I'm not following what this change has to do with moving GUP out of the
post_populate calls?
Ira
>
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -878,11 +878,10 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> }
>
> folio_unlock(folio);
> - WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> - (npages - i) < (1 << max_order));
>
> ret = -EINVAL;
> - while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> + while (!IS_ALIGNED(gfn, 1 << max_order) || (npages - i) < (1 << max_order) ||
> + !kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> KVM_MEMORY_ATTRIBUTE_PRIVATE,
> KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> if (!max_order)
>
>
>
[snip]
^ permalink raw reply
* Re: [REGRESSION] GHES firmware can't be readonly - Was: Re: [PATCH v3 3/3] arm64: acpi: Enable ACPI CCEL support
From: Will Deacon @ 2025-11-24 13:14 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Mauro Carvalho Chehab, Jonathan Cameron, linux-arm-kernel,
linux-kernel, linux-coco, catalin.marinas, gshan, aneesh.kumar,
sami.mujawar, sudeep.holla, steven.price, regressions
In-Reply-To: <4a750da6-7883-4afa-94c1-4806677e61c2@arm.com>
On Mon, Nov 24, 2025 at 05:21:00AM +0000, Suzuki K Poulose wrote:
> On 21/11/2025 21:46, Mauro Carvalho Chehab wrote:
> > Hi,
> >
> > Em Thu, 18 Sep 2025 13:56:18 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> escreveu:
> >
> > > Add support for ACPI CCEL by handling the EfiACPIMemoryNVS type memory.
> > > As per UEFI specifications NVS memory is reserved for Firmware use even
> > > after exiting boot services. Thus map the region as read-only.
> > >
> > > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
> > > Cc: Steven Price <steven.price@arm.com>
> > > Cc: Sudeep Holla <sudeep.holla@arm.com>
> > > Cc: Gavin Shan <gshan@redhat.com>
> > > Reviewed-by: Gavin Shan <gshan@redhat.com>
> > > Tested-by: Sami Mujawar <sami.mujawar@arm.com>
> > > Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > ---
> > > arch/arm64/kernel/acpi.c | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> > > index 4d529ff7ba51..b3195b3b895f 100644
> > > --- a/arch/arm64/kernel/acpi.c
> > > +++ b/arch/arm64/kernel/acpi.c
> > > @@ -357,6 +357,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> > > * as long as we take care not to create a writable
> > > * mapping for executable code.
> > > */
> > > + fallthrough;
> > > +
> > > + case EFI_ACPI_MEMORY_NVS:
> > > + /*
> > > + * ACPI NVS marks an area reserved for use by the
> > > + * firmware, even after exiting the boot service.
> > > + * This may be used by the firmware for sharing dynamic
> > > + * tables/data (e.g., ACPI CCEL) with the OS. Map it
> > > + * as read-only.
> > > + */
> > > prot = PAGE_KERNEL_RO;
> >
> > Please revert this change.
> >
> > Making area reserved to be used by firmware breaks some APEI
> > notification mechanisms:
>
> Thanks for the report. Clearly, we missed this case. I am happy for this
> patch to be reverted and we can work out the handling of NVS later.
I'll revert the change shortly.
Will
^ permalink raw reply
* Re: [PATCH v1 08/26] x86/virt/tdx: Add tdx_enable_ext() to enable of TDX Module Extensions
From: Xu Yilun @ 2025-11-24 10:52 UTC (permalink / raw)
To: Dave Hansen
Cc: linux-coco, linux-pci, chao.gao, dave.jiang, baolu.lu, yilun.xu,
zhenzhong.duan, kvm, rick.p.edgecombe, dave.hansen,
dan.j.williams, kas, x86
In-Reply-To: <ca331aa3-6304-4e07-9ed9-94dc69726382@intel.com>
> A few more nits, though. Please don't talk about things in terms of
> number of pages. Just give the usage in megabytes.
Yes.
...
> > -#define TDH_SYS_CONFIG 45
> > +#define TDH_SYS_CONFIG (45 | (1ULL << TDX_VERSION_SHIFT))
>
> That's my theory: we don't need to keep versions.
Good to know.
...
> > The TDX Module can only accept one root page (i.e. 512 HPAs at most), while
> > struct tdx_page_array contains the whole EXT memory (12800 pages). So we
> > can't populate all pages into one root page then tell TDX Module. We need to
> > populate one batch, tell tdx module, then populate the next batch, tell
> > tdx module...
>
> That is, indeed, the information that I was looking for. Can you please
> ensure that makes it into code comments?
Yes.
^ permalink raw reply
* Re: [PATCH v1 08/26] x86/virt/tdx: Add tdx_enable_ext() to enable of TDX Module Extensions
From: Xu Yilun @ 2025-11-24 10:41 UTC (permalink / raw)
To: Dave Hansen, linux-mm
Cc: linux-coco, linux-pci, chao.gao, dave.jiang, baolu.lu, yilun.xu,
zhenzhong.duan, kvm, rick.p.edgecombe, dave.hansen,
dan.j.williams, kas, x86, akpm
In-Reply-To: <167d9540-2d9a-4367-bc68-b96494bc4044@intel.com>
On Fri, Nov 21, 2025 at 07:38:03AM -0800, Dave Hansen wrote:
> On 11/21/25 07:15, Dave Hansen wrote:
> > On 11/21/25 04:54, Xu Yilun wrote:
> > ...
> >> For now, TDX Module Extensions consume quite large amount of memory
> >> (12800 pages), print this readout value on TDX Module Extentions
> >> initialization.
> > Overall, the description is looking better, thanks!
> >
> > A few more nits, though. Please don't talk about things in terms of
> > number of pages. Just give the usage in megabytes.
>
> Oh, and please at least have a discussion with the memory management
> folks about consuming this amount of memory forever. I think it's quite
> possible they will prefer it be allocated in a way other than thousands
> of plain old allocations.
>
> For example, imagine memory was fragmented and those 12800 pages came
> from 12,800 different 2M regions. Well, now you've got ~50GB of memory
> that is _permanently_ fragmented and will never be able to satisfy a 2M
> allocation.
>
> You might get an answer that it's better to do a small number of
> max-size buddy allocations than a large number of PAGE_SIZE allocations.
Loop in mm folks.
Hi mm folks, for Intel TDX (Trust Domain Extensions) feature, there is
a requirement to donate quite a number of pages (12800 x 4K = 50MB for
now) to TDX firmware (known as TDX Module) for its initialization. These
pages will never be revoked cause the TDX Module initialization is a one
way path.
The TDX Module doesn't require these pages be physically contiguous, and
the patches [1][2] in this series [3] does PAGE_SIZE allocation. But as
mentioned by Dave, the donation may _permanently_ fragment regions, stop
them from 2M huge page allocation. In worst case, 12800 x 2MB = 25GB
memory region.
So is order based buddy allocation a better choice? I believe so. And if
that fails, should we fall back to PAGE_SIZE allocation? Or PAGE_SIZE
allocation should be a hard no in this _permanent_ donation case?
[1]: https://lore.kernel.org/linux-coco/20251117022311.2443900-7-yilun.xu@linux.intel.com/
[2]: https://lore.kernel.org/linux-coco/20251117022311.2443900-9-yilun.xu@linux.intel.com/
[3]: https://lore.kernel.org/linux-coco/20251117022311.2443900-1-yilun.xu@linux.intel.com/
Thanks,
Yilun
^ permalink raw reply
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: Yan Zhao @ 2025-11-24 9:31 UTC (permalink / raw)
To: Michael Roth
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <20251121130144.u7eeaafonhcqf2bd@amd.com>
On Fri, Nov 21, 2025 at 07:01:44AM -0600, Michael Roth wrote:
> On Thu, Nov 20, 2025 at 05:11:48PM +0800, Yan Zhao wrote:
> > On Thu, Nov 13, 2025 at 05:07:59PM -0600, Michael Roth wrote:
> > > Currently the post-populate callbacks handle copying source pages into
> > > private GPA ranges backed by guest_memfd, where kvm_gmem_populate()
> > > acquires the filemap invalidate lock, then calls a post-populate
> > > callback which may issue a get_user_pages() on the source pages prior to
> > > copying them into the private GPA (e.g. TDX).
> > >
> > > This will not be compatible with in-place conversion, where the
> > > userspace page fault path will attempt to acquire filemap invalidate
> > > lock while holding the mm->mmap_lock, leading to a potential ABBA
> > > deadlock[1].
> > >
> > > Address this by hoisting the GUP above the filemap invalidate lock so
> > > that these page faults path can be taken early, prior to acquiring the
> > > filemap invalidate lock.
> > >
> > > It's not currently clear whether this issue is reachable with the
> > > current implementation of guest_memfd, which doesn't support in-place
> > > conversion, however it does provide a consistent mechanism to provide
> > > stable source/target PFNs to callbacks rather than punting to
> > > vendor-specific code, which allows for more commonality across
> > > architectures, which may be worthwhile even without in-place conversion.
> > >
> > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > > ---
> > > arch/x86/kvm/svm/sev.c | 40 ++++++++++++++++++++++++++------------
> > > arch/x86/kvm/vmx/tdx.c | 21 +++++---------------
> > > include/linux/kvm_host.h | 3 ++-
> > > virt/kvm/guest_memfd.c | 42 ++++++++++++++++++++++++++++++++++------
> > > 4 files changed, 71 insertions(+), 35 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > index 0835c664fbfd..d0ac710697a2 100644
> > > --- a/arch/x86/kvm/svm/sev.c
> > > +++ b/arch/x86/kvm/svm/sev.c
> > > @@ -2260,7 +2260,8 @@ struct sev_gmem_populate_args {
> > > };
> > >
> > > static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pfn,
> > > - void __user *src, int order, void *opaque)
> > > + struct page **src_pages, loff_t src_offset,
> > > + int order, void *opaque)
> > > {
> > > struct sev_gmem_populate_args *sev_populate_args = opaque;
> > > struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> > > @@ -2268,7 +2269,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > int npages = (1 << order);
> > > gfn_t gfn;
> > >
> > > - if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src))
> > > + if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_pages))
> > > return -EINVAL;
> > >
> > > for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
> > > @@ -2284,14 +2285,21 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > goto err;
> > > }
> > >
> > > - if (src) {
> > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > + if (src_pages) {
> > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > >
> > > - if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> > > - ret = -EFAULT;
> > > - goto err;
> > > + memcpy(dst_vaddr, src_vaddr + src_offset, PAGE_SIZE - src_offset);
> > > + kunmap_local(src_vaddr);
> > > +
> > > + if (src_offset) {
> > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > +
> > > + memcpy(dst_vaddr + PAGE_SIZE - src_offset, src_vaddr, src_offset);
> > > + kunmap_local(src_vaddr);
> > IIUC, src_offset is the src's offset from the first page. e.g.,
> > src could be 0x7fea82684100, with src_offset=0x100, while npages could be 512.
> >
> > Then it looks like the two memcpy() calls here only work when npages == 1 ?
>
> src_offset ends up being the offset into the pair of src pages that we
> are using to fully populate a single dest page with each iteration. So
> if we start at src_offset, read a page worth of data, then we are now at
> src_offset in the next src page and the loop continues that way even if
> npages > 1.
>
> If src_offset is 0 we never have to bother with straddling 2 src pages so
> the 2nd memcpy is skipped on every iteration.
>
> That's the intent at least. Is there a flaw in the code/reasoning that I
> missed?
Oh, I got you. SNP expects a single src_offset applies for each src page.
So if npages = 2, there're 4 memcpy() calls.
src: |---------|---------|---------| (VA contiguous)
^ ^ ^
| | |
dst: |---------|---------| (PA contiguous)
I previously incorrectly thought kvm_gmem_populate() should pass in src_offset
as 0 for the 2nd src page.
Would you consider checking if params.uaddr is PAGE_ALIGNED() in
snp_launch_update() to simplify the design?
> >
> > > }
> > > - kunmap_local(vaddr);
> > > +
> > > + kunmap_local(dst_vaddr);
> > > }
> > >
> > > ret = rmp_make_private(pfn + i, gfn << PAGE_SHIFT, PG_LEVEL_4K,
> > > @@ -2331,12 +2339,20 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > if (!snp_page_reclaim(kvm, pfn + i) &&
> > > sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
> > > sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
> > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > >
> > > - if (copy_to_user(src + i * PAGE_SIZE, vaddr, PAGE_SIZE))
> > > - pr_debug("Failed to write CPUID page back to userspace\n");
> > > + memcpy(src_vaddr + src_offset, dst_vaddr, PAGE_SIZE - src_offset);
> > > + kunmap_local(src_vaddr);
> > > +
> > > + if (src_offset) {
> > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > +
> > > + memcpy(src_vaddr, dst_vaddr + PAGE_SIZE - src_offset, src_offset);
> > > + kunmap_local(src_vaddr);
> > > + }
> > >
> > > - kunmap_local(vaddr);
> > > + kunmap_local(dst_vaddr);
> > > }
> > >
> > > /* pfn + i is hypervisor-owned now, so skip below cleanup for it. */
> > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > index 57ed101a1181..dd5439ec1473 100644
> > > --- a/arch/x86/kvm/vmx/tdx.c
> > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > @@ -3115,37 +3115,26 @@ struct tdx_gmem_post_populate_arg {
> > > };
> > >
> > > static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > - void __user *src, int order, void *_arg)
> > > + struct page **src_pages, loff_t src_offset,
> > > + int order, void *_arg)
> > > {
> > > struct tdx_gmem_post_populate_arg *arg = _arg;
> > > struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > > u64 err, entry, level_state;
> > > gpa_t gpa = gfn_to_gpa(gfn);
> > > - struct page *src_page;
> > > int ret, i;
> > >
> > > if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm))
> > > return -EIO;
> > >
> > > - if (KVM_BUG_ON(!PAGE_ALIGNED(src), kvm))
> > > + /* Source should be page-aligned, in which case src_offset will be 0. */
> > > + if (KVM_BUG_ON(src_offset))
> > if (KVM_BUG_ON(src_offset, kvm))
> >
> > > return -EINVAL;
> > >
> > > - /*
> > > - * Get the source page if it has been faulted in. Return failure if the
> > > - * source page has been swapped out or unmapped in primary memory.
> > > - */
> > > - ret = get_user_pages_fast((unsigned long)src, 1, 0, &src_page);
> > > - if (ret < 0)
> > > - return ret;
> > > - if (ret != 1)
> > > - return -ENOMEM;
> > > -
> > > - kvm_tdx->page_add_src = src_page;
> > > + kvm_tdx->page_add_src = src_pages[i];
> > src_pages[0] ? i is not initialized.
>
> Sorry, I switched on TDX options for compile testing but I must have done a
> sloppy job confirming it actually built. I'll re-test push these and squash
> in the fixes in the github tree.
>
> >
> > Should there also be a KVM_BUG_ON(order > 0, kvm) ?
>
> Seems reasonable, but I'm not sure this is the right patch. Maybe I
> could squash it into the preceeding documentation patch so as to not
> give the impression this patch changes those expectations in any way.
I don't think it should be documented as a user requirement.
However, we need to comment out that this assertion is due to that
tdx_vcpu_init_mem_region() passes npages as 1 to kvm_gmem_populate().
> >
> > > ret = kvm_tdp_mmu_map_private_pfn(arg->vcpu, gfn, pfn);
> > > kvm_tdx->page_add_src = NULL;
> > >
> > > - put_page(src_page);
> > > -
> > > if (ret || !(arg->flags & KVM_TDX_MEASURE_MEMORY_REGION))
> > > return ret;
> > >
> > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > index d93f75b05ae2..7e9d2403c61f 100644
> > > --- a/include/linux/kvm_host.h
> > > +++ b/include/linux/kvm_host.h
> > > @@ -2581,7 +2581,8 @@ int kvm_arch_gmem_prepare(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int max_ord
> > > * Returns the number of pages that were populated.
> > > */
> > > typedef int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > - void __user *src, int order, void *opaque);
> > > + struct page **src_pages, loff_t src_offset,
> > > + int order, void *opaque);
> > >
> > > long kvm_gmem_populate(struct kvm *kvm, gfn_t gfn, void __user *src, long npages,
> > > kvm_gmem_populate_cb post_populate, void *opaque);
> > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > index 9160379df378..e9ac3fd4fd8f 100644
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -814,14 +814,17 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> > > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
> > >
> > > #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE
> > > +
> > > +#define GMEM_GUP_NPAGES (1UL << PMD_ORDER)
> > Limiting GMEM_GUP_NPAGES to PMD_ORDER may only work when the max_order of a huge
> > folio is 2MB. What if the max_order returned from __kvm_gmem_get_pfn() is 1GB
> > when src_pages[] can only hold up to 512 pages?
>
> This was necessarily chosen in prep for hugepages, but more about my
> unease at letting userspace GUP arbitrarilly large ranges. PMD_ORDER
> happens to align with 2MB hugepages while seeming like a reasonable
> batching value so that's why I chose it.
>
> Even with 1GB support, I wasn't really planning to increase it. SNP
> doesn't really make use of RMP sizes >2MB, and it sounds like TDX
> handles promotion in a completely different path. So atm I'm leaning
> toward just letting GMEM_GUP_NPAGES be the cap for the max page size we
> support for kvm_gmem_populate() path and not bothering to change it
> until a solid use-case arises.
The problem is that with hugetlb-based guest_memfd, the folio itself could be
of 1GB, though SNP and TDX can force mapping at only 4KB.
Then since max_order = folio_order(folio) (at least in the tree for [1]),
WARN_ON() in kvm_gmem_populate() could still be hit:
folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
(npages - i) < (1 << max_order));
TDX is even easier to hit this warning because it always passes npages as 1.
[1] https://lore.kernel.org/all/cover.1747264138.git.ackerleytng@google.com
> > Increasing GMEM_GUP_NPAGES to (1UL << PUD_ORDER) is probabaly not a good idea.
> >
> > Given both TDX/SNP map at 4KB granularity, why not just invoke post_populate()
> > per 4KB while removing the max_order from post_populate() parameters, as done
> > in Sean's sketch patch [1]?
>
> That's an option too, but SNP can make use of 2MB pages in the
> post-populate callback so I don't want to shut the door on that option
> just yet if it's not too much of a pain to work in. Given the guest BIOS
> lives primarily in 1 or 2 of these 2MB regions the benefits might be
> worthwhile, and SNP doesn't have a post-post-populate promotion path
> like TDX (at least, not one that would help much for guest boot times)
I see.
So, what about below change?
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -878,11 +878,10 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
}
folio_unlock(folio);
- WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
- (npages - i) < (1 << max_order));
ret = -EINVAL;
- while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
+ while (!IS_ALIGNED(gfn, 1 << max_order) || (npages - i) < (1 << max_order) ||
+ !kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
KVM_MEMORY_ATTRIBUTE_PRIVATE,
KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
if (!max_order)
>
> >
> > Then the WARN_ON() in kvm_gmem_populate() can be removed, which would be easily
> > triggered by TDX when max_order > 0 && npages == 1:
> >
> > WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > (npages - i) < (1 << max_order));
> >
> >
> > [1] https://lore.kernel.org/all/aHEwT4X0RcfZzHlt@google.com/
> >
> > > long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
> > > kvm_gmem_populate_cb post_populate, void *opaque)
> > > {
> > > struct kvm_memory_slot *slot;
> > > - void __user *p;
> > > -
> > > + struct page **src_pages;
> > > int ret = 0, max_order;
> > > - long i;
> > > + loff_t src_offset = 0;
> > > + long i, src_npages;
> > >
> > > lockdep_assert_held(&kvm->slots_lock);
> > >
> > > @@ -836,9 +839,28 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > if (!file)
> > > return -EFAULT;
> > >
> > > + npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> > > + npages = min_t(ulong, npages, GMEM_GUP_NPAGES);
> > > +
> > > + if (src) {
> > > + src_npages = IS_ALIGNED((unsigned long)src, PAGE_SIZE) ? npages : npages + 1;
> > > +
> > > + src_pages = kmalloc_array(src_npages, sizeof(struct page *), GFP_KERNEL);
> > > + if (!src_pages)
> > > + return -ENOMEM;
> > > +
> > > + ret = get_user_pages_fast((unsigned long)src, src_npages, 0, src_pages);
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + if (ret != src_npages)
> > > + return -ENOMEM;
> > > +
> > > + src_offset = (loff_t)(src - PTR_ALIGN_DOWN(src, PAGE_SIZE));
> > > + }
> > > +
> > > filemap_invalidate_lock(file->f_mapping);
> > >
> > > - npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> > > for (i = 0; i < npages; i += (1 << max_order)) {
> > > struct folio *folio;
> > > gfn_t gfn = start_gfn + i;
> > > @@ -869,8 +891,8 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > max_order--;
> > > }
> > >
> > > - p = src ? src + i * PAGE_SIZE : NULL;
> > > - ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
> > > + ret = post_populate(kvm, gfn, pfn, src ? &src_pages[i] : NULL,
> > > + src_offset, max_order, opaque);
> > Why src_offset is not 0 starting from the 2nd page?
> >
> > > if (!ret)
> > > folio_mark_uptodate(folio);
> > >
> > > @@ -882,6 +904,14 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > >
> > > filemap_invalidate_unlock(file->f_mapping);
> > >
> > > + if (src) {
> > > + long j;
> > > +
> > > + for (j = 0; j < src_npages; j++)
> > > + put_page(src_pages[j]);
> > > + kfree(src_pages);
> > > + }
> > > +
> > > return ret && !i ? ret : i;
> > > }
> > > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);
> > > --
> > > 2.25.1
> > >
^ permalink raw reply
* Re: [REGRESSION] GHES firmware can't be readonly - Was: Re: [PATCH v3 3/3] arm64: acpi: Enable ACPI CCEL support
From: Mauro Carvalho Chehab @ 2025-11-24 9:33 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Jonathan Cameron, linux-arm-kernel, linux-kernel, linux-coco,
catalin.marinas, will, gshan, aneesh.kumar, sami.mujawar,
sudeep.holla, steven.price, regressions
In-Reply-To: <4a750da6-7883-4afa-94c1-4806677e61c2@arm.com>
Em Mon, 24 Nov 2025 05:21:00 +0000
Suzuki K Poulose <suzuki.poulose@arm.com> escreveu:
> On 21/11/2025 21:46, Mauro Carvalho Chehab wrote:
> > Hi,
> >
> > Em Thu, 18 Sep 2025 13:56:18 +0100
> > Suzuki K Poulose <suzuki.poulose@arm.com> escreveu:
> >
> >> Add support for ACPI CCEL by handling the EfiACPIMemoryNVS type memory.
> >> As per UEFI specifications NVS memory is reserved for Firmware use even
> >> after exiting boot services. Thus map the region as read-only.
> >>
> >> Cc: Sami Mujawar <sami.mujawar@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Aneesh Kumar K.V <aneesh.kumar@kernel.org>
> >> Cc: Steven Price <steven.price@arm.com>
> >> Cc: Sudeep Holla <sudeep.holla@arm.com>
> >> Cc: Gavin Shan <gshan@redhat.com>
> >> Reviewed-by: Gavin Shan <gshan@redhat.com>
> >> Tested-by: Sami Mujawar <sami.mujawar@arm.com>
> >> Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >> ---
> >> arch/arm64/kernel/acpi.c | 10 ++++++++++
> >> 1 file changed, 10 insertions(+)
> >>
> >> diff --git a/arch/arm64/kernel/acpi.c b/arch/arm64/kernel/acpi.c
> >> index 4d529ff7ba51..b3195b3b895f 100644
> >> --- a/arch/arm64/kernel/acpi.c
> >> +++ b/arch/arm64/kernel/acpi.c
> >> @@ -357,6 +357,16 @@ void __iomem *acpi_os_ioremap(acpi_physical_address phys, acpi_size size)
> >> * as long as we take care not to create a writable
> >> * mapping for executable code.
> >> */
> >> + fallthrough;
> >> +
> >> + case EFI_ACPI_MEMORY_NVS:
> >> + /*
> >> + * ACPI NVS marks an area reserved for use by the
> >> + * firmware, even after exiting the boot service.
> >> + * This may be used by the firmware for sharing dynamic
> >> + * tables/data (e.g., ACPI CCEL) with the OS. Map it
> >> + * as read-only.
> >> + */
> >> prot = PAGE_KERNEL_RO;
> >
> > Please revert this change.
> >
> > Making area reserved to be used by firmware breaks some APEI
> > notification mechanisms:
>
> Thanks for the report. Clearly, we missed this case. I am happy for this
> patch to be reverted and we can work out the handling of NVS later.
>
> We had this as PAGE_KERNEL in the first version, and "tightened to RO".
>
> Pardon my ignorance, but the ACPI specifications say,
> EFI_ACPI_MEMORY_NVS regions are reserved for the Firmware as noted in
> (linked in cover letter) [1].
>
> Is this a standard practise to write to NVS across the architectures ?
> I could see that x86 marks it as PAGE_KERNEL (but didn't really see
> why). I could use the reference to fix this. Also, are you able to
> dump the attributes for the region from the EFI memory map ?
Hi Susuki,
Not sure if this is broken or not on x86, as I don't have any code
yet to test APEI implementation on x86.
The problem here is related to GHESv2 spec:
https://uefi.org/specs/ACPI/6.5/18_Platform_Error_Interfaces.html#generic-hardware-error-source-version-2-ghesv2-structure
As you can see there, GHESv2 BIOS has a "Read Ack register". Such registers
are inside the firmware memory, inside the HEST table.
As described there:
"This field specifies the location of the Read Ack Register used to
notify the RAS controller that OSPM has processed the Error Status
Block. The OSPM writes the bit(s) specified in Read Ack Write,
while preserving the bit(s) specified in Read Ack Preserve."
The HEST table basically contains a data structure which is used by
the BIOS to report errors. As it is inside the firmware allocated memory,
so it should be marked as EfiACPIMemoryNVS.
If it contains GHESv2, one of the fields inside the records is a pointer to
the Read Ack Register. It is placed elsewhere, but its location is also
marked as EfiACPIMemoryNVS, as it is a firmware file. So, also marked
as EfiACPIMemoryNVS.
When a hardware error is detected, the firmware fills the HEST table
and notifies the OSPM. For GHESv2, the notification is asynchronous
(typically using GED on ARM). As the BIOS needs to wait for the OSPM to
handle before re-using the memory region to report new errors, it writes
zero to the Read Ack Register.
When the OSPM handles the error, it writes one to it, allowing the
BIOS to re-use the GHESv2 memory region to report a new error.
So, for GHESv2 to work, Linux has to mark the Read Ack Registers
as R/W - or the entire pages where they're contained(*).
(*) The spec allows multiple GHESv2 records, so one may have multiple
GHESv2 Read Ack Registers.
In the specific case of the QEMU implementation, the HEST table is
placed together with other ACPI tables like DSDT, but we place the
actual error records and the read ack registers are stored on a separate
firmware file.
You can see its current mapping at:
https://github.com/qemu/qemu/blob/master/docs/specs/acpi_hest_ghes.rst
> Also, are you able to dump the attributes for the region from the EFI memory map ?
This is the dump for the HEST table:
/*
* Intel ACPI Component Architecture
* AML/ASL+ Disassembler version 20240927 (64-bit version)
* Copyright (c) 2000 - 2023 Intel Corporation
*
* Disassembly of hest.dat
*
* ACPI Data Table [HEST]
*
* Format: [HexOffset DecimalOffset ByteLength] FieldName : FieldValue (in hex)
*/
[000h 0000 004h] Signature : "HEST" [Hardware Error Source
Table]
[004h 0004 004h] Table Length : 000000E0
[008h 0008 001h] Revision : 01
[009h 0009 001h] Checksum : E4
[00Ah 0010 006h] Oem ID : "BOCHS "
[010h 0016 008h] Oem Table ID : "BXPC "
[018h 0024 004h] Oem Revision : 00000001
[01Ch 0028 004h] Asl Compiler ID : "BXPC"
[020h 0032 004h] Asl Compiler Revision : 00000001
[024h 0036 004h] Error Source Count : 00000002
[028h 0040 002h] Subtable Type : 000A [Generic Hardware Error Sour
ce V2]
[02Ah 0042 002h] Source Id : 0000
[02Ch 0044 002h] Related Source Id : FFFF
[02Eh 0046 001h] Reserved : 00
[02Fh 0047 001h] Enabled : 01
[030h 0048 004h] Records To Preallocate : 00000001
[034h 0052 004h] Max Sections Per Record : 00000001
[038h 0056 004h] Max Raw Data Length : 00001000
[03Ch 0060 00Ch] Error Status Address : [Generic Address Structure]
[03Ch 0060 001h] Space ID : 00 [SystemMemory]
[03Dh 0061 001h] Bit Width : 40
[03Eh 0062 001h] Bit Offset : 00
[03Fh 0063 001h] Encoded Access Width : 04 [QWord Access:64]
[040h 0064 008h] Address : 0000000139B90000
[048h 0072 01Ch] Notify : [Hardware Error Notification Stru
cture]
[048h 0072 001h] Notify Type : 08 [SEA]
[049h 0073 001h] Notify Length : 1C
[04Ah 0074 002h] Configuration Write Enable : 0000
[04Ch 0076 004h] PollInterval : 00000000
[050h 0080 004h] Vector : 00000000
[054h 0084 004h] Polling Threshold Value : 00000000
[058h 0088 004h] Polling Threshold Window : 00000000
[05Ch 0092 004h] Error Threshold Value : 00000000
[060h 0096 004h] Error Threshold Window : 00000000
[064h 0100 004h] Error Status Block Length : 00001000
[068h 0104 00Ch] Read Ack Register : [Generic Address Structure]
[068h 0104 001h] Space ID : 00 [SystemMemory]
[069h 0105 001h] Bit Width : 40
[06Ah 0106 001h] Bit Offset : 00
[06Bh 0107 001h] Encoded Access Width : 04 [QWord Access:64]
[06Ch 0108 008h] Address : 0000000139B90010
[074h 0116 008h] Read Ack Preserve : FFFFFFFFFFFFFFFE
[07Ch 0124 008h] Read Ack Write : 0000000000000001
[084h 0132 002h] Subtable Type : 000A [Generic Hardware Error Sour
ce V2]
[086h 0134 002h] Source Id : 0001
[088h 0136 002h] Related Source Id : FFFF
[08Ah 0138 001h] Reserved : 00
[08Bh 0139 001h] Enabled : 01
[08Ch 0140 004h] Records To Preallocate : 00000001
[090h 0144 004h] Max Sections Per Record : 00000001
[094h 0148 004h] Max Raw Data Length : 00001000
[098h 0152 00Ch] Error Status Address : [Generic Address Structure]
[098h 0152 001h] Space ID : 00 [SystemMemory]
[099h 0153 001h] Bit Width : 40
[09Ah 0154 001h] Bit Offset : 00
[09Bh 0155 001h] Encoded Access Width : 04 [QWord Access:64]
[09Ch 0156 008h] Address : 0000000139B90008
[0A4h 0164 01Ch] Notify : [Hardware Error Notification Stru
cture]
[0A4h 0164 001h] Notify Type : 07 [GPIO]
[0A5h 0165 001h] Notify Length : 1C
[0A6h 0166 002h] Configuration Write Enable : 0000
[0A8h 0168 004h] PollInterval : 00000000
[0ACh 0172 004h] Vector : 00000000
[0B0h 0176 004h] Polling Threshold Value : 00000000
[0B4h 0180 004h] Polling Threshold Window : 00000000
[0B8h 0184 004h] Error Threshold Value : 00000000
[0BCh 0188 004h] Error Threshold Window : 00000000
[0C0h 0192 004h] Error Status Block Length : 00001000
[0C4h 0196 00Ch] Read Ack Register : [Generic Address Structure]
[0C4h 0196 001h] Space ID : 00 [SystemMemory]
[0C5h 0197 001h] Bit Width : 40
[0C6h 0198 001h] Bit Offset : 00
[0C7h 0199 001h] Encoded Access Width : 04 [QWord Access:64]
[0C8h 0200 008h] Address : 0000000139B90018
[0D0h 0208 008h] Read Ack Preserve : FFFFFFFFFFFFFFFE
[0D8h 0216 008h] Read Ack Write : 0000000000000001
Raw Table Data: Length 224 (0xE0)
0000: 48 45 53 54 E0 00 00 00 01 E4 42 4F 43 48 53 20 // HEST......BOCHS
0010: 42 58 50 43 20 20 20 20 01 00 00 00 42 58 50 43 // BXPC ....BXPC
0020: 01 00 00 00 02 00 00 00 0A 00 00 00 FF FF 00 01 // ................
0030: 01 00 00 00 01 00 00 00 00 10 00 00 00 40 00 04 // .............@..
0040: 00 00 B9 39 01 00 00 00 08 1C 00 00 00 00 00 00 // ...9............
0050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
0060: 00 00 00 00 00 10 00 00 00 40 00 04 10 00 B9 39 // .........@.....9
0070: 01 00 00 00 FE FF FF FF FF FF FF FF 01 00 00 00 // ................
0080: 00 00 00 00 0A 00 01 00 FF FF 00 01 01 00 00 00 // ................
0090: 01 00 00 00 00 10 00 00 00 40 00 04 08 00 B9 39 // .........@.....9
00A0: 01 00 00 00 07 1C 00 00 00 00 00 00 00 00 00 00 // ................
00B0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 // ................
00C0: 00 10 00 00 00 40 00 04 18 00 B9 39 01 00 00 00 // .....@.....9....
00D0: FE FF FF FF FF FF FF FF 01 00 00 00 00 00 00 00 // ................
I hope that helps.
Regards,
Mauro
> Kind regards
> Suzuki
>
> [1]
> https://uefi.org/specs/UEFI/2.10/07_Services_Boot_Services.html#memory-type-usage-before-exitbootservices
>
>
> >
> > [ 3.787189] {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 1
> > [ 3.787286] {1}[Hardware Error]: event severity: recoverable
> > [ 3.787367] {1}[Hardware Error]: Error 0, type: recoverable
> > [ 3.787471] {1}[Hardware Error]: section_type: ARM processor error
> > [ 3.787520] {1}[Hardware Error]: MIDR: 0x00000000000f0510
> > [ 3.787555] {1}[Hardware Error]: Multiprocessor Affinity Register (MPIDR): 0x0000000080000000
> > [ 3.787577] {1}[Hardware Error]: running state: 0x0
> > [ 3.787591] {1}[Hardware Error]: Power State Coordination Interface state: 0
> > [ 3.787621] {1}[Hardware Error]: Error info structure 0:
> > [ 3.787635] {1}[Hardware Error]: num errors: 2
> > [ 3.787736] {1}[Hardware Error]: error_type: 0x02: cache error
> > [ 3.787760] {1}[Hardware Error]: error_info: 0x000000000091000f
> > [ 3.787795] {1}[Hardware Error]: transaction type: Data Access
> > [ 3.787823] {1}[Hardware Error]: cache error, operation type: Data write
> > [ 3.787851] {1}[Hardware Error]: cache level: 2
> > [ 3.787876] {1}[Hardware Error]: processor context not corrupted
> > [ 3.788666] [Firmware Warn]: GHES: Unhandled processor error type 0x02: cache error
> > [ 3.789258] Unable to handle kernel write to read-only memory at virtual address ffff800080035018
> > [ 3.789277] Mem abort info:
> > [ 3.789289] ESR = 0x000000009600004f
> > [ 3.789324] EC = 0x25: DABT (current EL), IL = 32 bits
> > [ 3.789343] SET = 0, FnV = 0
> > [ 3.789358] EA = 0, S1PTW = 0
> > [ 3.789376] FSC = 0x0f: level 3 permission fault
> > [ 3.789396] Data abort info:
> > [ 3.789411] ISV = 0, ISS = 0x0000004f, ISS2 = 0x00000000
> > [ 3.789427] CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> > [ 3.789444] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> > [ 3.789501] swapper pgtable: 4k pages, 52-bit VAs, pgdp=00000000505d7000
> > [ 3.789524] [ffff800080035018] pgd=10000000510bc003, p4d=1000000100229403, pud=100000010022a403, pmd=100000010022b403, pte=0060000139b90483
> > [ 3.789936] Internal error: Oops: 000000009600004f [#1] SMP
> > [ 3.798553] Modules linked in:
> > [ 3.799147] CPU: 0 UID: 0 PID: 161 Comm: kworker/0:2 Not tainted 6.18.0-rc1-00016-g166324c9c7aa-dirty #46 PREEMPT
> > [ 3.799754] Hardware name: QEMU QEMU Virtual Machine, BIOS unknown 02/02/2022
> > [ 3.800251] Workqueue: kacpi_notify acpi_os_execute_deferred
> > [ 3.800928] pstate: 614020c5 (nZCv daIF +PAN -UAO -TCO +DIT -SSBS BTYPE=--)
> > [ 3.801207] pc : acpi_os_write_memory+0x120/0x190
> > [ 3.801415] lr : acpi_os_write_memory+0x2c/0x190
> > [ 3.801577] sp : ffff800080a83b60
> > [ 3.801748] x29: ffff800080a83b60 x28: ffff9f6c0f423a38 x27: ffff9f6c0d4e75b0
> > [ 3.802080] x26: ffff9f6c0f7bd930 x25: ffff9f6c0f1dae70 x24: 0000000000000000
> > [ 3.802369] x23: 0000000000000000 x22: ffff9f6c0e35acf8 x21: 0000000000000040
> > [ 3.802641] x20: 0000000000000001 x19: 0000000139b90018 x18: 0000000000000010
> > [ 3.802880] x17: 0000000000000000 x16: 0000000000000002 x15: 0000000000000020
> > [ 3.803133] x14: 00000000ffffffff x13: 0000000000000030 x12: fff00000c09392a0
> > [ 3.803422] x11: 0000000000000058 x10: 0000000000000018 x9 : ffff9f6c0d491634
> > [ 3.803681] x8 : 0000000000000010 x7 : 0000000139b90018 x6 : ffff9f6c0f41b518
> > [ 3.803925] x5 : 0000000139b91000 x4 : 0000000000000018 x3 : fff00000c09391e0
> > [ 3.804176] x2 : 0000000000000040 x1 : 0000000000000008 x0 : ffff800080035018
> > [ 3.804512] Call trace:
> > [ 3.804715] acpi_os_write_memory+0x120/0x190 (P)
> > [ 3.804956] apei_write+0xd0/0xf0
> > [ 3.805112] ghes_clear_estatus.part.0+0xc8/0xe0
> > [ 3.805290] ghes_proc+0xa4/0x220
> > [ 3.805417] ghes_notify_hed+0x5c/0xb8
> > [ 3.805546] notifier_call_chain+0x78/0x148
> > [ 3.805746] blocking_notifier_call_chain+0x4c/0x80
> > [ 3.805945] acpi_hed_notify+0x28/0x40
> > [ 3.806082] acpi_ev_notify_dispatch+0x50/0x80
> > [ 3.806255] acpi_os_execute_deferred+0x24/0x48
> > [ 3.806446] process_one_work+0x15c/0x3b0
> > [ 3.806574] worker_thread+0x2d0/0x400
> > [ 3.806721] kthread+0x148/0x228
> > [ 3.806849] ret_from_fork+0x10/0x20
> > [ 3.807114] Code: 17ffffeb 710102bf 54000341 d50332bf (f9000014)
> > [ 3.807504] ---[ end trace 0000000000000000 ]---
> > [ 4.116196] note: kworker/0:2[161] exited with irqs disabled
> > [ 4.116700] note: kworker/0:2[161] exited with preempt_count 1
> >
> > The problem happens when APEI tries to notify the firmware that a GPIO
> > notification was accepted by writing a value at the read_ack_register:
> >
> > (gdb) list *ghes_clear_estatus+0xc8
> > 0xffff800080945b90 is in ghes_clear_estatus (../drivers/acpi/apei/ghes.c:264).
> > 259 return;
> > 260
> > 261 val &= gv2->read_ack_preserve << gv2->read_ack_register.bit_offset;
> > 262 val |= gv2->read_ack_write << gv2->read_ack_register.bit_offset;
> > 263
> > 264 apei_write(val, &gv2->read_ack_register);
> > 265 }
> > 266
> > 267 static struct ghes *ghes_new(struct acpi_hest_generic *generic)
> > 268 {
> >
> > -
> >
> > You can reproduce it with QEMU v10.2.0-rc1:
> >
> > qemu-system-aarch64 -bios ../emulator/QEMU_EFI-silent.fd \
> > --nographic -monitor telnet:127.0.0.1:1234,server,nowait -m \
> > 4g,maxmem=8G,slots=8 -no-reboot -device pcie-root-port,id=root_port1 -device \
> > virtio-blk-pci,drive=hd -device virtio-net-pci,netdev=mynet,id=bob -object \
> > memory-backend-ram,size=4G,id=mem0 -netdev \
> > type=user,id=mynet,hostfwd=tcp::5555-:22 -qmp \
> > tcp:localhost:4445,server=on,wait=off -M virt,nvdimm=on,ras=on -cpu max -smp \
> > 4 -numa node,nodeid=0,cpus=0-3,memdev=mem0 -kernel \
> > ../work/arm64_build/arch/arm64/boot/Image.gz -append \
> > "earlycon nomodeset root=/dev/vda1 fsck.mode=skip tp_printk maxcpus=4" \
> > -drive if=none,file=../emulator/debian.qcow2,format=qcow2,id=hd
> >
> > using:
> >
> > scripts/ghes_inject.py arm
> >
> > Kernel 6.17 is not affected. The problem happens after 6.18-rc1.
> >
> > Thanks,
> > Mauro
>
>
Thanks,
Mauro
^ permalink raw reply
* Re: [PATCH v4 03/16] x86/virt/tdx: Simplify tdmr_get_pamt_sz()
From: Binbin Wu @ 2025-11-24 9:26 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-4-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> For each memory region that the TDX module might use (TDMR), the three
> separate PAMT allocations are needed. One for each supported page size
> (1GB, 2MB, 4KB). These store information on each page in the TDMR. In
> Linux, they are allocated out of one physically contiguous block, in order
> to more efficiently use some internal TDX module book keeping resources.
> So some simple math is needed to break the single large allocation into
> three smaller allocations for each page size.
>
> There are some commonalities in the math needed to calculate the base and
> size for each smaller allocation, and so an effort was made to share logic
> across the three. Unfortunately doing this turned out naturally tortured,
> with a loop iterating over the three page sizes, only to call into a
> function with a case statement for each page size. In the future Dynamic
> PAMT will add more logic that is special to the 4KB page size, making the
> benefit of the math sharing even more questionable.
>
> Three is not a very high number, so get rid of the loop and just duplicate
> the small calculation three times. In doing so, setup for future Dynamic
> PAMT changes and drop a net 33 lines of code.
>
> 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.
>
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>
One nit below.
[...]
> @@ -535,26 +518,18 @@ static int tdmr_set_up_pamt(struct tdmr_info *tdmr,
> * in overlapped TDMRs.
> */
> pamt = alloc_contig_pages(tdmr_pamt_size >> PAGE_SHIFT, GFP_KERNEL,
> - nid, &node_online_map);
> - if (!pamt)
> + nid, &node_online_map);
> + if (!pamt) {
> + /*
> + * tdmr->pamt_4k_base is zero so the
> + * error path will skip freeing.
> + */
> return -ENOMEM;
Nit:
Do you think it's OK to move the comment up so to avoid multiple lines of
comments as well as the curly braces?
/* tdmr->pamt_4k_base is zero so the error path will skip freeing. */
if (!pamt)
return -ENOMEM;
> -
> - /*
> - * Break the contiguous allocation back up into the
> - * individual PAMTs for each page size.
> - */
> - tdmr_pamt_base = page_to_pfn(pamt) << PAGE_SHIFT;
> - for (pgsz = TDX_PS_4K; pgsz < TDX_PS_NR; pgsz++) {
> - pamt_base[pgsz] = tdmr_pamt_base;
> - tdmr_pamt_base += pamt_size[pgsz];
> }
>
> - tdmr->pamt_4k_base = pamt_base[TDX_PS_4K];
> - tdmr->pamt_4k_size = pamt_size[TDX_PS_4K];
> - tdmr->pamt_2m_base = pamt_base[TDX_PS_2M];
> - tdmr->pamt_2m_size = pamt_size[TDX_PS_2M];
> - tdmr->pamt_1g_base = pamt_base[TDX_PS_1G];
> - tdmr->pamt_1g_size = pamt_size[TDX_PS_1G];
> + tdmr->pamt_4k_base = page_to_phys(pamt);
> + tdmr->pamt_2m_base = tdmr->pamt_4k_base + tdmr->pamt_4k_size;
> + tdmr->pamt_1g_base = tdmr->pamt_2m_base + tdmr->pamt_2m_size;
>
> return 0;
> }
>
[...]
^ permalink raw reply
* Re: [PATCH v4 02/16] x86/tdx: Add helpers to check return status codes
From: Binbin Wu @ 2025-11-24 8:56 UTC (permalink / raw)
To: Rick Edgecombe
Cc: bp, chao.gao, dave.hansen, isaku.yamahata, kai.huang, kas, kvm,
linux-coco, linux-kernel, mingo, pbonzini, seanjc, tglx,
vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu,
Kirill A. Shutemov
In-Reply-To: <20251121005125.417831-3-rick.p.edgecombe@intel.com>
On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
[...]
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 7b2833705d47..167c5b273c40 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -129,9 +129,9 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
>
> ret = __tdcall(TDG_MR_REPORT, &args);
> if (ret) {
> - if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> + if (IS_TDX_OPERAND_INVALID(ret))
> return -ENXIO;
> - else if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
> + else if (IS_TDX_OPERAND_BUSY(ret))
> return -EBUSY;
> return -EIO;
> }
> @@ -165,9 +165,9 @@ int tdx_mcall_extend_rtmr(u8 index, u8 *data)
>
> ret = __tdcall(TDG_MR_RTMR_EXTEND, &args);
> if (ret) {
> - if (TDCALL_RETURN_CODE(ret) == TDCALL_INVALID_OPERAND)
> + if (IS_TDX_OPERAND_INVALID(ret))
> return -ENXIO;
> - if (TDCALL_RETURN_CODE(ret) == TDCALL_OPERAND_BUSY)
> + if (IS_TDX_OPERAND_BUSY(ret))
After the changes in tdx_mcall_get_report0() and tdx_mcall_extend_rtmr(), the
macros defined in arch/x86/coco/tdx/tdx.c can be removed:
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 7b2833705d47..f72b7dfdacd1 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -33,11 +33,6 @@
#define VE_GET_PORT_NUM(e) ((e) >> 16)
#define VE_IS_IO_STRING(e) ((e) & BIT(4))
-/* TDX Module call error codes */
-#define TDCALL_RETURN_CODE(a) ((a) >> 32)
-#define TDCALL_INVALID_OPERAND 0xc0000100
-#define TDCALL_OPERAND_BUSY 0x80000200
-
#define TDREPORT_SUBTYPE_0 0
static atomic_long_t nr_shared;
> return -EBUSY;
> return -EIO;
> }
> @@ -316,7 +316,7 @@ static void reduce_unnecessary_ve(void)
> {
> u64 err = tdg_vm_wr(TDCS_TD_CTLS, TD_CTLS_REDUCE_VE, TD_CTLS_REDUCE_VE);
>
> - if (err == TDX_SUCCESS)
> + if (IS_TDX_SUCCESS(err))
> return;
>
> /*
> diff --git a/arch/x86/include/asm/shared/tdx_errno.h b/arch/x86/include/asm/shared/tdx_errno.h
> index 3aa74f6a6119..e302aed31b50 100644
> --- a/arch/x86/include/asm/shared/tdx_errno.h
> +++ b/arch/x86/include/asm/shared/tdx_errno.h
> @@ -5,7 +5,7 @@
> #include <asm/trapnr.h>
>
> /* Upper 32 bit of the TDX error code encodes the status */
> -#define TDX_SEAMCALL_STATUS_MASK 0xFFFFFFFF00000000ULL
> +#define TDX_STATUS_MASK 0xFFFFFFFF00000000ULL
>
> /*
> * TDX SEAMCALL Status Codes
> @@ -54,4 +54,49 @@
> #define TDX_OPERAND_ID_SEPT 0x92
> #define TDX_OPERAND_ID_TD_EPOCH 0xa9
>
> +#ifndef __ASSEMBLER__
> +#include <linux/bits.h>
> +#include <linux/types.h>
> +
> +static inline u64 TDX_STATUS(u64 err)
> +{
> + return err & TDX_STATUS_MASK;
> +}
Should be tagged with __always_inline since it's used in noinstr range.
[...]
^ permalink raw reply related
* Re: [PATCH v2 08/11] coco: guest: arm64: Add support for fetching and verifying device info
From: Aneesh Kumar K.V @ 2025-11-24 8:28 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-coco, kvmarm, linux-pci, linux-kernel, dan.j.williams, aik,
lukas, Samuel Ortiz, Xu Yilun, Jason Gunthorpe, Suzuki K Poulose,
Steven Price, Bjorn Helgaas, Catalin Marinas, Marc Zyngier,
Will Deacon, Oliver Upton
In-Reply-To: <20251120175432.00004af8@huawei.com>
Jonathan Cameron <jonathan.cameron@huawei.com> writes:
> On Mon, 17 Nov 2025 19:30:04 +0530
> "Aneesh Kumar K.V (Arm)" <aneesh.kumar@kernel.org> wrote:
>
...
>
>> + return -EIO;
>> + }
>> +
>> + dsc->dev_info.cert_id = dev_info->cert_id;
>> + dsc->dev_info.hash_algo = dev_info->hash_algo;
>> + dsc->dev_info.lock_nonce = dev_info->lock_nonce;
>> + dsc->dev_info.meas_nonce = dev_info->meas_nonce;
>> + dsc->dev_info.report_nonce = dev_info->report_nonce;
>> + memcpy(dsc->dev_info.cert_digest, dev_info->cert_digest, SHA512_DIGEST_SIZE);
>> + memcpy(dsc->dev_info.meas_digest, dev_info->meas_digest, SHA512_DIGEST_SIZE);
>> + memcpy(dsc->dev_info.report_digest, dev_info->report_digest, SHA512_DIGEST_SIZE);
>
> So copy everything other than flags. Any reason why not flags?
>
The flags field currently carries p2p_enabled and p2p_bound, but these
aren’t used yet. I’ll drop flags from dsc->dev_info for now and
reintroduce it once there’s an actual user.
>> +
>> + kfree(dev_info);
>> + /*
>> + * Verify that the digests of the provided reports match with the
>> + * digests from RMM
>> + */
>> + ret = verify_digests(dsc);
>> + if (ret) {
>> + pci_err(pdev, "device digest validation failed (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = cca_apply_interface_report_mappings(pdev, true);
>> + if (ret) {
>> + pci_err(pdev, "failed to validate the interface report\n");
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
^ 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