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: Thursday, May 20, 2021 4:10 AM
> 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
> 
> 
> 
> On 19/05/2021 08:27, Penny Zheng wrote:
> > 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.
> 
> You only need to set kinfo->mem.bank[ram_index].start once. This is when
> you know the bank is first used.
> 
> AFAICT, this function will map the memory for a range start at ``sgfn``.
> It doesn't feel this belongs to the function.
> 
> The same remark is valid for kinfo->mem.nr_banks.
> 

Ok. I finally totally understand what you suggest here.
I'll try to let the action related to setting kinfo->mem.bank[ram_index].start/
kinfo->mem.bank[ram_index].size/ kinfo->mem. nr_banks out of this function,
and only keep the simple functionality of mapping the memory for a range start
at ``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'.
> >>
> >
> > 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?
> 
> Yes. This should also reduce the current complexity of the code.
> 
> Cheers,
> 
> --
> Julien Grall

Cheers

Penny