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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, May 18, 2021 8:06 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; nd <nd@xxxxxxx>
> Subject: Re: [PATCH 10/10] xen/arm: introduce allocate_static_memory
> 
> 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?
> 

We need to constructing kinfo->mem(guest RAM banks) here, and
we are indexing in static_mem(physical ram banks). Multiple physical ram
banks consist of one guest ram bank(like, GUEST_RAM0). 

ram_addr  here will either be GUEST_RAM0_BASE, or GUEST_RAM1_BASE,
for now. I kinds struggled in how to name it. And maybe it shall not be a
parameter here.

Maybe I should switch.. case.. on the ram_index, if its 0, its GUEST_RAM0_BASE,
And if its 1, its GUEST_RAM1_BASE.

> > +                                               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'.
> 

Sure, I’ll use the 'bank' here.

> > +    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.

Sure, I'll rename to make it more clearly.

> 
> > +
> > +        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.

Sure, I'll change

> 
> > +                    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?
> 

Correct me if I understand wrongly, 

What you suggest here is that we make an array of guest banks, right now, 
including
GUEST_RAM0 and GUEST_RAM1. And if later, adding more guest banks, it will not
bring scalability problem here, right?


> Cheers,
> 
> --
> Julien Grall

Cheers

Penny Zheng