ACPI CPU hotplug parent can't be any QOM object, it must be a QDev.
Convert AcpiCpuHotplug::device field as QDev to enforce this.
Rename 'owner' and 'device' variables as 'parent'.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
hw/acpi/acpi-cpu-hotplug-stub.c | 2 +-
hw/acpi/cpu_hotplug.c | 18 +++++++++---------
hw/acpi/ich9.c | 5 +++--
hw/acpi/piix4.c | 2 +-
include/hw/acpi/cpu_hotplug.h | 4 ++--
5 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
index cbd7a6ec00..0fcc1ec8ea 100644
--- a/hw/acpi/acpi-cpu-hotplug-stub.c
+++ b/hw/acpi/acpi-cpu-hotplug-stub.c
@@ -13,7 +13,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
return;
}
-void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
+void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
AcpiCpuHotplug *gpe, uint16_t base)
{
return;
diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
index 636e985c50..b8c9081738 100644
--- a/hw/acpi/cpu_hotplug.c
+++ b/hw/acpi/cpu_hotplug.c
@@ -41,8 +41,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
*/
if (addr == 0 && data == 0) {
AcpiCpuHotplug *gpe = opaque;
- object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
- &error_abort);
+ object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
+ false, &error_abort);
}
}
@@ -66,8 +66,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
cpu_id = k->get_arch_id(cpu);
if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
- object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
- &error_abort);
+ object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
+ false, &error_abort);
return;
}
@@ -81,15 +81,15 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
}
-void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
+void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
AcpiCpuHotplug *gpe, uint16_t base)
{
CPUState *cpu;
- memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
+ memory_region_init_io(&gpe->io, OBJECT(parent), &AcpiCpuHotplug_ops,
gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
memory_region_add_subregion(container, base, &gpe->io);
- gpe->device = owner;
+ gpe->parent = parent;
CPU_FOREACH(cpu) {
acpi_set_cpu_present_bit(gpe, cpu);
@@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
CPUHotplugState *cpuhp_state,
uint16_t io_port)
{
- MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
+ MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->parent));
memory_region_del_subregion(container, &gpe->io);
- cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
+ cpu_hotplug_hw_init(container, OBJECT(gpe->parent), cpuhp_state, io_port);
}
void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
index 4f8385b894..6c9a737479 100644
--- a/hw/acpi/ich9.c
+++ b/hw/acpi/ich9.c
@@ -197,7 +197,7 @@ static bool vmstate_test_use_cpuhp(void *opaque)
static int vmstate_cpuhp_pre_load(void *opaque)
{
ICH9LPCPMRegs *s = opaque;
- Object *obj = OBJECT(s->gpe.device);
+ Object *obj = OBJECT(s->gpe.parent);
object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
return 0;
}
@@ -338,7 +338,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
qemu_register_powerdown_notifier(&pm->powerdown_notifier);
legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
- OBJECT(lpc_pci), &pm->gpe, ICH9_CPU_HOTPLUG_IO_BASE);
+ DEVICE(lpc_pci), &pm->gpe,
+ ICH9_CPU_HOTPLUG_IO_BASE);
if (pm->acpi_memory_hotplug.is_enabled) {
acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 5595fe5be5..3a61d89f92 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -571,7 +571,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
piix4_get_cpu_hotplug_legacy,
piix4_set_cpu_hotplug_legacy);
- legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
+ legacy_acpi_cpu_hotplug_init(container, DEVICE(s), &s->gpe,
PIIX4_CPU_HOTPLUG_IO_BASE);
if (s->acpi_memory_hotplug.is_enabled) {
diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
index 5ff24ec417..b2f990f0b8 100644
--- a/include/hw/acpi/cpu_hotplug.h
+++ b/include/hw/acpi/cpu_hotplug.h
@@ -20,7 +20,7 @@
#include "hw/acpi/cpu.h"
typedef struct AcpiCpuHotplug {
- Object *device;
+ DeviceState *parent;
MemoryRegion io;
uint8_t sts[ACPI_GPE_PROC_LEN];
} AcpiCpuHotplug;
@@ -28,7 +28,7 @@ typedef struct AcpiCpuHotplug {
void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
DeviceState *dev, Error **errp);
-void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
+void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
AcpiCpuHotplug *gpe, uint16_t base);
void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
--
2.38.1
On Fri, Feb 03, 2023 at 05:30:21PM +0100, Philippe Mathieu-Daudé wrote:
> ACPI CPU hotplug parent can't be any QOM object, it must be a QDev.
> Convert AcpiCpuHotplug::device field as QDev to enforce this.
> Rename 'owner' and 'device' variables as 'parent'.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
So instead of plain gpe->device we now have OBJECT all over
the place. Why is this an improvement?
> ---
> hw/acpi/acpi-cpu-hotplug-stub.c | 2 +-
> hw/acpi/cpu_hotplug.c | 18 +++++++++---------
> hw/acpi/ich9.c | 5 +++--
> hw/acpi/piix4.c | 2 +-
> include/hw/acpi/cpu_hotplug.h | 4 ++--
> 5 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index cbd7a6ec00..0fcc1ec8ea 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -13,7 +13,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> return;
> }
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base)
> {
> return;
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 636e985c50..b8c9081738 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -41,8 +41,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> */
> if (addr == 0 && data == 0) {
> AcpiCpuHotplug *gpe = opaque;
> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> - &error_abort);
> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> + false, &error_abort);
> }
> }
>
> @@ -66,8 +66,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
>
> cpu_id = k->get_arch_id(cpu);
> if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> - &error_abort);
> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> + false, &error_abort);
> return;
> }
>
> @@ -81,15 +81,15 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
> }
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base)
> {
> CPUState *cpu;
>
> - memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
> + memory_region_init_io(&gpe->io, OBJECT(parent), &AcpiCpuHotplug_ops,
> gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> memory_region_add_subregion(container, base, &gpe->io);
> - gpe->device = owner;
> + gpe->parent = parent;
>
> CPU_FOREACH(cpu) {
> acpi_set_cpu_present_bit(gpe, cpu);
> @@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> CPUHotplugState *cpuhp_state,
> uint16_t io_port)
> {
> - MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
> + MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->parent));
>
> memory_region_del_subregion(container, &gpe->io);
> - cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
> + cpu_hotplug_hw_init(container, OBJECT(gpe->parent), cpuhp_state, io_port);
> }
>
> void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 4f8385b894..6c9a737479 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -197,7 +197,7 @@ static bool vmstate_test_use_cpuhp(void *opaque)
> static int vmstate_cpuhp_pre_load(void *opaque)
> {
> ICH9LPCPMRegs *s = opaque;
> - Object *obj = OBJECT(s->gpe.device);
> + Object *obj = OBJECT(s->gpe.parent);
> object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
> return 0;
> }
> @@ -338,7 +338,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>
> legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> - OBJECT(lpc_pci), &pm->gpe, ICH9_CPU_HOTPLUG_IO_BASE);
> + DEVICE(lpc_pci), &pm->gpe,
> + ICH9_CPU_HOTPLUG_IO_BASE);
>
> if (pm->acpi_memory_hotplug.is_enabled) {
> acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5595fe5be5..3a61d89f92 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -571,7 +571,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> piix4_get_cpu_hotplug_legacy,
> piix4_set_cpu_hotplug_legacy);
> - legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
> + legacy_acpi_cpu_hotplug_init(container, DEVICE(s), &s->gpe,
> PIIX4_CPU_HOTPLUG_IO_BASE);
>
> if (s->acpi_memory_hotplug.is_enabled) {
> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> index 5ff24ec417..b2f990f0b8 100644
> --- a/include/hw/acpi/cpu_hotplug.h
> +++ b/include/hw/acpi/cpu_hotplug.h
> @@ -20,7 +20,7 @@
> #include "hw/acpi/cpu.h"
>
> typedef struct AcpiCpuHotplug {
> - Object *device;
> + DeviceState *parent;
> MemoryRegion io;
> uint8_t sts[ACPI_GPE_PROC_LEN];
> } AcpiCpuHotplug;
> @@ -28,7 +28,7 @@ typedef struct AcpiCpuHotplug {
> void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> DeviceState *dev, Error **errp);
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base);
>
> void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> --
> 2.38.1
On 28/2/23 22:41, Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2023 at 05:30:21PM +0100, Philippe Mathieu-Daudé wrote:
>> ACPI CPU hotplug parent can't be any QOM object, it must be a QDev.
>> Convert AcpiCpuHotplug::device field as QDev to enforce this.
>> Rename 'owner' and 'device' variables as 'parent'.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>
>
> So instead of plain gpe->device we now have OBJECT all over
> the place. Why is this an improvement?
From QOM PoV, in this prototype change:
void legacy_acpi_cpu_hotplug_init(MemoryRegion *container,
- Object *owner,
+ DeviceState *parent,
we promote the parent/owner argument from generic Object to
Device, which is more restrictive.
Thus now you can not pass any QOM object, it has to be a QDev.
>> ---
>> hw/acpi/acpi-cpu-hotplug-stub.c | 2 +-
>> hw/acpi/cpu_hotplug.c | 18 +++++++++---------
>> hw/acpi/ich9.c | 5 +++--
>> hw/acpi/piix4.c | 2 +-
>> include/hw/acpi/cpu_hotplug.h | 4 ++--
>> 5 files changed, 16 insertions(+), 15 deletions(-)
>>
>> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
>> index cbd7a6ec00..0fcc1ec8ea 100644
>> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
>> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
>> @@ -13,7 +13,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>> return;
>> }
>>
>> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
>> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
>> AcpiCpuHotplug *gpe, uint16_t base)
>> {
>> return;
>> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
>> index 636e985c50..b8c9081738 100644
>> --- a/hw/acpi/cpu_hotplug.c
>> +++ b/hw/acpi/cpu_hotplug.c
>> @@ -41,8 +41,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
>> */
>> if (addr == 0 && data == 0) {
>> AcpiCpuHotplug *gpe = opaque;
>> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
>> - &error_abort);
>> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
>> + false, &error_abort);
>> }
>> }
>>
>> @@ -66,8 +66,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
>>
>> cpu_id = k->get_arch_id(cpu);
>> if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
>> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
>> - &error_abort);
>> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
>> + false, &error_abort);
>> return;
>> }
>>
>> @@ -81,15 +81,15 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
>> acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
>> }
>>
>> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
>> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
>> AcpiCpuHotplug *gpe, uint16_t base)
>> {
>> CPUState *cpu;
>>
>> - memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
>> + memory_region_init_io(&gpe->io, OBJECT(parent), &AcpiCpuHotplug_ops,
>> gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
>> memory_region_add_subregion(container, base, &gpe->io);
>> - gpe->device = owner;
>> + gpe->parent = parent;
>>
>> CPU_FOREACH(cpu) {
>> acpi_set_cpu_present_bit(gpe, cpu);
>> @@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>> CPUHotplugState *cpuhp_state,
>> uint16_t io_port)
>> {
>> - MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
>> + MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->parent));
>>
>> memory_region_del_subregion(container, &gpe->io);
>> - cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
>> + cpu_hotplug_hw_init(container, OBJECT(gpe->parent), cpuhp_state, io_port);
>> }
>>
>> void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
>> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
>> index 4f8385b894..6c9a737479 100644
>> --- a/hw/acpi/ich9.c
>> +++ b/hw/acpi/ich9.c
>> @@ -197,7 +197,7 @@ static bool vmstate_test_use_cpuhp(void *opaque)
>> static int vmstate_cpuhp_pre_load(void *opaque)
>> {
>> ICH9LPCPMRegs *s = opaque;
>> - Object *obj = OBJECT(s->gpe.device);
>> + Object *obj = OBJECT(s->gpe.parent);
>> object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
>> return 0;
>> }
>> @@ -338,7 +338,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>>
>> legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
>> - OBJECT(lpc_pci), &pm->gpe, ICH9_CPU_HOTPLUG_IO_BASE);
>> + DEVICE(lpc_pci), &pm->gpe,
>> + ICH9_CPU_HOTPLUG_IO_BASE);
>>
>> if (pm->acpi_memory_hotplug.is_enabled) {
>> acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 5595fe5be5..3a61d89f92 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -571,7 +571,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
>> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
>> piix4_get_cpu_hotplug_legacy,
>> piix4_set_cpu_hotplug_legacy);
>> - legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
>> + legacy_acpi_cpu_hotplug_init(container, DEVICE(s), &s->gpe,
>> PIIX4_CPU_HOTPLUG_IO_BASE);
>>
>> if (s->acpi_memory_hotplug.is_enabled) {
>> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
>> index 5ff24ec417..b2f990f0b8 100644
>> --- a/include/hw/acpi/cpu_hotplug.h
>> +++ b/include/hw/acpi/cpu_hotplug.h
>> @@ -20,7 +20,7 @@
>> #include "hw/acpi/cpu.h"
>>
>> typedef struct AcpiCpuHotplug {
>> - Object *device;
>> + DeviceState *parent;
>> MemoryRegion io;
>> uint8_t sts[ACPI_GPE_PROC_LEN];
>> } AcpiCpuHotplug;
>> @@ -28,7 +28,7 @@ typedef struct AcpiCpuHotplug {
>> void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
>> DeviceState *dev, Error **errp);
>>
>> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
>> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
>> AcpiCpuHotplug *gpe, uint16_t base);
>>
>> void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>> --
>> 2.38.1
>
On Tue, Feb 28, 2023 at 11:05:24PM +0100, Philippe Mathieu-Daudé wrote:
> On 28/2/23 22:41, Michael S. Tsirkin wrote:
> > On Fri, Feb 03, 2023 at 05:30:21PM +0100, Philippe Mathieu-Daudé wrote:
> > > ACPI CPU hotplug parent can't be any QOM object, it must be a QDev.
> > > Convert AcpiCpuHotplug::device field as QDev to enforce this.
> > > Rename 'owner' and 'device' variables as 'parent'.
> > >
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> >
> >
> > So instead of plain gpe->device we now have OBJECT all over
> > the place. Why is this an improvement?
>
> From QOM PoV, in this prototype change:
>
> void legacy_acpi_cpu_hotplug_init(MemoryRegion *container,
> - Object *owner,
> + DeviceState *parent,
>
> we promote the parent/owner argument from generic Object to
> Device, which is more restrictive.
>
> Thus now you can not pass any QOM object, it has to be a QDev.
None of the functions really seem to care what kind of object
they get. Anything that results in less casting is a win
in my book. More casting - a loss.
> > > ---
> > > hw/acpi/acpi-cpu-hotplug-stub.c | 2 +-
> > > hw/acpi/cpu_hotplug.c | 18 +++++++++---------
> > > hw/acpi/ich9.c | 5 +++--
> > > hw/acpi/piix4.c | 2 +-
> > > include/hw/acpi/cpu_hotplug.h | 4 ++--
> > > 5 files changed, 16 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> > > index cbd7a6ec00..0fcc1ec8ea 100644
> > > --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> > > +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> > > @@ -13,7 +13,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> > > return;
> > > }
> > > -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> > > AcpiCpuHotplug *gpe, uint16_t base)
> > > {
> > > return;
> > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > > index 636e985c50..b8c9081738 100644
> > > --- a/hw/acpi/cpu_hotplug.c
> > > +++ b/hw/acpi/cpu_hotplug.c
> > > @@ -41,8 +41,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> > > */
> > > if (addr == 0 && data == 0) {
> > > AcpiCpuHotplug *gpe = opaque;
> > > - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> > > - &error_abort);
> > > + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> > > + false, &error_abort);
> > > }
> > > }
> > > @@ -66,8 +66,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
> > > cpu_id = k->get_arch_id(cpu);
> > > if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
> > > - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> > > - &error_abort);
> > > + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> > > + false, &error_abort);
> > > return;
> > > }
> > > @@ -81,15 +81,15 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> > > acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
> > > }
> > > -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> > > AcpiCpuHotplug *gpe, uint16_t base)
> > > {
> > > CPUState *cpu;
> > > - memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
> > > + memory_region_init_io(&gpe->io, OBJECT(parent), &AcpiCpuHotplug_ops,
> > > gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> > > memory_region_add_subregion(container, base, &gpe->io);
> > > - gpe->device = owner;
> > > + gpe->parent = parent;
> > > CPU_FOREACH(cpu) {
> > > acpi_set_cpu_present_bit(gpe, cpu);
> > > @@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> > > CPUHotplugState *cpuhp_state,
> > > uint16_t io_port)
> > > {
> > > - MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
> > > + MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->parent));
> > > memory_region_del_subregion(container, &gpe->io);
> > > - cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
> > > + cpu_hotplug_hw_init(container, OBJECT(gpe->parent), cpuhp_state, io_port);
> > > }
> > > void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> > > index 4f8385b894..6c9a737479 100644
> > > --- a/hw/acpi/ich9.c
> > > +++ b/hw/acpi/ich9.c
> > > @@ -197,7 +197,7 @@ static bool vmstate_test_use_cpuhp(void *opaque)
> > > static int vmstate_cpuhp_pre_load(void *opaque)
> > > {
> > > ICH9LPCPMRegs *s = opaque;
> > > - Object *obj = OBJECT(s->gpe.device);
> > > + Object *obj = OBJECT(s->gpe.parent);
> > > object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
> > > return 0;
> > > }
> > > @@ -338,7 +338,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> > > qemu_register_powerdown_notifier(&pm->powerdown_notifier);
> > > legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> > > - OBJECT(lpc_pci), &pm->gpe, ICH9_CPU_HOTPLUG_IO_BASE);
> > > + DEVICE(lpc_pci), &pm->gpe,
> > > + ICH9_CPU_HOTPLUG_IO_BASE);
> > > if (pm->acpi_memory_hotplug.is_enabled) {
> > > acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index 5595fe5be5..3a61d89f92 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -571,7 +571,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
> > > object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > > piix4_get_cpu_hotplug_legacy,
> > > piix4_set_cpu_hotplug_legacy);
> > > - legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
> > > + legacy_acpi_cpu_hotplug_init(container, DEVICE(s), &s->gpe,
> > > PIIX4_CPU_HOTPLUG_IO_BASE);
> > > if (s->acpi_memory_hotplug.is_enabled) {
> > > diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> > > index 5ff24ec417..b2f990f0b8 100644
> > > --- a/include/hw/acpi/cpu_hotplug.h
> > > +++ b/include/hw/acpi/cpu_hotplug.h
> > > @@ -20,7 +20,7 @@
> > > #include "hw/acpi/cpu.h"
> > > typedef struct AcpiCpuHotplug {
> > > - Object *device;
> > > + DeviceState *parent;
> > > MemoryRegion io;
> > > uint8_t sts[ACPI_GPE_PROC_LEN];
> > > } AcpiCpuHotplug;
> > > @@ -28,7 +28,7 @@ typedef struct AcpiCpuHotplug {
> > > void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> > > DeviceState *dev, Error **errp);
> > > -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> > > AcpiCpuHotplug *gpe, uint16_t base);
> > > void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> > > --
> > > 2.38.1
> >
On 28/2/23 23:09, Michael S. Tsirkin wrote: > On Tue, Feb 28, 2023 at 11:05:24PM +0100, Philippe Mathieu-Daudé wrote: >> On 28/2/23 22:41, Michael S. Tsirkin wrote: >>> On Fri, Feb 03, 2023 at 05:30:21PM +0100, Philippe Mathieu-Daudé wrote: >>>> ACPI CPU hotplug parent can't be any QOM object, it must be a QDev. >>>> Convert AcpiCpuHotplug::device field as QDev to enforce this. >>>> Rename 'owner' and 'device' variables as 'parent'. >>>> >>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org> >>> >>> >>> So instead of plain gpe->device we now have OBJECT all over >>> the place. Why is this an improvement? >> >> From QOM PoV, in this prototype change: >> >> void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, >> - Object *owner, >> + DeviceState *parent, >> >> we promote the parent/owner argument from generic Object to >> Device, which is more restrictive. >> >> Thus now you can not pass any QOM object, it has to be a QDev. > > None of the functions really seem to care what kind of object > they get. Anything that results in less casting is a win > in my book. More casting - a loss. OK we are discussing QOM design then. I'll go back to this in a future RFC. Sorry for having wasted your time meanwhile.
On Fri, 3 Feb 2023 17:30:21 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> ACPI CPU hotplug parent can't be any QOM object, it must be a QDev.
> Convert AcpiCpuHotplug::device field as QDev to enforce this.
> Rename 'owner' and 'device' variables as 'parent'.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> hw/acpi/acpi-cpu-hotplug-stub.c | 2 +-
> hw/acpi/cpu_hotplug.c | 18 +++++++++---------
> hw/acpi/ich9.c | 5 +++--
> hw/acpi/piix4.c | 2 +-
> include/hw/acpi/cpu_hotplug.h | 4 ++--
> 5 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> index cbd7a6ec00..0fcc1ec8ea 100644
> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
> @@ -13,7 +13,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> return;
> }
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base)
> {
> return;
> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> index 636e985c50..b8c9081738 100644
> --- a/hw/acpi/cpu_hotplug.c
> +++ b/hw/acpi/cpu_hotplug.c
> @@ -41,8 +41,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
> */
> if (addr == 0 && data == 0) {
> AcpiCpuHotplug *gpe = opaque;
> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> - &error_abort);
> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> + false, &error_abort);
> }
> }
>
> @@ -66,8 +66,8 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
>
> cpu_id = k->get_arch_id(cpu);
> if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
> - object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
> - &error_abort);
> + object_property_set_bool(OBJECT(gpe->parent), "cpu-hotplug-legacy",
> + false, &error_abort);
> return;
> }
>
> @@ -81,15 +81,15 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
> }
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base)
> {
> CPUState *cpu;
>
> - memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
> + memory_region_init_io(&gpe->io, OBJECT(parent), &AcpiCpuHotplug_ops,
> gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> memory_region_add_subregion(container, base, &gpe->io);
> - gpe->device = owner;
> + gpe->parent = parent;
>
> CPU_FOREACH(cpu) {
> acpi_set_cpu_present_bit(gpe, cpu);
> @@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> CPUHotplugState *cpuhp_state,
> uint16_t io_port)
> {
> - MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
> + MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->parent));
>
> memory_region_del_subregion(container, &gpe->io);
> - cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
> + cpu_hotplug_hw_init(container, OBJECT(gpe->parent), cpuhp_state, io_port);
did you forget to convert (..., Object *owner) here to DeviceState*?
> }
>
> void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 4f8385b894..6c9a737479 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -197,7 +197,7 @@ static bool vmstate_test_use_cpuhp(void *opaque)
> static int vmstate_cpuhp_pre_load(void *opaque)
> {
> ICH9LPCPMRegs *s = opaque;
> - Object *obj = OBJECT(s->gpe.device);
> + Object *obj = OBJECT(s->gpe.parent);
> object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
> return 0;
> }
> @@ -338,7 +338,8 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> qemu_register_powerdown_notifier(&pm->powerdown_notifier);
>
> legacy_acpi_cpu_hotplug_init(pci_address_space_io(lpc_pci),
> - OBJECT(lpc_pci), &pm->gpe, ICH9_CPU_HOTPLUG_IO_BASE);
> + DEVICE(lpc_pci), &pm->gpe,
> + ICH9_CPU_HOTPLUG_IO_BASE);
>
> if (pm->acpi_memory_hotplug.is_enabled) {
> acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), OBJECT(lpc_pci),
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 5595fe5be5..3a61d89f92 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -571,7 +571,7 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
> object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> piix4_get_cpu_hotplug_legacy,
> piix4_set_cpu_hotplug_legacy);
> - legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
> + legacy_acpi_cpu_hotplug_init(container, DEVICE(s), &s->gpe,
> PIIX4_CPU_HOTPLUG_IO_BASE);
>
> if (s->acpi_memory_hotplug.is_enabled) {
> diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> index 5ff24ec417..b2f990f0b8 100644
> --- a/include/hw/acpi/cpu_hotplug.h
> +++ b/include/hw/acpi/cpu_hotplug.h
> @@ -20,7 +20,7 @@
> #include "hw/acpi/cpu.h"
>
> typedef struct AcpiCpuHotplug {
> - Object *device;
> + DeviceState *parent;
> MemoryRegion io;
> uint8_t sts[ACPI_GPE_PROC_LEN];
> } AcpiCpuHotplug;
> @@ -28,7 +28,7 @@ typedef struct AcpiCpuHotplug {
> void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
> DeviceState *dev, Error **errp);
>
> -void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, DeviceState *parent,
> AcpiCpuHotplug *gpe, uint16_t base);
>
> void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
© 2016 - 2026 Red Hat, Inc.