[Qemu-devel] [RFC 00/12] Merge EEH support into spapr-pci-host-bridge

[Qemu-devel] [RFC 00/12] Merge EEH support into spapr-pci-host-bridge

[David Gibson]

This series of patches (2 to vfio, the remainder to the sPAPR specific
PCI code) improve the interfaces for passing through EEH (IBM's
Enhanced Error Handling facility) from guest PCI devices to host PCI
devices.

This allows EEH to be used on the normal spapr-pci-host-bridge device,
removing any remaining need for the special spapr-vfio-pci-host-bridge
device.

The series has some similarities to my earlier series merging EEH
support into the plain host bridge, but is substantially reworked to
remove the serious conceptual error I made then.

For now, this only allows EEH when (i) there is a single VFIO
container attached to the vPHB, and (ii) there is only a single VFIO
group attached to that container.

(ii) is due to a kernel limitation - at present it will allow EEH
calls on a container with multiple groups, but they won't work
properly, because there is no code to make sure the EEH state on the
host is synchronized between the groups.

(i) could be addressed in future in one of two ways, either perform
similar synchronization in qemu between the container states, or
present each container as a separate PAPR Partitionable Endpoint to
the guest and allow separate EEH state on each.  The second approach
is more complicated and may not work well with some PAPR addressing
conventions, but does allow more flexibility in the guest.

I hope to have this ready for merge in qemu-2.6.

David Gibson (12):
  vfio: Start improving VFIO/EEH interface
  spapr_pci: Switch to vfio_eeh_as_op() interface
  spapr_pci: Eliminate class callbacks
  spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_reset() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_get_state() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_eeh_set_option() into spapr_pci code
  spapr_pci: Fold spapr_phb_vfio_reset() into spapr_pci code
  spapr_pci: Allow EEH on spapr-pci-host-bridge
  spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge
  spapr_pci: Remove finish_realize hook
  vfio: Eliminate vfio_container_ioctl()

 hw/ppc/spapr_pci.c          | 203 +++++++++++++++++++++++++++---------
 hw/ppc/spapr_pci_vfio.c     | 245 +++-----------------------------------------
 hw/vfio/common.c            |  94 +++++++++++------
 include/hw/pci-host/spapr.h |  28 +----
 include/hw/vfio/vfio.h      |   4 +-
 5 files changed, 235 insertions(+), 339 deletions(-)

-- 
2.5.0

[Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[David Gibson]

At present the code handling IBM's Enhanced Error Handling (EEH) interface
on VFIO devices operates by bypassing the usual VFIO logic with
vfio_container_ioctl().  That's a poorly designed interface with unclear
semantics about exactly what can be operated on.

In particular it operates on a single vfio container internally (hence the
name), but takes an address space and group id, from which it deduces the
container in a rather roundabout way.  groupids are something that code
outside vfio shouldn't even be aware of.

This patch creates new interfaces for EEH operations.  Internally we
have vfio_eeh_container_op() which takes a VFIOContainer object
directly.  For external use we have vfio_eeh_as_ok() which determines
if an AddressSpace is usable for EEH (at present this means it has a
single container and at most a single group attached), and
vfio_eeh_as_op() which will perform an operation on an AddressSpace in
the unambiguous case, and otherwise returns an error.

This interface still isn't great, but it's enough of an improvement to
allow a number of cleanups in other places.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/vfio/vfio.h |  2 ++
 2 files changed, 79 insertions(+)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 6797208..4733625 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
 
     return vfio_container_do_ioctl(as, groupid, req, param);
 }
+
+/*
+ * Interfaces for IBM EEH (Enhanced Error Handling)
+ */
+static bool vfio_eeh_container_ok(VFIOContainer *container)
+{
+    /* A broken kernel implementation means EEH operations won't work
+     * correctly if there are multiple groups in a container */
+
+    if (!QLIST_EMPTY(&container->group_list)
+        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
+        return false;
+    }
+
+    return true;
+}
+
+static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op)
+{
+    struct vfio_eeh_pe_op pe_op = {
+        .argsz = sizeof(pe_op),
+        .op = op,
+    };
+    int ret;
+
+    if (!vfio_eeh_container_ok(container)) {
+        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
+                     " with multiple groups", op);
+        return -EPERM;
+    }
+
+    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
+    if (ret < 0) {
+        error_report("vfio/eeh: EEH_PE_OP 0x%x failed: %m", op);
+        return -errno;
+    }
+
+    return 0;
+}
+
+static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
+{
+    VFIOAddressSpace *space = vfio_get_address_space(as);
+    VFIOContainer *container = NULL;
+
+    if (QLIST_EMPTY(&space->containers)) {
+        /* No containers to act on */
+        goto out;
+    }
+
+    container = QLIST_FIRST(&space->containers);
+
+    if (QLIST_NEXT(container, next)) {
+        /* We don't yet have logic to synchronize EEH state across
+         * multiple containers */
+        container = NULL;
+        goto out;
+    }
+
+out:
+    vfio_put_address_space(space);
+    return container;
+}
+
+bool vfio_eeh_as_ok(AddressSpace *as)
+{
+    VFIOContainer *container = vfio_eeh_as_container(as);
+
+    return (container != NULL) && vfio_eeh_container_ok(container);
+}
+
+int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
+{
+    VFIOContainer *container = vfio_eeh_as_container(as);
+
+    return vfio_eeh_container_op(container, op);
+}
diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
index 0b26cd8..fd3933b 100644
--- a/include/hw/vfio/vfio.h
+++ b/include/hw/vfio/vfio.h
@@ -5,5 +5,7 @@
 
 extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
                                 int req, void *param);
+bool vfio_eeh_as_ok(AddressSpace *as);
+int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
 
 #endif
-- 
2.5.0

[Qemu-devel] [RFC 02/12] spapr_pci: Switch to vfio_eeh_as_op() interface

[David Gibson]

This switches all EEH on VFIO operations in spapr_pci_vfio.c from the
broken vfio_container_ioctl() interface to the new vfio_as_eeh_op()
interface.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>mak
---
 hw/ppc/spapr_pci_vfio.c | 50 ++++++++++++++++---------------------------------
 1 file changed, 16 insertions(+), 34 deletions(-)

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index a61b418..bfbeae9 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -72,15 +72,9 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
                                 spapr_tce_get_iommu(tcet));
 }
 
-static void spapr_phb_vfio_eeh_reenable(sPAPRPHBVFIOState *svphb)
+static void spapr_phb_vfio_eeh_reenable(sPAPRPHBState *sphb)
 {
-    struct vfio_eeh_pe_op op = {
-        .argsz = sizeof(op),
-        .op    = VFIO_EEH_PE_ENABLE
-    };
-
-    vfio_container_ioctl(&svphb->phb.iommu_as,
-                         svphb->iommugroupid, VFIO_EEH_PE_OP, &op);
+    vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_ENABLE);
 }
 
 static void spapr_phb_vfio_reset(DeviceState *qdev)
@@ -91,19 +85,18 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
      * ensures that the contained PCI devices will work properly
      * after reboot.
      */
-    spapr_phb_vfio_eeh_reenable(SPAPR_PCI_VFIO_HOST_BRIDGE(qdev));
+    spapr_phb_vfio_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
 }
 
 static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
                                          unsigned int addr, int option)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    uint32_t op;
     int ret;
 
     switch (option) {
     case RTAS_EEH_DISABLE:
-        op.op = VFIO_EEH_PE_DISABLE;
+        op = VFIO_EEH_PE_DISABLE;
         break;
     case RTAS_EEH_ENABLE: {
         PCIHostState *phb;
@@ -121,21 +114,20 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
             return RTAS_OUT_PARAM_ERROR;
         }
 
-        op.op = VFIO_EEH_PE_ENABLE;
+        op = VFIO_EEH_PE_ENABLE;
         break;
     }
     case RTAS_EEH_THAW_IO:
-        op.op = VFIO_EEH_PE_UNFREEZE_IO;
+        op = VFIO_EEH_PE_UNFREEZE_IO;
         break;
     case RTAS_EEH_THAW_DMA:
-        op.op = VFIO_EEH_PE_UNFREEZE_DMA;
+        op = VFIO_EEH_PE_UNFREEZE_DMA;
         break;
     default:
         return RTAS_OUT_PARAM_ERROR;
     }
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
     if (ret < 0) {
         return RTAS_OUT_HW_ERROR;
     }
@@ -145,13 +137,9 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
 
 static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
     int ret;
 
-    op.op = VFIO_EEH_PE_GET_STATE;
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_GET_STATE);
     if (ret < 0) {
         return RTAS_OUT_PARAM_ERROR;
     }
@@ -205,28 +193,26 @@ static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
 
 static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
+    uint32_t op;
     int ret;
 
     switch (option) {
     case RTAS_SLOT_RESET_DEACTIVATE:
-        op.op = VFIO_EEH_PE_RESET_DEACTIVATE;
+        op = VFIO_EEH_PE_RESET_DEACTIVATE;
         break;
     case RTAS_SLOT_RESET_HOT:
         spapr_phb_vfio_eeh_pre_reset(sphb);
-        op.op = VFIO_EEH_PE_RESET_HOT;
+        op = VFIO_EEH_PE_RESET_HOT;
         break;
     case RTAS_SLOT_RESET_FUNDAMENTAL:
         spapr_phb_vfio_eeh_pre_reset(sphb);
-        op.op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
+        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
         break;
     default:
         return RTAS_OUT_PARAM_ERROR;
     }
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
     if (ret < 0) {
         return RTAS_OUT_HW_ERROR;
     }
@@ -236,13 +222,9 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
 
 static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
 {
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_eeh_pe_op op = { .argsz = sizeof(op) };
     int ret;
 
-    op.op = VFIO_EEH_PE_CONFIGURE;
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_EEH_PE_OP, &op);
+    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_CONFIGURE);
     if (ret < 0) {
         return RTAS_OUT_PARAM_ERROR;
     }
