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

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

[Alex Williamson]

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.

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

[Marcel Apfelbaum]

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

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

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

[Marcel Apfelbaum]

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
>

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

[Alex Williamson]

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

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

[Michael S. Tsirkin]

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?

-- 
MST