Hi Bernhard,
On 19/10/2023 11:33, Bernhard Beschow wrote:
Am 18. Oktober 2023 17:38:33 UTC schrieb Salil Mehta <salil.me...@opnsrc.net>:
Hello,
Hi Salil,
Can we assume that every machine type will have all the features which a GED
Device can multiplex present together? like will Memory and CPU Hotplug makes
sense for all the type of machines?
I can't really answer these questions -- I'm by no means an ACPI expert. My
idea about removing TYPE_ACPI_GED_X86 really was not more than the commit
message says: To remove unneeded code.
Sure, cleanup is not an issue.
In fact, question is whether every machine type would be interested in
initializing other code like hot-plug related initialization in the
acpi_get_intfn() especially when that machine type does not supports it.
Another question is whether every machine can without breaking other
architecture or features?
Even in your case as well some unnecessary code legs will get
initialized so cleanup is not complete either - isn't it?
For now, I will proceed with changing this for ARM and then if x86 needs
it can either revert this patch or re-implement it as also suggested by
Michael?
That said, I wonder myself if the GED device could be uniformly implemented
across architectures and if -- in theory -- it could be used in the pc-i440fx
machine instead of the Frankenstein hotplug implementation in PIIX4.
I will leave it up to x86 maintainers to answer that.
But superficially, it looks there are some historical reasons (maybe
related to legacy firmware?) because of which the switch from legacy to
modern type of CPU Hotplug interface happens.
Thanks
Salil.
Best regards,
Bernhard
If answer is no, then shouldn't every machine type override the base GED type
and define it own versions of instance_init() function? AFAICS, GED can
multiplex non-hotplug events as well.
To support CPU Htoplug on ARM platforms we are using GED but x86/microvm does
not supports hot-plugging and while creating TYPE_GED_DEVICE it will end up
initializing CPU Hotplug regions and code as well. This is far from clean.
Beside 'qtest' fails for x86/microvm machine type because
'possible_cpus_arch_ids' is not defined for x86/microvm so we get errors like
below:
stderr:
qemu-system-x86_64: ../hw/acpi/cpu.c:224: cpu_hotplug_hw_init: Assertion
`mc->possible_cpu_arch_ids' failed.
Broken pipe
../tests/qtest/libqtest.c:200: kill_qemu() detected QEMU death from signal 6
(Aborted) (core dumped)
Above can be avoided if cpu_hotplug_hw_init() does not gets called for
x86/microvm machine.
ARM can have its own version of generic_event_device_arm64.c with its own
version of instance_init() having a call to cpu_hotplug_hw_init().
Maybe I have missed something here?
Many thanks
Salil.
On 05/10/2023 04:44, Michael S. Tsirkin wrote:
From: Bernhard Beschow <shen...@gmail.com>
Now that TYPE_ACPI_GED_X86 doesn't assign AcpiDeviceIfClass::madt_cpu any more
it is the same as TYPE_ACPI_GED.
Signed-off-by: Bernhard Beschow <shen...@gmail.com>
Reviewed-by: Philippe Mathieu-Daudé <phi...@linaro.org>
Message-Id: <20230908084234.17642-6-shen...@gmail.com>
Reviewed-by: Michael S. Tsirkin <m...@redhat.com>
Signed-off-by: Michael S. Tsirkin <m...@redhat.com>
---
include/hw/acpi/generic_event_device.h | 2 --
hw/i386/generic_event_device_x86.c | 27 --------------------------
hw/i386/microvm.c | 2 +-
hw/i386/meson.build | 1 -
4 files changed, 1 insertion(+), 31 deletions(-)
delete mode 100644 hw/i386/generic_event_device_x86.c
diff --git a/include/hw/acpi/generic_event_device.h
b/include/hw/acpi/generic_event_device.h
index d831bbd889..ba84ce0214 100644
--- a/include/hw/acpi/generic_event_device.h
+++ b/include/hw/acpi/generic_event_device.h
@@ -69,8 +69,6 @@
#define TYPE_ACPI_GED "acpi-ged"
OBJECT_DECLARE_SIMPLE_TYPE(AcpiGedState, ACPI_GED)
-#define TYPE_ACPI_GED_X86 "acpi-ged-x86"
-
#define ACPI_GED_EVT_SEL_OFFSET 0x0
#define ACPI_GED_EVT_SEL_LEN 0x4
diff --git a/hw/i386/generic_event_device_x86.c
b/hw/i386/generic_event_device_x86.c
deleted file mode 100644
index 8fc233e1f1..0000000000
--- a/hw/i386/generic_event_device_x86.c
+++ /dev/null
@@ -1,27 +0,0 @@
-/*
- * x86 variant of the generic event device for hw reduced acpi
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2 or later, as published by the Free Software Foundation.
- */
-
-#include "qemu/osdep.h"
-#include "hw/acpi/generic_event_device.h"
-
-static const TypeInfo acpi_ged_x86_info = {
- .name = TYPE_ACPI_GED_X86,
- .parent = TYPE_ACPI_GED,
- .interfaces = (InterfaceInfo[]) {
- { TYPE_HOTPLUG_HANDLER },
- { TYPE_ACPI_DEVICE_IF },
- { }
- }
-};
-
-static void acpi_ged_x86_register_types(void)
-{
- type_register_static(&acpi_ged_x86_info);
-}
-
-type_init(acpi_ged_x86_register_types)
diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 8deeb62774..b9c93039e2 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -204,7 +204,7 @@ static void microvm_devices_init(MicrovmMachineState *mms)
/* Optional and legacy devices */
if (x86_machine_is_acpi_enabled(x86ms)) {
- DeviceState *dev = qdev_new(TYPE_ACPI_GED_X86);
+ DeviceState *dev = qdev_new(TYPE_ACPI_GED);
qdev_prop_set_uint32(dev, "ged-event", ACPI_GED_PWR_DOWN_EVT);
sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, GED_MMIO_BASE);
/* sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, GED_MMIO_BASE_MEMHP); */
diff --git a/hw/i386/meson.build b/hw/i386/meson.build
index cfdbfdcbcb..ff879069c9 100644
--- a/hw/i386/meson.build
+++ b/hw/i386/meson.build
@@ -20,7 +20,6 @@ i386_ss.add(when: 'CONFIG_SGX', if_true:
files('sgx-epc.c','sgx.c'),
if_false: files('sgx-stub.c'))
i386_ss.add(when: 'CONFIG_ACPI', if_true: files('acpi-common.c'))
-i386_ss.add(when: 'CONFIG_ACPI_HW_REDUCED', if_true:
files('generic_event_device_x86.c'))
i386_ss.add(when: 'CONFIG_PC', if_true: files(
'pc.c',
'pc_sysfw.c',
