[PATCH v2 0/6] arm: extended regions fixes

[PATCH v2 0/6] arm: extended regions fixes

[Stewart Hildebrand]

v2 pipeline: https://gitlab.com/xen-project/people/stewarthildebrand/xen/-/pipelines/1807189300

v1->v2:
* rebase
* address feedback

Stewart Hildebrand (6):
  xen/arm: fix math in add_ext_regions
  xen/arm: fix math in add_hwdom_free_regions
  xen/arm: switch find_domU_holes to rangesets
  rangeset: introduce rangeset_subtract
  xen/arm: exclude xen,reg from domU extended regions
  tools/arm: exclude iomem from domU extended regions

 tools/libs/light/libxl_arm.c            | 118 ++++++++++++++++++++----
 xen/arch/arm/domain_build.c             |  55 ++++++++---
 xen/arch/arm/include/asm/static-shmem.h |   9 --
 xen/arch/arm/static-shmem.c             |  65 -------------
 xen/common/device-tree/dom0less-build.c |  19 +++-
 xen/common/device-tree/domain-build.c   |   2 +-
 xen/common/rangeset.c                   |  12 +++
 xen/include/xen/fdt-kernel.h            |   1 +
 xen/include/xen/rangeset.h              |   3 +
 9 files changed, 177 insertions(+), 107 deletions(-)


base-commit: ed9488a0d155562cc4f1c9a1c38031579a347cf4
-- 
2.49.0

[PATCH v2 1/6] xen/arm: fix math in add_ext_regions

[Stewart Hildebrand]

In commit f37a59813979, the arguments to add_ext_regions() were switched
from addresses to frame numbers. add_ext_regions() converts the frame
numbers back to addresses, but the end address (e) is rounded down to
page size alignment. The logic to calculate the size assumes e points to
the last address, not page, effectively leading to the region size being
erroneously calculated to be 2M smaller than the actual size of the
region.

Fix by adding 1 to the frame number before converting back to address.

Fixes: f37a59813979 ("xen/arm: domain_build: Track unallocated pages using the frame number")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>
---
v1->v2:
* add Michal's A-b
---
 xen/arch/arm/domain_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index df29619c4007..2f2b021dec3e 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -761,7 +761,7 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
     struct membanks *ext_regions = data;
     paddr_t start, size;
     paddr_t s = pfn_to_paddr(s_gfn);
-    paddr_t e = pfn_to_paddr(e_gfn);
+    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
 
     if ( ext_regions->nr_banks >= ext_regions->max_banks )
         return 0;
-- 
2.49.0

[PATCH v2 2/6] xen/arm: fix math in add_hwdom_free_regions

[Stewart Hildebrand]

Erroneous logic was duplicated from add_ext_regions() into
add_hwdom_free_regions(). Frame numbers are converted to addresses, but
the end address (e) is rounded down to page size alignment. The logic to
calculate the size assumes e points to the last address, not page,
effectively leading to the region size being erroneously calculated to
be 2M smaller than the actual size of the region.

Fix by adding 1 to the frame number before converting back to address.

Fixes: 02975cc38389 ("xen/arm: permit non direct-mapped Dom0 construction")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>
---
v1->v2:
* add Michal's A-b
* rebase
---
 xen/common/device-tree/domain-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/device-tree/domain-build.c b/xen/common/device-tree/domain-build.c
index 762b63e2b00a..9556af43e019 100644
--- a/xen/common/device-tree/domain-build.c
+++ b/xen/common/device-tree/domain-build.c
@@ -109,7 +109,7 @@ static int __init add_hwdom_free_regions(unsigned long s_gfn,
     struct membanks *free_regions = data;
     paddr_t start, size;
     paddr_t s = pfn_to_paddr(s_gfn);
-    paddr_t e = pfn_to_paddr(e_gfn);
+    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;
     unsigned int i, j;
 
     if ( free_regions->nr_banks >= free_regions->max_banks )
-- 
2.49.0

[PATCH v2 3/6] xen/arm: switch find_domU_holes to rangesets

[Stewart Hildebrand]

remove_shm_holes_for_domU() is unnecessarily complex: it re-creates the
extended regions from scratch.

Move the rangeset into find_domU_holes() and create the extended regions
only once. This makes is simpler to further manipulate the rangeset for
removing other regions.

Remove now-unused remove_shm_holes_for_domU().

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
v1->v2:
* add Stefano's R-b
---
 xen/arch/arm/domain_build.c             | 46 ++++++++++++-----
 xen/arch/arm/include/asm/static-shmem.h |  9 ----
 xen/arch/arm/static-shmem.c             | 65 -------------------------
 3 files changed, 35 insertions(+), 85 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 2f2b021dec3e..05a77a4f92c6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -933,34 +933,58 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
                                   struct membanks *ext_regions)
 {
     unsigned int i;
-    uint64_t bankend;
     const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
     const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
     const struct membanks *kinfo_mem = kernel_info_get_mem_const(kinfo);
-    int res = -ENOENT;
+    struct rangeset *mem_holes;
+    int res;
+
+    mem_holes = rangeset_new(NULL, NULL, 0);
+    if ( !mem_holes )
+        return -ENOMEM;
 
     for ( i = 0; i < GUEST_RAM_BANKS; i++ )
     {
-        struct membank *ext_bank = &(ext_regions->bank[ext_regions->nr_banks]);
+        uint64_t bankend, start, size = 0;
 
-        ext_bank->start = ROUNDUP(bankbase[i] + kinfo_mem->bank[i].size, SZ_2M);
+        start = ROUNDUP(bankbase[i] + kinfo_mem->bank[i].size, SZ_2M);
 
         bankend = ~0ULL >> (64 - p2m_ipa_bits);
         bankend = min(bankend, bankbase[i] + banksize[i] - 1);
-        if ( bankend > ext_bank->start )
-            ext_bank->size = bankend - ext_bank->start + 1;
+
+        if ( bankend > start )
+            size = bankend - start + 1;
 
         /* 64MB is the minimum size of an extended region */
-        if ( ext_bank->size < MB(64) )
+        if ( size < MB(64) )
             continue;
-        ext_regions->nr_banks++;
-        res = 0;
+
+        res = rangeset_add_range(mem_holes, PFN_DOWN(start), PFN_DOWN(bankend));
+        if ( res )
+        {
+            printk(XENLOG_ERR "Failed to add: %#"PRIx64"->%#"PRIx64"\n",
+                   start, start + size - 1);
+            goto out;
+        }
     }
 
+    /* Remove static shared memory regions */
+    res = remove_shm_from_rangeset(kinfo, mem_holes);
     if ( res )
-        return res;
+        goto out;
+
+    res = rangeset_report_ranges(mem_holes, 0,
+                                 PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
+                                 add_ext_regions, ext_regions);
+    if ( res )
+        ext_regions->nr_banks = 0;
+    else if ( !ext_regions->nr_banks )
+        res = -ENOENT;
 
-    return remove_shm_holes_for_domU(kinfo, ext_regions);
+ out:
+    rangeset_destroy(mem_holes);
+
+    return res;
 }
 
 static int __init find_host_extended_regions(const struct kernel_info *kinfo,
diff --git a/xen/arch/arm/include/asm/static-shmem.h b/xen/arch/arm/include/asm/static-shmem.h
index 4034cec32f87..6a4c33cca8c2 100644
--- a/xen/arch/arm/include/asm/static-shmem.h
+++ b/xen/arch/arm/include/asm/static-shmem.h
@@ -27,9 +27,6 @@ void init_sharedmem_pages(void);
 int remove_shm_from_rangeset(const struct kernel_info *kinfo,
                              struct rangeset *rangeset);
 
-int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
-                              struct membanks *ext_regions);
-
 int make_shm_resv_memory_node(const struct kernel_info *kinfo, int addrcells,
                               int sizecells);
 
@@ -73,12 +70,6 @@ static inline int remove_shm_from_rangeset(const struct kernel_info *kinfo,
     return 0;
 }
 
-static inline int remove_shm_holes_for_domU(const struct kernel_info *kinfo,
-                                            struct membanks *ext_regions)
-{
-    return 0;
-}
-
 static inline int make_shm_resv_memory_node(const struct kernel_info *kinfo,
                                             int addrcells, int sizecells)
 {
diff --git a/xen/arch/arm/static-shmem.c b/xen/arch/arm/static-shmem.c
index 1f8441d92046..32ec6d4bc69f 100644
--- a/xen/arch/arm/static-shmem.c
+++ b/xen/arch/arm/static-shmem.c
@@ -822,71 +822,6 @@ int __init remove_shm_from_rangeset(const struct kernel_info *kinfo,
     return 0;
 }
 
-int __init remove_shm_holes_for_domU(const struct kernel_info *kinfo,
-                                     struct membanks *ext_regions)
-{
-    const struct membanks *shm_mem = kernel_info_get_shm_mem_const(kinfo);
-    struct rangeset *guest_holes;
-    unsigned int i;
-    paddr_t start;
-    paddr_t end;
-    int res;
-
-    /* No static shared memory region. */
-    if ( shm_mem->nr_banks == 0 )
-        return 0;
-
-    dt_dprintk("Remove static shared memory holes from extended regions of DomU\n");
-
-    guest_holes = rangeset_new(NULL, NULL, 0);
-    if ( !guest_holes )
-        return -ENOMEM;
-
-    /* Copy extended regions sets into the rangeset */
-    for ( i = 0; i < ext_regions->nr_banks; i++ )
-    {
-        start = ext_regions->bank[i].start;
-        end = start + ext_regions->bank[i].size;
-
-        res = rangeset_add_range(guest_holes, PFN_DOWN(start),
-                                 PFN_DOWN(end - 1));
-        if ( res )
-        {
-            printk(XENLOG_ERR
-                   "Failed to add: %#"PRIpaddr"->%#"PRIpaddr", error: %d\n",
-                   start, end, res);
-            goto out;
-        }
-    }
-
-    /* Remove static shared memory regions */
-    res = remove_shm_from_rangeset(kinfo, guest_holes);
-    if ( res )
-        goto out;
-
-    /*
-     * Take the interval of memory starting from the first extended region bank
-     * start address and ending to the end of the last extended region bank.
-     */
-    i = ext_regions->nr_banks - 1;
-    start = ext_regions->bank[0].start;
-    end = ext_regions->bank[i].start + ext_regions->bank[i].size - 1;
-
-    /* Reset original extended regions to hold new value */
-    ext_regions->nr_banks = 0;
-    res = rangeset_report_ranges(guest_holes, PFN_DOWN(start), PFN_DOWN(end),
-                                 add_ext_regions, ext_regions);
-    if ( res )
-        ext_regions->nr_banks = 0;
-    else if ( !ext_regions->nr_banks )
-        res = -ENOENT;
-
- out:
-    rangeset_destroy(guest_holes);
-
-    return res;
-}
-
 void __init shm_mem_node_fill_reg_range(const struct kernel_info *kinfo,
                                         __be32 *reg, int *nr_cells,
                                         int addrcells, int sizecells)
-- 
2.49.0

[PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Stewart Hildebrand]

Introduce rangeset_subtract() to remove regions in r2 from r1.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
v1->v2:
* no change
---
 xen/common/rangeset.c      | 12 ++++++++++++
 xen/include/xen/rangeset.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
index e75871039087..b9e8912fb1c3 100644
--- a/xen/common/rangeset.c
+++ b/xen/common/rangeset.c
@@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct rangeset *r2)
     return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
 }
 
