Re: [PATCH v2 07/18] x86/pv: update guest LDT mappings using the linear entries

["Alejandro Vallejo" <alejandro.vallejo@xxxxxxxxx>]

On Fri Jan 10, 2025 at 2:44 PM GMT, Roger Pau Monné wrote:
> On Thu, Jan 09, 2025 at 02:34:05PM +0000, Alejandro Vallejo wrote:
> > On Wed Jan 8, 2025 at 2:26 PM GMT, Roger Pau Monne wrote:
> > > The pv_map_ldt_shadow_page() and pv_destroy_ldt() functions rely on the L1
> > > table(s) that contain such mappings being stashed in the domain 
> > > structure, and
> > > thus such mappings being modified by merely updating the require L1 
> > > entries.
> > >
> > > Switch pv_map_ldt_shadow_page() to unconditionally use the linear 
> > > recursive, as
> > > that logic is always called while the vCPU is running on the current pCPU.
> > >
> > > For pv_destroy_ldt() use the linear mappings if the vCPU is the one 
> > > currently
> > > running on the pCPU, otherwise use destroy_mappings().
> > >
> > > Note this requires keeping an array with the pages currently mapped at 
> > > the LDT
> > > area, as that allows dropping the extra taken page reference when 
> > > removing the
> > > mappings.
> > >
> > > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > > ---
> > >  xen/arch/x86/include/asm/domain.h   |  2 ++
> > >  xen/arch/x86/pv/descriptor-tables.c | 19 ++++++++++---------
> > >  xen/arch/x86/pv/domain.c            |  4 ++++
> > >  xen/arch/x86/pv/mm.c                |  3 ++-
> > >  4 files changed, 18 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/xen/arch/x86/include/asm/domain.h 
> > > b/xen/arch/x86/include/asm/domain.h
> > > index b79d6badd71c..b659cffc7f81 100644
> > > --- a/xen/arch/x86/include/asm/domain.h
> > > +++ b/xen/arch/x86/include/asm/domain.h
> > > @@ -523,6 +523,8 @@ struct pv_vcpu
> > >      struct trap_info *trap_ctxt;
> > >  
> > >      unsigned long gdt_frames[FIRST_RESERVED_GDT_PAGE];
> > > +    /* Max LDT entries is 8192, so 8192 * 8 = 64KiB (16 pages). */
> > > +    mfn_t ldt_frames[16];
> > >      unsigned long ldt_base;
> > >      unsigned int gdt_ents, ldt_ents;
> > >  
> > > diff --git a/xen/arch/x86/pv/descriptor-tables.c 
> > > b/xen/arch/x86/pv/descriptor-tables.c
> > > index 5a79f022ce13..95b598a4c0cf 100644
> > > --- a/xen/arch/x86/pv/descriptor-tables.c
> > > +++ b/xen/arch/x86/pv/descriptor-tables.c
> > > @@ -20,28 +20,29 @@
> > >   */
> > >  bool pv_destroy_ldt(struct vcpu *v)
> > >  {
> > > -    l1_pgentry_t *pl1e;
> > > +    const unsigned int nr_frames = ARRAY_SIZE(v->arch.pv.ldt_frames);
> > >      unsigned int i, mappings_dropped = 0;
> > > -    struct page_info *page;
> > >  
> > >      ASSERT(!in_irq());
> > >  
> > >      ASSERT(v == current || !vcpu_cpu_dirty(v));
> > >  
> > > -    pl1e = pv_ldt_ptes(v);
> > > +    destroy_perdomain_mapping(v, LDT_VIRT_START(v), nr_frames);
> > >  
> > > -    for ( i = 0; i < 16; i++ )
> > > +    for ( i = 0; i < nr_frames; i++ )
> > 
> > nit: While at this, can the "unsigned int" be moved here too?
>
> I don't mind much, but I also don't usually do such changes as I think
> it adds more noise.

Fair enough, nvm then.

>
> > >      {
> > > -        if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
> > > -            continue;
> > > +        mfn_t mfn = v->arch.pv.ldt_frames[i];
> > > +        struct page_info *page;
> > >  
> > > -        page = l1e_get_page(pl1e[i]);
> > > -        l1e_write(&pl1e[i], l1e_empty());
> > > -        mappings_dropped++;
> > > +        if ( mfn_eq(mfn, INVALID_MFN) )
> > > +            continue;
> > 
> > Can it really be disjoint? As in, why "continue" and not "break"?. Not that 
> > it
> > matters in the slightest, and I prefer this form; but I'm curious.
>
> I think so?  The PV guest LDT is populated as a result of page-faults,
> so if the guest only happens to use segment descriptors that are on
> the third page, the second page might not be mapped?
>
> The continue was there already, and I really didn't dare to change
> this, neither asked myself much.  Assumed due to how the guest LDT is
> mapped on a page-fault basis it could indeed be disjointly mapped.

Ah, I see. That makes sense then. I wouldn't suggest changing it either, I
was just curious :)

>
> > >  
> > > +        v->arch.pv.ldt_frames[i] = INVALID_MFN;
> > > +        page = mfn_to_page(mfn);
> > >          ASSERT_PAGE_IS_TYPE(page, PGT_seg_desc_page);
> > >          ASSERT_PAGE_IS_DOMAIN(page, v->domain);
> > >          put_page_and_type(page);
> > > +        mappings_dropped++;
> > >      }
> > >  
> > >      return mappings_dropped;
> > > diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
> > > index 7e8bffaae9a0..32d7488cc186 100644
> > > --- a/xen/arch/x86/pv/domain.c
> > > +++ b/xen/arch/x86/pv/domain.c
> > > @@ -303,6 +303,7 @@ void pv_vcpu_destroy(struct vcpu *v)
> > >  int pv_vcpu_initialise(struct vcpu *v)
> > >  {
> > >      struct domain *d = v->domain;
> > > +    unsigned int i;
> > >      int rc;
> > >  
> > >      ASSERT(!is_idle_domain(d));
> > > @@ -311,6 +312,9 @@ int pv_vcpu_initialise(struct vcpu *v)
> > >      if ( rc )
> > >          return rc;
> > >  
> > > +    for ( i = 0; i < ARRAY_SIZE(v->arch.pv.ldt_frames); i++ )
> > > +        v->arch.pv.ldt_frames[i] = INVALID_MFN;
> > > +
> > 
> > I think it makes more sense to move this earlier so ldt_frames[] is 
> > initialised
> > even if pv_vcpu_initialise() fails. It may be benign, but it looks like an
> > accident abount to happen.
>
> Right, pv_destroy_gdt_ldt_l1tab() doesn't care at all about the
> contents of ldt_frames[], but it will be safe to do change the
> ordering in pv_vcpu_initialise().
>
> Thanks, Roger.

Cheers,
Alejandro