-- 
2.5.0

[Qemu-devel] [RFC 03/12] spapr_pci: Eliminate class callbacks

[David Gibson]

The EEH operations in the spapr-vfio-pci-host-bridge no longer rely on the
special groupid field in sPAPRPHBVFIOState.  So we can simplify, removing
the class specific callbacks with direct calls based on a simple
spapr_phb_eeh_enabled() helper.  For now we implement that in terms of
a boolean in the class, but we'll continue to clean that up later.

On its own this is a rather strange way of doing things, but it's a useful
intermediate step to further cleanups.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 44 ++++++++++++++++++++++----------------------
 hw/ppc/spapr_pci_vfio.c     | 18 +++++++-----------
 include/hw/pci-host/spapr.h | 13 +++++++++----
 3 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 55fa8db..9203d15 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -91,6 +91,13 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
     return pci_find_device(phb->bus, bus_num, devfn);
 }
 
+static bool spapr_phb_eeh_available(sPAPRPHBState *sphb)
+{
+    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
+
+    return spc->eeh_available;
+}
+
 static uint32_t rtas_pci_cfgaddr(uint32_t arg)
 {
     /* This handles the encoding of extended config space addresses */
@@ -430,7 +437,6 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
                                     target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     uint32_t addr, option;
     uint64_t buid;
     int ret;
@@ -448,12 +454,11 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
-    ret = spc->eeh_set_option(sphb, addr, option);
+    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);
     rtas_st(rets, 0, ret);
     return;
 
@@ -468,7 +473,6 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
                                            target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     PCIDevice *pdev;
     uint32_t addr, option;
     uint64_t buid;
@@ -483,8 +487,7 @@ static void rtas_ibm_get_config_addr_info2(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
@@ -524,7 +527,6 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
                                             target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     uint64_t buid;
     int state, ret;
 
@@ -538,12 +540,11 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_get_state) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
-    ret = spc->eeh_get_state(sphb, &state);
+    ret = spapr_phb_vfio_eeh_get_state(sphb, &state);
     rtas_st(rets, 0, ret);
     if (ret != RTAS_OUT_SUCCESS) {
         return;
@@ -568,7 +569,6 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
                                     target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     uint32_t option;
     uint64_t buid;
     int ret;
@@ -584,12 +584,11 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_reset) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
-    ret = spc->eeh_reset(sphb, option);
+    ret = spapr_phb_vfio_eeh_reset(sphb, option);
     rtas_st(rets, 0, ret);
     return;
 
@@ -604,7 +603,6 @@ static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
                                   target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     uint64_t buid;
     int ret;
 
@@ -618,12 +616,11 @@ static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_configure) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
-    ret = spc->eeh_configure(sphb);
+    ret = spapr_phb_vfio_eeh_configure(sphb);
     rtas_st(rets, 0, ret);
     return;
 
@@ -639,7 +636,6 @@ static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
                                        target_ulong rets)
 {
     sPAPRPHBState *sphb;
-    sPAPRPHBClass *spc;
     int option;
     uint64_t buid;
 
@@ -653,8 +649,7 @@ static void rtas_ibm_slot_error_detail(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-    if (!spc->eeh_set_option) {
+    if (!spapr_phb_eeh_available(sphb)) {
         goto param_error_exit;
     }
 
@@ -1422,6 +1417,10 @@ static void spapr_phb_reset(DeviceState *qdev)
 {
     /* Reset the IOMMU state */
     object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
+
+    if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
+        spapr_phb_vfio_reset(qdev);
+    }
 }
 
 static Property spapr_phb_properties[] = {
@@ -1552,6 +1551,7 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->cannot_instantiate_with_device_add_yet = false;
     spc->finish_realize = spapr_phb_finish_realize;
+    spc->eeh_available = false;
     hp->plug = spapr_phb_hot_plug_child;
     hp->unplug = spapr_phb_hot_unplug_child;
 }
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index bfbeae9..abb8ac0 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -77,7 +77,7 @@ static void spapr_phb_vfio_eeh_reenable(sPAPRPHBState *sphb)
     vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_ENABLE);
 }
 
-static void spapr_phb_vfio_reset(DeviceState *qdev)
+void spapr_phb_vfio_reset(DeviceState *qdev)
 {
     /*
      * The PE might be in frozen state. To reenable the EEH
@@ -88,8 +88,8 @@ static void spapr_phb_vfio_reset(DeviceState *qdev)
     spapr_phb_vfio_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
 }
 
-static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
-                                         unsigned int addr, int option)
+int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
+                                  unsigned int addr, int option)
 {
     uint32_t op;
     int ret;
@@ -135,7 +135,7 @@ static int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
     return RTAS_OUT_SUCCESS;
 }
 
-static int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
+int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
 {
     int ret;
 
@@ -191,7 +191,7 @@ static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
        pci_for_each_bus(phb->bus, spapr_phb_vfio_eeh_clear_bus_msix, NULL);
 }
 
-static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
+int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
 {
     uint32_t op;
     int ret;
@@ -220,7 +220,7 @@ static int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
     return RTAS_OUT_SUCCESS;
 }
 
-static int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
+int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
 {
     int ret;
 
@@ -238,12 +238,8 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
     sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
 
     dc->props = spapr_phb_vfio_properties;
-    dc->reset = spapr_phb_vfio_reset;
     spc->finish_realize = spapr_phb_vfio_finish_realize;
-    spc->eeh_set_option = spapr_phb_vfio_eeh_set_option;
-    spc->eeh_get_state = spapr_phb_vfio_eeh_get_state;
-    spc->eeh_reset = spapr_phb_vfio_eeh_reset;
-    spc->eeh_configure = spapr_phb_vfio_eeh_configure;
+    spc->eeh_available = true;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7de5e02..cc1b82c 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -49,10 +49,7 @@ struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
-    int (*eeh_set_option)(sPAPRPHBState *sphb, unsigned int addr, int option);
-    int (*eeh_get_state)(sPAPRPHBState *sphb, int *state);
-    int (*eeh_reset)(sPAPRPHBState *sphb, int option);
-    int (*eeh_configure)(sPAPRPHBState *sphb);
+    bool eeh_available;
 };
 
 typedef struct spapr_pci_msi {
@@ -136,5 +133,13 @@ void spapr_pci_rtas_init(void);
 sPAPRPHBState *spapr_pci_find_phb(sPAPRMachineState *spapr, uint64_t buid);
 PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
                               uint32_t config_addr);
+void spapr_phb_vfio_reset(DeviceState *qdev);
+
+/* VFIO EEH hooks */
+int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
+                                  unsigned int addr, int option);
+int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state);
+int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option);
+int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb);
 
 #endif /* __HW_SPAPR_PCI_H__ */
-- 
2.5.0

[Qemu-devel] [RFC 04/12] spapr_pci: Fold spapr_phb_vfio_eeh_configure() into spapr_pci code

[David Gibson]

Simplify the sPAPR PCI code by folding spapr_phb_vfio_eeh_configure()
into rtas_ibm_configure_pe().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 11 +++++++++--
 hw/ppc/spapr_pci_vfio.c     | 12 ------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 9203d15..d564351 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -41,6 +41,9 @@
 #include "hw/ppc/spapr_drc.h"
 #include "sysemu/device_tree.h"
 
+#include "hw/vfio/vfio.h"
+#include <linux/vfio.h>
+
 /* Copied from the kernel arch/powerpc/platforms/pseries/msi.c */
 #define RTAS_QUERY_FN           0
 #define RTAS_CHANGE_FN          1
@@ -620,8 +623,12 @@ static void rtas_ibm_configure_pe(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    ret = spapr_phb_vfio_eeh_configure(sphb);
-    rtas_st(rets, 0, ret);
+    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_CONFIGURE);
+    if (ret < 0) {
+        goto param_error_exit;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index abb8ac0..4408087 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -220,18 +220,6 @@ int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
     return RTAS_OUT_SUCCESS;
 }
 
-int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb)
-{
-    int ret;
-
-    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_CONFIGURE);
-    if (ret < 0) {
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index cc1b82c..f02ef51 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -140,6 +140,5 @@ int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
                                   unsigned int addr, int option);
 int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state);
 int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option);
-int spapr_phb_vfio_eeh_configure(sPAPRPHBState *sphb);
 
 #endif /* __HW_SPAPR_PCI_H__ */
-- 
2.5.0

[Qemu-devel] [RFC 05/12] spapr_pci: Fold spapr_phb_vfio_eeh_reset() into spapr_pci code

[David Gibson]

Simplify the sPAPR PCI code by folding spapr_phb_vfio_eeh_reset() into
rtas_ibm_set_slot_reset().  We move several functions of which it was
the only caller (spapr_phb_eeh_clear_dev_msix(),
spapr_phb_eeh_clear_bus_msix() and spapr_phb_eeh_pre_reset()) into
spapr_pci.c along with it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 68 ++++++++++++++++++++++++++++++++++++++++--
 hw/ppc/spapr_pci_vfio.c     | 72 ---------------------------------------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 66 insertions(+), 75 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index d564351..76214ec 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -565,6 +565,48 @@ param_error_exit:
     rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
 }
 
