Re: [Qemu-devel] [PATCH 2/3] VFIO: Clear INTx pending state on EEH reset

[Gavin Shan]

On Thu, Mar 12, 2015 at 12:48:16PM +1100, David Gibson wrote:
>On Wed, Mar 11, 2015 at 05:11:53PM +1100, Gavin Shan wrote:
>> When Linux guest recovers from EEH error on the following Emulex
>> adapter, the MSIx interrupts are disabled and the INTx emulation
>> is enabled. One INTx interrupt is injected to the guest by host
>> because of detected pending INTx interrupts on the adapter. QEMU
>> disables mmap'ed BAR regions and starts a timer to enable those
>> regions at later point the INTx interrupt handler. Unfortunately,
>> "VFIOPCIDevice->intx.pending" isn't cleared, meaning those disabled
>> mapp'ed BAR regions won't be reenabled properly. It leads to EEH
>> recovery failure at guest side because of hanged MMIO access.
>> 
>>  # lspci | grep Emulex
>>  0000:01:00.0 Ethernet controller: Emulex Corporation \
>>               OneConnect 10Gb NIC (be3) (rev 02)
>>  0000:01:00.1 Ethernet controller: Emulex Corporation \
>>               OneConnect 10Gb NIC (be3) (rev 02)
>> 
>> The patch clears "VFIOPCIDevice->intx.pending" after EEH reset
>> is completed on the PE, which contains the adapter. In turn, the
>> mmap'ed BAR regions can be reenabled to avoid EEH recovery failure.
>> 
>> Signed-off-by: Gavin Shan <address@hidden>
>> ---
>>  hw/vfio/pci.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>> 
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index 8c4a8cb..55e0904 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -3352,6 +3352,20 @@ int vfio_container_eeh_event(AddressSpace *as, 
>> int32_t groupid,
>>          }
>>  
>>          break;
>> +    case VFIO_EEH_PE_RESET_DEACTIVATE:
>> +        /*
>> +         * We might have INTx interrupt whose handler disabled the
>> +         * memory mapped BARs. Without clearing the INTx pending
>> +         * state, the timer kicked by the INTx interrupt handler
>> +         * won't enable those disabled memory mapped BARs, which
>> +         * leads EEH recovery failure.
>> +         */
>> +        QLIST_FOREACH(vbasedev, &group->device_list, next) {
>> +            vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +            vdev->intx.pending = false;
>> +        }
>> +
>> +        break;
>>      }
>>  
>>      vfio_put_group(group);
>
>I'm not sure that invoking these side effects from the low-level
>ioctl() wrapper makes a lot of sense.  Wouldn't it be clearer to
>explicitly do the necessary state cleanup in the reset callers.
>

Yes, I agree that putting this into reset caller can help isolating
PowerPC unique EEH code from general code. The only problem I have is
"struct VFIOPCIDevice" is only visible in hw/vfio/pci.c

Thanks,
Gavin

>-- 
>David Gibson                   | I'll have my music baroque, and my code
>david AT gibson.dropbear.id.au | minimalist, thank you.  NOT _the_ _other_
>                               | _way_ _around_!
>http://www.ozlabs.org/~dgibson