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

[Alex Williamson]

On Tue, 5 Jul 2016 09:36:27 +0800
Zhou Jie <address@hidden> wrote:

> ping

Due to weekend and holiday in my country, there were zero regular
working hours between your emails.
 
> On 2016/7/3 12:00, Zhou Jie wrote:
> > Hi Alex,
> >
> > On 2016/6/30 9:45, Zhou Jie wrote:  
> >> Hi Alex,
> >>
> >> On 2016/6/30 2:22, Alex Williamson wrote:  
> >>> On Wed, 29 Jun 2016 16:54:05 +0800
> >>> Zhou Jie <address@hidden> wrote:
> >>>  
> >>>> Hi Alex,
> >>>>  
> >>>>> And yet we have struct pci_dev.broken_intx_masking and we test for
> >>>>> working DisINTx via pci_intx_mask_supported() rather than simply
> >>>>> looking for a PCIe device.  Some devices are broken and some simply
> >>>>> don't follow the spec, so you're going to need to deal with that or
> >>>>> exclude those devices.  
> >>>> For those devices I have no way to disable the INTx.  
> >>>
> >>> disable_irq()?  Clearly vfio-pci already manages these types of devices
> >>> and can disable INTx.  This is why I keep suggesting that maybe tearing
> >>> the interrupt setup down completely is a more complete and reliable
> >>> approach than masking in the command register.  Unless we're going to
> >>> exclude such devices from supporting this mode (which I don't condone),
> >>> we must deal with them.  
> >> Thank you for tell me that.
> >> Yes, I can use disable_irq to disable the pci device irq.
> >> But should I enable the irq after reset?
> >> I will dig into it.  
> >
> > I will alter the VFIO driver like following.
> > During err occurs and resume:
> > 1. Make config space read only.
> > 2. Disable INTx/MSI Interrupt.
> > 3. Do nothing for bar regions.
> >
> > 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.

> > 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.

> > 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.

> > 5. vfio_pci_aer_resume
> >    Set workable_state to true in "struct vfio_pci_device"
> >    About INTx:
> >      According to the value of "vdev->ctx[0].masked"
> >      to decide whether to enable INTx
> >    About MSI:
> >      After reset the "Bus Master Enable" bit is default to 0.
> >      So user should process this after reset.

Again, I think this is error prone, teardown the interrupts and define
that the device state, including interrupts, needs to be reinitialized
after error.  Why are you not incorporating this feedback?  Thanks,

Alex