Re: [Qemu-ppc] [PATCH v15 1/2] sPAPR: Implement EEH RTAS calls

[Gavin Shan]

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 <address@hidden>
>> >> ---
>> >>  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