Re: [Qemu-devel] [PATCH 1/1] target-ppc: Adding Functionality to rtas_ibm_get_system_parameter.

["Andreas Färber" <afaerber@suse.de>]

Hi,

Am 14.03.2014 17:10, schrieb Tomo Berry:
>  This patch adds the functionality for 
>  rtas_ibm_get_system_parameter to return a string containing 
>  the values for partition_max_entitled_capacity and 
>  system_potential_processors.

Unintentionally indented?

> 
> Signed-off-by: Tomo Berry <tbberry@us.ibm.com>
> ---
>  hw/ppc/spapr_rtas.c    | 34 ++++++++++++++++++++++++++++++++++
>  include/hw/ppc/spapr.h | 12 ++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index 1cb276d..931ba06 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -225,6 +225,9 @@ static void rtas_stop_self(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>  }
>  
>  #define DIAGNOSTICS_RUN_MODE        42
> +#define SPLPAR_CHARACTERISTICS_TOKEN 20
> +#define CMO_CHARACTERISTICS_TOKEN 44
> +#define CEDE_LATENCY_TOKEN 45
>  
>  static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>                                            sPAPREnvironment *spapr,
> @@ -236,6 +239,7 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>      target_ulong buffer = rtas_ld(args, 1);
>      target_ulong length = rtas_ld(args, 2);
>      target_ulong ret = RTAS_OUT_NOT_SUPPORTED;
> +    char *local_buffer;
>  
>      switch (parameter) {
>      case DIAGNOSTICS_RUN_MODE:
> @@ -244,6 +248,36 @@ static void rtas_ibm_get_system_parameter(PowerPCCPU *cpu,
>              ret = RTAS_OUT_SUCCESS;
>          }
>          break;
> +    case SPLPAR_CHARACTERISTICS_TOKEN:
> +
> +        /*  Create a string of length bytes locally to copy to buffer  */
> +
> +        local_buffer = calloc(length, 1);

Please use GLib allocation functions exclusively, e.g. g_malloc(length),
in particular with regards to handling out-of-memory situations.
calloc() might return NULL, g_{malloc,new}() will abort.

> +
> +        /*  These are the current system parameters supported.  The first
> +         *  16 bits of the buffer must contain the length of the string.
> +         *  These 16 bits are not included in the length of the string. */

            */ is preferred to make clear where the comment ends.

> +
> +        ((uint16_t *)local_buffer)[0] = cpu_to_be16(snprintf(local_buffer + 2,
> +             length - 2, "MaxEntCap=%d,MaxPlatProcs=%d", max_cpus, smp_cpus));
> +
> +        /*  Copy the string into buffer using rtas_st_buffer to
> +         *  convert the addresses. */

Is this comment really needed? :)

> +
> +        rtas_st_buffer(buffer, length, (uint8_t *)local_buffer);
> +
> +        free(local_buffer);

g_free() then.

> +        ret = RTAS_OUT_SUCCESS;
> +        break;
> +    case CMO_CHARACTERISTICS_TOKEN:
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +        break;
> +    case CEDE_LATENCY_TOKEN:
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +        break;
> +    default:
> +        ret = RTAS_OUT_NOT_SUPPORTED;
> +        break;
>      }
>  
>      rtas_st(rets, 0, ret);
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index b2f11e9..ee6ed2d 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -356,6 +356,18 @@ static inline void rtas_st(target_ulong phys, int n, uint32_t val)
>      stl_be_phys(ppc64_phys_to_real(phys + 4*n), val);
>  }
>  
> +/*  This function will store a buffer 1 byte at a time into the
> + *  address at phys up to a maximum of phys_len bytes.*/
> +
> +static inline void rtas_st_buffer(target_ulong phys,
> +                                  target_ulong phys_len,
> +                                  uint8_t *buffer) {

{ on new line please.

> +    uint32_t i;
> +    for (i = 0; i < phys_len; i++) {
> +        stb_phys(ppc64_phys_to_real(phys + i), buffer[i]);
> +    }
> +}
> +
>  typedef void (*spapr_rtas_fn)(PowerPCCPU *cpu, sPAPREnvironment *spapr,
>                                uint32_t token,
>                                uint32_t nargs, target_ulong args,

Am I interpreting correctly that this is 2.1 material?

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg