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

Carlo Nonato posted 12 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH v10 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Carlo Nonato 1 month, 1 week 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.

Take the opportunity to adapt the implementation of allocate_memory() so
that it uses the host layout when called on the hwdom, via
find_unallocated_memory().

Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
---
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             | 96 +++++++++++++++++++++++--
 xen/arch/arm/include/asm/domain_build.h |  1 +
 3 files changed, 93 insertions(+), 48 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 2c30792de8..a95376dcdc 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -416,7 +416,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 +507,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
@@ -1003,6 +1001,93 @@ 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 = 2;
+    paddr_t bank_start, bank_size;
+    struct membanks *hwdom_ext_regions = 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) )
+    {
+        ASSERT(llc_coloring_enabled);
+
+        hwdom_ext_regions = xzalloc_flex_struct(struct membanks, bank,
+                                                NR_MEM_BANKS);
+        if ( !hwdom_ext_regions )
+            goto fail;
+        hwdom_ext_regions->max_banks = NR_MEM_BANKS;
+
+        if ( find_unallocated_memory(kinfo, hwdom_ext_regions) )
+            goto fail;
+
+        nr_banks = hwdom_ext_regions->nr_banks;
+    }
+
+    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
+    {
+        if ( is_hardware_domain(d) )
+        {
+            bank_start = hwdom_ext_regions->bank[i].start;
+            bank_size = hwdom_ext_regions->bank[i].size;
+
+            if ( bank_size < min_t(paddr_t, kinfo->unassigned_mem, MB(128)) )
+                continue;
+        }
+        else
+        {
+            if ( i == 0 )
+            {
+                bank_start = GUEST_RAM0_BASE;
+                bank_size = GUEST_RAM0_SIZE;
+            }
+            else if ( i == 1 )
+            {
+                bank_start = GUEST_RAM1_BASE;
+                bank_size = GUEST_RAM1_SIZE;
+            }
+            else
+                goto fail;
+        }
+
+        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_ext_regions);
+    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)
 {
@@ -1223,7 +1308,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_unallocated_memory(kinfo, ext_regions);
@@ -2152,7 +2237,10 @@ 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);
+    if ( is_domain_direct_mapped(d) )
+        allocate_memory_11(d, &kinfo);
+    else
+        allocate_memory(d, &kinfo);
     find_gnttab_region(d, &kinfo);
 
     rc = process_shm_chosen(d, &kinfo);
diff --git a/xen/arch/arm/include/asm/domain_build.h b/xen/arch/arm/include/asm/domain_build.h
index e712afbc7f..5d77af2e8b 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);
-- 
2.43.0
Re: [PATCH v10 03/12] xen/arm: permit non direct-mapped Dom0 construction
Posted by Michal Orzel 4 weeks ago

On 19/11/2024 15:13, 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.
> 
> Take the opportunity to adapt the implementation of allocate_memory() so
> that it uses the host layout when called on the hwdom, via
> find_unallocated_memory().
> 
> Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> ---
> 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             | 96 +++++++++++++++++++++++--
>  xen/arch/arm/include/asm/domain_build.h |  1 +
>  3 files changed, 93 insertions(+), 48 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 2c30792de8..a95376dcdc 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -416,7 +416,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 +507,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
> @@ -1003,6 +1001,93 @@ 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 = 2;
Instead of hardcoding, shouldn't it be GUEST_RAM_BANKS?
Also, the second bank won't work with CONFIG_ARM_PA_BITS_32 which limits PA to 32bit.


> +    paddr_t bank_start, bank_size;
> +    struct membanks *hwdom_ext_regions = NULL;
AFAICT you search for free memory. Naming it as extended regions is not a good choice.
Instead, hwdom_free_mem?

