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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.08.2021 15:14, Jan Beulich wrote:
> 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.

I should have added: And as long as Dom0's E820 map gets cloned from
the host's, which will necessarily consider the 1Mb boundary special.

Jan