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

[Marcel Apfelbaum <marcel@redhat.com>]

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
>