[PATCH v3 01/37] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c

Zhenzhong Duan posted 37 patches 1 year, 1 month ago
There is a newer version of this series
[PATCH v3 01/37] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c
Posted by Zhenzhong Duan 1 year, 1 month ago
With vfio_eeh_as_ok/vfio_eeh_as_op moved and made static,
vfio.h becomes empty and is deleted.

No functional changes intended.

Suggested-by: Cédric Le Goater <clg@redhat.com>
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 include/hw/vfio/vfio.h  |   7 ---
 hw/ppc/spapr_pci_vfio.c | 100 +++++++++++++++++++++++++++++++++++++++-
 hw/vfio/ap.c            |   1 -
 hw/vfio/ccw.c           |   1 -
 hw/vfio/common.c        |   1 -
 hw/vfio/container.c     |  98 ---------------------------------------
 hw/vfio/helpers.c       |   1 -
 7 files changed, 99 insertions(+), 110 deletions(-)
 delete mode 100644 include/hw/vfio/vfio.h

diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
deleted file mode 100644
index 86248f5436..0000000000
--- a/include/hw/vfio/vfio.h
+++ /dev/null
@@ -1,7 +0,0 @@
-#ifndef HW_VFIO_H
-#define HW_VFIO_H
-
-bool vfio_eeh_as_ok(AddressSpace *as);
-int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
-
-#endif
diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
index 9016720547..f283f7e38d 100644
--- a/hw/ppc/spapr_pci_vfio.c
+++ b/hw/ppc/spapr_pci_vfio.c
@@ -18,14 +18,112 @@
  */
 
 #include "qemu/osdep.h"
+#include <sys/ioctl.h>
 #include <linux/vfio.h>
 #include "hw/ppc/spapr.h"
 #include "hw/pci-host/spapr.h"
 #include "hw/pci/msix.h"
 #include "hw/pci/pci_device.h"
-#include "hw/vfio/vfio.h"
+#include "hw/vfio/vfio-common.h"
 #include "qemu/error-report.h"
 
+/*
+ * Interfaces for IBM EEH (Enhanced Error Handling)
+ */
+static bool vfio_eeh_container_ok(VFIOContainer *container)
+{
+    /*
+     * As of 2016-03-04 (linux-4.5) the host kernel EEH/VFIO
+     * implementation is broken if there are multiple groups in a
+     * container.  The hardware works in units of Partitionable
+     * Endpoints (== IOMMU groups) and the EEH operations naively
+     * iterate across all groups in the container, without any logic
+     * to make sure the groups have their state synchronized.  For
+     * certain operations (ENABLE) that might be ok, until an error
+     * occurs, but for others (GET_STATE) it's clearly broken.
+     */
+
+    /*
+     * XXX Once fixed kernels exist, test for them here
+     */
+
+    if (QLIST_EMPTY(&container->group_list)) {
+        return false;
+    }
+
+    if (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: "
+                     "kernel requires a container with exactly one group", 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 ret;
+}
+
+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;
+}
+
+static bool vfio_eeh_as_ok(AddressSpace *as)
+{
+    VFIOContainer *container = vfio_eeh_as_container(as);
+
+    return (container != NULL) && vfio_eeh_container_ok(container);
+}
+
+static int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
+{
+    VFIOContainer *container = vfio_eeh_as_container(as);
+
+    if (!container) {
+        return -ENODEV;
+    }
+    return vfio_eeh_container_op(container, op);
+}
+
 bool spapr_phb_eeh_available(SpaprPhbState *sphb)
 {
     return vfio_eeh_as_ok(&sphb->iommu_as);
diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
index 5f257bffb9..bbf69ff55a 100644
--- a/hw/vfio/ap.c
+++ b/hw/vfio/ap.c
@@ -14,7 +14,6 @@
 #include <linux/vfio.h>
 #include <sys/ioctl.h>
 #include "qapi/error.h"
-#include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-common.h"
 #include "hw/s390x/ap-device.h"
 #include "qemu/error-report.h"
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 6623ae237b..d857bb8d0f 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -20,7 +20,6 @@
 #include <sys/ioctl.h>
 
 #include "qapi/error.h"
-#include "hw/vfio/vfio.h"
 #include "hw/vfio/vfio-common.h"
 #include "hw/s390x/s390-ccw.h"
 #include "hw/s390x/vfio-ccw.h"
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 9c5c6433f2..e72055e752 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -26,7 +26,6 @@
 #include <linux/vfio.h>
 
 #include "hw/vfio/vfio-common.h"
-#include "hw/vfio/vfio.h"
 #include "hw/vfio/pci.h"
 #include "exec/address-spaces.h"
 #include "exec/memory.h"
diff --git a/hw/vfio/container.c b/hw/vfio/container.c
index fc88222377..83c0f05bba 100644
--- a/hw/vfio/container.c
+++ b/hw/vfio/container.c
@@ -26,7 +26,6 @@
 #include <linux/vfio.h>
 
 #include "hw/vfio/vfio-common.h"
-#include "hw/vfio/vfio.h"
 #include "exec/address-spaces.h"
 #include "exec/memory.h"
 #include "exec/ram_addr.h"
@@ -1011,103 +1010,6 @@ static void vfio_put_base_device(VFIODevice *vbasedev)
     close(vbasedev->fd);
 }
 