+static int cf_check subtract(unsigned long s, unsigned long e, void *data)
+{
+    struct rangeset *r = data;
+
+    return rangeset_remove_range(r, s, e);
+}
+
+int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
+{
+    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
+}
+
 int rangeset_add_singleton(
     struct rangeset *r, unsigned long s)
 {
diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
index 96c918082501..817505badf6f 100644
--- a/xen/include/xen/rangeset.h
+++ b/xen/include/xen/rangeset.h
@@ -85,6 +85,9 @@ int rangeset_consume_ranges(struct rangeset *r,
 /* Merge rangeset r2 into rangeset r1. */
 int __must_check rangeset_merge(struct rangeset *r1, struct rangeset *r2);
 
+/* Subtract rangeset r2 from rangeset r1. */
+int __must_check rangeset_subtract(struct rangeset *r1, struct rangeset *r2);
+
 /* Add/remove/query a single number. */
 int __must_check rangeset_add_singleton(
     struct rangeset *r, unsigned long s);
-- 
2.49.0

[PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Stewart Hildebrand]

When a device is passed through to a dom0less domU, the xen,reg ranges
may overlap with the extended regions. Remove xen,reg from extended
regions.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Not sure if we need a Fixes: tag, but if we do:
Fixes: 2a2447757b3c ("xen/arm: implement domU extended regions")

v1->v2:
* adjust commit message to not mention xen,reg-cacheable
* don't call rangeset_destroy() in construct_dom0()
* rebase

I investigated an alternate approach of parsing the partial device tree
again to scan for xen,reg properties, but it resulted in quite a lot of
code duplication. Adding a rangeset pointer to "struct kernel_info" has
a much smaller diffstat, and then we avoid the need to parse the partial
device tree a second time.

I discovered this issue when booting a dom0less domU with a device
passed through. Partial device tree excerpt:

    passthrough {
        ... <snip> ...

        axi_uart16550_0: serial@a0001000 {
            clocks = <&uart_fixed_clk>;
            compatible = "ns16550a";
            interrupt-parent = <&gic>;
            interrupts = <0 89 4>;
            reg = <0x0 0xa0001000 0x0 0x1000>;
            reg-shift = <2>;

            xen,reg = <0x0 0xa0001000 0x00 0x1000 0x0 0xa0001000>;
            xen,path = "/amba_pl@0/serial@a0000000";
            xen,force-assign-without-iommu;
        };
    };

The domU was assigned an extended region overlapping with MMIO of the
passed through device:

(XEN) d1: extended region 0: 0xa0000000->0x100000000
(XEN) d1: extended region 1: 0x200000000->0xf000000000

The domU panicked when attempting to initialize the device:

[    3.490068] a0001000.serial: ttyS0 at MMIO 0xa0001000 (irq = 15, base_baud = 6249375) is a 16550A
[    3.498843] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
[    3.498853] Mem abort info:
[    3.498855]   ESR = 0x0000000096000044
[    3.498859]   EC = 0x25: DABT (current EL), IL = 32 bits
[    3.498864]   SET = 0, FnV = 0
[    3.498867]   EA = 0, S1PTW = 0
[    3.498870]   FSC = 0x04: level 0 translation fault
[    3.498874] Data abort info:
[    3.498876]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
[    3.498879]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
[    3.498884]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
[    3.498888] [0000000000000010] user address but active_mm is swapper
[    3.498894] Internal error: Oops: 0000000096000044 [#1] SMP
[    3.498899] Modules linked in:
[    3.498908] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.10-stew #1
[    3.498917] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    3.498924] pc : mem_serial_out+0x18/0x20
[    3.498936] lr : serial8250_do_set_mctrl+0x6c/0xc0
[    3.498943] sp : ffff800081bab6d0
[    3.498946] x29: ffff800081bab6d0 x28: ffff8000815e0dc8 x27: ffff000001c29c60
[    3.498957] x26: 0000000000000000 x25: ffff00000347b900 x24: ffff000005504c00
[    3.498968] x23: ffff00000347b800 x22: 0000000000000000 x21: ffff800081b69d78
[    3.498978] x20: ffff800081b69d78 x19: 0000000000000000 x18: ffffffffffffffff
[    3.498989] x17: 3d20647561625f65 x16: 736162202c353120 x15: 3d20717269282030
[    3.498999] x14: 3030313030306178 x13: ffff800081a21ff0 x12: 00000000000007fe
[    3.499010] x11: 00000000000002aa x10: ffff800081a4dff0 x9 : ffff800081a21ff0
[    3.499020] x8 : 00000000fffff7ff x7 : ffff800081a4dff0 x6 : 0000000000000008
[    3.499030] x5 : 0000000000000000 x4 : ffff800080797584 x3 : 0000000000000002
[    3.499040] x2 : 0000000000000000 x1 : 0000000000000010 x0 : 0000000000000000
[    3.499050] Call trace:
[    3.499053]  mem_serial_out+0x18/0x20
[    3.499059]  serial8250_set_mctrl+0x34/0x40
[    3.499065]  serial_core_register_port+0x534/0x7dc
[    3.499075]  serial_ctrl_register_port+0x10/0x1c
[    3.499084]  uart_add_one_port+0x10/0x1c
[    3.499092]  serial8250_register_8250_port+0x308/0x4c0
[    3.499102]  of_platform_serial_probe+0x2c4/0x48c
[    3.499110]  platform_probe+0x68/0xdc
[    3.499120]  really_probe+0xbc/0x298
[    3.499128]  __driver_probe_device+0x78/0x12c
[    3.499135]  driver_probe_device+0xdc/0x160
[    3.499142]  __driver_attach+0x94/0x19c
[    3.499149]  bus_for_each_dev+0x74/0xd0
[    3.499155]  driver_attach+0x24/0x30
[    3.499162]  bus_add_driver+0xe4/0x208
[    3.499168]  driver_register+0x60/0x128
[    3.499176]  __platform_driver_register+0x24/0x30
[    3.499184]  of_platform_serial_driver_init+0x1c/0x28
[    3.499192]  do_one_initcall+0x6c/0x1b0
[    3.499199]  kernel_init_freeable+0x178/0x258
[    3.499209]  kernel_init+0x20/0x1d0
[    3.499218]  ret_from_fork+0x10/0x20
[    3.499228] Code: f9400800 1ac32021 8b21c001 d50332bf (39000022)
[    3.499233] ---[ end trace 0000000000000000 ]---
[    3.499237] note: swapper/0[1] exited with irqs disabled
[    3.499247] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
[    3.499251] SMP: stopping secondary CPUs
[    3.499284] Kernel Offset: disabled
[    3.499286] CPU features: 0x00,00000080,00200000,0200420b
[    3.499292] Memory Limit: none
[    3.792412] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
---
 xen/arch/arm/domain_build.c             |  7 +++++++
 xen/common/device-tree/dom0less-build.c | 19 ++++++++++++++++++-
 xen/include/xen/fdt-kernel.h            |  1 +
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 05a77a4f92c6..b189a7cfae9f 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -973,6 +973,13 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     if ( res )
         goto out;
 
+    if ( kinfo->xen_reg_assigned )
+    {
+        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
+        if ( res )
+            goto out;
+    }
+
     res = rangeset_report_ranges(mem_holes, 0,
                                  PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
                                  add_ext_regions, ext_regions);
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 4aa36c8ef33f..2c56f13771ab 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -146,6 +146,14 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     int res;
     paddr_t mstart, size, gstart;
 
+    if ( !kinfo->xen_reg_assigned )
+    {
+        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
+
+        if ( !kinfo->xen_reg_assigned )
+            return -ENOMEM;
+    }
+
     /* xen,reg specifies where to map the MMIO region */
     cell = (const __be32 *)xen_reg->data;
     len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
@@ -187,6 +195,11 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
                    mstart, gstart);
             return -EFAULT;
         }
+
+        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
+                                 PFN_DOWN(gstart + size - 1));
+        if ( res )
+            return res;
     }
 
     /*
@@ -814,7 +827,11 @@ static int __init construct_domU(struct domain *d,
 
     domain_vcpu_affinity(d, node);
 
-    return alloc_xenstore_params(&kinfo);
+    rc = alloc_xenstore_params(&kinfo);
+
+    rangeset_destroy(kinfo.xen_reg_assigned);
+
+    return rc;
 }
 
 void __init create_domUs(void)
diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
index 7a6cd67c22f1..1939c3ebf7dc 100644
--- a/xen/include/xen/fdt-kernel.h
+++ b/xen/include/xen/fdt-kernel.h
@@ -24,6 +24,7 @@ struct kernel_info {
 #ifdef CONFIG_STATIC_SHM
     struct shared_meminfo shm_mem;
 #endif
+    struct rangeset *xen_reg_assigned;
 
     /* kernel entry point */
     paddr_t entry;
-- 
2.49.0

[PATCH v2 6/6] tools/arm: exclude iomem from domU extended regions

[Stewart Hildebrand]

When a device is passed through to a xl domU, the iomem ranges may
overlap with the extended regions. Remove iomem from extended regions.

Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
Not sure if we need a Fixes: tag, but if we do:
Fixes: 57f87857dc2d ("libxl/arm: Add handling of extended regions for DomU")

v1->v2:
* no change
---
 tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
 1 file changed, 99 insertions(+), 19 deletions(-)

diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
index 75c811053c7c..8ae16a1726fc 100644
--- a/tools/libs/light/libxl_arm.c
+++ b/tools/libs/light/libxl_arm.c
@@ -798,6 +798,8 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
     return 0;
 }
 
