Mailing List Archive: [PATCH 9/9] xen/arm: introduce allocate_static

[PATCH 9/9] xen/arm: introduce allocate_static_memory

Jun 6, 2021, 7:43 PM

Post #1 of 3 (8 views)

This commit introduces allocate_static_memory to allocate static memory as
guest RAM for Domain on Static Allocation.

It uses alloc_domstatic_pages to allocate pre-configured static memory banks
for this domain, and uses guest_physmap_add_page to set up P2M table.
These pre-defiend static memory ranges shall be firstly mapped to the
fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
`GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`.
`GUEST_RAM0` may take up several pre-defined physical RAM regions.

Signed-off-by: Penny Zheng <penny.zheng@arm.com>
---
changes v2:
- rename the values, like prefix it g/p
- fix the scalability issue
- allocate when parse
---
xen/arch/arm/domain_build.c | 155 +++++++++++++++++++++++++++++++++++-
1 file changed, 153 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 4166d7993c..63b6a97b2c 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct domain *d,
return true;
}

+/*
+ * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of #gsize,
+ * and #sgfn will be next guest address to map when returning.
+ */
+static bool __init allocate_static_bank_memory(struct domain *d,
+ struct kernel_info *kinfo,
+ int gbank,
+ gfn_t* sgfn,
+ mfn_t smfn,
+ paddr_t gsize)
+{
+ int res;
+ paddr_t tot_size = gsize;
+ const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+
+ while ( tot_size > 0 )
+ {
+ unsigned int order = get_allocation_size(tot_size);
+
+ res = guest_physmap_add_page(d, *sgfn, smfn, order);
+ if ( res )
+ {
+ dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
+ return false;
+ }
+
+ *sgfn = gfn_add(*sgfn, 1UL << order);
+ smfn = mfn_add(smfn, 1UL << order);
+ tot_size -= (1ULL << (PAGE_SHIFT + order));
+ }
+
+ /* Guest RAM bank in kinfo hasn't been initialized. */
+ if ( gbank == kinfo->mem.nr_banks )
+ {
+ kinfo->mem.bank[gbank].start = rambase[gbank];
+ kinfo->mem.nr_banks++;
+ }
+ kinfo->mem.bank[gbank].size += gsize;
+
+ return true;
+}
+
static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)
{
unsigned int i;
@@ -480,6 +522,116 @@ fail:
(unsigned long)kinfo->unassigned_mem >> 10);
}

