Hi Rafael, > From: Rafael J. Wysocki <rafael@xxxxxxxxxx> > Sent: Monday, April 15, 2024 1:04 PM > > On Mon, Apr 15, 2024 at 1:56 PM Jonathan Cameron > <Jonathan.Cameron@xxxxxxxxxx> wrote: > > > > On Mon, 15 Apr 2024 13:37:08 +0200 > > "Rafael J. Wysocki" <rafael@xxxxxxxxxx> wrote: > > > > > On Mon, Apr 15, 2024 at 10:46 AM Jonathan Cameron > > > <Jonathan.Cameron@xxxxxxxxxx> wrote: > > > > > > > > On Sat, 13 Apr 2024 01:23:48 +0200 Thomas Gleixner > > > > <tglx@xxxxxxxxxxxxx> wrote: > > > > > > > > > Russell! > > > > > > > > > > On Fri, Apr 12 2024 at 22:52, Russell King (Oracle) wrote: > > > > > > On Fri, Apr 12, 2024 at 10:54:32PM +0200, Thomas Gleixner wrote: > > > > > >> > 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. > > > > > > > > > > > > Yes. As far as I've been able to determine, > > > > > > arch_register_cpu() doesn't manipulate any of the CPU masks. > > > > > > All it seems to be doing is initialising the struct cpu, > > > > > > registering the embedded struct device, and setting up the sysfs links to its NUMA node. > > > > > > > > > > > > There is nothing obvious in there which manipulates any CPU > > > > > > masks, and this is rather my fundamental point when I said "I > > > > > > couldn't find anything in arch_register_cpu() that depends on ...". > > > > > > > > > > > > If there is something, then comments in the code would be a > > > > > > useful aid because it's highly non-obvious where such a > > > > > > manipulation is located, and hence why the locks are necessary. > > > > > > > > > > acpi_processor_hotadd_init() > > > > > ... > > > > > acpi_map_cpu(pr->handle, pr->phys_id, pr->acpi_id, > > > > > &pr->id); > > > > > > > > > > That ends up in fiddling with cpu_present_mask. > > > > > > > > > > I grant you that arch_register_cpu() is not, but it might rely > > > > > on the external locking too. I could not be bothered to figure that out. > > > > > > > > > > >> 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. > > > > > > > > > > > > This makes me wonder whether the Arm64 has been barking up the > > > > > > wrong tree then, and whether the whole "present" vs "enabled" > > > > > > thing comes from a misunderstanding as far as a CPU goes. > > > > > > > > > > > > However, there is a big difference between the two. On x86, a > > > > > > processor is just a processor. On Arm64, a "processor" is a > > > > > > slice of the system (includes the interrupt controller, PMUs > > > > > > etc) and we must enumerate those even when the processor > > > > > > itself is not enabled. This is the whole reason there's a > > > > > > difference between "present" and "enabled" and why there's a difference between x86 cpu hotplug and arm64 cpu hotplug. > > > > > > The processor never actually goes away in arm64, it's just > > > > > > prevented from being used. > > > > > > > > > > It's the same on X86 at least in the physical world. > > > > > > > > There were public calls on this via the Linaro Open Discussions > > > > group, so I can talk a little about how we ended up here. Note > > > > that (in my > > > > opinion) there is zero chance of this changing - it took us well > > > > over a year to get to this conclusion. So if we ever want ARM > > > > vCPU HP we need to work within these constraints. > > > > > > > > The ARM architecture folk (the ones defining the ARM ARM, relevant > > > > ACPI specs etc, not the kernel maintainers) are determined that > > > > they want to retain the option to do real physical CPU hotplug in > > > > the future with all the necessary work around dynamic interrupt > > > > controller initialization, debug and many other messy corners. > > > > > > That's OK, but the difference is not in the ACPi CPU enumeration/removal code. > > > > > > > Thus anything defined had to be structured in a way that was 'different' > > > > from that. > > > > > > Apparently, that's where things got confused. > > > > > > > I don't mind the proposed flattening of the 2 paths if the ARM > > > > kernel maintainers are fine with it but it will remove the > > > > distinctions and we will need to be very careful with the CPU > > > > masks - we can't handle them the same as x86 does. > > > > > > At the ACPI code level, there is no distinction. > > > > > > A CPU that was not available before has just become available. The > > > platform firmware has notified the kernel about it and now > > > acpi_processor_add() runs. Why would it need to use different code > > > paths depending on what _STA bits were clear before? > > > > I think we will continue to disagree on this. To my mind and from the > > ACPI specification, they are two different state transitions with > > different required actions. > > Well, please be specific: What exactly do you mean here and which parts of > the spec are you talking about? > > > Those state transitions are an ACPI level thing not an arch level one. > > However, I want a solution that moves things forwards so I'll give > > pushing that entirely into the arch code a try. > > Thanks! > > Though I think that there is a disconnect between us that needs to be > clarified first. > > > > > > > Yes, there is some arch stuff to be called and that arch stuff > > > should figure out what to do to make things actually work. > > > > > > > I'll get on with doing that, but do need input from Will / Catalin / James. > > > > There are some quirks that need calling out as it's not quite a > > > > simple as it appears from a high level. > > > > > > > > Another part of that long discussion established that there is > > > > userspace (Android IIRC) in which the CPU present mask must > > > > include all CPUs at boot. To change that would be userspace ABI > > > > breakage so we can't do that. Hence the dance around adding yet > > > > another mask to allow the OS to understand which CPUs are 'present' > but not possible to online. > > > > > > > > Flattening the two paths removes any distinction between calls > > > > that are for real hotplug and those that are for this online capable path. > > > > > > Which calls exactly do you mean? > > > > At the moment he distinction does not exist (because x86 only supports > > fake physical CPU HP and arm64 only vCPU HP / online capable), but if > > the architecture is defined for arm64 physical hotplug in the future > > we would need to do interrupt controller bring up + a lot of other stuff. > > > > It may be possible to do that in the arch code - will be hard to > > verify that until that arch is defined Today all I need to do is > > ensure that any attempt to do present bit setting for ARM64 returns an error. > > That looks to be straight forward. > > OK > > > > > > > > > > As a side note, the indicating bit for these flows is defined in > > > > ACPI for x86 from ACPI 6.3 as a flag in Processor Local APIC (the > > > > ARM64 definition is a cut and paste of that text). So someone is > > > > interested in this distinction on x86. I can't say who but if you > > > > have a mantis account you can easily follow the history and it > > > > might be instructive to not everyone considering the current x86 > > > > flow the right way to do it. > > > > > > So a physically absent processor is different from a physically > > > present processor that has not been disabled. No doubt about this. > > > > > > That said, I'm still unsure why these two cases require two > > > different code paths in acpi_processor_add(). > > > > It might be possible to push the checking down into > > arch_register_cpu() and have that for now reject any attempt to do > physical CPU HP on arm64. > > It is that gate that is vital to getting this accepted by ARM. > > > > I'm still very much stuck on the hotadd_init flag however, so any > > suggestions on that would be very welcome! > > I need to do some investigation which will take some time I suppose. You might find below cover letter and links to the presentations useful: 1. https://lore.kernel.org/qemu-devel/20230926100436.28284-1-salil.mehta@xxxxxxxxxx/ 2. https://kvm-forum.qemu.org/2023/KVM-forum-cpu-hotplug_7OJ1YyJ.pdf 3. https://kvm-forum.qemu.org/2023/Challenges_Revisited_in_Supporting_Virt_CPU_Hotplug_-__ii0iNb3.pdf 4. https://sched.co/eE4m Best regards Salil.