+#define MAX_NR_EXT_REGIONS   256
+
 static int make_hypervisor_node(libxl__gc *gc, void *fdt,
                                 const libxl_version_info *vers)
 {
@@ -821,7 +823,7 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
      */
     res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
                                        GUEST_ROOT_SIZE_CELLS,
-                                       GUEST_RAM_BANKS + 1);
+                                       MAX_NR_EXT_REGIONS + 1);
     if (res) return res;
 
     /*
@@ -1517,17 +1519,29 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
 
 #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
 
-static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
+static int compare_iomem(const void *a, const void *b)
+{
+    const libxl_iomem_range *x = a, *y = b;
+
+    if (x->gfn < y->gfn)
+        return -1;
+    if (x->gfn > y->gfn)
+        return 1;
+    return 0;
+}
+
+static int finalize_hypervisor_node(libxl__gc *gc,
+                                    libxl_domain_build_info *b_info,
+                                    struct xc_dom_image *dom)
 {
     void *fdt = dom->devicetree_blob;
-    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
-        bankend[GUEST_RAM_BANKS];
+    uint64_t region_base[MAX_NR_EXT_REGIONS], region_size[MAX_NR_EXT_REGIONS];
     uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
-                  (GUEST_RAM_BANKS + 1)];
+                  (MAX_NR_EXT_REGIONS + 1)];
     be32 *cells = &regs[0];
     const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
     const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
-    unsigned int i, len, nr_regions = 0;
+    unsigned int i, j, len, nr_regions = 0;
     libxl_dominfo info;
     int offset, rc;
 
@@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
     if (info.gpaddr_bits > 64)
         return ERROR_INVAL;
 
+    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
+          compare_iomem);
+
     /*
      * Try to allocate separate 2MB-aligned extended regions from the first
      * and second RAM banks taking into the account the maximum supported
      * guest physical address space size and the amount of memory assigned
      * to the guest.
      */
-    for (i = 0; i < GUEST_RAM_BANKS; i++) {
-        region_base[i] = bankbase[i] +
+    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
+        struct {
+            uint64_t start;
+            uint64_t end; /* inclusive */
+        } unallocated;
+        uint64_t size = 0;
+
+        unallocated.start = bankbase[i] +
             ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
 
-        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
-        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
-        if (bankend[i] > region_base[i])
-            region_size[i] = bankend[i] - region_base[i] + 1;
+        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
+        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
+
+        if (unallocated.end > unallocated.start)
+            size = unallocated.end - unallocated.start + 1;
+
+        if (size < EXT_REGION_MIN_SIZE)
+            continue;
+
+        /* Exclude iomem */
+        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
+             j++) {
+            struct {
+                uint64_t start;
+                uint64_t end; /* inclusive */
+            } iomem;
+
+            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
+            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
+                         << XC_PAGE_SHIFT) - 1;
+
+            if (iomem.end >= unallocated.start
+                && iomem.start <= unallocated.end) {
+
+                if (iomem.start <= unallocated.start) {
+                    unallocated.start = iomem.end + 1;
+
+                    if (iomem.end >= unallocated.end)
+                        /* Complete overlap, discard unallocated region */
+                        break;
+
+                    /* Beginning overlap */
+                    continue;
+                }
+
+                if (iomem.start > unallocated.start) {
+                    assert(unallocated.end > unallocated.start);
+                    size = iomem.start - unallocated.start;
+
+                    if (size >= EXT_REGION_MIN_SIZE) {
+                        region_base[nr_regions] = unallocated.start;
+                        region_size[nr_regions] = size;
+                        nr_regions++;
+                    }
+
+                    unallocated.start = iomem.end + 1;
+
+                    if (iomem.end >= unallocated.end)
+                        /* End overlap, discard remaining unallocated region */
+                        break;
+                }
+            }
+        }
+
+        if (unallocated.end > unallocated.start
+            && nr_regions < MAX_NR_EXT_REGIONS)
+        {
+            size = unallocated.end - unallocated.start + 1;
+
+            if (size >= EXT_REGION_MIN_SIZE) {
+                region_base[nr_regions] = unallocated.start;
+                region_size[nr_regions] = size;
+                nr_regions++;
+            }
+        }
     }
 
     /*
@@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
     set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
               GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
 
-    for (i = 0; i < GUEST_RAM_BANKS; i++) {
-        if (region_size[i] < EXT_REGION_MIN_SIZE)
-            continue;
-
+    for (i = 0; i < nr_regions; i++) {
         LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
-            nr_regions, region_base[i], region_base[i] + region_size[i]);
+            i, region_base[i], region_base[i] + region_size[i]);
 
         set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
                   region_base[i], region_size[i]);
-        nr_regions++;
     }
 
     if (!nr_regions)
@@ -1626,7 +1706,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
 
     }
 
-    res = finalize_hypervisor_node(gc, dom);
+    res = finalize_hypervisor_node(gc, &d_config->b_info, dom);
     if (res)
         return res;
 
-- 
2.49.0

Re: [PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Stefano Stabellini]

On Thu, 8 May 2025, Stewart Hildebrand wrote:
> When a device is passed through to a dom0less domU, the xen,reg ranges
> may overlap with the extended regions. Remove xen,reg from extended
> regions.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Not sure if we need a Fixes: tag, but if we do:
> Fixes: 2a2447757b3c ("xen/arm: implement domU extended regions")
> 
> v1->v2:
> * adjust commit message to not mention xen,reg-cacheable
> * don't call rangeset_destroy() in construct_dom0()
> * rebase
> 
> I investigated an alternate approach of parsing the partial device tree
> again to scan for xen,reg properties, but it resulted in quite a lot of
> code duplication. Adding a rangeset pointer to "struct kernel_info" has
> a much smaller diffstat, and then we avoid the need to parse the partial
> device tree a second time.
> 
> I discovered this issue when booting a dom0less domU with a device
> passed through. Partial device tree excerpt:
> 
>     passthrough {
>         ... <snip> ...
> 
>         axi_uart16550_0: serial@a0001000 {
>             clocks = <&uart_fixed_clk>;
>             compatible = "ns16550a";
>             interrupt-parent = <&gic>;
>             interrupts = <0 89 4>;
>             reg = <0x0 0xa0001000 0x0 0x1000>;
>             reg-shift = <2>;
> 
>             xen,reg = <0x0 0xa0001000 0x00 0x1000 0x0 0xa0001000>;
>             xen,path = "/amba_pl@0/serial@a0000000";
>             xen,force-assign-without-iommu;
>         };
>     };
> 
> The domU was assigned an extended region overlapping with MMIO of the
> passed through device:
> 
> (XEN) d1: extended region 0: 0xa0000000->0x100000000
> (XEN) d1: extended region 1: 0x200000000->0xf000000000
> 
> The domU panicked when attempting to initialize the device:
> 
> [    3.490068] a0001000.serial: ttyS0 at MMIO 0xa0001000 (irq = 15, base_baud = 6249375) is a 16550A
> [    3.498843] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> [    3.498853] Mem abort info:
> [    3.498855]   ESR = 0x0000000096000044
> [    3.498859]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    3.498864]   SET = 0, FnV = 0
> [    3.498867]   EA = 0, S1PTW = 0
> [    3.498870]   FSC = 0x04: level 0 translation fault
> [    3.498874] Data abort info:
> [    3.498876]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
> [    3.498879]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> [    3.498884]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    3.498888] [0000000000000010] user address but active_mm is swapper
> [    3.498894] Internal error: Oops: 0000000096000044 [#1] SMP
> [    3.498899] Modules linked in:
> [    3.498908] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.10-stew #1
> [    3.498917] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    3.498924] pc : mem_serial_out+0x18/0x20
> [    3.498936] lr : serial8250_do_set_mctrl+0x6c/0xc0
> [    3.498943] sp : ffff800081bab6d0
> [    3.498946] x29: ffff800081bab6d0 x28: ffff8000815e0dc8 x27: ffff000001c29c60
> [    3.498957] x26: 0000000000000000 x25: ffff00000347b900 x24: ffff000005504c00
> [    3.498968] x23: ffff00000347b800 x22: 0000000000000000 x21: ffff800081b69d78
> [    3.498978] x20: ffff800081b69d78 x19: 0000000000000000 x18: ffffffffffffffff
> [    3.498989] x17: 3d20647561625f65 x16: 736162202c353120 x15: 3d20717269282030
> [    3.498999] x14: 3030313030306178 x13: ffff800081a21ff0 x12: 00000000000007fe
> [    3.499010] x11: 00000000000002aa x10: ffff800081a4dff0 x9 : ffff800081a21ff0
> [    3.499020] x8 : 00000000fffff7ff x7 : ffff800081a4dff0 x6 : 0000000000000008
> [    3.499030] x5 : 0000000000000000 x4 : ffff800080797584 x3 : 0000000000000002
> [    3.499040] x2 : 0000000000000000 x1 : 0000000000000010 x0 : 0000000000000000
> [    3.499050] Call trace:
> [    3.499053]  mem_serial_out+0x18/0x20
> [    3.499059]  serial8250_set_mctrl+0x34/0x40
> [    3.499065]  serial_core_register_port+0x534/0x7dc
> [    3.499075]  serial_ctrl_register_port+0x10/0x1c
> [    3.499084]  uart_add_one_port+0x10/0x1c
> [    3.499092]  serial8250_register_8250_port+0x308/0x4c0
> [    3.499102]  of_platform_serial_probe+0x2c4/0x48c
> [    3.499110]  platform_probe+0x68/0xdc
> [    3.499120]  really_probe+0xbc/0x298
> [    3.499128]  __driver_probe_device+0x78/0x12c
> [    3.499135]  driver_probe_device+0xdc/0x160
> [    3.499142]  __driver_attach+0x94/0x19c
> [    3.499149]  bus_for_each_dev+0x74/0xd0
> [    3.499155]  driver_attach+0x24/0x30
> [    3.499162]  bus_add_driver+0xe4/0x208
> [    3.499168]  driver_register+0x60/0x128
> [    3.499176]  __platform_driver_register+0x24/0x30
> [    3.499184]  of_platform_serial_driver_init+0x1c/0x28
> [    3.499192]  do_one_initcall+0x6c/0x1b0
> [    3.499199]  kernel_init_freeable+0x178/0x258
> [    3.499209]  kernel_init+0x20/0x1d0
> [    3.499218]  ret_from_fork+0x10/0x20
> [    3.499228] Code: f9400800 1ac32021 8b21c001 d50332bf (39000022)
> [    3.499233] ---[ end trace 0000000000000000 ]---
> [    3.499237] note: swapper/0[1] exited with irqs disabled
> [    3.499247] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    3.499251] SMP: stopping secondary CPUs
> [    3.499284] Kernel Offset: disabled
> [    3.499286] CPU features: 0x00,00000080,00200000,0200420b
> [    3.499292] Memory Limit: none
> [    3.792412] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> ---
>  xen/arch/arm/domain_build.c             |  7 +++++++
>  xen/common/device-tree/dom0less-build.c | 19 ++++++++++++++++++-
>  xen/include/xen/fdt-kernel.h            |  1 +
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 05a77a4f92c6..b189a7cfae9f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -973,6 +973,13 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      if ( res )
>          goto out;
>  
> +    if ( kinfo->xen_reg_assigned )
> +    {
> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
> +        if ( res )
> +            goto out;
> +    }
> +
>      res = rangeset_report_ranges(mem_holes, 0,
>                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
>                                   add_ext_regions, ext_regions);
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 4aa36c8ef33f..2c56f13771ab 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -146,6 +146,14 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>      int res;
>      paddr_t mstart, size, gstart;
>  
> +    if ( !kinfo->xen_reg_assigned )
> +    {
> +        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
> +
> +        if ( !kinfo->xen_reg_assigned )
> +            return -ENOMEM;
> +    }
> +
>      /* xen,reg specifies where to map the MMIO region */
>      cell = (const __be32 *)xen_reg->data;
>      len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
> @@ -187,6 +195,11 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>                     mstart, gstart);
>              return -EFAULT;
>          }
> +
> +        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
> +                                 PFN_DOWN(gstart + size - 1));
> +        if ( res )
> +            return res;
>      }
>  
>      /*
> @@ -814,7 +827,11 @@ static int __init construct_domU(struct domain *d,
>  
>      domain_vcpu_affinity(d, node);
>  
> -    return alloc_xenstore_params(&kinfo);
> +    rc = alloc_xenstore_params(&kinfo);
> +
> +    rangeset_destroy(kinfo.xen_reg_assigned);
> +
> +    return rc;
>  }
>  
>  void __init create_domUs(void)
> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
> index 7a6cd67c22f1..1939c3ebf7dc 100644
> --- a/xen/include/xen/fdt-kernel.h
> +++ b/xen/include/xen/fdt-kernel.h
> @@ -24,6 +24,7 @@ struct kernel_info {
>  #ifdef CONFIG_STATIC_SHM
>      struct shared_meminfo shm_mem;
>  #endif
> +    struct rangeset *xen_reg_assigned;
>  
>      /* kernel entry point */
>      paddr_t entry;
> -- 
> 2.49.0
> 

Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Stefano Stabellini]

