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

[Alex Williamson]

On Wed, 2014-08-27 at 23:15 +1000, Gavin Shan wrote:
> On Tue, Aug 26, 2014 at 02:25:47PM -0600, Alex Williamson wrote:
> >On Wed, 2014-08-20 at 19:52 +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 <address@hidden>
> >> ---
> >>  hw/misc/vfio.c | 24 ++++++++++++++++++++++++
> >>  1 file changed, 24 insertions(+)
> >> 
> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
> >> index 1a3e7eb..3cf7f02 100644
> >> --- a/hw/misc/vfio.c
> >> +++ b/hw/misc/vfio.c
> >> @@ -4424,6 +4424,8 @@ int vfio_container_ioctl(AddressSpace *as, int32_t 
> >> groupid,
> >>  {
> >>      VFIOGroup *group;
> >>      VFIOContainer *container;
> >> +    VFIODevice *vdev;
> >> +    struct vfio_eeh_pe_op *arg;
> >
> >Define these within the scope of the case since they're not used outside
> >of it.
> >
> 
> Yes, I'll fix.
> 
> >>      int ret = -1;
> >>  
> >>      group = vfio_get_group(groupid, as);
> >> @@ -4442,7 +4444,29 @@ 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:
> >> +        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) {
> >> +                if (msix_enabled(&vdev->pdev)) {
> >> +                    vfio_disable_msix(vdev);
> >> +                }
> >> +
> >> +                msix_reset(&vdev->pdev);
> >> +            }
> >
> >We already have vfio_disable_interrupts(), can't we use that (blindly)?
> >Do we really need to call msix_reset()?  If so, should
> >vfio_disable_msix() call it?
> >
> 
> Yes, vfio_disable_interrupts() would be better to be used here as it
> can covers all types of interrupt (INTx/MSI/MSIx).
> 
> vfio_disable_interrupts() needn't clear MSIx vectors. If you prefer
> calling msix_reset() in the function, I guess I have to add one more
> parameter to vfio_disable_interrupts() to indicate if we need clear
> MSI/MSIx vector:
> 
> static void vfio_disable_interrupts(VFIODevice *vdev, bool clr_vector)

How about just creating a vfio_reset_interrupts() that calls
msix_reset() if the device supports MSIX?  We could wrap
vfio_disable_interrupts() and vfio_reset_interrupts() into a
vfio_disable_and_reset_interrupts(), but I'm not sure it's worth it.
The reset device path should also add the interrupt reset call.  Thanks,

Alex

> >> +
> >> +            break;
> >
> >Extraneous break
> >
> 
> Yep, I'll remove it.
> 
> >> +        }
> >> +
> >>          break;
> >>      default:
> >>          /* Return an error on unknown requests */
> >
> >Thanks,
> >Alex
> >
> 
> Thanks,
> Gavin
>