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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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)

All IOMMU modifications should be as a side effect of regular p2m/guest
physmap operations.  I suppose this is another breakage from the PV side
of things.

> @@ -1095,6 +1101,17 @@ static int __init pvh_setup_acpi(struct
>          nr_pages = PFN_UP((d->arch.e820[i].addr & ~PAGE_MASK) +
>                            d->arch.e820[i].size);
>  
> +        /* Memory below 1MB has been dealt with by pvh_populate_p2m(). */
> +        if ( pfn < PFN_DOWN(MB(1)) )
> +        {
> +            if ( pfn + nr_pages <= PFN_DOWN(MB(1)) )
> +                continue;
> +
> +            /* This shouldn't happen, but is easy to deal with. */

I'm not sure this comment is helpful.

Under PVH, there is nothing special about the 1M boundary, and we can
reasonably have something else here or crossing the boundary.


Preferably with this removed, Acked-by: Andrew Cooper
<andrew.cooper3@xxxxxxxxxx>, but only because this is an emergency fix. 
I really don't think it is an improvement to the logic.

~Andrew