[RFC,v2,2/2] VFIO: Clear stale MSIx table during EEH reset

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 <gwshan@linux.vnet.ibm.com>
---
 hw/misc/vfio.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

On Mon, 2014-09-01 at 10:53 +1000, 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 <gwshan@linux.vnet.ibm.com>
> ---
>  hw/misc/vfio.c | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> index 1a3e7eb..1f55051 100644
> --- a/hw/misc/vfio.c
> +++ b/hw/misc/vfio.c
> @@ -2724,6 +2724,17 @@ static void vfio_disable_interrupts(VFIODevice *vdev)
>      }
>  }
>  
> +static void vfio_disable_and_reset_interrupts(VFIODevice *vdev)
> +{
> +    vfio_disable_interrupts(vdev);
> +
> +    switch (vdev->interrupt) {
> +    case VFIO_INT_MSIX:
> +        msix_reset(&vdev->pdev);
> +        break;
> +    }

This is apparently untested because vdev->interrupt should never be set
to VFIO_INT_MSIX after vfio_disable_interrupts().  Also, you need to
update the normal reset path to call msix_reset() unless it's already
happening via another reset handler.  Thanks,

Alex

> +}
> +
>  static int vfio_setup_msi(VFIODevice *vdev, int pos)
>  {
>      uint16_t ctrl;
> @@ -4442,8 +4453,27 @@ 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: {
> +        VFIODevice *vdev;
> +        struct vfio_eeh_pe_op *arg = (struct vfio_eeh_pe_op *)param;
> +
> +        switch (arg->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(vdev, &group->device_list, next) {
> +                vfio_disable_and_reset_interrupts(vdev);
> +            }
> +        }
> +
> +        break;
> +    }
>      default:
>          /* Return an error on unknown requests */
>          error_report("vfio: unsupported ioctl %X", req);

On Tue, Sep 02, 2014 at 02:10:42PM -0600, Alex Williamson wrote:
>On Mon, 2014-09-01 at 10:53 +1000, 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 <gwshan@linux.vnet.ibm.com>
>> ---
>>  hw/misc/vfio.c | 32 +++++++++++++++++++++++++++++++-
>>  1 file changed, 31 insertions(+), 1 deletion(-)
>> 
>> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> index 1a3e7eb..1f55051 100644
>> --- a/hw/misc/vfio.c
>> +++ b/hw/misc/vfio.c
>> @@ -2724,6 +2724,17 @@ static void vfio_disable_interrupts(VFIODevice *vdev)
>>      }
>>  }
>>  
>> +static void vfio_disable_and_reset_interrupts(VFIODevice *vdev)
>> +{
>> +    vfio_disable_interrupts(vdev);
>> +
>> +    switch (vdev->interrupt) {
>> +    case VFIO_INT_MSIX:
>> +        msix_reset(&vdev->pdev);
>> +        break;
>> +    }
>
>This is apparently untested because vdev->interrupt should never be set
>to VFIO_INT_MSIX after vfio_disable_interrupts().  Also, you need to
>update the normal reset path to call msix_reset() unless it's already
>happening via another reset handler.  Thanks,
>

Yes, I didn't test this revision. I'll change the code according to
your comments and retest, then send v3 for your comments.

Thanks,
Gavin

>Alex
>
>> +}
>> +
>>  static int vfio_setup_msi(VFIODevice *vdev, int pos)
>>  {
>>      uint16_t ctrl;
>> @@ -4442,8 +4453,27 @@ 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: {
>> +        VFIODevice *vdev;
>> +        struct vfio_eeh_pe_op *arg = (struct vfio_eeh_pe_op *)param;
>> +
>> +        switch (arg->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(vdev, &group->device_list, next) {
>> +                vfio_disable_and_reset_interrupts(vdev);
>> +            }
>> +        }
>> +
>> +        break;
>> +    }
>>      default:
>>          /* Return an error on unknown requests */
>>          error_report("vfio: unsupported ioctl %X", req);
>
>
>