Re: [PATCH V4 10/10] xen/arm: introduce allocate_static_memory

[Julien Grall <julien@xen.org>]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This commit introduces allocate_static_memory to allocate static memory as
> guest RAM for Domain on Static Allocation.
> 
> It uses acquire_domstatic_pages to acquire pre-configured static memory
> for this domain, and uses guest_physmap_add_page to set up P2M table.
> These pre-defined static memory banks 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`, and so on.
> `GUEST_RAM0` may take up several pre-defined physical RAM regions.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> ---
>   xen/arch/arm/domain_build.c | 137 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 135 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index cdb16f2086..ed290ee31b 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -480,6 +480,139 @@ fail:
>             (unsigned long)kinfo->unassigned_mem >> 10);
>   }
>   
> +static bool __init append_static_memory_to_bank(struct domain *d,
> +                                                struct membank *bank,
> +                                                mfn_t smfn,
> +                                                paddr_t size)
> +{
> +    int res;
> +    paddr_t tot_size = size;
> +    /* Infer next GFN. */
> +    gfn_t sgfn = gaddr_to_gfn(bank->start + bank->size);
> +
> +    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;
> +        }
> +
> +        smfn = mfn_add(smfn, 1UL << order);
> +        tot_size -= (1UL << (PAGE_SHIFT + order));
> +    }

AFAICT, the loop is only here to suit guest_physmap_add_page(). Further 
down the line, the order will be converted back to a number of pages 
before calling p2m_insert_mapping().

So how about exporting p2m_insert_mapping() and use it?

> + > +    bank->size = bank->size + size;

We usually add a newline before the last return of the function.

> +    return true;
> +}
> +
> +/* Allocate memory from static memory as RAM for one specific domain d. */
> +static void __init allocate_static_memory(struct domain *d,
> +                                          struct kernel_info *kinfo,
> +                                          const struct dt_property *prop,
> +                                          u32 addr_cells, u32 size_cells)
> +{
> +    unsigned int nr_banks, gbank, bank = 0;
> +    const uint64_t rambase[] = GUEST_RAM_BANK_BASES;
> +    const uint64_t ramsize[] = GUEST_RAM_BANK_SIZES;
> +    const __be32 *cell;
> +    u32 reg_cells = addr_cells + size_cells;
> +    u64 tot_size = 0;
> +    paddr_t pbase, psize, gsize;
> +    mfn_t smfn;
> +
> +    /* Start with GUEST_RAM0. */
> +    kinfo->mem.nr_banks = 0;
> +    gbank = 0;
> +    gsize = ramsize[gbank];
> +    kinfo->mem.bank[gbank].start = rambase[gbank];
> +
> +    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 += psize;
> +        smfn = maddr_to_mfn(pbase);
> +
> +        if ( !acquire_domstatic_pages(d, psize >> PAGE_SHIFT, smfn, 0) )

I think we want to check that both pbase and psize are page aligned 
first. This can be done here or earlier when reserving the pages (this 
would be a previous patch).

Also, given that you can easily figure out the page from the mfn. I 
think it would be better for acquire_domstatic_pages() to return an 
error. This could be helpful to figure out an error.

> +        {
> +            printk(XENLOG_ERR
> +                    "%pd: cannot acquire static memory "
> +                    "(0x%"PRIpaddr" - 0x%"PRIpaddr").\n",
> +                    d, pbase, pbase + psize);
> +            goto fail;
> +        }
> +
> +        printk(XENLOG_INFO "%pd: STATIC BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"\n",

bank is unsigned so s/%d/%u/

> +               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 )
> +        {
> +            /*
> +             * The current physical bank is fully mapped.
> +             * Handle the next physical bank.
> +             */
> +            if ( gsize >= psize )
> +            {
> +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> +                                                   smfn, psize) )
> +                    goto fail;
> +
> +                gsize = gsize - psize;
> +                bank++;
> +                break;
> +            }
> +            /*
> +             * Current guest bank memory is not enough to map.
> +             * Check if we have another guest bank available.
> +             * gbank refers guest memory bank index.
> +             */
> +            else if ( (gbank + 2) > GUEST_RAM_BANKS ) {

I don't understand the +2. Can you clarify it?

Also, the coding style for Xen requires the { to be on a separate line.

> +                printk("Exhausted the number of guest bank\n");
> +                goto fail;
> +            }
> +            else
> +            {
> +                if ( !append_static_memory_to_bank(d, &kinfo->mem.bank[gbank],
> +                                                   smfn, gsize) )
> +                    goto fail;

As I may have mentionned earlier, I find the double loop quite difficult 
to read. I don't think we can drop the double loop, but we can at least 
try to simplify the code in the loops.

The one I can think right now is moving allocate_static_memory_to_bank() 
outside of the if/else. Something like:

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

> +
> +                psize = psize - gsize;
> +                smfn = mfn_add(smfn, gsize >> PAGE_SHIFT);
> +                /* Update to the next guest bank. */
> +                gbank++;
> +                gsize = ramsize[gbank];
> +                kinfo->mem.bank[gbank].start = rambase[gbank];
> +            }
> +        }
> +    }
> +
> +    kinfo->mem.nr_banks = ++gbank;
> +    kinfo->unassigned_mem -= tot_size;
> +    if ( kinfo->unassigned_mem )
> +        printk(XENLOG_ERR
> +               "Size of \"memory\" property doesn't match up with the ones "
> +               "defined in \"xen,static-mem\".\n");

We don't split the single line message accross multi-line even if the 
result code is more than 80 characters long.

> +
> +    return;
> +
> +fail:
> +    panic("Failed to allocate requested static memory for domain %pd."
> +          "Fix the VMs configurations.\n",

Same here.

> +          d);
> +}
> +
>   static int __init write_properties(struct domain *d, struct kernel_info *kinfo,
>                                      const struct dt_device_node *node)
>   {
> @@ -2486,8 +2619,8 @@ static int __init construct_domU(struct domain *d,
>       if ( !static_mem )
>           allocate_memory(d, &kinfo);
>       else
> -        /* TODO: allocate_static_memory(...). */
> -        BUG();
> +        allocate_static_memory(d, &kinfo, static_mem_prop,
> +                               static_mem_addr_cells, static_mem_size_cells);
>   
>       rc = prepare_dtb_domU(d, &kinfo);
>       if ( rc < 0 )
> 

Cheers,

-- 
Julien Grall