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

[Zhou Jie]

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.

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

Sincerely
Zhou Jie