Re: [Xen-devel] [PATCH] x86/pvh: fix memory accounting for Dom0

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, Sep 28, 2017 at 01:18:55PM +0000, Jan Beulich wrote:
> >>> On 28.09.17 at 12:16, <roger.pau@xxxxxxxxxx> wrote:
> > Make sure that the memory for the paging structures in case of a HVM
> > Dom0 is subtracted from the total amount of memory available for Dom0
> > to use. Also take into account whether the IOMMU is sharing the
> > page tables with HAP, or else also reserve some memory for the IOMMU
> > page tables.
> > 
> > While there re-organize the code slightly so that the for loop and the
> > need_paging local variable can be removed.
> 
> These two things very definitely should not be merged into a single
> patch; I'm not convinced the reorg is correct in the first place. Note
> how avail, which is being changed in the first iteration of the loop,
> feeds back into the second iteration.

I'm afraid I don't understand why this is done. Also, the second loop
is only going to happen when need_paging is true, which only happens
for HVM guests using shadow or without shared pt with the IOMMU.

AFAICT the original code is wrong (or I don't understand it).

Please bear with me. So let's assume Xen is trying to create a PVH
Dom0 using HAP and with shared page tables with the IOMMU. In this
case need_paging is false, so the following flow will happen:

for ( ; ; need_paging = false )
{
    [nr_pages calculations]

    if ( !need_paging )
        break; <- break

    /* Not reached */
    /* Reserve memory for shadow or HAP. */
    avail -= dom0_paging_pages(d, nr_pages);
}

In which case the reservation of the memory required for paging it's
not going to happen.

> > @@ -263,39 +262,39 @@ unsigned long __init dom0_compute_nr_pages(
> >              avail -= max_pdx >> s;
> >      }
> >  
> > -    need_paging = is_hvm_domain(d) &&
> > -        (!iommu_hap_pt_share || !paging_mode_hap(d));
> > -    for ( ; ; need_paging = false )
> > -    {
> > -        nr_pages = dom0_nrpages;
> > -        min_pages = dom0_min_nrpages;
> > -        max_pages = dom0_max_nrpages;
> > +    nr_pages = dom0_nrpages;
> > +    min_pages = dom0_min_nrpages;
> > +    max_pages = dom0_max_nrpages;
> >  
> > -        /*
> > -         * If allocation isn't specified, reserve 1/16th of available 
> > memory
> > -         * for things like DMA buffers. This reservation is clamped to a
> > -         * maximum of 128MB.
> > -         */
> > -        if ( nr_pages == 0 )
> > -            nr_pages = -min(avail / 16, 128UL << (20 - PAGE_SHIFT));
> > +    /*
> > +     * If allocation isn't specified, reserve 1/16th of available memory
> > +     * for things like DMA buffers. This reservation is clamped to a
> > +     * maximum of 128MB.
> > +     */
> > +    if ( nr_pages == 0 )
> > +        nr_pages = -min(avail / 16, 128UL << (20 - PAGE_SHIFT));
> >  
> > -        /* Negative specification means "all memory - specified amount". */
> > -        if ( (long)nr_pages  < 0 ) nr_pages  += avail;
> > -        if ( (long)min_pages < 0 ) min_pages += avail;
> > -        if ( (long)max_pages < 0 ) max_pages += avail;
> > +    /* Negative specification means "all memory - specified amount". */
> > +    if ( (long)nr_pages  < 0 ) nr_pages  += avail;
> > +    if ( (long)min_pages < 0 ) min_pages += avail;
> > +    if ( (long)max_pages < 0 ) max_pages += avail;
> >  
> > -        /* Clamp according to min/max limits and available memory. */
> > -        nr_pages = max(nr_pages, min_pages);
> > -        nr_pages = min(nr_pages, max_pages);
> > -        nr_pages = min(nr_pages, avail);
> > +    /* Clamp according to min/max limits and available memory. */
> > +    nr_pages = max(nr_pages, min_pages);
> > +    nr_pages = min(nr_pages, max_pages);
> >  
> > -        if ( !need_paging )
> > -            break;
> > +    if ( is_hvm_domain(d) )
> > +    {
> > +        unsigned long paging_mem = dom0_paging_pages(d, nr_pages);
> >  
> >          /* Reserve memory for shadow or HAP. */
> > -        avail -= dom0_paging_pages(d, nr_pages);
> > +        avail -= paging_mem;
> > +        /* Reserve the same amount for the IOMMU page tables if not 
> > shared. */
> > +        avail -= !iommu_hap_pt_share ? paging_mem : 0;
> 
> If you account for IOMMU tables here, why don't you delete the
> code ahead of what so far was a loop?

Oh right, this chunk is incorrect, there's no need to account for the
IOMMU memory, the more that PV guests also make use of the IOMMU. As
you say the memory is already accounted for in the chunk above.

> Also, why not
> 
>         avail -= iommu_hap_pt_share ? 0 : paging_mem;
> 
> ?

Yes, can change to that, it's shorter :).

Thanks, Roger.

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