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

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@xxxxxxxxxxxxxxxxxxxx> On Behalf Of Harsha 
> Shamsundara Havanur
> Sent: 06 April 2020 18:47
> To: xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Liu <wl@xxxxxxx>; Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Ian 
> Jackson
> <ian.jackson@xxxxxxxxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>; Harsha 
> Shamsundara Havanur
> <havanur@xxxxxxxxxx>; Roger Pau Monné <roger.pau@xxxxxxxxxx>
> Subject: [XEN PATCH] hvmloader: Enable MMIO and I/O decode, after all 
> resource allocation
> 
> It was observed that PCI MMIO and/or IO BARs were programmed with
> BUS master, memory and I/O decodes (bits 0,1 and 2 of PCI COMMAND
> register) enabled, during PCI setup phase. This resulted in
> spurious and premature bus transactions as soon as the lower bar of
> the 64 bit bar is programmed. It is highly recommended that BARs be
> programmed whilst decode bits are cleared to avoid spurious bus
> transactions.
> 

It's not so much spurious transactions that are the issue. I think "spurious 
and premature bus transactions" should be replaced with "incorrect mappings 
being created".

I believe the PCI spec says all three bits should be clear after reset anyway, 
and BAR programming whilst decodes are enabled causes problems for emulators 
such as QEMU which need to create and destroy mappings between the gaddr being 
programming into the virtual BAR and the maddr programmed into the physical BAR.
Specifically the case we see is that a 64-bit memory BAR is programmed by 
writing the lower half and then the upper half. After the first write the BAR 
is mapped to an address under 4G that happens to contain RAM, which is 
displaced by the mapping. After the second write the BAR is re-mapped to the 
intended location but the RAM displaced by the other mapping is not restored. 
The OS then continues to boot and function until at some point it happens to 
try to use that RAM at which point it suffers a page fault and crashes. It was 
only by noticing that the faulting address lay within the transient BAR mapping 
that we figured out what was happening.

> 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.
> PCI bus memory and I/O space is enabled in command register after
> all the resources like interrupts, I/O and/or MMIO BARs are
> programmed for all valid device functions. PCI BUS MASTER is kept
> disabled in the bootloader as this needs to be enabled by the guest
> OS driver once it initializes and takes control of the device.
> 
> Signed-off-by: Harsha Shamsundara Havanur <havanur@xxxxxxxxxx>
> Ack-by: Paul Durrant <pdurrant@xxxxxxxxxx>

With the comment fixed as I suggest, you can replace this with:

Reviewed-by: Paul Durrant <paul@xxxxxxx>

> ---
>  tools/firmware/hvmloader/pci.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/firmware/hvmloader/pci.c b/tools/firmware/hvmloader/pci.c
> index 0b708bf578..0f31866453 100644
> --- 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] = {};
> 
>      /* Resources assignable to PCI devices via BARs. */
>      struct resource {
> @@ -289,9 +290,14 @@ void pci_setup(void)
>                     devfn>>3, devfn&7, 'A'+pin-1, isa_irq);
>          }
> 
> -        /* Enable bus mastering. */
> +        /*
> +         * Disable bus mastering, memory and I/O space, which is typical 
> device
> +         * reset state. It is recommended that BAR programming be done whilst
> +         * decode bits are cleared to avoid spurious DMAs and bus 
> transactions.
> +         * Bus master should be enabled by guest driver when it deems fit.
> +         */
>          cmd = pci_readw(devfn, PCI_COMMAND);
> -        cmd |= PCI_COMMAND_MASTER;
> +        cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
>          pci_writew(devfn, PCI_COMMAND, cmd);
>      }
> 
> @@ -503,10 +509,9 @@ void pci_setup(void)
>          if ( (bar_reg == PCI_ROM_ADDRESS) ||
>               ((bar_data & PCI_BASE_ADDRESS_SPACE) ==
>                PCI_BASE_ADDRESS_SPACE_MEMORY) )
> -            cmd |= PCI_COMMAND_MEMORY;
> +            pci_devfn_decode_type[devfn] |= PCI_COMMAND_MEMORY;
>          else
> -            cmd |= PCI_COMMAND_IO;
> -        pci_writew(devfn, PCI_COMMAND, cmd);
> +            pci_devfn_decode_type[devfn] |= PCI_COMMAND_IO;
>      }
> 
>      if ( pci_hi_mem_start )
> @@ -530,6 +535,15 @@ void pci_setup(void)
>          cmd |= PCI_COMMAND_IO;
>          pci_writew(vga_devfn, PCI_COMMAND, cmd);
>      }
> +
> +    /* Enable memory and I/O space. */
> +    for ( devfn = 0; devfn < 256; devfn++ )
> +        if ( pci_devfn_decode_type[devfn] )
> +        {
> +            cmd = pci_readw(devfn, PCI_COMMAND);
> +            cmd |= pci_devfn_decode_type[devfn];
> +            pci_writew(devfn, PCI_COMMAND, cmd);
> +        }
>  }
> 
>  /*
> --
> 2.16.6
>