On Wed, Aug 27, 2014 at 07:29:41AM -0600, Alex Williamson wrote:
>On Wed, 2014-08-27 at 23:15 +1000, Gavin Shan wrote:
>> On Tue, Aug 26, 2014 at 02:25:47PM -0600, Alex Williamson wrote:
>> >On Wed, 2014-08-20 at 19:52 +1000, Gavin Shan wrote:
>> >> The PCI device MSIx table is cleaned out in hardware after EEH PE
>> >> reset. However, we still hold the stale MSIx entries in QEMU, which
>> >> should be cleared accordingly. Otherwise, we will run into another
>> >> (recursive) EEH error and the PCI devices contained in the PE have
>> >> to be offlined exceptionally.
>> >>
>> >> The patch clears stale MSIx table before EEH PE reset so that MSIx
>> >> table could be restored properly after EEH PE reset.
>> >>
>> >> Signed-off-by: Gavin Shan <gws...@linux.vnet.ibm.com>
>> >> ---
>> >> hw/misc/vfio.c | 24 ++++++++++++++++++++++++
>> >> 1 file changed, 24 insertions(+)
>> >>
>> >> diff --git a/hw/misc/vfio.c b/hw/misc/vfio.c
>> >> index 1a3e7eb..3cf7f02 100644
>> >> --- a/hw/misc/vfio.c
>> >> +++ b/hw/misc/vfio.c
>> >> @@ -4424,6 +4424,8 @@ int vfio_container_ioctl(AddressSpace *as, int32_t
>> >> groupid,
>> >> {
>> >> VFIOGroup *group;
>> >> VFIOContainer *container;
>> >> + VFIODevice *vdev;
>> >> + struct vfio_eeh_pe_op *arg;
>> >
>> >Define these within the scope of the case since they're not used outside
>> >of it.
>> >
>>
>> Yes, I'll fix.
>>
>> >> int ret = -1;
>> >>
>> >> group = vfio_get_group(groupid, as);
>> >> @@ -4442,7 +4444,29 @@ int vfio_container_ioctl(AddressSpace *as, int32_t
>> >> groupid,
>> >> switch (req) {
>> >> case VFIO_CHECK_EXTENSION:
>> >> case VFIO_IOMMU_SPAPR_TCE_GET_INFO:
>> >> + break;
>> >> case VFIO_EEH_PE_OP:
>> >> + arg = (struct vfio_eeh_pe_op *)param;
>> >> + switch (arg->op) {
>> >> + case VFIO_EEH_PE_RESET_HOT:
>> >> + case VFIO_EEH_PE_RESET_FUNDAMENTAL:
>> >> + /*
>> >> + * The MSIx table will be cleaned out by reset. We need
>> >> + * disable it so that it can be reenabled properly. Also,
>> >> + * the cached MSIx table should be cleared as it's not
>> >> + * reflecting the contents in hardware.
>> >> + */
>> >> + QLIST_FOREACH(vdev, &group->device_list, next) {
>> >> + if (msix_enabled(&vdev->pdev)) {
>> >> + vfio_disable_msix(vdev);
>> >> + }
>> >> +
>> >> + msix_reset(&vdev->pdev);
>> >> + }
>> >
>> >We already have vfio_disable_interrupts(), can't we use that (blindly)?
>> >Do we really need to call msix_reset()? If so, should
>> >vfio_disable_msix() call it?
>> >
>>
>> Yes, vfio_disable_interrupts() would be better to be used here as it
>> can covers all types of interrupt (INTx/MSI/MSIx).
>>
>> vfio_disable_interrupts() needn't clear MSIx vectors. If you prefer
>> calling msix_reset() in the function, I guess I have to add one more
>> parameter to vfio_disable_interrupts() to indicate if we need clear
>> MSI/MSIx vector:
>>
>> static void vfio_disable_interrupts(VFIODevice *vdev, bool clr_vector)
>
>How about just creating a vfio_reset_interrupts() that calls
>msix_reset() if the device supports MSIX? We could wrap
>vfio_disable_interrupts() and vfio_reset_interrupts() into a
>vfio_disable_and_reset_interrupts(), but I'm not sure it's worth it.
>The reset device path should also add the interrupt reset call. Thanks,
>
Thanks, Alex. It sounds a better way. I'll refactor the code
and send v2 out for your comments.
Thanks,
Gavin
>Alex
>
>> >> +
>> >> + break;
>> >
>> >Extraneous break
>> >
>>
>> Yep, I'll remove it.
>>
>> >> + }
>> >> +
>> >> break;
>> >> default:
>> >> /* Return an error on unknown requests */
>> >
>> >Thanks,
>> >Alex
>> >
>>
>> Thanks,
>> Gavin
>>
>
>
>