+static void spapr_phb_eeh_clear_dev_msix(PCIBus *bus, PCIDevice *pdev,
+                                         void *opaque)
+{
+    /* Check if the device is VFIO PCI device */
+    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+        return;
+    }
+
+    /*
+     * The MSIx table will be cleaned out by reset. We need
+     * disable it so that it can be reenabled properly. Also,
+     * the cached MSIx table should be cleared as it's not
+     * reflecting the contents in hardware.
+     */
+    if (msix_enabled(pdev)) {
+        uint16_t flags;
+
+        flags = pci_host_config_read_common(pdev,
+                                            pdev->msix_cap + PCI_MSIX_FLAGS,
+                                            pci_config_size(pdev), 2);
+        flags &= ~PCI_MSIX_FLAGS_ENABLE;
+        pci_host_config_write_common(pdev,
+                                     pdev->msix_cap + PCI_MSIX_FLAGS,
+                                     pci_config_size(pdev), flags, 2);
+    }
+
+    msix_reset(pdev);
+}
+
+static void spapr_phb_eeh_clear_bus_msix(PCIBus *bus, void *opaque)
+{
+       pci_for_each_device(bus, pci_bus_num(bus),
+                           spapr_phb_eeh_clear_dev_msix, NULL);
+}
+
+static void spapr_phb_eeh_pre_reset(sPAPRPHBState *sphb)
+{
+       PCIHostState *phb = PCI_HOST_BRIDGE(sphb);
+
+       pci_for_each_bus(phb->bus, spapr_phb_eeh_clear_bus_msix, NULL);
+}
+
 static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
                                     sPAPRMachineState *spapr,
                                     uint32_t token, uint32_t nargs,
@@ -574,6 +616,7 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
     sPAPRPHBState *sphb;
     uint32_t option;
     uint64_t buid;
+    uint32_t op;
     int ret;
 
     if ((nargs != 4) || (nret != 1)) {
@@ -591,8 +634,29 @@ static void rtas_ibm_set_slot_reset(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    ret = spapr_phb_vfio_eeh_reset(sphb, option);
-    rtas_st(rets, 0, ret);
+    switch (option) {
+    case RTAS_SLOT_RESET_DEACTIVATE:
+        op = VFIO_EEH_PE_RESET_DEACTIVATE;
+        break;
+    case RTAS_SLOT_RESET_HOT:
+        spapr_phb_eeh_pre_reset(sphb);
+        op = VFIO_EEH_PE_RESET_HOT;
+        break;
+    case RTAS_SLOT_RESET_FUNDAMENTAL:
+        spapr_phb_eeh_pre_reset(sphb);
+        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 4408087..0872dbf 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -148,78 +148,6 @@ int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
     return RTAS_OUT_SUCCESS;
 }
 
-static void spapr_phb_vfio_eeh_clear_dev_msix(PCIBus *bus,
-                                              PCIDevice *pdev,
-                                              void *opaque)
-{
-    /* Check if the device is VFIO PCI device */
-    if (!object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
-        return;
-    }
-
-    /*
-     * The MSIx table will be cleaned out by reset. We need
-     * disable it so that it can be reenabled properly. Also,
-     * the cached MSIx table should be cleared as it's not
-     * reflecting the contents in hardware.
-     */
-    if (msix_enabled(pdev)) {
-        uint16_t flags;
-
-        flags = pci_host_config_read_common(pdev,
-                                            pdev->msix_cap + PCI_MSIX_FLAGS,
-                                            pci_config_size(pdev), 2);
-        flags &= ~PCI_MSIX_FLAGS_ENABLE;
-        pci_host_config_write_common(pdev,
-                                     pdev->msix_cap + PCI_MSIX_FLAGS,
-                                     pci_config_size(pdev), flags, 2);
-    }
-
-    msix_reset(pdev);
-}
-
-static void spapr_phb_vfio_eeh_clear_bus_msix(PCIBus *bus, void *opaque)
-{
-       pci_for_each_device(bus, pci_bus_num(bus),
-                           spapr_phb_vfio_eeh_clear_dev_msix, NULL);
-}
-
-static void spapr_phb_vfio_eeh_pre_reset(sPAPRPHBState *sphb)
-{
-       PCIHostState *phb = PCI_HOST_BRIDGE(sphb);
-
-       pci_for_each_bus(phb->bus, spapr_phb_vfio_eeh_clear_bus_msix, NULL);
-}
-
-int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option)
-{
-    uint32_t op;
-    int ret;
-
-    switch (option) {
-    case RTAS_SLOT_RESET_DEACTIVATE:
-        op = VFIO_EEH_PE_RESET_DEACTIVATE;
-        break;
-    case RTAS_SLOT_RESET_HOT:
-        spapr_phb_vfio_eeh_pre_reset(sphb);
-        op = VFIO_EEH_PE_RESET_HOT;
-        break;
-    case RTAS_SLOT_RESET_FUNDAMENTAL:
-        spapr_phb_vfio_eeh_pre_reset(sphb);
-        op = VFIO_EEH_PE_RESET_FUNDAMENTAL;
-        break;
-    default:
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
-    if (ret < 0) {
-        return RTAS_OUT_HW_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index f02ef51..d31162b 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -139,6 +139,5 @@ void spapr_phb_vfio_reset(DeviceState *qdev);
 int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
                                   unsigned int addr, int option);
 int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state);
-int spapr_phb_vfio_eeh_reset(sPAPRPHBState *sphb, int option);
 
 #endif /* __HW_SPAPR_PCI_H__ */
-- 
2.5.0

[Qemu-devel] [RFC 06/12] spapr_pci: Fold spapr_phb_vfio_eeh_get_state() into spapr_pci code

[David Gibson]

Simplify the sPAPR PCI code by folding spapr_phb_vfio_eeh_get_state9)
into rtas_ibm_read_slot_reset_state2().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 12 ++++++------
 hw/ppc/spapr_pci_vfio.c     | 13 -------------
 include/hw/pci-host/spapr.h |  1 -
 3 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 76214ec..a13eff3 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -531,7 +531,7 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
 {
     sPAPRPHBState *sphb;
     uint64_t buid;
-    int state, ret;
+    int ret;
 
     if ((nargs != 3) || (nret != 4 && nret != 5)) {
         goto param_error_exit;
@@ -547,13 +547,13 @@ static void rtas_ibm_read_slot_reset_state2(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    ret = spapr_phb_vfio_eeh_get_state(sphb, &state);
-    rtas_st(rets, 0, ret);
-    if (ret != RTAS_OUT_SUCCESS) {
-        return;
+    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_GET_STATE);
+    if (ret < 0) {
+        goto param_error_exit;
     }
 
-    rtas_st(rets, 1, state);
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
+    rtas_st(rets, 1, ret);
     rtas_st(rets, 2, RTAS_EEH_SUPPORT);
     rtas_st(rets, 3, RTAS_EEH_PE_UNAVAIL_INFO);
     if (nret >= 5) {
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 0872dbf..8ca9fb3 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -135,19 +135,6 @@ int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
     return RTAS_OUT_SUCCESS;
 }
 
-int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state)
-{
-    int ret;
-
-    ret = vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_GET_STATE);
-    if (ret < 0) {
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    *state = ret;
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index d31162b..55237fc 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -138,6 +138,5 @@ void spapr_phb_vfio_reset(DeviceState *qdev);
 /* VFIO EEH hooks */
 int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
                                   unsigned int addr, int option);
-int spapr_phb_vfio_eeh_get_state(sPAPRPHBState *sphb, int *state);
 
 #endif /* __HW_SPAPR_PCI_H__ */
-- 
2.5.0

[Qemu-devel] [RFC 07/12] spapr_pci: Fold spapr_phb_vfio_eeh_set_option() into spapr_pci code

[David Gibson]

Simplify the sPAPR PCI code by folding spapr_phb_eeh_set_option() into
rtas_ibm_set_eeh_option().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 43 +++++++++++++++++++++++++++++++++++++++--
 hw/ppc/spapr_pci_vfio.c     | 47 ---------------------------------------------
 include/hw/pci-host/spapr.h |  4 ----
 3 files changed, 41 insertions(+), 53 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index a13eff3..d4cae05 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -442,6 +442,7 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
     sPAPRPHBState *sphb;
     uint32_t addr, option;
     uint64_t buid;
+    uint32_t op;
     int ret;
 
     if ((nargs != 4) || (nret != 1)) {
@@ -461,8 +462,46 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
         goto param_error_exit;
     }
 
-    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);
-    rtas_st(rets, 0, ret);
+    switch (option) {
+    case RTAS_EEH_DISABLE:
+        op = VFIO_EEH_PE_DISABLE;
+        break;
+    case RTAS_EEH_ENABLE: {
+        PCIHostState *phb;
+        PCIDevice *pdev;
+
+        /*
+         * The EEH functionality is enabled on basis of PCI device,
+         * instead of PE. We need check the validity of the PCI
+         * device address.
+         */
+        phb = PCI_HOST_BRIDGE(sphb);
+        pdev = pci_find_device(phb->bus,
+                               (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
+        if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
+            goto param_error_exit;
+        }
+
+        op = VFIO_EEH_PE_ENABLE;
+        break;
+    }
+    case RTAS_EEH_THAW_IO:
+        op = VFIO_EEH_PE_UNFREEZE_IO;
+        break;
+    case RTAS_EEH_THAW_DMA:
+        op = VFIO_EEH_PE_UNFREEZE_DMA;
+        break;
+    default:
+        goto param_error_exit;
+    }
+
+    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
+    if (ret < 0) {
+        rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
+        return;
+    }
+
+    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
     return;
 
 param_error_exit:
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 8ca9fb3..9aacf4b 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -88,53 +88,6 @@ void spapr_phb_vfio_reset(DeviceState *qdev)
     spapr_phb_vfio_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
 }
 
-int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
-                                  unsigned int addr, int option)
-{
-    uint32_t op;
-    int ret;
-
-    switch (option) {
-    case RTAS_EEH_DISABLE:
-        op = VFIO_EEH_PE_DISABLE;
-        break;
-    case RTAS_EEH_ENABLE: {
-        PCIHostState *phb;
-        PCIDevice *pdev;
-
-        /*
-         * The EEH functionality is enabled on basis of PCI device,
-         * instead of PE. We need check the validity of the PCI
-         * device address.
-         */
-        phb = PCI_HOST_BRIDGE(sphb);
-        pdev = pci_find_device(phb->bus,
-                               (addr >> 16) & 0xFF, (addr >> 8) & 0xFF);
-        if (!pdev || !object_dynamic_cast(OBJECT(pdev), "vfio-pci")) {
-            return RTAS_OUT_PARAM_ERROR;
-        }
-
-        op = VFIO_EEH_PE_ENABLE;
-        break;
-    }
-    case RTAS_EEH_THAW_IO:
-        op = VFIO_EEH_PE_UNFREEZE_IO;
-        break;
-    case RTAS_EEH_THAW_DMA:
-        op = VFIO_EEH_PE_UNFREEZE_DMA;
-        break;
-    default:
-        return RTAS_OUT_PARAM_ERROR;
-    }
-
-    ret = vfio_eeh_as_op(&sphb->iommu_as, op);
-    if (ret < 0) {
-        return RTAS_OUT_HW_ERROR;
-    }
-
-    return RTAS_OUT_SUCCESS;
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 55237fc..d32750e 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -135,8 +135,4 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
                               uint32_t config_addr);
 void spapr_phb_vfio_reset(DeviceState *qdev);
 
-/* VFIO EEH hooks */
-int spapr_phb_vfio_eeh_set_option(sPAPRPHBState *sphb,
-                                  unsigned int addr, int option);
-
 #endif /* __HW_SPAPR_PCI_H__ */
-- 
2.5.0

[Qemu-devel] [RFC 08/12] spapr_pci: Fold spapr_phb_vfio_reset() into spapr_pci code

[David Gibson]

Simplify the sPAPR PCI code by folding spapr_phb_vfio_reset() into
spapr_phb_reset().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c      | 13 ++++++++++++-
 hw/ppc/spapr_pci_vfio.c | 16 ----------------
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index d4cae05..915f51d 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1523,13 +1523,24 @@ static int spapr_phb_children_reset(Object *child, void *opaque)
     return 0;
 }
 
+static void spapr_phb_eeh_reenable(sPAPRPHBState *sphb)
+{
+    vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_ENABLE);
+}
+
 static void spapr_phb_reset(DeviceState *qdev)
 {
     /* Reset the IOMMU state */
     object_child_foreach(OBJECT(qdev), spapr_phb_children_reset, NULL);
 
     if (spapr_phb_eeh_available(SPAPR_PCI_HOST_BRIDGE(qdev))) {
-        spapr_phb_vfio_reset(qdev);
+        /*
+         * The PE might be in frozen state. To reenable the EEH
+         * functionality on it will clean the frozen state, which
+         * ensures that the contained PCI devices will work properly
+         * after reboot.
+         */
+        spapr_phb_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
     }
 }
 
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 9aacf4b..aca9c46 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -72,22 +72,6 @@ static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
                                 spapr_tce_get_iommu(tcet));
 }
 
-static void spapr_phb_vfio_eeh_reenable(sPAPRPHBState *sphb)
-{
-    vfio_eeh_as_op(&sphb->iommu_as, VFIO_EEH_PE_ENABLE);
-}
-
-void spapr_phb_vfio_reset(DeviceState *qdev)
-{
-    /*
-     * The PE might be in frozen state. To reenable the EEH
-     * functionality on it will clean the frozen state, which
-     * ensures that the contained PCI devices will work properly
-     * after reboot.
-     */
-    spapr_phb_vfio_eeh_reenable(SPAPR_PCI_HOST_BRIDGE(qdev));
-}
-
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-- 
2.5.0

[Qemu-devel] [RFC 09/12] spapr_pci: Allow EEH on spapr-pci-host-bridge

[David Gibson]

Now that the EEH code is independent of the special
spapr-vfio-pci-host-bridge device, we can allow it on all spapr PCI
host bridges instead.  We do this by changing spapr_phb_eeh_available()
to be based on the vfio_eeh_as_ok() call instead of the host bridge class.

Because the value of vfio_eeh_as_ok() can change with devices being
hotplugged or unplugged, this can potentially lead to some strange edge
cases where the guest starts using EEH, then it starts failing because
of a change in status.

However, it's not really any worse than the current situation.  Cases that
would have worked previously will still work (i.e. VFIO devices from at
most one VFIO IOMMU group per vPHB), it's just that it's no longer
necessary to use spapr-vfio-pci-host-bridge with the groupid pre-specified.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 5 +----
 hw/ppc/spapr_pci_vfio.c     | 1 -
 include/hw/pci-host/spapr.h | 1 -
 3 files changed, 1 insertion(+), 6 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 915f51d..38271ef 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -96,9 +96,7 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
 
 static bool spapr_phb_eeh_available(sPAPRPHBState *sphb)
 {
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
-
-    return spc->eeh_available;
+    return vfio_eeh_as_ok(&sphb->iommu_as);
 }
 
 static uint32_t rtas_pci_cfgaddr(uint32_t arg)
@@ -1672,7 +1670,6 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->cannot_instantiate_with_device_add_yet = false;
     spc->finish_realize = spapr_phb_finish_realize;
-    spc->eeh_available = false;
     hp->plug = spapr_phb_hot_plug_child;
     hp->unplug = spapr_phb_hot_unplug_child;
 }
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index aca9c46..1908fc3 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -79,7 +79,6 @@ static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 
     dc->props = spapr_phb_vfio_properties;
     spc->finish_realize = spapr_phb_vfio_finish_realize;
-    spc->eeh_available = true;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index d32750e..9313209 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -49,7 +49,6 @@ struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
 
     void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
-    bool eeh_available;
 };
 
 typedef struct spapr_pci_msi {
-- 
2.5.0

[Qemu-devel] [RFC 10/12] spapr_pci: (Mostly) remove spapr-pci-vfio-host-bridge

[David Gibson]

Now that the regular spapr-pci-host-bridge can handle EEH, there are only
two things that spapr-pci-vfio-host-bridge does differently:
    1. automatically sizes its DMA window to match the host IOMMU
    2. checks if the attached VFIO container is backed by the
       VFIO_SPAPR_TCE_IOMMU type on the host

(1) is not particularly useful, since the default window used by the
regular host bridge will work with the host IOMMU configuration on all
current systems anyway.

Plus, automatically changing guest visible configuration (such as the DMA
window) based on host settings is generally a bad idea.  It's not
definitively broken, since spapr-pci-vfio-host-bridge is only supposed to
support VFIO devices which can't be migrated anyway, but still.

(2) is not really useful, because if a guest tries to configure EEH on a
different host IOMMU, the first call will fail and that will be that.

It's possible there are scripts or tools out there which expect
spapr-pci-vfio-host-bridge, so we don't remove it entirely.  This patch
reduces it to just a stub for backwards compatibility.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci_vfio.c     | 64 ++++++++++++---------------------------------
 include/hw/pci-host/spapr.h | 11 --------
 2 files changed, 17 insertions(+), 58 deletions(-)

diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 1908fc3..aa7ae05 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -19,74 +19,44 @@
 
 #include "hw/ppc/spapr.h"
 #include "hw/pci-host/spapr.h"
-#include "hw/pci/msix.h"
-#include "linux/vfio.h"
-#include "hw/vfio/vfio.h"
+#include "qemu/error-report.h"
 
-static Property spapr_phb_vfio_properties[] = {
-    DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
-    DEFINE_PROP_END_OF_LIST(),
-};
+#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
 
-static void spapr_phb_vfio_finish_realize(sPAPRPHBState *sphb, Error **errp)
-{
-    sPAPRPHBVFIOState *svphb = SPAPR_PCI_VFIO_HOST_BRIDGE(sphb);
-    struct vfio_iommu_spapr_tce_info info = { .argsz = sizeof(info) };
-    int ret;
-    sPAPRTCETable *tcet;
-    uint32_t liobn = svphb->phb.dma_liobn;
+#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
+    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
 
-    if (svphb->iommugroupid == -1) {
-        error_setg(errp, "Wrong IOMMU group ID %d", svphb->iommugroupid);
-        return;
-    }
+typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_CHECK_EXTENSION,
-                               (void *) VFIO_SPAPR_TCE_IOMMU);
-    if (ret != 1) {
-        error_setg_errno(errp, -ret,
-                         "spapr-vfio: SPAPR extension is not supported");
-        return;
-    }
+struct sPAPRPHBVFIOState {
+    sPAPRPHBState phb;
 
-    ret = vfio_container_ioctl(&svphb->phb.iommu_as, svphb->iommugroupid,
-                               VFIO_IOMMU_SPAPR_TCE_GET_INFO, &info);
-    if (ret) {
-        error_setg_errno(errp, -ret,
-                         "spapr-vfio: get info from container failed");
-        return;
-    }
+    int32_t iommugroupid;
+};
 
-    tcet = spapr_tce_new_table(DEVICE(sphb), liobn, info.dma32_window_start,
-                               SPAPR_TCE_PAGE_SHIFT,
-                               info.dma32_window_size >> SPAPR_TCE_PAGE_SHIFT,
-                               true);
-    if (!tcet) {
-        error_setg(errp, "spapr-vfio: failed to create VFIO TCE table");
-        return;
-    }
+static Property spapr_phb_vfio_properties[] = {
+    DEFINE_PROP_INT32("iommu", sPAPRPHBVFIOState, iommugroupid, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
 
-    /* Register default 32bit DMA window */
-    memory_region_add_subregion(&sphb->iommu_root, tcet->bus_offset,
-                                spapr_tce_get_iommu(tcet));
+static void spapr_phb_vfio_instance_init(Object *obj)
+{
+    error_report("spapr-pci-vfio-host-bridge is deprecated");
 }
 
 static void spapr_phb_vfio_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
 
     dc->props = spapr_phb_vfio_properties;
-    spc->finish_realize = spapr_phb_vfio_finish_realize;
 }
 
 static const TypeInfo spapr_phb_vfio_info = {
     .name          = TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE,
     .parent        = TYPE_SPAPR_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBVFIOState),
+    .instance_init = spapr_phb_vfio_instance_init,
     .class_init    = spapr_phb_vfio_class_init,
-    .class_size    = sizeof(sPAPRPHBClass),
 };
 
 static void spapr_pci_vfio_register_types(void)
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 9313209..7110222 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -28,14 +28,10 @@
 #include "hw/ppc/xics.h"
 
 #define TYPE_SPAPR_PCI_HOST_BRIDGE "spapr-pci-host-bridge"
-#define TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE "spapr-pci-vfio-host-bridge"
 
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
 
-#define SPAPR_PCI_VFIO_HOST_BRIDGE(obj) \
-    OBJECT_CHECK(sPAPRPHBVFIOState, (obj), TYPE_SPAPR_PCI_VFIO_HOST_BRIDGE)
-
 #define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
      OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
 #define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
@@ -43,7 +39,6 @@
 
 typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
-typedef struct sPAPRPHBVFIOState sPAPRPHBVFIOState;
 
 struct sPAPRPHBClass {
     PCIHostBridgeClass parent_class;
@@ -90,12 +85,6 @@ struct sPAPRPHBState {
     QLIST_ENTRY(sPAPRPHBState) list;
 };
 
-struct sPAPRPHBVFIOState {
-    sPAPRPHBState phb;
-
-    int32_t iommugroupid;
-};
-
 #define SPAPR_PCI_MAX_INDEX          255
 
 #define SPAPR_PCI_BASE_BUID          0x800000020000000ULL
-- 
2.5.0

[Qemu-devel] [RFC 11/12] spapr_pci: Remove finish_realize hook

[David Gibson]

Now that spapr-pci-vfio-host-bridge is reduced to just a stub, there is
only one implementation of the finish_realize hook in sPAPRPHBClass.  So,
we can fold that implementation into its (single) caller, and remove the
hook.  That's the last thing left in sPAPRPHBClass, so that can go away as
well.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/ppc/spapr_pci.c          | 25 +++++--------------------
 include/hw/pci-host/spapr.h | 12 ------------
 2 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
index 38271ef..1b6a299 100644
--- a/hw/ppc/spapr_pci.c
+++ b/hw/ppc/spapr_pci.c
@@ -1327,11 +1327,12 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
     SysBusDevice *s = SYS_BUS_DEVICE(dev);
     sPAPRPHBState *sphb = SPAPR_PCI_HOST_BRIDGE(s);
     PCIHostState *phb = PCI_HOST_BRIDGE(s);
-    sPAPRPHBClass *info = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(s);
     char *namebuf;
     int i;
     PCIBus *bus;
     uint64_t msi_window_size = 4096;
+    sPAPRTCETable *tcet;
+    uint32_t nb_table;
 
     if (sphb->index != (uint32_t)-1) {
         hwaddr windows_base;
@@ -1481,33 +1482,20 @@ static void spapr_phb_realize(DeviceState *dev, Error **errp)
         }
     }
 
-    if (!info->finish_realize) {
-        error_setg(errp, "finish_realize not defined");
-        return;
-    }
-
-    info->finish_realize(sphb, errp);
-
-    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
-}
-
-static void spapr_phb_finish_realize(sPAPRPHBState *sphb, Error **errp)
-{
-    sPAPRTCETable *tcet;
-    uint32_t nb_table;
-
     nb_table = sphb->dma_win_size >> SPAPR_TCE_PAGE_SHIFT;
     tcet = spapr_tce_new_table(DEVICE(sphb), sphb->dma_liobn,
                                0, SPAPR_TCE_PAGE_SHIFT, nb_table, false);
     if (!tcet) {
         error_setg(errp, "Unable to create TCE table for %s",
                    sphb->dtbusname);
-        return ;
+        return;
     }
 
     /* Register default 32bit DMA window */
     memory_region_add_subregion(&sphb->iommu_root, sphb->dma_win_addr,
                                 spapr_tce_get_iommu(tcet));
+
+    sphb->msi = g_hash_table_new_full(g_int_hash, g_int_equal, g_free, g_free);
 }
 
 static int spapr_phb_children_reset(Object *child, void *opaque)
