[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/pv: limit GDT and LDT mappings areas to max number of vCPUs
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |