qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Michael Roth
Subject: Re: [Qemu-devel] [PATCH v4 03/17] spapr_rtas: add get/set-power-level RTAS interfaces
Date: Tue, 27 Jan 2015 18:42:36 -0600
User-agent: alot/0.3.4

Quoting Tyrel Datwyler (2015-01-27 16:05:52)
> On 01/27/2015 01:36 PM, Michael Roth wrote:
> > Quoting David Gibson (2015-01-26 23:24:11)
> >> 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.
> > 
> > Hmm, looking at it again, set-indicator and get-sensor-state aren't actually
> > specific to DR, but might be extended to handle a number of other types of
> > sensors in the future ("Reset Component", "Error Log", and "Global Interrupt
> > Queue Control" may be interesting in this regard).
> > 
> > So it looks like only configure-connector is specifically for DR. Still
> > planning on moving it to spapr_drc_rtas.c, unless you'd prefer not to
> > at this point (it'll be lonely for the foreseable future).
> 
> I will point out that configure-connector would be used for
> hibernation/migration if we ever implement it as it is defined by PAPR.
> I know for migration there had been some talk in the past about someday
> doing it the right way.

Interesting. Do you know if calls are still tied to DRCs in this case, or
is is there something more general like fetching the entire device-tree? I
didn't see anything in PAPR+ 2.7 outside of the DR use-case where it acts
on a particular DRC.

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]