[PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction

Carlo Nonato posted 12 patches 1 week, 6 days ago
There is a newer version of this series
[PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Carlo Nonato 1 week, 6 days ago
Cache coloring requires Dom0 not to be direct-mapped because of its non
contiguous mapping nature, so allocate_memory() is needed in this case.
8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
moved allocate_memory() in dom0less_build.c. In order to use it
in Dom0 construction bring it back to domain_build.c and declare it in
domain_build.h.

Adapt the implementation of allocate_memory() so that it uses the host
layout when called on the hwdom, via find_unallocated_memory(). Take the
opportunity to keep the parameter order consistent with
rangeset_report_ranges() and move the membanks struct after the callback
function in find_unallocated_memory().

Introduce add_hwdom_free_regions() callback to add hwdom banks in descending
order.

Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
v12:
- used the new generic find_unallocated_memory()
- added add_hwdom_free_regions() callback
v11:
- GUEST_RAM_BANKS instead of hardcoding the number of banks in allocate_memory()
- hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
- added a comment in allocate_memory() when skipping small banks
v10:
- fixed a compilation bug that happened when dom0less support was disabled
v9:
- no changes
v8:
- patch adapted to new changes to allocate_memory()
v7:
- allocate_memory() now uses the host layout when called on the hwdom
v6:
- new patch
---
 xen/arch/arm/dom0less-build.c           |  44 -------
 xen/arch/arm/domain_build.c             | 164 +++++++++++++++++++++++-
 xen/arch/arm/include/asm/domain_build.h |   4 +
 3 files changed, 161 insertions(+), 51 deletions(-)

diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
index d93a85434e..67b1503647 100644
--- a/xen/arch/arm/dom0less-build.c
+++ b/xen/arch/arm/dom0less-build.c
@@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
     return ( !dom0found && domUfound );
 }
 
-static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
-{
-    struct membanks *mem = kernel_info_get_mem(kinfo);
-    unsigned int i;
-    paddr_t bank_size;
-
-    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
-           /* Don't want format this as PRIpaddr (16 digit hex) */
-           (unsigned long)(kinfo->unassigned_mem >> 20), d);
-
-    mem->nr_banks = 0;
-    bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
-    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
-                               bank_size) )
-        goto fail;
-
-    bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
-    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
-                               bank_size) )
-        goto fail;
-
-    if ( kinfo->unassigned_mem )
-        goto fail;
-
-    for( i = 0; i < mem->nr_banks; i++ )
-    {
-        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
-               d,
-               i,
-               mem->bank[i].start,
-               mem->bank[i].start + mem->bank[i].size,
-               /* Don't want format this as PRIpaddr (16 digit hex) */
-               (unsigned long)(mem->bank[i].size >> 20));
-    }
-
-    return;
-
-fail:
-    panic("Failed to allocate requested domain memory."
-          /* Don't want format this as PRIpaddr (16 digit hex) */
-          " %ldKB unallocated. Fix the VMs configurations.\n",
-          (unsigned long)kinfo->unassigned_mem >> 10);
-}
-
 #ifdef CONFIG_VGICV2
 static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
 {
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index adf26f2778..bfcff99194 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2,6 +2,7 @@
 #include <xen/init.h>
 #include <xen/compile.h>
 #include <xen/lib.h>
+#include <xen/llc-coloring.h>
 #include <xen/mm.h>
 #include <xen/param.h>
 #include <xen/domain_page.h>
@@ -416,7 +417,6 @@ static void __init allocate_memory_11(struct domain *d,
     }
 }
 
-#ifdef CONFIG_DOM0LESS_BOOT
 bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
                                     alloc_domheap_mem_cb cb, void *extra)
 {
@@ -508,7 +508,6 @@ bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
 
     return true;
 }