-/*
- * Interfaces for IBM EEH (Enhanced Error Handling)
- */
-static bool vfio_eeh_container_ok(VFIOContainer *container)
-{
-    /*
-     * As of 2016-03-04 (linux-4.5) the host kernel EEH/VFIO
-     * implementation is broken if there are multiple groups in a
-     * container.  The hardware works in units of Partitionable
-     * Endpoints (== IOMMU groups) and the EEH operations naively
-     * iterate across all groups in the container, without any logic
-     * to make sure the groups have their state synchronized.  For
-     * certain operations (ENABLE) that might be ok, until an error
-     * occurs, but for others (GET_STATE) it's clearly broken.
-     */
-
-    /*
-     * XXX Once fixed kernels exist, test for them here
-     */
-
-    if (QLIST_EMPTY(&container->group_list)) {
-        return false;
-    }
-
-    if (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: "
-                     "kernel requires a container with exactly one group", 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 ret;
-}
-
-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);
-
-    if (!container) {
-        return -ENODEV;
-    }
-    return vfio_eeh_container_op(container, op);
-}
-
 static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp)
 {
     char *tmp, group_path[PATH_MAX], *group_name;
diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
index 7e5da21b31..168847e7c5 100644
--- a/hw/vfio/helpers.c
+++ b/hw/vfio/helpers.c
@@ -23,7 +23,6 @@
 #include <sys/ioctl.h>
 
 #include "hw/vfio/vfio-common.h"
-#include "hw/vfio/vfio.h"
 #include "hw/hw.h"
 #include "trace.h"
 #include "qapi/error.h"
-- 
2.34.1


Re: [PATCH v3 01/37] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c
Posted by Cédric Le Goater 1 year, 1 month ago
On 10/26/23 12:30, Zhenzhong Duan wrote:
> With vfio_eeh_as_ok/vfio_eeh_as_op moved and made static,
> vfio.h becomes empty and is deleted.
> 
> No functional changes intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>


Thanks,

C.


> ---
>   include/hw/vfio/vfio.h  |   7 ---
>   hw/ppc/spapr_pci_vfio.c | 100 +++++++++++++++++++++++++++++++++++++++-
>   hw/vfio/ap.c            |   1 -
>   hw/vfio/ccw.c           |   1 -
>   hw/vfio/common.c        |   1 -
>   hw/vfio/container.c     |  98 ---------------------------------------
>   hw/vfio/helpers.c       |   1 -
>   7 files changed, 99 insertions(+), 110 deletions(-)
>   delete mode 100644 include/hw/vfio/vfio.h
> 
> diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
> deleted file mode 100644
> index 86248f5436..0000000000
> --- a/include/hw/vfio/vfio.h
> +++ /dev/null
> @@ -1,7 +0,0 @@
> -#ifndef HW_VFIO_H
> -#define HW_VFIO_H
> -
> -bool vfio_eeh_as_ok(AddressSpace *as);
> -int vfio_eeh_as_op(AddressSpace *as, uint32_t op);
> -
> -#endif
> diff --git a/hw/ppc/spapr_pci_vfio.c b/hw/ppc/spapr_pci_vfio.c
> index 9016720547..f283f7e38d 100644
> --- a/hw/ppc/spapr_pci_vfio.c
> +++ b/hw/ppc/spapr_pci_vfio.c
> @@ -18,14 +18,112 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#include <sys/ioctl.h>
>   #include <linux/vfio.h>
>   #include "hw/ppc/spapr.h"
>   #include "hw/pci-host/spapr.h"
>   #include "hw/pci/msix.h"
>   #include "hw/pci/pci_device.h"
> -#include "hw/vfio/vfio.h"
> +#include "hw/vfio/vfio-common.h"
>   #include "qemu/error-report.h"
>   
> +/*
> + * Interfaces for IBM EEH (Enhanced Error Handling)
> + */
> +static bool vfio_eeh_container_ok(VFIOContainer *container)
> +{
> +    /*
> +     * As of 2016-03-04 (linux-4.5) the host kernel EEH/VFIO
> +     * implementation is broken if there are multiple groups in a
> +     * container.  The hardware works in units of Partitionable
> +     * Endpoints (== IOMMU groups) and the EEH operations naively
> +     * iterate across all groups in the container, without any logic
> +     * to make sure the groups have their state synchronized.  For
> +     * certain operations (ENABLE) that might be ok, until an error
> +     * occurs, but for others (GET_STATE) it's clearly broken.
> +     */
> +
> +    /*
> +     * XXX Once fixed kernels exist, test for them here
> +     */
> +
> +    if (QLIST_EMPTY(&container->group_list)) {
> +        return false;
> +    }
> +
> +    if (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: "
> +                     "kernel requires a container with exactly one group", 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 ret;
> +}
> +
> +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;
> +}
> +
> +static bool vfio_eeh_as_ok(AddressSpace *as)
> +{
> +    VFIOContainer *container = vfio_eeh_as_container(as);
> +
> +    return (container != NULL) && vfio_eeh_container_ok(container);
> +}
> +
> +static int vfio_eeh_as_op(AddressSpace *as, uint32_t op)
> +{
> +    VFIOContainer *container = vfio_eeh_as_container(as);
> +
> +    if (!container) {
> +        return -ENODEV;
> +    }
> +    return vfio_eeh_container_op(container, op);
> +}
> +
>   bool spapr_phb_eeh_available(SpaprPhbState *sphb)
>   {
>       return vfio_eeh_as_ok(&sphb->iommu_as);
> diff --git a/hw/vfio/ap.c b/hw/vfio/ap.c
> index 5f257bffb9..bbf69ff55a 100644
> --- a/hw/vfio/ap.c
> +++ b/hw/vfio/ap.c
> @@ -14,7 +14,6 @@
>   #include <linux/vfio.h>
>   #include <sys/ioctl.h>
>   #include "qapi/error.h"
> -#include "hw/vfio/vfio.h"
>   #include "hw/vfio/vfio-common.h"
>   #include "hw/s390x/ap-device.h"
>   #include "qemu/error-report.h"
> diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
> index 6623ae237b..d857bb8d0f 100644
> --- a/hw/vfio/ccw.c
> +++ b/hw/vfio/ccw.c
> @@ -20,7 +20,6 @@
>   #include <sys/ioctl.h>
>   
>   #include "qapi/error.h"
> -#include "hw/vfio/vfio.h"
>   #include "hw/vfio/vfio-common.h"
>   #include "hw/s390x/s390-ccw.h"
>   #include "hw/s390x/vfio-ccw.h"
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 9c5c6433f2..e72055e752 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -26,7 +26,6 @@
>   #include <linux/vfio.h>
>   
>   #include "hw/vfio/vfio-common.h"
> -#include "hw/vfio/vfio.h"
>   #include "hw/vfio/pci.h"
>   #include "exec/address-spaces.h"
>   #include "exec/memory.h"
> diff --git a/hw/vfio/container.c b/hw/vfio/container.c
> index fc88222377..83c0f05bba 100644
> --- a/hw/vfio/container.c
> +++ b/hw/vfio/container.c
> @@ -26,7 +26,6 @@
>   #include <linux/vfio.h>
>   
>   #include "hw/vfio/vfio-common.h"
> -#include "hw/vfio/vfio.h"
>   #include "exec/address-spaces.h"
>   #include "exec/memory.h"
>   #include "exec/ram_addr.h"
> @@ -1011,103 +1010,6 @@ static void vfio_put_base_device(VFIODevice *vbasedev)
>       close(vbasedev->fd);
>   }
>   
> -/*
> - * Interfaces for IBM EEH (Enhanced Error Handling)
> - */
> -static bool vfio_eeh_container_ok(VFIOContainer *container)
> -{
> -    /*
> -     * As of 2016-03-04 (linux-4.5) the host kernel EEH/VFIO
> -     * implementation is broken if there are multiple groups in a
> -     * container.  The hardware works in units of Partitionable
> -     * Endpoints (== IOMMU groups) and the EEH operations naively
> -     * iterate across all groups in the container, without any logic
> -     * to make sure the groups have their state synchronized.  For
> -     * certain operations (ENABLE) that might be ok, until an error
> -     * occurs, but for others (GET_STATE) it's clearly broken.
> -     */
> -
> -    /*
> -     * XXX Once fixed kernels exist, test for them here
> -     */
> -
> -    if (QLIST_EMPTY(&container->group_list)) {
> -        return false;
> -    }
> -
> -    if (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: "
> -                     "kernel requires a container with exactly one group", 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 ret;
> -}
> -
> -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);
> -
> -    if (!container) {
> -        return -ENODEV;
> -    }
> -    return vfio_eeh_container_op(container, op);
> -}
> -
>   static int vfio_device_groupid(VFIODevice *vbasedev, Error **errp)
>   {
>       char *tmp, group_path[PATH_MAX], *group_name;
> diff --git a/hw/vfio/helpers.c b/hw/vfio/helpers.c
> index 7e5da21b31..168847e7c5 100644
> --- a/hw/vfio/helpers.c
> +++ b/hw/vfio/helpers.c
> @@ -23,7 +23,6 @@
>   #include <sys/ioctl.h>
>   
>   #include "hw/vfio/vfio-common.h"
> -#include "hw/vfio/vfio.h"
>   #include "hw/hw.h"
>   #include "trace.h"
>   #include "qapi/error.h"


Re: [PATCH v3 01/37] vfio/container: Move IBM EEH related functions into spapr_pci_vfio.c
Posted by Eric Farman 1 year, 1 month ago
On Thu, 2023-10-26 at 18:30 +0800, Zhenzhong Duan wrote:
> With vfio_eeh_as_ok/vfio_eeh_as_op moved and made static,
> vfio.h becomes empty and is deleted.
> 
> No functional changes intended.
> 
> Suggested-by: Cédric Le Goater <clg@redhat.com>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  include/hw/vfio/vfio.h  |   7 ---
>  hw/ppc/spapr_pci_vfio.c | 100
> +++++++++++++++++++++++++++++++++++++++-
>  hw/vfio/ap.c            |   1 -
>  hw/vfio/ccw.c           |   1 -
>  hw/vfio/common.c        |   1 -
>  hw/vfio/container.c     |  98 --------------------------------------
> -
>  hw/vfio/helpers.c       |   1 -
>  7 files changed, 99 insertions(+), 110 deletions(-)
>  delete mode 100644 include/hw/vfio/vfio.h

Can't speak to the code movement, but removing the header file from
those places where it's not used is fine.

Acked-by: Eric Farman <farman@linux.ibm.com>