qemu-ppc
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-ppc] [PATCH v2] spapr: add ibm, (get|set)-system-parameter


From: Alexander Graf
Subject: Re: [Qemu-ppc] [PATCH v2] spapr: add ibm, (get|set)-system-parameter
Date: Tue, 19 Nov 2013 11:29:42 +0100

On 19.11.2013, at 02:23, Alexey Kardashevskiy <address@hidden> wrote:

> On 11/19/2013 07:58 AM, Alexander Graf wrote:
>> 
>> On 17.11.2013, at 22:09, Alexey Kardashevskiy <address@hidden> wrote:
>> 
>>> This adds very basic handlers for ibm,get-system-parameter and
>>> ibm,set-system-parameter RTAS calls.
>>> 
>>> The only parameter handled at the moment is
>>> "platform-processor-diagnostics-run-mode" which is always disabled and
>>> does not support changing. This is expected to make
>>> "ppc64_cpu --run-mode=1" happy.
>>> 
>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>> ---
>>> Changes:
>>> v2:
>>> * addressed comments from Alex Graf
>>> ---
>>> hw/ppc/spapr_rtas.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 48 insertions(+)
>>> 
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index eb542f2..8053a67 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -224,6 +224,50 @@ static void rtas_stop_self(PowerPCCPU *cpu, 
>>> sPAPREnvironment *spapr,
>>>    env->msr = 0;
>>> }
>>> 
>>> +#define DIAGNOSTICS_RUN_MODE        42
>>> +
>>> +static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>>> +                                          sPAPREnvironment *spapr,
>>> +                                          uint32_t token, uint32_t nargs,
>>> +                                          target_ulong args,
>>> +                                          uint32_t nret, target_ulong rets)
>>> +{
>>> +    target_ulong papameter = rtas_ld(args, 0);
>>> +    target_ulong buffer = rtas_ld(args, 1);
>>> +    target_ulong length = rtas_ld(args, 2);
>>> +    target_ulong ret = -3; /* System parameter is not supported */
>> 
>> Is this an RTAS function defined return value or a global RTAS return value?
> 
> 
> Sorry for my ignorance, I do not understand the question. So far every RTAS
> call I looked at defined all return codes right in the description of the 
> call.

It's just a prelude to the suggestion further down.

> 
> Like this (sorry, cut-n-paste from PDF killed formatting but the point is
> still valid):
> 
> 7.3.16.2 ibm,set-system-parameter
> Table 96. ibm,set-system-parameter Argument Call Buffer
> Parameter Type
> Name
> Token
> Values
> Token for ibm,set-system-parameter
> Number Inputs
> In
> 2
> Number Outputs 1
> Parameter
> Token number of the target system parameter
> buffer
> Out
> Real address of data buffer
> Status 0: Success
>      -1: Hardware Error
>     -2: Busy, Try again later
>    -3: System parameter not supported
>   -9002: Setting not allowed/authorized
>  -9999: Parameter Error
> 990x:Extended delay where x is a number 0-5 (see text below)
> 
> 
> 
>>> +
>>> +    switch (papameter) {
>>> +    case DIAGNOSTICS_RUN_MODE:
>>> +        if (length == 1) {
>>> +            rtas_st(buffer, 0, 0);
>>> +            ret = 0; /* Success */
>> 
>> I assume this one is RTAS global?
>> 
>>> +        }
>>> +        break;
>>> +    }
>>> +
>>> +    rtas_st(rets, 0, ret);
>>> +}
>>> +
>>> +static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>> +                                          sPAPREnvironment *spapr,
>>> +                                          uint32_t token, uint32_t nargs,
>>> +                                          target_ulong args,
>>> +                                          uint32_t nret, target_ulong rets)
>>> +{
>>> +    target_ulong papameter = rtas_ld(args, 0);
>>> +    /* target_ulong buffer = rtas_ld(args, 1); */
>> 
>> Not addressed. Just remove this line until it gets used.
> 
> What is bad in having such a comment? I thought by having one return per
> function we help other people from the future to add functionality easier
> and such comment would help them too.

One return per function makes the code flow easier. It's only partly about 
future functionality.

I do see your point and I've usually written code the same way, until I had it 
rejected upstream. I'm honestly not sure which way is better. We have 3 choices 
here:

  1) Keep it as comment
  2) Add __attribute__((unused))
  3) Remove the line

I like option 1 the least. But if you insist on it I won't nack the patch 
because of it.

> 
> 
>>> +    target_ulong ret = -3; /* System parameter is not supported */
>>> +
>>> +    switch (papameter) {
>>> +    case DIAGNOSTICS_RUN_MODE:
>>> +        ret = -9002; /* Setting not allowed/authorized */
>> 
>> -9002 seems to always mean "Not Authorized".
>> 
>> In fact, reading through the PAPR spec it seems as if we can easily give
>> return values reasonable names:
>> 
>>  #define RTAS_OUT_SUCCESS 0
>>  #define RTAS_OUT_ERROR -1
>>  #define RTAS_OUT_BUSY -2
>>  #define RTAS_OUT_NOT_SUPPORTED -3
>>  #define RTAS_OUT_NOMEM -5
>>  #define RTAS_OUT_NOT_AUTHORIZED -9002
>>  #define RTAS_OUT_PARAM_ERROR -9999
> 
> 
> These are RTAS local, noone else really cares about them. The kernel uses
> numbers too (arch/powerpc/kernel/rtas.c). Why should I replace easy
> understandable (look at spec -> look at gdb -> everything is clear) numbers
> with definitions?

For the same reason that you use -ENOINT or -EINVAL in kernel code. For the 
same reason that you use H_SUCCESS rather than 0 as return value for hypercalls.

In general it makes the code easier to read for people who don't have the spec 
in front of them.

> 
> 
>> We can't always have a 1:1 map between name and number, but at least
>> name -> number should always be unique:
> 
>>  (ibm,configure-connector)
>>  #define RTAS_OUT_DR_CONN_UNUSABLE -9003
>>  #define RTAS_OUT_DR_CONN_NOT_SUPPORTED -9002
>>  #define RTAS_OUT_DR_SYSTEM_NOT_SUPPORTED -9001
>> 
>> Do you see cases where this wouldn't work out? That way we don't have to
>> have explicit comments in the code explaining what a return value really
>> means, making the code more easy to read.
> 
> 
> I need to skim the whole PAPR to make sure that this is the case and then
> skim the guest kernel to make sure it is not broken somewhere. And I do not

Why? The guest kernel doesn't care, does it? Worst case we call a return value 
by a name internally to QEMU that's not appropriate.

> see any profit from this job and it will definitely make my life harder as
> I really (really) want to see exact numbers in the code when I debug binary
> interface.

You still see them, no?

> However you can always ignore my comments and say "do what I say" and I'll
> do, no big deal. Thanks.

This is review that should've happened back when the first RTAS code got in. 
I'm not trying to make your life hard, I'm trying to make sure we have code 
that's readable and maintainable by non-Alex(ey)s :).


Alex




reply via email to

[Prev in Thread] Current Thread [Next in Thread]