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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 ..."

>   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 ..." ?

> 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?

>
> 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'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.

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?

Thanks,

~Andrew