From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56552) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMd56-0003jL-CG for qemu-devel@nongnu.org; Wed, 27 Aug 2014 09:16:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XMd4x-0000fC-0R for qemu-devel@nongnu.org; Wed, 27 Aug 2014 09:16:08 -0400 Received: from e23smtp01.au.ibm.com ([202.81.31.143]:58481) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XMd4w-0000eu-Cc for qemu-devel@nongnu.org; Wed, 27 Aug 2014 09:15:58 -0400 Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Wed, 27 Aug 2014 23:15:51 +1000 Received: from d23relay07.au.ibm.com (d23relay07.au.ibm.com [9.190.26.37]) by d23dlp02.au.ibm.com (Postfix) with ESMTP id D845F2BB0047 for ; Wed, 27 Aug 2014 23:15:47 +1000 (EST) Received: from d23av04.au.ibm.com (d23av04.au.ibm.com [9.190.235.139]) by d23relay07.au.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id s7RDGhrY17760448 for ; Wed, 27 Aug 2014 23:16:44 +1000 Received: from d23av04.au.ibm.com (localhost [127.0.0.1]) by d23av04.au.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s7RDFkkS010751 for ; Wed, 27 Aug 2014 23:15:46 +1000 Date: Wed, 27 Aug 2014 23:15:41 +1000 From: Gavin Shan Message-ID: <20140827131541.GA5118@shangw> References: <1408528328-11560-1-git-send-email-gwshan@linux.vnet.ibm.com> <1408528328-11560-3-git-send-email-gwshan@linux.vnet.ibm.com> <1409084747.2906.147.camel@ul30vt.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1409084747.2906.147.camel@ul30vt.home> Subject: Re: [Qemu-devel] [RFC PATCH 2/2] VFIO: Clear stale MSIx table during EEH reset Reply-To: Gavin Shan List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Williamson Cc: aik@ozlabs.ru, Gavin Shan , qemu-devel@nongnu.org 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 >> --- >> 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) >> + >> + break; > >Extraneous break > Yep, I'll remove it. >> + } >> + >> break; >> default: >> /* Return an error on unknown requests */ > >Thanks, >Alex > Thanks, Gavin