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 <gws...@linux.vnet.ibm.com>
> >> ---
> >> 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.
> >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.
[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.
--
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
pgpfQZdyesRRw.pgp
Description: PGP signature
