Re: [Xen-devel] [PATCH v3 17/25] xen/arm: introduce allocate_memory

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 1 Aug 2018, Julien Grall wrote:
> Hi,
> 
> On 01/08/18 00:28, 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.
> > 
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v3:
> > - new patch
> > ---
> >   xen/arch/arm/domain_build.c | 125
> > +++++++++++++++++++++++++++++++++++++++++++-
> >   1 file changed, 124 insertions(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index ab72c36..dfa74e4 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -369,6 +369,129 @@ static void __init allocate_memory_11(struct domain
> > *d,
> >       }
> >   }
> >   +static bool __init insert_bank(struct domain *d,
> > +                               struct kernel_info *kinfo,
> > +                               struct page_info *pg,
> > +                               unsigned int order)
> > +{
> > +    int res, i;
> > +    mfn_t smfn;
> > +    paddr_t gaddr, size;
> > +    struct membank *bank;
> > +
> > +    smfn = page_to_mfn(pg);
> 
> This could combine with the declaration above.

OK


> > +    size = pfn_to_paddr(1UL << order);
> 
> Ditto.

OK


> > +
> > +    /*
> > +     * DomU memory is provided in two banks:
> > +     *   GUEST_RAM0_BASE - GUEST_RAM0_BASE + GUEST_RAM0_SIZE
> > +     *   GUEST_RAM1_BASE - GUEST_RAM1_BASE + GUEST_RAM1_SIZE
> > +     *
> > +     * Find the right gaddr address for DomUs accordingly.
> > +     */
> > +    gaddr = GUEST_RAM0_BASE;
> > +    if ( kinfo->mem.nr_banks > 0 )
> > +    {
> > +        for( i = 0; i < kinfo->mem.nr_banks; i++ )
> > +        {
> > +            bank = &kinfo->mem.bank[i];
> > +            gaddr = bank->start + bank->size;
> > +        }
> > +        if ( bank->start == GUEST_RAM0_BASE &&
> > +             gaddr + size > (GUEST_RAM0_BASE + GUEST_RAM0_SIZE) )
> > +            gaddr = GUEST_RAM1_BASE;
> > +        if ( bank->start == GUEST_RAM1_BASE &&
> > +             gaddr + size > (GUEST_RAM1_BASE + GUEST_RAM1_SIZE) )
> > +            goto fail;
> > +    }
> 
> I still really dislike this code. This is difficult to understand and not
> scalable. As I said in the previous version, it would be possible to have more
> than 2 banks in the future. This will either come with PCI PT or dynamic
> memory layout.
> 
> What should really be done is a function allocate_memory that take in
> parameter the range to allocate. E.g
> 
> allocate_bank_memory(struct domain *d, gfn_t sgfn, unsigned long order);
> 
> Then the function allocate_memory will compute the size of each bank based on
> mem_ and call allocate_bank_memory for each bank.

I'll make the change.


> > +
> > +    dprintk(XENLOG_INFO,
> > +            "Allocated %#"PRIpaddr"-%#"PRIpaddr":%#"PRIpaddr"-%#"PRIpaddr"
> > (%ldMB/%ldMB, order %d)\n",
> 
> It would be possible to request a guest with 16KB of memory. This would be
> printed as 0.

I'll printk KBs instead of MBs.


> > +            mfn_to_maddr(smfn), mfn_to_maddr(smfn) + size,
> > +            gaddr, gaddr + size,
> > +            1UL << (order + PAGE_SHIFT - 20),
> > +            /* Don't want format this as PRIpaddr (16 digit hex) */
> > +            (unsigned long)(kinfo->unassigned_mem >> 20),
> > +            order);
> > +
> > +    res = guest_physmap_add_page(d, gaddr_to_gfn(gaddr), smfn, order);
> > +    if ( res )
> > +    {
> > +        dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> > +        goto fail;
> > +    }
> > +
> > +    kinfo->unassigned_mem -= size;
> > +    bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> > +
> > +    bank->start = gaddr;
> > +    bank->size = size;
> > +    kinfo->mem.nr_banks++;
> > +    return true;
> > +
> > +fail:
> > +    free_domheap_pages(pg, order);
> > +    return false;
> > +}
> > +
> > +static void __init allocate_memory(struct domain *d, struct kernel_info
> > *kinfo)
> > +{
> > +    const unsigned int min_order = get_order_from_bytes(MB(4));
> 
> Why do you have this limitation for non-direct mapped domain? There are
> nothing wrong to allocate 2MB/4K pages for them.

I'll remove


> > +    struct page_info *pg;
> > +    unsigned int order = get_allocation_size(kinfo->unassigned_mem);
> > +    int i;
> > +
> > +    dprintk(XENLOG_INFO, "Allocating mappings totalling %ldMB for
> > dom%d:\n",
> 
> Ditto.

I'll print KBs


> > +            /* Don't want format this as PRIpaddr (16 digit hex) */
> > +            (unsigned long)(kinfo->unassigned_mem >> 20), d->domain_id);
> > +
> > +    kinfo->mem.nr_banks = 0;
> > +
> > +    order = get_allocation_size(kinfo->unassigned_mem);
> > +    if ( order > GUEST_RAM0_SIZE )
> > +        order = GUEST_RAM0_SIZE;
> 
> I don't understand this check. You are comparing a power of 2 with KB.

I'll fix


> > +    while ( kinfo->unassigned_mem )
> > +    {
> > +        pg = alloc_domheap_pages(d, order, 0);
> > +        if ( !pg )
> > +        {
> > +            order --;
> > +
> > +            if ( order >= min_order )
> > +                continue;
> > +
> > +            /* No more we can do */
> > +            break;
> > +        }
> > +
> > +        if ( !insert_bank(d, kinfo, pg, order) )
> > +            break;
> > +
> > +        /*
> > +         * Success, next time around try again to get the largest order
> > +         * allocation possible.
> > +         */
> > +        order = get_allocation_size(kinfo->unassigned_mem);
> > +    }
> > +
> > +    if ( kinfo->unassigned_mem )
> > +        dprintk(XENLOG_WARNING, "Failed to allocate requested domain
> > memory."
> > +               /* Don't want format this as PRIpaddr (16 digit hex) */
> > +               " %ldMB unallocated\n",
> > +               (unsigned long)kinfo->unassigned_mem >> 20);
> 
> I understand this is the current behavior for direct mapped domain. It makes
> sense for them because we don't want to create very small bank (we request 4MB
> min). But for non direct mapped domain, there are no need for such limitation.
> So if you don't allocate the memory, then it means Xen has no more RAM and it
> makes little sense to boot them.

Yes, it makes sense to print an error instead of a warning and stop the
boot, so that the user can go back and fix the configuration.


> > +
> > +    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));
> > +    }
> > +}
> > +
> >   static int __init write_properties(struct domain *d, struct kernel_info
> > *kinfo,
> >                                      const struct dt_device_node *node)
> >   {
> > @@ -2241,7 +2364,7 @@ static int __init construct_domU(struct domain *d,
> > struct dt_device_node *node)
> >       /* type must be set before allocate memory */
> >       d->arch.type = kinfo.type;
> >   #endif
> > -    allocate_memory_11(d, &kinfo);
> > +    allocate_memory(d, &kinfo);
> 
> The call to allocate_memory_11() should have never been added. Please
> refactor/re-order the patch to avoid introducing wrong code.

done

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