Re: [Xen-devel] [PATCH v5 11/25] xen/arm: introduce allocate_memory

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Tue, 30 Oct 2018, Julien Grall wrote:
> Hi Stefano,
> 
> More comments on it :).
> 
> On 10/23/18 3:02 AM, Stefano Stabellini wrote:
> > Introduce an allocate_memory function able to allocate memory for DomUs
> > and map it at the right guest addresses, according to the guest memory
> > map: GUEST_RAM0_BASE and GUEST_RAM1_BASE.
> > 
> > This is under #if 0 as not used for now.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v5:
> > - improve commit message
> > - code style
> > - remove unneeded local var
> > - while loop in allocate_bank_memory to allocate memory so that it
> >    doesn't have to be contiguos
> > - combile while loops in allocate_memory
> > 
> > Changes in v4:
> > - move earlier, add #if 0
> > - introduce allocate_bank_memory, remove insert_bank
> > - allocate_bank_memory allocate memory and inserts the bank, while
> >    allocate_memory specifies where to do that
> > 
> > Changes in v3:
> > - new patch
> > ---
> >   xen/arch/arm/domain_build.c | 99
> > +++++++++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 99 insertions(+)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index c61a27f..146d81e 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -368,6 +368,105 @@ static void __init allocate_memory_11(struct domain
> > *d,
> >       }
> >   }
> >   +#if 0
> > +static bool __init allocate_bank_memory(struct domain *d,
> > +                                        struct kernel_info *kinfo,
> > +                                        gfn_t sgfn,
> > +                                        unsigned int order)
> > +{
> > +    int res;
> > +    struct page_info *pg;
> > +    struct membank *bank;
> > +    paddr_t size = pfn_to_paddr(1UL << order);
> > +    gfn_t _sgfn = sgfn;
> > +    gfn_t _egfn = gfn_add(sgfn, 1UL << order);
> > +
> > +    while ( gfn_x(_sgfn) < gfn_x(_egfn) )
> > +    {
> > +        pg = alloc_domheap_pages(d, order, 0);
> > +        if ( pg != NULL )
> > +        {
> > +            res = guest_physmap_add_page(d, _sgfn, page_to_mfn(pg), order);
> > +            if ( res )
> > +            {
> > +                dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> > +                goto fail;
> > +            }
> > +            _sgfn = gfn_add(_sgfn, 1UL << order);
> > +        }
> > +        else
> > +        {
> > +            order--;
> > +            if ( order == 0 )
> > +            {
> > +                dprintk(XENLOG_ERR, "Failed to allocated DOMU memory");
> > +                goto fail;
> > +            }
> > +        }
> > +    }
> > +
> > +    bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> > +    bank->start = gfn_to_gaddr(sgfn);
> > +    bank->size = size;
> > +    kinfo->mem.nr_banks++;
> > +    kinfo->unassigned_mem -= size;
> > +
> > +    return true;
> > +
> > +fail:
> > +    /*
> > +     * Fatal failure, don't attempt to free memory.
> > +     */
> > +    return false;
> > +}
> > +
> > +static void __init allocate_memory(struct domain *d, struct kernel_info
> > *kinfo)
> > +{
> > +    unsigned int order = get_allocation_size(kinfo->unassigned_mem);
> > +    unsigned int order_req;
> > +    unsigned int i;
> > +
> > +    dprintk(XENLOG_INFO, "Allocating mappings totalling %ldMB for
> > dom%d:\n",
> > +            /* Don't want format this as PRIpaddr (16 digit hex) */
> > +            (unsigned long)(kinfo->unassigned_mem >> 20), d->domain_id);
> 
> While you are re-spinning this series. I would turn all the dprintk to printk
> as those messages are useful in non-debug build.

OK


> > +
> > +    kinfo->mem.nr_banks = 0;
> > +
> > +    order = get_allocation_size(kinfo->unassigned_mem);
> > +    if ( order > get_order_from_bytes(GUEST_RAM0_SIZE) )
> > +        order_req = get_order_from_bytes(GUEST_RAM0_SIZE);
> > +    else
> > +        order_req = order;
> > +    if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> > order_req) )
> > +        goto fail;
> > +
> > +    order -= order_req;
> > +    if ( order > 0 &&
> > +         !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> > order) )
> > +        goto fail;
> > +
> > +    for( i = 0; i < kinfo->mem.nr_banks; i++ )
> > +    {
> > +        dprintk(XENLOG_INFO, "Dom%d BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"
> > (%ldMB)\n",
> > +                d->domain_id,
> > +                i,
> > +                kinfo->mem.bank[i].start,
> > +                kinfo->mem.bank[i].start + kinfo->mem.bank[i].size,
> > +                /* Don't want format this as PRIpaddr (16 digit hex) */
> > +                (unsigned long)(kinfo->mem.bank[i].size >> 20));
> > +    }
> > +
> > +    return;
> > +
> > +fail:
> > +    dprintk(XENLOG_ERR, "Failed to allocate requested domain memory."
> > +            /* Don't want format this as PRIpaddr (16 digit hex) */
> > +            " %ldKB unallocated. Fix the VMs configurations.\n",
> > +            (unsigned long)kinfo->unassigned_mem >> 10);
> 
> For this one, I would turned to a panic, avoid the BUG() below.

OK


> > +    BUG();
> > +}
> > +#endif
> > +
> >   static int __init write_properties(struct domain *d, struct kernel_info
> > *kinfo,
> >                                      const struct dt_device_node *node)
> >   {

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel