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

[Bernhard Beschow]

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