qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Zhou Jie
Subject: Re: [Qemu-devel] [PATCH v8 11/12] vfio: register aer resume notification handler for aer resume
Date: Wed, 6 Jul 2016 10:01:28 +0800
User-agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]