Re: [PATCH] x86/PVH: extend checking in hwdom_fixup_p2m()

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jul 07, 2025 at 04:44:25PM +0200, Jan Beulich wrote:
> We're generally striving to minimize behavioral differences between PV
> and PVH Dom0. Using (just?) is_memory_hole() in the PVH case looks quite
> a bit weaker to me, compared to the page ownership check done in the PV
> case. Extend checking accordingly.

The PV code path is also used by non-priviledged domains, while
pf-fixup is strictly limited to the hardware domain.  But I agree that
the page ownership check is possibly better, and more in-line with the
PV counterpart.

> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

LGTM, I would just request that you also remove the is_memory_hole()
check at the same time.

> ---
> The addition may actually be suitable to replace the use of
> is_memory_hole() here. While dropping that would in particular extend
> coverage to E820_RESERVED regions, those are identity-mapped anyway
> (albeit oddly enough still by IOMMU code).

You could avoid getting E820_RESERVED regions identity mapped if
dom0-iommu=map-reserved=0 is specified on the command line, at which
point your suggestion to use the page ownership check seems better
because it would also allow for pf-fixup to deal with E820_RESERVED
regions.

I think it would be better to remove the is_memory_hole() check if we
introduce the page ownership checking.  Otherwise it's kind of
redundant and possibly confusing.

> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -184,6 +184,22 @@ static int hwdom_fixup_p2m(paddr_t addr)
>           !is_memory_hole(_mfn(gfn), _mfn(gfn)) )
>          return -EPERM;
>  
> +    /*
> +     * Much like get_page_from_l1e() for PV Dom0 does, check that the page
> +     * accessed is actually an MMIO one: Either its MFN is out of range, or
> +     * it's owned by DOM_IO.
> +     */
> +    if ( mfn_valid(_mfn(gfn)) )
> +    {
> +        struct page_info *pg = mfn_to_page(_mfn(gfn));

We should consider introducing a mfn_t mfn = _mfn(gfn) local variable,
as there's a non-trivial amount of _mfn(gfn) instances.  Not that you
need to do it here, just noticed the amount of instances we have of
it.

Thanks, Roger.