From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50657) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YaIiC-0003ef-CU for qemu-devel@nongnu.org; Tue, 24 Mar 2015 02:53:17 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YaIiA-0003pG-R4 for qemu-devel@nongnu.org; Tue, 24 Mar 2015 02:53:16 -0400 Date: Tue, 24 Mar 2015 17:54:02 +1100 From: David Gibson Message-ID: <20150324065402.GJ25043@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> <20150324054121.GF25043@voom.fritz.box> <20150324062455.GA21610@shangw> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="94Ornb/7sD1MvElF" Content-Disposition: inline In-Reply-To: <20150324062455.GA21610@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 --94Ornb/7sD1MvElF Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Mar 24, 2015 at 05:24:55PM +1100, Gavin Shan wrote: > On Tue, Mar 24, 2015 at 04:41:21PM +1100, David Gibson wrote: > >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, wh= ich > >> >> >> should be cleared accordingly. Otherwise, we will run into anoth= er > >> >> >> (recursive) EEH error and the PCI devices contained in the PE ha= ve > >> >> >> to be offlined exceptionally. > >> >> >>=20 > >> >> >> The patch clears stale MSIx table before EEH PE reset so that MS= Ix > >> >> >> 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, = int32_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 ef= fect > >> >> >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 d= oing. > >> 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 > It wouldn't a good idea if I understand your proposal correctly. Currnetl= y, > the global function vfio_container_ioctl() can be called from sPAPR platf= orm > for any ioctl commands handled in kernel source file vfio_iommu_spapr_tce= =2Ec, > which means the function isn't called for EEH only. Other sPAPR TCE conta= iner > ioctl commands are also routed by this function. There will be lots if ha= ving > one global function for each ioctl commands, which just improve the cost = to > maintain the code. I don't really follow your objection. I'm only suggesting separate wrappers for things which require extra actions currently implemented in vfio_container_event(). Things which only ned the plain ioctl() can still use the simple vfio_container_ioctl() wrapper. > Alternatively, we might expose another function vfio_container_eeh_ioctl(= ), > which calls vfio_container_ioctl() after doing what we did in vfio_contai= ner_event() > if necessary. >=20 > Thanks, > Gavin >=20 >=20 >=20 --=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 --94Ornb/7sD1MvElF Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVEQoKAAoJEGw4ysog2bOSe3MQALopBpID1PIfivt9htAfB5Sg 2x8YlIo+L5ydsxJLQzGH5GzHWN377MMeQmJtkGpi+okXgsGXdTFuJcXdLmfp3sji lxOsTAWAg7oss9lhksPrdun4jqKMbdfyQ1tS2gOh/UzRxE62n3FRfQwEmLWnOnbP rjLO1Zk+jlCcLZDX5z2VGBsH2V1+60/YEgfCfeDF4ozZWLSPa8Oxc3YMTy2507g3 IYgilFwqLyRjvPd0cSZyViENPNnKecIlYQLtivMKpud7c8VcxNnDqPoq2vAi21AV 37OQV9MtW+IHzxcCRj9w5XN4A3FmzzZzz6W/xT5TwONP6ZsX3bkuUi5Lxey9WI2a W67LHKKSDHNJXfQXfkFYr9REFT+u/jt6SLnA24hA6vu1+RXa047NmnFMEKX3Q9OO Wz+njpLBhUaMqUrD4ydyIN7xpsWaTDxSQHtwieR6huIgw+c642ajvG1tFIBFnjvg sewHPtw1X6AxESWF9XTlWISQXdZPEkkv6ialQMlC9Ipq3hTUkEUmdU33sgvIezVX MrtZJDePQ83ZurCRVIkWoZ498YpTSG/UM+OkCuBG+yteKEK/ATuec7adsFhf0w+L 1gxUseU/wEbjLtvr2cfoN4orF9I/wG3i2l10v2hDmoOVSnch2Gb71y8oaGMiLtya dtkxErEx+McrFcriZLys =M5hZ -----END PGP SIGNATURE----- --94Ornb/7sD1MvElF--