-#endif
 
 /*
  * When PCI passthrough is available we want to keep the
@@ -900,6 +899,52 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
     return 0;
 }
 
+static int __init add_hwdom_free_regions(unsigned long s_gfn,
+                                         unsigned long e_gfn, void *data)
+{
+    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);
+    unsigned int i, j;
+
+    if ( free_regions->nr_banks >= free_regions->max_banks )
+        return 0;
+
+    /*
+     * Both start and size of the free region should be 2MB aligned to
+     * potentially allow superpage mapping.
+     */
+    start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
+    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);
+
+    /* Find the insert position (descending order). */
+    for ( i = 0; i < free_regions->nr_banks ; i++)
+        if ( size > free_regions->bank[i].size )
+            break;
+
+    /* Move the other banks to make space. */
+    for ( j = free_regions->nr_banks; j > i ; j-- )
+    {
+        free_regions->bank[j].start = free_regions->bank[j - 1].start;
+        free_regions->bank[j].size = free_regions->bank[j - 1].size;
+    }
+
+    free_regions->bank[i].start = start;
+    free_regions->bank[i].size = size;
+    free_regions->nr_banks++;
+
+    return 0;
+}
+
 /*
  * Find unused regions of Host address space which can be exposed to domain
  * using the host memory layout. In order to calculate regions we exclude every
@@ -908,10 +953,10 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
 static int __init find_unallocated_memory(const struct kernel_info *kinfo,
                                           const struct membanks *mem_banks[],
                                           unsigned int nr_mem_banks,
-                                          struct membanks *free_regions,
                                           int (*cb)(unsigned long s_gfn,
                                                     unsigned long e_gfn,
-                                                    void *data))
+                                                    void *data),
+                                          struct membanks *free_regions)
 {
     const struct membanks *mem = bootinfo_get_mem();
     struct rangeset *unalloc_mem;
@@ -977,6 +1022,108 @@ out:
     return res;
 }
 
+void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
+{
+    struct membanks *mem = kernel_info_get_mem(kinfo);
+    unsigned int i, nr_banks = GUEST_RAM_BANKS;
+    struct membanks *hwdom_free_mem = NULL;
+
+    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
+           /* Don't want format this as PRIpaddr (16 digit hex) */
+           (unsigned long)(kinfo->unassigned_mem >> 20), d);
+
+    mem->nr_banks = 0;
+    /*
+     * Use host memory layout for hwdom. Only case for this is when LLC coloring
+     * is enabled.
+     */
+    if ( is_hardware_domain(d) )
+    {
+        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, bank, 1);
+        /*
+         * Exclude the following regions:
+         * 1) Remove reserved memory
+         * 2) Grant table assigned to Dom0
+         */
+        const struct membanks *mem_banks[] = {
+            bootinfo_get_reserved_mem(),
+            gnttab,
+        };
+
+        ASSERT(llc_coloring_enabled);
+
+        if ( !gnttab )
+            goto fail;
+
+        gnttab->nr_banks = 1;
+        gnttab->bank[0].start = kinfo->gnttab_start;
+        gnttab->bank[0].size = kinfo->gnttab_start + kinfo->gnttab_size;
+
+        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
+                                             NR_MEM_BANKS);
+        if ( !hwdom_free_mem )
+            goto fail;
+
+        hwdom_free_mem->max_banks = NR_MEM_BANKS;
+
+        if ( find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
+                                     add_hwdom_free_regions, hwdom_free_mem) )
+            goto fail;
+
+        nr_banks = hwdom_free_mem->nr_banks;
+        xfree(gnttab);
+    }
+
+    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
+    {
+        paddr_t bank_start, bank_size;
+
+        if ( is_hardware_domain(d) )
+        {
+            bank_start = hwdom_free_mem->bank[i].start;
+            bank_size = hwdom_free_mem->bank[i].size;
+        }
+        else
+        {
+            const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
+            const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
+
+            if ( i >= GUEST_RAM_BANKS )
+                goto fail;
+
+            bank_start = bankbase[i];
+            bank_size = banksize[i];
+        }
+
+        bank_size = MIN(bank_size, kinfo->unassigned_mem);
+        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), bank_size) )
+            goto fail;
+    }
+
+    if ( kinfo->unassigned_mem )
+        goto fail;
+
+    for( i = 0; i < mem->nr_banks; i++ )
+    {
+        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
+               d,
+               i,
+               mem->bank[i].start,
+               mem->bank[i].start + mem->bank[i].size,
+               /* Don't want format this as PRIpaddr (16 digit hex) */
+               (unsigned long)(mem->bank[i].size >> 20));
+    }
+
+    xfree(hwdom_free_mem);
+    return;
+
+fail:
+    panic("Failed to allocate requested domain memory."
+          /* Don't want format this as PRIpaddr (16 digit hex) */
+          " %ldKB unallocated. Fix the VMs configurations.\n",
+          (unsigned long)kinfo->unassigned_mem >> 10);
+}
+
 static int __init handle_pci_range(const struct dt_device_node *dev,
                                    uint64_t addr, uint64_t len, void *data)
 {
@@ -1176,7 +1323,7 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
     gnttab->bank[0].size = kinfo->gnttab_size;
 
     res = find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
-                                  ext_regions, add_ext_regions);
+                                  add_ext_regions, ext_regions);
     xfree(gnttab);
 
     return res;
@@ -1235,7 +1382,7 @@ int __init make_hypervisor_node(struct domain *d,
 
         ext_regions->max_banks = NR_MEM_BANKS;
 
-        if ( is_domain_direct_mapped(d) )
+        if ( domain_use_host_layout(d) )
         {
             if ( !is_iommu_enabled(d) )
                 res = find_host_extended_regions(kinfo, ext_regions);
@@ -2164,8 +2311,11 @@ static int __init construct_dom0(struct domain *d)
     /* type must be set before allocate_memory */
     d->arch.type = kinfo.type;
 #endif
-    allocate_memory_11(d, &kinfo);
     find_gnttab_region(d, &kinfo);
+    if ( is_domain_direct_mapped(d) )
+        allocate_memory_11(d, &kinfo);
+    else
+        allocate_memory(d, &kinfo);
 
     rc = process_shm_chosen(d, &kinfo);
     if ( rc < 0 )
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index e712afbc7f..b0d646e173 100644
--- a/xen/arch/arm/include/asm/domain_build.h
+++ b/xen/arch/arm/include/asm/domain_build.h
@@ -11,6 +11,7 @@ bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
                              alloc_domheap_mem_cb cb, void *extra);
 bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
                           paddr_t tot_size);
+void allocate_memory(struct domain *d, struct kernel_info *kinfo);
 int construct_domain(struct domain *d, struct kernel_info *kinfo);
 int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
 int make_chosen_node(const struct kernel_info *kinfo);
@@ -54,6 +55,9 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
 int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
 #endif
 
+typedef int (*add_free_regions_fn)(unsigned long s_gfn, unsigned long e_gfn,
+                                   void *data);
+
 int add_ext_regions(unsigned long s_gfn, unsigned long e_gfn, void *data);
 
 #endif
-- 
2.43.0
Re: [PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Michal Orzel 1 week, 3 days ago

On 13/12/2024 17:28, Carlo Nonato wrote:
> 
> 
> Cache coloring requires Dom0 not to be direct-mapped because of its non
> contiguous mapping nature, so allocate_memory() is needed in this case.
> 8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
> moved allocate_memory() in dom0less_build.c. In order to use it
> in Dom0 construction bring it back to domain_build.c and declare it in
> domain_build.h.
> 
> Adapt the implementation of allocate_memory() so that it uses the host
> layout when called on the hwdom, via find_unallocated_memory(). Take the
> opportunity to keep the parameter order consistent with
> rangeset_report_ranges() and move the membanks struct after the callback
> function in find_unallocated_memory().
Why? What benefit does this change (that is irrelevant to the goal of this patch) provide?

> 
> Introduce add_hwdom_free_regions() callback to add hwdom banks in descending
> order.
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> ---
> v12:
> - used the new generic find_unallocated_memory()
> - added add_hwdom_free_regions() callback
> v11:
> - GUEST_RAM_BANKS instead of hardcoding the number of banks in allocate_memory()
> - hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
> - added a comment in allocate_memory() when skipping small banks
> v10:
> - fixed a compilation bug that happened when dom0less support was disabled
> v9:
> - no changes
> v8:
> - patch adapted to new changes to allocate_memory()
> v7:
> - allocate_memory() now uses the host layout when called on the hwdom
> v6:
> - new patch
> ---
>  xen/arch/arm/dom0less-build.c           |  44 -------
>  xen/arch/arm/domain_build.c             | 164 +++++++++++++++++++++++-
>  xen/arch/arm/include/asm/domain_build.h |   4 +
>  3 files changed, 161 insertions(+), 51 deletions(-)
> 
> diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> index d93a85434e..67b1503647 100644
> --- a/xen/arch/arm/dom0less-build.c
> +++ b/xen/arch/arm/dom0less-build.c
> @@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
>      return ( !dom0found && domUfound );
>  }
> 
> -static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> -{
> -    struct membanks *mem = kernel_info_get_mem(kinfo);
> -    unsigned int i;
> -    paddr_t bank_size;
> -
> -    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> -           /* Don't want format this as PRIpaddr (16 digit hex) */
> -           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> -
> -    mem->nr_banks = 0;
> -    bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> -                               bank_size) )
> -        goto fail;
> -
> -    bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> -                               bank_size) )
> -        goto fail;
> -
> -    if ( kinfo->unassigned_mem )
> -        goto fail;
> -
> -    for( i = 0; i < mem->nr_banks; i++ )
> -    {
> -        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
> -               d,
> -               i,
> -               mem->bank[i].start,
> -               mem->bank[i].start + mem->bank[i].size,
> -               /* Don't want format this as PRIpaddr (16 digit hex) */
> -               (unsigned long)(mem->bank[i].size >> 20));
> -    }
> -
> -    return;
> -
> -fail:
> -    panic("Failed to allocate requested domain memory."
> -          /* Don't want format this as PRIpaddr (16 digit hex) */
> -          " %ldKB unallocated. Fix the VMs configurations.\n",
> -          (unsigned long)kinfo->unassigned_mem >> 10);
> -}
> -
>  #ifdef CONFIG_VGICV2
>  static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
>  {
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index adf26f2778..bfcff99194 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -2,6 +2,7 @@
>  #include <xen/init.h>
>  #include <xen/compile.h>
>  #include <xen/lib.h>
> +#include <xen/llc-coloring.h>
>  #include <xen/mm.h>
>  #include <xen/param.h>
>  #include <xen/domain_page.h>
> @@ -416,7 +417,6 @@ static void __init allocate_memory_11(struct domain *d,
>      }
>  }
> 
> -#ifdef CONFIG_DOM0LESS_BOOT
>  bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
>                                      alloc_domheap_mem_cb cb, void *extra)
>  {
> @@ -508,7 +508,6 @@ bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> 
>      return true;
>  }
> -#endif
> 
>  /*
>   * When PCI passthrough is available we want to keep the
> @@ -900,6 +899,52 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>      return 0;
>  }
> 
> +static int __init add_hwdom_free_regions(unsigned long s_gfn,
> +                                         unsigned long e_gfn, void *data)
> +{
> +    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);
> +    unsigned int i, j;
> +
> +    if ( free_regions->nr_banks >= free_regions->max_banks )
> +        return 0;
> +
> +    /*
> +     * Both start and size of the free region should be 2MB aligned to
> +     * potentially allow superpage mapping.
> +     */
> +    start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
> +    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);
> +
> +    /* Find the insert position (descending order). */
> +    for ( i = 0; i < free_regions->nr_banks ; i++)
CODING_STYLE:
for ( i = 0; i < free_regions->nr_banks; i++ )

> +        if ( size > free_regions->bank[i].size )
> +            break;
> +
> +    /* Move the other banks to make space. */
> +    for ( j = free_regions->nr_banks; j > i ; j-- )
> +    {
> +        free_regions->bank[j].start = free_regions->bank[j - 1].start;
> +        free_regions->bank[j].size = free_regions->bank[j - 1].size;
> +    }
> +
> +    free_regions->bank[i].start = start;
> +    free_regions->bank[i].size = size;
> +    free_regions->nr_banks++;
The algorithm looks good. In my head I thought you will use sort() after adding all the banks, but
I'm not sure which solution is more efficient. Probably yours and it avoids implementing cmp and swap functions.