On Thu, 8 May 2025, Stewart Hildebrand wrote:
> Introduce rangeset_subtract() to remove regions in r2 from r1.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
> v1->v2:
> * no change
> ---
>  xen/common/rangeset.c      | 12 ++++++++++++
>  xen/include/xen/rangeset.h |  3 +++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/xen/common/rangeset.c b/xen/common/rangeset.c
> index e75871039087..b9e8912fb1c3 100644
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct rangeset *r2)
>      return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
>  }
>  
> +static int cf_check subtract(unsigned long s, unsigned long e, void *data)
> +{
> +    struct rangeset *r = data;
> +
> +    return rangeset_remove_range(r, s, e);
> +}
> +
> +int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
> +{
> +    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
> +}
> +
>  int rangeset_add_singleton(
>      struct rangeset *r, unsigned long s)
>  {
> diff --git a/xen/include/xen/rangeset.h b/xen/include/xen/rangeset.h
> index 96c918082501..817505badf6f 100644
> --- a/xen/include/xen/rangeset.h
> +++ b/xen/include/xen/rangeset.h
> @@ -85,6 +85,9 @@ int rangeset_consume_ranges(struct rangeset *r,
>  /* Merge rangeset r2 into rangeset r1. */
>  int __must_check rangeset_merge(struct rangeset *r1, struct rangeset *r2);
>  
> +/* Subtract rangeset r2 from rangeset r1. */
> +int __must_check rangeset_subtract(struct rangeset *r1, struct rangeset *r2);
> +
>  /* Add/remove/query a single number. */
>  int __must_check rangeset_add_singleton(
>      struct rangeset *r, unsigned long s);
> -- 
> 2.49.0
> 

Re: [PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Orzel, Michal]

On 08/05/2025 15:20, Stewart Hildebrand wrote:
> When a device is passed through to a dom0less domU, the xen,reg ranges
> may overlap with the extended regions. Remove xen,reg from extended
> regions.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> Not sure if we need a Fixes: tag, but if we do:
> Fixes: 2a2447757b3c ("xen/arm: implement domU extended regions")
> 
> v1->v2:
> * adjust commit message to not mention xen,reg-cacheable
> * don't call rangeset_destroy() in construct_dom0()
> * rebase
> 
> I investigated an alternate approach of parsing the partial device tree
> again to scan for xen,reg properties, but it resulted in quite a lot of
> code duplication. Adding a rangeset pointer to "struct kernel_info" has
> a much smaller diffstat, and then we avoid the need to parse the partial
> device tree a second time.
> 
> I discovered this issue when booting a dom0less domU with a device
> passed through. Partial device tree excerpt:
> 
>     passthrough {
>         ... <snip> ...
> 
>         axi_uart16550_0: serial@a0001000 {
>             clocks = <&uart_fixed_clk>;
>             compatible = "ns16550a";
>             interrupt-parent = <&gic>;
>             interrupts = <0 89 4>;
>             reg = <0x0 0xa0001000 0x0 0x1000>;
>             reg-shift = <2>;
> 
>             xen,reg = <0x0 0xa0001000 0x00 0x1000 0x0 0xa0001000>;
>             xen,path = "/amba_pl@0/serial@a0000000";
>             xen,force-assign-without-iommu;
>         };
>     };
> 
> The domU was assigned an extended region overlapping with MMIO of the
> passed through device:
> 
> (XEN) d1: extended region 0: 0xa0000000->0x100000000
> (XEN) d1: extended region 1: 0x200000000->0xf000000000
> 
> The domU panicked when attempting to initialize the device:
> 
> [    3.490068] a0001000.serial: ttyS0 at MMIO 0xa0001000 (irq = 15, base_baud = 6249375) is a 16550A
> [    3.498843] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000010
> [    3.498853] Mem abort info:
> [    3.498855]   ESR = 0x0000000096000044
> [    3.498859]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    3.498864]   SET = 0, FnV = 0
> [    3.498867]   EA = 0, S1PTW = 0
> [    3.498870]   FSC = 0x04: level 0 translation fault
> [    3.498874] Data abort info:
> [    3.498876]   ISV = 0, ISS = 0x00000044, ISS2 = 0x00000000
> [    3.498879]   CM = 0, WnR = 1, TnD = 0, TagAccess = 0
> [    3.498884]   GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0
> [    3.498888] [0000000000000010] user address but active_mm is swapper
> [    3.498894] Internal error: Oops: 0000000096000044 [#1] SMP
> [    3.498899] Modules linked in:
> [    3.498908] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.10-stew #1
> [    3.498917] pstate: 400000c5 (nZcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    3.498924] pc : mem_serial_out+0x18/0x20
> [    3.498936] lr : serial8250_do_set_mctrl+0x6c/0xc0
> [    3.498943] sp : ffff800081bab6d0
> [    3.498946] x29: ffff800081bab6d0 x28: ffff8000815e0dc8 x27: ffff000001c29c60
> [    3.498957] x26: 0000000000000000 x25: ffff00000347b900 x24: ffff000005504c00
> [    3.498968] x23: ffff00000347b800 x22: 0000000000000000 x21: ffff800081b69d78
> [    3.498978] x20: ffff800081b69d78 x19: 0000000000000000 x18: ffffffffffffffff
> [    3.498989] x17: 3d20647561625f65 x16: 736162202c353120 x15: 3d20717269282030
> [    3.498999] x14: 3030313030306178 x13: ffff800081a21ff0 x12: 00000000000007fe
> [    3.499010] x11: 00000000000002aa x10: ffff800081a4dff0 x9 : ffff800081a21ff0
> [    3.499020] x8 : 00000000fffff7ff x7 : ffff800081a4dff0 x6 : 0000000000000008
> [    3.499030] x5 : 0000000000000000 x4 : ffff800080797584 x3 : 0000000000000002
> [    3.499040] x2 : 0000000000000000 x1 : 0000000000000010 x0 : 0000000000000000
> [    3.499050] Call trace:
> [    3.499053]  mem_serial_out+0x18/0x20
> [    3.499059]  serial8250_set_mctrl+0x34/0x40
> [    3.499065]  serial_core_register_port+0x534/0x7dc
> [    3.499075]  serial_ctrl_register_port+0x10/0x1c
> [    3.499084]  uart_add_one_port+0x10/0x1c
> [    3.499092]  serial8250_register_8250_port+0x308/0x4c0
> [    3.499102]  of_platform_serial_probe+0x2c4/0x48c
> [    3.499110]  platform_probe+0x68/0xdc
> [    3.499120]  really_probe+0xbc/0x298
> [    3.499128]  __driver_probe_device+0x78/0x12c
> [    3.499135]  driver_probe_device+0xdc/0x160
> [    3.499142]  __driver_attach+0x94/0x19c
> [    3.499149]  bus_for_each_dev+0x74/0xd0
> [    3.499155]  driver_attach+0x24/0x30
> [    3.499162]  bus_add_driver+0xe4/0x208
> [    3.499168]  driver_register+0x60/0x128
> [    3.499176]  __platform_driver_register+0x24/0x30
> [    3.499184]  of_platform_serial_driver_init+0x1c/0x28
> [    3.499192]  do_one_initcall+0x6c/0x1b0
> [    3.499199]  kernel_init_freeable+0x178/0x258
> [    3.499209]  kernel_init+0x20/0x1d0
> [    3.499218]  ret_from_fork+0x10/0x20
> [    3.499228] Code: f9400800 1ac32021 8b21c001 d50332bf (39000022)
> [    3.499233] ---[ end trace 0000000000000000 ]---
> [    3.499237] note: swapper/0[1] exited with irqs disabled
> [    3.499247] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    3.499251] SMP: stopping secondary CPUs
> [    3.499284] Kernel Offset: disabled
> [    3.499286] CPU features: 0x00,00000080,00200000,0200420b
> [    3.499292] Memory Limit: none
> [    3.792412] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]---
> ---
>  xen/arch/arm/domain_build.c             |  7 +++++++
>  xen/common/device-tree/dom0less-build.c | 19 ++++++++++++++++++-
>  xen/include/xen/fdt-kernel.h            |  1 +
>  3 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 05a77a4f92c6..b189a7cfae9f 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -973,6 +973,13 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      if ( res )
>          goto out;
>  
> +    if ( kinfo->xen_reg_assigned )
> +    {
> +        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
> +        if ( res )
> +            goto out;
> +    }
> +
>      res = rangeset_report_ranges(mem_holes, 0,
>                                   PFN_DOWN((1ULL << p2m_ipa_bits) - 1),
>                                   add_ext_regions, ext_regions);
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 4aa36c8ef33f..2c56f13771ab 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -146,6 +146,14 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>      int res;
>      paddr_t mstart, size, gstart;
>  
> +    if ( !kinfo->xen_reg_assigned )
> +    {
> +        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
> +
> +        if ( !kinfo->xen_reg_assigned )
> +            return -ENOMEM;
> +    }
> +
>      /* xen,reg specifies where to map the MMIO region */
>      cell = (const __be32 *)xen_reg->data;
>      len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
> @@ -187,6 +195,11 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>                     mstart, gstart);
>              return -EFAULT;
>          }
> +
> +        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
> +                                 PFN_DOWN(gstart + size - 1));
> +        if ( res )
> +            return res;
>      }
>  
>      /*
> @@ -814,7 +827,11 @@ static int __init construct_domU(struct domain *d,
>  
>      domain_vcpu_affinity(d, node);
>  
> -    return alloc_xenstore_params(&kinfo);
> +    rc = alloc_xenstore_params(&kinfo);
> +
> +    rangeset_destroy(kinfo.xen_reg_assigned);
> +
> +    return rc;
>  }
>  
>  void __init create_domUs(void)
> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
> index 7a6cd67c22f1..1939c3ebf7dc 100644
> --- a/xen/include/xen/fdt-kernel.h
> +++ b/xen/include/xen/fdt-kernel.h
> @@ -24,6 +24,7 @@ struct kernel_info {
>  #ifdef CONFIG_STATIC_SHM
>      struct shared_meminfo shm_mem;
>  #endif
> +    struct rangeset *xen_reg_assigned;
The purpose of your newly introduced xen_reg_assigned is to keep track of these
ranges so that we can remove them from extended regions. The concept of extended
regions exists only for Arm today. Therefore I'm not sure why making all these
common i.e. entry in struct, rangeset allocation, etc. The other aspect is that
extended regions may be disabled by the user and you would still allocate
rangeset and add xen,reg to it for no purpose - i.e. dead code.

Also, what about direct-mapped domUs? We don't seem to take xen,reg into account
there.

P.S.
After recent dom0less code movement there are some issues that I reported to
Oleksii. Long story short, we shouldn't be making the code common (e.g. static
mem, shmem, domain type) that is implemented for now only for one arch. If the
need arises in the future, the feature code together with callers can be moved
to common. At the moment, we have some features being in arch specific
directories but callers in common code and #ifdef-ed (making the stubs
unreachable). That's not great.

~Michal

Re: [PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Stewart Hildebrand]

On 5/9/25 02:54, Orzel, Michal wrote:
> On 08/05/2025 15:20, Stewart Hildebrand wrote:
>> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
>> index 7a6cd67c22f1..1939c3ebf7dc 100644
>> --- a/xen/include/xen/fdt-kernel.h
>> +++ b/xen/include/xen/fdt-kernel.h
>> @@ -24,6 +24,7 @@ struct kernel_info {
>>  #ifdef CONFIG_STATIC_SHM
>>      struct shared_meminfo shm_mem;
>>  #endif
>> +    struct rangeset *xen_reg_assigned;
> The purpose of your newly introduced xen_reg_assigned is to keep track of these
> ranges so that we can remove them from extended regions. The concept of extended
> regions exists only for Arm today. Therefore I'm not sure why making all these
> common i.e. entry in struct, rangeset allocation, etc. The other aspect is that
> extended regions may be disabled by the user and you would still allocate
> rangeset and add xen,reg to it for no purpose - i.e. dead code.

How about an arch hook? E.g. see work-in-progress/untested patch at the
end.

> Also, what about direct-mapped domUs? We don't seem to take xen,reg into account
> there.

Right, we ought to take xen,reg into account for direct-map domUs too.
This is because, even though the domU is direct-mapped, xen,reg can
still set up a translated mapping (gfn != mfn). Also, xen,reg doesn't
need to correspond to a real device, it can be any arbitrary mapping.
I'll send a patch.

> P.S.
> After recent dom0less code movement there are some issues that I reported to
> Oleksii. Long story short, we shouldn't be making the code common (e.g. static
> mem, shmem, domain type) that is implemented for now only for one arch. If the
> need arises in the future, the feature code together with callers can be moved
> to common. At the moment, we have some features being in arch specific
> directories but callers in common code and #ifdef-ed (making the stubs
> unreachable). That's not great.
> 
> ~Michal



diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b189a7cfae9f..f099e27d846c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -929,6 +929,31 @@ out:
     return res;
 }
 
+int __init arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart,
+                            paddr_t size)
+{
+    if ( !opt_ext_regions )
+        return 0;
+
+    if ( !kinfo->arch.xen_reg_assigned )
+    {
+        kinfo->arch.xen_reg_assigned = rangeset_new(NULL, NULL, 0);
+
+        if ( !kinfo->arch.xen_reg_assigned )
+            return -ENOMEM;
+    }
+
+    return rangeset_add_range(kinfo->arch.xen_reg_assigned, PFN_DOWN(gstart),
+                              PFN_DOWN(gstart + size - 1));
+}
+
+int __init arch_cleanup(struct kernel_info *kinfo)
+{
+    rangeset_destroy(kinfo->arch.xen_reg_assigned);
+
+    return 0;
+}
+
 static int __init find_domU_holes(const struct kernel_info *kinfo,
                                   struct membanks *ext_regions)
 {
@@ -973,9 +998,9 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
     if ( res )
         goto out;
 
-    if ( kinfo->xen_reg_assigned )
+    if ( kinfo->arch.xen_reg_assigned )
     {
-        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
+        res = rangeset_subtract(mem_holes, kinfo->arch.xen_reg_assigned);
         if ( res )
             goto out;
     }
diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
index 7c3b7fde5b64..8d6bd2dd77f9 100644
--- a/xen/arch/arm/include/asm/kernel.h
+++ b/xen/arch/arm/include/asm/kernel.h
@@ -16,6 +16,8 @@ struct arch_kernel_info
 
     /* Enable pl011 emulation */
     bool vpl011;
