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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi,

> On Oct 12, 2023, at 00:48, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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 ..."
> 
>>  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)?

Fine for me to include this bugfix,

Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Kind regards,
Henry


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