| Message ID | 1413204767-39317-2-git-send-email-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
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@citrix.com> > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Andreas Faerber <afaerber@suse.de> > Cc: Thomas Huth <thuth@linux.vnet.ibm.com> > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > Cc: Christian Borntraeger <borntraeger@de.ibm.com> Do you really need multiple notifiers? 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. > --- > 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
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 14 October 2014 10:54 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> > > Cc: Michael S. Tsirkin <mst@redhat.com> > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > Cc: Andreas Faerber <afaerber@suse.de> > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com> > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > > 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? > 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. 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 > > > > --- > > 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
On Tue, Oct 14, 2014 at 10:08:06AM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 14 October 2014 10:54 > > To: Paul Durrant > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > Cc: Andreas Faerber <afaerber@suse.de> > > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com> > > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > > > > 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? > > > 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. > 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? > > > > > > > --- > > > 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
> -----Original Message----- > From: Michael S. Tsirkin [mailto:mst@redhat.com] > Sent: 14 October 2014 12:27 > To: Paul Durrant > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@redhat.com] > > > Sent: 14 October 2014 10:54 > > > To: Paul Durrant > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > Cc: Andreas Faerber <afaerber@suse.de> > > > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com> > > > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > > > > > > 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. > > > > > 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
On Tue, Oct 14, 2014 at 12:44:06PM +0000, Paul Durrant wrote: > > -----Original Message----- > > From: Michael S. Tsirkin [mailto:mst@redhat.com] > > Sent: 14 October 2014 12:27 > > To: Paul Durrant > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@redhat.com] > > > > Sent: 14 October 2014 10:54 > > > > To: Paul Durrant > > > > Cc: qemu-devel@nongnu.org; xen-devel@lists.xenproject.org; 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@citrix.com> > > > > > Cc: Michael S. Tsirkin <mst@redhat.com> > > > > > Cc: Paolo Bonzini <pbonzini@redhat.com> > > > > > Cc: Andreas Faerber <afaerber@suse.de> > > > > > Cc: Thomas Huth <thuth@linux.vnet.ibm.com> > > > > > Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> > > > > > Cc: Christian Borntraeger <borntraeger@de.ibm.com> > > > > > > > > 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
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;
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@citrix.com> Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Andreas Faerber <afaerber@suse.de> Cc: Thomas Huth <thuth@linux.vnet.ibm.com> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com> Cc: Christian Borntraeger <borntraeger@de.ibm.com> --- hw/pci/pci.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++ include/hw/pci/pci.h | 9 +++++++ include/qemu/typedefs.h | 1 + 3 files changed, 75 insertions(+)