+
+    struct rangeset *xen_reg_assigned;
 };
 
 #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index 2c56f13771ab..654575612744 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -146,14 +146,6 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
     int res;
     paddr_t mstart, size, gstart;
 
-    if ( !kinfo->xen_reg_assigned )
-    {
-        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
-
-        if ( !kinfo->xen_reg_assigned )
-            return -ENOMEM;
-    }
-
     /* xen,reg specifies where to map the MMIO region */
     cell = (const __be32 *)xen_reg->data;
     len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
@@ -196,8 +188,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
             return -EFAULT;
         }
 
-        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
-                                 PFN_DOWN(gstart + size - 1));
+        res = arch_add_xen_reg(kinfo, gstart, size);
         if ( res )
             return res;
     }
@@ -828,10 +819,10 @@ static int __init construct_domU(struct domain *d,
     domain_vcpu_affinity(d, node);
 
     rc = alloc_xenstore_params(&kinfo);
+    if ( rc < 0 )
+        return rc;
 
-    rangeset_destroy(kinfo.xen_reg_assigned);
-
-    return rc;
+    return arch_cleanup(&kinfo);
 }
 
 void __init create_domUs(void)
diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
index e0ad0429ec74..3e577e4dbe10 100644
--- a/xen/include/asm-generic/dom0less-build.h
+++ b/xen/include/asm-generic/dom0less-build.h
@@ -61,6 +61,10 @@ void set_domain_type(struct domain *d, struct kernel_info *kinfo);
 int init_intc_phandle(struct kernel_info *kinfo, const char *name,
                       const int node_next, const void *pfdt);
 
+int arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart, paddr_t size);
+
+int arch_cleanup(struct kernel_info *kinfo);
+
 #else /* !CONFIG_DOM0LESS_BOOT */
 
 static inline void create_domUs(void) {}
diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
index 1939c3ebf7dc..7a6cd67c22f1 100644
--- a/xen/include/xen/fdt-kernel.h
+++ b/xen/include/xen/fdt-kernel.h
@@ -24,7 +24,6 @@ struct kernel_info {
 #ifdef CONFIG_STATIC_SHM
     struct shared_meminfo shm_mem;
 #endif
-    struct rangeset *xen_reg_assigned;
 
     /* kernel entry point */
     paddr_t entry;

Re: [PATCH v2 5/6] xen/arm: exclude xen,reg from domU extended regions

[Orzel, Michal]

On 12/05/2025 21:55, Stewart Hildebrand wrote:
> On 5/9/25 02:54, Orzel, Michal wrote:
>> On 08/05/2025 15:20, Stewart Hildebrand wrote:
>>> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
>>> index 7a6cd67c22f1..1939c3ebf7dc 100644
>>> --- a/xen/include/xen/fdt-kernel.h
>>> +++ b/xen/include/xen/fdt-kernel.h
>>> @@ -24,6 +24,7 @@ struct kernel_info {
>>>  #ifdef CONFIG_STATIC_SHM
>>>      struct shared_meminfo shm_mem;
>>>  #endif
>>> +    struct rangeset *xen_reg_assigned;
>> The purpose of your newly introduced xen_reg_assigned is to keep track of these
>> ranges so that we can remove them from extended regions. The concept of extended
>> regions exists only for Arm today. Therefore I'm not sure why making all these
>> common i.e. entry in struct, rangeset allocation, etc. The other aspect is that
>> extended regions may be disabled by the user and you would still allocate
>> rangeset and add xen,reg to it for no purpose - i.e. dead code.
> 
> How about an arch hook? E.g. see work-in-progress/untested patch at the
> end.
Still, this is only needed for extended regions and your solution a) does not
mention this fact at all and b) assumes that other arches (let's focus on RISCV
for now) have a plan to use it in the future. If b) is true (I'm not sure
because Oleksii did not move this code to common), then making the hooks global
while extended regions creation logic still being under /arm does not seem
beneficial.

> 
>> Also, what about direct-mapped domUs? We don't seem to take xen,reg into account
>> there.
> 
> Right, we ought to take xen,reg into account for direct-map domUs too.
> This is because, even though the domU is direct-mapped, xen,reg can
> still set up a translated mapping (gfn != mfn). Also, xen,reg doesn't
> need to correspond to a real device, it can be any arbitrary mapping.
> I'll send a patch.
> 
>> P.S.
>> After recent dom0less code movement there are some issues that I reported to
>> Oleksii. Long story short, we shouldn't be making the code common (e.g. static
>> mem, shmem, domain type) that is implemented for now only for one arch. If the
>> need arises in the future, the feature code together with callers can be moved
>> to common. At the moment, we have some features being in arch specific
>> directories but callers in common code and #ifdef-ed (making the stubs
>> unreachable). That's not great.
>>
>> ~Michal
> 
> 
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b189a7cfae9f..f099e27d846c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -929,6 +929,31 @@ out:
>      return res;
>  }
>  
> +int __init arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart,
> +                            paddr_t size)
> +{
> +    if ( !opt_ext_regions )
> +        return 0;
> +
> +    if ( !kinfo->arch.xen_reg_assigned )
> +    {
> +        kinfo->arch.xen_reg_assigned = rangeset_new(NULL, NULL, 0);
> +
> +        if ( !kinfo->arch.xen_reg_assigned )
> +            return -ENOMEM;
> +    }
> +
> +    return rangeset_add_range(kinfo->arch.xen_reg_assigned, PFN_DOWN(gstart),
> +                              PFN_DOWN(gstart + size - 1));
> +}
> +
> +int __init arch_cleanup(struct kernel_info *kinfo)
> +{
> +    rangeset_destroy(kinfo->arch.xen_reg_assigned);
> +
> +    return 0;
> +}
> +
>  static int __init find_domU_holes(const struct kernel_info *kinfo,
>                                    struct membanks *ext_regions)
>  {
> @@ -973,9 +998,9 @@ static int __init find_domU_holes(const struct kernel_info *kinfo,
>      if ( res )
>          goto out;
>  
> -    if ( kinfo->xen_reg_assigned )
> +    if ( kinfo->arch.xen_reg_assigned )
>      {
> -        res = rangeset_subtract(mem_holes, kinfo->xen_reg_assigned);
> +        res = rangeset_subtract(mem_holes, kinfo->arch.xen_reg_assigned);
>          if ( res )
>              goto out;
>      }
> diff --git a/xen/arch/arm/include/asm/kernel.h b/xen/arch/arm/include/asm/kernel.h
> index 7c3b7fde5b64..8d6bd2dd77f9 100644
> --- a/xen/arch/arm/include/asm/kernel.h
> +++ b/xen/arch/arm/include/asm/kernel.h
> @@ -16,6 +16,8 @@ struct arch_kernel_info
>  
>      /* Enable pl011 emulation */
>      bool vpl011;
> +
> +    struct rangeset *xen_reg_assigned;
>  };
>  
>  #endif /* #ifdef __ARCH_ARM_KERNEL_H__ */
> diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
> index 2c56f13771ab..654575612744 100644
> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -146,14 +146,6 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>      int res;
>      paddr_t mstart, size, gstart;
>  
> -    if ( !kinfo->xen_reg_assigned )
> -    {
> -        kinfo->xen_reg_assigned = rangeset_new(NULL, NULL, 0);
> -
> -        if ( !kinfo->xen_reg_assigned )
> -            return -ENOMEM;
> -    }
> -
>      /* xen,reg specifies where to map the MMIO region */
>      cell = (const __be32 *)xen_reg->data;
>      len = fdt32_to_cpu(xen_reg->len) / ((address_cells * 2 + size_cells) *
> @@ -196,8 +188,7 @@ static int __init handle_passthrough_prop(struct kernel_info *kinfo,
>              return -EFAULT;
>          }
>  
> -        res = rangeset_add_range(kinfo->xen_reg_assigned, PFN_DOWN(gstart),
> -                                 PFN_DOWN(gstart + size - 1));
> +        res = arch_add_xen_reg(kinfo, gstart, size);
>          if ( res )
>              return res;
>      }
> @@ -828,10 +819,10 @@ static int __init construct_domU(struct domain *d,
>      domain_vcpu_affinity(d, node);
>  
>      rc = alloc_xenstore_params(&kinfo);
> +    if ( rc < 0 )
> +        return rc;
>  
> -    rangeset_destroy(kinfo.xen_reg_assigned);
> -
> -    return rc;
> +    return arch_cleanup(&kinfo);
>  }
>  
>  void __init create_domUs(void)
> diff --git a/xen/include/asm-generic/dom0less-build.h b/xen/include/asm-generic/dom0less-build.h
> index e0ad0429ec74..3e577e4dbe10 100644
> --- a/xen/include/asm-generic/dom0less-build.h
> +++ b/xen/include/asm-generic/dom0less-build.h
> @@ -61,6 +61,10 @@ void set_domain_type(struct domain *d, struct kernel_info *kinfo);
>  int init_intc_phandle(struct kernel_info *kinfo, const char *name,
>                        const int node_next, const void *pfdt);
>  
> +int arch_add_xen_reg(struct kernel_info *kinfo, paddr_t gstart, paddr_t size);
> +
> +int arch_cleanup(struct kernel_info *kinfo);
> +
>  #else /* !CONFIG_DOM0LESS_BOOT */
>  
>  static inline void create_domUs(void) {}
> diff --git a/xen/include/xen/fdt-kernel.h b/xen/include/xen/fdt-kernel.h
> index 1939c3ebf7dc..7a6cd67c22f1 100644
> --- a/xen/include/xen/fdt-kernel.h
> +++ b/xen/include/xen/fdt-kernel.h
> @@ -24,7 +24,6 @@ struct kernel_info {
>  #ifdef CONFIG_STATIC_SHM
>      struct shared_meminfo shm_mem;
>  #endif
> -    struct rangeset *xen_reg_assigned;
>  
>      /* kernel entry point */
>      paddr_t entry;

~Michal

Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Jan Beulich]

On 08.05.2025 15:20, Stewart Hildebrand wrote:
> --- a/xen/common/rangeset.c
> +++ b/xen/common/rangeset.c
> @@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct rangeset *r2)
>      return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
>  }
>  
> +static int cf_check subtract(unsigned long s, unsigned long e, void *data)
> +{
> +    struct rangeset *r = data;
> +
> +    return rangeset_remove_range(r, s, e);
> +}
> +
> +int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
> +{
> +    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
> +}

I understand this was committed already, but I don't understand why: This
introduces a Misra rule 2.1 violation aiui. The rule isn't tagged as clean
yet, but it was accepted and hence I thought we would strive towards not
introducing new violations. What's the deal?

Jan

Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Stewart Hildebrand]

On 5/13/25 11:39, Jan Beulich wrote:
> On 08.05.2025 15:20, Stewart Hildebrand wrote:
>> --- a/xen/common/rangeset.c
>> +++ b/xen/common/rangeset.c
>> @@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct rangeset *r2)
>>      return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
>>  }
>>  
>> +static int cf_check subtract(unsigned long s, unsigned long e, void *data)
>> +{
>> +    struct rangeset *r = data;
>> +
>> +    return rangeset_remove_range(r, s, e);
>> +}
>> +
>> +int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
>> +{
>> +    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
>> +}
> 
> I understand this was committed already, but I don't understand why: This
> introduces a Misra rule 2.1 violation aiui. The rule isn't tagged as clean
> yet, but it was accepted and hence I thought we would strive towards not
> introducing new violations. What's the deal?
> 
> Jan

The very next patch (also committed) makes use of the function, so the
series as a whole did not introduce a violation. Our code review
guidelines still say to organize new independent helper functions into
logically separate patches [0]. To be clear, and for future reference,
would your expectation be to squash the introduction of the helper
function into the patch where it's used? Perhaps we ought to finally
update the code review guidelines...

[0] https://xenbits.xenproject.org/governance/code-review-guide.html

Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Jan Beulich]

On 13.05.2025 19:01, Stewart Hildebrand wrote:
> On 5/13/25 11:39, Jan Beulich wrote:
>> On 08.05.2025 15:20, Stewart Hildebrand wrote:
>>> --- a/xen/common/rangeset.c
>>> +++ b/xen/common/rangeset.c
>>> @@ -397,6 +397,18 @@ int rangeset_merge(struct rangeset *r1, struct rangeset *r2)
>>>      return rangeset_report_ranges(r2, 0, ~0UL, merge, r1);
>>>  }
>>>  
>>> +static int cf_check subtract(unsigned long s, unsigned long e, void *data)
>>> +{
>>> +    struct rangeset *r = data;
>>> +
>>> +    return rangeset_remove_range(r, s, e);
>>> +}
>>> +
>>> +int rangeset_subtract(struct rangeset *r1, struct rangeset *r2)
>>> +{
>>> +    return rangeset_report_ranges(r2, 0, ~0UL, subtract, r1);
>>> +}
>>
>> I understand this was committed already, but I don't understand why: This
>> introduces a Misra rule 2.1 violation aiui. The rule isn't tagged as clean
>> yet, but it was accepted and hence I thought we would strive towards not
>> introducing new violations. What's the deal?
> 
> The very next patch (also committed) makes use of the function, so the
> series as a whole did not introduce a violation. Our code review
> guidelines still say to organize new independent helper functions into
> logically separate patches [0]. To be clear, and for future reference,
> would your expectation be to squash the introduction of the helper
> function into the patch where it's used?

Well, it's not so much my than Misra's expectation. With a small helper
like the one here folding certainly wouldn't have caused much of a
headache, yet I agree things can be different when the helper is quite
a bit larger; some re-arrangements may be necessary to make in such a
situation. And yes, imo ...

> Perhaps we ought to finally
> update the code review guidelines...
> 
> [0] https://xenbits.xenproject.org/governance/code-review-guide.html

... the guidelines better wouldn't be in conflict with Misra requirements.

Jan

Re: [PATCH v2 1/6] xen/arm: fix math in add_ext_regions

[Julien Grall]

Hi Stewart,

On 08/05/2025 14:20, Stewart Hildebrand wrote:
> In commit f37a59813979, the arguments to add_ext_regions() were switched
> from addresses to frame numbers. add_ext_regions() converts the frame
> numbers back to addresses, but the end address (e) is rounded down to
> page size alignment. The logic to calculate the size assumes e points to
> the last address, not page, effectively leading to the region size being
> erroneously calculated to be 2M smaller than the actual size of the
> region.
> 
> Fix by adding 1 to the frame number before converting back to address.
> 
> Fixes: f37a59813979 ("xen/arm: domain_build: Track unallocated pages using the frame number")
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> Acked-by: Michal Orzel <michal.orzel@amd.com>
> ---
> v1->v2:
> * add Michal's A-b
> ---
>   xen/arch/arm/domain_build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index df29619c4007..2f2b021dec3e 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -761,7 +761,7 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>       struct membanks *ext_regions = data;
>       paddr_t start, size;
>       paddr_t s = pfn_to_paddr(s_gfn);
> -    paddr_t e = pfn_to_paddr(e_gfn);
> +    paddr_t e = pfn_to_paddr(e_gfn + 1) - 1;

I noticed this patch. While reading the function, I noticed this would 
result to some confusing code:


     if ( start > e )
         return 0;

     [...]

     /*
      * e is actually "end-1" because it is called by rangeset functions
      * which are inclusive of the last address.
      */
     e += 1;
     size = (e - start) & ~(SZ_2M - 1);

So substract 1, but then re-add after. Is there any reason we didn't 
adjust the rest of the code?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 4/6] rangeset: introduce rangeset_subtract

[Roger Pau Monné]

On Thu, May 08, 2025 at 09:20:33AM -0400, Stewart Hildebrand wrote:
> Introduce rangeset_subtract() to remove regions in r2 from r1.

Oh, you could have replaced the code in arch_iommu_hwdom_init() to
make use of this new helper.  I will prepare a patch now.

Regards, Roger.

Re: [PATCH v2 6/6] tools/arm: exclude iomem from domU extended regions

[Stefano Stabellini]

On Thu, 8 May 2025, Stewart Hildebrand wrote:
> When a device is passed through to a xl domU, the iomem ranges may
> overlap with the extended regions. Remove iomem from extended regions.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
> Not sure if we need a Fixes: tag, but if we do:
> Fixes: 57f87857dc2d ("libxl/arm: Add handling of extended regions for DomU")
> 
> v1->v2:
> * no change
> ---
>  tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
>  1 file changed, 99 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 75c811053c7c..8ae16a1726fc 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -798,6 +798,8 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +#define MAX_NR_EXT_REGIONS   256
> +
>  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>                                  const libxl_version_info *vers)
>  {
> @@ -821,7 +823,7 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>       */
>      res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>                                         GUEST_ROOT_SIZE_CELLS,
> -                                       GUEST_RAM_BANKS + 1);
> +                                       MAX_NR_EXT_REGIONS + 1);
>      if (res) return res;
>  
>      /*
> @@ -1517,17 +1519,29 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
>  
>  #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>  
> -static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
> +static int compare_iomem(const void *a, const void *b)
> +{
> +    const libxl_iomem_range *x = a, *y = b;
> +
> +    if (x->gfn < y->gfn)
> +        return -1;
> +    if (x->gfn > y->gfn)
> +        return 1;
> +    return 0;
> +}
> +
> +static int finalize_hypervisor_node(libxl__gc *gc,
> +                                    libxl_domain_build_info *b_info,
> +                                    struct xc_dom_image *dom)
>  {
>      void *fdt = dom->devicetree_blob;
> -    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
> -        bankend[GUEST_RAM_BANKS];
> +    uint64_t region_base[MAX_NR_EXT_REGIONS], region_size[MAX_NR_EXT_REGIONS];
>      uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> -                  (GUEST_RAM_BANKS + 1)];
> +                  (MAX_NR_EXT_REGIONS + 1)];
>      be32 *cells = &regs[0];
>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>      const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> -    unsigned int i, len, nr_regions = 0;
> +    unsigned int i, j, len, nr_regions = 0;
>      libxl_dominfo info;
>      int offset, rc;
>  
> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      if (info.gpaddr_bits > 64)
>          return ERROR_INVAL;
>  
> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
> +          compare_iomem);
> +
>      /*
>       * Try to allocate separate 2MB-aligned extended regions from the first
>       * and second RAM banks taking into the account the maximum supported
>       * guest physical address space size and the amount of memory assigned
>       * to the guest.
>       */
> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> -        region_base[i] = bankbase[i] +
> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
> +        struct {
> +            uint64_t start;
> +            uint64_t end; /* inclusive */
> +        } unallocated;
> +        uint64_t size = 0;
> +
> +        unallocated.start = bankbase[i] +
>              ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
>  
> -        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> -        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> -        if (bankend[i] > region_base[i])
> -            region_size[i] = bankend[i] - region_base[i] + 1;
> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
> +
> +        if (unallocated.end > unallocated.start)
> +            size = unallocated.end - unallocated.start + 1;
> +
> +        if (size < EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        /* Exclude iomem */
> +        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
> +             j++) {
> +            struct {
> +                uint64_t start;
> +                uint64_t end; /* inclusive */
> +            } iomem;
> +
> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
> +                         << XC_PAGE_SHIFT) - 1;
> +
> +            if (iomem.end >= unallocated.start
> +                && iomem.start <= unallocated.end) {
> +
> +                if (iomem.start <= unallocated.start) {
> +                    unallocated.start = iomem.end + 1;
> +
> +                    if (iomem.end >= unallocated.end)
> +                        /* Complete overlap, discard unallocated region */
> +                        break;
> +
> +                    /* Beginning overlap */
> +                    continue;
> +                }
> +
> +                if (iomem.start > unallocated.start) {
> +                    assert(unallocated.end > unallocated.start);
> +                    size = iomem.start - unallocated.start;
> +
> +                    if (size >= EXT_REGION_MIN_SIZE) {
> +                        region_base[nr_regions] = unallocated.start;
> +                        region_size[nr_regions] = size;
> +                        nr_regions++;
> +                    }
> +
> +                    unallocated.start = iomem.end + 1;
> +
> +                    if (iomem.end >= unallocated.end)
> +                        /* End overlap, discard remaining unallocated region */
> +                        break;
> +                }
> +            }
> +        }
> +
> +        if (unallocated.end > unallocated.start
> +            && nr_regions < MAX_NR_EXT_REGIONS)
> +        {
> +            size = unallocated.end - unallocated.start + 1;
> +
> +            if (size >= EXT_REGION_MIN_SIZE) {
> +                region_base[nr_regions] = unallocated.start;
> +                region_size[nr_regions] = size;
> +                nr_regions++;
> +            }
> +        }
>      }
>  
>      /*
> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>                GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>  
> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> -        if (region_size[i] < EXT_REGION_MIN_SIZE)
> -            continue;
> -
> +    for (i = 0; i < nr_regions; i++) {
>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +            i, region_base[i], region_base[i] + region_size[i]);
>  
>          set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>                    region_base[i], region_size[i]);
> -        nr_regions++;
>      }
>  
>      if (!nr_regions)
> @@ -1626,7 +1706,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>  
>      }
>  
> -    res = finalize_hypervisor_node(gc, dom);
> +    res = finalize_hypervisor_node(gc, &d_config->b_info, dom);
>      if (res)
>          return res;
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH v2 6/6] tools/arm: exclude iomem from domU extended regions

[Stefano Stabellini]

I plan to commit this patch, unless someone objects

On Thu, 8 May 2025, Stewart Hildebrand wrote:
> When a device is passed through to a xl domU, the iomem ranges may
> overlap with the extended regions. Remove iomem from extended regions.
> 
> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
> ---
> Not sure if we need a Fixes: tag, but if we do:
> Fixes: 57f87857dc2d ("libxl/arm: Add handling of extended regions for DomU")
> 
> v1->v2:
> * no change
> ---
>  tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
>  1 file changed, 99 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
> index 75c811053c7c..8ae16a1726fc 100644
> --- a/tools/libs/light/libxl_arm.c
> +++ b/tools/libs/light/libxl_arm.c
> @@ -798,6 +798,8 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>      return 0;
>  }
>  
> +#define MAX_NR_EXT_REGIONS   256
> +
>  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>                                  const libxl_version_info *vers)
>  {
> @@ -821,7 +823,7 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>       */
>      res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>                                         GUEST_ROOT_SIZE_CELLS,
> -                                       GUEST_RAM_BANKS + 1);
> +                                       MAX_NR_EXT_REGIONS + 1);
>      if (res) return res;
>  
>      /*
> @@ -1517,17 +1519,29 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
>  
>  #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>  
> -static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
> +static int compare_iomem(const void *a, const void *b)
> +{
> +    const libxl_iomem_range *x = a, *y = b;
> +
> +    if (x->gfn < y->gfn)
> +        return -1;
> +    if (x->gfn > y->gfn)
> +        return 1;
> +    return 0;
> +}
> +
> +static int finalize_hypervisor_node(libxl__gc *gc,
> +                                    libxl_domain_build_info *b_info,
> +                                    struct xc_dom_image *dom)
>  {
>      void *fdt = dom->devicetree_blob;
> -    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
> -        bankend[GUEST_RAM_BANKS];
> +    uint64_t region_base[MAX_NR_EXT_REGIONS], region_size[MAX_NR_EXT_REGIONS];
>      uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
> -                  (GUEST_RAM_BANKS + 1)];
> +                  (MAX_NR_EXT_REGIONS + 1)];
>      be32 *cells = &regs[0];
>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>      const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> -    unsigned int i, len, nr_regions = 0;
> +    unsigned int i, j, len, nr_regions = 0;
>      libxl_dominfo info;
>      int offset, rc;
>  
> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      if (info.gpaddr_bits > 64)
>          return ERROR_INVAL;
>  
> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
> +          compare_iomem);
> +
>      /*
>       * Try to allocate separate 2MB-aligned extended regions from the first
>       * and second RAM banks taking into the account the maximum supported
>       * guest physical address space size and the amount of memory assigned
>       * to the guest.
>       */
> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> -        region_base[i] = bankbase[i] +
> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
> +        struct {
> +            uint64_t start;
> +            uint64_t end; /* inclusive */
> +        } unallocated;
> +        uint64_t size = 0;
> +
> +        unallocated.start = bankbase[i] +
>              ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
>  
> -        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
> -        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
> -        if (bankend[i] > region_base[i])
> -            region_size[i] = bankend[i] - region_base[i] + 1;
> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
> +
> +        if (unallocated.end > unallocated.start)
> +            size = unallocated.end - unallocated.start + 1;
> +
> +        if (size < EXT_REGION_MIN_SIZE)
> +            continue;
> +
> +        /* Exclude iomem */
> +        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
> +             j++) {
> +            struct {
> +                uint64_t start;
> +                uint64_t end; /* inclusive */
> +            } iomem;
> +
> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
> +                         << XC_PAGE_SHIFT) - 1;
> +
> +            if (iomem.end >= unallocated.start
> +                && iomem.start <= unallocated.end) {
> +
> +                if (iomem.start <= unallocated.start) {
> +                    unallocated.start = iomem.end + 1;
> +
> +                    if (iomem.end >= unallocated.end)
> +                        /* Complete overlap, discard unallocated region */
> +                        break;
> +
> +                    /* Beginning overlap */
> +                    continue;
> +                }
> +
> +                if (iomem.start > unallocated.start) {
> +                    assert(unallocated.end > unallocated.start);
> +                    size = iomem.start - unallocated.start;
> +
> +                    if (size >= EXT_REGION_MIN_SIZE) {
> +                        region_base[nr_regions] = unallocated.start;
> +                        region_size[nr_regions] = size;
> +                        nr_regions++;
> +                    }
> +
> +                    unallocated.start = iomem.end + 1;
> +
> +                    if (iomem.end >= unallocated.end)
> +                        /* End overlap, discard remaining unallocated region */
> +                        break;
> +                }
> +            }
> +        }
> +
> +        if (unallocated.end > unallocated.start
> +            && nr_regions < MAX_NR_EXT_REGIONS)
> +        {
> +            size = unallocated.end - unallocated.start + 1;
> +
> +            if (size >= EXT_REGION_MIN_SIZE) {
> +                region_base[nr_regions] = unallocated.start;
> +                region_size[nr_regions] = size;
> +                nr_regions++;
> +            }
> +        }
>      }
>  
>      /*
> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>      set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>                GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>  
> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
> -        if (region_size[i] < EXT_REGION_MIN_SIZE)
> -            continue;
> -
> +    for (i = 0; i < nr_regions; i++) {
>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
> +            i, region_base[i], region_base[i] + region_size[i]);
>  
>          set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>                    region_base[i], region_size[i]);
> -        nr_regions++;
>      }
>  
>      if (!nr_regions)
> @@ -1626,7 +1706,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>  
>      }
>  
> -    res = finalize_hypervisor_node(gc, dom);
> +    res = finalize_hypervisor_node(gc, &d_config->b_info, dom);
>      if (res)
>          return res;
>  
> -- 
> 2.49.0
> 
> 

Re: [PATCH v2 6/6] tools/arm: exclude iomem from domU extended regions

[Orzel, Michal]

On 03/06/2025 02:38, Stefano Stabellini wrote:
> I plan to commit this patch, unless someone objects
AFAICT there is a new revision (v3) that has pending comments:
https://lore.kernel.org/xen-devel/20250513195452.699600-1-stewart.hildebrand@amd.com/

~Michal

> 
> On Thu, 8 May 2025, Stewart Hildebrand wrote:
>> When a device is passed through to a xl domU, the iomem ranges may
>> overlap with the extended regions. Remove iomem from extended regions.
>>
>> Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
>> ---
>> Not sure if we need a Fixes: tag, but if we do:
>> Fixes: 57f87857dc2d ("libxl/arm: Add handling of extended regions for DomU")
>>
>> v1->v2:
>> * no change
>> ---
>>  tools/libs/light/libxl_arm.c | 118 +++++++++++++++++++++++++++++------
>>  1 file changed, 99 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/libs/light/libxl_arm.c b/tools/libs/light/libxl_arm.c
>> index 75c811053c7c..8ae16a1726fc 100644
>> --- a/tools/libs/light/libxl_arm.c
>> +++ b/tools/libs/light/libxl_arm.c
>> @@ -798,6 +798,8 @@ static int make_timer_node(libxl__gc *gc, void *fdt,
>>      return 0;
>>  }
>>  
>> +#define MAX_NR_EXT_REGIONS   256
>> +
>>  static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>>                                  const libxl_version_info *vers)
>>  {
>> @@ -821,7 +823,7 @@ static int make_hypervisor_node(libxl__gc *gc, void *fdt,
>>       */
>>      res = fdt_property_reg_placeholder(gc, fdt, GUEST_ROOT_ADDRESS_CELLS,
>>                                         GUEST_ROOT_SIZE_CELLS,
>> -                                       GUEST_RAM_BANKS + 1);
>> +                                       MAX_NR_EXT_REGIONS + 1);
>>      if (res) return res;
>>  
>>      /*
>> @@ -1517,17 +1519,29 @@ static void finalise_one_node(libxl__gc *gc, void *fdt, const char *uname,
>>  
>>  #define EXT_REGION_MIN_SIZE   xen_mk_ullong(0x0004000000) /* 64MB */
>>  
>> -static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>> +static int compare_iomem(const void *a, const void *b)
>> +{
>> +    const libxl_iomem_range *x = a, *y = b;
>> +
>> +    if (x->gfn < y->gfn)
>> +        return -1;
>> +    if (x->gfn > y->gfn)
>> +        return 1;
>> +    return 0;
>> +}
>> +
>> +static int finalize_hypervisor_node(libxl__gc *gc,
>> +                                    libxl_domain_build_info *b_info,
>> +                                    struct xc_dom_image *dom)
>>  {
>>      void *fdt = dom->devicetree_blob;
>> -    uint64_t region_size[GUEST_RAM_BANKS] = {0}, region_base[GUEST_RAM_BANKS],
>> -        bankend[GUEST_RAM_BANKS];
>> +    uint64_t region_base[MAX_NR_EXT_REGIONS], region_size[MAX_NR_EXT_REGIONS];
>>      uint32_t regs[(GUEST_ROOT_ADDRESS_CELLS + GUEST_ROOT_SIZE_CELLS) *
>> -                  (GUEST_RAM_BANKS + 1)];
>> +                  (MAX_NR_EXT_REGIONS + 1)];
>>      be32 *cells = &regs[0];
>>      const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>>      const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>> -    unsigned int i, len, nr_regions = 0;
>> +    unsigned int i, j, len, nr_regions = 0;
>>      libxl_dominfo info;
>>      int offset, rc;
>>  
>> @@ -1542,20 +1556,90 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>>      if (info.gpaddr_bits > 64)
>>          return ERROR_INVAL;
>>  
>> +    qsort(b_info->iomem, b_info->num_iomem, sizeof(libxl_iomem_range),
>> +          compare_iomem);
>> +
>>      /*
>>       * Try to allocate separate 2MB-aligned extended regions from the first
>>       * and second RAM banks taking into the account the maximum supported
>>       * guest physical address space size and the amount of memory assigned
>>       * to the guest.
>>       */
>> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> -        region_base[i] = bankbase[i] +
>> +    for (i = 0; i < GUEST_RAM_BANKS && nr_regions < MAX_NR_EXT_REGIONS; i++) {
>> +        struct {
>> +            uint64_t start;
>> +            uint64_t end; /* inclusive */
>> +        } unallocated;
>> +        uint64_t size = 0;
>> +
>> +        unallocated.start = bankbase[i] +
>>              ALIGN_UP_TO_2MB((uint64_t)dom->rambank_size[i] << XC_PAGE_SHIFT);
>>  
>> -        bankend[i] = ~0ULL >> (64 - info.gpaddr_bits);
>> -        bankend[i] = min(bankend[i], bankbase[i] + banksize[i] - 1);
>> -        if (bankend[i] > region_base[i])
>> -            region_size[i] = bankend[i] - region_base[i] + 1;
>> +        unallocated.end = ~0ULL >> (64 - info.gpaddr_bits);
>> +        unallocated.end = min(unallocated.end, bankbase[i] + banksize[i] - 1);
>> +
>> +        if (unallocated.end > unallocated.start)
>> +            size = unallocated.end - unallocated.start + 1;
>> +
>> +        if (size < EXT_REGION_MIN_SIZE)
>> +            continue;
>> +
>> +        /* Exclude iomem */
>> +        for (j = 0; j < b_info->num_iomem && nr_regions < MAX_NR_EXT_REGIONS;
>> +             j++) {
>> +            struct {
>> +                uint64_t start;
>> +                uint64_t end; /* inclusive */
>> +            } iomem;
>> +
>> +            iomem.start = b_info->iomem[j].gfn << XC_PAGE_SHIFT;
>> +            iomem.end = ((b_info->iomem[j].gfn + b_info->iomem[j].number)
>> +                         << XC_PAGE_SHIFT) - 1;
>> +
>> +            if (iomem.end >= unallocated.start
>> +                && iomem.start <= unallocated.end) {
>> +
>> +                if (iomem.start <= unallocated.start) {
>> +                    unallocated.start = iomem.end + 1;
>> +
>> +                    if (iomem.end >= unallocated.end)
>> +                        /* Complete overlap, discard unallocated region */
>> +                        break;
>> +
>> +                    /* Beginning overlap */
>> +                    continue;
>> +                }
>> +
>> +                if (iomem.start > unallocated.start) {
>> +                    assert(unallocated.end > unallocated.start);
>> +                    size = iomem.start - unallocated.start;
>> +
>> +                    if (size >= EXT_REGION_MIN_SIZE) {
>> +                        region_base[nr_regions] = unallocated.start;
>> +                        region_size[nr_regions] = size;
>> +                        nr_regions++;
>> +                    }
>> +
>> +                    unallocated.start = iomem.end + 1;
>> +
>> +                    if (iomem.end >= unallocated.end)
>> +                        /* End overlap, discard remaining unallocated region */
>> +                        break;
>> +                }
>> +            }
>> +        }
>> +
>> +        if (unallocated.end > unallocated.start
>> +            && nr_regions < MAX_NR_EXT_REGIONS)
>> +        {
>> +            size = unallocated.end - unallocated.start + 1;
>> +
>> +            if (size >= EXT_REGION_MIN_SIZE) {
>> +                region_base[nr_regions] = unallocated.start;
>> +                region_size[nr_regions] = size;
>> +                nr_regions++;
>> +            }
>> +        }
>>      }
>>  
>>      /*
>> @@ -1565,16 +1649,12 @@ static int finalize_hypervisor_node(libxl__gc *gc, struct xc_dom_image *dom)
>>      set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>>                GUEST_GNTTAB_BASE, GUEST_GNTTAB_SIZE);
>>  
>> -    for (i = 0; i < GUEST_RAM_BANKS; i++) {
>> -        if (region_size[i] < EXT_REGION_MIN_SIZE)
>> -            continue;
>> -
>> +    for (i = 0; i < nr_regions; i++) {
>>          LOG(DEBUG, "Extended region %u: %#"PRIx64"->%#"PRIx64"",
>> -            nr_regions, region_base[i], region_base[i] + region_size[i]);
>> +            i, region_base[i], region_base[i] + region_size[i]);
>>  
>>          set_range(&cells, GUEST_ROOT_ADDRESS_CELLS, GUEST_ROOT_SIZE_CELLS,
>>                    region_base[i], region_size[i]);
>> -        nr_regions++;
>>      }
>>  
>>      if (!nr_regions)
>> @@ -1626,7 +1706,7 @@ int libxl__arch_domain_finalise_hw_description(libxl__gc *gc,
>>  
>>      }
>>  
>> -    res = finalize_hypervisor_node(gc, dom);
>> +    res = finalize_hypervisor_node(gc, &d_config->b_info, dom);
>>      if (res)
>>          return res;
>>  
>> -- 
>> 2.49.0
>>
>>
> 

Re: [PATCH v2 6/6] tools/arm: exclude iomem from domU extended regions

[Stefano Stabellini]

On Tue, 3 Jun 2025, Orzel, Michal wrote:
> On 03/06/2025 02:38, Stefano Stabellini wrote:
> > I plan to commit this patch, unless someone objects
> AFAICT there is a new revision (v3) that has pending comments:
> https://lore.kernel.org/xen-devel/20250513195452.699600-1-stewart.hildebrand@amd.com/

Thanks Michal! Due to the change in CC and 0 email subject I didn't
notice the newer version