| Message ID | c9c8ba69d5dbe5c1c6370e1f09ebd7531509d075.1696477105.git.mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,v2,01/53] pci: SLT must be RO | expand |
Hello, 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? 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 <shentey@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 <shentey@gmail.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Message-Id: <20230908084234.17642-6-shentey@gmail.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@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', >
Am 18. Oktober 2023 17:38:33 UTC schrieb Salil Mehta <salil.mehta@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. 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. 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 <shentey@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 <shentey@gmail.com> >> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >> Message-Id: <20230908084234.17642-6-shentey@gmail.com> >> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> Signed-off-by: Michael S. Tsirkin <mst@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', >>
On Thu, Oct 19, 2023 at 10:33:08AM +0000, Bernhard Beschow wrote: > > > Am 18. Oktober 2023 17:38:33 UTC schrieb Salil Mehta <salil.mehta@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. > > 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. > > Best regards, > Bernhard I am redoing the pull anyway. I dropped this until this discussion concludes then.
Hi Bernhard, On 19/10/2023 11:33, Bernhard Beschow wrote: > > > Am 18. Oktober 2023 17:38:33 UTC schrieb Salil Mehta <salil.mehta@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 <shentey@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 <shentey@gmail.com> >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> Message-Id: <20230908084234.17642-6-shentey@gmail.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@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', >>>
On Sat, 21 Oct 2023 00:54:56 +0100 Salil Mehta <salil.mehta@opnsrc.net> wrote: > Hi Bernhard, > > On 19/10/2023 11:33, Bernhard Beschow wrote: > > > > > > Am 18. Oktober 2023 17:38:33 UTC schrieb Salil Mehta <salil.mehta@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? user (board) were supposed to opt-in by setting the events property so only enabled events would be handled. > > 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. x86 can theoretically use GED as well but that will prevent hotplug working with old guests that don't know about new-ish GED. Hence it's not likely for pc/q35 to switch to new GED. > > > 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 <shentey@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 <shentey@gmail.com> > >>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > >>> Message-Id: <20230908084234.17642-6-shentey@gmail.com> > >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > >>> Signed-off-by: Michael S. Tsirkin <mst@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', > >>> >
On Wed, 18 Oct 2023 18:38:33 +0100 Salil Mehta <salil.mehta@opnsrc.net> wrote: > Hello, > 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? > > 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. cpu_hotplug_hw_init() should not be called at initfn time, but rather at realize time. > > 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(). lets try to avoid that for now. > > Maybe I have missed something here? > > > Many thanks > Salil. > > > On 05/10/2023 04:44, Michael S. Tsirkin wrote: > > From: Bernhard Beschow <shentey@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 <shentey@gmail.com> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Message-Id: <20230908084234.17642-6-shentey@gmail.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@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', > > >
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',