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

[Tyrel Datwyler]

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.

-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