Re: [PATCH v2 02/14] x86/mm: introduce populate_perdomain_mapping()

[George Dunlap <dunlapg@xxxxxxxxx>]

On Thu, Sep 3, 2026 at 4:57 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> > +    if ( likely(this_cpu(pgtable_vcpu) == v) )
> > +    {
> > +        unsigned int i;
> > +
> > +        /*
> > +         * Fast path: v's page-tables are loaded on this pCPU, so the L1
> > +         * entries can be reached using the recursive linear mappings.
> > +         */
> > +        pl1e = &__linear_l1_table[l1_linear_offset(va)];
>
> As mentioned elsewhere, I'm concerned of this (or really any) new use of
> the linear page tables. (Which, ftaod, isn't an objection.)

FWIW here we can drop this fast path at any time, and we get exactly
the same result as if we drop it from the patch now.

> > +    l2tab = 
> > map_domain_page_irqoff(l3e_get_mfn(l3tab[l3_table_offset(va)]));
>
> l3tab[] isn't used any further, so I think it wants unmapping right away. No
> need to have undue pressure on the number of active mappings.

Ack

> > +        /*
> > +         * As the fast path -- and the slow path holds IRQs off throughout,
> > +         * so replacing area-owned backing here is never permitted.
> > +         */
>
> With this comment I think ...
>
> > +        if ( unlikely(perdomain_l1e_needs_freeing(*pl1e)) )
> > +        {
> > +            ASSERT_UNREACHABLE();
> > +            free_domheap_page(l1e_get_page(*pl1e));
>
> ... this call should be removed from here (I would have suggested to comment
> it out, but Misra dislikes that iirc). Otherwise it would in principle be
> reachable in release builds.

Ah, right -- sorry, this was safe in v1's "xenheap-walk" approach
which doesn't need to disable interrupts; with the "mapcache-walk"
approach we can't do this any more.

> Maybe instead of ASSERT_UNREACHABLE() it should really be BUG() here.

Hrm, docs/misc/xen-error-handling.txt used to have guidelines about
*which* error handling to use.  Basically, BUG() is an immediate
hypervisor crash DoS in production, and should only be used when
there's no way to continue without doing something worse.  Leaking
memory is a lower-grade DoS, and so would be preferable in production
(though in this case probably with a printk, so there's some hope of
figuring out what's wrong).

In theory we could allow callers who knew they'd take the fast path to
do the free; but then we couldn't just rip out the fast path without
doing more surgery.

So I'd propose:  In both fast and slow paths, replace the free with a
printk (keeping the ASSERT_UNREACHABLE), with a comment explaining why
leaking is preferable to BUG.

 -George