From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:40169) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBbA3-0004Pj-7k for qemu-devel@nongnu.org; Wed, 14 Jan 2015 22:31:56 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YBb9v-0004YK-Pm for qemu-devel@nongnu.org; Wed, 14 Jan 2015 22:31:55 -0500 Received: from e23smtp09.au.ibm.com ([202.81.31.142]:50727) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YBb9v-0004XL-5m for qemu-devel@nongnu.org; Wed, 14 Jan 2015 22:31:47 -0500 Received: from /spool/local by e23smtp09.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 15 Jan 2015 13:21:29 +1000 Date: Thu, 15 Jan 2015 14:21:22 +1100 From: Gavin Shan Message-ID: <20150115032122.GA17053@shangw> 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> <20150115013359.GC5297@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150115013359.GC5297@voom.fritz.box> Subject: Re: [Qemu-devel] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls Reply-To: Gavin Shan List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: qemu-devel@nongnu.org, aik@ozlabs.ru, agraf@suse.de, Gavin Shan , alex.williamson@redhat.com, qemu-ppc@nongnu.org On Thu, Jan 15, 2015 at 12:33:59PM +1100, David Gibson wrote: >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. >> >> >> >> The patch defines constants used by EEH RTAS calls and adds >> >> callback sPAPRPHBClass::eeh_handler, which is going to be used >> >> this way: >> >> >> >> * 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. >> >> >> >> [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(-) >> >> >> >> 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_number(PowerPCCPU *cpu, >> >> rtas_st(rets, 2, 1);/* 0 == level; 1 == edge */ >> >> } >> >> >> >> +static int rtas_handle_eeh_request(sPAPREnvironment *spapr, >> >> + uint64_t buid, uint32_t req, uint32_t opt) >> >> +{ >> >> + sPAPRPHBState *sphb = 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. >> > >> >> It makes sense: RTAS_OUT_PARAM_ERROR, instead of RTAS_OUT_HW_ERROR >> should be returned for invalid sPAPRPHBState and sPAPRPHBClass (as below). >> >> 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->eeh_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. > Yeah, it's irrelevant after rtas_handle_eeh_request() is dropped. >> >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. >> > >> >> 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_request() >> 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. > Thanks for confirm. I'll change code accordingly in next revision. >[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. >> > >> >> 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. > Yes, I still need make it more obvious like this: int ret; ret = info->eeh_handler(); #if RTAS_CALL_SUPPORTS_HW_ERROR_AS_RETURN_VALUE if (ret < 0) { rtas_st(rets, 0, RTAS_OUT_HW_ERROR); return; } #else if (ret < 0) { goto param_error_exit; } #endif /* Handle the result from info->eeh_handler() */ rtas_st(rets, 0, RTAS_OUT_SUCCESS); return; param_error_exit: rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR); Thanks, Gavin >-- >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