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

[Roger Pau Monné <roger@xxxxxxxxxxxxxx>]

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.

Thanks, Roger.