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

[Don Slutz <dslutz@xxxxxxxxxxx>]

On 06/09/15 09:55, Paul Durrant wrote:
>> -----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.
> 

Nope.  I added routines that would call on the device listener
callbacks.  Then added the calls to the new routines when the secondary
bus number changed. What you are saying looks real close to what I did
on V1.

   -Don Slutz

>   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