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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.09.2026 15:51, George Dunlap wrote:
> 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

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

Interesting - room for improvement there then.

Jan

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