Re: [Qemu-devel] [PATCH 1/3] VFIO: Clear stale MSIx table during EEH reset

[David Gibson]

On Wed, Mar 11, 2015 at 05:11:52PM +1100, Gavin Shan wrote:
> The PCI device MSIx table is cleaned out in hardware after EEH PE
> reset. However, we still hold the stale MSIx entries in QEMU, which
> should be cleared accordingly. Otherwise, we will run into another
> (recursive) EEH error and the PCI devices contained in the PE have
> to be offlined exceptionally.
> 
> The patch clears stale MSIx table before EEH PE reset so that MSIx
> table could be restored properly after EEH PE reset.
> 
> Signed-off-by: Gavin Shan <address@hidden>
> ---
>  hw/vfio/common.c       |  6 +++++-
>  hw/vfio/pci.c          | 39 +++++++++++++++++++++++++++++++++++++++
>  include/hw/vfio/vfio.h |  3 ++-
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 148eb53..e3833f4 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -949,8 +949,12 @@ int vfio_container_ioctl(AddressSpace *as, int32_t 
> groupid,
>      switch (req) {
>      case VFIO_CHECK_EXTENSION:
>      case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
> -    case VFIO_EEH_PE_OP:
>          break;
> +    case VFIO_EEH_PE_OP:
> +        if (!vfio_container_eeh_event(as, groupid, param)) {

Please use == 0 not !, remembering that !some_function() is the
success case hurts my brain.

> +            break;
> +        }
> +        /* fallthru */

It doesn't look like the fallthrough will generate the correct error
message: it will say "unsupported ioctl" but
vfio_container_eeh_event() could fail for some other reason.

>      default:
>          /* Return an error on unknown requests */
>          error_report("vfio: unsupported ioctl %X", req);
> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> index 6b80539..8c4a8cb 100644
> --- a/hw/vfio/pci.c
> +++ b/hw/vfio/pci.c
> @@ -3319,6 +3319,45 @@ static void vfio_unregister_req_notifier(VFIOPCIDevice 
> *vdev)
>      vdev->req_enabled = false;
>  }
>  
> +int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
> +                             struct vfio_eeh_pe_op *op)
> +{
> +    VFIOGroup *group;
> +    VFIODevice *vbasedev;
> +    VFIOPCIDevice *vdev;
> +
> +    group = vfio_get_group(groupid, as);
> +    if (!group) {
> +        vfio_put_group(group);

Is vfio_put_group(NULL) really what you want?

> +        error_report("vfio: group %d not found\n", groupid);
> +        return -1;
> +    }
> +
> +    switch (op->op) {
> +    case VFIO_EEH_PE_RESET_HOT:
> +    case VFIO_EEH_PE_RESET_FUNDAMENTAL:
> +        /*
> +         * 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.
> +         */
> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
> +            vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
> +            if (msix_enabled(&vdev->pdev)) {
> +                vfio_disable_msix(vdev);
> +            }
> +
> +            msix_reset(&vdev->pdev);
> +        }
> +
> +        break;
> +    }
> +
> +    vfio_put_group(group);
> +    return 0;
> +}
> +
>  static int vfio_initfn(PCIDevice *pdev)
>  {
>      VFIOPCIDevice *vdev = DO_UPCAST(VFIOPCIDevice, pdev, pdev);
> diff --git a/include/hw/vfio/vfio.h b/include/hw/vfio/vfio.h
> index 0b26cd8..99528a3 100644
> --- a/include/hw/vfio/vfio.h
> +++ b/include/hw/vfio/vfio.h
> @@ -5,5 +5,6 @@
>  
>  extern int vfio_container_ioctl(AddressSpace *as, int32_t groupid,
>                                  int req, void *param);
> -
> +extern int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
> +                                    struct vfio_eeh_pe_op *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

pgp5jsXoCcb6y.pgp
Description: PGP signature