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

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Thu, Sep 3, 2026 at 3:07 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Thanks -- I'll make that change for v3 if nobody objects.

 -George