Received-SPF: pass (google.com: domain of linux-kernel+bounces-145175-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240412143719.11398-1-Jonathan.Cameron@huawei.com>
 <20240412143719.11398-4-Jonathan.Cameron@huawei.com> <CAJZ5v0gNvy2e=hOGQQ2kLpnrDr8=QGBax-E5odEJ=7BA8qW-9A@mail.gmail.com>
 <ZhmWkE+fCEG/WFoi@shell.armlinux.org.uk> <87bk6ez4hj.ffs@tglx> <e80bfa8cb9b74997a4214e531366c71d@huawei.com>
In-Reply-To: <e80bfa8cb9b74997a4214e531366c71d@huawei.com>
From: "Rafael J. Wysocki" <rafael@kernel.org>
Date: Mon, 15 Apr 2024 14:51:27 +0200
Message-ID: <CAJZ5v0hgABc=c6L4ctTHZ+5afeQTM1-bXhxthEKGxs8hG8R=YQ@mail.gmail.com>
Subject: Re: [PATCH v5 03/18] ACPI: processor: Register deferred CPUs from acpi_processor_get_info()
To: Salil Mehta <salil.mehta@huawei.com>
Cc: Thomas Gleixner <tglx@linutronix.de>, "Russell King (Oracle)" <linux@armlinux.org.uk>, 
	"Rafael J. Wysocki" <rafael@kernel.org>, Jonathan Cameron <jonathan.cameron@huawei.com>, 
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>, 
	"loongarch@lists.linux.dev" <loongarch@lists.linux.dev>, 
	"linux-acpi@vger.kernel.org" <linux-acpi@vger.kernel.org>, 
	"linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, 
	"linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, 
	"kvmarm@lists.linux.dev" <kvmarm@lists.linux.dev>, "x86@kernel.org" <x86@kernel.org>, 
	Miguel Luis <miguel.luis@oracle.com>, James Morse <james.morse@arm.com>, 
	Jean-Philippe Brucker <jean-philippe@linaro.org>, Catalin Marinas <catalin.marinas@arm.com>, 
	Will Deacon <will@kernel.org>, Linuxarm <linuxarm@huawei.com>, 
	"justin.he@arm.com" <justin.he@arm.com>, "jianyong.wu@arm.com" <jianyong.wu@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 15, 2024 at 1:51=E2=80=AFPM Salil Mehta <salil.mehta@huawei.com=
> wrote:
>
> Hello,
>
> >  From: Thomas Gleixner <tglx@linutronix.de>
> >  Sent: Friday, April 12, 2024 9:55 PM
> >
> >  On Fri, Apr 12 2024 at 21:16, Russell King (Oracle) wrote:
> >  > On Fri, Apr 12, 2024 at 08:30:40PM +0200, Rafael J. Wysocki wrote:
> >  >> Say acpi_map_cpu) / acpi_unmap_cpu() are turned into arch calls.
> >  >> What's the difference then?  The locking, which should be fine if I=
'm
> >  >> not mistaken and need_hotplug_init that needs to be set if this cod=
e
> >  >> runs after the processor driver has loaded AFAICS.
> >  >
> >  > It is over this that I walked away from progressing this code, becau=
se
> >  > I don't think it's quite as simple as you make it out to be.
> >  >
> >  > Yes, acpi_map_cpu() and acpi_unmap_cpu() are already arch
> >  implemented
> >  > functions, so Arm64 can easily provide stubs for these that do nothi=
ng.
> >  > That never caused me any concern.
> >  >
> >  > What does cause me great concern though are the finer details. For
> >  > example, above you seem to drop the evaluation of _STA for the
> >  > "make_present" case - I've no idea whether that is something that
> >  > should be deleted or not (if it is something that can be deleted, th=
en
> >  > why not delete it now?)
> >  >
> >  > As for the cpu locking, I couldn't find anything in
> >  > arch_register_cpu() that depends on the cpu_maps_update stuff nor
> >  > needs the cpus_write_lock being taken - so I've no idea why the
> >  > "make_present" case takes these locks.
> >
> >  Anything which updates a CPU mask, e.g. cpu_present_mask, after early
> >  boot must hold the appropriate write locks. Otherwise it would be poss=
ible
> >  to online a CPU which just got marked present, but the registration ha=
s not
> >  completed yet.
> >
> >  > Finally, the "pr->flags.need_hotplug_init =3D 1" thing... it's not
> >  > obvious that this is required - remember that with Arm64's "enabled"
> >  > toggling, the "processor" is a slice of the system and doesn't
> >  > actually go away - it's just "not enabled" for use.
> >  >
> >  > Again, as "processors" in Arm64 are slices of the system, they have =
to
> >  > be fully described in ACPI before the OS boots, and they will be
> >  > marked as being "present", which means they will be enumerated, and
> >  > the driver will be probed. Any processor that is not to be used will
> >  > not have its enabled bit set. It is my understanding that every
> >  > processor will result in the ACPI processor driver being bound to it
> >  > whether its enabled or not.
> >  >
> >  > The difference between real hotplug and Arm64 hotplug is that real
> >  > hotplug makes stuff not-present (and thus unenumerable). Arm64
> >  hotplug
> >  > makes stuff not-enabled which is still enumerable.
> >
> >  Define "real hotplug" :)
> >
> >  Real physical hotplug does not really exist. That's at least true for =
x86, where
> >  the physical hotplug support was chased for a while, but never ended u=
p in
> >  production.
> >
> >  Though virtualization happily jumped on it to hot add/remove CPUs to/f=
rom
> >  a guest.
> >
> >  There are limitations to this and we learned it the hard way on X86. A=
t the
> >  end we came up with the following restrictions:
> >
> >      1) All possible CPUs have to be advertised at boot time via firmwa=
re
> >         (ACPI/DT/whatever) independent of them being present at boot ti=
me
> >         or not.
> >
> >         That guarantees proper sizing and ensures that associations
> >         between hardware entities and software representations and the
> >         resulting topology are stable for the lifetime of a system.
> >
> >         It is really required to know the full topology of the system a=
t
> >         boot time especially with hybrid CPUs where some of the cores
> >         have hyperthreading and the others do not.
> >
> >
> >      2) Hot add can only mark an already registered (possible) CPU
> >         present. Adding non-registered CPUs after boot is not possible.
> >
> >         The CPU must have been registered in #1 already to ensure that
> >         the system topology does not suddenly change in an incompatible
> >         way at run-time.
> >
> >  The same restriction would apply to real physical hotplug. I don't thi=
nk that's
> >  any different for ARM64 or any other architecture.
>
>
> There is a difference:
>
> 1.   ARM arch does not allows for any processor to be NOT present. Hence,=
 because of
> this restriction any of its related per-cpu components must be present an=
d enumerated
> at the boot time as well (exposed by firmware and ACPI). This means all t=
he enumerated
> processors will be marked as 'present' but they might exist in NOT enable=
d (_STA.enabled=3D0)
> state.
>
> There was one clear difference and please correct me if I'm wrong here,  =
for x86, the LAPIC
> associated with the x86 core can be brought online later even after boot?
>
> But for ARM Arch, processors and its corresponding per-cpu components lik=
e redistributors
> all need to be present and enumerated during the boot time. Redistributor=
s are part of
> ALWAYS-ON power domain.

OK

So what exactly is the problem with this and what does
acpi_processor_add() have to do with it?

Do you want the per-CPU structures etc. to be created from the
acpi_processor_add() path?

This plain won't work because acpi_processor_add(), as defined in the
mainline kernel today (and the Jonathan's patches don't change that
AFAICS), returns an error for processor devices with the "enabled" bit
clear in _STA (it returns an error if the "present" bit is clear too,
but that's obvious), so it only gets to calling arch_register_cpu() if
*both* "present" and "enabled" _STA bits are set for the given
processor device.

That, BTW, is why I keep saying that from the ACPI CPU enumeration
code perspective, there is no difference between "present" and
"enabled".

> 2.  Agreed regarding the topology. Are you suggesting that we must call a=
rch_register_cpu()
> during boot time for all the 'present' CPUs? Even if that's the case, we =
might still want to defer
> registration of the cpu device (register_cpu() API) with the Linux device=
 model. Later is what
> we are doing to hide/unhide the CPUs from the user while STA.Enabled Bit =
is toggled due to
> CPU (un)plug action.

There are two ways to approach this IMV, and both seem to be valid in princ=
iple.

One would be to treat CPUs with the "enabled" bit clear as not present
and create all of the requisite data structures for them when they
become available (in analogy with the "real hot-add" case).

The alternative one is to create all of the requisite data structures
for the CPUs that you find during boot, but register CPU devices for
those having the "enabled" _STA bit set.

It looks like you have chosen the second approach, which is fine with
me (although personally, I would rather choose the first one), but
then your arch code needs to arrange for the requisite CPU data
structures etc. to be set up before acpi_processor_add() gets called
because, as per the above, the latter just rejects CPUs with the
"enabled" _STA bit clear.

Thanks!