Re: XSA-378 fixes breaking PVH Dom0 (was: [xen-4.15-testing test] 164495: regressions - FAIL)

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.08.2021 16:46, Jan Beulich wrote:
> On 27.08.2021 15:29, Jan Beulich wrote:
>> On 27.08.2021 08:52, osstest service owner wrote:
>>> flight 164495 xen-4.15-testing real [real]
>>> flight 164509 xen-4.15-testing real-retest [real]
>>> http://logs.test-lab.xenproject.org/osstest/logs/164495/
>>> http://logs.test-lab.xenproject.org/osstest/logs/164509/
>>>
>>> Regressions :-(
>>>
>>> Tests which did not succeed and are blocking,
>>> including tests which could not be run:
>>>  test-amd64-amd64-dom0pvh-xl-amd  8 xen-boot              fail REGR. vs. 
>>> 163759
>>>  test-amd64-amd64-dom0pvh-xl-intel  8 xen-boot            fail REGR. vs. 
>>> 163759
>>
>> This is fallout from XSA-378. During Dom0 setup we first call
>> iommu_hwdom_init(), which maps reserved regions (p2m_access_rw).
>> Later map_mmio_regions() gets used to identity-map the low first
>> Mb. This, using set_mmio_p2m_entry(), establishes default-access
>> mappings (p2m_access_rwx).
>>
>> Hence even if relaxing the logic in set_typed_p2m_entry() to
>>
>>     if ( p2m_is_special(ot) )
>>     {
>>         gfn_unlock(p2m, gfn, order);
>>         if ( mfn_eq(mfn, omfn) && gfn_p2mt == ot && access == a )
>>             return 0;
>>         domain_crash(d);
>>         return -EPERM;
>>     }
>>
>> we're still in trouble (because the two access types don't match)
>> when there is any reserved region below 1Mb.
>>
>> One approach would be to avoid blindly mapping the low first Mb,
>> and to instead honor mappings which are already there. Or the
>> opposite - avoid mapping anything from arch_iommu_hwdom_init()
>> which is below 1Mb. (Other mappings down the call tree from
>> pvh_setup_acpi() imo would then also need adjusting, to avoid
>> redundant mapping attempts of space below 1Mb. At least RSDP is
>> known to possibly live there on various systems.)
>>
>> Another approach could be to stop passing ->default_access from
>> set_mmio_p2m_entry() to set_typed_p2m_entry(). (And I think the
>> same should go for set_foreign_p2m_entry()). At the very least
>> right now it makes no sense at all to make RWX mappings there,
>> except when mapping PCI device ROMs. But of course reducing
>> permissions always comes with a (however large or small) risk of
>> regressions.
>>
>> While I think the latter aspect wants improving in any event,
>> right now I'm leaning towards the "opposite" variant of the
>> former. I'll draft a patch along these lines at least to see if
>> it helps, or if there is yet more fallout.
> 
> There is more fallout - with the initial issue addressed as
> described I'm now hitting another similar domain_crash() in
> guest_physmap_add_entry(). There's no question there whether to
> check that old and new mappings match - they are different. Here
> PVH Dom0 setup really does what the final XSA-378 patch is
> intended to disallow: It produces MMIO mappings to then replace
> some (or really most) by RAM ones. This means I'll have to
> further adjust how pvh_populate_p2m() works.

And with this taken care of I'm hitting the original assertion
again, now in the course of mapping ACPI memory. This time the
collision is between RMRRs getting identity mapped first, and on
this box an RMRR region being enclosed by an ACPI NVS region of
the E820 map (and hence the _almost_ same mapping getting
requested a 2nd time). As a first step I'll further relax the
adjustment to set_typed_p2m_entry() still in context above. But
I'd be really thankful for thoughts by anyone else on how to
deal with all of this mess.

Jan