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

[Alex Williamson]

On Wed, 13 Jul 2016 09:04:16 +0800
Zhou Jie <address@hidden> wrote:

> Hi Alex,
> 
> >> I will use workable state support flag
> >> to let user know whether the kenerl support block feature.
> >> And make configure space writing and ioctl function blocked.  
> >
> > And what of my suggestion that a user may desire to poll the state of
> > the device?  
> I will also add a poll function to vfio_fops.

Can you explain how this will work?  I was only suggesting that one of
the flag bits in vfio_device_info be allocated to report the current
state of blocking and the user could poll by repeatedly calling the
DEVICE_INFO ioctl.  Are you thinking of using POLLOUT/POLLIN?  I'm not
sure if those are a perfect match since it's really only the PCI config
region and a few ioctls where access is blocked, other operations may
proceed normally.
 
> > A user does know what the vfio driver has done if you define the
> > behavior that on an AER error reported event, as signaled to the user
> > via the error notification interrupt, vfio-pci will teardown device
> > interrupts to an uninitialized state.  The difference between the
> > command register approach you suggest and the teardown I suggest is
> > that the command register is simply masking interrupt deliver while the
> > teardown approach returns the device to an uninitialized interrupt
> > state.  Take a look at the device state when a bus reset occurs, what
> > state is saved and restored and what is left at a default PCI value.
> > The command register is saved and restored, so any manipulation we do
> > of it is racing the host kernel AER handling and bus reset.  What about
> > MSI and MSI-X?  Looks to me like those are left at the PCI default
> > initialization state, so now after an AER error we have irq handlers
> > and eventfds configured, while in fact the device has been
> > de-programmed.  To handle that we're expecting users to teardown the
> > interrupt state and re-establish it?  Again, why not just teardown the
> > interrupt state ourselves?  I dont' see the value in simply masking the
> > command register, especially when it doesn't handle the no-DisINTx case.  
> I understand.
> Thank you very much to explain this to me.
> I will teardown the interrupt state.
> 
> > We cannot depend on the behavior of any given driver and the fact that
> > the guest driver may teardown interrupts anyway is not a justification
> > that vfio shouldn't be doing this to make the device state presented to
> > the user consistent.  Thanks,  
> I understand.
> 
> The following code will be modified.
> 1. vfio_pci_ioctl
>     add a flag in vfio_device_info for workable_state support
> 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:
>     block write to configure space
> 4. vfio_pci_aer_err_detected
>     Set workable_state to false in "struct vfio_pci_device"
>     teardown the interrupt
> 5. vfio_pci_aer_resume
>     Set workable_state to true in "struct vfio_pci_device"
> 6. vfio_fops
>     Add poll function

I would still suggest that the name "workable_state" is quite vague.
Something like aer_error_in_progress is much more specific.  Thanks,

Alex