@@ -1659,7 +1647,6 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
 {
     PCIHostBridgeClass *hc = PCI_HOST_BRIDGE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
-    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_CLASS(klass);
     HotplugHandlerClass *hp = HOTPLUG_HANDLER_CLASS(klass);
 
     hc->root_bus_path = spapr_phb_root_bus_path;
@@ -1669,7 +1656,6 @@ static void spapr_phb_class_init(ObjectClass *klass, void *data)
     dc->vmsd = &vmstate_spapr_pci;
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
     dc->cannot_instantiate_with_device_add_yet = false;
-    spc->finish_realize = spapr_phb_finish_realize;
     hp->plug = spapr_phb_hot_plug_child;
     hp->unplug = spapr_phb_hot_unplug_child;
 }
@@ -1679,7 +1665,6 @@ static const TypeInfo spapr_phb_info = {
     .parent        = TYPE_PCI_HOST_BRIDGE,
     .instance_size = sizeof(sPAPRPHBState),
     .class_init    = spapr_phb_class_init,
-    .class_size    = sizeof(sPAPRPHBClass),
     .interfaces    = (InterfaceInfo[]) {
         { TYPE_HOTPLUG_HANDLER },
         { }
diff --git a/include/hw/pci-host/spapr.h b/include/hw/pci-host/spapr.h
index 7110222..a603feb 100644
--- a/include/hw/pci-host/spapr.h
+++ b/include/hw/pci-host/spapr.h
@@ -32,20 +32,8 @@
 #define SPAPR_PCI_HOST_BRIDGE(obj) \
     OBJECT_CHECK(sPAPRPHBState, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
 
-#define SPAPR_PCI_HOST_BRIDGE_CLASS(klass) \
-     OBJECT_CLASS_CHECK(sPAPRPHBClass, (klass), TYPE_SPAPR_PCI_HOST_BRIDGE)
-#define SPAPR_PCI_HOST_BRIDGE_GET_CLASS(obj) \
-     OBJECT_GET_CLASS(sPAPRPHBClass, (obj), TYPE_SPAPR_PCI_HOST_BRIDGE)
-
-typedef struct sPAPRPHBClass sPAPRPHBClass;
 typedef struct sPAPRPHBState sPAPRPHBState;
 
-struct sPAPRPHBClass {
-    PCIHostBridgeClass parent_class;
-
-    void (*finish_realize)(sPAPRPHBState *sphb, Error **errp);
-};
-
 typedef struct spapr_pci_msi {
     uint32_t first_irq;
     uint32_t num;
-- 
2.5.0

[Qemu-devel] [RFC 12/12] vfio: Eliminate vfio_container_ioctl()

[David Gibson]

vfio_container_ioctl() was a bad interface that bypassed abstraction
boundaries, had semantics that sat uneasily with its name, and was unsafe
in many realistic circumstances.  Now that spapr-pci-vfio-host-bridge has
been folded into spapr-pci-host-bridge, there are no more users, so remove
it.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/vfio/common.c       | 45 ---------------------------------------------
 include/hw/vfio/vfio.h |  2 --
 2 files changed, 47 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 4733625..6645502 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -958,51 +958,6 @@ void vfio_put_base_device(VFIODevice *vbasedev)
     close(vbasedev->fd);
 }
 
-static int vfio_container_do_ioctl(AddressSpace *as, int32_t groupid,
-                                   int req, void *param)
-{
-    VFIOGroup *group;
-    VFIOContainer *container;
-    int ret = -1;
-
-    group = vfio_get_group(groupid, as);
-    if (!group) {
-        error_report("vfio: group %d not registered", groupid);
-        return ret;
-    }
-
-    container = group->container;
-    if (group->container) {
-        ret = ioctl(container->fd, req, param);
-        if (ret < 0) {
-            error_report("vfio: failed to ioctl %d to container: ret=%d, %s",
-                         _IOC_NR(req) - VFIO_BASE, ret, strerror(errno));
-        }
-    }
-
-    vfio_put_group(group);
-
-    return ret;
-}
-
-int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
-                         int req, void *param)
-{
-    /* We allow only certain ioctls to the container */
-    switch (req) {
-    case VFIO_CHECK_EXTENSION:
-    case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
-    case VFIO_EEH_PE_OP:
-        break;
-    default:
-        /* Return an error on unknown requests */
-        error_report("vfio: unsupported ioctl %X", req);
-        return -1;
-    }
-
-    return vfio_container_do_ioctl(as, groupid, req, param);
-}
-
 /*
  * Interfaces for IBM EEH (Enhanced Error Handling)
  */
diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
index fd3933b..7153604 100644
--- a/include/hw/vfio/vfio.h
+++ b/include/hw/vfio/vfio.h
@@ -3,8 +3,6 @@
 
 #include "qemu/typedefs.h"
 
-extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
-                                int req, void *param);
 bool vfio_eeh_as_ok(AddressSpace *as);
 int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
 
-- 
2.5.0

Re: [Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[Alex Williamson]

On Thu, 2015-11-19 at 15:29 +1100, David Gibson wrote:
> At present the code handling IBM's Enhanced Error Handling (EEH) interface
> on VFIO devices operates by bypassing the usual VFIO logic with
> vfio_container_ioctl().  That's a poorly designed interface with unclear
> semantics about exactly what can be operated on.
> 
> In particular it operates on a single vfio container internally (hence the
> name), but takes an address space and group id, from which it deduces the
> container in a rather roundabout way.  groupids are something that code
> outside vfio shouldn't even be aware of.
> 
> This patch creates new interfaces for EEH operations.  Internally we
> have vfio_eeh_container_op() which takes a VFIOContainer object
> directly.  For external use we have vfio_eeh_as_ok() which determines
> if an AddressSpace is usable for EEH (at present this means it has a
> single container and at most a single group attached), and
> vfio_eeh_as_op() which will perform an operation on an AddressSpace in
> the unambiguous case, and otherwise returns an error.
> 
> This interface still isn't great, but it's enough of an improvement to
> allow a number of cleanups in other places.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio.h |  2 ++
>  2 files changed, 79 insertions(+)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 6797208..4733625 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>  
>      return vfio_container_do_ioctl(as, groupid, req, param);
>  }
> +
> +/*
> + * Interfaces for IBM EEH (Enhanced Error Handling)
> + */
> +static bool vfio_eeh_container_ok(VFIOContainer *container)
> +{
> +    /* A broken kernel implementation means EEH operations won't work
> +     * correctly if there are multiple groups in a container */
> +
> +    if (!QLIST_EMPTY(&container->group_list)
> +        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
> +        return false;
> +    }
> +
> +    return true;
> +}

Seems like there are ways to make this a non-eeh specific function,
vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
etc.

> +
> +static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op)
> +{
> +    struct vfio_eeh_pe_op pe_op = {
> +        .argsz = sizeof(pe_op),
> +        .op = op,
> +    };
> +    int ret;
> +
> +    if (!vfio_eeh_container_ok(container)) {
> +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> +                     " with multiple groups", op);
> +        return -EPERM;
> +    }
> +
> +    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
> +    if (ret < 0) {
> +        error_report("vfio/eeh: EEH_PE_OP 0x%x failed: %m", op);
> +        return -errno;
> +    }
> +
> +    return 0;
> +}
> +
> +static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
> +{
> +    VFIOAddressSpace *space = vfio_get_address_space(as);
> +    VFIOContainer *container = NULL;
> +
> +    if (QLIST_EMPTY(&space->containers)) {
> +        /* No containers to act on */
> +        goto out;
> +    }
> +
> +    container = QLIST_FIRST(&space->containers);
> +
> +    if (QLIST_NEXT(container, next)) {
> +        /* We don't yet have logic to synchronize EEH state across
> +         * multiple containers */
> +        container = NULL;
> +        goto out;
> +    }
> +
> +out:
> +    vfio_put_address_space(space);
> +    return container;
> +}


