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

[Alexander Graf]

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?

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