Re: [PATCH for-4.18 v2] x86/pvh: fix identity mapping of low 1MB

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.10.2023 16:51, Roger Pau Monné wrote:
> On Mon, Oct 16, 2023 at 04:07:22PM +0200, Jan Beulich wrote:
>> On 16.10.2023 15:51, Roger Pau Monné wrote:
>>> On Mon, Oct 16, 2023 at 03:32:54PM +0200, Jan Beulich wrote:
>>>> On 13.10.2023 10:56, Roger Pau Monne wrote:
>>>>> The mapping of memory regions below the 1MB mark was all done by the PVH 
>>>>> dom0
>>>>> builder code, causing the region to be avoided by the arch specific IOMMU
>>>>> hardware domain initialization code.  That lead to the IOMMU being enabled
>>>>> without reserved regions in the low 1MB identity mapped in the p2m for PVH
>>>>> hardware domains.  Firmware which happens to be missing RMRR/IVMD ranges
>>>>> describing E820 reserved regions in the low 1MB would transiently trigger 
>>>>> IOMMU
>>>>> faults until the p2m is populated by the PVH dom0 builder:
>>>>>
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:13.1 d0 addr 00000000000eb380 flags 0x20 RW
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:13.1 d0 addr 00000000000eb340 flags 0
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:13.2 d0 addr 00000000000ea1c0 flags 0
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:14.5 d0 addr 00000000000eb480 flags 0x20 RW
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:12.0 d0 addr 00000000000eb080 flags 0x20 RW
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:14.5 d0 addr 00000000000eb400 flags 0
>>>>> AMD-Vi: IO_PAGE_FAULT: 0000:00:12.0 d0 addr 00000000000eb040 flags 0
>>>>>
>>>>> Those errors have been observed on the osstest pinot{0,1} boxes (AMD 
>>>>> Fam15h
>>>>> Opteron(tm) Processor 3350 HE).
>>>>>
>>>>> Mostly remove the special handling of the low 1MB done by the PVH dom0 
>>>>> builder,
>>>>> leaving just the data copy between RAM regions.  Otherwise rely on the 
>>>>> IOMMU
>>>>> arch init code to create any identity mappings for reserved regions in 
>>>>> that
>>>>> range (like it already does for reserved regions elsewhere).
>>>>>
>>>>> Note there's a small difference in behavior, as holes in the low 1MB will 
>>>>> no
>>>>> longer be identity mapped to the p2m.
>>>>
>>>> I certainly like the simplification, but I'm concerned by this: The BDA
>>>> is not normally reserved, yet may want accessing by Dom0 (to see the real
>>>> machine contents). We do access that first page of memory ourselves, so
>>>> I expect OSes may do so as well (even if the specific aspect I'm thinking
>>>> of - the warm/cold reboot field - is under Xen's control).
>>>
>>> The BDA on the systems I've checked falls into a RAM area on the
>>> memory map, but if you think it can be problematic I could arrange for
>>> arch_iommu_hwdom_init() to also identity map holes in the low 1MB.
>>
>> Hmm, this again is a case where I'd wish CPU and IOMMU mappings could
>> be different. I don't see reasons to try I/O to such holes, but I can
>> see reasons for CPU accesses (of more or less probing kind).
> 
> Hm, while I agree devices have likely no reason to access holes (there
> or elsewhere) I don't see much benefit of having this differentiation,
> it's easier to just map everything for accesses from both device and
> CPU rather than us having to decide (and maybe get wrong) whether
> ranges should only be accessed by the CPU.

I understand that, and I also follow Andrew's arguments towards not
making such a distinction. The consequence though is that we need
to map more than possibly necessary, and never too little.

>>> Keep in mind this is only for PVH, it won't affect PV.
>>
>> Of course.
> 
> Would you be willing to Ack it?

If "it" is the present version, then me doing so would be stretch.
How averse are you to re-adding the hole mappings?

Jan