|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 04/18] x86/pv: introduce function to populate perdomain area and use it to map Xen GDT
On Fri Jan 10, 2025 at 2:29 PM GMT, Roger Pau Monné wrote:
> On Thu, Jan 09, 2025 at 09:55:44AM +0000, Alejandro Vallejo wrote:
> > On Wed Jan 8, 2025 at 2:26 PM GMT, Roger Pau Monne wrote:
> > > The current code to update the Xen part of the GDT when running a PV guest
> > > relies on caching the direct map address of all the L1 tables used to map
> > > the
> > > GDT and LDT, so that entries can be modified.
> > >
> > > Introduce a new function that populates the per-domain region, either
> > > using the
> > > recursive linear mappings when the target vCPU is the current one, or by
> > > directly modifying the L1 table of the per-domain region.
> > >
> > > Using such function to populate per-domain addresses drops the need to
> > > keep a
> > > reference to per-domain L1 tables previously used to change the per-domain
> > > mappings.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > ---
> > > xen/arch/x86/domain.c | 11 +++-
> > > xen/arch/x86/include/asm/desc.h | 6 +-
> > > xen/arch/x86/include/asm/mm.h | 2 +
> > > xen/arch/x86/include/asm/processor.h | 5 ++
> > > xen/arch/x86/mm.c | 88 ++++++++++++++++++++++++++++
> > > xen/arch/x86/smpboot.c | 6 +-
> > > xen/arch/x86/traps.c | 10 ++--
> > > 7 files changed, 113 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > > index 1f680bf176ee..0bd0ef7e40f4 100644
> > > --- a/xen/arch/x86/domain.c
> > > +++ b/xen/arch/x86/domain.c
> > > @@ -1953,9 +1953,14 @@ static always_inline bool need_full_gdt(const
> > > struct domain *d)
> > >
> > > static void update_xen_slot_in_full_gdt(const struct vcpu *v, unsigned
> > > int cpu)
> > > {
> > > - l1e_write(pv_gdt_ptes(v) + FIRST_RESERVED_GDT_PAGE,
> > > - !is_pv_32bit_vcpu(v) ? per_cpu(gdt_l1e, cpu)
> > > - : per_cpu(compat_gdt_l1e, cpu));
> > > + ASSERT(v != current);
> >
> > For this assert, and others below. IIUC, curr_vcpu == current when we're
> > properly switched. When we're idling current == idle and curr_vcpu ==
> > prev_ctx.
> >
> > Granted, calling this in the middle of a lazy idle loop would be weird, but
> > would it make sense for PT consistency to use curr_vcpu here...
>
> Hm, this function is called in a very specific context, and the assert
> intends to reflect that. TBH I could just drop it, as
> populate_perdomain_mapping() will DTRT also when v == current. The
> expectation for the context is also that current == curr_vcpu.
>
> Note however that if v == current we would need a flush after the
> populate_perdomain_mapping() call, since populate_perdomain_mapping()
> doesn't perform any flushing of the modified entries. The main
> purpose of the ASSERT() is to notice this.
>
> > > +
> > > + populate_perdomain_mapping(v,
> > > + GDT_VIRT_START(v) +
> > > + (FIRST_RESERVED_GDT_PAGE << PAGE_SHIFT),
> > > + !is_pv_32bit_vcpu(v) ? &per_cpu(gdt_mfn,
> > > cpu)
> > > + :
> > > &per_cpu(compat_gdt_mfn,
> > > + cpu), 1);
> > > }
> > >
> > > static void load_full_gdt(const struct vcpu *v, unsigned int cpu)
> > > diff --git a/xen/arch/x86/include/asm/desc.h
> > > b/xen/arch/x86/include/asm/desc.h
> > > index a1e0807d97ed..33981bfca588 100644
> > > --- a/xen/arch/x86/include/asm/desc.h
> > > +++ b/xen/arch/x86/include/asm/desc.h
> > > @@ -44,6 +44,8 @@
> > >
> > > #ifndef __ASSEMBLY__
> > >
> > > +#include <xen/mm-frame.h>
> > > +
> > > #define GUEST_KERNEL_RPL(d) (is_pv_32bit_domain(d) ? 1 : 3)
> > >
> > > /* Fix up the RPL of a guest segment selector. */
> > > @@ -212,10 +214,10 @@ struct __packed desc_ptr {
> > >
> > > extern seg_desc_t boot_gdt[];
> > > DECLARE_PER_CPU(seg_desc_t *, gdt);
> > > -DECLARE_PER_CPU(l1_pgentry_t, gdt_l1e);
> > > +DECLARE_PER_CPU(mfn_t, gdt_mfn);
> > > extern seg_desc_t boot_compat_gdt[];
> > > DECLARE_PER_CPU(seg_desc_t *, compat_gdt);
> > > -DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);
> > > +DECLARE_PER_CPU(mfn_t, compat_gdt_mfn);
> > > DECLARE_PER_CPU(bool, full_gdt_loaded);
> > >
> > > static inline void lgdt(const struct desc_ptr *gdtr)
> > > diff --git a/xen/arch/x86/include/asm/mm.h b/xen/arch/x86/include/asm/mm.h
> > > index 6c7e66ee21ab..b50a51327b2b 100644
> > > --- a/xen/arch/x86/include/asm/mm.h
> > > +++ b/xen/arch/x86/include/asm/mm.h
> > > @@ -603,6 +603,8 @@ int compat_arch_memory_op(unsigned long cmd,
> > > XEN_GUEST_HANDLE_PARAM(void) arg);
> > > int create_perdomain_mapping(struct domain *d, unsigned long va,
> > > unsigned int nr, l1_pgentry_t **pl1tab,
> > > struct page_info **ppg);
> > > +void populate_perdomain_mapping(const struct vcpu *v, unsigned long va,
> > > + mfn_t *mfn, unsigned long nr);
> > > void destroy_perdomain_mapping(struct domain *d, unsigned long va,
> > > unsigned int nr);
> > > void free_perdomain_mappings(struct domain *d);
> > > diff --git a/xen/arch/x86/include/asm/processor.h
> > > b/xen/arch/x86/include/asm/processor.h
> > > index d247ef8dd226..82ee89f736c2 100644
> > > --- a/xen/arch/x86/include/asm/processor.h
> > > +++ b/xen/arch/x86/include/asm/processor.h
> > > @@ -243,6 +243,11 @@ static inline unsigned long cr3_pa(unsigned long cr3)
> > > return cr3 & X86_CR3_ADDR_MASK;
> > > }
> > >
> > > +static inline mfn_t cr3_mfn(unsigned long cr3)
> > > +{
> > > + return maddr_to_mfn(cr3_pa(cr3));
> > > +}
> > > +
> > > static inline unsigned int cr3_pcid(unsigned long cr3)
> > > {
> > > return IS_ENABLED(CONFIG_PV) ? cr3 & X86_CR3_PCID_MASK : 0;
> > > diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> > > index 3d5dd22b6c36..0abea792486c 100644
> > > --- a/xen/arch/x86/mm.c
> > > +++ b/xen/arch/x86/mm.c
> > > @@ -6423,6 +6423,94 @@ int create_perdomain_mapping(struct domain *d,
> > > unsigned long va,
> > > return rc;
> > > }
> > >
> > > +void populate_perdomain_mapping(const struct vcpu *v, unsigned long va,
> > > + mfn_t *mfn, unsigned long nr)
> > > +{
> > > + l1_pgentry_t *l1tab = NULL, *pl1e;
> > > + const l3_pgentry_t *l3tab;
> > > + const l2_pgentry_t *l2tab;
> > > + struct domain *d = v->domain;
> > > +
> > > + ASSERT(va >= PERDOMAIN_VIRT_START &&
> > > + va < PERDOMAIN_VIRT_SLOT(PERDOMAIN_SLOTS));
> > > + ASSERT(!nr || !l3_table_offset(va ^ (va + nr * PAGE_SIZE - 1)));
> > > +
> > > + /* Use likely to force the optimization for the fast path. */
> > > + if ( likely(v == current) )
> >
> > ... and here? In particular I'd expect using curr_vcpu here means...
>
> I'm afraid not, this is a trap I've fallen originally when doing this
> series, as I indeed had v == curr_vcpu here (and no
> sync_local_execstate() call).
>
> However as a result of an interrupt, a call to sync_local_execstate()
> might happen, at which point the previous check of v == curr_vcpu
> becomes stale.
Wow, that's nasty! More than fair enough then. Guess the XSAVE wrappers (and
more generally all vCPU-local memory accessors) will have to take this into
account before poking into the contents of the perdomain region.
>
> > > + {
> > > + unsigned int i;
> > > +
> > > + /* Ensure page-tables are from current (if current !=
> > > curr_vcpu). */
> > > + sync_local_execstate();
> >
> > ... this should not be needed.
>
> As kind of mentioned above, this is required to ensure the page-tables
> are in-sync with the vCPU in current, and cannot change as a result of
> an interrupt triggering a call to sync_local_execstate().
>
> Otherwise the page-tables could change while or after the call to
> populate_perdomain_mapping(), and the mappings could end up being
> created on the wrong page-tables.
>
> Thanks, Roger.
Cheers,
Alejandro
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |