Re: [PATCH v2 04/18] x86/pv: introduce function to populate perdomain area and use it to map Xen GDT

["Roger Pau Monné" <roger.pau@citrix.com>]

On Thu, Jan 09, 2025 at 10:10:20AM +0100, Jan Beulich wrote:
> On 08.01.2025 15:26, 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.
> 
> Well, yes. You now record MFNs instead. And you do so at the expense of about
> 100 lines of new code. I'm afraid I'm lacking justification for this price to
> be paid.

Oh, I should have been more explicit on the commit message probably.
The cover letter kind of covers this, the objective is to remove the
stashing of L1 page-table references in the domain struct.  Currently
the per-vCPU GDT L1 are stored in the domain struct, so PTEs can be
easily manipulated.

When moving the per-domain slot to being per-vCPU this stashing of the
L1 tables will become much more complex, and hence I wanted to get rid
of it.

With the introduction of populate_perdomain_mapping() I'm attempting
to get rid of all those L1 references in the domain struct, by having
a generic function that allows modifying the linea address range that
belongs to the per-domain slot.

See for example how patch 8 gets rid of all the l1_pgentry_t GDT/LDT
references in the domain struct.  And how patch 9 simplifies the
create_perdomain_mapping() interface to be much simpler.  All this is
built upon the addition of the populate_perdomain_mapping() helper and
the dropping of the l1_pgentry_t references in the domain struct.

Hope this helps clarify the intent of the change here.

> > @@ -2219,11 +2219,9 @@ void __init trap_init(void)
> >      init_ler();
> >  
> >      /* Cache {,compat_}gdt_l1e now that physically relocation is done. */
> > -    this_cpu(gdt_l1e) =
> > -        l1e_from_pfn(virt_to_mfn(boot_gdt), __PAGE_HYPERVISOR_RW);
> > +    this_cpu(gdt_mfn) = _mfn(virt_to_mfn(boot_gdt));
> >      if ( IS_ENABLED(CONFIG_PV32) )
> > -        this_cpu(compat_gdt_l1e) =
> > -            l1e_from_pfn(virt_to_mfn(boot_compat_gdt), __PAGE_HYPERVISOR_RW);
> > +        this_cpu(compat_gdt_mfn) = _mfn(virt_to_mfn(boot_compat_gdt));
> 
> The comment's going stale this way.

Right, the cache is still there but using a different field name.  I
can adjust to:

/* Cache {,compat_}gdt_mfn now that physically relocation is done. */

Thanks, Roger.