Re: [Xen-devel] [PATCH] xen: fix setup of PVH Dom0 memory map

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.05.14 at 17:56, <roger.pau@xxxxxxxxxx> wrote:
> @@ -353,6 +356,9 @@ static __init void pvh_map_all_iomem(struct domain *d)
>                  nump = end_pfn - start_pfn;
>                  /* Add pages to the mapping */
>                  pvh_add_mem_mapping(d, start_pfn, start_pfn, nump);
> +                if ( start_pfn <= nr_pages )

I guess to avoid future questions or confusion, this ought to be <
instead of <=.

> @@ -369,6 +375,107 @@ static __init void pvh_map_all_iomem(struct domain *d)
>          nump = end_pfn - start_pfn;
>          pvh_add_mem_mapping(d, start_pfn, start_pfn, nump);
>      }
> +
> +    /*
> +     * Add the memory removed by the holes at the end of the
> +     * memory map.
> +     */
> +    for ( i = 0, entry = e820.map; i < e820.nr_map && nr_holes > 0;
> +          i++, entry++ )
> +    {
> +        if ( entry->type != E820_RAM )
> +            continue;
> +
> +        end_pfn = PFN_UP(entry->addr + entry->size);
> +        if ( end_pfn <= nr_pages )
> +            continue;
> +
> +        navail = end_pfn - nr_pages;
> +        nmap = navail > nr_holes ? nr_holes : navail;
> +        nr_holes -= nmap;
> +        start_pfn = PFN_DOWN(entry->addr) < nr_pages ?
> +                        nr_pages : PFN_DOWN(entry->addr);
> +        order = get_order_from_pages(nmap);
> +        page = alloc_domheap_pages(d, order, 0);
> +        if ( !page )
> +            panic("Not enough RAM for domain 0");
> +        free_pages = 1UL << order;
> +        while ( nmap )
> +        {
> +            order = get_order_from_pages(nmap);
> +            /*
> +             * get_order_from_pages ceils the allocation,
> +             * but we don't want to add more memory than the
> +             * requested amount, so always use at least one
> +             * order less than the returned.
> +             */
> +            order = order > 9 ? 9 : (order > 0 ? order - 1 : 0);

I don't understand this at all: You did an allocation of "order", why
would you not want to put this in all in one go? What you need to
do instead is decrement the order _before_ attempting the
allocation (see alloc_chunk() - I wonder whether you shouldn't
actually use it), or else you risk triggering the code path issuing
"Over-allocation for domain %u: ...".

> +            rc = guest_physmap_add_page(d, start_pfn, page_to_mfn(page), 
> order);

And actually I was wrong with what I said on the previous round:
While the backend functions indeed only support 0, 9, and 18 as
order, p2m_set_entry() takes care of any other orders. Hence you
should indeed be able to get away without any (inner) loop here.

> +static __init void pvh_setup_e820(struct domain *d, unsigned long nr_pages)
> +{
> +    struct e820entry *entry, *entry_guest;
> +    unsigned int i;
> +    unsigned long pages, cur_pages = 0;
> +
> +    /*
> +     * Craft the e820 memory map for Dom0 based on the hardware e820 map.
> +     */
> +    d->arch.e820 = xzalloc_array(struct e820entry, e820.nr_map);
> +    if ( !d->arch.e820 )
> +        panic("Unable to allocate memory for Dom0 e820 map");
> +    entry_guest = d->arch.e820;
> +
> +    /* Clamp e820 memory map to match the memory assigned to Dom0 */
> +    for ( i = 0, entry = e820.map; i < e820.nr_map; i++, entry++ )
> +    {
> +        if ( entry->type != E820_RAM )
> +        {
> +            memcpy(entry_guest++, entry, sizeof(*entry));

I'd prefer if you used assignments for structure copies, as they're type-
safe, while memcpy() isn't.

> +            d->arch.nr_e820++;
> +            continue;
> +        }
> +
> +        if ( nr_pages == cur_pages )
> +        {
> +            /*
> +             * We already have all the assigned memory,
> +             * skip this entry
> +             */
> +            continue;
> +        }
> +
> +        memcpy(entry_guest, entry, sizeof(*entry));
> +        pages = entry_guest->size >> PAGE_SHIFT;

Aren't you relying on the entries being page-aligned, or the guest
ignoring non-page-aligned leading/trailing fragments?

Jan

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