> +
> +    return 0;
> +}
> +
>  /*
>   * Find unused regions of Host address space which can be exposed to domain
>   * using the host memory layout. In order to calculate regions we exclude every
> @@ -908,10 +953,10 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
>  static int __init find_unallocated_memory(const struct kernel_info *kinfo,
>                                            const struct membanks *mem_banks[],
>                                            unsigned int nr_mem_banks,
> -                                          struct membanks *free_regions,
>                                            int (*cb)(unsigned long s_gfn,
>                                                      unsigned long e_gfn,
> -                                                    void *data))
> +                                                    void *data),
> +                                          struct membanks *free_regions)
>  {
>      const struct membanks *mem = bootinfo_get_mem();
>      struct rangeset *unalloc_mem;
> @@ -977,6 +1022,108 @@ out:
>      return res;
>  }
> 
> +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> +{
> +    struct membanks *mem = kernel_info_get_mem(kinfo);
> +    unsigned int i, nr_banks = GUEST_RAM_BANKS;
> +    struct membanks *hwdom_free_mem = NULL;
> +
> +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> +           /* Don't want format this as PRIpaddr (16 digit hex) */
> +           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> +
> +    mem->nr_banks = 0;
> +    /*
> +     * Use host memory layout for hwdom. Only case for this is when LLC coloring
> +     * is enabled.
> +     */
> +    if ( is_hardware_domain(d) )
> +    {
> +        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, bank, 1);
> +        /*
> +         * Exclude the following regions:
> +         * 1) Remove reserved memory
> +         * 2) Grant table assigned to Dom0
Can we not mention 'Dom0'? In the future hwdom may not necessarily be dom0. Especially that
in other places you mention hwdom.

> +         */
> +        const struct membanks *mem_banks[] = {
> +            bootinfo_get_reserved_mem(),
> +            gnttab,
> +        };
> +
> +        ASSERT(llc_coloring_enabled);
Remove this assert. There's nothing LLC special here and this could be re-used in the future
to provide non 1:1 hwdom.

> +
> +        if ( !gnttab )
> +            goto fail;
> +
> +        gnttab->nr_banks = 1;
> +        gnttab->bank[0].start = kinfo->gnttab_start;
> +        gnttab->bank[0].size = kinfo->gnttab_start + kinfo->gnttab_size;
Incorrect. You assign to 'end' to'size'. It should simply be:
gnttab->bank[0].size = kinfo->gnttab_size.

> +
> +        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
> +                                             NR_MEM_BANKS);
> +        if ( !hwdom_free_mem )
> +            goto fail;
> +
> +        hwdom_free_mem->max_banks = NR_MEM_BANKS;
> +
> +        if ( find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
> +                                     add_hwdom_free_regions, hwdom_free_mem) )
> +            goto fail;
> +
> +        nr_banks = hwdom_free_mem->nr_banks;
> +        xfree(gnttab);
> +    }
> +
> +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
> +    {
> +        paddr_t bank_start, bank_size;
> +
> +        if ( is_hardware_domain(d) )
> +        {
> +            bank_start = hwdom_free_mem->bank[i].start;
> +            bank_size = hwdom_free_mem->bank[i].size;
> +        }
> +        else
> +        {
> +            const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> +            const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> +
> +            if ( i >= GUEST_RAM_BANKS )
> +                goto fail;
> +
> +            bank_start = bankbase[i];
> +            bank_size = banksize[i];
> +        }
> +
> +        bank_size = MIN(bank_size, kinfo->unassigned_mem);
> +        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), bank_size) )
> +            goto fail;
> +    }
> +
> +    if ( kinfo->unassigned_mem )
> +        goto fail;
> +
> +    for( i = 0; i < mem->nr_banks; i++ )
> +    {
> +        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
> +               d,
> +               i,
> +               mem->bank[i].start,
> +               mem->bank[i].start + mem->bank[i].size,
> +               /* Don't want format this as PRIpaddr (16 digit hex) */
> +               (unsigned long)(mem->bank[i].size >> 20));
> +    }
> +
> +    xfree(hwdom_free_mem);
> +    return;
> +
> +fail:
> +    panic("Failed to allocate requested domain memory."
> +          /* Don't want format this as PRIpaddr (16 digit hex) */
> +          " %ldKB unallocated. Fix the VMs configurations.\n",
> +          (unsigned long)kinfo->unassigned_mem >> 10);
> +}
> +
>  static int __init handle_pci_range(const struct dt_device_node *dev,
>                                     uint64_t addr, uint64_t len, void *data)
>  {
> @@ -1176,7 +1323,7 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
>      gnttab->bank[0].size = kinfo->gnttab_size;
> 
>      res = find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
> -                                  ext_regions, add_ext_regions);
> +                                  add_ext_regions, ext_regions);
>      xfree(gnttab);
> 
>      return res;
> @@ -1235,7 +1382,7 @@ int __init make_hypervisor_node(struct domain *d,
> 
>          ext_regions->max_banks = NR_MEM_BANKS;
> 
> -        if ( is_domain_direct_mapped(d) )
> +        if ( domain_use_host_layout(d) )
>          {
>              if ( !is_iommu_enabled(d) )
>                  res = find_host_extended_regions(kinfo, ext_regions);
> @@ -2164,8 +2311,11 @@ static int __init construct_dom0(struct domain *d)
>      /* type must be set before allocate_memory */
>      d->arch.type = kinfo.type;
>  #endif
> -    allocate_memory_11(d, &kinfo);
>      find_gnttab_region(d, &kinfo);
This re-ordering should be mentioned in commit msg.

> +    if ( is_domain_direct_mapped(d) )
> +        allocate_memory_11(d, &kinfo);
> +    else
> +        allocate_memory(d, &kinfo);
> 
>      rc = process_shm_chosen(d, &kinfo);
>      if ( rc < 0 )
> diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
> index e712afbc7f..b0d646e173 100644
> --- a/xen/arch/arm/include/asm/domain_build.h
> +++ b/xen/arch/arm/include/asm/domain_build.h
> @@ -11,6 +11,7 @@ bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
>                               alloc_domheap_mem_cb cb, void *extra);
>  bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
>                            paddr_t tot_size);
> +void allocate_memory(struct domain *d, struct kernel_info *kinfo);
>  int construct_domain(struct domain *d, struct kernel_info *kinfo);
>  int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
>  int make_chosen_node(const struct kernel_info *kinfo);
> @@ -54,6 +55,9 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
>  int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
>  #endif
> 
> +typedef int (*add_free_regions_fn)(unsigned long s_gfn, unsigned long e_gfn,
> +                                   void *data);
Random code? Remove.

