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

[Julien Grall <julien.grall@xxxxxxxxxx>]

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?
	- allocsz doesn't need to be initialized. It will be unconditionally 
set at the beginning of the loop
        - rc can be set 0 at the beginning of the function.

> +    {
> +        allocsz = nr_pfns - pfn;
> +        if ( allocsz > 1024*1024 )
> +            allocsz = 1024*1024;
> +
> +        rc = xc_domain_populate_physmap_exact(
> +            dom->xch, dom->guest_domid, allocsz,
> +            0, 0, &dom->p2m_host[pfn]);
> +    }
> +
> +    return rc;
> +}
> +
>   int arch_setup_meminit(struct xc_dom_image *dom)
>   {
>       int rc;
> -    xen_pfn_t pfn, allocsz, i;
> +    xen_pfn_t pfn;
>       uint64_t modbase;
>
>       /* Convenient */
> @@ -259,6 +291,8 @@ int arch_setup_meminit(struct xc_dom_image *dom)
>       const uint64_t ram0size = ramsize;
>       const uint64_t ram0end = GUEST_RAM0_BASE + ram0size;
>
> +    const xen_pfn_t p2m_size = (ram0end - GUEST_RAM0_BASE) >> XC_PAGE_SHIFT;
> +
>       const uint64_t kernbase = dom->kernel_seg.vstart;
>       const uint64_t kernend = ROUNDUP(dom->kernel_seg.vend, 21/*2MB*/);
>       const uint64_t kernsize = kernend - kernbase;
> @@ -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).
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).
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).
Regards,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel