|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 12/26] xen/arm: introduce allocate_memory
On Fri, 9 Nov 2018, Julien Grall wrote:
> Hi Stefano,
>
> Most of the code is mine, so it is hard to review it :). Although, I have a
> few comments below.
>
>
> On 02/11/2018 23:45, 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: Julien Grall <julien.grall@xxxxxxx>
> > Signed-off-by: Stefano Stabellini <stefanos@xxxxxxxxxx>
> > ---
> > Changes in v6:
> > - turn dprintks into printk
> > - use panic instead of printk+BUG_ON
> > - use Julien's implementation of allocate_bank_memory
> >
> > 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 | 102
> > ++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 102 insertions(+)
> >
> > diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> > index 66a258a..86abcc6 100644
> > --- a/xen/arch/arm/domain_build.c
> > +++ b/xen/arch/arm/domain_build.c
> > @@ -368,6 +368,108 @@ 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 long tot_size)
> > +{
> > + int res;
> > + struct page_info *pg;
> > + struct membank *bank;
> > + gfn_t gfn = sgfn;
> > + unsigned long size = tot_size;
>
> The introduction for those 2 variables can be avoided if you pre-populate the
> bank before hand (see more below). I think this would avoid to confuse between
> the different variables.
OK
> > + unsigned int max_order = ~0;
> > +
> > + while ( size > 0 )
> > + {
> > + unsigned int order = get_allocation_size(size);
> > +
> > + order = min(max_order, order);
> > +
> > + pg = alloc_domheap_pages(d, order, 0);
> > + if ( !pg )
> > + {
> > + /*
> > + * If we can't allocate one page, then it is unlikely to
> > + * succeed in the next iteration. So bail out.
> > + */
> > + if ( !order )
> > + return false;
> > +
> > + /*
> > + * If we can't allocate memory with order, then it is
> > + * unlikely to succeed in the next iteration.
> > + * Record the order - 1 to avoid re-trying.
> > + */
> > + max_order = order - 1;
> > + continue;
> > + }
> > +
> > + res = guest_physmap_add_page(d, gfn, page_to_mfn(pg), order);
> > + if ( res )
> > + {
> > + dprintk(XENLOG_ERR, "Failed map pages to DOMU: %d", res);
> > + return false;
> > + }
> > +
> > + gfn = gfn_add(gfn, 1UL << order);
> > + size -= (1ULL << (PAGE_SHIFT + order));
> > + }
> > +
> > + bank = &kinfo->mem.bank[kinfo->mem.nr_banks];
> > + bank->start = gfn_to_gaddr(sgfn);
> > + bank->size = tot_size;
>
> In relation with my comment on the variables, this is the 3 lines that could
> be moved earlier.
understood
> > + kinfo->mem.nr_banks++;
> > + kinfo->unassigned_mem -= tot_size;
> > +
> > + return true;
> > +}
> > +
> > +static void __init allocate_memory(struct domain *d, struct kernel_info
> > *kinfo)
> > +{
> > + unsigned int i;
> > + unsigned long bank_size;
> > +
> > + printk(XENLOG_INFO "Allocating mappings totalling %ldMB for dom%d:\n",
>
> NIT: You can use the recently %pd to print the domain.
Sure
> > + /* Don't want format this as PRIpaddr (16 digit hex) */
> > + (unsigned long)(kinfo->unassigned_mem >> 20), d->domain_id);
> > +
> > + kinfo->mem.nr_banks = 0;
> > + bank_size = MIN(GUEST_RAM0_SIZE, kinfo->unassigned_mem);
> > + if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM0_BASE),
> > + bank_size) )
> > + goto fail;
> > +
> > + bank_size = MIN(GUEST_RAM1_SIZE, kinfo->unassigned_mem);
> > + if ( !allocate_bank_memory(d, kinfo, gaddr_to_gfn(GUEST_RAM1_BASE),
> > + bank_size) )
> > + goto fail;
> > +
> > + if ( kinfo->unassigned_mem )
> > + goto fail;
> > +
> > + for( i = 0; i < kinfo->mem.nr_banks; i++ )
> > + {
> > + printk(XENLOG_INFO "Dom%d BANK[%d] %#"PRIpaddr"-%#"PRIpaddr"
> > (%ldMB)\n",
>
> Same here.
OK
> > + 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:
> > + panic("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);
> > +}
> > +#endif
> > +
> > static int __init write_properties(struct domain *d, struct kernel_info
> > *kinfo,
> > const struct dt_device_node *node)
> > {
> >
>
> Cheers,
>
> --
> Julien Grall
>
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |