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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, July 3, 2021 10:18 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; jbeulich@xxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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