Re: [Qemu-devel] [RFC v10 13/19] pci: add bus reset_notifiers callbacks for host bus reset

[Alex Williamson]

On Wed, 2015-06-17 at 08:47 +0200, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 09:41:40AM +0800, Chen Fan wrote:
> > 
> > On 06/16/2015 06:20 PM, Michael S. Tsirkin wrote:
> > >On Tue, Jun 16, 2015 at 04:10:57PM +0800, Chen Fan wrote:
> > >>Particularly, For vfio devices, Once need to recovery devices
> > >>by bus reset such as AER, we always need to reset the host bus
> > >>to recovery the devices under the bus, so we need to add pci bus
> > >>callbacks to specify to do host bus reset.
> > >>
> > >>Signed-off-by: Chen Fan <address@hidden>
> > >Interesting. What prevents guest from triggering reset at an arbitrary
> > >time, killing all VFs for all guests?
> > If the VF device was assigned to VM, with enable aer, we check the
> > VF affected devices e.g. the other VFs and PF is belonged to VM or not.
> > so It can't to affect other VMs for this. if disable aer, there will no
> > affection.
> > 
> > Thanks,
> > Chen
> 
> So fundamentally this assumes PF is assigned to the VM?
> Why even bother with this aer on VFs then?
> Do this for PFs only.

At QEMU, we don't know or care whether we're dealing with a PF or VF and
I don't see this as an opportunity to add that distinction.  The issue
is simply that we can't do a bus reset on a VF, but that's also true of
PFs that reside on the host root complex.  The vfio-pci bus reset
interface handles this, it tells us when we can and can't do a bus reset
and lists the affected devices when we can.  A VF has no parent bridge
on which to perform a secondary bus reset, just like a PF on the root
complex.  So we can just let the devices fall out there without caring
that they're VFs.  Neither the QEMU PCI level nor the guest (other than
device IDs) really knows that the device is a VF either.  Thanks,

Alex

> > >>---
> > >>  hw/pci/pci.c             | 16 ++++++++++++++++
> > >>  hw/pci/pci_bridge.c      |  6 ++++++
> > >>  include/hw/pci/pci.h     |  4 ++++
> > >>  include/hw/pci/pci_bus.h |  2 ++
> > >>  4 files changed, 28 insertions(+)
> > >>
> > >>diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > >>index 3423c3a..3bd954e 100644
> > >>--- a/hw/pci/pci.c
> > >>+++ b/hw/pci/pci.c
> > >>@@ -74,11 +74,27 @@ static const VMStateDescription vmstate_pcibus = {
> > >>      }
> > >>  };
> > >>+void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify)
> > >>+{
> > >>+    notifier_list_add(&bus->reset_notifiers, notify);
> > >>+}
> > >>+
> > >>+void pci_bus_remove_reset_notifier(Notifier *notify)
> > >>+{
> > >>+    notifier_remove(notify);
> > >>+}
> > >>+
> > >>+void pci_bus_run_reset_notifier(PCIBus *bus, void *opaque)
> > >>+{
> > >>+    notifier_list_notify(&bus->reset_notifiers, opaque);
> > >>+}
> > >>+
> > >>  static void pci_bus_realize(BusState *qbus, Error **errp)
> > >>  {
> > >>      PCIBus *bus = PCI_BUS(qbus);
> > >>      vmstate_register(NULL, -1, &vmstate_pcibus, bus);
> > >>+    notifier_list_init(&bus->reset_notifiers);
> > >>  }
> > >>  static void pci_bus_unrealize(BusState *qbus, Error **errp)
> > >>diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> > >>index 40c97b1..a8e3f57 100644
> > >>--- a/hw/pci/pci_bridge.c
> > >>+++ b/hw/pci/pci_bridge.c
> > >>@@ -267,6 +267,12 @@ void pci_bridge_write_config(PCIDevice *d,
> > >>      newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL);
> > >>      if (~oldctl & newctl & PCI_BRIDGE_CTL_BUS_RESET) {
> > >>+        /*
> > >>+         * Notify all vfio-pci devices under the bus
> > >>+         * to do physical bus reset.
> > >>+         */
> > >>+        pci_for_each_bus(&s->sec_bus,
> > >>+                pci_bus_run_reset_notifier, d);
> > >>          /* Trigger hot reset on 0->1 transition. */
> > >>          qbus_reset_all(&s->sec_bus.qbus);
> > >>      }
> > >>diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> > >>index 6c2af0d..d353c9d 100644
> > >>--- a/include/hw/pci/pci.h
> > >>+++ b/include/hw/pci/pci.h
> > >>@@ -7,6 +7,7 @@
> > >>  #include "exec/memory.h"
> > >>  #include "sysemu/dma.h"
> > >>  #include "qapi/error.h"
> > >>+#include "qemu/notify.h"
> > >>  /* PCI includes legacy ISA access.  */
> > >>  #include "hw/isa/isa.h"
> > >>@@ -377,6 +378,9 @@ void pci_bus_fire_intx_routing_notifier(PCIBus *bus);
> > >>  void pci_device_set_intx_routing_notifier(PCIDevice *dev,
> > >>                                            PCIINTxRoutingNotifier 
> > >> notifier);
> > >>  void pci_device_reset(PCIDevice *dev);
> > >>+void pci_bus_add_reset_notifier(PCIBus *bus, Notifier *notify);
> > >>+void pci_bus_remove_reset_notifier(Notifier *notify);
> > >>+void pci_bus_run_reset_notifier(PCIBus *bus, void *opaque);
> > >>  PCIDevice *pci_nic_init_nofail(NICInfo *nd, PCIBus *rootbus,
> > >>                                 const char *default_model,
> > >>diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
> > >>index fabaeee..3b551d7 100644
> > >>--- a/include/hw/pci/pci_bus.h
> > >>+++ b/include/hw/pci/pci_bus.h
> > >>@@ -29,6 +29,8 @@ struct PCIBus {
> > >>         Keep a count of the number of devices with raised IRQs.  */
> > >>      int nirq;
> > >>      int *irq_count;
> > >>+
> > >>+    NotifierList reset_notifiers;
> > >>  };
> > >>  typedef struct PCIBridgeWindows PCIBridgeWindows;
> > >>-- 
> > >>1.9.3
> > >>
> > >.
> > >