Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/4] spapr: Fix RTAS sysparm DIAGNOSTICS_RUN_MODE

[Alexander Graf]

On 25.06.14 05:54, Sam Bobroff wrote:
> This allows the ibm,get-system-parameter RTAS call to succeed for the
> DIAGNOSTICS_RUN_MODE system parameter.
>
> The problem can be seen with "ppc64_cpu --run-mode" from the
> powerpc-utils package which fails before this patch with "Machine does
> not support diagnostic run mode".
>
> This is corrected by using the rtas_st_buffer() function to write to
> the buffer.
>
> The RTAS constants are also moved out into a header file, some new
> constants added and the surrounding code slightly simplified.
>
> Signed-off-by: Sam Bobroff <address@hidden>
> ---
>   hw/ppc/spapr_rtas.c    | 18 +++++++++---------
>   include/hw/ppc/spapr.h | 13 +++++++++++++
>   2 files changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index ea4a2b2..d3b57a1 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -224,8 +224,6 @@ 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,
> @@ -235,16 +233,18 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>       target_ulong parameter = rtas_ld(args, 0);
>       target_ulong buffer = rtas_ld(args, 1);
>       target_ulong length = rtas_ld(args, 2);
> -    target_ulong ret = RTAS_OUT_NOT_SUPPORTED;
> +    target_ulong ret = RTAS_OUT_SUCCESS;
>   
>       switch (parameter) {
> -    case DIAGNOSTICS_RUN_MODE:
> -        if (length == 1) {
> -            rtas_st(buffer, 0, 0);
> -            ret = RTAS_OUT_SUCCESS;
> -        }
> +    case RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE: {
> +        uint8_t param_val = DIAGNOSTICS_RUN_MODE_DISABLED;
> +
> +        rtas_st_buffer(buffer, length, &param_val, sizeof(param_val));
>           break;
>       }
> +    default:
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +    }
>   
>       rtas_st(rets, 0, ret);
>   }
> @@ -259,7 +259,7 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>       target_ulong ret = RTAS_OUT_NOT_SUPPORTED;
>   
>       switch (parameter) {
> -    case DIAGNOSTICS_RUN_MODE:
> +    case RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE:
>           ret = RTAS_OUT_NOT_AUTHORIZED;
>           break;
>       }
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 088baba..9b28e96 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -358,6 +358,19 @@ static inline int spapr_allocate_lsi(int hint)
>   #define RTAS_OUT_NOT_SUPPORTED      -3
>   #define RTAS_OUT_NOT_AUTHORIZED     -9002
>   
> +/* RTAS ibm,get-system-parameter token values */
> +#define RTAS_SYSPARM_DIAGNOSTICS_RUN_MODE        42
> +
> +/* Possible values for the platform-processor-diagnostics-run-mode parameter
> + * of the RTAS ibm,get-system-parameter call.
> + * (Only _DISABLED is currently used. The rest are the possible values defined
> + * by SPAPR.)

Comments like these are incredibly hard to keep in sync. When another 
patch adds support to return a different value, it's very likely nobody 
updates the comment above because it's out of scope.

So I'll remove the 2 lines above while applying the patch.


Alex

> + */
> +#define DIAGNOSTICS_RUN_MODE_DISABLED  0
> +#define DIAGNOSTICS_RUN_MODE_STAGGERED 1
> +#define DIAGNOSTICS_RUN_MODE_IMMEDIATE 2
> +#define DIAGNOSTICS_RUN_MODE_PERIODIC  3
> +
>   static inline uint64_t ppc64_phys_to_real(uint64_t addr)
>   {
>       return addr & ~0xF000000000000000ULL;