On 20.06.2023 20:41, Jason Andryuk wrote:
> On Mon, Jun 19, 2023 at 10:24 AM Jan Beulich <jbeul...@suse.com> 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
