Re: [Qemu-devel] [PATCH 1/1] Fix device listener interface for PCI to PCI bridges

[Michael S. Tsirkin]

On Thu, May 28, 2015 at 04:46:37AM -0400, Don Slutz wrote:
> The commit 707ff80021ccd7a68f4b3d2c44eebf87efbb41c4 assumed that a
> PCI device has a static address.  This is not true for PCI devices
> that are on the secondary bus of a PCI to PCI bridge.
> 
> BIOS and/or guest OS can change the secondary bus number to a new
> value at any time.
> 
> When a PCI to PCI bridge bridge is reset, the secondary bus number
> is set to zero.  Normally the BIOS will set it to 255 during PCI bus
> scanning so that only the PCI devices on the root can be accessed
> via bus 0.  Later it is set to a number between 1 and 254.
> 
> Adjust xen_map_pcidev() to not register with Xen for secondary bus
> numbers 0 and 255.
> 
> Extend the device listener interface to be called when ever the
> secondary bus number is set to a usable value.  This includes
> a call on unrealize if the secondary bus number was valid.
> 
> Signed-off-by: Don Slutz <address@hidden>
> ---
> 
> Note: Right now hvmloader in Xen does not handle PCI to PCI bridges
> and so SeaBIOS does not have access to PCI device(s) on secondary
> buses.  How ever domU can setup the secondary bus(es) and this patch
> will restore access to these PCI devices.
> 
>  hw/core/qdev.c              | 10 ++++++++++
>  hw/pci/pci_bridge.c         | 30 ++++++++++++++++++++++++++++++
>  include/hw/qdev-core.h      |  2 ++
>  include/hw/xen/xen_common.h | 31 +++++++++++++++++++++++++------
>  trace-events                |  1 +
>  5 files changed, 68 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> index b0f0f84..6a514ee 100644
> --- a/hw/core/qdev.c
> +++ b/hw/core/qdev.c
> @@ -239,6 +239,16 @@ void device_listener_unregister(DeviceListener *listener)
>      QTAILQ_REMOVE(&device_listeners, listener, link);
>  }
>  
> +void device_listener_realize(DeviceState *dev)
> +{
> +    DEVICE_LISTENER_CALL(realize, Forward, dev);
> +}
> +
> +void device_listener_unrealize(DeviceState *dev)
> +{
> +    DEVICE_LISTENER_CALL(unrealize, Forward, dev);
> +}
> +
>  static void device_realize(DeviceState *dev, Error **errp)
>  {
>      DeviceClass *dc = DEVICE_GET_CLASS(dev);


This looks wrong.  Devices not accessible to config cycles are still
accessible e.g. to memory or IO.  It's not the same as unrealize.

You need some other API that makes sense,
probably pci specific.



> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 40c97b1..042680d 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -241,6 +241,18 @@ void pci_bridge_update_mappings(PCIBridge *br)
>      pci_bridge_region_cleanup(br, w);
>  }
>  
> +static void pci_bridge_realize_sub(PCIBus *b, PCIDevice *d,
> +                                   void *opaque)
> +{
> +    device_listener_realize(DEVICE(d));
> +}
> +
> +static void pci_bridge_unrealize_sub(PCIBus *b, PCIDevice *d,
> +                                     void *opaque)
> +{
> +    device_listener_unrealize(DEVICE(d));
> +}
> +
>  /* default write_config function for PCI-to-PCI bridge */
>  void pci_bridge_write_config(PCIDevice *d,
>                               uint32_t address, uint32_t val, int len)
> @@ -248,6 +260,8 @@ void pci_bridge_write_config(PCIDevice *d,
>      PCIBridge *s = PCI_BRIDGE(d);
>      uint16_t oldctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>      uint16_t newctl;
> +    uint8_t oldbus = pci_get_byte(d->config + PCI_SECONDARY_BUS);
> +    uint8_t newbus;
>  
>      pci_default_write_config(d, address, val, len);
>  
> @@ -265,6 +279,22 @@ void pci_bridge_write_config(PCIDevice *d,
>          pci_bridge_update_mappings(s);
>      }
>  
> +    newbus = pci_get_byte(d->config + PCI_SECONDARY_BUS);
> +    if (newbus != oldbus) {
> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(s);
> +
> +        if (oldbus && oldbus != 255) {
> +            pci_set_byte(d->config + PCI_SECONDARY_BUS, oldbus);
> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                                pci_bridge_unrealize_sub, NULL);
> +            pci_set_byte(d->config + PCI_SECONDARY_BUS, newbus);
> +        }
> +        if (newbus && newbus != 255) {
> +            pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> +                                pci_bridge_realize_sub, NULL);
> +        }
> +    }
> +


