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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.02.2025 09:36, Penny, Zheng wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@xxxxxxxx>
>> Sent: Tuesday, February 18, 2025 10:49 PM
>>
>> 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_Contr
>>>>> ol
>>>>> .html?highlight=cppc#power-performance-and-throttling-state-dependen
>>>>> ci 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@amd.c
>>> om/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.
> 
> Are you suggesting that we only do either set_cppc_pminfo or set_px_pminfo?
> Only one side data get set to avoid the consequence of mixture.

That's one of the possible ways to avoid mixing things. Then again Dom0
won't know what cpufreq= was passed to Xen, and hence it may not be in
the position to decide what to upload. It hence may need to be Xen to
simply ignore the uploading of data it's not going to use.

Jan