From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34510) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBalb-00063i-2A for qemu-devel@nongnu.org; Wed, 14 Jan 2015 22:06:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBalW-0002Wf-H3 for qemu-devel@nongnu.org; Wed, 14 Jan 2015 22:06:38 -0500 Date: Thu, 15 Jan 2015 12:33:59 +1100 From: David Gibson Message-ID: <20150115013359.GC5297@voom.fritz.box> References: <1420417588-32562-1-git-send-email-gwshan@linux.vnet.ibm.com> <1420417588-32562-2-git-send-email-gwshan@linux.vnet.ibm.com> <20150114013935.GQ3654@voom.BigPond> <20150115001436.GA18267@shangw> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="sHrvAb52M6C8blB9" Content-Disposition: inline In-Reply-To: <20150115001436.GA18267@shangw> Subject: Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Gavin Shan Cc: aik@ozlabs.ru, alex.williamson@redhat.com, qemu-ppc@nongnu.org, qemu-devel@nongnu.org, agraf@suse.de --sHrvAb52M6C8blB9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 15, 2015 at 11:14:36AM +1100, Gavin Shan wrote: > On Wed, Jan 14, 2015 at 12:39:35PM +1100, David Gibson wrote: > >On Mon, Jan 05, 2015 at 11:26:27AM +1100, Gavin Shan wrote: > >> The emulation for EEH RTAS requests from guest isn't covered > >> by QEMU yet and the patch implements them. > >>=20 > >> The patch defines constants used by EEH RTAS calls and adds > >> callback sPAPRPHBClass::eeh_handler, which is going to be used > >> this way: > >>=20 > >> * RTAS calls are received in spapr_pci.c, sanity check is done > >> there. > >> * RTAS handlers handle what they can. If there is something it > >> cannot handle and sPAPRPHBClass::eeh_handler callback is defined, > >> it is called. > >> * sPAPRPHBClass::eeh_handler is only implemented for VFIO now. It > >> does ioctl() to the IOMMU container fd to complete the call. Error > >> codes from that ioctl() are transferred back to the guest. > >>=20 > >> [aik: defined RTAS tokens for EEH RTAS calls] > >> Signed-off-by: Gavin Shan > >> --- > >> hw/ppc/spapr_pci.c | 275 +++++++++++++++++++++++++++++++++++= +++++++++ > >> include/hw/pci-host/spapr.h | 7 ++ > >> include/hw/ppc/spapr.h | 43 ++++++- > >> 3 files changed, 323 insertions(+), 2 deletions(-) > >>=20 > >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c > >> index 21b95b3..a150074 100644 > >> --- a/hw/ppc/spapr_pci.c > >> +++ b/hw/ppc/spapr_pci.c > >> @@ -406,6 +406,262 @@ static void rtas_ibm_query_interrupt_source_numb= er(PowerPCCPU *cpu, > >> rtas_st(rets, 2, 1);/* 0 =3D=3D level; 1 =3D=3D edge */ > >> } > >> =20 > >> +static int rtas_handle_eeh_request(sPAPREnvironment *spapr, > >> + uint64_t buid, uint32_t req, uint3= 2_t opt) > >> +{ > >> + sPAPRPHBState *sphb =3D find_phb(spapr, buid); > >> + sPAPRPHBClass *info; > >> + > >> + if (!sphb) { > >> + return -ENODEV; > > > >I think it would make more sense to return RTAS error codes here, > >rather than errnos. At present all the callers seem to ignore the > >exact value of this return value. > > > >But it's not really correct to return RTAS_OUT_HW_ERROR for a bad > >BUID, which is what this will do now. > > >=20 > It makes sense: RTAS_OUT_PARAM_ERROR, instead of RTAS_OUT_HW_ERROR > should be returned for invalid sPAPRPHBState and sPAPRPHBClass (as below). >=20 > It's a bit hard to have RTAS_OUT_* as the function's return value because > RTAS_OUT_PARAM_ERROR should be returned for some RTAS calls even info->ee= h_handler() > returns negative value. Um.. I don't quite see why that makes returning RTAS erorr values difficult. But I think it's made irrelevant by your suggestion later. > >Also several of the callers have already done a find_phb() by the time > >they call this. Perhaps it would make more sense for this function to > >take a sPAPRPHBState * instead of the buid. > > >=20 > It's not sure that find_phb() called by callers() before calling this > function. We did call find_dev() before calling this function for some > cases. How about changing the code like this way: Drop rtas_handle_eeh_re= quest() > and put the logic into its callers, which would give more flexibility for > the callers to return proper values. Yes, I think that might be the best idea, rtas_handle_eeh_request() seems like a bit of an odd multiplexer at the moment. [snip] > >The ret < 0 case isn't handled here. It will fall through to > >param_error_exit, which is non-obvious, and it also seems unlikely > >that a parameter error is the only possible thing that can go wrong. > > >=20 > Yeah, PAPR spec states the return value RTAS_OUT_SUCCESS or > RTAS_OUT_PARAMETER_ERROR. There is no RTAS_OUT_HW_ERROR for > this RTAS call "ibm,read-slot-reset-state2". Heh, ok. I think you still want to make the fall-through more obvious, it's easy to miss. --=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 --sHrvAb52M6C8blB9 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJUtxkHAAoJEGw4ysog2bOSZZ8QAMawNWy69jKvaCk3jYKavFiO SLkUmg3ApL21N6U9UptFLOaDyyviBpry7o9o5ywMiB5MMfqEZxZ3Y0fK74QCkVH9 JwI/zFTxPrQOjCXNRwLBddOGOvJhEc+syeuaSevF/MbNkCZH82P4r4oPNRCHfXJh f9obXIDiUfcTIgHe3hpg49oz5sCtUxBoEUrxt2/2moR6TggFt42LW0u3qNCCQ8Ch b/cYltxQvOYU0NiCP5vQFlkHTM5tX32fpjAozALycGBMAwZNpQVfplbQI2LlFmzi TIza4+HkzOQhIvtXH7Ef7ph03NQrqppMRHszvueWqn/0NElJFLtFvYpBxG34CPwd D6PeowDoCLqHSFz7KaCPRv3CUsmi80gfcSloNjwMke9jjGGKhFbexbLZyqfPeNTp us2l7rnR65xxg6cIIEsR9KlqsbnH6ufY7drUV6Hmt1iPldevbAD0Bou7AshN/Wio 6dmiIwxBxV01z75JuGbpqjabiVR5TPBh4mPneClAug/q9ae8XN5PtTRxpZO6Caas W8DLWtMt3o4RMFWMN89wyXdb6soNAie3MbWEktUO+DDrL9fcHZ8quMiyH1sZO1Vc DYmqGXQDr7yJBDWkYgtaPJQKCuvKr3CBU5m2PLAVIy/aFqyXn8czkB9TrRLo1Nn9 HHxnAZnu8EdZtpQWzWZD =ZlBc -----END PGP SIGNATURE----- --sHrvAb52M6C8blB9--