Re: [Xen-devel] [PATCH v2 2/4] Extend device listener interface for PCI to PCI bridges

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Don Slutz [mailto:dslutz@xxxxxxxxxxx]
> Sent: 09 June 2015 14:53
> To: Paul Durrant; Slutz, Donald Christopher; qemu-devel@xxxxxxxxxx; xen-
> devel@xxxxxxxxxxxxx
> Cc: Michael S. Tsirkin; Stefano Stabellini; Don Slutz
> Subject: Re: [PATCH v2 2/4] Extend device listener interface for PCI to PCI
> bridges
> 
> On 06/09/15 05:08, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Don Slutz [mailto:dslutz@xxxxxxxxxxx]
> >> Sent: 08 June 2015 22:19
> >> To: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxx
> >> Cc: Michael S. Tsirkin; Paul Durrant; Stefano Stabellini; Don Slutz; Don 
> >> Slutz
> >> Subject: [PATCH v2 2/4] Extend device listener interface for PCI to PCI
> >> bridges
> >>
> >> 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.
> >>
> >> Extend the device listener interface to be called when ever the
> >> secondary bus number is set to a new value.  This new interface
> >> is called for all PCI devices that are on the secondary bridge.
> >>
> >
> > Can't you just make unrealize and realize calls back-to-back when the bus
> number changes, rather than having a new callback?
> >
> 
> That is what V1 had.  Michael S. Tsirkin did not like this.
> 

I thought he objected to you actually calling the actual bus unrealize/realize 
functions. All I'm suggesting is calling the device listener unrealize/realize 
callbacks, rather than adding a new one.

  Paul

>    -Don Slutz
> 
> >   Paul
> >
> >> Signed-off-by: Don Slutz <dslutz@xxxxxxxxxxx>
> >> CC: Don Slutz <don.slutz@xxxxxxxxx>
> >> ---
> >>  hw/core/qdev.c         |  7 +++++++
> >>  hw/pci/pci_bridge.c    | 18 ++++++++++++++++++
> >>  include/hw/qdev-core.h |  3 +++
> >>  3 files changed, 28 insertions(+)
> >>
> >> diff --git a/hw/core/qdev.c b/hw/core/qdev.c
> >> index b0f0f84..7728ee7 100644
> >> --- a/hw/core/qdev.c
> >> +++ b/hw/core/qdev.c
> >> @@ -239,6 +239,13 @@ void device_listener_unregister(DeviceListener
> >> *listener)
> >>      QTAILQ_REMOVE(&device_listeners, listener, link);
> >>  }
> >>
> >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d,
> void
> >> *opaque)
> >> +{
> >> +    uint8_t oldbus = GPOINTER_TO_INT(opaque);
> >> +
> >> +    DEVICE_LISTENER_CALL(change_pci_bus_num, Forward, d, oldbus);
> >> +}
> >> +
> >>  static void device_realize(DeviceState *dev, Error **errp)
> >>  {
> >>      DeviceClass *dc = DEVICE_GET_CLASS(dev);
> >> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> >> index 40c97b1..742c4d8 100644
> >> --- a/hw/pci/pci_bridge.c
> >> +++ b/hw/pci/pci_bridge.c
> >> @@ -248,6 +248,7 @@ 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);
> >>
> >>      pci_default_write_config(d, address, val, len);
> >>
> >> @@ -265,6 +266,14 @@ void pci_bridge_write_config(PCIDevice *d,
> >>          pci_bridge_update_mappings(s);
> >>      }
> >>
> >> +    if (oldbus != pci_get_byte(d->config + PCI_SECONDARY_BUS)) {
> >> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(s);
> >> +
> >> +        pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> >> +                            device_listener_change_pci_bus_num,
> >> +                            GINT_TO_POINTER(oldbus));
> >> +    }
> >> +
> >>      newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> >>      if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> >>          /* Trigger hot reset on 0->1 transition. */
> >> @@ -297,6 +306,7 @@ void pci_bridge_reset(DeviceState *qdev)
> >>  {
> >>      PCIDevice *dev = PCI_DEVICE(qdev);
> >>      uint8_t *conf = dev->config;
> >> +    uint8_t oldbus = conf[PCI_SECONDARY_BUS];
> >>
> >>      conf[PCI_PRIMARY_BUS] = 0;
> >>      conf[PCI_SECONDARY_BUS] = 0;
> >> @@ -329,6 +339,14 @@ void pci_bridge_reset(DeviceState *qdev)
> >>      pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0);
> >>
> >>      pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
> >> +
> >> +    if (oldbus) {
> >> +        PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(dev));
> >> +
> >> +        pci_for_each_device(sec_bus, pci_bus_num(sec_bus),
> >> +                            device_listener_change_pci_bus_num,
> >> +                            GINT_TO_POINTER(oldbus));
> >> +    }
> >>  }
> >>
> >>  /* default qdev initialization function for PCI-to-PCI bridge */
> >> diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
> >> index d4be92f..154b4c1 100644
> >> --- a/include/hw/qdev-core.h
> >> +++ b/include/hw/qdev-core.h
> >> @@ -168,6 +168,8 @@ struct DeviceState {
> >>  struct DeviceListener {
> >>      void (*realize)(DeviceListener *listener, DeviceState *dev);
> >>      void (*unrealize)(DeviceListener *listener, DeviceState *dev);
> >> +    void (*change_pci_bus_num)(DeviceListener *listener, PCIDevice *d,
> >> +                               uint8_t oldbus);
> >>      QTAILQ_ENTRY(DeviceListener) link;
> >>  };
> >>
> >> @@ -387,5 +389,6 @@ static inline bool qbus_is_hotpluggable(BusState
> >> *bus)
> >>
> >>  void device_listener_register(DeviceListener *listener);
> >>  void device_listener_unregister(DeviceListener *listener);
> >> +void device_listener_change_pci_bus_num(PCIBus *b, PCIDevice *d,
> void
> >> *opaque);
> >>
> >>  #endif
> >> --
> >> 1.8.4
> >

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel