Re: [PATCH] x86/pvh: fix identity mapping of low 1MB

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

On Thu, Oct 12, 2023 at 12:48:20AM +0800, Andrew Cooper wrote:
> On 11/10/2023 11:37 pm, Roger Pau Monne wrote:
> > The mapping of memory regions below the 1MB mark was all done by the PVH 
> > dom0
> > builder code, thus completely avoiding that region in the arch-specific 
> > IOMMU
> > hardware domain initialization code.
> 
> This took a while to parse.  I think it would be clearer to say "builder
> code, causing the region to be avoided by the arch ..."

Yes, that's likely better, so:

"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 with missing RMRR/IVMD ranges that would 
> > otherwise
> > be located in the low 1MB would transiently trigger IOMMU faults until the 
> > p2m
> > is populated by the PVH dom0 builder:
> 
> "Firmware which happens to be missing RMRR/IVMD ranges describing E820
> reserved regions in the low 1MB would ..." ?

Will adjust.

> > 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
> >
> > 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 such
> > range (like it already does for all reserved regions).
> 
> "in such ranges", or in this case "in that range" would be better.  Also
> "for reserved regions elsewhere" IMO.
> 
> Just to confirm, we're saying our default treatment of identity mapping
> e820 reserved regions into the IOMMU is masking (or not) a missing
> RMRR/IVMD entry?

Yes, the provided tables are missing (some?) IVMD region(s) for the
USB devices (the 0xea and 0xeb pages at least, possibly others).  We
identity map reserved regions in the memory map for dom0 in order to
cover up for missing RMRR/IVMD regions, because it's quite common.

> >
> > Note there's a small difference in behavior, as holes in the low 1MB will no
> > longer be identity mapped to the p2m.
> >
> > Fixes: 6b4f6a31ace1 ('x86/PVH: de-duplicate mappings for first Mb of Dom0 
> > memory')
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> I suppose you intended to mark this for 4.18 as you CC'd Henry, and also
> send it for x86 (CC added)?

I didn't run add_maintainer on it, sorry.

> I'm tempted to commit it based on the diffstat alone.  How do we still
> have so much junk code like this lying around breaking things...
> 
> Anyway - it's a clear improvement.

I've got further improvements in this area, but not 4.18 material,
will try to post them soon anyway.

> But a question first.  Is this from debugging the XSA-442 fallout?  If
> so, it's probably worth mentioning the hardware we saw this on (which
> IIRC was fairly old AMD), and that XSA-442 unmasked a pre-existing bug. 
> And we think it's USB/PS2 emulation?

Yeah, can add this all.  It is triggered by pinot{0,1} which is AMD
Fam15h (AMD Opteron(tm) Processor 3350 HE).

Thanks, Roger.