Message-ID: <20260127165824.0000247f@huawei.com>
Date: Tue, 27 Jan 2026 16:58:24 +0000
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: "Rafael J. Wysocki" <rafael@...nel.org>
CC: Lifeng Zheng <zhenglifeng1@...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>
Subject: Re: [PATCH] ACPI: processor: Add acpi_processor_start() back to
 parse _CPC tables before CPU online

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.

Thanks,

Jonathan




> 
> Jonathan?
> 
> > +
> > +       if (result && !IS_ENABLED(CONFIG_ACPI_CPU_FREQ_PSS))
> > +               dev_dbg(&device->dev, "CPPC data invalid or not present\n");
> > +
> > +       return 0;
> > +}
> > +
> >  static int acpi_processor_stop(struct device *dev)
> >  {
> >         struct acpi_device *device = ACPI_COMPANION(dev);
> > --
> > 2.33.0
> >
> >  
> 

Powered by blists - more mailing lists