Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'

[Michael S. Tsirkin]

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