Re: [Xen-devel] [PATCH v2 1/2] Add PCI bus listener interface

["Michael S. Tsirkin" <mst@xxxxxxxxxx>]

On Tue, Oct 14, 2014 at 12:44:06PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@xxxxxxxxxx]
> > Sent: 14 October 2014 12:27
> > To: Paul Durrant
> > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Paolo
> > Bonzini; Andreas Faerber; Thomas Huth; Peter Crosthwaite; Christian
> > Borntraeger
> > Subject: Re: [PATCH v2 1/2] Add PCI bus listener interface
> > 
> > On Tue, Oct 14, 2014 at 10:08:06AM +0000, Paul Durrant wrote:
> > > > -----Original Message-----
> > > > From: Michael S. Tsirkin [mailto:mst@xxxxxxxxxx]
> > > > Sent: 14 October 2014 10:54
> > > > To: Paul Durrant
> > > > Cc: qemu-devel@xxxxxxxxxx; xen-devel@xxxxxxxxxxxxxxxxxxxx; Paolo
> > > > Bonzini; Andreas Faerber; Thomas Huth; Peter Crosthwaite; Christian
> > > > Borntraeger
> > > > Subject: Re: [PATCH v2 1/2] Add PCI bus listener interface
> > > >
> > > > On Mon, Oct 13, 2014 at 01:52:46PM +0100, Paul Durrant wrote:
> > > > > The Xen ioreq-server API, introduced in Xen 4.5, requires that PCI
> > device
> > > > > models explicitly register with Xen for config space accesses. This 
> > > > > patch
> > > > > adds a PCI bus listener interface which can be used by the Xen 
> > > > > interface
> > > > > code to monitor PCI buses for arrival and departure of devices.
> > > > >
> > > > > Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> > > > > Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> > > > > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > > > > Cc: Andreas Faerber <afaerber@xxxxxxx>
> > > > > Cc: Thomas Huth <thuth@xxxxxxxxxxxxxxxxxx>
> > > > > Cc: Peter Crosthwaite <peter.crosthwaite@xxxxxxxxxx>
> > > > > Cc: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> > > >
> > > > Do you really need multiple notifiers?
> > > >
> > >
> > > I don't quite understand what you mean by multiple notifiers. Are you
> > suggesting that the PCI listener could be combined with the memory
> > listener?
> > 
> > 
> > Bus is already notified about the hotplug.
> > Do you need another notification, or can you override that one?
> > 
> 
> I'm not sure. IIRC there's some sort of 'coldplug' bypass for things that are 
> present on the PCI bus before the guest is started, so I'm not sure the 
> notification would happen.

AFAIK it happens for all devices including coldplug.

> > >
> > > > In any case, I think such a mechanism makes more sense on QOM level:
> > > > we have APIs to find objects of a given class and/or at a given path,
> > > > why not a mechanism to get notified when said objects are
> > added/removed.
> > > >
> > >
> > > Having a general object listener could work as long as notifications
> > > could be filtered such that the Xen code could get notifications
> > > purely for PCI devices.
> > 
> > As all PCI devices inherit TYPE_PCI_DEVICE, it's easy enough
> > to do the filtering.
> > 
> 
> That sounds plausible then. I'll investigate.
> 
> > > The set of listeners clearly cannot be added
> > > to the object itself though, as you could then only register listeners
> > > after the object is created.
> > >
> > >   Paul
> > 
> > Yes.
> > 
> > 
> > One other thing you need to consider: do you want to be notified
> > when removal is requested, or after it happens?
> > 
> 
> The unmapping of the device from Xen needs to happen after it's really gone 
> away, so I guess object destruction time it right for that one.
> 
>   Paul
> 
> > > >
> > > >
> > > > > ---
> > > > >  hw/pci/pci.c            |   65
> > > > +++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/pci/pci.h    |    9 +++++++
> > > > >  include/qemu/typedefs.h |    1 +
> > > > >  3 files changed, 75 insertions(+)
> > > > >
> > > > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > > > index 6ce75aa..53c955d 100644
> > > > > --- a/hw/pci/pci.c
> > > > > +++ b/hw/pci/pci.c
> > > > > @@ -122,6 +122,66 @@ static uint16_t pci_default_sub_device_id =
> > > > PCI_SUBDEVICE_ID_QEMU;
> > > > >
> > > > >  static QLIST_HEAD(, PCIHostState) pci_host_bridges;
> > > > >
> > > > > +static QTAILQ_HEAD(pci_listeners, PCIListener) pci_listeners
> > > > > +    = QTAILQ_HEAD_INITIALIZER(pci_listeners);
> > > > > +
> > > > > +enum ListenerDirection { Forward, Reverse };
> > > > > +
> > > > > +#define PCI_LISTENER_CALL(_callback, _direction, _args...)      \
> > > > > +    do {                                                        \
> > > > > +        PCIListener *_listener;                                 \
> > > > > +                                                                \
> > > > > +        switch (_direction) {                                   \
> > > > > +        case Forward:                                           \
> > > > > +            QTAILQ_FOREACH(_listener, &pci_listeners, link) {   \
> > > > > +                if (_listener->_callback) {                     \
> > > > > +                    _listener->_callback(_listener, ##_args);   \
> > > > > +                }                                               \
> > > > > +            }                                                   \
> > > > > +            break;                                              \
> > > > > +        case Reverse:                                           \
> > > > > +            QTAILQ_FOREACH_REVERSE(_listener, &pci_listeners,   \
> > > > > +                                   pci_listeners, link) {       \
> > > > > +                if (_listener->_callback) {                     \
> > > > > +                    _listener->_callback(_listener, ##_args);   \
> > > > > +                }                                               \
> > > > > +            }                                                   \
> > > > > +            break;                                              \
> > > > > +        default:                                                \
> > > > > +            abort();                                            \
> > > > > +        }                                                       \
> > > > > +    } while (0)
> > > > > +
> > > > > +static int pci_listener_add(DeviceState *dev, void *opaque)
> > > > > +{
> > > > > +    if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) {
> > > > > +        PCIDevice *pci_dev = PCI_DEVICE(dev);
> > > > > +
> > > > > +        PCI_LISTENER_CALL(device_add, Forward, pci_dev);
> > > > > +    }
> > > > > +
> > > > > +    return 0;
> > > > > +}
> > > > > +
> > > > > +void pci_listener_register(PCIListener *listener)
> > > > > +{
> > > > > +    PCIHostState *host;
> > > > > +
> > > > > +    QTAILQ_INSERT_TAIL(&pci_listeners, listener, link);
> > > > > +
> > > > > +    QLIST_FOREACH(host, &pci_host_bridges, next) {
> > > > > +        PCIBus *bus = host->bus;
> > > > > +
> > > > > +        qbus_walk_children(&bus->qbus, NULL, NULL, pci_listener_add,
> > > > > +                           NULL, NULL);
> > > > > +    }
> > > > > +}
> > > > > +
> > > > > +void pci_listener_unregister(PCIListener *listener)
> > > > > +{
> > > > > +    QTAILQ_REMOVE(&pci_listeners, listener, link);
> > > > > +}
> > > > > +
> > > > >  static int pci_bar(PCIDevice *d, int reg)
> > > > >  {
> > > > >      uint8_t type;
> > > > > @@ -795,6 +855,8 @@ static void pci_config_free(PCIDevice *pci_dev)
> > > > >
> > > > >  static void do_pci_unregister_device(PCIDevice *pci_dev)
> > > > >  {
> > > > > +    PCI_LISTENER_CALL(device_del, Reverse, pci_dev);
> > > > > +
> > > > >      pci_dev->bus->devices[pci_dev->devfn] = NULL;
> > > > >      pci_config_free(pci_dev);
> > > > >
> > > > > @@ -878,6 +940,9 @@ static PCIDevice
> > *do_pci_register_device(PCIDevice
> > > > *pci_dev, PCIBus *bus,
> > > > >      pci_dev->config_write = config_write;
> > > > >      bus->devices[devfn] = pci_dev;
> > > > >      pci_dev->version_id = 2; /* Current pci device vmstate version */
> > > > > +
> > > > > +    PCI_LISTENER_CALL(device_add, Forward, pci_dev);
> > > > > +
> > > > >      return pci_dev;
> > > > >  }
> > > > >
> > > > > diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > > > > index c352c7b..6c21b37 100644
> > > > > --- a/include/hw/pci/pci.h
> > > > > +++ b/include/hw/pci/pci.h
> > > > > @@ -303,6 +303,15 @@ struct PCIDevice {
> > > > >      MSIVectorPollNotifier msix_vector_poll_notifier;
> > > > >  };
> > > > >
> > > > > +struct PCIListener {
> > > > > +    void (*device_add)(PCIListener *listener, PCIDevice *pci_dev);
> > > > > +    void (*device_del)(PCIListener *listener, PCIDevice *pci_dev);
> > > > > +    QTAILQ_ENTRY(PCIListener) link;
> > > > > +};
> > > > > +
> > > > > +void pci_listener_register(PCIListener *listener);
> > > > > +void pci_listener_unregister(PCIListener *listener);
> > > > > +
> > > > >  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> > > > >                        uint8_t attr, MemoryRegion *memory);
> > > > >  void pci_register_vga(PCIDevice *pci_dev, MemoryRegion *mem,
> > > > > diff --git a/include/qemu/typedefs.h b/include/qemu/typedefs.h
> > > > > index 04df51b..2b974c6 100644
> > > > > --- a/include/qemu/typedefs.h
> > > > > +++ b/include/qemu/typedefs.h
> > > > > @@ -54,6 +54,7 @@ typedef struct PCIHostState PCIHostState;
> > > > >  typedef struct PCIExpressHost PCIExpressHost;
> > > > >  typedef struct PCIBus PCIBus;
> > > > >  typedef struct PCIDevice PCIDevice;
> > > > > +typedef struct PCIListener PCIListener;
> > > > >  typedef struct PCIExpressDevice PCIExpressDevice;
> > > > >  typedef struct PCIBridge PCIBridge;
> > > > >  typedef struct PCIEAERMsg PCIEAERMsg;
> > > > > --
> > > > > 1.7.10.4

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