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

[Alex Williamson]

On Wed, 6 Jul 2016 10:01:28 +0800
Zhou Jie <address@hidden> wrote:

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

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

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

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