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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.04.2020 19:46, Harsha Shamsundara Havanur wrote:
> --- 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] = {};

256 bytes of new stack space consumption looks quite a lot to me.
Did you consider using two 256-bit bitmaps instead, totaling to
an extra 64 bytes of stack space needed?

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

Identifying the commit that introduced the issue, even if very old,
would be nice (bbfa22f8c9ca). From looking at current code I first
got the impression that it might have been a result of splitting a
loop, as the main issue is that the bits get set after a first
BAR got programmed, without considering that there might be more.
However, the original commit looks to have assumed that there's
at most one BAR or each type per device (which may have been true
back then for just the few emulated devices there were).

> @@ -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);
>      }

I agree with Andrew that there doesn't look to be a reason to
fiddle with bus mastering here. This is the more that you don't
re-enable it later.

I'm also curious whether there are actually devices getting
handed to the domain (and hence hvmloader) with the memory
and/or I/O decode bits set. This would look to be wrong to me,
and would perhaps want fixing wherever they get set prematurely.
(This isn't to say that, to be on the safe side, we couldn't
also re-clear them here. Maybe there should be a warning issued
if either bit is set?)

Jan