Re: [PATCH v2 01/14] x86/domain_page: introduce IRQs-off variants of {,un}map_domain_page()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.09.2026 11:43, George Dunlap wrote:
> From: George Dunlap <gwd@xxxxxxxxxxxxxx>
> 
> Currently, map_domain_page() cannot be called in the context switch
> path.  However, Xen already needs to update the slot of an incoming PV
> vcpu's GDT during context switch; and when we soon switch to per-vCPU
> root pagetables, we'll have to modify two more places.
> 
> Xen currently solves the problem by special-casing the GDT/LDT L1
> tables to be allocated from the xenheap, and stashing a pointer to its
> address in the xenheap in the domain struct.  Rather than add more Xen
> pagetable pages to the xenheap, introduce a version of map_domain_page
> which can be called from the context switch path.
> 
> The reason map_domain_page() cannot be called from the context switch
> path is x86's lazy context-switch state.  Mapcache mappings are
> created in the page-tables that are loaded on the pCPU.  When Xen is
> in a lazy context-switch state, current is the idle vCPU while the
> previously-running vCPU's page-tables remain loaded.  If in this
> state, another pcpu wants access to the lazily-swapped-out vcpu's
> state, it will send a FLUSH_VCPU_STATE IPI to the processor, which
> will call sync_local_execstate().
> 
> sync_local_execstate() is implemented internally by calling a full
> __context_switch().  In addition to copying the processor state into
> the vcpu structure, this also switches the loaded pagetables to the
> idle vcpu's, which would in turn cause mappings created before the IPI
> to disappear mid-use.  Therefore, mappings cannot be held in the
> mapcache when a FLUSH_VCPU_STATE IPI may execute.  To this end,
> map_domain_page() calls sync_local_execstate() itself proactively when
> it detects a lazy context-switch state.  This guarantees that the
> pagetables will remain consistent at least until the next context
> switch.
> 
> But of course, that synchronization must not be triggered from the
> context switch path itself: sync_local_execstate() ends up in
> __context_switch(), so a call made while a context switch is in
> progress would recurse, and the assertions along that path (current
> being the idle vCPU) don't hold there either.
> 
> A full synchronization is sufficient to prevent a FLUSH_VCPU_STATE IPI
> from switching the pagetables; however, it is not necessary.  It
> suffices to maintain interrupts disabled from before the page is
> mapped until after it is unmapped.  This condition is satisfied for
> the mappings used on the context switch path.
> 
> Introduce {,un}map_domain_page_irqoff() variants for callers which
> guarantee that interrupts remain disabled from the map until the
> matching unmap.  Under that guarantee the synchronization is
> unnecessary rather than merely inconvenient: no IPI can be delivered
> while the mapping is in use, so the lazy state cannot change under the
> caller's feet, and this_cpu(pgtable_vcpu) accurately identifies the
> mapcache to use (see 622c9a5ba95d "x86/mm: accurately track which vCPU
> page-tables are loaded").  The variants assert that interrupts are
> disabled on entry; the rest of the contract remains the caller's
> responsibility.
> 
> This will be used by the next patch, which introduces a function which
> will be used to modify the incoming vCPU's per-domain mappings from
> within __context_switch(); it will also be used in future ASI
> patches (tearing down and establishing per-CPU stack mappings during
> context switch).
> 
> No functional change for existing callers.
> 
> Assisted-by: Claude Code:claude-fable-5
> Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
with one aspect for further consideration:

> @@ -59,7 +68,7 @@ static inline struct vcpu *mapcache_current_vcpu(void)
>  #define MAPCACHE_L1ENT(idx) \
>      __linear_l1_table[l1_linear_offset(MAPCACHE_VIRT_START + 
> pfn_to_paddr(idx))]
>  
> -void *map_domain_page(mfn_t mfn)
> +static void *do_map_domain_page(mfn_t mfn, bool irqs_off)
>  {

do_...() commonly (but sadly not consistently) mark top-level hypercall
handlers. Personally I'd prefer if the "do" (but not the underscore) were
dropped here and ...

> @@ -165,7 +174,19 @@ void *map_domain_page(mfn_t mfn)
>      return (void *)MAPCACHE_VIRT_START + pfn_to_paddr(idx);
>  }
>  
> -void unmap_domain_page(const void *ptr)
> +void *map_domain_page(mfn_t mfn)
> +{
> +    return do_map_domain_page(mfn, false);
> +}
> +
> +void *map_domain_page_irqoff(mfn_t mfn)
> +{
> +    ASSERT(!local_irq_is_enabled());
> +
> +    return do_map_domain_page(mfn, true);
> +}
> +
> +static void do_unmap_domain_page(const void *ptr, bool irqs_off)

... here. Identifiers with a single leading underscore (and no following
upper-case letter) are designated for use by static functions, after all.

Jan