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

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

On Thu, Nov 21, 2024 at 11:26:19AM +0000, Andrew Cooper wrote:
> 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.

I don't see XLAT being pre-allocated like the GDT/LDT area is, it's
only populated on a per-vCPU basis in setup_compat_arg_xlat() which is
already bounded to the number of initialized vCPUs.

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

Hm, I've considered leaving that alone and keep passing GDT_LDT_MBYTES
to destroy the whole slot.  The performance advantage of not iterating
over the known empty slots is negligible IMO.  No strong opinion, I
can leave as-is if it's considered better.

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

Yeah, MAX_VIRT_CPUS is already calculated based on 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.

I would rather use something like:

if ( (d->max_vcpus << GDT_LDT_VCPU_SHIFT) >
     (PERDOMAIN_SLOT_MBYTES << (20 - PAGE_SHIFT)) )
    ASSERT_UNREACHABLE();
    return -EINVAL;
}

As it should be impossible to call pv_domain_initialise() with a
number of vCPUs past what fits in the GDT/LDT slot.
arch_sanitise_domain_config() should have filtered such attempt before
it gets into pv_domain_initialise().

Thanks, Roger.