Re: [Xen-devel] [PATCH v2 1/2] x86/dom0: rename paging function

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

On Tue, Dec 11, 2018 at 08:33:08AM -0700, Jan Beulich wrote:
> >>> On 11.12.18 at 16:19, <roger.pau@xxxxxxxxxx> wrote:
> > On Tue, Dec 11, 2018 at 08:08:51AM -0700, Jan Beulich wrote:
> >> >>> On 05.12.18 at 15:54, <roger.pau@xxxxxxxxxx> wrote:
> >> > To note it's calculating the approximate amount of memory required by
> >> > shadow paging.
> >> 
> >> I don't understand this logic, and ...
> >> 
> >> > @@ -325,7 +325,7 @@ unsigned long __init dom0_compute_nr_pages(
> >> >              break;
> >> >  
> >> >          /* Reserve memory for shadow or HAP. */
> >> > -        avail -= dom0_paging_pages(d, nr_pages);
> >> > +        avail -= dom0_shadow_pages(d, nr_pages);
> >> >      }
> >> 
> >> ... the comment here (and lack of conditional restricting the
> >> code to shadow mode) appear to support me: Have you
> >> been mislead by the function having a comment referring
> >> to libxl_get_required_shadow_memory()? I think if anything
> >> that libxl function would want to be renamed (to replace
> >> "shadow" by something more generic in its name).
> > 
> > But the logic in dom0_shadow_pages to calculate the size of the paging
> > memory pool is specifically for shadow AFAICT, I don't think HAP needs
> > to take the number of vCPUs into account, since there's only a
> > single p2m for the whole domain. OTOH shadow needs to take the number
> > of vCPUs into account because each one will have a different shadow.
> 
> Yes, the vCPU count aspect is indeed shadow specific. However,
> as said in reply to the other patch, the calculation here was at
> least supposed to also take into account the P2M part of the
> needed allocations. Yet the P2M part ought to be similar between
> both modes.
> 
> > Note that patch 2 in this series adds a function to calculate the size
> > of the paging memory pool for HAP, and a conditional is added to the
> > expression above that takes into account whether shadow or HAP is in
> > use when subtracting from the amount of available memory.
> 
> Well, assuming we can settle on what shape patch 2 should take
> I can see the point in doing the rename here, but then with an
> adjusted description: Especially in light of the code comment still
> visible above you'll want to point out that the rename is in
> preparation of splitting the calculations. Since I question the split,
> though, the rename (in a separate patch) is questionable to me
> too. If we used uniform P2M calculations and added just shadow's
> per-vCPU extra on top, no rename in a separate patch would
> seem warranted.

The current calculations in dom0_paging_pages assume 1 page is needed
for each 1MB of guest memory for the p2m, do you think this is OK?
(and suitable to be used for HAP/IOMMU page tables also)

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel