Re: [PATCH v4 09/15] cpufreq: Export HWP parameters to userspace as CPPC

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.06.2023 20:41, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 10:24 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 14.06.2023 20:02, Jason Andryuk wrote:
>>> +    else
>>>      {
>>> +        if ( !(scaling_available_governors =
>>> +               xzalloc_array(char, gov_num * CPUFREQ_NAME_LEN)) )
>>> +            return -ENOMEM;
>>> +        if ( (ret = read_scaling_available_governors(
>>> +                        scaling_available_governors,
>>> +                        gov_num * CPUFREQ_NAME_LEN * sizeof(char))) )
>>
>> I realize you only re-indent this, but since you need to touch it anyway,
>> may I suggest to also switch to siezof(*scaling_available_governors)?
> 
> How about dropping sizeof(*scaling_available_governors)?  This length ...
> 
>>> +        {
>>> +            xfree(scaling_available_governors);
>>> +            return ret;
>>> +        }
>>> +        ret = copy_to_guest(op->u.get_para.scaling_available_governors,
>>> +                    scaling_available_governors, gov_num * 
>>> CPUFREQ_NAME_LEN);
>>
>> Similarly here: Please adjust indentation while you touch this code.
> 
> ... should match here, but this second one lacks the "* sizeof($foo)".

Indeed it does, because that multiplication happens inside copy_to_guest()
(really copy_to_guest_offset()).

> They are strings, so multiplying by sizeof() is unusual.

Kind of, but in code which may want switching to Unicode (and not just
UTF-8,but e.g. UCS2 or UCS4) at some point it's a good idea to have such
right away. We don't mean to do any such switch, but I think it's good
practice to not assume that strings / string literals only ever consist
of plain char elements.

> FTAOD, you want the indenting as:
>         ret = copy_to_guest(op->u.get_para.scaling_available_governors,
>                             scaling_available_governors,
>                             gov_num * CPUFREQ_NAME_LEN);
> ?

That's one conforming way, yes. Another would be

        ret = copy_to_guest(
                  op->u.get_para.scaling_available_governors,
                  scaling_available_governors,
                  gov_num * CPUFREQ_NAME_LEN);

>>> --- a/xen/include/public/sysctl.h
>>> +++ b/xen/include/public/sysctl.h
>>> @@ -296,6 +296,61 @@ struct xen_ondemand {
>>>      uint32_t up_threshold;
>>>  };
>>>
>>> +struct xen_cppc_para {
>>> +    /* OUT */
>>> +    /* activity_window supported if 1 */
>>> +#define XEN_SYSCTL_CPPC_FEAT_ACT_WINDOW  (1 << 0)
>>
>> I think 1 isn't very helpful, looking forward. Perhaps better "set" or
>> "flag set"?
> 
> "set" works for me.
> 
>>> +    uint32_t features; /* bit flags for features */
>>> +    /*
>>> +     * See Intel SDM: HWP Performance Range and Dynamic Capabilities
>>> +     *
>>> +     * These four are 0-255 hardware-provided values.  They "continuous,
>>> +     * abstract unit-less, performance" values.  smaller numbers are slower
>>> +     * and larger ones are faster.
>>> +     */
>>> +    uint32_t lowest;
>>> +    uint32_t lowest_nonlinear; /* most_efficient */
>>
>> Why non_linear in the external interface when internally you use
>> most_efficient (merely put in the comment here)?
>>
>>> +    uint32_t nominal; /* guaranteed */
>>
>> Similar question for the name choice here.
> 
> There is a naming mismatch between the HWP fields and the CPPC fields.
> The commit message includes:
> The HWP most_efficient is mapped to CPPC lowest_nonlinear, and guaranteed is
> mapped to nominal.  CPPC has a guaranteed that is optional while nominal
> is required.  ACPI spec says "If this register is not implemented, OSPM
> assumes guaranteed performance is always equal to nominal performance."
> 
> So the comments were to help with the mapping.  Should I prefix the
> comments like "HWP: most_efficient"?

Yes please. (I was going to suggest exactly that when I hadn't read this
question, yet.)

Jan