Re: [PATCH v3 10/18] x86/domain_page: don't unmap a mapcache entry that is not present

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

On Tue, Oct 13, 2026 at 11:33:55AM +0100, George Dunlap wrote:
> On Thu, Oct 8, 2026 at 12:04 PM Roger Pau Monné <roger@xxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Oct 07, 2026 at 11:40:43AM +0100, George Dunlap wrote:
> > > Mappings must be torn down by the vCPU that created them, and only
> > > once.  Have unmap_domain_page() check that the entry it is handed is
> > > present and bail out otherwise (ASSERT_UNREACHABLE()), rather than read
> > > an MFN out of an empty PTE and act on it, as a second unmap of an entry
> > > the first one zapped would.  (A second unmap of an entry the maphash
> > > kept is caught, as before, only by the refcount ASSERT() in debug
> > > builds.)
> > >
> > > Assisted-by: Claude Code:claude-opus-5-5
> > > Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
> >
> > Reviewed-by: Roger Pau Monné <roger@xxxxxxxxxxxxxx>
> >
> > > ---
> > > Changes in v3:
> > > - New in this version, split out of "x86/hvm: introduce per-vCPU
> > >   mapcache for vCPU-PT domains" for review.
> > > ---
> > >  xen/arch/x86/domain_page.c | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/xen/arch/x86/domain_page.c b/xen/arch/x86/domain_page.c
> > > index f91ec8a75a..708df90a00 100644
> > > --- a/xen/arch/x86/domain_page.c
> > > +++ b/xen/arch/x86/domain_page.c
> > > @@ -230,6 +230,16 @@ void unmap_domain_page(const void *ptr)
> > >      ASSERT(cache->inuse);
> > >
> > >      idx = PFN_DOWN(va - MAPCACHE_VIRT_START);
> > > +    /*
> > > +     * Mappings must be unmapped once, by the vCPU that created them.  
> > > With
> > > +     * nothing present there is nothing to tear down: leave the hash and 
> > > the
> > > +     * bitmaps alone rather than act on a stale or foreign pointer.
> >
> > The comment makes more claims that what can be checked for: there's no
> > check that the unmap is being done by the same vCPU that created it,
> > neither is easy to add one.  Any mismatch like that would become more
> > obvious once we switch to a per-vCPU mapcache.  I would possibly
> > reduce the comment to:
> >
> > /* Ensure the entry to unmap is populated. */
> >
> > The commit message already have the lengthily details.
> 
> The problem is that now the comment just repeats the condition of the
> if() statement, which an experienced developer can already read
> without effort; it doesn't say *why* we're doing the check.  What
> about something like the following?
> 
>     /*
>      * Sanity check: an entry that isn't present means a double or stray
>      * unmap.  Play it safe: don't act on the MFN in an empty PTE.
>      */

Oh, I'm actually happy if you want to drop the comment altogether.  I
think the check + ASSERT_UNREACHABLE() is already clear to understand.
And we expect anyone touching this code to know that a double or stray
free of an entry that's not present is not a good thing.

Thanks, Roger.