With the remarks addressed:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal
Re: [PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Carlo Nonato 1 week, 3 days ago
Hi Michal,

On Mon, Dec 16, 2024 at 1:08 PM Michal Orzel <michal.orzel@amd.com> wrote:
>
> On 13/12/2024 17:28, Carlo Nonato wrote:
> >
> > Cache coloring requires Dom0 not to be direct-mapped because of its non
> > contiguous mapping nature, so allocate_memory() is needed in this case.
> > 8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
> > moved allocate_memory() in dom0less_build.c. In order to use it
> > in Dom0 construction bring it back to domain_build.c and declare it in
> > domain_build.h.
> >
> > Adapt the implementation of allocate_memory() so that it uses the host
> > layout when called on the hwdom, via find_unallocated_memory(). Take the
> > opportunity to keep the parameter order consistent with
> > rangeset_report_ranges() and move the membanks struct after the callback
> > function in find_unallocated_memory().
> Why? What benefit does this change (that is irrelevant to the goal of this patch) provide?

You're right, it's irrelevant to this patch. Do you think it can be done in a
separate patch? If nothing else, find_unallocated_memory() appears to be
called 2 times while rangeset_report_ranges() 11 times.

> > Introduce add_hwdom_free_regions() callback to add hwdom banks in descending
> > order.
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> > ---
> > v12:
> > - used the new generic find_unallocated_memory()
> > - added add_hwdom_free_regions() callback
> > v11:
> > - GUEST_RAM_BANKS instead of hardcoding the number of banks in allocate_memory()
> > - hwdom_ext_regions -> hwdom_free_mem in allocate_memory()
> > - added a comment in allocate_memory() when skipping small banks
> > v10:
> > - fixed a compilation bug that happened when dom0less support was disabled
> > v9:
> > - no changes
> > v8:
> > - patch adapted to new changes to allocate_memory()
> > v7:
> > - allocate_memory() now uses the host layout when called on the hwdom
> > v6:
> > - new patch
> > ---
> >  xen/arch/arm/dom0less-build.c           |  44 -------
> >  xen/arch/arm/domain_build.c             | 164 +++++++++++++++++++++++-
> >  xen/arch/arm/include/asm/domain_build.h |   4 +
> >  3 files changed, 161 insertions(+), 51 deletions(-)
> >
> > diff --git a/xen/arch/arm/dom0less-build.c b/xen/arch/arm/dom0less-build.c
> > index d93a85434e..67b1503647 100644
> > --- a/xen/arch/arm/dom0less-build.c
> > +++ b/xen/arch/arm/dom0less-build.c
> > @@ -49,50 +49,6 @@ bool __init is_dom0less_mode(void)
> >      return ( !dom0found && domUfound );
> >  }
> >
> > -static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> > -{
> > -    struct membanks *mem = kernel_info_get_mem(kinfo);
> > -    unsigned int i;
> > -    paddr_t bank_size;
> > -
> > -    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> > -           /* Don't want format this as PRIpaddr (16 digit hex) */
> > -           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> > -
> > -    mem->nr_banks = 0;
> > -    bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> > -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> > -                               bank_size) )
> > -        goto fail;
> > -
> > -    bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> > -    if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> > -                               bank_size) )
> > -        goto fail;
> > -
> > -    if ( kinfo->unassigned_mem )
> > -        goto fail;
> > -
> > -    for( i = 0; i < mem->nr_banks; i++ )
> > -    {
> > -        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
> > -               d,
> > -               i,
> > -               mem->bank[i].start,
> > -               mem->bank[i].start + mem->bank[i].size,
> > -               /* Don't want format this as PRIpaddr (16 digit hex) */
> > -               (unsigned long)(mem->bank[i].size >> 20));
> > -    }
> > -
> > -    return;
> > -
> > -fail:
> > -    panic("Failed to allocate requested domain memory."
> > -          /* Don't want format this as PRIpaddr (16 digit hex) */
> > -          " %ldKB unallocated. Fix the VMs configurations.\n",
> > -          (unsigned long)kinfo->unassigned_mem >> 10);
> > -}
> > -
> >  #ifdef CONFIG_VGICV2
> >  static int __init make_gicv2_domU_node(struct kernel_info *kinfo)
> >  {
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index adf26f2778..bfcff99194 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -2,6 +2,7 @@
> >  #include <xen/init.h>
> >  #include <xen/compile.h>
> >  #include <xen/lib.h>
> > +#include <xen/llc-coloring.h>
> >  #include <xen/mm.h>
> >  #include <xen/param.h>
> >  #include <xen/domain_page.h>
> > @@ -416,7 +417,6 @@ static void __init allocate_memory_11(struct domain *d,
> >      }
> >  }
> >
> > -#ifdef CONFIG_DOM0LESS_BOOT
> >  bool __init allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> >                                      alloc_domheap_mem_cb cb, void *extra)
> >  {
> > @@ -508,7 +508,6 @@ bool __init allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> >
> >      return true;
> >  }
> > -#endif
> >
> >  /*
> >   * When PCI passthrough is available we want to keep the
> > @@ -900,6 +899,52 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> >      return 0;
> >  }
> >
> > +static int __init add_hwdom_free_regions(unsigned long s_gfn,
> > +                                         unsigned long e_gfn, void *data)
> > +{
> > +    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);
> > +    unsigned int i, j;
> > +
> > +    if ( free_regions->nr_banks >= free_regions->max_banks )
> > +        return 0;
> > +
> > +    /*
> > +     * Both start and size of the free region should be 2MB aligned to
> > +     * potentially allow superpage mapping.
> > +     */
> > +    start = (s + SZ_2M - 1) & ~(SZ_2M - 1);
> > +    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);
> > +
> > +    /* Find the insert position (descending order). */
> > +    for ( i = 0; i < free_regions->nr_banks ; i++)
> CODING_STYLE:
> for ( i = 0; i < free_regions->nr_banks; i++ )
>
> > +        if ( size > free_regions->bank[i].size )
> > +            break;
> > +
> > +    /* Move the other banks to make space. */
> > +    for ( j = free_regions->nr_banks; j > i ; j-- )
> > +    {
> > +        free_regions->bank[j].start = free_regions->bank[j - 1].start;
> > +        free_regions->bank[j].size = free_regions->bank[j - 1].size;
> > +    }
> > +
> > +    free_regions->bank[i].start = start;
> > +    free_regions->bank[i].size = size;
> > +    free_regions->nr_banks++;
> The algorithm looks good. In my head I thought you will use sort() after adding all the banks, but
> I'm not sure which solution is more efficient. Probably yours and it avoids implementing cmp and swap functions.
>
> > +
> > +    return 0;
> > +}
> > +
> >  /*
> >   * Find unused regions of Host address space which can be exposed to domain
> >   * using the host memory layout. In order to calculate regions we exclude every
> > @@ -908,10 +953,10 @@ int __init add_ext_regions(unsigned long s_gfn, unsigned long e_gfn,
> >  static int __init find_unallocated_memory(const struct kernel_info *kinfo,
> >                                            const struct membanks *mem_banks[],
> >                                            unsigned int nr_mem_banks,
> > -                                          struct membanks *free_regions,
> >                                            int (*cb)(unsigned long s_gfn,
> >                                                      unsigned long e_gfn,
> > -                                                    void *data))
> > +                                                    void *data),
> > +                                          struct membanks *free_regions)
> >  {
> >      const struct membanks *mem = bootinfo_get_mem();
> >      struct rangeset *unalloc_mem;
> > @@ -977,6 +1022,108 @@ out:
> >      return res;
> >  }
> >
> > +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
> > +{
> > +    struct membanks *mem = kernel_info_get_mem(kinfo);
> > +    unsigned int i, nr_banks = GUEST_RAM_BANKS;
> > +    struct membanks *hwdom_free_mem = NULL;
> > +
> > +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
> > +           /* Don't want format this as PRIpaddr (16 digit hex) */
> > +           (unsigned long)(kinfo->unassigned_mem >> 20), d);
> > +
> > +    mem->nr_banks = 0;
> > +    /*
> > +     * Use host memory layout for hwdom. Only case for this is when LLC coloring
> > +     * is enabled.
> > +     */
> > +    if ( is_hardware_domain(d) )
> > +    {
> > +        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, bank, 1);
> > +        /*
> > +         * Exclude the following regions:
> > +         * 1) Remove reserved memory
> > +         * 2) Grant table assigned to Dom0
> Can we not mention 'Dom0'? In the future hwdom may not necessarily be dom0. Especially that
> in other places you mention hwdom.
>
> > +         */
> > +        const struct membanks *mem_banks[] = {
> > +            bootinfo_get_reserved_mem(),
> > +            gnttab,
> > +        };
> > +
> > +        ASSERT(llc_coloring_enabled);
> Remove this assert. There's nothing LLC special here and this could be re-used in the future
> to provide non 1:1 hwdom.
>
> > +
> > +        if ( !gnttab )
> > +            goto fail;
> > +
> > +        gnttab->nr_banks = 1;
> > +        gnttab->bank[0].start = kinfo->gnttab_start;
> > +        gnttab->bank[0].size = kinfo->gnttab_start + kinfo->gnttab_size;
> Incorrect. You assign to 'end' to'size'. It should simply be:
> gnttab->bank[0].size = kinfo->gnttab_size.
>
> > +
> > +        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
> > +                                             NR_MEM_BANKS);
> > +        if ( !hwdom_free_mem )
> > +            goto fail;
> > +
> > +        hwdom_free_mem->max_banks = NR_MEM_BANKS;
> > +
> > +        if ( find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
> > +                                     add_hwdom_free_regions, hwdom_free_mem) )
> > +            goto fail;
> > +
> > +        nr_banks = hwdom_free_mem->nr_banks;
> > +        xfree(gnttab);
> > +    }
> > +
> > +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
> > +    {
> > +        paddr_t bank_start, bank_size;
> > +
> > +        if ( is_hardware_domain(d) )
> > +        {
> > +            bank_start = hwdom_free_mem->bank[i].start;
> > +            bank_size = hwdom_free_mem->bank[i].size;
> > +        }
> > +        else
> > +        {
> > +            const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
> > +            const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
> > +
> > +            if ( i >= GUEST_RAM_BANKS )
> > +                goto fail;
> > +
> > +            bank_start = bankbase[i];
> > +            bank_size = banksize[i];
> > +        }
> > +
> > +        bank_size = MIN(bank_size, kinfo->unassigned_mem);
> > +        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), bank_size) )
> > +            goto fail;
> > +    }
> > +
> > +    if ( kinfo->unassigned_mem )
> > +        goto fail;
> > +
> > +    for( i = 0; i < mem->nr_banks; i++ )
> > +    {
> > +        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
> > +               d,
> > +               i,
> > +               mem->bank[i].start,
> > +               mem->bank[i].start + mem->bank[i].size,
> > +               /* Don't want format this as PRIpaddr (16 digit hex) */
> > +               (unsigned long)(mem->bank[i].size >> 20));
> > +    }
> > +
> > +    xfree(hwdom_free_mem);
> > +    return;
> > +
> > +fail:
> > +    panic("Failed to allocate requested domain memory."
> > +          /* Don't want format this as PRIpaddr (16 digit hex) */
> > +          " %ldKB unallocated. Fix the VMs configurations.\n",
> > +          (unsigned long)kinfo->unassigned_mem >> 10);
> > +}
> > +
> >  static int __init handle_pci_range(const struct dt_device_node *dev,
> >                                     uint64_t addr, uint64_t len, void *data)
> >  {
> > @@ -1176,7 +1323,7 @@ static int __init find_host_extended_regions(const struct kernel_info *kinfo,
> >      gnttab->bank[0].size = kinfo->gnttab_size;
> >
> >      res = find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
> > -                                  ext_regions, add_ext_regions);
> > +                                  add_ext_regions, ext_regions);
> >      xfree(gnttab);
> >
> >      return res;
> > @@ -1235,7 +1382,7 @@ int __init make_hypervisor_node(struct domain *d,
> >
> >          ext_regions->max_banks = NR_MEM_BANKS;
> >
> > -        if ( is_domain_direct_mapped(d) )
> > +        if ( domain_use_host_layout(d) )
> >          {
> >              if ( !is_iommu_enabled(d) )
> >                  res = find_host_extended_regions(kinfo, ext_regions);
> > @@ -2164,8 +2311,11 @@ static int __init construct_dom0(struct domain *d)
> >      /* type must be set before allocate_memory */
> >      d->arch.type = kinfo.type;
> >  #endif
> > -    allocate_memory_11(d, &kinfo);
> >      find_gnttab_region(d, &kinfo);
> This re-ordering should be mentioned in commit msg.
>
> > +    if ( is_domain_direct_mapped(d) )
> > +        allocate_memory_11(d, &kinfo);
> > +    else
> > +        allocate_memory(d, &kinfo);
> >
> >      rc = process_shm_chosen(d, &kinfo);
> >      if ( rc < 0 )
> > diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
> > index e712afbc7f..b0d646e173 100644
> > --- a/xen/arch/arm/include/asm/domain_build.h
> > +++ b/xen/arch/arm/include/asm/domain_build.h
> > @@ -11,6 +11,7 @@ bool allocate_domheap_memory(struct domain *d, paddr_t tot_size,
> >                               alloc_domheap_mem_cb cb, void *extra);
> >  bool allocate_bank_memory(struct kernel_info *kinfo, gfn_t sgfn,
> >                            paddr_t tot_size);
> > +void allocate_memory(struct domain *d, struct kernel_info *kinfo);
> >  int construct_domain(struct domain *d, struct kernel_info *kinfo);
> >  int domain_fdt_begin_node(void *fdt, const char *name, uint64_t unit);
> >  int make_chosen_node(const struct kernel_info *kinfo);
> > @@ -54,6 +55,9 @@ static inline int prepare_acpi(struct domain *d, struct kernel_info *kinfo)
> >  int prepare_acpi(struct domain *d, struct kernel_info *kinfo);
> >  #endif
> >
> > +typedef int (*add_free_regions_fn)(unsigned long s_gfn, unsigned long e_gfn,
> > +                                   void *data);
> Random code? Remove.
>
> With the remarks addressed:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>
>
> ~Michal

Thanks.

- Carlo
Re: [PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Michal Orzel 1 week, 3 days ago

On 16/12/2024 16:51, Carlo Nonato wrote:
> 
> 
> Hi Michal,
> 
> On Mon, Dec 16, 2024 at 1:08 PM Michal Orzel <michal.orzel@amd.com> wrote:
>>
>> On 13/12/2024 17:28, Carlo Nonato wrote:
>>>
>>> Cache coloring requires Dom0 not to be direct-mapped because of its non
>>> contiguous mapping nature, so allocate_memory() is needed in this case.
>>> 8d2c3ab18cc1 ("arm/dom0less: put dom0less feature code in a separate module")
>>> moved allocate_memory() in dom0less_build.c. In order to use it
>>> in Dom0 construction bring it back to domain_build.c and declare it in
>>> domain_build.h.
>>>
>>> Adapt the implementation of allocate_memory() so that it uses the host
>>> layout when called on the hwdom, via find_unallocated_memory(). Take the
>>> opportunity to keep the parameter order consistent with
>>> rangeset_report_ranges() and move the membanks struct after the callback
>>> function in find_unallocated_memory().
>> Why? What benefit does this change (that is irrelevant to the goal of this patch) provide?
> 
> You're right, it's irrelevant to this patch. Do you think it can be done in a
> separate patch? If nothing else, find_unallocated_memory() appears to be
> called 2 times while rangeset_report_ranges() 11 times.
Leave it as is. I don't understand why you think find_unallocated_memory is supposed to follow some
special format like rangeset_report_ranges. Don't focus on things that are not relevant for now. Merging this series for
4.20 is more important.

Also, please trim down your replies.

~Michal


Re: [PATCH v12 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Jan Beulich 1 week, 3 days ago
On 16.12.2024 13:08, Michal Orzel wrote:
> On 13/12/2024 17:28, Carlo Nonato wrote:
>> @@ -977,6 +1022,108 @@ out:
>>      return res;
>>  }
>>
>> +void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
>> +{
>> +    struct membanks *mem = kernel_info_get_mem(kinfo);
>> +    unsigned int i, nr_banks = GUEST_RAM_BANKS;
>> +    struct membanks *hwdom_free_mem = NULL;
>> +
>> +    printk(XENLOG_INFO "Allocating mappings totalling %ldMB for %pd:\n",
>> +           /* Don't want format this as PRIpaddr (16 digit hex) */
>> +           (unsigned long)(kinfo->unassigned_mem >> 20), d);
>> +
>> +    mem->nr_banks = 0;
>> +    /*
>> +     * Use host memory layout for hwdom. Only case for this is when LLC coloring
>> +     * is enabled.
>> +     */
>> +    if ( is_hardware_domain(d) )
>> +    {
>> +        struct membanks *gnttab = xzalloc_flex_struct(struct membanks, bank, 1);
>> +        /*
>> +         * Exclude the following regions:
>> +         * 1) Remove reserved memory
>> +         * 2) Grant table assigned to Dom0
> Can we not mention 'Dom0'? In the future hwdom may not necessarily be dom0. Especially that
> in other places you mention hwdom.
> 
>> +         */
>> +        const struct membanks *mem_banks[] = {
>> +            bootinfo_get_reserved_mem(),
>> +            gnttab,
>> +        };
>> +
>> +        ASSERT(llc_coloring_enabled);
> Remove this assert. There's nothing LLC special here and this could be re-used in the future
> to provide non 1:1 hwdom.
> 
>> +
>> +        if ( !gnttab )
>> +            goto fail;
>> +
>> +        gnttab->nr_banks = 1;
>> +        gnttab->bank[0].start = kinfo->gnttab_start;
>> +        gnttab->bank[0].size = kinfo->gnttab_start + kinfo->gnttab_size;
> Incorrect. You assign to 'end' to'size'. It should simply be:
> gnttab->bank[0].size = kinfo->gnttab_size.
> 
>> +
>> +        hwdom_free_mem = xzalloc_flex_struct(struct membanks, bank,
>> +                                             NR_MEM_BANKS);
>> +        if ( !hwdom_free_mem )
>> +            goto fail;
>> +
>> +        hwdom_free_mem->max_banks = NR_MEM_BANKS;
>> +
>> +        if ( find_unallocated_memory(kinfo, mem_banks, ARRAY_SIZE(mem_banks),
>> +                                     add_hwdom_free_regions, hwdom_free_mem) )
>> +            goto fail;
>> +
>> +        nr_banks = hwdom_free_mem->nr_banks;
>> +        xfree(gnttab);
>> +    }
>> +
>> +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
>> +    {
>> +        paddr_t bank_start, bank_size;
>> +
>> +        if ( is_hardware_domain(d) )
>> +        {
>> +            bank_start = hwdom_free_mem->bank[i].start;
>> +            bank_size = hwdom_free_mem->bank[i].size;
>> +        }
>> +        else
>> +        {
>> +            const uint64_t bankbase[] = GUEST_RAM_BANK_BASES;
>> +            const uint64_t banksize[] = GUEST_RAM_BANK_SIZES;
>> +
>> +            if ( i >= GUEST_RAM_BANKS )
>> +                goto fail;
>> +
>> +            bank_start = bankbase[i];
>> +            bank_size = banksize[i];
>> +        }
>> +
>> +        bank_size = MIN(bank_size, kinfo->unassigned_mem);
>> +        if ( !allocate_bank_memory(kinfo, gaddr_to_gfn(bank_start), bank_size) )
>> +            goto fail;
>> +    }
>> +
>> +    if ( kinfo->unassigned_mem )
>> +        goto fail;
>> +
>> +    for( i = 0; i < mem->nr_banks; i++ )
>> +    {
>> +        printk(XENLOG_INFO "%pd BANK[%d] %#"PRIpaddr"-%#"PRIpaddr" (%ldMB)\n",
>> +               d,
>> +               i,
>> +               mem->bank[i].start,
>> +               mem->bank[i].start + mem->bank[i].size,
>> +               /* Don't want format this as PRIpaddr (16 digit hex) */
>> +               (unsigned long)(mem->bank[i].size >> 20));
>> +    }
>> +
>> +    xfree(hwdom_free_mem);
>> +    return;
>> +
>> +fail:

Nit: Style (missing indentation).

Jan