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

["Cédric Le Goater" <clg@redhat.com>]

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"