Re: [Qemu-devel] [PATCH v4 03/17] spapr_rtas: add get/set-power-level RTAS interfaces

[David Gibson]

On Sun, Jan 25, 2015 at 11:21:26PM -0600, Michael Roth wrote:
> Quoting David Gibson (2015-01-16 00:21:55)
> > On Tue, Dec 23, 2014 at 06:30:17AM -0600, Michael Roth wrote:
> > > From: Nathan Fontenot <address@hidden>
> > > 
> > > Signed-off-by: Nathan Fontenot <address@hidden>
> > > Signed-off-by: Michael Roth <address@hidden>
> > > ---
> > >  hw/ppc/spapr_rtas.c | 25 +++++++++++++++++++++++++
> > >  1 file changed, 25 insertions(+)
> > > 
> > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > > index 2ec2a8e..a2fb533 100644
> > > --- a/hw/ppc/spapr_rtas.c
> > > +++ b/hw/ppc/spapr_rtas.c
> > > @@ -290,6 +290,27 @@ static void rtas_ibm_os_term(PowerPCCPU *cpu,
> > >      rtas_st(rets, 0, ret);
> > >  }
> > >  
> > > +static void rtas_set_power_level(PowerPCCPU *cpu, sPAPREnvironment 
> > > *spapr,
> > > +                                 uint32_t token, uint32_t nargs,
> > > +                                 target_ulong args, uint32_t nret,
> > > +                                 target_ulong rets)
> > > +{
> > > +    /* we currently only use a single, "live insert" powerdomain for
> > > +     * hotplugged/dlpar'd resources, so the power is always live/full 
> > > (100)
> > > +     */
> > 
> > Even so, you should at least validate the number of args and rets, and
> > preferably check that the user isn't attempt to set something for some
> > other, non-existent power domain.
> > 
> > > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > +    rtas_st(rets, 1, 100);
> > > +}
> > > +
> > > +static void rtas_get_power_level(PowerPCCPU *cpu, sPAPREnvironment 
> > > *spapr,
> > > +                                  uint32_t token, uint32_t nargs,
> > > +                                  target_ulong args, uint32_t nret,
> > > +                                  target_ulong rets)
> > > +{
> > > +    rtas_st(rets, 0, RTAS_OUT_SUCCESS);
> > > +    rtas_st(rets, 1, 100);
> > > +}
> > > +
> > >  static struct rtas_call {
> > >      const char *name;
> > >      spapr_rtas_fn fn;
> > > @@ -419,6 +440,10 @@ static void core_rtas_register_types(void)
> > >                          rtas_ibm_set_system_parameter);
> > >      spapr_rtas_register(RTAS_IBM_OS_TERM, "ibm,os-term",
> > >                          rtas_ibm_os_term);
> > > +    spapr_rtas_register(RTAS_SET_POWER_LEVEL, "set-power-level",
> > > +                        rtas_set_power_level);
> > > +    spapr_rtas_register(RTAS_GET_POWER_LEVEL, "get-power-level",
> > > +                        rtas_get_power_level);
> > >  }
> > >  
> > >  type_init(core_rtas_register_types)
> > 
> > This code should probably go in spapr_drc.c.  The idea that spapr_rtas
> > was just the RTAS dispatch code, and RTAS functions that had no other
> > home.  Generally RTAS functions should live with the devices they're
> > connected to.
> 
> In this particular case the calls act on a "power domain" which isn't
> actually modeled in QEMU (we just assume a single "live-insertion" domain
> which just magically does everything we want), so I think it makes
> sense to leave these here.

Yeah, fair enough.

> But for the others it does make sense to tie them with spapr_drc.c, or
> maybe spapr_drc_rtas.c to maintain the encapsulation of DRC state behind
> well-defined accessors.

Ok.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

pgpI9POk5F_s8.pgp
Description: PGP signature