> +
> +    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) )
> +    {
> +        ASSERT(llc_coloring_enabled);
> +
> +        hwdom_ext_regions = xzalloc_flex_struct(struct membanks, bank,
> +                                                NR_MEM_BANKS);
> +        if ( !hwdom_ext_regions )
> +            goto fail;
empty line here please

> +        hwdom_ext_regions->max_banks = NR_MEM_BANKS;
> +
> +        if ( find_unallocated_memory(kinfo, hwdom_ext_regions) )
If you reuse find_unallocated_memory for a purpose other than extended regions, I think
the description of this function should change as well as comments inside.

Today, the function gets all RAM and exclude dom0 RAM (in your case is 0 at this point, reserved memory,
static shmem and gnttab (in your case is 0 at this point). I think we cannot get away without
making this function more generic. Maybe it should take as a parameter struct membanks * array?
Also, the callback add_ext_regions() may not be suited for all purposes (i.e. it takes only banks
> 64MB into account). I know that there will be more use cases for a function that will return the
free memory for domains. As an example, today, for direct mapped domains we hardcode the gnttab region
(only dom0 is special cased). This should not be like that. We would need to find a free memory region
to expose as gnttab.

> +            goto fail;
> +
> +        nr_banks = hwdom_ext_regions->nr_banks;
> +    }
> +
> +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
> +    {
> +        if ( is_hardware_domain(d) )
> +        {
> +            bank_start = hwdom_ext_regions->bank[i].start;
> +            bank_size = hwdom_ext_regions->bank[i].size;
> +
> +            if ( bank_size < min_t(paddr_t, kinfo->unassigned_mem, MB(128)) )
I would expect some explanation.

> +                continue;
> +        }
> +        else
> +        {
> +            if ( i == 0 )
> +            {
> +                bank_start = GUEST_RAM0_BASE;
> +                bank_size = GUEST_RAM0_SIZE;
> +            }
> +            else if ( i == 1 )
> +            {
> +                bank_start = GUEST_RAM1_BASE;
> +                bank_size = GUEST_RAM1_SIZE;
> +            }
> +            else
> +                goto fail;
This could be simplified:
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];

This patch requires also opinion of other maintainers.

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

On Thu, Nov 28, 2024 at 11:34 AM Michal Orzel <michal.orzel@amd.com> wrote:
> On 19/11/2024 15:13, 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.
> >
> > Take the opportunity to adapt the implementation of allocate_memory() so
> > that it uses the host layout when called on the hwdom, via
> > find_unallocated_memory().
> >
> > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
> > ---
> > 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             | 96 +++++++++++++++++++++++--
> >  xen/arch/arm/include/asm/domain_build.h |  1 +
> >  3 files changed, 93 insertions(+), 48 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 2c30792de8..a95376dcdc 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -416,7 +416,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 +507,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
> > @@ -1003,6 +1001,93 @@ 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 = 2;
> Instead of hardcoding, shouldn't it be GUEST_RAM_BANKS?

Right.

> Also, the second bank won't work with CONFIG_ARM_PA_BITS_32 which limits PA to 32bit.

How is this being addressed in the current allocate_memory?
Also, LLC_COLORING depends on ARM_64. ARM_PA_BITS_32 requires ARM_32, so the
two configurations should be incompatible as of now.

> > +    paddr_t bank_start, bank_size;
> > +    struct membanks *hwdom_ext_regions = NULL;
> AFAICT you search for free memory. Naming it as extended regions is not a good choice.
> Instead, hwdom_free_mem?

Yes.

> > +
> > +    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) )
> > +    {
> > +        ASSERT(llc_coloring_enabled);
> > +
> > +        hwdom_ext_regions = xzalloc_flex_struct(struct membanks, bank,
> > +                                                NR_MEM_BANKS);
> > +        if ( !hwdom_ext_regions )
> > +            goto fail;
> empty line here please
>
> > +        hwdom_ext_regions->max_banks = NR_MEM_BANKS;
> > +
> > +        if ( find_unallocated_memory(kinfo, hwdom_ext_regions) )
> If you reuse find_unallocated_memory for a purpose other than extended regions, I think
> the description of this function should change as well as comments inside.

I can definetely change that.

> Today, the function gets all RAM and exclude dom0 RAM (in your case is 0 at this point, reserved memory,
> static shmem and gnttab (in your case is 0 at this point). I think we cannot get away without
> making this function more generic. Maybe it should take as a parameter struct membanks * array?
> Also, the callback add_ext_regions() may not be suited for all purposes (i.e. it takes only banks
> > 64MB into account). I know that there will be more use cases for a function that will return the
> free memory for domains. As an example, today, for direct mapped domains we hardcode the gnttab region
> (only dom0 is special cased). This should not be like that. We would need to find a free memory region
> to expose as gnttab.

For the current cases (including llc coloring) it works. If there are no
objections, a TODO plus comments and description changes you talked above is
probably sufficient to cover the (current) use-cases and "ensure" this is not
forgotten for the future extension you mention.

> > +            goto fail;
> > +
> > +        nr_banks = hwdom_ext_regions->nr_banks;
> > +    }
> > +
> > +    for ( i = 0; kinfo->unassigned_mem > 0 && nr_banks > 0; i++, nr_banks-- )
> > +    {
> > +        if ( is_hardware_domain(d) )
> > +        {
> > +            bank_start = hwdom_ext_regions->bank[i].start;
> > +            bank_size = hwdom_ext_regions->bank[i].size;
> > +
> > +            if ( bank_size < min_t(paddr_t, kinfo->unassigned_mem, MB(128)) )
> I would expect some explanation.
>
> > +                continue;
> > +        }
> > +        else
> > +        {
> > +            if ( i == 0 )
> > +            {
> > +                bank_start = GUEST_RAM0_BASE;
> > +                bank_size = GUEST_RAM0_SIZE;
> > +            }
> > +            else if ( i == 1 )
> > +            {
> > +                bank_start = GUEST_RAM1_BASE;
> > +                bank_size = GUEST_RAM1_SIZE;
> > +            }
> > +            else
> > +                goto fail;
> This could be simplified:
> 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];

Ok.

> This patch requires also opinion of other maintainers.
>
> ~Michal

Thanks.

- Carlo