From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48847) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YawJG-0006eY-0y for qemu-devel@nongnu.org; Wed, 25 Mar 2015 21:10:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YawJB-00045j-E1 for qemu-devel@nongnu.org; Wed, 25 Mar 2015 21:10:09 -0400 Date: Thu, 26 Mar 2015 12:10:52 +1100 From: David Gibson Message-ID: <20150326011052.GB28039@voom.redhat.com> 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> <20150324065402.GJ25043@voom.fritz.box> <1427201609.3643.730.camel@redhat.com> <20150326005348.GA15928@shangw> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="1LKvkjL3sHcu1TtY" Content-Disposition: inline In-Reply-To: <20150326005348.GA15928@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 , qemu-ppc@nongnu.org, qemu-devel@nongnu.org --1LKvkjL3sHcu1TtY Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Mar 26, 2015 at 11:53:48AM +1100, Gavin Shan wrote: > On Tue, Mar 24, 2015 at 06:53:29AM -0600, Alex Williamson wrote: > >On Tue, 2015-03-24 at 17:54 +1100, David Gibson wrote: > >> 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 QEM= U, 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 th= at 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 sep= arate > >> > >> >> >> 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 si= de effect > >> > >> >> >from a function whose name suggest's it's just a trivial wrap= per > >> > >> >> >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 implem= eneted > >> > >> >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 wrappe= r. > >> > >> > > >> > >>=20 > >> > >> Indeed, vfio_container_ioctl() isn't indicating what the function= is doing. > >> > >> How about renaming it to vfio_container_event_and_ioctl()? I'm al= ways bad > >> > >> at giving a good function name :) > >> > > > >> > >Well, I don't think your wrapper should be multiplexed. The multip= lex > >> > >works for the simple ioctl() wrapper, because there really is nothi= ng > >> > >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 oth= er > >> > >functions. > >> > > > >> >=20 > >> > It wouldn't a good idea if I understand your proposal correctly. Cur= rnetly, > >> > the global function vfio_container_ioctl() can be called from sPAPR = platform > >> > for any ioctl commands handled in kernel source file vfio_iommu_spap= r_tce.c, > >> > which means the function isn't called for EEH only. Other sPAPR TCE = container > >> > ioctl commands are also routed by this function. There will be lots = if having > >> > one global function for each ioctl commands, which just improve the = cost to > >> > maintain the code. > >>=20 > >> 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. > > > >vfio_container_ioctl() also filters to a limited set of ioctls, it > >clearly does not allow any ioctl. > > >=20 > Ok. I think your guys expect something like follows. Note that the follow= ing > vfio_container_eeh_ioctl() will accept a limited set of EEH operations, s= imilar > to what's doing in vfio_contain_ioctl() to the ioctl commands: >=20 > If you agree to have the changes, I'll put another patch on top of this o= ne > to replace vfio_container_ioctl() in spapr_pci_vfio.c with vfio_container= _eeh_ioctl() > for EEH cases. >=20 > int vfio_container_eeh_ioctl(AddressSpace *as, int32_t groupid, > struct vfio_eeh_pe_op *op) > { > switch (op->op) { > case VFIO_EEH_PE_RESET_HOT: > case VFIO_EEH_PE_RESET_FUNDAMENTAL: { > VFIOGroup *group; > VFIODevice *vbasedev; > VFIOPCIDevice *vdev; >=20 > /* > * 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. > */ > group =3D vfio_get_group(groupid, as); > if (!group) { > error_report("vfio: group %d not found\n", groupid); > return -1; > } >=20 > QLIST_FOREACH(vbasedev, &group->device_list, next) { > vdev =3D container_of(vbasedev, VFIOPCIDevice, vbasedev); > if (msix_enabled(&vdev->pdev)) { > vfio_disable_msix(vdev); > } >=20 > msix_reset(&vdev->pdev); > } >=20 > vfio_put_group(group); >=20 > break; > } > case VFIO_EEH_PE_DISABLE: > case VFIO_EEH_PE_ENABLE: > case VFIO_EEH_PE_UNFREEZE_IO: > case VFIO_EEH_PE_UNFREEZE_DMA: > case VFIO_EEH_PE_GET_STATE: > case VFIO_EEH_PE_RESET_DEACTIVATE: > case VFIO_EEH_PE_CONFIGURE: > break; > default: > error_report("vfio: unsupported EEH operation %X\n", op->op); > return -1; > } >=20 > return vfio_container_ioctl(as, groupid, VFIO_EEH_PE_OP, op); > } No, extra operation specific logic inside the ioctl wrapper is exactly what I want to avoid. Instead I want to see vfio_container_eeh_ioctl() remain as it is now - doing nothing but verifying the ioctl() number, then passing the arguments on to ioctl(). What I'm expecting is then to add a new functions, along the lines of: int vfio_eeh_pe_reset(...) { VFIOGroup *group; VFIODevice *vbasedev; VFIOPCIDevice *vdev; /* * 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. */ group =3D vfio_get_group(groupid, as); if (!group) { error_report("vfio: group %d not found\n", groupid); return -1; } QLIST_FOREACH(vbasedev, &group->device_list, next) { vdev =3D container_of(vbasedev, VFIOPCIDevice, vbasedev); if (msix_enabled(&vdev->pdev)) { vfio_disable_msix(vdev); } msix_reset(&vdev->pdev); } vfio_put_group(group); return vfio_eeh_container_ioctl(as, groupid, VFIO_EEH_PE_RESET_FUNDAMENTAL, op); } I this function can build the op structure itself from sensible arguments, then that's even better. --=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 --1LKvkjL3sHcu1TtY Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVE1ycAAoJEGw4ysog2bOS4wQQAN1J7KQACSKsqJQJfZAyxdcc 8MctI7y0Z2TMDiMlMmQiD3RH9m9Hfnsjf3yEJFps0pKCxy9Q58s5lTWj5B+wUc3d C8amfWubawXehN/xmDLSM2XzvDEvFpA0pbsoxndk/hPBe7yWgdbWVdssyIb7afrA YzWDimjFnQYdUCCsgU8YgN4pcDEJcE+fjUhgC2vmzdgEcC0lXOhUbQ0dF3kihp5u FqGo+UrQmxA9nJKdwxskoyV0hFqEnMre3Fx07ZNW/iwLMotHvBpQnevsGrvJVX36 Rrj3T5GqN658Fi4mMKVEy66x1HD9RX6d6vIeMvO+aqdZthV0DxPkJC2Er5ks25ZB FktMne3EyWquWX2FPVSPq8jQ8g5LYH4VEydUN2Es20gY4pVZXFSf1kyx8f8SjZ9s YK92W6hlA4arKkeBPFkU7XEweYzUlDuWr7++91dzYn/v4maHu9zvIJaqt6hv2xCk ruN+cjbmcMen5rr/WgiXJDVzn696iwpOaGO0/2MAHe3pnCwEMoW8V6xet9LQ9bCB m9CRWKz9NqeN+aWAvO7dXXQZmQLdcKSARFXPJ331edULPlEvjsX7BiRq3OBGqhzU 2bO4Hz1KcCjSAcY8T1pAlyL0JeurUPOgkjweLWxrFEEoV5WbKxwSivnSAWWXhCQt aPQ3T9Q5UEalfZOVdSio =MAq2 -----END PGP SIGNATURE----- --1LKvkjL3sHcu1TtY--