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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> -----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-dependencies
> 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.)

>> Will consumers be able to tell which of the two were correctly invoked, 
>> before using
>> respective data? Even in the event of no code changes at all to address 
>> this, it will
>> want discussing in the patch description.
>>
> 
> I have checked the relevant spec. it shall be the following logic:
> ```
> Software enables Collaborative Processor Performance Control by setting
> CPPC_ENABLE[CPPC_En] (bit 0) = 1. Once it gets enabled, the processor ignores 
> the legacy P-state control interface.
> ```
> Hmmm, I shall add relevant comment in Doc?

Mentioning this in the description would help. Yet the processor ignoring
the other P-state control interface shouldn't mean we can nevertheless try
to drive it. It has to be clear (and at least halfway obvious) internally
to Xen that we only ever use one of the two. My present reading of the
patches suggests that this is all implicit (and maybe not even guaranteed)
right now.

Jan