| Message ID | 20160118230413.2140.8336.stgit@gimli.home |
|---|---|
| State | New |
| Headers | show |
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 <alex.williamson@redhat.com> > --- > 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? Anyway, it looks OK to me. Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> Thanks, Marcel
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 <alex.williamson@redhat.com> > > --- > > 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 <marcel@redhat.com> Thanks, Alex
On 01/19/2016 06:38 PM, Alex Williamson wrote: > 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 <alex.williamson@redhat.com> >>> --- >>> 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. Is what I thought. Thanks, Marcel > >> Anyway, it looks OK to me. >> >> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > > Thanks, > Alex >
Hi Michael, Would you like me to send this for a 4th time or are you OK with me pulling this through my tree with Marcel's R-b? Thanks, Alex On Mon, 18 Jan 2016 16:06:03 -0700 Alex Williamson <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com> > --- > 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. >
On Wed, Feb 17, 2016 at 01:28:12PM -0700, Alex Williamson wrote: > > Hi Michael, > > Would you like me to send this for a 4th time or are you OK with me > pulling this through my tree with Marcel's R-b? Thanks, > > Alex Sorry, keep forgetting about it. > > On Mon, 18 Jan 2016 16:06:03 -0700 > Alex Williamson <alex.williamson@redhat.com> 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 <alex.williamson@redhat.com> > > --- > > 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. > > It's kind of nasty that we do it on each access ... How about storing the size in device itself?
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.
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 <alex.williamson@redhat.com> --- 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(+)