Re: [PATCH v1 05/11] xen/x86: introduce a new amd pstate driver for cpufreq scaling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.01.2025 07:14, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Thursday, January 9, 2025 6:55 PM
>>
>> On 03.12.2024 09:11, Penny Zheng wrote:
>>> +    {
>>> +        /* Read Processor Max Speed(mhz) from DMI table as anchor point */
>>> +        mul = dmi_max_speed_mhz;
>>> +        div = data->hw.highest_perf;
>>> +
>>> +        return (unsigned int)(mul / div) * data->hw.lowest_perf *
>>> + 1000;
>>
>> No risk of the cast chopping off set bits?
> 
> Mul shall be uint64_t, highest_perf and lowest_perf shall be uint8_t, and 
> normally
> the equation output shall not be a too big value...
> If you think it's safer to do cast after comparing with the UINT32_MAX, I 
> will add the check

Well, I can only say as much: Dividing a uint64_t by a uint8_t has ample
opportunity for there being more than 32 significant bits in the result.

>>> +    if ( !(val & AMD_CPPC_ENABLE) )
>>> +    {
>>> +        val |= AMD_CPPC_ENABLE;
>>> +        if ( wrmsr_safe(MSR_AMD_CPPC_ENABLE, val) )
>>> +        {
>>> +            amd_pstate_err(policy->cpu,
>> "wrmsr_safe(MSR_AMD_CPPC_ENABLE, %lx)\n", val);
>>> +            data->err = -EINVAL;
>>> +            return;
>>> +        }
>>> +    }
>>
>> Do you really need to enable befor reading ...
>>
>>> +    if ( rdmsr_safe(MSR_AMD_CPPC_CAP1, data->amd_caps) )
>>
>> ... capabilities and ...
>>
> 
> Yes.
> Only when CPPC is enabled, the hardware will calculate the processor’s 
> performance capabilities and
> initialize the performance level fields in the CPPC capability registers.
> See 17.6.3 
> https://www.amd.com/content/dam/amd/en/documents/processor-tech-docs/programmer-references/24593.pdf

Ah yes, I see. Yet then the text there also says that the ENABLE bit can't
be cleared (except by reset), so your error handling is questionable.

>>> +    {
>>> +        amd_pstate_err(policy->cpu, "rdmsr_safe(MSR_AMD_CPPC_CAP1)\n");
>>> +        goto error;
>>> +    }
>>> +
>>> +    if ( data->hw.highest_perf == 0 || data->hw.lowest_perf == 0 ||
>>> +         data->hw.nominal_perf == 0 || data->hw.lowest_nonlinear_perf == 0 
>>> )
>>> +    {
>>> +        amd_pstate_err(policy->cpu, "Platform malfunction, read CPPC
>> highest_perf: %u, lowest_perf: %u, nominal_perf: %u, lowest_nonlinear_perf: 
>> %u
>> zero value\n",
>>> +                       data->hw.highest_perf, data->hw.lowest_perf,
>>> +                       data->hw.nominal_perf, 
>>> data->hw.lowest_nonlinear_perf);
>>> +        goto error;
>>> +    }
>>> +
>>> +    min_freq = amd_get_min_freq(data);
>>> +    nominal_freq = amd_get_nominal_freq(data);
>>> +    max_freq = amd_get_max_freq(data);
>>> +    if ( min_freq > max_freq )
>>> +    {
>>> +        amd_pstate_err(policy->cpu, "min_freq(%u) or max_freq(%u) value is
>> incorrect\n",
>>> +                       min_freq, max_freq);
>>> +        goto error;
>>> +    }
>>
>> ... checking them? Error handling would be easier if you enabled only 
>> afterwards.
>>
>>> +    highest_perf = data->hw.highest_perf;
>>> +    nominal_perf = data->hw.nominal_perf;
>>> +
>>> +    if ( highest_perf <= nominal_perf )
>>> +        return;
>>> +
>>> +    policy->turbo = CPUFREQ_TURBO_ENABLED; }
>>
>> And why the helper variables in the first place?
> 
> Sorry, maybe a bit more specific, got confused about the question ;/

With local variables like these, you end up with more code volume for
no real gain. IOW what's wrong with

    if ( data->hw.highest_perf <= data->hw.nominal_perf )
        return;

? (As an aside, even if the variables were actually useful, it would
still be better to have them have initializers than to use separate
declarations and assignments.)

Jan