Here too, vfio_container_from_as()

Overall the series looks good to me, nice cleanup both in power and vfio
code.  Thanks,

Alex


> +
> +bool vfio_eeh_as_ok(AddressSpace *as)
> +{
> +    VFIOContainer *container = vfio_eeh_as_container(as);
> +
> +    return (container != NULL) && vfio_eeh_container_ok(container);
> +}
> +
> +int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
> +{
> +    VFIOContainer *container = vfio_eeh_as_container(as);
> +
> +    return vfio_eeh_container_op(container, op);
> +}
> diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
> index 0b26cd8..fd3933b 100644
> --- a/include/hw/vfio/vfio.h
> +++ b/include/hw/vfio/vfio.h
> @@ -5,5 +5,7 @@
>  
>  extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>                                  int req, void *param);
> +bool vfio_eeh_as_ok(AddressSpace *as);
> +int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
>  
>  #endif

Re: [Qemu-devel] [Qemu-ppc] [RFC 03/12] spapr_pci: Eliminate class callbacks

[Alexander Graf]

On 19.11.15 05:29, David Gibson wrote:
> The EEH operations in the spapr-vfio-pci-host-bridge no longer rely on the
> special groupid field in sPAPRPHBVFIOState.  So we can simplify, removing
> the class specific callbacks with direct calls based on a simple
> spapr_phb_eeh_enabled() helper.  For now we implement that in terms of
> a boolean in the class, but we'll continue to clean that up later.
> 
> On its own this is a rather strange way of doing things, but it's a useful
> intermediate step to further cleanups.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  hw/ppc/spapr_pci.c          | 44 ++++++++++++++++++++++----------------------
>  hw/ppc/spapr_pci_vfio.c     | 18 +++++++-----------
>  include/hw/pci-host/spapr.h | 13 +++++++++----
>  3 files changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> index 55fa8db..9203d15 100644
> --- a/hw/ppc/spapr_pci.c
> +++ b/hw/ppc/spapr_pci.c
> @@ -91,6 +91,13 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
>      return pci_find_device(phb->bus, bus_num, devfn);
>  }
>  
> +static bool spapr_phb_eeh_available(sPAPRPHBState *sphb)
> +{
> +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> +
> +    return spc->eeh_available;
> +}
> +
>  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
>  {
>      /* This handles the encoding of extended config space addresses */
> @@ -430,7 +437,6 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>                                      target_ulong rets)
>  {
>      sPAPRPHBState *sphb;
> -    sPAPRPHBClass *spc;
>      uint32_t addr, option;
>      uint64_t buid;
>      int ret;
> @@ -448,12 +454,11 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
>          goto param_error_exit;
>      }
>  
> -    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> -    if (!spc->eeh_set_option) {
> +    if (!spapr_phb_eeh_available(sphb)) {
>          goto param_error_exit;
>      }
>  
> -    ret = spc->eeh_set_option(sphb, addr, option);
> +    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);

First of all, I think the direction you're taking with this series is sound.

However the hunk above would break compilation on systems that don't
have VFIO enabled (such as Windows hosts for example).

The same holds true for the defines you're using later in the series.
Things like VFIO_EEH_PE_GET_STATE are not defined for non-Linux hosts.

I think the best path to get to where we really want to be is to define
qemu internal defines and eeh helpers for emulated phb mode. Make sure
the constants match the VFIO constants and use them interchangibly (with
a big fat comment saying that they are, and a number of ifdefs making
sure they stay identical).


Alex

Re: [Qemu-devel] [Qemu-ppc] [RFC 03/12] spapr_pci: Eliminate class callbacks

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3307 bytes --]

On Tue, Nov 24, 2015 at 10:00:15AM +0100, Alexander Graf wrote:
> 
> 
> On 19.11.15 05:29, David Gibson wrote:
> > The EEH operations in the spapr-vfio-pci-host-bridge no longer rely on the
> > special groupid field in sPAPRPHBVFIOState.  So we can simplify, removing
> > the class specific callbacks with direct calls based on a simple
> > spapr_phb_eeh_enabled() helper.  For now we implement that in terms of
> > a boolean in the class, but we'll continue to clean that up later.
> > 
> > On its own this is a rather strange way of doing things, but it's a useful
> > intermediate step to further cleanups.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/ppc/spapr_pci.c          | 44 ++++++++++++++++++++++----------------------
> >  hw/ppc/spapr_pci_vfio.c     | 18 +++++++-----------
> >  include/hw/pci-host/spapr.h | 13 +++++++++----
> >  3 files changed, 38 insertions(+), 37 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index 55fa8db..9203d15 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -91,6 +91,13 @@ PCIDevice *spapr_pci_find_dev(sPAPRMachineState *spapr, uint64_t buid,
> >      return pci_find_device(phb->bus, bus_num, devfn);
> >  }
> >  
> > +static bool spapr_phb_eeh_available(sPAPRPHBState *sphb)
> > +{
> > +    sPAPRPHBClass *spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> > +
> > +    return spc->eeh_available;
> > +}
> > +
> >  static uint32_t rtas_pci_cfgaddr(uint32_t arg)
> >  {
> >      /* This handles the encoding of extended config space addresses */
> > @@ -430,7 +437,6 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >                                      target_ulong rets)
> >  {
> >      sPAPRPHBState *sphb;
> > -    sPAPRPHBClass *spc;
> >      uint32_t addr, option;
> >      uint64_t buid;
> >      int ret;
> > @@ -448,12 +454,11 @@ static void rtas_ibm_set_eeh_option(PowerPCCPU *cpu,
> >          goto param_error_exit;
> >      }
> >  
> > -    spc = SPAPR_PCI_HOST_BRIDGE_GET_CLASS(sphb);
> > -    if (!spc->eeh_set_option) {
> > +    if (!spapr_phb_eeh_available(sphb)) {
> >          goto param_error_exit;
> >      }
> >  
> > -    ret = spc->eeh_set_option(sphb, addr, option);
> > +    ret = spapr_phb_vfio_eeh_set_option(sphb, addr, option);
> 
> First of all, I think the direction you're taking with this series is sound.
> 
> However the hunk above would break compilation on systems that don't
> have VFIO enabled (such as Windows hosts for example).

Ah drat, good point.

> The same holds true for the defines you're using later in the series.
> Things like VFIO_EEH_PE_GET_STATE are not defined for non-Linux hosts.
> 
> I think the best path to get to where we really want to be is to define
> qemu internal defines and eeh helpers for emulated phb mode. Make sure
> the constants match the VFIO constants and use them interchangibly (with
> a big fat comment saying that they are, and a number of ifdefs making
> sure they stay identical).

I'll see what I can do.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6382 bytes --]

On Mon, Nov 23, 2015 at 02:58:11PM -0700, Alex Williamson wrote:
> On Thu, 2015-11-19 at 15:29 +1100, David Gibson wrote:
> > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > on VFIO devices operates by bypassing the usual VFIO logic with
> > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > semantics about exactly what can be operated on.
> > 
> > In particular it operates on a single vfio container internally (hence the
> > name), but takes an address space and group id, from which it deduces the
> > container in a rather roundabout way.  groupids are something that code
> > outside vfio shouldn't even be aware of.
> > 
> > This patch creates new interfaces for EEH operations.  Internally we
> > have vfio_eeh_container_op() which takes a VFIOContainer object
> > directly.  For external use we have vfio_eeh_as_ok() which determines
> > if an AddressSpace is usable for EEH (at present this means it has a
> > single container and at most a single group attached), and
> > vfio_eeh_as_op() which will perform an operation on an AddressSpace in
> > the unambiguous case, and otherwise returns an error.
> > 
> > This interface still isn't great, but it's enough of an improvement to
> > allow a number of cleanups in other places.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  include/hw/vfio/vfio.h |  2 ++
> >  2 files changed, 79 insertions(+)
> > 
> > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > index 6797208..4733625 100644
> > --- a/hw/vfio/common.c
> > +++ b/hw/vfio/common.c
> > @@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >  
> >      return vfio_container_do_ioctl(as, groupid, req, param);
> >  }
> > +
> > +/*
> > + * Interfaces for IBM EEH (Enhanced Error Handling)
> > + */
> > +static bool vfio_eeh_container_ok(VFIOContainer *container)
> > +{
> > +    /* A broken kernel implementation means EEH operations won't work
> > +     * correctly if there are multiple groups in a container */
> > +
> > +    if (!QLIST_EMPTY(&container->group_list)
> > +        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
> > +        return false;
> > +    }
> > +
> > +    return true;
> > +}
> 
> Seems like there are ways to make this a non-eeh specific function,
> vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
> etc.

I guess, but I don't know of anything else that needs to know, so is
there a point?

Actually.. I could do with a another opinion here: so, logically EEH
operations should be possible on a container basis - the kernel
interface correctly reflects that (my previous comments that the
interface was broken were mistaken).

The current kernel implementation *is* broken (and is non-trivial to
fix) which is what this test is about.  But is checking for a probably
broken kernel state something that we ought to be checking for in
qemu?  As it stands when the kernel is fixed we'll need a new
capability so that qemu can know to disable this test.

Should we instead just proceed with any container and just advise
people not to attach multiple groups until the kernel is fixed?

A relevant point here might be that while I haven't implemented it so
far, I think it will be possible to workaround the broken kernel with
full functionality by forcing each group into a separate container and
using one of a couple of possible different methods to handle EEH
functionality across multiple containers on a vPHB.

> > +
> > +static int vfio_eeh_container_op(VFIOContainer *container, uint32_t op)
> > +{
> > +    struct vfio_eeh_pe_op pe_op = {
> > +        .argsz = sizeof(pe_op),
> > +        .op = op,
> > +    };
> > +    int ret;
> > +
> > +    if (!vfio_eeh_container_ok(container)) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x called on container"
> > +                     " with multiple groups", op);
> > +        return -EPERM;
> > +    }
> > +
> > +    ret = ioctl(container->fd, VFIO_EEH_PE_OP, &pe_op);
> > +    if (ret < 0) {
> > +        error_report("vfio/eeh: EEH_PE_OP 0x%x failed: %m", op);
> > +        return -errno;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static VFIOContainer *vfio_eeh_as_container(AddressSpace *as)
> > +{
> > +    VFIOAddressSpace *space = vfio_get_address_space(as);
> > +    VFIOContainer *container = NULL;
> > +
> > +    if (QLIST_EMPTY(&space->containers)) {
> > +        /* No containers to act on */
> > +        goto out;
> > +    }
> > +
> > +    container = QLIST_FIRST(&space->containers);
> > +
> > +    if (QLIST_NEXT(container, next)) {
> > +        /* We don't yet have logic to synchronize EEH state across
> > +         * multiple containers */
> > +        container = NULL;
> > +        goto out;
> > +    }
> > +
> > +out:
> > +    vfio_put_address_space(space);
> > +    return container;
> > +}
> 
> 
> Here too, vfio_container_from_as()

Ok, I'll make that change.

> Overall the series looks good to me, nice cleanup both in power and vfio
> code.  Thanks,
> 
> Alex
> 
> 
> > +
> > +bool vfio_eeh_as_ok(AddressSpace *as)
> > +{
> > +    VFIOContainer *container = vfio_eeh_as_container(as);
> > +
> > +    return (container != NULL) && vfio_eeh_container_ok(container);
> > +}
> > +
> > +int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
> > +{
> > +    VFIOContainer *container = vfio_eeh_as_container(as);
> > +
> > +    return vfio_eeh_container_op(container, op);
> > +}
> > diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
> > index 0b26cd8..fd3933b 100644
> > --- a/include/hw/vfio/vfio.h
> > +++ b/include/hw/vfio/vfio.h
> > @@ -5,5 +5,7 @@
> >  
> >  extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> >                                  int req, void *param);
> > +bool vfio_eeh_as_ok(AddressSpace *as);
> > +int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
> >  
> >  #endif
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[Alex Williamson]

