(re-adding the list)
On 05.09.2025 06:58, Penny, Zheng wrote:
> [Public]
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, September 4, 2025 7:51 PM
>> To: Penny, Zheng <penny.zh...@amd.com>; Andryuk, Jason
>> <jason.andr...@amd.com>
>> Cc: Andrew Cooper <andrew.coop...@citrix.com>; Roger Pau Monné
>> <roger....@citrix.com>; xen-devel@lists.xenproject.org
>> Subject: Re: [PATCH v9 1/8] xen/cpufreq: embed hwp into struct
>> cpufreq_policy{}
>>
>> On 04.09.2025 08:35, Penny Zheng wrote:
>>> For cpus sharing one cpufreq domain, cpufreq_driver.init() is only
>>> invoked on the firstcpu, so current per-CPU hwp driver data struct
>>> hwp_drv_data{} actually fails to be allocated for cpus other than the
>>> first one. There is no need to make it per-CPU.
>>> We embed struct hwp_drv_data{} into struct cpufreq_policy{}, then cpus
>>> could share the hwp driver data allocated for the firstcpu, like the
>>> way they share struct cpufreq_policy{}. We also make it a union, with
>>> "hwp", and later "amd-cppc" as a sub-struct.
>>
>> And ACPI, as per my patch (which then will need re-basing).
>>
>>> Suggested-by: Jan Beulich <jbeul...@suse.com>
>>
>> Not quite, this really is Reported-by: as it's a bug you fix, and in turn it
>> also wants to
>> gain a Fixes: tag. This also will need backporting.
>>
>> It would also have been nice if you had Cc-ed Jason right away, seeing that
>> this
>> code was all written by him.
>>
>>> @@ -259,7 +258,7 @@ static int cf_check hwp_cpufreq_target(struct
>> cpufreq_policy *policy,
>>> unsigned int relation) {
>>> unsigned int cpu = policy->cpu;
>>> - struct hwp_drv_data *data = per_cpu(hwp_drv_data, cpu);
>>> + struct hwp_drv_data *data = policy->u.hwp;
>>> /* Zero everything to ensure reserved bits are zero... */
>>> union hwp_request hwp_req = { .raw = 0 };
>>
>> Further down in this same function we have
>>
>> on_selected_cpus(cpumask_of(cpu), hwp_write_request, policy, 1);
>>
>> That's similarly problematic when the CPU denoted by policy->cpu isn't online
>> anymore. (It's not quite clear whether all related issues would want fixing
>> together,
>> or in multiple patches.)
>
> Checking the logic in cpufreq_del_cpu(), once any processor in the
> domain gets offline, the governor will stop.
Yet with HWP and CPPC drivers being governor-less, how would that matter?
> That is to say, only all processors in the domain are online, cpufreq driver
> could still be effective. Which is also complies to the description in _PSD
> ACPI SPEC for "Num Processors" [1]:
> ```
> The number of processors belonging to the domain for this logical processor’s
> P-states. OSPM will not start performing power state transitions to a
> particular P-state until this number of processors belonging to the same
> domain have been detected and started.
> ```
> [1]
> https://uefi.org/specs/ACPI/6.5/08_Processor_Configuration_and_Control.html?highlight=cppc#pstatedependency-package-values
>
> I know that AMD CPPC is obeying the _PSD dependency relation too, even if
> both CPPC Request register (ccd[15:0]_lthree0_core[7:0]_thread[1:0];
> MSRC001_02B3) and CPPC Capability Register
> (_ccd[15:0]_lthree0_core[7:0]_thread[1:0]; MSRC001_02B0) is Per-thread MSR.
> I don't have the hardware to test "sharing" logic. All my platform says
> "HW_ALL" in _PSD.
Aiui that's not mandated by the CPU spec, though. Plus HW_ALL still doesn't say
anything about the scope/size of each domain.
Jan
