Re: [XEN PATCH v3] hvmloader: Enable MMIO and I/O decode, after all resource allocation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/04/2020 15:12, Jan Beulich wrote:
> On 14.04.2020 13:12, Harsha Shamsundara Havanur wrote:
>> It was observed that PCI MMIO and/or IO BARs were programmed with
>> memory and I/O decodes (bits 0 and 1 of PCI COMMAND register) enabled,
>> during PCI setup phase. This resulted in incorrect memory mapping as
>> soon as the lower half of the 64 bit bar is programmed.
>> This displaced any RAM mappings under 4G. After the
>> upper half is programmed PCI memory mapping is restored to its
>> intended high mem location, but the RAM displaced is not restored.
>> The OS then continues to boot and function until it tries to access
>> the displaced RAM at which point it suffers a page fault and crashes.
>>
>> This patch address the issue by deferring enablement of memory and
>> I/O decode in command register until all the resources, like interrupts
>> I/O and/or MMIO BARs for all the PCI device functions are programmed,
>> in the descending order of memory requested.
>>
>> Signed-off-by: Harsha Shamsundara Havanur <havanur@xxxxxxxxxx>
>> Reviewed-by: Paul Durrant <pdurrant@xxxxxxxxxx>
>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

I have not acked this patch.  My comment on v1 was correctly your
mis-spelling of "Ack-by" for Paul's tag.  I apologise if that wasn't
terribly clear.

> You've fixed bugs, implying you need to drop tags covering the code
> which was fixed. As said on v2, imo you should have dropped the tags
> then already.
>
>> --- a/tools/firmware/hvmloader/pci.c
>> +++ b/tools/firmware/hvmloader/pci.c
>> @@ -84,6 +84,7 @@ void pci_setup(void)
>>      uint32_t vga_devfn = 256;
>>      uint16_t class, vendor_id, device_id;
>>      unsigned int bar, pin, link, isa_irq;
>> +    uint8_t pci_devfn_decode_type[256] = {};
> I'm still waiting for a reply on my v1 comment on the stack
> consumption of this, suggesting two 256-bit bitmaps instead.

256 bytes of stack space isn't something to worry about.  Definitely not
for the complexity of using bitmaps instead.

~Andrew