Re: [PATCH v2 6/8] tools: arm: refactor code to setup guest p2m and fill it with RAM

[Ian Campbell <Ian.Campbell@citrix.com>]

On Fri, 2014-04-25 at 13:51 +0100, Julien Grall wrote:
> Hi Ian,
> 
> On 25/04/14 12:22, Ian Campbell wrote:
> > +static int populate_guest_memory(struct xc_dom_image *dom,
> > +                                 xen_pfn_t base_pfn, xen_pfn_t nr_pfns)
> > +{
> > +    int rc;
> > +    xen_pfn_t allocsz, pfn;
> > +
> > +    if (!nr_pfns)
> > +        return 0;
> > +
> > +    DOMPRINTF("%s: populating RAM @ %016"PRIx64"-%016"PRIx64" (%"PRId64"MB)",
> > +              __FUNCTION__,
> > +              (uint64_t)base_pfn << XC_PAGE_SHIFT,
> > +              (uint64_t)(base_pfn + nr_pfns) << XC_PAGE_SHIFT,
> > +              (uint64_t)nr_pfns >> (20-XC_PAGE_SHIFT));
> > +
> > +    for ( pfn = 0; pfn < nr_pfns; pfn++ )
> > +        dom->p2m_host[pfn] = base_pfn + pfn;
> > +
> > +    for ( pfn = rc = allocsz = 0; (pfn < nr_pfns) && !rc; pfn += allocsz )
> 
> May I ask for a bit of clean up here?

This is code motion. I deliberately don't want to change it for that
reason.

> > @@ -292,27 +326,17 @@ int arch_setup_meminit(struct xc_dom_image *dom)
> >
> >       dom->shadow_enabled = 1;
> >
> > -    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * dom->total_pages);
> > +    dom->p2m_host = xc_dom_malloc(dom, sizeof(xen_pfn_t) * p2m_size);
> >       if ( dom->p2m_host == NULL )
> >           return -EINVAL;
> > +    for ( pfn = 0; pfn < p2m_size; pfn++ )
> > +        dom->p2m_host[pfn] = INVALID_MFN;
> 
> With this solution, you will loop 262244 times for nothing (the hole 
> between the 2 banks).

Yes, this is the simplest way to ensure that p2m_host is definitely
completely initialised, irrespective of the presence of any holes in the
address space.

> Also when the guess will have lots of RAM, it will be slow because we 
> loop nearly twice the array (one here, the other in populate_guest_memory).

This is dwarfed by all the other overheads of course, like actually
filling in the RAM on the second pass.

> It think we can avoid looping twice by making the two banks contiguous 
> in the memory (i.e starting the second bank at 4GB instead of 8GB).

As explained in the commit message I have deliberately left a hole so
that we can see that such configurations actually work.

Ian.