Re: [PATCH] ACPI: processor: Add acpi_processor_start() back to parse _CPC tables before CPU online

[zhenglifeng (A)]

On 2/6/2026 5:57 PM, zhenglifeng (A) wrote:
> 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@xxxxxxxxxx> wrote:
>>>>
>>>> On Tue, 27 Jan 2026 15:42:16 +0100
>>>> "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote:
>>>>
>>>>> On Tue, Jan 20, 2026 at 12:33 PM Lifeng Zheng <zhenglifeng1@xxxxxxxxxx> 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@xxxxxxxxxx>
>>>>>> ---
>>>>>>  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?
> 

Hi Rafael,

Although some time has passed, I still look forward to your advice.

Thanks.