Hi Salil,
On 27/02/2023 09:49, Salil Mehta wrote:
-----Original Message----- From: James Morse james.morse@arm.com Sent: Friday, February 24, 2023 5:39 PM To: Vishnu Pajjuri vishnu@amperemail.onmicrosoft.com; linaro-open- discussions@op-lists.linaro.org; Salil Mehta salil.mehta@huawei.com; salil.mehta@opnsrc.net Subject: Re: [Linaro-open-discussions] Re: [RFC PATCH v0.1 22/25] ACPI: add support to register CPUs based on the _STA enabled bit
On 23/02/2023 06:10, Vishnu Pajjuri via Linaro-open-discussions wrote:
On 31-08-2022 16:39, James Morse wrote:
acpi_processor_get_info() registers all present CPUs. Registering a CPU is what creates the sysfs entries and triggers the udev notifications.
arm64 virtual machines that support 'virtual cpu hotplug' use the enabled bit to indicate whether the CPU can be brought online, as the existing ACPI tables require all hardware to be described and present.
If firmware describes a CPU as present, but disabled, skip the registration. Such CPUs are present, but can't be brought online for whatever reason. (e.g. firmware/hypervisor policy).
Once firmware sets the enabled bit, the CPU can be registered and brought online by user-space. Online CPUs, or CPUs that are missing an _STA method must always be registered.
Enumeration failure while trying to hot plug vcpus after guest reboot
as the vcpu *disabled*(_STA.Enabled=0), vcpu has been made not present and flags flags.initialized and flags.visited were not cleared
(It's important that everything remains present all the time. When a CPU is disabled, its offline and unregistered, but all the bits and pieces of logic that go with the CPU are still present. This is what makes it safe to do this on physical hardware.)
And following fix resolves the issue
Turns out Qemu is returning in '0' in _STA until the CPU is added, which makes it 0xf. Once its removed, it becomes 0xd.
Qemu should be reporting the CPU as at least present from the very beginning, as it is described in the MADT.
Salil, could look into this on the qemu side?
Yes, It’s a valid point but can interfere with the legacy? But will again check this today.
It doesn't. Existing versions of linux don't comprehend the MADT ONLINE_CAPABLE bit, so those present CPUs are removed from the present mask. When ACPI tries to set them up, acpi_processor_hotadd_init() will bale out with ENODEV because the CPUID isn't "mapped", and can't be mapped, because the arch code doesn't consider that CPU present.
You can't bodge round legacy things like this in Qemu as this reboot case shows. Even if you try and hook reboot, the guest may use kexec instead.
I'd much prefer we change the ACPI logic to only enumerate devices that are both present and enabled. Do the last four patches here, (or indeed the whole branch), fix this issue for you: https://gitlab.arm.com/linux-arm/linux-jm/- /tree/virtual_cpu_hotplug/rfc/snapshot/20230224
The only reason not to do this, is a widely deploy set of hardware where only the present bit is set, because previously that was sufficient.
Exactly this.
I think we knew such problems would surface if we start to bank upon "enabled" than "present". There could be more such corners waiting to be rolled out.
ACPI 6.3's 6.3.7 is pretty clear that both these bits have to be set. I'd argue this is a bug in linux if its trying to probe disabled devices.
I'm not too worried about CPUs, but docks for ten year old laptops. In the worst case the wreckage should be confined to x86, so we can have a per-arch 'ignore the enabled bit' that keeps the existing behaviour.
If this is found to be true, the workaround would be to use acpi_dev_ready_for_enumeration() in all the places I've added acpi_device_is_present_and_enabled(), and abuse acpi_dev_ready_for_enumeration() to return different values depending on the type of device, the architecture and/or phase of the moon.
I guess just enabled check would have suffice here. Device has to be "present" to be "enabled"?
It is, lets hope the firmware folk have been reading that bit of the spec!
I think the best thing to do is check (enabled || functional) in acpi_dev_ready_for_enumeration(), with a patch we hope we don't have to merge that gives x86 a 'ACPI_IGNORE_STA_ENABLED' to keep the current behaviour.
Thanks,
James