On 02.09.2026 11:43, George Dunlap wrote:
> From: Roger Pau Monné <[email protected]>
>
> 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é <[email protected]>
> Assisted-by: Claude Code:claude-fable-5, Claude Code:claude-opus-4-8
> Signed-off-by: George Dunlap <[email protected]>
With the adjustment above:
Reviewed-by: Jan Beulich <[email protected]>
> --- 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
