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

[Igor Mammedov <imammedo@redhat.com>]

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