+/* Allocate memory from static memory as RAM for one specific domain d. */
+static u64 __init allocate_static_memory(struct domain *d,
+ struct kernel_info *kinfo,
+ const struct dt_device_node *node)
+{
+ int nr_banks, bank = 0, gbank = 0;
+ const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
+ const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
+ const __be32 *cell;
+ const struct dt_property *prop;
+ struct dt_device_node *static_mem_node;
+ const struct dt_device_node *parent = dt_find_node_by_path("/reserved-memory");
+ u32 addr_cells = 2, size_cells = 2, reg_cells;
+ u64 tot_size;
+
+ paddr_t pbase, psize, gsize;
+ gfn_t sgfn;
+ mfn_t smfn;
+
+ kinfo->mem.nr_banks = 0;
+ /* Start with GUEST_RAM0. */
+ gsize = ramsize[gbank];
+ sgfn = gaddr_to_gfn(rambase[gbank]);
+
+ /* Parse phandle in `xen,static-mem`. */
+ static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
+ if ( !static_mem_node )
+ goto fail;
+
+ /*
+ * #address-cells and #size-cells must be consistent with the parent node,
+ * "reserved-memory".
+ */
+ dt_property_read_u32(parent, "#address-cells", &addr_cells);
+ dt_property_read_u32(parent, "#size-cells", &size_cells);
+ BUG_ON(size_cells > 2 || addr_cells > 2);
+ reg_cells = addr_cells + size_cells;
+
+ prop = dt_find_property(static_mem_node, "reg", NULL);
+ if ( !prop )
+ goto fail;
+ cell = (const __be32 *)prop->value;
+ nr_banks = (prop->length) / (reg_cells * sizeof (u32));
+ BUG_ON(nr_banks > NR_MEM_BANKS);
+
+ while ( bank < nr_banks )
+ {
+ device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);
+ tot_size += (u64)psize;
+ smfn = maddr_to_mfn(pbase);
+
+ if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
+ {
+ printk(XENLOG_ERR
+ "%pd: cannot allocate static memory"
+ "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
+ d, pbase, pbase + psize);
+ goto fail;
+ }
+
+ printk(XENLOG_INFO "%pd STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",
+ d, bank, pbase, pbase + psize);
+
+ /*
+ * It shall be mapped to the fixed guest RAM address rambase[i],
+ * And until it exhausts the ramsize[i], it will seek to the next
+ * rambase[i+1].
+ */
+ while ( 1 )
+ {
+ if ( gsize >= psize )
+ {
+ if ( !allocate_static_bank_memory(d, kinfo, gbank,
+ &sgfn, smfn, psize) )
+ goto fail;
+
+ gsize = gsize - psize;
+ bank++;
+ break;
+ }
+ else
+ {
+ if ( !allocate_static_bank_memory(d, kinfo, gbank,
+ &sgfn, smfn, gsize) )
+ goto fail;
+
+ /*
+ * Physical bank hasn't been totally mapped,
+ * seeking to the next guest RAM i+1, if exist.
+ */
+ if ( ++gbank < GUEST_RAM_BANKS )
+ {
+ psize = psize - gsize;
+ smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
+ gsize = ramsize[gbank];
+ sgfn = gaddr_to_gfn(rambase[gbank]);
+ }
+ else
+ goto fail;
+ }
+ }
+ }
+ return tot_size;
+
+fail:
+ panic("Failed to allocate requested static memory for domain %pd."
+ "Fix the VMs configurations.\n",
+ d);
+}
+
static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
const struct dt_device_node *node)
{
@@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
if ( prop )
{
static_mem = true;
- /* static_mem_size = allocate_static_memory(...); */
- BUG();
+ static_mem_size = allocate_static_memory(d, &kinfo, node);
}

rc = dt_property_read_u64(node, "memory", &mem);
--
2.25.1

Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory [ In reply to ]

julien at xen

Jul 3, 2021, 7:18 AM

Post #2 of 3 (8 views)

Permalink

Hi Penny,

On 07/06/2021 03:43, Penny Zheng wrote:
> This commit introduces allocate_static_memory to allocate static memory as
> guest RAM for Domain on Static Allocation.
>
> It uses alloc_domstatic_pages to allocate pre-configured static memory banks
> for this domain, and uses guest_physmap_add_page to set up P2M table.
> These pre-defiend static memory ranges shall be firstly mapped to the
> fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
> `GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`.
> `GUEST_RAM0` may take up several pre-defined physical RAM regions.
>
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
> changes v2:
> - rename the values, like prefix it g/p
> - fix the scalability issue
> - allocate when parse
> ---
> xen/arch/arm/domain_build.c | 155 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 153 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 4166d7993c..63b6a97b2c 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct domain *d,
> return true;
> }
>
> +/*
> + * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of #gsize,
> + * and #sgfn will be next guest address to map when returning.
> + */
> +static bool __init allocate_static_bank_memory(struct domain *d,
> + struct kernel_info *kinfo,
> + int gbank,

Please use unsigned int for index.

> + gfn_t* sgfn,
> + mfn_t smfn,
> + paddr_t gsize)

This function doesn't allocate memory and neither a "bank" per-se. So
would suggest to rename to (or something similar):

append_static_memory_to_bank()

Also, I don't think you need all those parameters. You can infer the
next GFN to use from the bank information. So how about something like:

static bool __init append_static_memory_to_bank(struct domain *d,
struct membank *bank,
mfn_t smfn,
paddr_t size)
{
gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);

[....]
}

> +{
> + int res;
> + paddr_t tot_size = gsize;
> + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;

I don't like the idea of introducing rambase here. Can't the bank be
initialized in the caller?

> +
> + while ( tot_size > 0 )
> + {
> + unsigned int order = get_allocation_size(tot_size);
> +
> + res = guest_physmap_add_page(d, *sgfn, smfn, order);
> + if ( res )
> + {
> + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> + return false;
> + }
> +
> + *sgfn = gfn_add(*sgfn, 1UL << order);
> + smfn = mfn_add(smfn, 1UL << order);
> + tot_size -= (1ULL << (PAGE_SHIFT + order));
> + }
> +
> + /* Guest RAM bank in kinfo hasn't been initialized. */
> + if ( gbank == kinfo->mem.nr_banks )
> + {
> + kinfo->mem.bank[gbank].start = rambase[gbank];
> + kinfo->mem.nr_banks++;
> + }
> + kinfo->mem.bank[gbank].size += gsize;
> +
> + return true;
> +}
> +
> static void __init allocate_memory(struct domain *d, struct kernel_info *kinfo)

It feels a bit odd that the two functions you introduced are not
together. Can they be moved together?

> {
> unsigned int i;
> @@ -480,6 +522,116 @@ fail:
> (unsigned long)kinfo->unassigned_mem >> 10);
> }
>
> +/* Allocate memory from static memory as RAM for one specific domain d. */
> +static u64 __init allocate_static_memory(struct domain *d,
> + struct kernel_info *kinfo,
> + const struct dt_device_node *node)
> +{
> + int nr_banks, bank = 0, gbank = 0;
> + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> + const __be32 *cell;
> + const struct dt_property *prop;
> + struct dt_device_node *static_mem_node;
> + const struct dt_device_node *parent = dt_find_node_by_path("/reserved-memory");
> + u32 addr_cells = 2, size_cells = 2, reg_cells;
> + u64 tot_size;
> +
> + paddr_t pbase, psize, gsize;
> + gfn_t sgfn;
> + mfn_t smfn;
> +
> + kinfo->mem.nr_banks = 0;
> + /* Start with GUEST_RAM0. */
> + gsize = ramsize[gbank];
> + sgfn = gaddr_to_gfn(rambase[gbank]);
> +
> + /* Parse phandle in `xen,static-mem`. */
> + static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
> + if ( !static_mem_node )
> + goto fail;
> +
> + /*
> + * #address-cells and #size-cells must be consistent with the parent node,
> + * "reserved-memory".
> + */
> + dt_property_read_u32(parent, "#address-cells", &addr_cells);
> + dt_property_read_u32(parent, "#size-cells", &size_cells);

The return for dt_property_read_u32() should be checked.

> + BUG_ON(size_cells > 2 || addr_cells > 2);
> + reg_cells = addr_cells + size_cells;
> +
> + prop = dt_find_property(static_mem_node, "reg", NULL);
> + if ( !prop )
> + goto fail;
> + cell = (const __be32 *)prop->value;
> + nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> + BUG_ON(nr_banks > NR_MEM_BANKS);
> +
> + while ( bank < nr_banks )
> + {
> + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase, &psize);

We seem to have quite a few functions now that will iterate over "regs".
It would be worth considering to introduce a helper to iterate it.

> + tot_size += (u64)psize;

This cast seems unnecessary.

> + smfn = maddr_to_mfn(pbase);
> +
> + if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
> + {
> + printk(XENLOG_ERR
> + "%pd: cannot allocate static memory"
> + "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
> + d, pbase, pbase + psize);
> + goto fail;
> + }
> +
> + printk(XENLOG_INFO "%pd STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",
> + d, bank, pbase, pbase + psize);
> +
> + /*
> + * It shall be mapped to the fixed guest RAM address rambase[i],
> + * And until it exhausts the ramsize[i], it will seek to the next
> + * rambase[i+1].
> + */
> + while ( 1 )
> + {
> + if ( gsize >= psize )
> + {
> + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> + &sgfn, smfn, psize) )
> + goto fail;
> +
> + gsize = gsize - psize;
> + bank++;
> + break;
> + }
> + else
> + {
> + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> + &sgfn, smfn, gsize) )
> + goto fail;
> +
> + /*
> + * Physical bank hasn't been totally mapped,
> + * seeking to the next guest RAM i+1, if exist.
> + */
> + if ( ++gbank < GUEST_RAM_BANKS )
> + {
> + psize = psize - gsize;
> + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> + gsize = ramsize[gbank];
> + sgfn = gaddr_to_gfn(rambase[gbank]);
> + }
> + else
> + goto fail;
> + }

The double loop is not nice but I can't think of a better way. However,
I think the code in the loop can be simplified. You could write
something like:

/* Map as much as possible the static range to the guest bank */
if ( !allocate_static_bank(..., min(psize, gsize) )
goto fail;

/* The physical bank is fully mapped. Handle the next bank. */
if ( gsize >= psize )
{
gsize = gsize - psize;
bank++;
break;
}
/* We still have memory to map. Check if we have another guest bank
available */
else if ( ++gbank > GUEST_RAM_BANKS ) {
printk("Exhausted the number of guest bank\n");
goto fail;
}

[Update to the next guest bank]

> + }
> + }
> + return tot_size;
> +
> +fail:
> + panic("Failed to allocate requested static memory for domain %pd."
> + "Fix the VMs configurations.\n",
> + d);
> +}
> +
> static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
> const struct dt_device_node *node)
> {
> @@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
> if ( prop )
> {
> static_mem = true;
> - /* static_mem_size = allocate_static_memory(...); */
> - BUG();
> + static_mem_size = allocate_static_memory(d, &kinfo, node);
> }
>
> rc = dt_property_read_u64(node, "memory", &mem);
>

Cheers,

--
Julien Grall

RE: [PATCH 9/9] xen/arm: introduce allocate_static_memory [ In reply to ]

Penny.Zheng at arm

Jul 6, 2021, 12:30 AM

Post #3 of 3 (8 views)

Permalink

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: Saturday, July 3, 2021 10:18 PM
> To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org;
> sstabellini@kernel.org; jbeulich@suse.com
> Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Wei Chen
> <Wei.Chen@arm.com>
> Subject: Re: [PATCH 9/9] xen/arm: introduce allocate_static_memory
>
> Hi Penny,
>
> On 07/06/2021 03:43, Penny Zheng wrote:
> > This commit introduces allocate_static_memory to allocate static
> > memory as guest RAM for Domain on Static Allocation.
> >
> > It uses alloc_domstatic_pages to allocate pre-configured static memory
> > banks for this domain, and uses guest_physmap_add_page to set up P2M
> table.
> > These pre-defiend static memory ranges shall be firstly mapped to the
> > fixed guest RAM address `GUEST_RAM0_BASE`. And until it exhausts the
> > `GUEST_RAM0_SIZE`, it will seek to `GUEST_RAM1_BASE`.
> > `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> >
> > Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> > ---
> > changes v2:
> > - rename the values, like prefix it g/p
> > - fix the scalability issue
> > - allocate when parse
> > ---
> > xen/arch/arm/domain_build.c | 155
> +++++++++++++++++++++++++++++++++++-
> > 1 file changed, 153 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 4166d7993c..63b6a97b2c 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -437,6 +437,48 @@ static bool __init allocate_bank_memory(struct
> domain *d,
> > return true;
> > }
> >
> > +/*
> > + * Static memory bank at #smfn of #gsize shall be mapped to #sgfn of
> > +#gsize,
> > + * and #sgfn will be next guest address to map when returning.
> > + */
> > +static bool __init allocate_static_bank_memory(struct domain *d,
> > + struct kernel_info *kinfo,
> > + int gbank,
>
> Please use unsigned int for index.
>

Sure.

> > + gfn_t* sgfn,
> > + mfn_t smfn,
> > + paddr_t gsize)
>
> This function doesn't allocate memory and neither a "bank" per-se. So would
> suggest to rename to (or something similar):
>
> append_static_memory_to_bank()
>

Ok~~~

> Also, I don't think you need all those parameters. You can infer the next GFN
> to use from the bank information. So how about something like:
>
> static bool __init append_static_memory_to_bank(struct domain *d,
> struct membank *bank,
> mfn_t smfn,
> paddr_t size) {
> gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
>
> [....]
> }
>

Oh. Right, since we are setting info in kinfo.mem.bank[i], we could infer itself to get the
Next GFN, clever! Thks a lot!

> > +{
> > + int res;
> > + paddr_t tot_size = gsize;
> > + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
>
> I don't like the idea of introducing rambase here. Can't the bank be initialized
> in the caller?
>

Hmm, I'm kinds of confused the suggestion here. If we using rambase[] here,
then in later double loop, when updating to the next guest bank, we could
use rambase[gbank] to refer.

> > +
> > + while ( tot_size > 0 )
> > + {
> > + unsigned int order = get_allocation_size(tot_size);
> > +
> > + res = guest_physmap_add_page(d, *sgfn, smfn, order);
> > + if ( res )
> > + {
> > + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> > + return false;
> > + }
> > +
> > + *sgfn = gfn_add(*sgfn, 1UL << order);
> > + smfn = mfn_add(smfn, 1UL << order);
> > + tot_size -= (1ULL << (PAGE_SHIFT + order));
> > + }
> > +
> > + /* Guest RAM bank in kinfo hasn't been initialized. */
> > + if ( gbank == kinfo->mem.nr_banks )
> > + {
> > + kinfo->mem.bank[gbank].start = rambase[gbank];
> > + kinfo->mem.nr_banks++;
> > + }
> > + kinfo->mem.bank[gbank].size += gsize;
> > +
> > + return true;
> > +}
> > +
> > static void __init allocate_memory(struct domain *d, struct
> > kernel_info *kinfo)
>
> It feels a bit odd that the two functions you introduced are not together. Can
> they be moved together?
>

Ok. Will do.

> > {
> > unsigned int i;
> > @@ -480,6 +522,116 @@ fail:
> > (unsigned long)kinfo->unassigned_mem >> 10);
> > }
> >
> > +/* Allocate memory from static memory as RAM for one specific domain
> > +d. */ static u64 __init allocate_static_memory(struct domain *d,
> > + struct kernel_info *kinfo,
> > + const struct dt_device_node
> > +*node) {
> > + int nr_banks, bank = 0, gbank = 0;
> > + const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> > + const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> > + const __be32 *cell;
> > + const struct dt_property *prop;
> > + struct dt_device_node *static_mem_node;
> > + const struct dt_device_node *parent =
> dt_find_node_by_path("/reserved-memory");
> > + u32 addr_cells = 2, size_cells = 2, reg_cells;
> > + u64 tot_size;
> > +
> > + paddr_t pbase, psize, gsize;
> > + gfn_t sgfn;
> > + mfn_t smfn;
> > +
> > + kinfo->mem.nr_banks = 0;
> > + /* Start with GUEST_RAM0. */
> > + gsize = ramsize[gbank];
> > + sgfn = gaddr_to_gfn(rambase[gbank]);
> > +
> > + /* Parse phandle in `xen,static-mem`. */
> > + static_mem_node = dt_parse_phandle(node, "xen,static-mem", 0);
> > + if ( !static_mem_node )
> > + goto fail;
> > +
> > + /*
> > + * #address-cells and #size-cells must be consistent with the parent node,
> > + * "reserved-memory".
> > + */
> > + dt_property_read_u32(parent, "#address-cells", &addr_cells);
> > + dt_property_read_u32(parent, "#size-cells", &size_cells);
>
> The return for dt_property_read_u32() should be checked.

Sure.

>
> > + BUG_ON(size_cells > 2 || addr_cells > 2);
> > + reg_cells = addr_cells + size_cells;
> > +
> > + prop = dt_find_property(static_mem_node, "reg", NULL);
> > + if ( !prop )
> > + goto fail;
> > + cell = (const __be32 *)prop->value;
> > + nr_banks = (prop->length) / (reg_cells * sizeof (u32));
> > + BUG_ON(nr_banks > NR_MEM_BANKS);
> > +
> > + while ( bank < nr_banks )
> > + {
> > + device_tree_get_reg(&cell, addr_cells, size_cells, &pbase,
> > + &psize);
>
> We seem to have quite a few functions now that will iterate over "regs".
> It would be worth considering to introduce a helper to iterate it.
>

Do you want to expand the "device_tree_get_reg" to only cope with "regs" property,
right now, it passes into cells, "prop->value", the cells of the reg property.
Changing it to that the input parameter will be the device node holding the reg property, like
"device_tree_get_reg(const struct dt_device_node *node, u32 addr_cells, u32 size_cells, struct meminfo *info)"
Or we still retain the original device_tree_get_reg()(maybe, the name shall be changed....), and
Introduce a new function to do what I said above.

> > + tot_size += (u64)psize;
>
> This cast seems unnecessary.
>
> > + smfn = maddr_to_mfn(pbase);
> > +
> > + if ( !alloc_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )
> > + {
> > + printk(XENLOG_ERR
> > + "%pd: cannot allocate static memory"
> > + "(0x%"PRIpaddr" - 0x%"PRIpaddr")",
> > + d, pbase, pbase + psize);
> > + goto fail;
> > + }
> > +
> > + printk(XENLOG_INFO "%pd STATIC BANK[%d] %#"PRIpaddr"-
> %#"PRIpaddr"\n",
> > + d, bank, pbase, pbase + psize);
> > +
> > + /*
> > + * It shall be mapped to the fixed guest RAM address rambase[i],
> > + * And until it exhausts the ramsize[i], it will seek to the next
> > + * rambase[i+1].
> > + */
> > + while ( 1 )
> > + {
> > + if ( gsize >= psize )
> > + {
> > + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> > + &sgfn, smfn, psize) )
> > + goto fail;
> > +
> > + gsize = gsize - psize;
> > + bank++;
> > + break;
> > + }
> > + else
> > + {
> > + if ( !allocate_static_bank_memory(d, kinfo, gbank,
> > + &sgfn, smfn, gsize) )
> > + goto fail;
> > +
> > + /*
> > + * Physical bank hasn't been totally mapped,
> > + * seeking to the next guest RAM i+1, if exist.
> > + */
> > + if ( ++gbank < GUEST_RAM_BANKS )
> > + {
> > + psize = psize - gsize;
> > + smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> > + gsize = ramsize[gbank];
> > + sgfn = gaddr_to_gfn(rambase[gbank]);
> > + }
> > + else
> > + goto fail;
> > + }
>
> The double loop is not nice but I can't think of a better way. However, I think
> the code in the loop can be simplified. You could write something like:
>
> /* Map as much as possible the static range to the guest bank */ if
> ( !allocate_static_bank(..., min(psize, gsize) )
> goto fail;
>
> /* The physical bank is fully mapped. Handle the next bank. */ if ( gsize >=
> psize ) {
> gsize = gsize - psize;
> bank++;
> break;
> }
> /* We still have memory to map. Check if we have another guest bank
> available */ else if ( ++gbank > GUEST_RAM_BANKS ) {
> printk("Exhausted the number of guest bank\n");
> goto fail;
> }
>
> [Update to the next guest bank]
>

Thanks for the detailed simplification!!!

> > + }
> > + }
> > + return tot_size;
> > +
> > +fail:
> > + panic("Failed to allocate requested static memory for domain %pd."
> > + "Fix the VMs configurations.\n",
> > + d);
> > +}
> > +
> > static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
> > const struct dt_device_node *node)
> > {
> > @@ -2437,8 +2589,7 @@ static int __init construct_domU(struct domain *d,
> > if ( prop )
> > {
> > static_mem = true;
> > - /* static_mem_size = allocate_static_memory(...); */
> > - BUG();
> > + static_mem_size = allocate_static_memory(d, &kinfo, node);
> > }
> >
> > rc = dt_property_read_u64(node, "memory", &mem);
> >
>
> Cheers,
>
> --
> Julien Grall

Cheers

--
Penny Zheng