Re: [Qemu-devel] [PATCH v8 11/12] vfio: register aer resume notification handler for aer resume

[Alex Williamson]

On Fri, 8 Jul 2016 09:38:50 +0800
Zhou Jie <address@hidden> wrote:

> Hi Alex,
> 
> >>>>> The following code will be modified.
> >>>>> 1. vfio_pci_ioctl
> >>>>>    add a flag in vfio_device_info for workable_state support
> >>>>>    return workable_state in "struct vfio_pci_device" when user get info 
> >>>>>  
> >>>
> >>> Seems like two flags are required, one to indicate the presence of this
> >>> feature and another to indicate the state.  I'd prefer something like
> >>> access_blocked.  
> >> User can get the state by state flag.
> >> And also maybe blocked by ioctl or write functons.
> >> User has choice to invoke which functions.  
> >
> > Let's imagine there's one flag bit, there are two possible polarities,
> > a) the bit is set when access is available, b) the bit is set when
> > access is blocked.
> >
> > Let's examine a), if the bit is not set, does that means that access is
> > not available or does that mean the kernel doesn't support that
> > feature?  There's no way to know.  Fail.  So we switch to b), an error
> > occurs, the bit is not set, does that mean access is blocked or does
> > that mean that the kernel we're using doesn't support the feature.
> > Fail.  If there's a way to do this with one bit, please explain it to
> > me.  Relying on a function to block, which may not be a valid
> > assumption on the kernel we're using also fails.  Userspace must be
> > able to know, in a deterministic and backwards compatible way, the
> > features of the kernel and the behavior to expect.  
> Sorry, I didn't say clearly.
> There is one flag and one variable.
> 1) workable state support flag
>     This flag let user know whether the kenerl support block feature.
> 2) workable state variable in "struct vfio_pci_device"
>     This variable let user know whether the device is blocked.

The variable clearly isn't visible to the user, so the user can know
whether the kernel supports this feature, but not whether the feature
is currently active.  Perhaps there's no way to avoid races completely,
but don't you expect that if we define that certain operations are
blocked after an error notification that a user may want some way to
poll for whether the block is active after a reset rather than simply
calling a blocked interface to probe for it?
 
> >>>>> 2. vfio_pci_ioctl
> >>>>>    During err occurs and resume:
> >>>>>    if (cmd == VFIO_DEVICE_SET_IRQS || VFIO_DEVICE_RESET
> >>>>>    || VFIO_DEVICE_GET_PCI_HOT_RESET_INFO || VFIO_DEVICE_PCI_HOT_RESET)
> >>>>>    block for workable_state clearing
> >>>>> 3. vfio_pci_write
> >>>>>    During err occurs and resume:
> >>>>>    ignore and return 0  
> >>>
> >>> This is contradictory to your comment "Do nothing for bar regions".
> >>> ISTR that returning 0 for read/write calls is an easy way to break
> >>> users since we've return neither the desired bytes nor an error code.  
> >> No, there is not change for read.
> >> Just return 0 for write.
> >> Return 0 mean that there is no byte has been written.
> >> Consider that the aer has occurred,
> >> it is better to not to write any thing to device.
> >> User can still read/write bar regions by mmap address,
> >> this may generate some date errors,
> >> but it doesn't matter as device is going to be reset.  
> >
> > My statement still stands, you state "Do nothing for bar regions" and
> > "return 0 for write".  Those are contradictory and as I indicate,
> > returning 0 is problematic for userspace.  Additionally, making
> > vfio_pci_write return zero while still allowing writes through the BAR
> > mmap is inconsistent.  
> I understand.
> I will block writing to configure space.
> And don't change for writing bar regions.
> 
> >>>>> 4. vfio_pci_aer_err_detected
> >>>>>    Set workable_state to false in "struct vfio_pci_device"
> >>>>>    Disable INTx:
> >>>>>      If Disable INTx is support
> >>>>>        disable by PCI_COMMAND
> >>>>>      else
> >>>>>        disable by disable_irq function
> >>>>>    Disable MSI:
> >>>>>        disable by clearing the "Bus Master Enable" bit of PCI_COMMAND  
> >>>
> >>> I've suggested repeatedly to properly teardown these interrupts.  I
> >>> disagree with your proposed approach here.  If the device is intended to
> >>> be in a state that requires re-initialization then the interrupt setup
> >>> should be part of that.  
> >> I have traced the INTx functions.
> >> -vfio_pci_intx_unmask_handler
> >> -vfio_pci_intx_mask
> >> -vfio_intx_set_signal
> >>
> >> They are invoked by User.
> >> -vfio_pci_write
> >> -vfio_pci_ioctl
> >>
> >> During err occurs and resume above functions are blocked.
> >> So, User cann't set the INTx.
> >> I will disable the INTx in vfio_pci_aer_err_detected.
> >> And reset the INTx in vfio_pci_aer_resume
> >> according the original user setting(vdev->ctx[0].masked).  
> >
> > Again, you're missing the point.  If the device is expected to be
> > reinitialized after resume, why don't we return the device to an
> > initial state where interrupts are not configured?  I think this
> > presents inconsistent behavior to the user.  
> About return the device to an
> initial state where interrupts are not configured.
> Do you mean free_irq?
> User request_irq by vfio_pci_ioctl.
> If we free_irq, how can the user know it?
> Do you think guest will reinitializ the device after resume,
> so it doesn't matter?
> 
> Maybe we should not expect what the guest will do.
> What I want to do in vfio driver is as following.
> 1. aer occurs
> 2. Disable INTx and MSI
> 3. aer driver reset the device
> 4. Restore INTx and MSI
> 5. user process the aer event

As we've discussed before, the AER notification needs to be relayed to
the user without delay, otherwise we only increase the gap where the
user might consume bogus data.  It also only seems reasonable to modify
the behavior of the interfaces (ie. blocking) if the user is notified,
which would be through the existing error notifier.  We can never
depend on a specific behavior from the user, we may be dealing with a
malicious user.

We already disable interrupts in vfio_pci_disable() simply by calling
the ioctl function directly.  If we simply disable and re-enable
interrupts as you propose, how does the user deal with edge triggered
interrupts that may have occurred during that period?  Are they lost?
Should we instead leave the interrupts enabled but skip
eventfd_signal() in the interrupts handlers, queuing interrupts for
re-delivery after the device is resumed?  Or does it make more sense to
simply disable the interrupts as done in vfio_pci_disable() and define
that the user needs to re-establish interrupts before continuing after
an error event?  Thanks,

Alex