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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 11/19/2013 07:58 AM, Alexander Graf wrote:
> 
> On 17.11.2013, at 22:09, Alexey Kardashevskiy <aik@ozlabs.ru> 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 <aik@ozlabs.ru>
>> ---
>> 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.

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.


>> +    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?


> 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
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.


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


-- 
Alexey