This is relying on undocumented assumptions and how specific firmware
works. There's nothing special about bus number 255, and 0 is not very
special either (except it happens to be the reset value).

To know whether device is accessible to config cycles, you
really need to scan the hierarchy from root downwards.

>      newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
>      if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
>          /* Trigger hot reset on 0->1 transition. */
> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> index d4be92f..8bd38af 100644
> --- a/include/hw/qdev-core.h
> +++ b/include/hw/qdev-core.h
> @@ -387,5 +387,7 @@ static inline bool qbus_is_hotpluggable(BusState *bus)
>  
>  void device_listener_register(DeviceListener *listener);
>  void device_listener_unregister(DeviceListener *listener);
> +void device_listener_realize(DeviceState *dev);
> +void device_listener_unrealize(DeviceState *dev);
>  
>  #endif
> diff --git a/include/hw/xen/xen_common.h b/include/hw/xen/xen_common.h
> index 38f29fb..1c27d51 100644
> --- a/include/hw/xen/xen_common.h
> +++ b/include/hw/xen/xen_common.h
> @@ -347,12 +347,31 @@ static inline void xen_map_pcidev(XenXC xc, domid_t dom,
>                                    ioservid_t ioservid,
>                                    PCIDevice *pci_dev)
>  {
> -    trace_xen_map_pcidev(ioservid, pci_bus_num(pci_dev->bus),
> -                         PCI_SLOT(pci_dev->devfn), PCI_FUNC(pci_dev->devfn));
> -    xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid,
> -                                      0, pci_bus_num(pci_dev->bus),
> -                                      PCI_SLOT(pci_dev->devfn),
> -                                      PCI_FUNC(pci_dev->devfn));
> +    if (pci_bus_is_root(pci_dev->bus)) {
> +        trace_xen_map_pcidev(ioservid, 0,
> +                             PCI_SLOT(pci_dev->devfn),
> +                             PCI_FUNC(pci_dev->devfn));
> +        xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid,
> +                                          0, 0,
> +                                          PCI_SLOT(pci_dev->devfn),
> +                                          PCI_FUNC(pci_dev->devfn));
> +    } else {
> +        int subbus = pci_bus_num(pci_dev->bus);
> +
> +        if (subbus && subbus != 255) {
> +            trace_xen_map_pcidev(ioservid, subbus,
> +                                 PCI_SLOT(pci_dev->devfn),
> +                                 PCI_FUNC(pci_dev->devfn));
> +            xc_hvm_map_pcidev_to_ioreq_server(xc, dom, ioservid,
> +                                              0, subbus,
> +                                              PCI_SLOT(pci_dev->devfn),
> +                                              PCI_FUNC(pci_dev->devfn));
> +        } else {
> +            trace_xen_map_pcidev_unknown(ioservid,
> +                                         PCI_SLOT(pci_dev->devfn),
> +                                         PCI_FUNC(pci_dev->devfn));
> +        }
> +    }
>  }
>  
>  static inline void xen_unmap_pcidev(XenXC xc, domid_t dom,
> diff --git a/trace-events b/trace-events
> index 11387c3..63c0361 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -931,6 +931,7 @@ xen_unmap_mmio_range(uint32_t id, uint64_t start_addr, 
> uint64_t end_addr) "id: %
>  xen_map_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) 
> "id: %u start: %#"PRIx64" end: %#"PRIx64
>  xen_unmap_portio_range(uint32_t id, uint64_t start_addr, uint64_t end_addr) 
> "id: %u start: %#"PRIx64" end: %#"PRIx64
>  xen_map_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: %u 
> bdf: %02x.%02x.%02x"
> +xen_map_pcidev_unknown(uint32_t id, uint8_t dev, uint8_t func) "id: %u bdf: 
> ??.%02x.%02x"
>  xen_unmap_pcidev(uint32_t id, uint8_t bus, uint8_t dev, uint8_t func) "id: 
> %u bdf: %02x.%02x.%02x"
>  
>  # xen-mapcache.c
> -- 
> 1.8.4