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

[Jan Beulich <jbeulich@xxxxxxxx>]

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>

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.

> @@ -120,6 +121,9 @@ void pci_setup(void)
>       */
>      bool allow_memory_relocate = 1;
>  
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMORY != 
> PCI_COMMAND_MEMORY);
> +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO != 
> PCI_COMMAND_IO);

These lines are too long.

> @@ -288,10 +292,21 @@ void pci_setup(void)
>              printf("pci dev %02x:%x INT%c->IRQ%u\n",
>                     devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
>          }
> -
>          /* Enable bus mastering. */

Please don't drop a blank line like this.

>          cmd = pci_readw(devfn, PCI_COMMAND);
>          cmd |= PCI_COMMAND_MASTER;
> +
> +        /*
> +         * Disable memory and I/O decode,
> +         * It is recommended that BAR programming be done whilst
> +         * decode bits are cleared to avoid incorrect mappings being created,
> +         * when 64-bit memory BAR is programmed first by writing the lower 
> half
> +         * and then the upper half, which first maps to an address under 4G
> +         * replacing any RAM mapped in that address, which is not restored
> +         * back after the upper half is written and PCI memory is correctly
> +         * mapped to its intended high mem address.
> +         */
> +        cmd &= ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
>          pci_writew(devfn, PCI_COMMAND, cmd);
>      }

I'd like to note that the disabling of IO and MEMORY you do here comes too
late: It should happen before any of the BARs gets written with ~0. In
particular for 64-bit BARs these writes can again cause undue effects on
the intermediately resulting effective addresses.

> @@ -526,10 +537,16 @@ void pci_setup(void)
>           * has IO enabled, even if there is no I/O BAR on that
>           * particular device.
>           */
> -        cmd = pci_readw(vga_devfn, PCI_COMMAND);
> -        cmd |= PCI_COMMAND_IO;
> -        pci_writew(vga_devfn, PCI_COMMAND, cmd);
> +        pci_devfn_decode_type[vga_devfn] |= PCI_COMMAND_IO;
>      }
> +
> +    /* Enable memory and I/O decode. */
> +    for ( devfn = 0; devfn < 256; devfn++ )
> +        if ( pci_devfn_decode_type[devfn] ) {

Style: The brace belongs on its own line.

To save one set of writes to the command registers I would, btw,
be okay with you moving the MASTER enabling here.

Jan