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

[Greg Kurz <gkurz@linux.vnet.ibm.com>]

On Thu, 17 Apr 2014 10:33:44 -0500
Tomohiro B Berry <tbberry@us.ibm.com> wrote:

> Hi, 
> 
> I was just wondering if anything else needed to be done to get this pushed 
> upstream? 
> 
> Thanks,
> 
> Tomo
> 

Hi Tomo,

A good start would be to also Cc: qemu-ppc@nongnu.org : this is mandatory for
all PPC specific patches. Also, consider using the ./scripts/checkpatch.pl
tool before sending patches:

ERROR: trailing whitespace
#50: FILE: hw/ppc/spapr_rtas.c:259:
+         *  These 16 bits are not included in the length of the string. $

ERROR: trailing whitespace
#86: FILE: include/hw/ppc/spapr.h:364:
+                                  uint8_t *buffer) $

total: 2 errors, 0 warnings, 69 lines checked


> Tomohiro B Berry/Austin/IBM@IBMUS wrote on 03/20/2014 04:17:33 PM:
> 
> > From: Tomohiro B Berry/Austin/IBM@IBMUS
> > To: qemu-devel@nongnu.org, 
> > Cc: Tomohiro B Berry/Austin/IBM@IBMUS
> > Date: 03/20/2014 04:17 PM
> > Subject: [PATCH 1/1 V2] target-ppc: Adding Functionality to 
> > rtas_ibm_get_system_parameter.
> > 
> > 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.
> > 
> > Signed-off-by: Tomo Berry <tbberry@us.ibm.com>
> > ---
> > V1->V2:
> > changed memory allocation to glib calls
> > I'm not sure if this is meant for 2.1 or not
> > 
> >  hw/ppc/spapr_rtas.c    | 32 ++++++++++++++++++++++++++++++++
> >  include/hw/ppc/spapr.h | 13 +++++++++++++
> >  2 files changed, 45 insertions(+)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 1cb276d..734ebc0 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,34 @@ 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 = g_malloc0(length);
> > +
> > +        /*  These are the current system parameters supported.  The first

s/system parameters supported/supported system parameters/

> > +         *  16 bits of the buffer must contain the length of the string.
> > +         *  These 16 bits are not included in the length of the string. 
> > +         */
> > +
> > +        ((uint16_t *)local_buffer)[0] = cpu_to_be16(snprintf
> > (local_buffer + 2,
> > +             length - 2, "MaxEntCap=%d,MaxPlatProcs=%d", max_cpus, 
> > smp_cpus));
> > +

This would look nicer with a separate uint16_t variable for the buffer size.

> > +        rtas_st_buffer(buffer, length, (uint8_t *)local_buffer);
> > +
> > +        g_free(local_buffer);
> > +        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;

I'd say the above lines are useless...

> > +    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..bf5b528 100644
> > --- a/include/hw/ppc/spapr.h
> > +++ b/include/hw/ppc/spapr.h
> > @@ -356,6 +356,19 @@ 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) 
> > +{
> > +    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,
> > -- 
> > 1.8.3.1
> > 

Cheers.

-- 
Gregory Kurz                                     kurzgreg@fr.ibm.com
                                                 gkurz@linux.vnet.ibm.com
Software Engineer @ IBM/Meiosys                  http://www.ibm.com
Tel +33 (0)562 165 496

"Anarchy is about taking complete responsibility for yourself."
        Alan Moore.