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

[Zhou Jie <zhoujie2011@cn.fujitsu.com>]

Hi Alex,

> Due to weekend and holiday in my country, there were zero regular
> working hours between your emails.
I wish you had a good time.

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

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

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

>>> 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,
The reinitialization depend on user.
For VFIO driver the process is following.
1. aer occurs
2. disable the following functions of the device
    write(except the bar regions), ioctl and interrupt
3. aer driver reset the device
4. renable the device for user
5. user process the aer event
    Maybe reset the device and reinitialization

What I do is make sure the following points.
1. Host can reset the device between step 2 and 4.
2. The user settings is the same at step 1 and 5.

Sincerely
Zhou Jie