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

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

[Alexey Kardashevskiy]

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 */
+
+    switch (papameter) {
+    case DIAGNOSTICS_RUN_MODE:
+        if (length == 1) {
+            rtas_st(buffer, 0, 0);
+            ret = 0; /* Success */
+        }
+        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); */
+    target_ulong ret = -3; /* System parameter is not supported */
+
+    switch (papameter) {
+    case DIAGNOSTICS_RUN_MODE:
+        ret = -9002; /* Setting not allowed/authorized */
+        break;
+    }
+
+    rtas_st(rets, 0, ret);
+}
+
 static struct rtas_call {
     const char *name;
     spapr_rtas_fn fn;
@@ -345,6 +389,10 @@ static void core_rtas_register_types(void)
                         rtas_query_cpu_stopped_state);
     spapr_rtas_register("start-cpu", rtas_start_cpu);
     spapr_rtas_register("stop-self", rtas_stop_self);
+    spapr_rtas_register("ibm,get-system-parameter",
+                        rtas_ibm_get_system_parameter);
+    spapr_rtas_register("ibm,set-system-parameter",
+                        rtas_ibm_set_system_parameter);
 }
 
 type_init(core_rtas_register_types)
-- 
1.8.4.rc4

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

[Alexander Graf]

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?

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

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

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.


Alex

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

[Alexey Kardashevskiy]

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

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

[Alexander Graf]

On 19.11.2013, at 02:23, Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

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

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