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

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

On Tue, 2020-04-14 at 10:20 +0200, Roger Pau Monné 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 Tue, Apr 14, 2020 at 10:10:09AM +0200, Roger Pau Monné wrote:
> > On Mon, Apr 13, 2020 at 09:33:42PM +0000, Harsha Shamsundara
> > Havanur wrote:
> > > 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
> > > incorrect memory mapping as soon as the lower half of the 64 bit
> > > bar
> > > is programmed, which displaced any RAM mappings under 4G. After
> > > the
> > > upper half is programmed PCI memory mapping is restored to its
> > > intended mapping 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.
> > > 
> > > Signed-off-by: Harsha Shamsundara Havanur <havanur@amazon.com>
> > > Reviewed-by: Paul Durrant <pdurrant@amazon.com>
> > > Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > > ---
> > >  tools/firmware/hvmloader/pci.c | 35 +++++++++++++++++++++++++++-
> > > -------
> > >  1 file changed, 27 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/tools/firmware/hvmloader/pci.c
> > > b/tools/firmware/hvmloader/pci.c
> > > index 0b708bf578..f74471b255 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 {
> > > @@ -120,6 +121,9 @@ void pci_setup(void)
> > >       */
> > >      bool allow_memory_relocate = 1;
> > > 
> > > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_MEM
> > > ORY != PCI_COMMAND_MEMORY);
> > > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO
> > > != PCI_COMMAND_IO);
> > > +    BUILD_BUG_ON((typeof(*pci_devfn_decode_type))PCI_COMMAND_IO
> > > != PCI_COMMAND_MASTER);
> > >      s = xenstore_read(HVM_XS_ALLOW_MEMORY_RELOCATE, NULL);
> > >      if ( s )
> > >          allow_memory_relocate = strtoll(s, NULL, 0);
> > > @@ -289,9 +293,22 @@ 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 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.
> > > +         *
> > > +         * Capture the state of bus master to restore it back
> > > once BAR
> > > +         * programming is completed.
> > > +         */
> > >          cmd = pci_readw(devfn, PCI_COMMAND);
> > > -        cmd |= PCI_COMMAND_MASTER;
> > > +        pci_devfn_decode_type[devfn] = cmd &
> > > ~(PCI_COMMAND_MEMORY | PCI_COMMAND_IO);
> > > +        cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY |
> > > PCI_COMMAND_IO);
> > >          pci_writew(devfn, PCI_COMMAND, cmd);
> > >      }
> > > 
> > > @@ -503,10 +520,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 )
> > > @@ -526,10 +542,13 @@ 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 space. Restore saved BUS MASTER
> > > state */
> > > +    for ( devfn = 0; devfn < 256; devfn++ )
> > > +        if ( pci_devfn_decode_type[devfn] )
> > > +            pci_writew(devfn, PCI_COMMAND,
> > > pci_devfn_decode_type[devfn]);
> > 
> > Why don't you enable the decoding after done with programming all
> > the
> > BARs on the device in the loop above? Is there any reason to defer
> > this until all devices have been programmed?
> > 
> > If so, I think you would also need to introduce a pre-loop that
> > disables all of this for all devices before programming the BARs,
> > or
> > else you are still programming BARs while some devices might have
> > the
> > bus mastering or decoding bits enabled.
> 
> Oh, forget that last paragraph, I see that decoding is indeed
> disabled
> before programming any devices BARs. I still think that it might be
> feasible to enable it once all BARs on the device have been
> programmed, which would allow to get rid of the extra loop and the
> pci_devfn_decode_type local variable.

BARs are programmed sorted by their memory requirement and thus same
pci function could be programmed multiple times in this loop
422     /* Assign iomem and ioport resources in descending order of
size. */
423     for ( i = 0; i < nr_bars; i++ )

Hence I am waiting for this loop to be completed to enable decode bits
in a saparate loop later.
> 
> Thanks, Roger.