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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> @@ -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?

Also, why not

        avail -= iommu_hap_pt_share ? 0 : paging_mem;

?

Jan


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