| Message ID | 20230203163021.35754-4-philmd@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' | expand |
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,
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.
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,
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(-)