Hi Igor,
On 06/07/2024 14:28, Igor Mammedov wrote:
On Fri, 7 Jun 2024 12:56:45 +0100
Salil Mehta <salil.me...@huawei.com> wrote:
OSPM evaluates _EVT method to map the event. The CPU hotplug event eventually
results in start of the CPU scan. Scan figures out the CPU and the kind of
event(plug/unplug) and notifies it back to the guest. Update the GED AML _EVT
method with the call to \\_SB.CPUS.CSCN
Also, macro CPU_SCAN_METHOD might be referred in other places like during GED
intialization so it makes sense to have its definition placed in some common
header file like cpu_hotplug.h. But doing this can cause compilation break
because of the conflicting macro definitions present in cpu.c and cpu_hotplug.c
one of the reasons is that you reusing legacy hw/acpi/cpu_hotplug.h,
see below for suggestion.
ok
and because both these files get compiled due to historic reasons of x86 world
i.e. decision to use legacy(GPE.2)/modern(GED) CPU hotplug interface happens
during runtime [1]. To mitigate above, for now, declare a new common macro
ACPI_CPU_SCAN_METHOD for CPU scan method instead.
(This needs a separate discussion later on for clean-up)
Reference:
[1]
https://lore.kernel.org/qemu-devel/1463496205-251412-24-git-send-email-imamm...@redhat.com/
Co-developed-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Keqian Zhu <zhukeqi...@huawei.com>
Signed-off-by: Salil Mehta <salil.me...@huawei.com>
Reviewed-by: Jonathan Cameron <jonathan.came...@huawei.com>
Reviewed-by: Gavin Shan <gs...@redhat.com>
Tested-by: Vishnu Pajjuri <vis...@os.amperecomputing.com>
Tested-by: Xianglai Li <lixiang...@loongson.cn>
Tested-by: Miguel Luis <miguel.l...@oracle.com>
Reviewed-by: Shaoqin Huang <shahu...@redhat.com>
Tested-by: Zhao Liu <zhao1....@intel.com>
---
hw/acpi/cpu.c | 2 +-
hw/acpi/generic_event_device.c | 4 ++++
include/hw/acpi/cpu_hotplug.h | 2 ++
3 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c
index 473b37ba88..af2b6655d2 100644
--- a/hw/acpi/cpu.c
+++ b/hw/acpi/cpu.c
@@ -327,7 +327,7 @@ const VMStateDescription vmstate_cpu_hotplug = {
#define CPUHP_RES_DEVICE "PRES"
#define CPU_LOCK "CPLK"
#define CPU_STS_METHOD "CSTA"
-#define CPU_SCAN_METHOD "CSCN"
+#define CPU_SCAN_METHOD ACPI_CPU_SCAN_METHOD
#define CPU_NOTIFY_METHOD "CTFY"
#define CPU_EJECT_METHOD "CEJ0"
#define CPU_OST_METHOD "COST"
diff --git a/hw/acpi/generic_event_device.c b/hw/acpi/generic_event_device.c
index 54d3b4bf9d..63226b0040 100644
--- a/hw/acpi/generic_event_device.c
+++ b/hw/acpi/generic_event_device.c
@@ -109,6 +109,10 @@ void build_ged_aml(Aml *table, const char *name,
HotplugHandler *hotplug_dev,
aml_append(if_ctx, aml_call0(MEMORY_DEVICES_CONTAINER "."
MEMORY_SLOT_SCAN_METHOD));
break;
+ case ACPI_GED_CPU_HOTPLUG_EVT:
+ aml_append(if_ctx, aml_call0(ACPI_CPU_CONTAINER "."
+ ACPI_CPU_SCAN_METHOD));
I don't particularly like exposing cpu hotplug internals for outside code
and then making that code do plumbing hoping that nothing will explode
in the future.
I understand your point but I've followed what was already existing.
For example,
build_dsdt()
{
[...]
acpi_dsdt_add_uart(scope, &memmap[VIRT_UART],
(irqmap[VIRT_UART] + ARM_SPI_BASE));
if (vmc->acpi_expose_flash) {
acpi_dsdt_add_flash(scope, &memmap[VIRT_FLASH]);
}
fw_cfg_acpi_dsdt_add(scope, &memmap[VIRT_FW_CFG]);
virtio_acpi_dsdt_add(scope, memmap[VIRT_MMIO].base,
memmap[VIRT_MMIO].size,
(irqmap[VIRT_MMIO] + ARM_SPI_BASE),
0, NUM_VIRTIO_TRANSPORTS);
acpi_dsdt_add_pci(scope, memmap, irqmap[VIRT_PCIE] + ARM_SPI_BASE,
vms);
if (vms->acpi_dev) {
build_ged_aml(scope, "\\_SB."GED_DEVICE,
HOTPLUG_HANDLER(vms->acpi_dev),
irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE,
AML_SYSTEM_MEMORY,
memmap[VIRT_ACPI_GED].base);
} else {
acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
(irqmap[VIRT_GPIO] + ARM_SPI_BASE));
}
[...]
}
Refactoring all of this code will create a noise in this patch-set.
build_cpus_aml() takes event_handler_method to create a method that
can be called by platform. What I suggest is to call that method here
instead of trying to expose CPU hotplug internals and manually building
call path here.
aka:
build_cpus_aml(event_handler_method = PATH_TO_GED_DEVICE.CSCN)
and then call here
aml_append(if_ctx, aml_call0(CSCN));
which will call CSCN in GED scope, that was be populated by
build_cpus_aml() to do cpu scan properly without need to expose
cpu hotplug internal names and then trying to fixup conflicts caused by that.
PS:
we should do the same for memory hotplug, we see in context above
Although. I agree with your suggested change but I think this should be
carried in another patch-set.
Best, Salil.
+ break;
case ACPI_GED_PWR_DOWN_EVT:
aml_append(if_ctx,
aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE),
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 48b291e45e..ef631750b4 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -20,6 +20,8 @@
#include "hw/acpi/cpu.h"
#define ACPI_CPU_HOTPLUG_REG_LEN 12
+#define ACPI_CPU_SCAN_METHOD "CSCN"
+#define ACPI_CPU_CONTAINER "\\_SB.CPUS"
typedef struct AcpiCpuHotplug {
Object *device;
