RE: [PATCH v5 03/18] ACPI: processor: Register deferred CPUs from acpi_processor_get_info()

[Salil Mehta]

>  From: Rafael J. Wysocki <rafael@xxxxxxxxxx>
>  Sent: Monday, April 15, 2024 1:51 PM
>  
>  On Mon, Apr 15, 2024 at 1:51 PM Salil Mehta <salil.mehta@xxxxxxxxxx>
>  wrote:
>  >
>  > Hello,
>  >
>  > >  From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>  > >  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 code  >> runs after the processor driver has loaded AFAICS.
>  > >  >
>  > >  > It is over this that I walked away from progressing this code,
>  > > because  > 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 nothing.
>  > >  > 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, then  > 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 possible  to online a CPU which just got marked present,
>  > > but the registration has not  completed yet.
>  > >
>  > >  > Finally, the "pr->flags.need_hotplug_init = 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 up in  production.
>  > >
>  > >  Though virtualization happily jumped on it to hot add/remove CPUs
>  > > to/from  a guest.
>  > >
>  > >  There are limitations to this and we learned it the hard way on
>  > > X86. At the  end we came up with the following restrictions:
>  > >
>  > >      1) All possible CPUs have to be advertised at boot time via firmware
>  > >         (ACPI/DT/whatever) independent of them being present at boot time
>  > >         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 at
>  > >         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
>  > > think 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
>  > and enumerated at the boot time as well (exposed by firmware and
>  > ACPI). This means all the enumerated processors will be marked as
>  > 'present' but they might exist in NOT enabled (_STA.enabled=0) 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
>  > like redistributors all need to be present and enumerated during the
>  > boot time. Redistributors 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?


For ARM Arch, during boot time, it should add processor as if no hotplug exists. But later,
in context to the (fake) hotplug trigger from the virtualizer as a result of the CPU (un)plug
action  it should just end up in registering the already present CPU with the Linux Driver Model. 


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


I referred to the components related to ARM CPU Arch like redistributors etc.
which will get initialized in context to Arch specific _init code not here. This
i.e. acpi_processor_add() is arch agnostic code common to all architectures.

[ A digression: You do have _weak functions which can be overridden to arch specific
 handling like  arch_(un)map_cpu() etc. but we can't use those to defer initialize
 the CPU related components - ARM Arch constraint!]


>  
>  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.


If you are referring to the _STA check in the XX_hot_add_init() then in the current
kernel code it only checks for the ACPI_STA_DEVICE_PRESENT flag and not
the ACPI_STA_DEVICE_ENABLED flag(?). The code being reviewed has changed
exactly that behavior for 2 legs i.e. make-present and make-enabled legs.

I'm open to further address your point clearly.

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


Agreed but there is still a subtle difference.  Enumeration happens once and
for all the processors during the boot time. And as confirmed by yourself and
Thomas as well that even in x86 arch all the processors will be discovered and
their topology fixed during the boot time which is effectively the same behavior
as in the ARM Arch. But ARM assumes those 'present' bits in the present masks
to be set during the boot time which is not like x86(?).  Hence, 'present cpu' Bits
will always be equal to 'possible cpu' Bits. This is a constraint put by the ARM
maintainers and looks unshakable. 


>  
>  > 2.  Agreed regarding the topology. Are you suggesting that we must
>  > call arch_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
>  principle.
>  
>  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).


Right. This one is out-of-scope for ARM Arch as we cannot defer any Arch
specific sizing and initializations after boot i.e. when processor_add() gets
called again later as a trigger of CPU plug action at the Virtualizer.


>  
>  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.


Correct. and we defer the registration for CPUs with online-capable Bit
set in the ACPI MADT/GICC data structure. These CPUs basically form
set of hot-pluggable CPUs on ARM. 


>  
>  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.

Yes, correct. First one is not possible - at least for now and to have that it will
require lot of rework especially at GIC. But there are many other arch components
(like timers, PMUs, etc.) whose behavior needs to be specified somewhere in the
architecture as well. All these are closely coupled with the ARM CPU architecture.
(it's beyond this discussion and lets leave that to ARM folks).

This patch-set has a change to deal with ACPI _STA.Enabled Bit accordingly.


Best regards
Salil.

>  
>  Thanks!