On Tue, 2015-12-01 at 13:23 +1100, David Gibson wrote:
> On Mon, Nov 23, 2015 at 02:58:11PM -0700, Alex Williamson wrote:
> > On Thu, 2015-11-19 at 15:29 +1100, David Gibson wrote:
> > > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > > on VFIO devices operates by bypassing the usual VFIO logic with
> > > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > > semantics about exactly what can be operated on.
> > > 
> > > In particular it operates on a single vfio container internally (hence the
> > > name), but takes an address space and group id, from which it deduces the
> > > container in a rather roundabout way.  groupids are something that code
> > > outside vfio shouldn't even be aware of.
> > > 
> > > This patch creates new interfaces for EEH operations.  Internally we
> > > have vfio_eeh_container_op() which takes a VFIOContainer object
> > > directly.  For external use we have vfio_eeh_as_ok() which determines
> > > if an AddressSpace is usable for EEH (at present this means it has a
> > > single container and at most a single group attached), and
> > > vfio_eeh_as_op() which will perform an operation on an AddressSpace in
> > > the unambiguous case, and otherwise returns an error.
> > > 
> > > This interface still isn't great, but it's enough of an improvement to
> > > allow a number of cleanups in other places.
> > > 
> > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > ---
> > >  hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/vfio/vfio.h |  2 ++
> > >  2 files changed, 79 insertions(+)
> > > 
> > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > index 6797208..4733625 100644
> > > --- a/hw/vfio/common.c
> > > +++ b/hw/vfio/common.c
> > > @@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> > >  
> > >      return vfio_container_do_ioctl(as, groupid, req, param);
> > >  }
> > > +
> > > +/*
> > > + * Interfaces for IBM EEH (Enhanced Error Handling)
> > > + */
> > > +static bool vfio_eeh_container_ok(VFIOContainer *container)
> > > +{
> > > +    /* A broken kernel implementation means EEH operations won't work
> > > +     * correctly if there are multiple groups in a container */
> > > +
> > > +    if (!QLIST_EMPTY(&container->group_list)
> > > +        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
> > > +        return false;
> > > +    }
> > > +
> > > +    return true;
> > > +}
> > 
> > Seems like there are ways to make this a non-eeh specific function,
> > vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
> > etc.
> 
> I guess, but I don't know of anything else that needs to know, so is
> there a point?

Yes, long term maintainability.  Simple functions that are named based
on what they do are building blocks for other users, even if we don't
yet know they exist.  Functions tainted with the name and purpose of
their currently intended callers are cruft and code duplication waiting
to happen.

> Actually.. I could do with a another opinion here: so, logically EEH
> operations should be possible on a container basis - the kernel
> interface correctly reflects that (my previous comments that the
> interface was broken were mistaken).
> 
> The current kernel implementation *is* broken (and is non-trivial to
> fix) which is what this test is about.  But is checking for a probably
> broken kernel state something that we ought to be checking for in
> qemu?  As it stands when the kernel is fixed we'll need a new
> capability so that qemu can know to disable this test.
> 
> Should we instead just proceed with any container and just advise
> people not to attach multiple groups until the kernel is fixed?
> 
> A relevant point here might be that while I haven't implemented it so
> far, I think it will be possible to workaround the broken kernel with
> full functionality by forcing each group into a separate container and
> using one of a couple of possible different methods to handle EEH
> functionality across multiple containers on a vPHB.

This sounds vaguely similar to the discussions we're having around AER
handling.  We really need to be able to translate a guest bus reset to a
host bus reset to enable guest participation in AER recovery, but iommu
grouping doesn't encompass any sort of shared bus property on x86 like
it does on power.  Therefore the configurations where we can enable AER
are only a subset of what we can enable otherwise.  However, not
everyone cares about AER recovery and perhaps the same is true of EEH.
So you really don't want to prevent useful configurations if the user
doesn't opt-in for that feature.

So for AER we're thinking about a new vfio-pci option, aer=on, that
indicates the device must be in a configuration that supports AER or the
VM instantiation (or device hotplug) should fail.  Should EEH do
something similar?  Should we share an option to make life easier for
libvirt so it doesn't need to care about EEH vs AER?  If the kernel
interface is eventually fixed, maybe that just relaxes some of the
configuration parameters making EEH support easier to achieve, but still
optional?  Thanks,

Alex

Re: [Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6534 bytes --]

