Re: [Qemu-devel] [RE-RESEND PATCH] pci: Adjust PCI config limit based on bus topology

[Alex Williamson]

On Tue, 2016-01-19 at 10:54 +0200, Marcel Apfelbaum wrote:
> On 01/19/2016 01:06 AM, Alex Williamson wrote:
> > A conventional PCI bus does not support config space accesses above
> > the standard 256 byte configuration space.  PCIe-to-PCI bridges are
> > not permitted to forward transactions if the extended register
> > address
> > field is non-zero and must handle it as an unsupported request
> > (PCIe
> > bridge spec rev 1.0, 4.1.3, 4.1.4).  Therefore, we should not
> > support
> > extended config space if there is a conventional bus anywhere on
> > the
> > path to a device.
> > 
> > Signed-off-by: Alex Williamson <address@hidden>
> > ---
> > Previous postings:
> > https://lists.gnu.org/archive/html/qemu-devel/2015-10/msg05384.html
> > https://lists.gnu.org/archive/html/qemu-devel/2015-11/msg02422.html
> > 
> >   hw/pci/pci_host.c |   26 ++++++++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> > 
> > diff --git a/hw/pci/pci_host.c b/hw/pci/pci_host.c
> > index 49f59a5..3a3e294 100644
> > --- a/hw/pci/pci_host.c
> > +++ b/hw/pci/pci_host.c
> > @@ -19,6 +19,7 @@
> >    */
> > 
> >   #include "hw/pci/pci.h"
> > +#include "hw/pci/pci_bridge.h"
> >   #include "hw/pci/pci_host.h"
> >   #include "hw/pci/pci_bus.h"
> >   #include "trace.h"
> > @@ -49,9 +50,29 @@ static inline PCIDevice
> > *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> >       return pci_find_device(bus, bus_num, devfn);
> >   }
> > 
> > +static void pci_adjust_config_limit(PCIBus *bus, uint32_t *limit)
> > +{
> > +    if (*limit > PCI_CONFIG_SPACE_SIZE) {
> > +        if (!pci_bus_is_express(bus)) {
> > +            *limit = PCI_CONFIG_SPACE_SIZE;
> > +            return;
> > +        }
> > +
> > +        if (!pci_bus_is_root(bus)) {
> > +            PCIDevice *bridge = pci_bridge_get_device(bus);
> > +            pci_adjust_config_limit(bridge->bus, limit);
> > +        }
> > +    }
> > +}
> > +
> >   void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t
> > addr,
> >                                     uint32_t limit, uint32_t val,
> > uint32_t len)
> >   {
> > +    pci_adjust_config_limit(pci_dev->bus, &limit);
> > +    if (limit <= addr) {
> > +        return;
> > +    }
> > +
> >       assert(len <= 4);
> >       /* non-zero functions are only exposed when function 0 is
> > present,
> >        * allowing direct removal of unexposed functions.
> > @@ -70,6 +91,11 @@ uint32_t pci_host_config_read_common(PCIDevice
> > *pci_dev, uint32_t addr,
> >   {
> >       uint32_t ret;
> > 
> > +    pci_adjust_config_limit(pci_dev->bus, &limit);
> > +    if (limit <= addr) {
> > +        return ~0x0;
> > +    }
> > +
> >       assert(len <= 4);
> >       /* non-zero functions are only exposed when function 0 is
> > present,
> >        * allowing direct removal of unexposed functions.
> > 
> > 
> 
> Quick question: could we check the limit as part of pci_config_size?

If we plugin a physical PCIe card behind a bridge that masks access to
the extended configuration space, does the config size for that card
change?  No, it's up to the bridge to drop the transactions, which
seems like how we probably want to handle it in QEMU as well.

> Anyway, it looks OK to me.
> 
> Reviewed-by: Marcel Apfelbaum <address@hidden>

Thanks,
Alex