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

[Jan Beulich <jbeulich@suse.com>]

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.

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

As long as we have this special treatment of the low Mb, the boundary
is meaningful imo. I'd see the comment go away when the handling in
general gets streamlined.

> Preferably with this removed, Acked-by: Andrew Cooper
> <andrew.cooper3@citrix.com>, but only because this is an emergency fix.

Thanks. I see. You'll like patch 2 even less; at least I do. And I'm
not really certain that change is enough to cover all possible
systems.

> I really don't think it is an improvement to the logic.

Yet I suppose you also have no immediate suggestions towards doing
better? Of course right here a full rework is out of scope. But if
there were smaller bits that - if adjusted - would make you feel
better about the change as a whole, I'd be happy to consider making
adjustments.

Jan