Re: [PATCH v2 04/14] x86/pv: set/clear guest GDT mappings using populate_perdomain_mapping()

[George Dunlap <dunlapg@xxxxxxxxx>]

On Mon, Sep 7, 2026 at 1:51 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 02.09.2026 11:43, George Dunlap wrote:
> > From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >
> > Until the previous patch, update_xen_slot_in_full_gdt() used the
>
> Please can we avoid "previous patch" (also again below) and alike in
> commit messages?

Something like this then?

8<---
update_xen_slot_in_full_gdt() used to update the incoming vCPU's page
tables with Xen's GDT through the stashed pointer in
d->arch.pv.gdt_ldt_l1tab, because map_domain_page() couldn't be called
in a context switch. Having a handy pointer to an always-mapped
version of the GDT/LDT L1 table, other sites which modify the table
started using it for convenience, even though they aren't called from
within a context switch. One example is pv{set,destroy}gdt().

With update_xen_slot_in_full_gdt() now using
populate_perdomain_mapping() (see "x86/pv: use
populate_perdomain_mapping() to map the Xen GDT"), the stashed
reference has lost the user that justified it. Switch
pv{set,destroy}gdt() to populate_perdomain_mapping() as well; the LDT
paths are the remaining users, after which the stash can go.
--->8

> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > Assisted-by: Claude Code:claude-fable-5, Claude Code:claude-opus-4-8
> > Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
>
> With the adjustment above:
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Thanks!

> > --- a/xen/arch/x86/pv/descriptor-tables.c
> > +++ b/xen/arch/x86/pv/descriptor-tables.c
> > @@ -49,33 +49,42 @@ bool pv_destroy_ldt(struct vcpu *v)
> >
> >  void pv_destroy_gdt(struct vcpu *v)
> >  {
> > -    l1_pgentry_t *pl1e = pv_gdt_ptes(v);
> > -    mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
> > -    l1_pgentry_t zero_l1e = l1e_from_mfn(zero_mfn, __PAGE_HYPERVISOR_RO);
> > +    const mfn_t zero_mfn = _mfn(virt_to_mfn(zero_page));
> > +    mfn_t zero_mfns[ARRAY_SIZE(v->arch.pv.gdt_frames)];
>
> I wonder if having an initializer here might not result in better code. The
> array could likely be filled with REP STOSQ here, and ...
>
> >      unsigned int i;
> >
> >      ASSERT(v == current || !vcpu_cpu_dirty(v));
> >
> >      v->arch.pv.gdt_ents = 0;
> > -    for ( i = 0; i < FIRST_RESERVED_GDT_PAGE; i++ )
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(zero_mfns); i++ )
> >      {
> > -        mfn_t mfn = l1e_get_mfn(pl1e[i]);
> > +        zero_mfns[i] = zero_mfn;
>
> ... the live range of zero_mfn would reduce to about nothing.

Tried it: GCC didn't use REP STOSQ [1], but seems worth it for clarity
and register efficiency anyway.

 -George

[1] Fable experimented with a few different compiler flags, but
couldn't get a REP operation;  it said "The reason is structural:
GCC's string-op machinery handles block clears and byte-splat fills
(memset means a repeated byte); a fill with a non-constant 8-byte
value is not a memset and never reaches that expander".