Re: [PATCH v3 17/18] x86/mm: run guest contexts on the sparse directmap view

[George Dunlap <gwd@xxxxxxxxxxxxxx>]

On Fri, Oct 9, 2026 at 8:58 AM Roger Pau Monné <roger@xxxxxxxxxxxxxx> wrote:
> On Wed, Oct 07, 2026 at 11:40:50AM +0100, George Dunlap wrote:
> > In init_xen_l4_slots(), if d->arch.sparse_dmap is set, point the
> > directmap L3 at the sparse view.
> >
> > d->arch.sparse_dmap is decided at domain creation: PV domains all have a
> > mapcache, HVM domains need per-vCPU page-tables for one.  No option asks
> > for the sparse view yet.
>
> Is it strictly needed to tie the sparse directmap to having a per-vCPU
> mapcache on HVM?  AFAICT HVM domains could also use the classic
> per-domain mapcache.

They could, if it were implemented.  I haven't implemented it, and
didn't plan to; so until it's implemented, the sparse view needs
vcpu_pt on HVM domains.

> > The sparse view becomes active once dom0 is built, before it first runs:
> > dom0's construction runs on the full directmap, which it needs, and its
> > root page-tables so far (a PV dom0's first L4s, a PVH dom0's monitor
> > tables) are switched over then.  A PV dom0 booted with dom0=shadow is
> > switched as well: shadow mode is enabled only later, from a tasklet, and
> > its shadow L4s then get the sparse view from init_xen_l4_slots(), as for
> > any PV domain entering shadow mode.  Anything built later gets the
> > sparse view from init_xen_l4_slots().  The sparse view's slots are
> > marked NX at activation, as the full directmap's slots are.
>
> Is NX worth mentioning here?  The expectation is that the sparse dmap
> slots will use the same attributes as the full dmap, and hence I
> would focus on the differences (if any).

Sure, we can drop that.

> > @@ -1743,6 +1744,10 @@ void init_xen_l4_slots(l4_pgentry_t *l4t, mfn_t 
> > l4mfn,
> >                 (ROOT_PAGETABLE_FIRST_XEN_SLOT + slots -
> >                  l4_table_offset(XEN_VIRT_START)) * sizeof(*l4t));
> >      }
> > +
> > +    /* The domain's contexts may run on the sparse view of the directmap. 
> > */
> > +    if ( sparse_dmap_active && d->arch.sparse_dmap )
> > +        sparse_dmap_install(l4t, short_directmap);
>
> Instead of doing the copy of the full directmap from the idle page
> tables, and then rewriting, won't it be simpler if the copied
> directmap was the correct one right away rather than doing a fixup
> afterwards?

Yes; I'll rewrite this.

> > @@ -437,3 +439,130 @@ void __init sparse_dmap_init(void)
> >
> >      printk(XENLOG_INFO "Sparse directmap: %lu pages mapped\n", mapped);
> >  }
> > +
> > +/*
> > + * Point the directmap slots of the root page-table @l4t at the sparse 
> > view.
> > + * Every slot it covers gets its L3, whatever the full directmap has there,
> > + * so that memory added later is covered too.  A PV root has guest slots
> > + * above; any other root loses the rest of the directmap, which the sparse
> > + * view does not cover.
> > + *
> > + * Directmap mappings may be global, in either hierarchy.  Translations of
> > + * the full directmap must not remain usable on a root carrying the sparse
> > + * view: every CR3 write which can move a CPU from the one to the other 
> > goes
> > + * through switch_cr3_cr4(), which flushes what the old root left in the 
> > TLB,
> > + * global entries included, before it returns.  The other CR3 writes, which
> > + * may keep global or PCID-tagged entries (_toggle_guest_pt(), XPTI's entry
> > + * and exit paths), switch between one vCPU's roots, which carry the same
> > + * directmap, or to XPTI's per-CPU root, which maps only this CPU's stack,
> > + * IDT and TSS there.  Removals from the sparse view flush all CPUs, global
> > + * entries included.
>
> IMO parts of this comment are likely out of scope here.  It's fine to
> discuss about the flushing strategy in the commit message, however
> doing so in a comment here, that's disconnected from the definitions
> of switch_cr3_cr4() and _toggle_guest_pt() seems out of place.
>
> I would also avoid referencing function names in code comments: it's
> fine to do so in the commit message, but it's likely those references
> in comments will go stale as functions change names or disappear.
> Commit messages are otherwise tied to a specific point in time, so the
> context and the referenced functions are always accurate and it will
> never go stale in the context is was committed.

Yes, I've done a general pass to trim down comments.

> > + */
> > +void sparse_dmap_install(l4_pgentry_t *l4t, bool pv)
> > +{
> > +    unsigned int i;
> > +
> > +    ASSERT(sparse_dmap_root);
> > +
> > +    for ( i = l4_table_offset(SPARSE_DMAP_START);
> > +          i < l4_table_offset(SPARSE_DMAP_END); i++ )
> > +        l4t[i] = sparse_dmap_root[i];
> > +
> > +    if ( !pv )
> > +        for ( ; i < l4_table_offset(DIRECTMAP_VIRT_END); i++ )
> > +            l4t[i] = l4e_empty();
> > +}
> > +
> > +static void __init sparse_dmap_install_mfn(mfn_t mfn, bool pv)
> > +{
> > +    l4_pgentry_t *l4t = map_domain_page(mfn);
> > +
> > +    sparse_dmap_install(l4t, pv);
> > +    unmap_domain_page(l4t);
> > +}
> > +
> > +/*
> > + * Let guest contexts run on the sparse view.  Root page-tables built from
> > + * here on carry it (init_xen_l4_slots()); those of the domains built so 
> > far
> > + * (dom0, which has not run yet) are switched over here.  Its construction 
> > ran
> > + * on the full directmap, and needed to.
> > + */
> > +void __init sparse_dmap_activate(void)
> > +{
> > +    struct domain *d;
> > +    struct vcpu *v;
> > +    unsigned long va, mapped = 0;
> > +    unsigned int i, switched = 0;
> > +
> > +    if ( !sparse_dmap_root )
> > +        return;
> > +
> > +    /*
> > +     * Nothing in the directmap is executed: as subarch_init_memory() does
> > +     * for the full directmap, mark the sparse view's slots NX, before any
> > +     * root page-table takes a copy of them.
> > +     */
> > +    if ( cpu_has_nx )
> > +        for ( i = l4_table_offset(SPARSE_DMAP_START);
> > +              i < l4_table_offset(SPARSE_DMAP_END); i++ )
> > +            l4e_add_flags(sparse_dmap_root[i], _PAGE_NX_BIT);
>
> Shouldn't this be done when the sparse directmap L4 are populated,
> instead of here?

In the meantime, this function has been removed; NX bit is now set in
sparse_dmap_init(), and the rest of the code you comment in this
function has been removed.

 -George