Hi,
> On Oct 12, 2023, at 00:48, Andrew Cooper <andrew.coop...@citrix.com> 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....@citrix.com>
>
> 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.w...@arm.com>
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
