* [PATCH for-4.22 v2 1/5] tools/bitops: adjust bitmap_or() interface to match hypervisor
2026-06-03 19:18 [PATCH for-4.22 v2 0/5] numa: add unit testing plus fix regression Roger Pau Monne
@ 2026-06-03 19:18 ` Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() " Roger Pau Monne
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monne @ 2026-06-03 19:18 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Anthony PERARD, Juergen Gross
Adjust the only toolstack caller to use the new interface. No functional
change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@vates.tech>
---
tools/include/xen-tools/bitops.h | 7 ++++---
tools/libs/guest/xg_sr_save.c | 3 ++-
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/tools/include/xen-tools/bitops.h b/tools/include/xen-tools/bitops.h
index 3b98fba6d74c..29587e89fa6d 100644
--- a/tools/include/xen-tools/bitops.h
+++ b/tools/include/xen-tools/bitops.h
@@ -81,14 +81,15 @@ static inline int test_and_set_bit(unsigned long nr, void *addr)
return oldbit;
}
-static inline void bitmap_or(void *_dst, const void *_other,
+static inline void bitmap_or(void *_dst, const void *_src1, const void *_src2,
unsigned long nr_bits)
{
char *dst = _dst;
- const char *other = _other;
+ const char *src1 = _src1, *src2 = _src2;
unsigned long i;
+
for ( i = 0; i < bitmap_size(nr_bits); ++i )
- dst[i] |= other[i];
+ dst[i] = src1[i] | src2[i];
}
#endif /* __XEN_TOOLS_BITOPS_H__ */
diff --git a/tools/libs/guest/xg_sr_save.c b/tools/libs/guest/xg_sr_save.c
index 3b2c5222e429..fdbceab52e46 100644
--- a/tools/libs/guest/xg_sr_save.c
+++ b/tools/libs/guest/xg_sr_save.c
@@ -668,7 +668,8 @@ static int suspend_and_send_dirty(struct xc_sr_context *ctx)
else
xc_set_progress_prefix(xch, "Checkpointed save");
- bitmap_or(dirty_bitmap, ctx->save.deferred_pages, ctx->save.p2m_size);
+ bitmap_or(dirty_bitmap, dirty_bitmap, ctx->save.deferred_pages,
+ ctx->save.p2m_size);
if ( !ctx->save.live && ctx->stream_type == XC_STREAM_COLO )
{
--
2.53.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() interface to match hypervisor
2026-06-03 19:18 [PATCH for-4.22 v2 0/5] numa: add unit testing plus fix regression Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 1/5] tools/bitops: adjust bitmap_or() interface to match hypervisor Roger Pau Monne
@ 2026-06-03 19:18 ` Roger Pau Monne
2026-06-05 15:24 ` Andrew Cooper
2026-06-03 19:18 ` [PATCH for-4.22 v2 3/5] xen/numa: prepare NUMA setup code for unit testing Roger Pau Monne
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Roger Pau Monne @ 2026-06-03 19:18 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Anthony PERARD, Juergen Gross, Christian Lindig,
David Scott, Julien Grall
Adjust user-space callers to use the new interface. No functional change
intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
---
tools/console/daemon/io.c | 2 +-
tools/include/xen-tools/common-macros.h | 4 +---
tools/libs/call/buffer.c | 3 ++-
tools/libs/foreignmemory/linux.c | 2 +-
tools/libs/gnttab/freebsd.c | 2 +-
tools/libs/gnttab/linux.c | 2 +-
tools/libs/guest/xg_core.c | 2 +-
tools/libs/guest/xg_dom_arm.c | 6 +++---
tools/libs/guest/xg_dom_x86.c | 2 +-
tools/libs/guest/xg_private.h | 4 ++--
tools/libs/guest/xg_sr_common.c | 6 +++---
tools/libs/guest/xg_sr_stream_format.h | 2 +-
tools/libs/light/libxl_arm_acpi.c | 24 +++++++++++------------
tools/libs/light/libxl_create.c | 2 +-
tools/libs/light/libxl_sr_stream_format.h | 2 +-
tools/libs/light/libxl_stream_read.c | 2 +-
tools/libs/light/libxl_stream_write.c | 4 ++--
tools/misc/xen-mfndump.c | 2 +-
tools/ocaml/libs/xc/xenctrl_stubs.c | 2 +-
tools/xenstored/core.c | 4 ++--
tools/xenstored/domain.c | 9 +++++----
tools/xenstored/watch.c | 2 +-
22 files changed, 45 insertions(+), 45 deletions(-)
diff --git a/tools/console/daemon/io.c b/tools/console/daemon/io.c
index 43d4973c247c..b6c46d11dea7 100644
--- a/tools/console/daemon/io.c
+++ b/tools/console/daemon/io.c
@@ -1233,7 +1233,7 @@ static int set_fds(int fd, short events)
/* Round up to 2^8 boundary, in practice this just
* make newsize larger than current_array_size.
*/
- newsize = ROUNDUP(nr_fds + 1, 8);
+ newsize = ROUNDUP(nr_fds + 1, 1U << 8);
new_fds = realloc(fds, sizeof(struct pollfd)*newsize);
if (!new_fds)
diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 9838a108aaea..9e2799178235 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -68,9 +68,7 @@
})
#endif
-#ifndef ROUNDUP
-#define ROUNDUP(_x,_w) (((unsigned long)(_x)+(1UL<<(_w))-1) & ~((1UL<<(_w))-1))
-#endif
+#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
diff --git a/tools/libs/call/buffer.c b/tools/libs/call/buffer.c
index 2579b8c71986..155e4f9d43eb 100644
--- a/tools/libs/call/buffer.c
+++ b/tools/libs/call/buffer.c
@@ -155,7 +155,8 @@ struct allocation_header {
void *xencall_alloc_buffer(xencall_handle *xcall, size_t size)
{
- size_t actual_size = ROUNDUP(size + sizeof(struct allocation_header), PAGE_SHIFT);
+ size_t actual_size = ROUNDUP(size + sizeof(struct allocation_header),
+ PAGE_SIZE);
int nr_pages = actual_size >> PAGE_SHIFT;
struct allocation_header *hdr;
diff --git a/tools/libs/foreignmemory/linux.c b/tools/libs/foreignmemory/linux.c
index 12f959765abd..6d2f30cdf1b6 100644
--- a/tools/libs/foreignmemory/linux.c
+++ b/tools/libs/foreignmemory/linux.c
@@ -198,7 +198,7 @@ void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
*/
privcmd_mmapbatch_t ioctlx;
xen_pfn_t *pfn;
- unsigned int pfn_arr_size = ROUNDUP((num * sizeof(*pfn)), XC_PAGE_SHIFT);
+ unsigned int pfn_arr_size = ROUNDUP(num * sizeof(*pfn), XC_PAGE_SIZE);
int os_page_size = sysconf(_SC_PAGESIZE);
if ( pfn_arr_size <= os_page_size )
diff --git a/tools/libs/gnttab/freebsd.c b/tools/libs/gnttab/freebsd.c
index d69d928a165e..801274478217 100644
--- a/tools/libs/gnttab/freebsd.c
+++ b/tools/libs/gnttab/freebsd.c
@@ -74,7 +74,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
int domids_stride;
unsigned int refs_size = ROUNDUP(count *
sizeof(struct ioctl_gntdev_grant_ref),
- XC_PAGE_SHIFT);
+ XC_PAGE_SIZE);
int os_page_size = getpagesize();
domids_stride = (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN) ? 0 : 1;
diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 7286c1d4fe83..829877e64baa 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -101,7 +101,7 @@ void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
map = alloca(map_size);
else
{
- map_size = ROUNDUP(map_size, XC_PAGE_SHIFT);
+ map_size = ROUNDUP(map_size, XC_PAGE_SIZE);
map = mmap(NULL, map_size, PROT_READ | PROT_WRITE,
MAP_PRIVATE | MAP_ANON | MAP_POPULATE, -1, 0);
if ( map == MAP_FAILED )
diff --git a/tools/libs/guest/xg_core.c b/tools/libs/guest/xg_core.c
index f83436d6cbf8..d3640f0ef891 100644
--- a/tools/libs/guest/xg_core.c
+++ b/tools/libs/guest/xg_core.c
@@ -696,7 +696,7 @@ xc_domain_dumpcore_via_callback(xc_interface *xch,
for ( i = 1; i < sheaders->num; i++ )
sheaders->shdrs[i].sh_offset += fixup;
offset += fixup;
- dummy_len = ROUNDUP(offset, PAGE_SHIFT) - offset; /* padding length */
+ dummy_len = ROUNDUP(offset, PAGE_SIZE) - offset; /* padding length */
offset += dummy_len;
/* pages */
diff --git a/tools/libs/guest/xg_dom_arm.c b/tools/libs/guest/xg_dom_arm.c
index c8d091850622..739ec1c33837 100644
--- a/tools/libs/guest/xg_dom_arm.c
+++ b/tools/libs/guest/xg_dom_arm.c
@@ -364,12 +364,12 @@ static int meminit(struct xc_dom_image *dom)
/* Convenient */
const uint64_t kernbase = dom->kernel_seg.vstart;
- const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
+ const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, MB(2));
const uint64_t kernsize = kernend - kernbase;
const uint64_t dtb_size = dom->devicetree_blob ?
- ROUNDUP(dom->devicetree_size, XC_PAGE_SHIFT) : 0;
+ ROUNDUP(dom->devicetree_size, XC_PAGE_SIZE) : 0;
const uint64_t ramdisk_size = dom->modules[0].blob ?
- ROUNDUP(dom->modules[0].size, XC_PAGE_SHIFT) : 0;
+ ROUNDUP(dom->modules[0].size, XC_PAGE_SIZE) : 0;
const uint64_t modsize = dtb_size + ramdisk_size;
const uint64_t ram128mb = bankbase[0] + (128<<20);
diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
index 268936efe25b..9af83535944a 100644
--- a/tools/libs/guest/xg_dom_x86.c
+++ b/tools/libs/guest/xg_dom_x86.c
@@ -678,7 +678,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
{
if ( dom->cmdline )
{
- dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 3);
+ dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 1U << 3);
start_info_size += dom->cmdline_size;
}
}
diff --git a/tools/libs/guest/xg_private.h b/tools/libs/guest/xg_private.h
index 285229cf82a3..31a79bccf75e 100644
--- a/tools/libs/guest/xg_private.h
+++ b/tools/libs/guest/xg_private.h
@@ -135,7 +135,7 @@ typedef uint64_t x86_pgentry_t;
#define PAGE_SIZE_X86 (1UL << PAGE_SHIFT_X86)
#define PAGE_MASK_X86 (~(PAGE_SIZE_X86-1))
-#define NRPAGES(x) (ROUNDUP(x, PAGE_SHIFT) >> PAGE_SHIFT)
+#define NRPAGES(x) (ROUNDUP(x, PAGE_SIZE) >> PAGE_SHIFT)
static inline xen_pfn_t xc_pfn_to_mfn(xen_pfn_t pfn, xen_pfn_t *p2m,
unsigned gwidth)
@@ -167,7 +167,7 @@ int pin_table(xc_interface *xch, unsigned int type, unsigned long mfn,
*/
#define M2P_SHIFT L2_PAGETABLE_SHIFT_PAE
#define M2P_CHUNK_SIZE (1 << M2P_SHIFT)
-#define M2P_SIZE(_m) ROUNDUP(((_m) * sizeof(xen_pfn_t)), M2P_SHIFT)
+#define M2P_SIZE(_m) ROUNDUP(((_m) * sizeof(xen_pfn_t)), M2P_CHUNK_SIZE)
#define M2P_CHUNKS(_m) (M2P_SIZE((_m)) >> M2P_SHIFT)
#if defined(__x86_64__) || defined(__i386__)
diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
index 7ccdc3b1f6aa..54dde924a7c0 100644
--- a/tools/libs/guest/xg_sr_common.c
+++ b/tools/libs/guest/xg_sr_common.c
@@ -56,11 +56,11 @@ const char *rec_type_to_str(uint32_t type)
int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
void *buf, size_t sz)
{
- static const char zeroes[(1u << REC_ALIGN_ORDER) - 1] = { 0 };
+ static const char zeroes[REC_ALIGN - 1] = { 0 };
xc_interface *xch = ctx->xch;
typeof(rec->length) combined_length = rec->length + sz;
- size_t record_length = ROUNDUP(combined_length, REC_ALIGN_ORDER);
+ size_t record_length = ROUNDUP(combined_length, REC_ALIGN);
struct iovec parts[] = {
{ &rec->type, sizeof(rec->type) },
{ &combined_length, sizeof(combined_length) },
@@ -110,7 +110,7 @@ int read_record(struct xc_sr_context *ctx, int fd, struct xc_sr_record *rec)
return -1;
}
- datasz = ROUNDUP(rhdr.length, REC_ALIGN_ORDER);
+ datasz = ROUNDUP(rhdr.length, REC_ALIGN);
if ( datasz )
{
diff --git a/tools/libs/guest/xg_sr_stream_format.h b/tools/libs/guest/xg_sr_stream_format.h
index 8a0da26f7543..4310f4311e65 100644
--- a/tools/libs/guest/xg_sr_stream_format.h
+++ b/tools/libs/guest/xg_sr_stream_format.h
@@ -53,7 +53,7 @@ struct xc_sr_rhdr
};
/* All records must be aligned up to an 8 octet boundary */
-#define REC_ALIGN_ORDER (3U)
+#define REC_ALIGN (1U << 3)
/* Somewhat arbitrary - 128MB */
#define REC_LENGTH_MAX (128U << 20)
diff --git a/tools/libs/light/libxl_arm_acpi.c b/tools/libs/light/libxl_arm_acpi.c
index ba874c3d3224..ac8165de15b6 100644
--- a/tools/libs/light/libxl_arm_acpi.c
+++ b/tools/libs/light/libxl_arm_acpi.c
@@ -107,12 +107,12 @@ int libxl__get_acpi_size(libxl__gc *gc,
if (rc < 0)
goto out;
- *out = ROUNDUP(size, 3) +
- ROUNDUP(sizeof(struct acpi_table_rsdp), 3) +
- ROUNDUP(sizeof(struct acpi_table_xsdt), 3) +
- ROUNDUP(sizeof(struct acpi_table_gtdt), 3) +
- ROUNDUP(sizeof(struct acpi_table_fadt), 3) +
- ROUNDUP(sizeof(dsdt_anycpu_arm_len), 3);
+ *out = ROUNDUP(size, 1U << 3) +
+ ROUNDUP(sizeof(struct acpi_table_rsdp), 1U << 3) +
+ ROUNDUP(sizeof(struct acpi_table_xsdt), 1U << 3) +
+ ROUNDUP(sizeof(struct acpi_table_gtdt), 1U << 3) +
+ ROUNDUP(sizeof(struct acpi_table_fadt), 1U << 3) +
+ ROUNDUP(sizeof(dsdt_anycpu_arm_len), 1U << 3);
out:
return rc;
@@ -128,7 +128,7 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
acpitables[RSDP].addr = GUEST_ACPI_BASE;
acpitables[RSDP].size = sizeof(struct acpi_table_rsdp);
- dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 1U << 3);
acpitables[XSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
/*
@@ -137,11 +137,11 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
*/
acpitables[XSDT].size = sizeof(struct acpi_table_xsdt) +
sizeof(uint64_t) * 2;
- dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 1U << 3);
acpitables[GTDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
acpitables[GTDT].size = sizeof(struct acpi_table_gtdt);
- dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 1U << 3);
acpitables[MADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
@@ -150,15 +150,15 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
goto out;
acpitables[MADT].size = size;
- dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 1U << 3);
acpitables[FADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
acpitables[FADT].size = sizeof(struct acpi_table_fadt);
- dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 1U << 3);
acpitables[DSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
acpitables[DSDT].size = dsdt_anycpu_arm_len;
- dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 3);
+ dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 1U << 3);
assert(dom->acpi_modules[0].length <= GUEST_ACPI_SIZE);
dom->acpi_modules[0].data = libxl__zalloc(gc, dom->acpi_modules[0].length);
diff --git a/tools/libs/light/libxl_create.c b/tools/libs/light/libxl_create.c
index 6fd62d140389..a8b0c8c50070 100644
--- a/tools/libs/light/libxl_create.c
+++ b/tools/libs/light/libxl_create.c
@@ -600,7 +600,7 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config *d_config,
.opts = 0, /* .opts will be set below */
.nr = b_info->altp2m_count,
},
- .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SHIFT),
+ .vmtrace_size = ROUNDUP(b_info->vmtrace_buf_kb << 10, XC_PAGE_SIZE),
.cpupool_id = info->poolid,
};
diff --git a/tools/libs/light/libxl_sr_stream_format.h b/tools/libs/light/libxl_sr_stream_format.h
index f8f4723c2e91..b02c954a388e 100644
--- a/tools/libs/light/libxl_sr_stream_format.h
+++ b/tools/libs/light/libxl_sr_stream_format.h
@@ -29,7 +29,7 @@ typedef struct libxl__sr_rec_hdr
} libxl__sr_rec_hdr;
/* All records must be aligned up to an 8 octet boundary */
-#define REC_ALIGN_ORDER 3U
+#define REC_ALIGN (1U << 3)
#define REC_TYPE_END 0x00000000U
#define REC_TYPE_LIBXC_CONTEXT 0x00000001U
diff --git a/tools/libs/light/libxl_stream_read.c b/tools/libs/light/libxl_stream_read.c
index e64e8f0eadec..99c7607b6c54 100644
--- a/tools/libs/light/libxl_stream_read.c
+++ b/tools/libs/light/libxl_stream_read.c
@@ -511,7 +511,7 @@ static void record_header_done(libxl__egc *egc,
return;
}
- size_t bytes_to_read = ROUNDUP(rec->hdr.length, REC_ALIGN_ORDER);
+ size_t bytes_to_read = ROUNDUP(rec->hdr.length, REC_ALIGN);
rec->body = libxl__malloc(NOGC, bytes_to_read);
rc = setup_read(stream, "record body",
diff --git a/tools/libs/light/libxl_stream_write.c b/tools/libs/light/libxl_stream_write.c
index 98d44597a732..9ea64369352f 100644
--- a/tools/libs/light/libxl_stream_write.c
+++ b/tools/libs/light/libxl_stream_write.c
@@ -119,7 +119,7 @@ static void setup_generic_write(libxl__egc *egc,
void *body,
sws_record_done_cb cb)
{
- static const uint8_t zero_padding[1U << REC_ALIGN_ORDER] = { 0 };
+ static const uint8_t zero_padding[REC_ALIGN] = { 0 };
libxl__datacopier_state *dc = &stream->dc;
int rc;
@@ -136,7 +136,7 @@ static void setup_generic_write(libxl__egc *egc,
return;
}
- size_t padsz = ROUNDUP(hdr->length, REC_ALIGN_ORDER) - hdr->length;
+ size_t padsz = ROUNDUP(hdr->length, REC_ALIGN) - hdr->length;
uint32_t length = hdr->length;
/* Insert header */
diff --git a/tools/misc/xen-mfndump.c b/tools/misc/xen-mfndump.c
index 28687afbf01a..99a0b1d3b5c0 100644
--- a/tools/misc/xen-mfndump.c
+++ b/tools/misc/xen-mfndump.c
@@ -10,7 +10,7 @@
#include <xen-tools/common-macros.h>
-#define M2P_SIZE(_m) ROUNDUP(((_m) * sizeof(xen_pfn_t)), 21)
+#define M2P_SIZE(_m) ROUNDUP(((_m) * sizeof(xen_pfn_t)), MB(2))
#define is_mapped(pfn_type) (!((pfn_type) & 0x80000000UL))
#define ERROR(msg, args...) fprintf(stderr, msg, ## args)
diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c
index c55f73b265b2..7f6381cdd2fe 100644
--- a/tools/ocaml/libs/xc/xenctrl_stubs.c
+++ b/tools/ocaml/libs/xc/xenctrl_stubs.c
@@ -221,7 +221,7 @@ CAMLprim value stub_xc_domain_create(value xch_val, value wanted_domid, value co
if ( altp2m_nr != (uint16_t)altp2m_nr )
caml_invalid_argument("altp2m_count");
- vmtrace_size = ROUNDUP(vmtrace_size << 10, XC_PAGE_SHIFT);
+ vmtrace_size = ROUNDUP(vmtrace_size << 10, XC_PAGE_SIZE);
if ( vmtrace_size != (uint32_t)vmtrace_size )
caml_invalid_argument("vmtrace_buf_kb");
diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
index d6d462b7bc82..86c86b3e9a77 100644
--- a/tools/xenstored/core.c
+++ b/tools/xenstored/core.c
@@ -466,7 +466,7 @@ int set_fd(int fd, short events)
/* Round up to 2^8 boundary, in practice this just
* make newsize larger than current_array_size.
*/
- newsize = ROUNDUP(nr_fds + 1, 8);
+ newsize = ROUNDUP(nr_fds + 1, 1U << 8);
new_fds = realloc(poll_fds, sizeof(struct pollfd)*newsize);
if (!new_fds)
@@ -3067,7 +3067,7 @@ static int dump_state_node(const void *ctx, struct connection *conn,
head.length += node->hdr.num_perms * sizeof(*sn.perms);
head.length += pathlen;
head.length += node->hdr.datalen;
- head.length = ROUNDUP(head.length, 3);
+ head.length = ROUNDUP(head.length, 1U << 3);
if (fwrite(&head, sizeof(head), 1, fp) != 1)
return dump_state_node_err(data, "Dump node head error");
diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
index 2db452144dd4..a880ff678ef9 100644
--- a/tools/xenstored/domain.c
+++ b/tools/xenstored/domain.c
@@ -2159,7 +2159,7 @@ const char *dump_state_connections(FILE *fp)
if (ret)
return ret;
head.length += sc.data_in_len + sc.data_out_len;
- head.length = ROUNDUP(head.length, 3);
+ head.length = ROUNDUP(head.length, 1U << 3);
if (c->domain) {
sc.fields |= XS_STATE_CONN_FIELDS_UNIQ_ID;
head.length += sizeof(uint64_t);
@@ -2232,7 +2232,8 @@ void read_state_connection(const void *ctx, const void *state)
unsigned long off;
off = sizeof(*sc) + sc->data_in_len + sc->data_out_len;
- domain->unique_id = *(uint64_t *)(state + ROUNDUP(off, 3));
+ domain->unique_id =
+ *(uint64_t *)(state + ROUNDUP(off, 1U << 3));
}
}
@@ -2308,7 +2309,7 @@ static int dump_state_domain(const void *k, void *v, void *arg)
n_quota = get_quota_size(domain->acc, &rec_len);
rec_len += n_quota * sizeof(sd->quota_val[0]);
rec_len += sizeof(*sd);
- rec_len = ROUNDUP(rec_len, 3);
+ rec_len = ROUNDUP(rec_len, 1U << 3);
record = talloc_size(NULL, rec_len + sizeof(*head));
if (!record)
@@ -2372,7 +2373,7 @@ const char *dump_state_glb_quota(FILE *fp)
n_quota = get_quota_size(quotas, &rec_len);
rec_len += n_quota * sizeof(glb->quota_val[0]);
rec_len += sizeof(*glb);
- rec_len = ROUNDUP(rec_len, 3);
+ rec_len = ROUNDUP(rec_len, 1U << 3);
record = talloc_size(NULL, rec_len + sizeof(*head));
if (!record)
diff --git a/tools/xenstored/watch.c b/tools/xenstored/watch.c
index a9a06e9e4816..309c5bb66bef 100644
--- a/tools/xenstored/watch.c
+++ b/tools/xenstored/watch.c
@@ -349,7 +349,7 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
}
head.length += path_len + token_len;
- head.length = ROUNDUP(head.length, 3);
+ head.length = ROUNDUP(head.length, 1U << 3);
if (fwrite(&head, sizeof(head), 1, fp) != 1)
return "Dump watch state error";
--
2.53.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() interface to match hypervisor
2026-06-03 19:18 ` [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() " Roger Pau Monne
@ 2026-06-05 15:24 ` Andrew Cooper
2026-06-05 15:38 ` Roger Pau Monné
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2026-06-05 15:24 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Andrew Cooper, Anthony PERARD, Juergen Gross, Julien Grall
On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> Adjust user-space callers to use the new interface. No functional change
> intended.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Yeah, libxl's choice to use order was always bizarre... I'm glad we're
getting rid of this.
> diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
> index 268936efe25b..9af83535944a 100644
> --- a/tools/libs/guest/xg_dom_x86.c
> +++ b/tools/libs/guest/xg_dom_x86.c
> @@ -678,7 +678,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> {
> if ( dom->cmdline )
> {
> - dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 3);
> + dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 1U << 3);
> start_info_size += dom->cmdline_size;
> }
> }
I think this would be better as a literal 8.
> diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
> index 7ccdc3b1f6aa..54dde924a7c0 100644
> --- a/tools/libs/guest/xg_sr_common.c
> +++ b/tools/libs/guest/xg_sr_common.c
> @@ -56,11 +56,11 @@ const char *rec_type_to_str(uint32_t type)
> int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
> void *buf, size_t sz)
> {
> - static const char zeroes[(1u << REC_ALIGN_ORDER) - 1] = { 0 };
> + static const char zeroes[REC_ALIGN - 1] = { 0 };
= {} as you're editing the line.
I have no idea why this is 7 in libxg, but 8 in libxl, but dropping the
-1 here is probably wise.
> diff --git a/tools/libs/guest/xg_sr_stream_format.h b/tools/libs/guest/xg_sr_stream_format.h
> index 8a0da26f7543..4310f4311e65 100644
> --- a/tools/libs/guest/xg_sr_stream_format.h
> +++ b/tools/libs/guest/xg_sr_stream_format.h
> @@ -53,7 +53,7 @@ struct xc_sr_rhdr
> };
>
> /* All records must be aligned up to an 8 octet boundary */
> -#define REC_ALIGN_ORDER (3U)
> +#define REC_ALIGN (1U << 3)
This really does want to be 8 rather than a shift.
> /* Somewhat arbitrary - 128MB */
> #define REC_LENGTH_MAX (128U << 20)
>
> diff --git a/tools/libs/light/libxl_arm_acpi.c b/tools/libs/light/libxl_arm_acpi.c
> index ba874c3d3224..ac8165de15b6 100644
> --- a/tools/libs/light/libxl_arm_acpi.c
> +++ b/tools/libs/light/libxl_arm_acpi.c
> @@ -107,12 +107,12 @@ int libxl__get_acpi_size(libxl__gc *gc,
> if (rc < 0)
> goto out;
>
> - *out = ROUNDUP(size, 3) +
> - ROUNDUP(sizeof(struct acpi_table_rsdp), 3) +
> - ROUNDUP(sizeof(struct acpi_table_xsdt), 3) +
> - ROUNDUP(sizeof(struct acpi_table_gtdt), 3) +
> - ROUNDUP(sizeof(struct acpi_table_fadt), 3) +
> - ROUNDUP(sizeof(dsdt_anycpu_arm_len), 3);
> + *out = ROUNDUP(size, 1U << 3) +
> + ROUNDUP(sizeof(struct acpi_table_rsdp), 1U << 3) +
> + ROUNDUP(sizeof(struct acpi_table_xsdt), 1U << 3) +
> + ROUNDUP(sizeof(struct acpi_table_gtdt), 1U << 3) +
> + ROUNDUP(sizeof(struct acpi_table_fadt), 1U << 3) +
> + ROUNDUP(sizeof(dsdt_anycpu_arm_len), 1U << 3);
>
> out:
> return rc;
> @@ -128,7 +128,7 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
>
> acpitables[RSDP].addr = GUEST_ACPI_BASE;
> acpitables[RSDP].size = sizeof(struct acpi_table_rsdp);
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 1U << 3);
>
> acpitables[XSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> /*
> @@ -137,11 +137,11 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
> */
> acpitables[XSDT].size = sizeof(struct acpi_table_xsdt) +
> sizeof(uint64_t) * 2;
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 1U << 3);
>
> acpitables[GTDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> acpitables[GTDT].size = sizeof(struct acpi_table_gtdt);
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 1U << 3);
>
> acpitables[MADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
>
> @@ -150,15 +150,15 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
> goto out;
>
> acpitables[MADT].size = size;
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 1U << 3);
>
> acpitables[FADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> acpitables[FADT].size = sizeof(struct acpi_table_fadt);
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 1U << 3);
>
> acpitables[DSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> acpitables[DSDT].size = dsdt_anycpu_arm_len;
> - dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 3);
> + dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 1U << 3);
And all of these.
> diff --git a/tools/libs/light/libxl_sr_stream_format.h b/tools/libs/light/libxl_sr_stream_format.h
> index f8f4723c2e91..b02c954a388e 100644
> --- a/tools/libs/light/libxl_sr_stream_format.h
> +++ b/tools/libs/light/libxl_sr_stream_format.h
> @@ -29,7 +29,7 @@ typedef struct libxl__sr_rec_hdr
> } libxl__sr_rec_hdr;
>
> /* All records must be aligned up to an 8 octet boundary */
> -#define REC_ALIGN_ORDER 3U
> +#define REC_ALIGN (1U << 3)
>
> #define REC_TYPE_END 0x00000000U
> #define REC_TYPE_LIBXC_CONTEXT 0x00000001U
> diff --git a/tools/libs/light/libxl_stream_write.c b/tools/libs/light/libxl_stream_write.c
> index 98d44597a732..9ea64369352f 100644
> --- a/tools/libs/light/libxl_stream_write.c
> +++ b/tools/libs/light/libxl_stream_write.c
> @@ -119,7 +119,7 @@ static void setup_generic_write(libxl__egc *egc,
> void *body,
> sws_record_done_cb cb)
> {
> - static const uint8_t zero_padding[1U << REC_ALIGN_ORDER] = { 0 };
> + static const uint8_t zero_padding[REC_ALIGN] = { 0 };
These want the same adjustments as the libxg side.
> diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> index d6d462b7bc82..86c86b3e9a77 100644
> --- a/tools/xenstored/core.c
> +++ b/tools/xenstored/core.c
> @@ -3067,7 +3067,7 @@ static int dump_state_node(const void *ctx, struct connection *conn,
> head.length += node->hdr.num_perms * sizeof(*sn.perms);
> head.length += pathlen;
> head.length += node->hdr.datalen;
> - head.length = ROUNDUP(head.length, 3);
> + head.length = ROUNDUP(head.length, 1U << 3);
>
> if (fwrite(&head, sizeof(head), 1, fp) != 1)
> return dump_state_node_err(data, "Dump node head error");
> diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> index 2db452144dd4..a880ff678ef9 100644
> --- a/tools/xenstored/domain.c
> +++ b/tools/xenstored/domain.c
> @@ -2159,7 +2159,7 @@ const char *dump_state_connections(FILE *fp)
> if (ret)
> return ret;
> head.length += sc.data_in_len + sc.data_out_len;
> - head.length = ROUNDUP(head.length, 3);
> + head.length = ROUNDUP(head.length, 1U << 3);
> if (c->domain) {
> sc.fields |= XS_STATE_CONN_FIELDS_UNIQ_ID;
> head.length += sizeof(uint64_t);
> @@ -2232,7 +2232,8 @@ void read_state_connection(const void *ctx, const void *state)
> unsigned long off;
>
> off = sizeof(*sc) + sc->data_in_len + sc->data_out_len;
> - domain->unique_id = *(uint64_t *)(state + ROUNDUP(off, 3));
> + domain->unique_id =
> + *(uint64_t *)(state + ROUNDUP(off, 1U << 3));
> }
> }
>
> @@ -2308,7 +2309,7 @@ static int dump_state_domain(const void *k, void *v, void *arg)
> n_quota = get_quota_size(domain->acc, &rec_len);
> rec_len += n_quota * sizeof(sd->quota_val[0]);
> rec_len += sizeof(*sd);
> - rec_len = ROUNDUP(rec_len, 3);
> + rec_len = ROUNDUP(rec_len, 1U << 3);
>
> record = talloc_size(NULL, rec_len + sizeof(*head));
> if (!record)
> @@ -2372,7 +2373,7 @@ const char *dump_state_glb_quota(FILE *fp)
> n_quota = get_quota_size(quotas, &rec_len);
> rec_len += n_quota * sizeof(glb->quota_val[0]);
> rec_len += sizeof(*glb);
> - rec_len = ROUNDUP(rec_len, 3);
> + rec_len = ROUNDUP(rec_len, 1U << 3);
>
> record = talloc_size(NULL, rec_len + sizeof(*head));
> if (!record)
> diff --git a/tools/xenstored/watch.c b/tools/xenstored/watch.c
> index a9a06e9e4816..309c5bb66bef 100644
> --- a/tools/xenstored/watch.c
> +++ b/tools/xenstored/watch.c
> @@ -349,7 +349,7 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
> }
>
> head.length += path_len + token_len;
> - head.length = ROUNDUP(head.length, 3);
> + head.length = ROUNDUP(head.length, 1U << 3);
> if (fwrite(&head, sizeof(head), 1, fp) != 1)
> return "Dump watch state error";
>
And these want to be 8 as well I think.
Definitely with the migration formation adjustments, and preferably with
the others too, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() interface to match hypervisor
2026-06-05 15:24 ` Andrew Cooper
@ 2026-06-05 15:38 ` Roger Pau Monné
0 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monné @ 2026-06-05 15:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Anthony PERARD, Juergen Gross, Julien Grall
On Fri, Jun 05, 2026 at 04:24:27PM +0100, Andrew Cooper wrote:
> On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> > Adjust user-space callers to use the new interface. No functional change
> > intended.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
>
> Yeah, libxl's choice to use order was always bizarre... I'm glad we're
> getting rid of this.
>
> > diff --git a/tools/libs/guest/xg_dom_x86.c b/tools/libs/guest/xg_dom_x86.c
> > index 268936efe25b..9af83535944a 100644
> > --- a/tools/libs/guest/xg_dom_x86.c
> > +++ b/tools/libs/guest/xg_dom_x86.c
> > @@ -678,7 +678,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
> > {
> > if ( dom->cmdline )
> > {
> > - dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 3);
> > + dom->cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 1U << 3);
> > start_info_size += dom->cmdline_size;
> > }
> > }
>
> I think this would be better as a literal 8.
>
> > diff --git a/tools/libs/guest/xg_sr_common.c b/tools/libs/guest/xg_sr_common.c
> > index 7ccdc3b1f6aa..54dde924a7c0 100644
> > --- a/tools/libs/guest/xg_sr_common.c
> > +++ b/tools/libs/guest/xg_sr_common.c
> > @@ -56,11 +56,11 @@ const char *rec_type_to_str(uint32_t type)
> > int write_split_record(struct xc_sr_context *ctx, struct xc_sr_record *rec,
> > void *buf, size_t sz)
> > {
> > - static const char zeroes[(1u << REC_ALIGN_ORDER) - 1] = { 0 };
> > + static const char zeroes[REC_ALIGN - 1] = { 0 };
>
> = {} as you're editing the line.
>
> I have no idea why this is 7 in libxg, but 8 in libxl, but dropping the
> -1 here is probably wise.
I've noticed this oddity, but was cautious with doing any change
there, given the code freeze status. I can do the adjustment.
> > diff --git a/tools/libs/guest/xg_sr_stream_format.h b/tools/libs/guest/xg_sr_stream_format.h
> > index 8a0da26f7543..4310f4311e65 100644
> > --- a/tools/libs/guest/xg_sr_stream_format.h
> > +++ b/tools/libs/guest/xg_sr_stream_format.h
> > @@ -53,7 +53,7 @@ struct xc_sr_rhdr
> > };
> >
> > /* All records must be aligned up to an 8 octet boundary */
> > -#define REC_ALIGN_ORDER (3U)
> > +#define REC_ALIGN (1U << 3)
>
> This really does want to be 8 rather than a shift.
>
> > /* Somewhat arbitrary - 128MB */
> > #define REC_LENGTH_MAX (128U << 20)
> >
> > diff --git a/tools/libs/light/libxl_arm_acpi.c b/tools/libs/light/libxl_arm_acpi.c
> > index ba874c3d3224..ac8165de15b6 100644
> > --- a/tools/libs/light/libxl_arm_acpi.c
> > +++ b/tools/libs/light/libxl_arm_acpi.c
> > @@ -107,12 +107,12 @@ int libxl__get_acpi_size(libxl__gc *gc,
> > if (rc < 0)
> > goto out;
> >
> > - *out = ROUNDUP(size, 3) +
> > - ROUNDUP(sizeof(struct acpi_table_rsdp), 3) +
> > - ROUNDUP(sizeof(struct acpi_table_xsdt), 3) +
> > - ROUNDUP(sizeof(struct acpi_table_gtdt), 3) +
> > - ROUNDUP(sizeof(struct acpi_table_fadt), 3) +
> > - ROUNDUP(sizeof(dsdt_anycpu_arm_len), 3);
> > + *out = ROUNDUP(size, 1U << 3) +
> > + ROUNDUP(sizeof(struct acpi_table_rsdp), 1U << 3) +
> > + ROUNDUP(sizeof(struct acpi_table_xsdt), 1U << 3) +
> > + ROUNDUP(sizeof(struct acpi_table_gtdt), 1U << 3) +
> > + ROUNDUP(sizeof(struct acpi_table_fadt), 1U << 3) +
> > + ROUNDUP(sizeof(dsdt_anycpu_arm_len), 1U << 3);
> >
> > out:
> > return rc;
> > @@ -128,7 +128,7 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
> >
> > acpitables[RSDP].addr = GUEST_ACPI_BASE;
> > acpitables[RSDP].size = sizeof(struct acpi_table_rsdp);
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[RSDP].size, 1U << 3);
> >
> > acpitables[XSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> > /*
> > @@ -137,11 +137,11 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
> > */
> > acpitables[XSDT].size = sizeof(struct acpi_table_xsdt) +
> > sizeof(uint64_t) * 2;
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[XSDT].size, 1U << 3);
> >
> > acpitables[GTDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> > acpitables[GTDT].size = sizeof(struct acpi_table_gtdt);
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[GTDT].size, 1U << 3);
> >
> > acpitables[MADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> >
> > @@ -150,15 +150,15 @@ static int libxl__allocate_acpi_tables(libxl__gc *gc,
> > goto out;
> >
> > acpitables[MADT].size = size;
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[MADT].size, 1U << 3);
> >
> > acpitables[FADT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> > acpitables[FADT].size = sizeof(struct acpi_table_fadt);
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[FADT].size, 1U << 3);
> >
> > acpitables[DSDT].addr = GUEST_ACPI_BASE + dom->acpi_modules[0].length;
> > acpitables[DSDT].size = dsdt_anycpu_arm_len;
> > - dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 3);
> > + dom->acpi_modules[0].length += ROUNDUP(acpitables[DSDT].size, 1U << 3);
>
> And all of these.
>
> > diff --git a/tools/libs/light/libxl_sr_stream_format.h b/tools/libs/light/libxl_sr_stream_format.h
> > index f8f4723c2e91..b02c954a388e 100644
> > --- a/tools/libs/light/libxl_sr_stream_format.h
> > +++ b/tools/libs/light/libxl_sr_stream_format.h
> > @@ -29,7 +29,7 @@ typedef struct libxl__sr_rec_hdr
> > } libxl__sr_rec_hdr;
> >
> > /* All records must be aligned up to an 8 octet boundary */
> > -#define REC_ALIGN_ORDER 3U
> > +#define REC_ALIGN (1U << 3)
> >
> > #define REC_TYPE_END 0x00000000U
> > #define REC_TYPE_LIBXC_CONTEXT 0x00000001U
> > diff --git a/tools/libs/light/libxl_stream_write.c b/tools/libs/light/libxl_stream_write.c
> > index 98d44597a732..9ea64369352f 100644
> > --- a/tools/libs/light/libxl_stream_write.c
> > +++ b/tools/libs/light/libxl_stream_write.c
> > @@ -119,7 +119,7 @@ static void setup_generic_write(libxl__egc *egc,
> > void *body,
> > sws_record_done_cb cb)
> > {
> > - static const uint8_t zero_padding[1U << REC_ALIGN_ORDER] = { 0 };
> > + static const uint8_t zero_padding[REC_ALIGN] = { 0 };
>
> These want the same adjustments as the libxg side.
>
> > diff --git a/tools/xenstored/core.c b/tools/xenstored/core.c
> > index d6d462b7bc82..86c86b3e9a77 100644
> > --- a/tools/xenstored/core.c
> > +++ b/tools/xenstored/core.c
> > @@ -3067,7 +3067,7 @@ static int dump_state_node(const void *ctx, struct connection *conn,
> > head.length += node->hdr.num_perms * sizeof(*sn.perms);
> > head.length += pathlen;
> > head.length += node->hdr.datalen;
> > - head.length = ROUNDUP(head.length, 3);
> > + head.length = ROUNDUP(head.length, 1U << 3);
> >
> > if (fwrite(&head, sizeof(head), 1, fp) != 1)
> > return dump_state_node_err(data, "Dump node head error");
> > diff --git a/tools/xenstored/domain.c b/tools/xenstored/domain.c
> > index 2db452144dd4..a880ff678ef9 100644
> > --- a/tools/xenstored/domain.c
> > +++ b/tools/xenstored/domain.c
> > @@ -2159,7 +2159,7 @@ const char *dump_state_connections(FILE *fp)
> > if (ret)
> > return ret;
> > head.length += sc.data_in_len + sc.data_out_len;
> > - head.length = ROUNDUP(head.length, 3);
> > + head.length = ROUNDUP(head.length, 1U << 3);
> > if (c->domain) {
> > sc.fields |= XS_STATE_CONN_FIELDS_UNIQ_ID;
> > head.length += sizeof(uint64_t);
> > @@ -2232,7 +2232,8 @@ void read_state_connection(const void *ctx, const void *state)
> > unsigned long off;
> >
> > off = sizeof(*sc) + sc->data_in_len + sc->data_out_len;
> > - domain->unique_id = *(uint64_t *)(state + ROUNDUP(off, 3));
> > + domain->unique_id =
> > + *(uint64_t *)(state + ROUNDUP(off, 1U << 3));
> > }
> > }
> >
> > @@ -2308,7 +2309,7 @@ static int dump_state_domain(const void *k, void *v, void *arg)
> > n_quota = get_quota_size(domain->acc, &rec_len);
> > rec_len += n_quota * sizeof(sd->quota_val[0]);
> > rec_len += sizeof(*sd);
> > - rec_len = ROUNDUP(rec_len, 3);
> > + rec_len = ROUNDUP(rec_len, 1U << 3);
> >
> > record = talloc_size(NULL, rec_len + sizeof(*head));
> > if (!record)
> > @@ -2372,7 +2373,7 @@ const char *dump_state_glb_quota(FILE *fp)
> > n_quota = get_quota_size(quotas, &rec_len);
> > rec_len += n_quota * sizeof(glb->quota_val[0]);
> > rec_len += sizeof(*glb);
> > - rec_len = ROUNDUP(rec_len, 3);
> > + rec_len = ROUNDUP(rec_len, 1U << 3);
> >
> > record = talloc_size(NULL, rec_len + sizeof(*head));
> > if (!record)
> > diff --git a/tools/xenstored/watch.c b/tools/xenstored/watch.c
> > index a9a06e9e4816..309c5bb66bef 100644
> > --- a/tools/xenstored/watch.c
> > +++ b/tools/xenstored/watch.c
> > @@ -349,7 +349,7 @@ const char *dump_state_watches(FILE *fp, struct connection *conn,
> > }
> >
> > head.length += path_len + token_len;
> > - head.length = ROUNDUP(head.length, 3);
> > + head.length = ROUNDUP(head.length, 1U << 3);
> > if (fwrite(&head, sizeof(head), 1, fp) != 1)
> > return "Dump watch state error";
> >
>
> And these want to be 8 as well I think.
>
> Definitely with the migration formation adjustments, and preferably with
> the others too, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks, I've done all the adjustments.
Roger.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH for-4.22 v2 3/5] xen/numa: prepare NUMA setup code for unit testing
2026-06-03 19:18 [PATCH for-4.22 v2 0/5] numa: add unit testing plus fix regression Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 1/5] tools/bitops: adjust bitmap_or() interface to match hypervisor Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 2/5] tools/macros: adjust ROUNDUP() " Roger Pau Monne
@ 2026-06-03 19:18 ` Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic Roger Pau Monne
2026-06-03 19:18 ` [PATCH for-4.22 v2 5/5] xen/numa: fix setup of non-aligned memory affinity ranges Roger Pau Monne
4 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monne @ 2026-06-03 19:18 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Andrew Cooper, Anthony PERARD, Michal Orzel,
Jan Beulich, Julien Grall, Stefano Stabellini
Introduce __XEN__ guards to differentiate between hypervisor vs unit test
builds. Also move numa_set_node() so it's outside the __XEN__ guards. No
functional change intended.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
xen/common/numa.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/xen/common/numa.c b/xen/common/numa.c
index ad75955a1622..8544a1598218 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -4,6 +4,7 @@
* Adapted for Xen: Ryan Harper <ryanh@us.ibm.com>
*/
+#ifdef __XEN__
#include <xen/init.h>
#include <xen/keyhandler.h>
#include <xen/mm.h>
@@ -13,6 +14,7 @@
#include <xen/pfn.h>
#include <xen/sched.h>
#include <xen/softirq.h>
+#endif /* __XEN__ */
static nodemask_t __initdata processor_nodes_parsed;
static nodemask_t __initdata memory_nodes_parsed;
@@ -561,6 +563,12 @@ void __init numa_init_array(void)
}
}
+void numa_set_node(unsigned int cpu, nodeid_t node)
+{
+ cpu_to_node[cpu] = node;
+}
+
+#ifdef __XEN__
#ifdef CONFIG_NUMA_EMU
static unsigned int __initdata numa_fake;
@@ -661,11 +669,6 @@ void numa_add_cpu(unsigned int cpu)
cpumask_set_cpu(cpu, &node_to_cpumask[cpu_to_node(cpu)]);
}
-void numa_set_node(unsigned int cpu, nodeid_t node)
-{
- cpu_to_node[cpu] = node;
-}
-
/* [numa=off] */
static int __init cf_check numa_setup(const char *opt)
{
@@ -830,3 +833,4 @@ static int __init cf_check register_numa_trigger(void)
return 0;
}
__initcall(register_numa_trigger);
+#endif /* __XEN__ */
--
2.53.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-03 19:18 [PATCH for-4.22 v2 0/5] numa: add unit testing plus fix regression Roger Pau Monne
` (2 preceding siblings ...)
2026-06-03 19:18 ` [PATCH for-4.22 v2 3/5] xen/numa: prepare NUMA setup code for unit testing Roger Pau Monne
@ 2026-06-03 19:18 ` Roger Pau Monne
2026-06-05 15:10 ` Anthony PERARD
2026-06-05 15:41 ` Andrew Cooper
2026-06-03 19:18 ` [PATCH for-4.22 v2 5/5] xen/numa: fix setup of non-aligned memory affinity ranges Roger Pau Monne
4 siblings, 2 replies; 14+ messages in thread
From: Roger Pau Monne @ 2026-06-03 19:18 UTC (permalink / raw)
To: xen-devel; +Cc: Roger Pau Monne, Anthony PERARD
NUMA setup, like PDX, requires certain amount of logic to configure the
internal structures and parameters for NUMA operation. Introduce some very
basic testing that allows building and testing NUMA setup logic in as a
user-space unit test. This allows feeding synthetic memory affinity and
map to the logic, allowing to reproduce bugs that would otherwise need
access to real systems with such a configuration.
For the time being introduce a single test case, based on a known working
NUMA setup for an AMD Turin system. Also the testing after setup is
currently limited to ensuring the start and end RAM region addresses fall
into a correctly setup memory block.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
- Remove unneeded !!.
- Remove redundant parentheses around macro parameters.
---
tools/tests/Makefile | 1 +
tools/tests/numa/.gitignore | 2 +
tools/tests/numa/Makefile | 47 ++++++++
tools/tests/numa/harness.h | 184 +++++++++++++++++++++++++++++
tools/tests/numa/test-numa.c | 222 +++++++++++++++++++++++++++++++++++
5 files changed, 456 insertions(+)
create mode 100644 tools/tests/numa/.gitignore
create mode 100644 tools/tests/numa/Makefile
create mode 100644 tools/tests/numa/harness.h
create mode 100644 tools/tests/numa/test-numa.c
diff --git a/tools/tests/Makefile b/tools/tests/Makefile
index 6477a4386dda..fc0ed8091510 100644
--- a/tools/tests/Makefile
+++ b/tools/tests/Makefile
@@ -4,6 +4,7 @@ include $(XEN_ROOT)/tools/Rules.mk
SUBDIRS-y :=
SUBDIRS-y += domid
SUBDIRS-y += mem-claim
+SUBDIRS-y += numa
SUBDIRS-y += paging-mempool
SUBDIRS-y += pdx
SUBDIRS-y += rangeset
diff --git a/tools/tests/numa/.gitignore b/tools/tests/numa/.gitignore
new file mode 100644
index 000000000000..0710a767f400
--- /dev/null
+++ b/tools/tests/numa/.gitignore
@@ -0,0 +1,2 @@
+/numa.h
+/test-numa
diff --git a/tools/tests/numa/Makefile b/tools/tests/numa/Makefile
new file mode 100644
index 000000000000..5235f9d6297f
--- /dev/null
+++ b/tools/tests/numa/Makefile
@@ -0,0 +1,47 @@
+XEN_ROOT=$(CURDIR)/../../..
+include $(XEN_ROOT)/tools/Rules.mk
+
+TARGETS := test-numa
+
+.PHONY: all
+all: $(TARGETS)
+
+.PHONY: run
+run: $(TARGETS)
+ifeq ($(CC),$(HOSTCC))
+ set -e; \
+ for test in $? ; do \
+ ./$$test ; \
+ done
+else
+ $(warning HOSTCC != CC, will not run test)
+endif
+
+.PHONY: clean
+clean:
+ $(RM) -- *.o $(TARGETS) $(DEPS_RM) numa.h
+
+.PHONY: distclean
+distclean: clean
+ $(RM) -- *~
+
+.PHONY: install
+install: all
+ $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC)/tests
+ $(INSTALL_PROG) $(TARGETS) $(DESTDIR)$(LIBEXEC)/tests
+
+.PHONY: uninstall
+uninstall:
+ $(RM) -- $(patsubst %,$(DESTDIR)$(LIBEXEC)/tests/%,$(TARGETS))
+
+numa.h: $(XEN_ROOT)/xen/include/xen/numa.h
+ sed -e '/^#[[:space:]]*include/d' <$< >$@
+
+CFLAGS += -D__XEN_TOOLS__
+CFLAGS += $(APPEND_CFLAGS)
+CFLAGS += $(CFLAGS_xeninclude)
+
+test-numa: test-numa.c numa.h
+ $(CC) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_$*.o) -o $@ $< $(APPEND_CFLAGS)
+
+-include $(DEPS_INCLUDE)
diff --git a/tools/tests/numa/harness.h b/tools/tests/numa/harness.h
new file mode 100644
index 000000000000..9eec77f31402
--- /dev/null
+++ b/tools/tests/numa/harness.h
@@ -0,0 +1,184 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Unit tests for NUMA setup.
+ *
+ * Copyright (C) 2026 Cloud Software Group
+ */
+
+#ifndef _TEST_HARNESS_
+#define _TEST_HARNESS_
+
+#include <assert.h>
+#include <errno.h>
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include <xen-tools/bitops.h>
+#include <xen-tools/common-macros.h>
+
+#define CONFIG_DEBUG
+#define CONFIG_NUMA
+#define CONFIG_NR_NUMA_NODES 64
+#define NR_CPUS 256
+#define MAX_RANGES 128
+#define PADDR_BITS 52
+
+#define __init
+#define __initdata
+#define __ro_after_init
+#define __read_mostly
+
+#define printk printf
+#define XENLOG_INFO ""
+#define XENLOG_DEBUG ""
+#define XENLOG_WARNING ""
+#define KERN_INFO ""
+#define KERN_ERR ""
+#define KERN_WARNING ""
+#define KERN_DEBUG ""
+
+#define PAGE_SHIFT 12
+/* Some libcs define PAGE_SIZE in limits.h. */
+#undef PAGE_SIZE
+#define PAGE_SIZE (1L << PAGE_SHIFT)
+#define MAX_ORDER 18 /* 2 * PAGETABLE_ORDER (9) */
+
+#define PFN_DOWN(x) ((x) >> PAGE_SHIFT)
+#define PFN_UP(x) (((x) + PAGE_SIZE-1) >> PAGE_SHIFT)
+
+#define paddr_to_pfn(pa) ((unsigned long)((pa) >> PAGE_SHIFT))
+#define mfn_to_pdx(mfn) (mfn)
+#define paddr_to_pdx(pa) ((pa) >> PAGE_SHIFT)
+#define mfn_to_maddr(mfn) ((mfn) << PAGE_SHIFT)
+
+#define ASSERT assert
+#define ASSERT_UNREACHABLE() assert(0)
+
+/* For the purposes of the testing assume arch NID == Xen NID. */
+#define numa_node_to_arch_nid(n) (n)
+
+typedef uint64_t paddr_t;
+#define PRIpaddr "016" PRIx64
+
+typedef unsigned long mfn_t;
+typedef uint8_t nodeid_t;
+
+#define __set_bit set_bit
+#define __clear_bit clear_bit
+
+static inline unsigned int find_next_bit(
+ const unsigned long *addr, unsigned int size, unsigned int off)
+{
+ unsigned int i;
+
+ ASSERT(size <= BITS_PER_LONG);
+
+ for ( i = off; i < size; i++ )
+ if ( *addr & (1UL << i) )
+ return i;
+
+ return size;
+}
+
+#define find_first_bit(b, s) find_next_bit(b, s, 0)
+
+/* Minimal cpumask support. */
+typedef struct cpumask{ DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
+
+#define cpumask_clear_cpu(c, m) clear_bit(c, (m)->bits)
+
+/* Define the nodemask helpers used. */
+typedef struct nodemask{ DECLARE_BITMAP(bits, CONFIG_NR_NUMA_NODES); } nodemask_t;
+
+#define node_set(node, dst) set_bit(node, (dst).bits)
+
+#define first_node(n) __first_node(&(n), CONFIG_NR_NUMA_NODES)
+static inline int __first_node(const nodemask_t *srcp, unsigned int s)
+{
+ return min(s, find_next_bit(srcp->bits, s, 0));
+}
+
+#define next_node(n, m) __next_node(n, &(m), CONFIG_NR_NUMA_NODES)
+static inline int __next_node(unsigned int n, const nodemask_t *srcp,
+ unsigned int s)
+{
+ return min(s, find_next_bit(srcp->bits, s, n + 1));
+}
+
+#define nodes_or(dst, src1, src2) \
+ bitmap_or((dst).bits, (src1).bits, (src2).bits, CONFIG_NR_NUMA_NODES)
+
+static inline bool nodemask_test(unsigned int node, const nodemask_t *dst)
+{
+ return test_bit(node, dst->bits);
+}
+
+#define node_set_online(node) set_bit(node, node_online_map.bits)
+
+#define cycle_node(n, src) __cycle_node(n, &(src), MAX_NUMNODES)
+static inline int __cycle_node(int n, const nodemask_t *maskp,
+ unsigned int nbits)
+{
+ unsigned int nxt = __next_node(n, maskp, nbits);
+
+ if ( nxt == nbits )
+ nxt = __first_node(maskp, nbits);
+
+ return nxt;
+}
+
+#define for_each_node_mask(node, mask) \
+ for ( (node) = first_node(mask); \
+ (node) < MAX_NUMNODES; \
+ (node) = next_node(node, mask) )
+
+/*
+ * Dummy helper to satisfy allocate_cachealigned_memnodemap(), the memory
+ * allocation is instead done in vmap_contig().
+ */
+static inline mfn_t alloc_boot_pages(unsigned long nr, unsigned long align)
+{
+ return 0;
+}
+
+static inline void *vmap_contig(mfn_t mfn, unsigned int nr)
+{
+ assert(!mfn);
+ return calloc(PAGE_SIZE, nr);
+}
+
+static inline void panic(const char *msg)
+{
+ printf("%s\n", msg);
+ abort();
+}
+
+/* Dummy implementations to satisfy the build. */
+static inline bool arch_numa_disabled(void)
+{
+ return false;
+}
+
+static inline void numa_fw_bad(void) { }
+
+static inline bool arch_numa_unavailable(void)
+{
+ return false;
+}
+
+#include "numa.h"
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
new file mode 100644
index 000000000000..bced68d4d7f1
--- /dev/null
+++ b/tools/tests/numa/test-numa.c
@@ -0,0 +1,222 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Unit tests for NUMA setup.
+ *
+ * Copyright (C) 2026 Cloud Software Group
+ */
+
+#include "harness.h"
+
+static paddr_t mem_hotplug;
+unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
+
+#include "../../xen/common/numa.c"
+
+static void numa_reset_state(void)
+{
+ bitmap_clear(processor_nodes_parsed.bits, CONFIG_NR_NUMA_NODES);
+ bitmap_clear(memory_nodes_parsed.bits, CONFIG_NR_NUMA_NODES);
+ bitmap_clear(memblk_hotplug, NR_NODE_MEMBLKS);
+ memset(numa_nodes, 0, sizeof(numa_nodes));
+ memset(node_memblk_range, 0, sizeof(node_memblk_range));
+ memset(memblk_nodeid, 0, sizeof(memblk_nodeid));
+ memset(node_data, 0, sizeof(node_data));
+ memset(node_to_cpumask, 0, sizeof(node_to_cpumask));
+ memset(cpu_to_node, NUMA_NO_NODE, sizeof(cpu_to_node));
+ num_node_memblks = 0;
+ memnode_shift = 0;
+ memnodemapsize = 0;
+ if ( memnodemap != _memnodemap )
+ free(memnodemap);
+ memnodemap = NULL;
+ bitmap_clear(node_online_map.bits, CONFIG_NR_NUMA_NODES);
+ node_set(1, node_online_map);
+}
+
+struct mem_affinity {
+ /* Ranges are defined as [start, end]. */
+ paddr_t start, end;
+ unsigned int nid;
+};
+
+struct mem_range {
+ /* Ranges are defined as [start, end]. */
+ paddr_t start, end;
+};
+
+const static struct mem_range *ram;
+
+int arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
+{
+ if ( idx >= MAX_RANGES || !ram[idx].end )
+ return -ENOENT;
+
+ *start = ram[idx].start;
+ *end = ram[idx].end + 1;
+
+ return 0;
+}
+
+static void print_ranges(const struct mem_affinity *r)
+{
+ unsigned int i;
+
+ printf("Affinity ranges:\n");
+ for ( i = 0; i < MAX_RANGES; i++ )
+ {
+ if ( !r[i].end )
+ break;
+
+ printf(" NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
+ r[i].nid, r[i].start, r[i].end);
+ }
+
+ printf("RAM ranges:\n");
+ for ( i = 0; i < MAX_RANGES; i++ )
+ {
+ if ( !ram[i].end )
+ break;
+
+ printf(" [%" PRIpaddr ", %" PRIpaddr "]\n",
+ ram[i].start, ram[i].end);
+ }
+}
+
+static bool test_paddr(paddr_t addr)
+{
+ mfn_t mfn = PFN_DOWN(addr);
+ unsigned int idx = mfn >> memnode_shift;
+ unsigned int nid;
+
+ if ( idx >= memnodemapsize )
+ {
+ printf("Fail: MFN %lx -> IDX %u outside of memnodemap range\n",
+ mfn, idx);
+ return false;
+ }
+
+ nid = memnodemap[idx];
+ if ( nid >= MAX_NUMNODES )
+ {
+ printf("Fail: MFN %lx -> NID %u >= MAX_NUMNODES (%u)\n",
+ mfn, nid, MAX_NUMNODES);
+ return false;
+ }
+
+ if ( !node_data[nid].node_spanned_pages )
+ {
+ printf("Fail: MFN %lx -> NID %u without spanned pages\n",
+ mfn, nid);
+ return false;
+
+ }
+
+ if ( !node_data[nid].node_spanned_pages )
+ {
+ printf("Fail: MFN %lx -> NID %u without spanned pages\n",
+ mfn, nid);
+ return false;
+
+ }
+
+ if ( !node_data[nid].node_spanned_pages )
+ {
+ printf("Fail: MFN %lx outside NID range [%013lx, %013lx]\n",
+ mfn, node_data[nid].node_start_pfn,
+ node_data[nid].node_start_pfn +
+ node_data[nid].node_spanned_pages - 1);
+ return false;
+ }
+
+ return true;
+}
+
+int main(int argc, char **argv)
+{
+ static const struct {
+ struct mem_affinity affinity[MAX_RANGES];
+ struct mem_range ram[MAX_RANGES];
+ } tests[] = {
+ /* AMD Turin system. */
+ {
+ .affinity = {
+ { .nid = 0, .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
+ { .nid = 0, .start = 0x000000c0000ULL, .end = 0x000afffffffULL },
+ { .nid = 0, .start = 0x00100000000ULL, .end = 0x0c04fffffffULL },
+ { .nid = 1, .start = 0x0c050000000ULL, .end = 0x0fc4fffffffULL },
+ { .nid = 1, .start = 0x10000000000ULL, .end = 0x183ffffffffULL },
+ },
+ .ram = {
+ { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
+ { .start = 0x00000100000ULL, .end = 0x0007590ffffULL },
+ { .start = 0x000759d1000ULL, .end = 0x00075a0ffffULL },
+ { .start = 0x00076000000ULL, .end = 0x00094c73fffULL },
+ { .start = 0x0009b5ff000ULL, .end = 0x0009fff9fffULL },
+ { .start = 0x0009ffff000ULL, .end = 0x0009fffffffULL },
+ { .start = 0x00100010000ULL, .end = 0x0fc4fffffffULL },
+ { .start = 0x10000000000ULL, .end = 0x183f7ffffffULL },
+ { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
+ },
+ },
+ };
+ int ret_code = EXIT_SUCCESS;
+
+ /* Dummy firmware interface provider name, use TST for TEST. */
+ numa_fw_nid_name = "TST";
+
+ for ( unsigned int i = 0 ; i < ARRAY_SIZE(tests); i++ )
+ {
+ paddr_t min = ~(paddr_t)0, max = 0;
+ unsigned int j;
+
+ numa_reset_state();
+
+ ram = tests[i].ram;
+
+ for ( j = 0;
+ j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
+ j++ )
+ {
+ const struct mem_affinity *affinity = &tests[i].affinity[j];
+ paddr_t length = affinity->end - affinity->start + 1;
+
+ if ( !numa_update_node_memblks(affinity->nid, affinity->nid,
+ affinity->start, length, false) )
+ {
+ printf("Fail to add NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
+ affinity->nid, affinity->start, affinity->end);
+ ret_code = EXIT_FAILURE;
+ continue;
+ }
+
+ min = min(min, affinity->start);
+ max = max(max, affinity->end);
+ }
+
+ if ( !numa_process_nodes(min, max + 1) )
+ {
+ printf("Unable to process nodes\n");
+ print_ranges(tests[i].affinity);
+ ret_code = EXIT_FAILURE;
+ continue;
+ }
+
+ for ( j = 0;
+ j < ARRAY_SIZE(tests[i].ram) && tests[i].ram[j].end;
+ j++ )
+ if ( !test_paddr(tests[i].ram[j].start) ||
+ !test_paddr(tests[i].ram[j].end) )
+ ret_code = EXIT_FAILURE;
+ }
+
+ return ret_code;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
--
2.53.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-03 19:18 ` [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic Roger Pau Monne
@ 2026-06-05 15:10 ` Anthony PERARD
2026-06-05 15:22 ` Roger Pau Monné
2026-06-05 15:41 ` Andrew Cooper
1 sibling, 1 reply; 14+ messages in thread
From: Anthony PERARD @ 2026-06-05 15:10 UTC (permalink / raw)
To: Roger Pau Monne; +Cc: xen-devel
[-- Attachment #1: Type: text/plain, Size: 1772 bytes --]
On Wed, Jun 03, 2026 at 09:18:35PM +0200, Roger Pau Monne wrote:
> diff --git a/tools/tests/numa/Makefile b/tools/tests/numa/Makefile
> new file mode 100644
> index 000000000000..5235f9d6297f
> --- /dev/null
> +++ b/tools/tests/numa/Makefile
> +
> +.PHONY: uninstall
> +uninstall:
> + $(RM) -- $(patsubst %,$(DESTDIR)$(LIBEXEC)/tests/%,$(TARGETS))
There's a simpler way to write this, with
$(addprefix $(DESTDIR)$(LIBEXEC)/tests/,$(TARGETS))
But that's ok to.
> +
> +numa.h: $(XEN_ROOT)/xen/include/xen/numa.h
> + sed -e '/^#[[:space:]]*include/d' <$< >$@
> +
> +CFLAGS += -D__XEN_TOOLS__
> +CFLAGS += $(APPEND_CFLAGS)
> +CFLAGS += $(CFLAGS_xeninclude)
> +
> +test-numa: test-numa.c numa.h
> + $(CC) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_$*.o) -o $@ $< $(APPEND_CFLAGS)
$* should be undefined here.
So we have $(CFLAGS_.o), but that variable doesn't exit either.
You could remove $(CFLAGS_$*.o), it's not use here.
Also, $(APPEND_CFLAGS) is added twice, once via $(CFLAGS) and a second
time on the command line. I think the one added to $(CFLAGS) should be
removed.
> diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> new file mode 100644
> index 000000000000..bced68d4d7f1
> --- /dev/null
> +++ b/tools/tests/numa/test-numa.c
> +
> + for ( j = 0;
> + j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
Why do you test the value `.end` ? ARRAY_SIZE is likely enough as the
test array is static. Same thing later, with the `ram` array.
Anyway, it's all look good enough to me:
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Thanks,
--
Anthony Perard | Vates XCP-ng Developer
XCP-ng & Xen Orchestra - Vates solutions
web: https://vates.tech
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-05 15:10 ` Anthony PERARD
@ 2026-06-05 15:22 ` Roger Pau Monné
0 siblings, 0 replies; 14+ messages in thread
From: Roger Pau Monné @ 2026-06-05 15:22 UTC (permalink / raw)
To: Anthony PERARD; +Cc: xen-devel
On Fri, Jun 05, 2026 at 05:10:23PM +0200, Anthony PERARD wrote:
> On Wed, Jun 03, 2026 at 09:18:35PM +0200, Roger Pau Monne wrote:
> > diff --git a/tools/tests/numa/Makefile b/tools/tests/numa/Makefile
> > new file mode 100644
> > index 000000000000..5235f9d6297f
> > --- /dev/null
> > +++ b/tools/tests/numa/Makefile
> > +
> > +.PHONY: uninstall
> > +uninstall:
> > + $(RM) -- $(patsubst %,$(DESTDIR)$(LIBEXEC)/tests/%,$(TARGETS))
>
> There's a simpler way to write this, with
> $(addprefix $(DESTDIR)$(LIBEXEC)/tests/,$(TARGETS))
> But that's ok to.
I've merely c&p what we had in other Makefiles, and didn't think much
about the specific runes. I will leave that one alone if you don't
mind, we could always do a sweep change of this pattern in
tools/tests.
> > +
> > +numa.h: $(XEN_ROOT)/xen/include/xen/numa.h
> > + sed -e '/^#[[:space:]]*include/d' <$< >$@
> > +
> > +CFLAGS += -D__XEN_TOOLS__
> > +CFLAGS += $(APPEND_CFLAGS)
> > +CFLAGS += $(CFLAGS_xeninclude)
> > +
> > +test-numa: test-numa.c numa.h
> > + $(CC) $(CPPFLAGS) $(CFLAGS) $(CFLAGS_$*.o) -o $@ $< $(APPEND_CFLAGS)
>
> $* should be undefined here.
> So we have $(CFLAGS_.o), but that variable doesn't exit either.
> You could remove $(CFLAGS_$*.o), it's not use here.
Ack.
> Also, $(APPEND_CFLAGS) is added twice, once via $(CFLAGS) and a second
> time on the command line. I think the one added to $(CFLAGS) should be
> removed.
Done, will remove that from CLFAGS then.
> > diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> > new file mode 100644
> > index 000000000000..bced68d4d7f1
> > --- /dev/null
> > +++ b/tools/tests/numa/test-numa.c
> > +
> > + for ( j = 0;
> > + j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
>
> Why do you test the value `.end` ? ARRAY_SIZE is likely enough as the
> test array is static. Same thing later, with the `ram` array.
To skip processing empty/uninitialized entries (ie: ones with start
== end == 0).
>
>
> Anyway, it's all look good enough to me:
> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Thanks, will do the above changes and do one last pre-push CI run.
Roger.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-03 19:18 ` [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic Roger Pau Monne
2026-06-05 15:10 ` Anthony PERARD
@ 2026-06-05 15:41 ` Andrew Cooper
2026-06-05 15:49 ` Roger Pau Monné
1 sibling, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2026-06-05 15:41 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel; +Cc: Andrew Cooper, Anthony PERARD
On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> diff --git a/tools/tests/numa/harness.h b/tools/tests/numa/harness.h
> new file mode 100644
> index 000000000000..9eec77f31402
> --- /dev/null
> +++ b/tools/tests/numa/harness.h
> @@ -0,0 +1,184 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for NUMA setup.
> + *
> + * Copyright (C) 2026 Cloud Software Group
> + */
> +
> +#ifndef _TEST_HARNESS_
> +#define _TEST_HARNESS_
This is overly generic, and liable to break if anyone copies it. Maybe
NUMA_HARNESS, or WRAP_XEN_NUMA because ...
Looking below, how about naming it wrapped-xen-numa.h, so ...
> <snip>
>
> +
> +static inline bool arch_numa_unavailable(void)
> +{
> + return false;
> +}
> +
> +#include "numa.h"
I presume this is the real xen/numa.h ?
> +
> +#endif
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> new file mode 100644
> index 000000000000..bced68d4d7f1
> --- /dev/null
> +++ b/tools/tests/numa/test-numa.c
> @@ -0,0 +1,222 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Unit tests for NUMA setup.
> + *
> + * Copyright (C) 2026 Cloud Software Group
> + */
> +
> +#include "harness.h"
> +
> +static paddr_t mem_hotplug;
> +unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
> +
> +#include "../../xen/common/numa.c"
... this has
#include "wrapped-xen-numa.h"
#include "../../xen/common/numa.c"
which I think is clearer to follow.
> <snip>
>
> +int main(int argc, char **argv)
> +{
> + static const struct {
> + struct mem_affinity affinity[MAX_RANGES];
> + struct mem_range ram[MAX_RANGES];
> + } tests[] = {
> + /* AMD Turin system. */
I'd suggest /* From an arbitrary AMD Turin system */
Just "AMD Turin system" feels a little as if all systems are like this,
which is absolutely not the case.
> + {
> + .affinity = {
> + { .nid = 0, .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
> + { .nid = 0, .start = 0x000000c0000ULL, .end = 0x000afffffffULL },
> + { .nid = 0, .start = 0x00100000000ULL, .end = 0x0c04fffffffULL },
> + { .nid = 1, .start = 0x0c050000000ULL, .end = 0x0fc4fffffffULL },
> + { .nid = 1, .start = 0x10000000000ULL, .end = 0x183ffffffffULL },
> + },
> + .ram = {
> + { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
> + { .start = 0x00000100000ULL, .end = 0x0007590ffffULL },
> + { .start = 0x000759d1000ULL, .end = 0x00075a0ffffULL },
> + { .start = 0x00076000000ULL, .end = 0x00094c73fffULL },
> + { .start = 0x0009b5ff000ULL, .end = 0x0009fff9fffULL },
> + { .start = 0x0009ffff000ULL, .end = 0x0009fffffffULL },
> + { .start = 0x00100010000ULL, .end = 0x0fc4fffffffULL },
> + { .start = 0x10000000000ULL, .end = 0x183f7ffffffULL },
> + { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
> + },
> + },
> + };
> + int ret_code = EXIT_SUCCESS;
> +
> + /* Dummy firmware interface provider name, use TST for TEST. */
> + numa_fw_nid_name = "TST";
> +
> + for ( unsigned int i = 0 ; i < ARRAY_SIZE(tests); i++ )
> + {
> + paddr_t min = ~(paddr_t)0, max = 0;
> + unsigned int j;
> +
> + numa_reset_state();
> +
> + ram = tests[i].ram;
> +
> + for ( j = 0;
> + j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
> + j++ )
> + {
> + const struct mem_affinity *affinity = &tests[i].affinity[j];
> + paddr_t length = affinity->end - affinity->start + 1;
> +
> + if ( !numa_update_node_memblks(affinity->nid, affinity->nid,
> + affinity->start, length, false) )
> + {
> + printf("Fail to add NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
> + affinity->nid, affinity->start, affinity->end);
> + ret_code = EXIT_FAILURE;
> + continue;
> + }
> +
> + min = min(min, affinity->start);
> + max = max(max, affinity->end);
> + }
> +
> + if ( !numa_process_nodes(min, max + 1) )
> + {
> + printf("Unable to process nodes\n");
> + print_ranges(tests[i].affinity);
> + ret_code = EXIT_FAILURE;
> + continue;
This is mis-indented. Best double check the whole file.
> + }
> +
> + for ( j = 0;
> + j < ARRAY_SIZE(tests[i].ram) && tests[i].ram[j].end;
> + j++ )
> + if ( !test_paddr(tests[i].ram[j].start) ||
> + !test_paddr(tests[i].ram[j].end) )
> + ret_code = EXIT_FAILURE;
> + }
> +
> + return ret_code;
This is fine for now, but we're going to have to consolidate the
patterns eventually.
Do you have a Gitlab CI run with this passing?
~Andrew
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-05 15:41 ` Andrew Cooper
@ 2026-06-05 15:49 ` Roger Pau Monné
2026-06-05 15:53 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Roger Pau Monné @ 2026-06-05 15:49 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Anthony PERARD
On Fri, Jun 05, 2026 at 04:41:58PM +0100, Andrew Cooper wrote:
> On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> > diff --git a/tools/tests/numa/harness.h b/tools/tests/numa/harness.h
> > new file mode 100644
> > index 000000000000..9eec77f31402
> > --- /dev/null
> > +++ b/tools/tests/numa/harness.h
> > @@ -0,0 +1,184 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for NUMA setup.
> > + *
> > + * Copyright (C) 2026 Cloud Software Group
> > + */
> > +
> > +#ifndef _TEST_HARNESS_
> > +#define _TEST_HARNESS_
>
> This is overly generic, and liable to break if anyone copies it. Maybe
> NUMA_HARNESS, or WRAP_XEN_NUMA because ...
>
> Looking below, how about naming it wrapped-xen-numa.h, so ...
>
> > <snip>
> >
> > +
> > +static inline bool arch_numa_unavailable(void)
> > +{
> > + return false;
> > +}
> > +
> > +#include "numa.h"
>
> I presume this is the real xen/numa.h ?
>
> > +
> > +#endif
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> > new file mode 100644
> > index 000000000000..bced68d4d7f1
> > --- /dev/null
> > +++ b/tools/tests/numa/test-numa.c
> > @@ -0,0 +1,222 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +/*
> > + * Unit tests for NUMA setup.
> > + *
> > + * Copyright (C) 2026 Cloud Software Group
> > + */
> > +
> > +#include "harness.h"
> > +
> > +static paddr_t mem_hotplug;
> > +unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
> > +
> > +#include "../../xen/common/numa.c"
>
> ... this has
>
> #include "wrapped-xen-numa.h"
> #include "../../xen/common/numa.c"
>
> which I think is clearer to follow.
>
> > <snip>
> >
> > +int main(int argc, char **argv)
> > +{
> > + static const struct {
> > + struct mem_affinity affinity[MAX_RANGES];
> > + struct mem_range ram[MAX_RANGES];
> > + } tests[] = {
> > + /* AMD Turin system. */
>
> I'd suggest /* From an arbitrary AMD Turin system */
>
> Just "AMD Turin system" feels a little as if all systems are like this,
> which is absolutely not the case.
>
> > + {
> > + .affinity = {
> > + { .nid = 0, .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
> > + { .nid = 0, .start = 0x000000c0000ULL, .end = 0x000afffffffULL },
> > + { .nid = 0, .start = 0x00100000000ULL, .end = 0x0c04fffffffULL },
> > + { .nid = 1, .start = 0x0c050000000ULL, .end = 0x0fc4fffffffULL },
> > + { .nid = 1, .start = 0x10000000000ULL, .end = 0x183ffffffffULL },
> > + },
> > + .ram = {
> > + { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
> > + { .start = 0x00000100000ULL, .end = 0x0007590ffffULL },
> > + { .start = 0x000759d1000ULL, .end = 0x00075a0ffffULL },
> > + { .start = 0x00076000000ULL, .end = 0x00094c73fffULL },
> > + { .start = 0x0009b5ff000ULL, .end = 0x0009fff9fffULL },
> > + { .start = 0x0009ffff000ULL, .end = 0x0009fffffffULL },
> > + { .start = 0x00100010000ULL, .end = 0x0fc4fffffffULL },
> > + { .start = 0x10000000000ULL, .end = 0x183f7ffffffULL },
> > + { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
> > + },
> > + },
> > + };
> > + int ret_code = EXIT_SUCCESS;
> > +
> > + /* Dummy firmware interface provider name, use TST for TEST. */
> > + numa_fw_nid_name = "TST";
> > +
> > + for ( unsigned int i = 0 ; i < ARRAY_SIZE(tests); i++ )
> > + {
> > + paddr_t min = ~(paddr_t)0, max = 0;
> > + unsigned int j;
> > +
> > + numa_reset_state();
> > +
> > + ram = tests[i].ram;
> > +
> > + for ( j = 0;
> > + j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
> > + j++ )
> > + {
> > + const struct mem_affinity *affinity = &tests[i].affinity[j];
> > + paddr_t length = affinity->end - affinity->start + 1;
> > +
> > + if ( !numa_update_node_memblks(affinity->nid, affinity->nid,
> > + affinity->start, length, false) )
> > + {
> > + printf("Fail to add NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
> > + affinity->nid, affinity->start, affinity->end);
> > + ret_code = EXIT_FAILURE;
> > + continue;
> > + }
> > +
> > + min = min(min, affinity->start);
> > + max = max(max, affinity->end);
> > + }
> > +
> > + if ( !numa_process_nodes(min, max + 1) )
> > + {
> > + printf("Unable to process nodes\n");
> > + print_ranges(tests[i].affinity);
> > + ret_code = EXIT_FAILURE;
> > + continue;
>
> This is mis-indented. Best double check the whole file.
>
> > + }
> > +
> > + for ( j = 0;
> > + j < ARRAY_SIZE(tests[i].ram) && tests[i].ram[j].end;
> > + j++ )
> > + if ( !test_paddr(tests[i].ram[j].start) ||
> > + !test_paddr(tests[i].ram[j].end) )
> > + ret_code = EXIT_FAILURE;
> > + }
> > +
> > + return ret_code;
>
> This is fine for now, but we're going to have to consolidate the
> patterns eventually.
>
> Do you have a Gitlab CI run with this passing?
Yes, this for example:
https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/14693648817
Thanks, Roger.
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic
2026-06-05 15:49 ` Roger Pau Monné
@ 2026-06-05 15:53 ` Andrew Cooper
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-06-05 15:53 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Andrew Cooper, xen-devel, Anthony PERARD
On 05/06/2026 4:49 pm, Roger Pau Monné wrote:
> On Fri, Jun 05, 2026 at 04:41:58PM +0100, Andrew Cooper wrote:
>> On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
>>> diff --git a/tools/tests/numa/harness.h b/tools/tests/numa/harness.h
>>> new file mode 100644
>>> index 000000000000..9eec77f31402
>>> --- /dev/null
>>> +++ b/tools/tests/numa/harness.h
>>> @@ -0,0 +1,184 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Unit tests for NUMA setup.
>>> + *
>>> + * Copyright (C) 2026 Cloud Software Group
>>> + */
>>> +
>>> +#ifndef _TEST_HARNESS_
>>> +#define _TEST_HARNESS_
>> This is overly generic, and liable to break if anyone copies it. Maybe
>> NUMA_HARNESS, or WRAP_XEN_NUMA because ...
>>
>> Looking below, how about naming it wrapped-xen-numa.h, so ...
>>
>>> <snip>
>>>
>>> +
>>> +static inline bool arch_numa_unavailable(void)
>>> +{
>>> + return false;
>>> +}
>>> +
>>> +#include "numa.h"
>> I presume this is the real xen/numa.h ?
>>
>>> +
>>> +#endif
>>> +
>>> +/*
>>> + * Local variables:
>>> + * mode: C
>>> + * c-file-style: "BSD"
>>> + * c-basic-offset: 4
>>> + * indent-tabs-mode: nil
>>> + * End:
>>> + */
>>> diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
>>> new file mode 100644
>>> index 000000000000..bced68d4d7f1
>>> --- /dev/null
>>> +++ b/tools/tests/numa/test-numa.c
>>> @@ -0,0 +1,222 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +/*
>>> + * Unit tests for NUMA setup.
>>> + *
>>> + * Copyright (C) 2026 Cloud Software Group
>>> + */
>>> +
>>> +#include "harness.h"
>>> +
>>> +static paddr_t mem_hotplug;
>>> +unsigned int __read_mostly nr_cpu_ids = NR_CPUS;
>>> +
>>> +#include "../../xen/common/numa.c"
>> ... this has
>>
>> #include "wrapped-xen-numa.h"
>> #include "../../xen/common/numa.c"
>>
>> which I think is clearer to follow.
>>
>>> <snip>
>>>
>>> +int main(int argc, char **argv)
>>> +{
>>> + static const struct {
>>> + struct mem_affinity affinity[MAX_RANGES];
>>> + struct mem_range ram[MAX_RANGES];
>>> + } tests[] = {
>>> + /* AMD Turin system. */
>> I'd suggest /* From an arbitrary AMD Turin system */
>>
>> Just "AMD Turin system" feels a little as if all systems are like this,
>> which is absolutely not the case.
>>
>>> + {
>>> + .affinity = {
>>> + { .nid = 0, .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
>>> + { .nid = 0, .start = 0x000000c0000ULL, .end = 0x000afffffffULL },
>>> + { .nid = 0, .start = 0x00100000000ULL, .end = 0x0c04fffffffULL },
>>> + { .nid = 1, .start = 0x0c050000000ULL, .end = 0x0fc4fffffffULL },
>>> + { .nid = 1, .start = 0x10000000000ULL, .end = 0x183ffffffffULL },
>>> + },
>>> + .ram = {
>>> + { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
>>> + { .start = 0x00000100000ULL, .end = 0x0007590ffffULL },
>>> + { .start = 0x000759d1000ULL, .end = 0x00075a0ffffULL },
>>> + { .start = 0x00076000000ULL, .end = 0x00094c73fffULL },
>>> + { .start = 0x0009b5ff000ULL, .end = 0x0009fff9fffULL },
>>> + { .start = 0x0009ffff000ULL, .end = 0x0009fffffffULL },
>>> + { .start = 0x00100010000ULL, .end = 0x0fc4fffffffULL },
>>> + { .start = 0x10000000000ULL, .end = 0x183f7ffffffULL },
>>> + { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
>>> + },
>>> + },
>>> + };
>>> + int ret_code = EXIT_SUCCESS;
>>> +
>>> + /* Dummy firmware interface provider name, use TST for TEST. */
>>> + numa_fw_nid_name = "TST";
>>> +
>>> + for ( unsigned int i = 0 ; i < ARRAY_SIZE(tests); i++ )
>>> + {
>>> + paddr_t min = ~(paddr_t)0, max = 0;
>>> + unsigned int j;
>>> +
>>> + numa_reset_state();
>>> +
>>> + ram = tests[i].ram;
>>> +
>>> + for ( j = 0;
>>> + j < ARRAY_SIZE(tests[i].affinity) && tests[i].affinity[j].end;
>>> + j++ )
>>> + {
>>> + const struct mem_affinity *affinity = &tests[i].affinity[j];
>>> + paddr_t length = affinity->end - affinity->start + 1;
>>> +
>>> + if ( !numa_update_node_memblks(affinity->nid, affinity->nid,
>>> + affinity->start, length, false) )
>>> + {
>>> + printf("Fail to add NID %u [%" PRIpaddr ", %" PRIpaddr "]\n",
>>> + affinity->nid, affinity->start, affinity->end);
>>> + ret_code = EXIT_FAILURE;
>>> + continue;
>>> + }
>>> +
>>> + min = min(min, affinity->start);
>>> + max = max(max, affinity->end);
>>> + }
>>> +
>>> + if ( !numa_process_nodes(min, max + 1) )
>>> + {
>>> + printf("Unable to process nodes\n");
>>> + print_ranges(tests[i].affinity);
>>> + ret_code = EXIT_FAILURE;
>>> + continue;
>> This is mis-indented. Best double check the whole file.
>>
>>> + }
>>> +
>>> + for ( j = 0;
>>> + j < ARRAY_SIZE(tests[i].ram) && tests[i].ram[j].end;
>>> + j++ )
>>> + if ( !test_paddr(tests[i].ram[j].start) ||
>>> + !test_paddr(tests[i].ram[j].end) )
>>> + ret_code = EXIT_FAILURE;
>>> + }
>>> +
>>> + return ret_code;
>> This is fine for now, but we're going to have to consolidate the
>> patterns eventually.
>>
>> Do you have a Gitlab CI run with this passing?
> Yes, this for example:
>
> https://gitlab.com/xen-project/hardware/xen-staging/-/jobs/14693648817
Ok. Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> preferably with
the suggested adjustments.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH for-4.22 v2 5/5] xen/numa: fix setup of non-aligned memory affinity ranges
2026-06-03 19:18 [PATCH for-4.22 v2 0/5] numa: add unit testing plus fix regression Roger Pau Monne
` (3 preceding siblings ...)
2026-06-03 19:18 ` [PATCH for-4.22 v2 4/5] tests/numa: add unit tests for NUMA setup logic Roger Pau Monne
@ 2026-06-03 19:18 ` Roger Pau Monne
2026-06-05 15:49 ` Andrew Cooper
4 siblings, 1 reply; 14+ messages in thread
From: Roger Pau Monne @ 2026-06-03 19:18 UTC (permalink / raw)
To: xen-devel
Cc: Roger Pau Monne, Anthony PERARD, Andrew Cooper, Michal Orzel,
Jan Beulich, Julien Grall, Stefano Stabellini
The logic to populate memnodemap in populate_memnodemap() assumes that all
ranges are aligned to the hash shift, this however is only true for the
first address in a memory affinity node. Any subsequent ranges belonging
to the same node might not be aligned to the hash shift value.
Such lack of alignment causes issues to the logic in populate_memnodemap(),
as then the tail of the range might not be properly accounted for and setup
in memnodemap. Fix this by forcing the start address of all regions to
be aligned to the hash shift; if such alignment causes a region overlap it
would always be between regions on the same node, and hence will never
cause setup issues of the memnodemap array.
Introduce two additional test cases to the user-space NUMA setup unit
testing, first test case is the native memory affinity and memory map of
the system where this issue was found, second test case is a simplification
to demonstrate the original problem more clearly.
Fixes: 1666086b0044 ("x86/NUMA: improve memnode_shift calculation for multi node system")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v1:
- Do the rounding after the checking.
---
tools/include/xen-tools/common-macros.h | 1 +
tools/tests/numa/test-numa.c | 45 +++++++++++++++++++++++++
xen/common/numa.c | 6 ++++
3 files changed, 52 insertions(+)
diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 9e2799178235..88b4a0e5a693 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -69,6 +69,7 @@
#endif
#define ROUNDUP(x, a) (((x) + (a) - 1) & ~((a) - 1))
+#define ROUNDDOWN(x, a) ((x) & ~((a) - 1))
#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
index bced68d4d7f1..8122e63a88ed 100644
--- a/tools/tests/numa/test-numa.c
+++ b/tools/tests/numa/test-numa.c
@@ -158,6 +158,51 @@ int main(int argc, char **argv)
{ .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
},
},
+ /* System with unaligned affinity memblock. */
+ {
+ .affinity = {
+ { .nid = 0, .start = 0x00000000000ULL, .end = 0x000afffffffULL },
+ { .nid = 0, .start = 0x00100000000ULL, .end = 0x0fc4fffffffULL },
+ { .nid = 0, .start = 0x10000000000ULL, .end = 0x103ffffffffULL },
+ { .nid = 1, .start = 0x10400000000ULL, .end = 0x203ffffffffULL },
+ },
+ .ram = {
+ { .start = 0x00000000000ULL, .end = 0x0000009ffffULL },
+ { .start = 0x00000100000ULL, .end = 0x000165bffffULL },
+ { .start = 0x00016600000ULL, .end = 0x0001aa1dfffULL },
+ { .start = 0x0001aa1f000ULL, .end = 0x0001aa53fffULL },
+ { .start = 0x0001aab8000ULL, .end = 0x0001aac6fffULL },
+ { .start = 0x0001aacc000ULL, .end = 0x0006f3fefffULL },
+ { .start = 0x00075dff000ULL, .end = 0x00075dfffffULL },
+ { .start = 0x00076000000ULL, .end = 0x000a7ffffffULL },
+ { .start = 0x00100010000ULL, .end = 0x0fc43ffffffULL },
+ { .start = 0x0fc45000000ULL, .end = 0x0fc47ffffffULL },
+ { .start = 0x0fc49000000ULL, .end = 0x0fc4bffffffULL },
+ { .start = 0x0fc4d000000ULL, .end = 0x0fc4d3bffffULL },
+ { .start = 0x0fc4f000000ULL, .end = 0x0fc4f0fffffULL },
+ { .start = 0x10000000000ULL, .end = 0x203fd7fffffULL },
+ },
+ },
+ /*
+ * Reduction of the issue above: introduce an unaligned middle region
+ * with regards to the hash shift.
+ */
+ {
+ .affinity = {
+ { .nid = 0, .start = 0x00000ULL, .end = 0x00fffULL },
+ /*
+ * The offset of the region below is not aligned with the hash
+ * shift: the shift calculation only takes into account the
+ * start of node address.
+ */
+ { .nid = 0, .start = 0x01000ULL, .end = 0x04fffULL },
+ { .nid = 1, .start = 0x14000ULL, .end = 0x14fffULL },
+ },
+ .ram = {
+ { .start = 0x00000ULL, .end = 0x04fffULL },
+ { .start = 0x14000ULL, .end = 0x14fffULL },
+ },
+ },
};
int ret_code = EXIT_SUCCESS;
diff --git a/xen/common/numa.c b/xen/common/numa.c
index 8544a1598218..92f8f1cedce1 100644
--- a/xen/common/numa.c
+++ b/xen/common/numa.c
@@ -405,6 +405,12 @@ static int __init populate_memnodemap(const struct node *nodes,
if ( (epdx >> shift) >= memnodemapsize )
return 0;
+ /*
+ * Round down start address: if start is not aligned to the memnodemap
+ * chunk size the tail remainder might not be added. Overlaps created
+ * by rounding will fall into the same NUMA region.
+ */
+ spdx = ROUNDDOWN(spdx, 1UL << shift);
do {
if ( memnodemap[spdx >> shift] != NUMA_NO_NODE &&
(!nodeids || memnodemap[spdx >> shift] != nodeids[i]) )
--
2.53.0
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH for-4.22 v2 5/5] xen/numa: fix setup of non-aligned memory affinity ranges
2026-06-03 19:18 ` [PATCH for-4.22 v2 5/5] xen/numa: fix setup of non-aligned memory affinity ranges Roger Pau Monne
@ 2026-06-05 15:49 ` Andrew Cooper
0 siblings, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2026-06-05 15:49 UTC (permalink / raw)
To: Roger Pau Monne, xen-devel
Cc: Andrew Cooper, Anthony PERARD, Michal Orzel, Jan Beulich,
Julien Grall, Stefano Stabellini
On 03/06/2026 8:18 pm, Roger Pau Monne wrote:
> diff --git a/tools/tests/numa/test-numa.c b/tools/tests/numa/test-numa.c
> index bced68d4d7f1..8122e63a88ed 100644
> --- a/tools/tests/numa/test-numa.c
> +++ b/tools/tests/numa/test-numa.c
> @@ -158,6 +158,51 @@ int main(int argc, char **argv)
> { .start = 0x183f8800000ULL, .end = 0x183faabffffULL },
> },
> },
> + /* System with unaligned affinity memblock. */
This probably wants to say "Found on a pre-production system", rather
than just something we invented. The reader can make the implication
given the Zen5 above.
Otherwise, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
^ permalink raw reply [flat|nested] 14+ messages in thread