Message-ID: <4257cdb1-c18b-4274-95b4-617f1da76518@huawei.com>
Date: Fri, 6 Feb 2026 17:57:52 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: Jonathan Cameron <jonathan.cameron@...wei.com>, <catalin.marinas@....com>,
	<linux-acpi@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linuxarm@...wei.com>, <gshan@...hat.com>, <miguel.luis@...cle.com>,
	<guohanjun@...wei.com>, <zhanjie9@...ilicon.com>, <lihuisong@...wei.com>,
	<yubowen8@...wei.com>, <zhangpengjie2@...wei.com>, <wangzhi12@...wei.com>,
	<linhongye@...artners.com>, <salil.mehta@...wei.com>, Viresh Kumar
	<viresh.kumar@...aro.org>, Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH] ACPI: processor: Add acpi_processor_start() back to parse
 _CPC tables before CPU online

On 2026/1/29 20:45, zhenglifeng (A) wrote:
> 
> 
> On 2026/1/28 2:00, Rafael J. Wysocki wrote:
>> +linux-pm and Viresh
>>
>> On Tue, Jan 27, 2026 at 5:58 PM Jonathan Cameron
>> <jonathan.cameron@...wei.com> wrote:
>>>
>>> On Tue, 27 Jan 2026 15:42:16 +0100
>>> "Rafael J. Wysocki" <rafael@...nel.org> wrote:
>>>
>>>> On Tue, Jan 20, 2026 at 12:33 PM Lifeng Zheng <zhenglifeng1@...wei.com> wrote:
>>>>>
>>>>> Currently, if boot with maxcpus less than NR_CPUS, the cppc_cpufreq driver
>>>>> will fail to register. Because it requires the domain information of all
>>>>> possible CPUs to construct shared_cpu_map, which shows the CPUs that share
>>>>> the same domain.
>>>>>
>>>>> Commit c1385c1f0ba3 ("ACPI: processor: Simplify initial onlining to use
>>>>> same path for cold and hotplug") removes probe() of acpi_processor_driver
>>>>> and makes acpi_cppc_processor_probe() only being called the first time CPU
>>>>> goes online. This means that CPUs that haven't yet gone online will not
>>>>> have pre-parsed _CPC objects and causes cppc_cpufreq driver register fail.
>>>>>
>>>>> Add acpi_processor_start() back as the probe() callback of
>>>>> acpi_processor_driver and call acpi_cppc_processor_probe() in it to make
>>>>> sure all _CPC tables will be parsed when acpi_processor_driver registered.
>>>>>
>>>>> Fixes: c1385c1f0ba3 ("ACPI: processor: Simplify initial onlining to use same path for cold and hotplug")
>>>>> Signed-off-by: Lifeng Zheng <zhenglifeng1@...wei.com>
>>>>> ---
>>>>>  drivers/acpi/processor_driver.c | 30 ++++++++++++++++++++++++++----
>>>>>  1 file changed, 26 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
>>>>> index 65e779be64ff..c8b4daf580b0 100644
>>>>> --- a/drivers/acpi/processor_driver.c
>>>>> +++ b/drivers/acpi/processor_driver.c
>>>>> @@ -33,6 +33,7 @@ MODULE_AUTHOR("Paul Diefenbaugh");
>>>>>  MODULE_DESCRIPTION("ACPI Processor Driver");
>>>>>  MODULE_LICENSE("GPL");
>>>>>
>>>>> +static int acpi_processor_start(struct device *dev);
>>>>>  static int acpi_processor_stop(struct device *dev);
>>>>>
>>>>>  static const struct acpi_device_id processor_device_ids[] = {
>>>>> @@ -46,6 +47,7 @@ static struct device_driver acpi_processor_driver = {
>>>>>         .name = "processor",
>>>>>         .bus = &cpu_subsys,
>>>>>         .acpi_match_table = processor_device_ids,
>>>>> +       .probe = acpi_processor_start,
>>>>>         .remove = acpi_processor_stop,
>>>>>  };
>>>>>
>>>>> @@ -162,10 +164,6 @@ static int __acpi_processor_start(struct acpi_device *device)
>>>>>         if (!pr)
>>>>>                 return -ENODEV;
>>>>>
>>>>> -       result = acpi_cppc_processor_probe(pr);
>>>>> -       if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
>>>>> -               dev_dbg(&device->dev, "CPPC data invalid or not present\n");
>>>>> -
>>>>>         if (!cpuidle_get_driver() || cpuidle_get_driver() == &acpi_idle_driver)
>>>>>                 acpi_processor_power_init(pr);
>>>>>
>>>>> @@ -192,6 +190,30 @@ static int __acpi_processor_start(struct acpi_device *device)
>>>>>         return result;
>>>>>  }
>>>>>
>>>>> +static int acpi_processor_start(struct device *dev)
>>>>> +{
>>>>> +       struct acpi_device *device = ACPI_COMPANION(dev);
>>>>> +       struct acpi_processor *pr;
>>>>> +       int result;
>>>>> +
>>>>> +       if (!device)
>>>>> +               return -ENODEV;
>>>>> +
>>>>> +       pr = acpi_driver_data(device);
>>>>> +       if (!pr)
>>>>> +               return -ENODEV;
>>>>> +
>>>>> +       /* Protect against concurrent CPU hotplug operations */
>>>>> +       cpu_hotplug_disable();
>>>>> +       result = acpi_cppc_processor_probe(pr);
>>>>> +       cpu_hotplug_enable();
>>>>
>>>> This means that CPPC will be initialized for vCPUs that are not
>>>> enabled on ARM if I'm not mistaken.
>>>
>>> If we are just talking powered down at boot it used to do that
>>> so I assume it was fine. The corner case is ones we are explicitly
>>> saying are not onlineable yet but marked online capable and will
>>> turn up later.
>>>
>>>>
>>>> I'm not sure if it is valid to do so.
>>>
>>> The conclusion of the following is I think this is fine but I'm not
>>> entirely confident about it.
>>>
>>> I'm struggling to figure out the right answer to this and
>>> it's not easy to test. I vaguely recall having some nasty emulation
>>> hacks to poke some x86 related _CPC stuff a while back.
>>> I might be able to hack something up for this as well and try to
>>> create pathological corner cases.
>>>
>>> The short answer is CPPC + hotplug isn't a thing today in KVM + QEMU,
>>> but that's not to say it never will be if someone virtualizes CPC for
>>> a guest.  Let's consider that hypothetical virtualization / emulation.
>>>
>>> So the questions:
>>> 1) Does simply making this acpi_cppc_processor_probe() result in any
>>>    register accesses to the registers that might be found in _CPC or
>>>    used via other ACPI methods?
>>> 2) Can we rely on a a VMM not do something nasty if those are accessed
>>>    on CPUs that haven't been instantiated yet?  e.g. Bus error.
>>>    A related useful question is: Can we assume these registers are
>>>    accessible on offlined CPUs?  If they can be unsafe to access from
>>>    CPUs that are temporary powered down / offline then I think we are fine because
>>>    the CPPC code must guarantee not to access them. (I'm relying on this!)
>>>
>>> For the particular case Lifeng has run into, I think the code that matters
>>> (beyond instantiation of the infrastructure) is the creation of the
>>> domain info in acpi_get_psd(). I think _PSD can only be static data so
>>> shouldn't cause any register accesses to the powered down CPUs.
>>>
>>> So 'probably' fine + we'll not really know unless we get CPU HP and
>>> CPC.
>>>
>>> Alternative much more complex change would be to separate the grabbing of
>>> static data (done here) from setting up anything dynamic which would remain
>>> in the hotplug handler.  If those registers haven't been discovered we definitely
>>> can't access them from the cpu freq driver.
>>
>> I'm thinking that maybe cppc_cpufreq should be updated instead.
>>
>> I'm not really sure why it needs to collect information on offline
>> CPUs.  Surely, they don't matter until they are brought online.
> 
> This information is collected in order to generate related_cpus. Without
> doing so, a new policy will be created when the second CPU in the same
> domain comes online, instead of reusing the existing policy. And this will
> make a mess.
> 
> I can't find a good way to solve this problem in cppc_cpufreq or cpufreq.

Hi Rafael,

If we have to make sure not to use the information on offline CPUs, there
will be many things that need to be modified:

1. acpi_get_psd_map() in cppc_acpi.c: only use the information on online
CPUs, and update shared_cpu_map when a new CPU in the same domain goes
online.

2. cpufreq_online() in cpufreq.c: create a new policy and decide whether it
should be kept because it may turns out that the CPU should share an
already exist policy with other CPUs.

3. The init() callbacks in cppc_cpufreq, acpi-cpufreq and maybe other
drivers: verify whether the newly generated policy is necessary and return
the result.

...

Is it necessary to make such a big change for solving this problem? It
seems to me that it is reasonable and fewer changes to parse the _CPC table
for all CPUs in advance because it never change, isn't it?

Powered by blists - more mailing lists