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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Oct 16, 2023 at 04:55:30PM +0200, Jan Beulich wrote:
> 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?

Given the point we are regarding the release I guess it's safer to
leave the mapping of the holes in the low 1MB as-is, and consider
removing it for 4.19?  That would give us a full release cycle to
check whether it causes issues on systems.

I will send the updated patch.

Thanks, Roger.