From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:37139) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaHZp-0007jE-5d for qemu-devel@nongnu.org; Tue, 24 Mar 2015 01:40:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaHZn-0006b7-Pf for qemu-devel@nongnu.org; Tue, 24 Mar 2015 01:40:33 -0400 Date: Tue, 24 Mar 2015 16:41:21 +1100 From: David Gibson Message-ID: <20150324054121.GF25043@voom.fritz.box> References: <1426523486-9794-1-git-send-email-gwshan@linux.vnet.ibm.com> <20150320060401.GY5741@voom.redhat.com> <20150320062729.GA13024@shangw> <20150323050656.GR25043@voom.fritz.box> <20150323052510.GA9147@shangw> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="QN0KHc3254GHvltB" Content-Disposition: inline In-Reply-To: <20150323052510.GA9147@shangw> Subject: Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 1/3] VFIO: Clear stale MSIx table during EEH reset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan Cc: alex.williamson@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org --QN0KHc3254GHvltB Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 23, 2015 at 04:25:10PM +1100, Gavin Shan wrote: > On Mon, Mar 23, 2015 at 04:06:56PM +1100, David Gibson wrote: > >On Fri, Mar 20, 2015 at 05:27:29PM +1100, Gavin Shan wrote: > >> On Fri, Mar 20, 2015 at 05:04:01PM +1100, David Gibson wrote: > >> >On Tue, Mar 17, 2015 at 03:31:24AM +1100, 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. > >> >>=20 > >> >> The patch clears stale MSIx table before EEH PE reset so that MSIx > >> >> table could be restored properly after EEH PE reset. > >> >>=20 > >> >> Signed-off-by: Gavin Shan > >> >> --- > >> >> v2: vfio_container_eeh_event() stub for !CONFIG_PCI and separate > >> >> error message for this function. Dropped vfio_put_group() > >> >> on NULL group > >> >> --- > >> >> hw/vfio/Makefile.objs | 6 +++++- > >> >> hw/vfio/common.c | 7 +++++++ > >> >> hw/vfio/pci-stub.c | 17 +++++++++++++++++ > >> >> hw/vfio/pci.c | 38 ++++++++++++++++++++++++++++++++++++++ > >> >> include/hw/vfio/vfio.h | 2 ++ > >> >> 5 files changed, 69 insertions(+), 1 deletion(-) > >> >> create mode 100644 hw/vfio/pci-stub.c > >> >>=20 > >> >> diff --git a/hw/vfio/Makefile.objs b/hw/vfio/Makefile.objs > >> >> index e31f30e..1b8a065 100644 > >> >> --- a/hw/vfio/Makefile.objs > >> >> +++ b/hw/vfio/Makefile.objs > >> >> @@ -1,4 +1,8 @@ > >> >> ifeq ($(CONFIG_LINUX), y) > >> >> obj-$(CONFIG_SOFTMMU) +=3D common.o > >> >> -obj-$(CONFIG_PCI) +=3D pci.o > >> >> +ifeq ($(CONFIG_PCI), y) > >> >> +obj-y +=3D pci.o > >> >> +else > >> >> +obj-y +=3D pci-stub.o > >> >> +endif > >> >> endif > >> >> diff --git a/hw/vfio/common.c b/hw/vfio/common.c > >> >> index 148eb53..ed07814 100644 > >> >> --- a/hw/vfio/common.c > >> >> +++ b/hw/vfio/common.c > >> >> @@ -949,7 +949,14 @@ int vfio_container_ioctl(AddressSpace *as, int= 32_t groupid, > >> >> switch (req) { > >> >> case VFIO_CHECK_EXTENSION: > >> >> case VFIO_IOMMU_SPAPR_TCE_GET_INFO: > >> >> + break; > >> >> case VFIO_EEH_PE_OP: > >> >> + if (vfio_container_eeh_event(as, groupid, param) !=3D 0) { > >> > > >> >I really dislike the idea of having an arbitrarily complex side effect > >> >from a function whose name suggest's it's just a trivial wrapper > >> >around the ioctl(). > >> > > >>=20 > >> Ok. I guess you would like putting the complex in the callers of > >> vfio_container_ioctl(). > > > >Well.. maybe. I'd also be happy if helper functions were implemeneted > >which both called the ioctl() and did the other necessary pieces. > >They should just be called something that indicates their full > >function, not a name which suggests they're just an ioctl wrapper. > > >=20 > Indeed, vfio_container_ioctl() isn't indicating what the function is doin= g. > How about renaming it to vfio_container_event_and_ioctl()? I'm always bad > at giving a good function name :) Well, I don't think your wrapper should be multiplexed. The multiplex works for the simple ioctl() wrapper, because there really is nothing that varies apart from the exact ioctl number called. But now that you have different operations here, I think you want wrappers for each one - each one will call the ioctl(), then do the specific extra steps necessary for that operation. So vfio_container_event() will go away as well, split into various other functions. --=20 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 --QN0KHc3254GHvltB Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEPkBAAoJEGw4ysog2bOSDe0P/0KUjftDDW85JI1Sw34Q3RCx NdKSk1huQbttZKHvcjCmQ8oXaCTS2L05+4/l1ltUS2EiLqseMDQuXKLncD2RnhiD cQ+U3uX1YeBakMhJPZkYIGs5MzLFGmPjTJyMnzm71UwOQPTY0yFSrePhP7YcPu48 uM4+Qt3IyZSR+1Bk9TQDF+pHrXWsAUm+ms7LzWQbwOWLoSuLsivDOiaDpt7jYIia 9EOrW5S9xUl+wPmcuPvpar4dgyYqnX42KD9K/ofsxO2SOUKsTU2X7d6X0mJiXQyX UGHkN00s5iZnCtqCUWEo4p0Y2M+4kgpadM0EQfqXcWwzJ6dALOgXLFFNaCnwIjfW uvh2GjON/avsVIck4bNVESnHgyFAuq1KfWrE+BN0gABAmi8M8HBFN4WIA8DZZobL 69ycZMWnPfiLkuYz1Y5wHtemiO7zMvSSkhe82+cDs9TS0Zis0BKXggO3WLrjaHfO 1gCkmSRzJrdpUJuUQzJPkac4EB/3VNQzHNJKlmY1iMfxlzUOQ1w3HEAUxbJxBYBM E67pLoE/LWqcv3KsPKVEJKjjHWNlElBhBSx5SFdCKArRlwUDmPigwNaYu0V97bRk mxHDG4pCpAo+6xx0EOzR8t2OYkM/jqyop0tgSRQU/ti06ho28dUA8CGPKxRXWXy7 esZV4GdKRH4X3W0DCCe5 =Bw5Y -----END PGP SIGNATURE----- --QN0KHc3254GHvltB--