Re: [PATCH] x86/pv: limit GDT and LDT mappings areas to max number of vCPUs

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/11/2024 11:12 am, Roger Pau Monne wrote:
> The allocation of the paging structures in the per-domain area for mapping the
> guest GDT and LDT can be limited to the maximum number of vCPUs the guest can
> have.  The maximum number of vCPUs is available at domain creation since 
> commit
> 4737fa52ce86.
>
> Limiting to the actual number of vCPUs avoids wasting memory for paging
> structures that will never be used.  Current logic unconditionally uses 513
> pages, one page for the L3, plus 512 L1 pages.  For guests with equal or less
> than 16 vCPUs only 2 pages are used (each guest vCPU GDT/LDT can only consume
> 32 L1 slots).
>
> No functional change intended, all possible domain vCPUs should have the GDT
> and LDT paging structures allocated and setup at domain creation, just like
> before the change.
>
> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Ooh, that's a optimisation I'd not considered when doing the work.

But, is it really only the the LDT/GDT area which can benefit from
this?  The XLAT area seems like another good candidate.

> ---
>  xen/arch/x86/pv/domain.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> index d5a8564c1cbe..e861e3ce71d9 100644
> --- a/xen/arch/x86/pv/domain.c
> +++ b/xen/arch/x86/pv/domain.c
> @@ -346,7 +346,7 @@ void pv_domain_destroy(struct domain *d)
>      pv_l1tf_domain_destroy(d);
>  
>      destroy_perdomain_mapping(d, GDT_LDT_VIRT_START,
> -                              GDT_LDT_MBYTES << (20 - PAGE_SHIFT));
> +                              d->max_vcpus << GDT_LDT_VCPU_SHIFT);

Probably not for this patch, but, should we really be passing in a size
here?

Don't we just want to tear down everything in the relevant slot?

>  
>      XFREE(d->arch.pv.cpuidmasks);
>  
> @@ -377,7 +377,7 @@ int pv_domain_initialise(struct domain *d)
>          goto fail;
>  
>      rc = create_perdomain_mapping(d, GDT_LDT_VIRT_START,
> -                                  GDT_LDT_MBYTES << (20 - PAGE_SHIFT),
> +                                  d->max_vcpus << GDT_LDT_VCPU_SHIFT,
>                                    NULL, NULL);

I'd suggest putting a note here saying that the maximum bound for PV
vCPUs is governed by GDT_LDT_MBYTES.

Or alternatively, we could have create_perdomain_mapping() fail if it
tries to allocate more than one slot's worth of mappings?   It feels
like an acceptable safety net.

~Andrew