Re: [PATCH 1/4] x86/PVH: de-duplicate mappings for first Mb of Dom0 memory

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Aug 31, 2021 at 03:14:06PM +0200, Jan Beulich wrote:

Sorry for the delay, and likely not being of much help right now.

> On 31.08.2021 15:02, Andrew Cooper wrote:
> > On 30/08/2021 14:02, Jan Beulich wrote:
> >> One of the changes comprising the fixes for XSA-378 disallows replacing
> >> MMIO mappings by unintended (for this purpose) code paths.
> > 
> > I'd drop the brackets.  All it does is confuse the sentence.
> > 
> >>  This means we
> >> need to be more careful about the mappings put in place in this range -
> >> mappings should be created exactly once:
> >> - iommu_hwdom_init() comes first; it should avoid the first Mb,
> >> - pvh_populate_p2m() should insert identity mappings only into ranges
> >>   not populated as RAM,
> >> - pvh_setup_acpi() should again avoid the first Mb, which was already
> >>   dealt with at that point.
> > 
> > This really is a mess.  It also seems very fragile.
> 
> So it seems to me.
> 
> > Why is iommu_hwdom_init() separate in the first place?  It only does
> > mappings up to 4G in the first place, and with this change, it is now
> > [1M-4G)
> 
> I guess we'll want to wait for Roger to return to shed some light on
> this.

iommu_hwdom_init should cover from [0, max_pdx], or 4G if max_pdx is
below that.

IIRC first PVH dom0 implementations used to have a behavior more
similar to PV in iommu_hwdom_init, as they would get almost everything
below 4GB that's not RAM identity mapped in the (IOMMU) page tables.

PVH dom0 has since diverged, and now iommu_hwdom_init just identity
maps reserved regions. We could likely move this somewhere else, but
given it's still shared with PV dom0 (by using the same command line
option, dom0-iommu=map-reserved) I think it would just make option
handling more confusing.

One way to simplify things would be to rely on the hardware provided
memory map to correctly have the regions in the low 1M marked as
reserved, so that iommu_hwdom_init would identity map them. Then we
would just need a bit of special handling to duplicate the data in RAM
regions for the low 1M but we could avoid a lot of complexity.  This
however requires trusting the hardware is not missing required regions
in the low 1M.

Another option might be to slightly modify hwdom_iommu_map so that for
PVH it returns true for all non-RAM regions in the low 1M. That would
avoid having to add another loop in pvh_populate_p2m to map those.

Thanks, Roger.