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

[Gavin Shan]

On Wed, Mar 18, 2015 at 10:26:33AM +1100, Gavin Shan wrote:
>On Tue, Mar 17, 2015 at 03:09:52PM -0600, Alex Williamson wrote:
>>On Tue, 2015-03-17 at 03:31 +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>
>>> ---
>>> v2: vfio_container_eeh_event() stub for !CONFIG_PCI and separate
>>>     error message for this function. Dropped vfio_put_group()
>>>     on NULL group
>>> ---
>>>  hw/vfio/Makefile.objs  |  6 +++++-
>>>  hw/vfio/common.c       |  7 +++++++
>>>  hw/vfio/pci-stub.c     | 17 +++++++++++++++++
>>>  hw/vfio/pci.c          | 38 ++++++++++++++++++++++++++++++++++++++
>>>  include/hw/vfio/vfio.h |  2 ++
>>>  5 files changed, 69 insertions(+), 1 deletion(-)
>>>  create mode 100644 hw/vfio/pci-stub.c
>>> 
>>> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs
>>> index e31f30e..1b8a065 100644
>>> --- a/hw/vfio/Makefile.objs
>>> +++ b/hw/vfio/Makefile.objs
>>> @@ -1,4 +1,8 @@
>>>  ifeq ($(CONFIG_LINUX), y)
>>>  obj-$(CONFIG_SOFTMMU) += common.o
>>> -obj-$(CONFIG_PCI) += pci.o
>>> +ifeq ($(CONFIG_PCI), y)
>>> +obj-y += pci.o
>>> +else
>>> +obj-y += pci-stub.o
>>> +endif
>>>  endif
>>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>>> index 148eb53..ed07814 100644
>>> --- a/hw/vfio/common.c
>>> +++ b/hw/vfio/common.c
>>> @@ -949,7 +949,14 @@ int vfio_container_ioctl(AddressSpace *as, int32_t 
>>> groupid,
>>>      switch (req) {
>>>      case VFIO_CHECK_EXTENSION:
>>>      case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
>>> +        break;
>>>      case VFIO_EEH_PE_OP:
>>> +        if (vfio_container_eeh_event(as, groupid, param) != 0) {
>>> +            error_report("vfio: cannot handle EEH event on group %d\n",
>>> +                         groupid);
>>> +            return -1;
>>> +        }
>>> +
>>>          break;
>>>      default:
>>>          /* Return an error on unknown requests */
>>> diff --git a/hw/vfio/pci-stub.c b/hw/vfio/pci-stub.c
>>> new file mode 100644
>>> index 0000000..7726a7a
>>> --- /dev/null
>>> +++ b/hw/vfio/pci-stub.c
>>> @@ -0,0 +1,17 @@
>>> +/*
>>> + * To include the file on !CONFIG_PCI
>>> + *
>>> + * This work is licensed under the terms of the GNU GPL, version 2.  See
>>> + * the COPYING file in the top-level directory.
>>> + */
>>> +
>>> +#include <linux/vfio.h>
>>> +
>>> +#include "exec/memory.h"
>>> +#include "hw/vfio/vfio.h"
>>> +
>>> +int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
>>> +                             struct vfio_eeh_pe_op *op)
>>> +{
>>> +    return -1;
>>> +}
>>
>>How about just a static inline in vfio.h?
>>
>
>Yes, It would make things simpler to have something as follows, but I didn't
>find there is a header file, which includes "#define CONFIG_PCI 1" or similar
>thing. Do you know the header file containing CONFIG_PCI, or alternative macro
>for the purpose?
>
>#ifdef CONFIG_PCI /* Or alternative macro */
>extern int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
>                                   struct vfio_eeh_pe_op *op);
>#else
>static ineline int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
>                                            struct vfio_eeh_pe_op *op)
>{
>    return -1;
>}
>#endif /* CONFIG_PCI */
>

Alex.W, any idea on this? :-)

Thanks,
Gavin

>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>>> index 6b80539..fca1edc 100644
>>> --- a/hw/vfio/pci.c
>>> +++ b/hw/vfio/pci.c
>>> @@ -3319,6 +3319,44 @@ 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) {
>>> +        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..e9a3abb 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);
>>> +extern int vfio_container_eeh_event(AddressSpace *as, int32_t groupid,
>>> +                                    struct vfio_eeh_pe_op *op);
>>>  
>>>  #endif
>>
>>
>>