Re: [PATCH v2 02/11] xen/x86: introduce new sub-hypercall to propagate CPPC data

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.02.2025 07:05, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Monday, February 17, 2025 3:39 PM
>>
>> On 17.02.2025 08:20, Penny, Zheng wrote:
>>> [AMD Official Use Only - AMD Internal Distribution Only]
>>
>> Btw, boiler plates like this aren't really liked on public mailing lists, 
>> for being contrary
>> to the purpose of such lists.

You did read this, didn't you? I ask because the same boilerplate keeps
appearing in your mails.

>>>> -----Original Message-----
>>>> From: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Sent: Tuesday, February 11, 2025 7:10 PM
>>>>
>>>> On 06.02.2025 09:32, Penny Zheng wrote:
>>>>> +{
>>>>> +    int ret = 0, cpuid;
>>>>> +    struct processor_pminfo *pm_info;
>>>>> +
>>>>> +    cpuid = get_cpu_id(acpi_id);
>>>>> +    if ( cpuid < 0 || !cppc_data )
>>>>> +    {
>>>>> +        ret = -EINVAL;
>>>>> +        goto out;
>>>>> +    }
>>>>> +    if ( cpufreq_verbose )
>>>>> +        printk("Set CPU acpi_id(%d) cpuid(%d) CPPC State info:\n",
>>>>> +               acpi_id, cpuid);
>>>>> +
>>>>> +    pm_info = processor_pminfo[cpuid];
>>>>> +    if ( !pm_info )
>>>>> +    {
>>>>> +        pm_info = xvzalloc(struct processor_pminfo);
>>>>> +        if ( !pm_info )
>>>>> +        {
>>>>> +            ret = -ENOMEM;
>>>>> +            goto out;
>>>>> +        }
>>>>> +        processor_pminfo[cpuid] = pm_info;
>>>>> +    }
>>>>> +    pm_info->acpi_id = acpi_id;
>>>>> +    pm_info->id = cpuid;
>>>>> +    pm_info->cppc_data = *cppc_data;
>>>>> +
>>>>> +    if ( cpufreq_verbose )
>>>>> +        print_CPPC(&pm_info->cppc_data);
>>>>> +
>>>>> + out:
>>>>> +    return ret;
>>>>> +}
>>>>
>>>> What's the interaction between the data set by set_px_pminfo() and
>>>> the data set here? In particular, what's going to happen if both
>>>> functions come into play for the same CPU? Shouldn't there be some sanity
>> checks?
>>>
>>> Yes, I've considered this and checked ACPI spec. I'll refer them here:
>>> ```
>>> If the platform supports CPPC, the _CPC object must exist under all 
>>> processor
>> objects.
>>> That is, OSPM is not expected to support mixed mode (CPPC & legacy PSS,
>> _PCT, _PPC) operation.
>>> ```
>>> See
>>> https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control
>>> .html?highlight=cppc#power-performance-and-throttling-state-dependenci
>>> es So CPUs could have both _CPC and legacy P-state info in ACPI for
>>> each entry, they just can't have mixed-mode Maybe we shall add sanity
>>> check to see if _CPC exists, it shall exist for all pcpus?
>>
>> Maybe, but that wasn't the point of my remark.
>>
>> Properly behaving Dom0 should probably be passing only one of the two 
>> possible
>> pieces of information. Yet maybe we'd better sanity check _that_?
>> (I don't recall seeing Linux kernel side patches yet; if they were posted 
>> somewhere,
>> they may at least partly address my concern.)
>>
> 
> In my linux patch, 
> https://lore.kernel.org/lkml/20241204082430.469092-1-Penny.Zheng@xxxxxxx/T/
> I only did zero-value check in xen_processor_get_perf_caps(), Do you think in 
> that place, I shall add
> more strict sanity check, like the register value shall not be zero and also 
> must smaller than UINT8_T?
> Or we just do the above check in Xen part when receiving the data?

Value range checking is nice to have in Dom0, but the same checking
needs doing in the hypervisor anyway. But that isn't what my comment
was about. What I'm asking is how it is being made sure that we won't
have to deal with a mix of traditional and CPPC data in the hypervisor.

Jan