Re: [PULL v2 29/53] hw/i386: Remove now redundant TYPE_ACPI_GED_X86

[Igor Mammedov]

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',
> >   
>