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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

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 <gwshan@linux.vnet.ibm.com>
> ---
>  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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]