On Wed, Dec 02, 2015 at 01:09:34PM -0700, Alex Williamson wrote:
> On Tue, 2015-12-01 at 13:23 +1100, David Gibson wrote:
> > On Mon, Nov 23, 2015 at 02:58:11PM -0700, Alex Williamson wrote:
> > > On Thu, 2015-11-19 at 15:29 +1100, David Gibson wrote:
> > > > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > > > on VFIO devices operates by bypassing the usual VFIO logic with
> > > > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > > > semantics about exactly what can be operated on.
> > > > 
> > > > In particular it operates on a single vfio container internally (hence the
> > > > name), but takes an address space and group id, from which it deduces the
> > > > container in a rather roundabout way.  groupids are something that code
> > > > outside vfio shouldn't even be aware of.
> > > > 
> > > > This patch creates new interfaces for EEH operations.  Internally we
> > > > have vfio_eeh_container_op() which takes a VFIOContainer object
> > > > directly.  For external use we have vfio_eeh_as_ok() which determines
> > > > if an AddressSpace is usable for EEH (at present this means it has a
> > > > single container and at most a single group attached), and
> > > > vfio_eeh_as_op() which will perform an operation on an AddressSpace in
> > > > the unambiguous case, and otherwise returns an error.
> > > > 
> > > > This interface still isn't great, but it's enough of an improvement to
> > > > allow a number of cleanups in other places.
> > > > 
> > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > ---
> > > >  hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/vfio/vfio.h |  2 ++
> > > >  2 files changed, 79 insertions(+)
> > > > 
> > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > index 6797208..4733625 100644
> > > > --- a/hw/vfio/common.c
> > > > +++ b/hw/vfio/common.c
> > > > @@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> > > >  
> > > >      return vfio_container_do_ioctl(as, groupid, req, param);
> > > >  }
> > > > +
> > > > +/*
> > > > + * Interfaces for IBM EEH (Enhanced Error Handling)
> > > > + */
> > > > +static bool vfio_eeh_container_ok(VFIOContainer *container)
> > > > +{
> > > > +    /* A broken kernel implementation means EEH operations won't work
> > > > +     * correctly if there are multiple groups in a container */
> > > > +
> > > > +    if (!QLIST_EMPTY(&container->group_list)
> > > > +        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    return true;
> > > > +}
> > > 
> > > Seems like there are ways to make this a non-eeh specific function,
> > > vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
> > > etc.
> > 
> > I guess, but I don't know of anything else that needs to know, so is
> > there a point?
> 
> Yes, long term maintainability.  Simple functions that are named based
> on what they do are building blocks for other users, even if we don't
> yet know they exist.  Functions tainted with the name and purpose of
> their currently intended callers are cruft and code duplication waiting
> to happen.

Ok, point taken.

> > Actually.. I could do with a another opinion here: so, logically EEH
> > operations should be possible on a container basis - the kernel
> > interface correctly reflects that (my previous comments that the
> > interface was broken were mistaken).
> > 
> > The current kernel implementation *is* broken (and is non-trivial to
> > fix) which is what this test is about.  But is checking for a probably
> > broken kernel state something that we ought to be checking for in
> > qemu?  As it stands when the kernel is fixed we'll need a new
> > capability so that qemu can know to disable this test.
> > 
> > Should we instead just proceed with any container and just advise
> > people not to attach multiple groups until the kernel is fixed?
> > 
> > A relevant point here might be that while I haven't implemented it so
> > far, I think it will be possible to workaround the broken kernel with
> > full functionality by forcing each group into a separate container and
> > using one of a couple of possible different methods to handle EEH
> > functionality across multiple containers on a vPHB.
> 
> This sounds vaguely similar to the discussions we're having around AER
> handling.  We really need to be able to translate a guest bus reset to a
> host bus reset to enable guest participation in AER recovery, but iommu
> grouping doesn't encompass any sort of shared bus property on x86 like
> it does on power.  Therefore the configurations where we can enable AER
> are only a subset of what we can enable otherwise.  However, not
> everyone cares about AER recovery and perhaps the same is true of EEH.
> So you really don't want to prevent useful configurations if the user
> doesn't opt-in for that feature.
> 
> So for AER we're thinking about a new vfio-pci option, aer=on, that
> indicates the device must be in a configuration that supports AER or the
> VM instantiation (or device hotplug) should fail.  Should EEH do
> something similar?

Yes, I think that's a good idea.  I'd been thinking about a PHB option
for enabling EEH, but I think one on the devices themselves makes
things work better.

> Should we share an option to make life easier for
> libvirt so it doesn't need to care about EEH vs AER?

My initial thought is yes, but I'm not really sure if there are
wrinkles that could make that a problem.

> If the kernel
> interface is eventually fixed, maybe that just relaxes some of the
> configuration parameters making EEH support easier to achieve, but still
> optional?  Thanks,

So, yes, and that's good, but that's not really what I was asking
about.

The kernel *interface* is not broken, just the implementation.  Which
means when it's fixed it won't be discoverable unless we also add a
capability advertising the fix.

So the question is: do we workaround in qemu until such a capability
comes along, or just assume that it's (potentially) working and
declare it a kernel problem if it doesn't?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [RFC 01/12] vfio: Start improving VFIO/EEH interface

[Alex Williamson]

On Thu, 2015-12-03 at 15:22 +1100, David Gibson wrote:
> On Wed, Dec 02, 2015 at 01:09:34PM -0700, Alex Williamson wrote:
> > On Tue, 2015-12-01 at 13:23 +1100, David Gibson wrote:
> > > On Mon, Nov 23, 2015 at 02:58:11PM -0700, Alex Williamson wrote:
> > > > On Thu, 2015-11-19 at 15:29 +1100, David Gibson wrote:
> > > > > At present the code handling IBM's Enhanced Error Handling (EEH) interface
> > > > > on VFIO devices operates by bypassing the usual VFIO logic with
> > > > > vfio_container_ioctl().  That's a poorly designed interface with unclear
> > > > > semantics about exactly what can be operated on.
> > > > > 
> > > > > In particular it operates on a single vfio container internally (hence the
> > > > > name), but takes an address space and group id, from which it deduces the
> > > > > container in a rather roundabout way.  groupids are something that code
> > > > > outside vfio shouldn't even be aware of.
> > > > > 
> > > > > This patch creates new interfaces for EEH operations.  Internally we
> > > > > have vfio_eeh_container_op() which takes a VFIOContainer object
> > > > > directly.  For external use we have vfio_eeh_as_ok() which determines
> > > > > if an AddressSpace is usable for EEH (at present this means it has a
> > > > > single container and at most a single group attached), and
> > > > > vfio_eeh_as_op() which will perform an operation on an AddressSpace in
> > > > > the unambiguous case, and otherwise returns an error.
> > > > > 
> > > > > This interface still isn't great, but it's enough of an improvement to
> > > > > allow a number of cleanups in other places.
> > > > > 
> > > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > > > > ---
> > > > >  hw/vfio/common.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  include/hw/vfio/vfio.h |  2 ++
> > > > >  2 files changed, 79 insertions(+)
> > > > > 
> > > > > diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> > > > > index 6797208..4733625 100644
> > > > > --- a/hw/vfio/common.c
> > > > > +++ b/hw/vfio/common.c
> > > > > @@ -1002,3 +1002,80 @@ int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
> > > > >  
> > > > >      return vfio_container_do_ioctl(as, groupid, req, param);
> > > > >  }
> > > > > +
> > > > > +/*
> > > > > + * Interfaces for IBM EEH (Enhanced Error Handling)
> > > > > + */
> > > > > +static bool vfio_eeh_container_ok(VFIOContainer *container)
> > > > > +{
> > > > > +    /* A broken kernel implementation means EEH operations won't work
> > > > > +     * correctly if there are multiple groups in a container */
> > > > > +
> > > > > +    if (!QLIST_EMPTY(&container->group_list)
> > > > > +        && QLIST_NEXT(QLIST_FIRST(&container->group_list), container_next)) {
> > > > > +        return false;
> > > > > +    }
> > > > > +
> > > > > +    return true;
> > > > > +}
> > > > 
> > > > Seems like there are ways to make this a non-eeh specific function,
> > > > vfio_container_group_count(), vfio_container_group_empty_or_singleton(),
> > > > etc.
> > > 
> > > I guess, but I don't know of anything else that needs to know, so is
> > > there a point?
> > 
> > Yes, long term maintainability.  Simple functions that are named based
> > on what they do are building blocks for other users, even if we don't
> > yet know they exist.  Functions tainted with the name and purpose of
> > their currently intended callers are cruft and code duplication waiting
> > to happen.
> 
> Ok, point taken.
> 
> > > Actually.. I could do with a another opinion here: so, logically EEH
> > > operations should be possible on a container basis - the kernel
> > > interface correctly reflects that (my previous comments that the
> > > interface was broken were mistaken).
> > > 
> > > The current kernel implementation *is* broken (and is non-trivial to
> > > fix) which is what this test is about.  But is checking for a probably
> > > broken kernel state something that we ought to be checking for in
> > > qemu?  As it stands when the kernel is fixed we'll need a new
> > > capability so that qemu can know to disable this test.
> > > 
> > > Should we instead just proceed with any container and just advise
> > > people not to attach multiple groups until the kernel is fixed?
> > > 
> > > A relevant point here might be that while I haven't implemented it so
> > > far, I think it will be possible to workaround the broken kernel with
> > > full functionality by forcing each group into a separate container and
> > > using one of a couple of possible different methods to handle EEH
> > > functionality across multiple containers on a vPHB.
> > 
> > This sounds vaguely similar to the discussions we're having around AER
> > handling.  We really need to be able to translate a guest bus reset to a
> > host bus reset to enable guest participation in AER recovery, but iommu
> > grouping doesn't encompass any sort of shared bus property on x86 like
> > it does on power.  Therefore the configurations where we can enable AER
> > are only a subset of what we can enable otherwise.  However, not
> > everyone cares about AER recovery and perhaps the same is true of EEH.
> > So you really don't want to prevent useful configurations if the user
> > doesn't opt-in for that feature.
> > 
> > So for AER we're thinking about a new vfio-pci option, aer=on, that
> > indicates the device must be in a configuration that supports AER or the
> > VM instantiation (or device hotplug) should fail.  Should EEH do
> > something similar?
> 
> Yes, I think that's a good idea.  I'd been thinking about a PHB option
> for enabling EEH, but I think one on the devices themselves makes
> things work better.
> 
> > Should we share an option to make life easier for
> > libvirt so it doesn't need to care about EEH vs AER?
> 
> My initial thought is yes, but I'm not really sure if there are
> wrinkles that could make that a problem.
> 
> > If the kernel
> > interface is eventually fixed, maybe that just relaxes some of the
> > configuration parameters making EEH support easier to achieve, but still
> > optional?  Thanks,
> 
> So, yes, and that's good, but that's not really what I was asking
> about.
> 
> The kernel *interface* is not broken, just the implementation.  Which
> means when it's fixed it won't be discoverable unless we also add a
> capability advertising the fix.
> 
> So the question is: do we workaround in qemu until such a capability
> comes along, or just assume that it's (potentially) working and
> declare it a kernel problem if it doesn't?

I'm in favor of deterministic behavior, so I think we better consider
the limitations of the current implementation to be part of the ABI and
add a capability when that limitation is lifted.  Whether you choose to
abandon EEH in QEMU until the implementation better suits your needs or
stumble along with restricted configurations is up to you, but QEMU
shouldn't need to assume anything.  Thanks,

Alex