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

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

On 18/05/2021 06:21, 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-defined static memory banks
> for this domain, and uses guest_physmap_add_page to set up P2M table,
> guest starting at fixed GUEST_RAM0_BASE, GUEST_RAM1_BASE.
>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> ---
>   xen/arch/arm/domain_build.c | 157 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 155 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 30b55588b7..9f662313ad 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -437,6 +437,50 @@ static bool __init allocate_bank_memory(struct domain *d,
>       return true;
>   }
>   
> +/*
> + * #ram_index and #ram_index refer to the index and starting address of guest
> + * memory kank stored in kinfo->mem.
> + * Static memory at #smfn of #tot_size shall be mapped #sgfn, 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 ram_index,
Please use unsigned.

> +                                               paddr_t ram_addr,
> +                                               gfn_t* sgfn,
I am confused, what is the difference between ram_addr and sgfn?

> +                                               mfn_t smfn,
> +                                               paddr_t tot_size)
> +{
> +    int res;
> +    struct membank *bank;
> +    paddr_t _size = tot_size;
> +
> +    bank = &kinfo->mem.bank[ram_index];
> +    bank->start = ram_addr;
> +    bank->size = bank->size + tot_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;
> +        }
> +
> +        *sgfn = gfn_add(*sgfn, 1UL << order);
> +        smfn = mfn_add(smfn, 1UL << order);
> +        tot_size -= (1ULL << (PAGE_SHIFT + order));
> +    }
> +
> +    kinfo->mem.nr_banks = ram_index + 1;
> +    kinfo->unassigned_mem -= _size;
> +
> +    return true;
> +}
> +
>   static void __init allocate_memory(struct domain *d, struct kernel_info 
> *kinfo)
>   {
>       unsigned int i;
> @@ -480,6 +524,116 @@ fail:
>             (unsigned long)kinfo->unassigned_mem >> 10);
>   }
>   
> +/* 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)
> +{
> +    int nr_banks, _banks = 0;
AFAICT, _banks is the index in the array. I think it would be clearer if 
it is caller 'bank' or 'idx'.
> +    size_t ram0_size = GUEST_RAM0_SIZE, ram1_size = GUEST_RAM1_SIZE;
> +    paddr_t bank_start, bank_size;
> +    gfn_t sgfn;
> +    mfn_t smfn;
> +
> +    kinfo->mem.nr_banks = 0;
> +    sgfn = gaddr_to_gfn(GUEST_RAM0_BASE);
> +    nr_banks = d->arch.static_mem.nr_banks;
> +    ASSERT(nr_banks >= 0);
> +
> +    if ( kinfo->unassigned_mem <= 0 )
> +        goto fail;
> +
> +    while ( _banks < nr_banks )
> +    {
> +        bank_start = d->arch.static_mem.bank[_banks].start;
> +        smfn = maddr_to_mfn(bank_start);
> +        bank_size = d->arch.static_mem.bank[_banks].size;
The variable name are slightly confusing because it doesn't tell whether 
this is physical are guest RAM. You might want to consider to prefix 
them with p (resp. g) for physical (resp. guest) RAM.
> +
> +        if ( !alloc_domstatic_pages(d, bank_size >> PAGE_SHIFT, bank_start, 0) 
> )
> +        {
> +            printk(XENLOG_ERR
> +                    "%pd: cannot allocate static memory"
> +                    "(0x%"PRIx64" - 0x%"PRIx64")",
bank_start and bank_size are both paddr_t. So this should be PRIpaddr.

> +                    d, bank_start, bank_start + bank_size);
> +            goto fail;
> +        }
> +
> +        /*
> +         * By default, it shall be mapped to the fixed guest RAM address
> +         * `GUEST_RAM0_BASE`, `GUEST_RAM1_BASE`.
> +         * Starting from RAM0(GUEST_RAM0_BASE).
> +         */
Ok. So you are first trying to exhaust the guest bank 0 and then moved 
to bank 1. This wasn't entirely clear from the design document.
I am fine with that, but in this case, the developper should not need to 
know that (in fact this is not part of the ABI).
Regarding this code, I am a bit concerned about the scalability if we 
introduce a second bank.
Can we have an array of the possible guest banks and increment the index 
when exhausting the current bank?
Cheers,

--
Julien Grall