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 <jbeul...@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.
