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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> stashed pointer in d->arch.pv.gdt_ldt_l1tab to update the incoming
> vCPU's page tables with Xen's GDT; this was previously necessary
> 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 if they weren't called from within a context switch.
> One example is pv_{set,destroy}_gdt().
> 
> The previous patch switched the main user of the stashed reference to
> use populate_perdomain_mapping() instead.  Continue that process by
> switching both pv_{set,destroy}_gdt() to it as well.
> 
> pv_destroy_gdt() currently loops over the L1 entries directly,
> extracting the MFN from each, dropping the type and reference unless
> it was the zero page, and replacing the entry with a read-only mapping
> of the zero page.  Rather than reading from the stashed L1, drop the
> references using v->arch.pv.gdt_frames[] instead, and install the
> zero-page mappings with a single populate_perdomain_mapping() call.
> This makes gdt_frames[] consistently the source of truth for MFNs.
> 
> Note that we must maintain the invariant introduced in cf6d39f819
> ("x86/PV: properly populate descriptor tables"): pv_destroy_gdt() maps
> the zero page read-only in torn-down slots rather than unmapping them,
> so that LAR/LSL/VERR/VERW on a selector beyond the guest's limit clear
> ZF as on native rather than taking a #PF-converted #GP.  (And since
> pv_set_gdt() tears down the old GDT before installing the new one,
> guests never see unmapped entries, only zero-page entries.)
> 
> In the case of pv_set_gdt(), we have a slightly awkward situation with
> types.  The ABI with the guest uses unsigned long[], but
> populate_perdomain_mapping() wants an array of mfn_t.
> v->arch.pv.gdt_frames being unsigned long means we can just copy from
> it across the guest ABI with no conversions.  We could in theory
> convert it to mfn_t[] instead, and then pass v->arch.pv.gdt_frames
> into populate_perdomain_mapping(); but then we'd need to add a
> conversion on all the places where frames are copied out.  We choose
> instead to copy frames into a temporary mfn_t array on the stack to
> pass into populate_perdomain_mapping().
> 
> 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>

> --- 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.

Jan