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

["Shamsundara Havanur, Harsha" <havanur@amazon.com>]

On Tue, 2020-04-14 at 16:12 +0200, Jan Beulich wrote:
> CAUTION: This email originated from outside of the organization. Do
> not click links or open attachments unless you can confirm the sender
> and know the content is safe.
> 
> 
> 
> 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@amazon.com>
> > Reviewed-by: Paul Durrant <pdurrant@amazon.com>
> > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> 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.
> 

I chose uint8 array over bitmaps as this reduces complexity of code
to get and set individual bits. Sorry for missing this out in the v1
conversation.

> > @@ -120,6 +121,9 @@ void pci_setup(void)
> >       */
> >      bool allow_memory_relocate = 1;
> > 
> > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEMOR
> > Y != 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.
> 
I agree, this needs to be done before writing BARS with ~0

> > @@ -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.
> 
Good point. I will make these changes in v4.

> Jan