Re: [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Feb 28, 2023 at 11:16:46PM +0100, Philippe Mathieu-Daudé wrote:
> On 28/2/23 22:43, Michael S. Tsirkin wrote:
> > On Fri, Feb 03, 2023 at 05:30:20PM +0100, Philippe Mathieu-Daudé wrote:
> > > No logical change, rename for clarity.
> > > 
> > > Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > 
> > Can't say container is clearer. If we are changing things
> > I'd like to see a real improvement.
> 
> QOM API usually use 'parent' to indicate the inheritance / hierarchy
> between object.
> Memory API uses 'container', in particular when used with
> memory_region_add_subregion().

It says "container" in the comment but not in the API:

/**
 * memory_region_add_subregion: Add a subregion to a container.
 *  
 * Adds a subregion at @offset.  The subregion may not overlap with other
 * subregions (except for those explicitly marked as overlapping).  A region
 * may only be added once as a subregion (unless removed with
 * memory_region_del_subregion()); use memory_region_init_alias() if you
 * want a region to be a subregion in multiple locations.
 *
 * @mr: the region to contain the new subregion; must be a container
 *      initialized with memory_region_init().
 * @offset: the offset relative to @mr where @subregion is added.
 * @subregion: the subregion to be added.
 */
void memory_region_add_subregion(MemoryRegion *mr,
                                 hwaddr offset,
                                 MemoryRegion *subregion);


> I still believe this is a worthwhile change as API style cleanup,
> but I agree the commit description lacks this explanation.

If you are cleaning up APIs it's a good idea to add documentation.
As long as you don't these renames don't add much.

> > > ---
> > >   hw/acpi/acpi-cpu-hotplug-stub.c |  2 +-
> > >   hw/acpi/cpu_hotplug.c           | 10 +++++-----
> > >   hw/acpi/piix4.c                 | 10 +++++-----
> > >   include/hw/acpi/cpu_hotplug.h   |  2 +-
> > >   4 files changed, 12 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
> > > index b590ad57e1..cbd7a6ec00 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 *parent, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > >                                     AcpiCpuHotplug *gpe, uint16_t base)
> > >   {
> > >       return;
> > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
> > > index 3cfa4f45dc..636e985c50 100644
> > > --- a/hw/acpi/cpu_hotplug.c
> > > +++ b/hw/acpi/cpu_hotplug.c
> > > @@ -81,14 +81,14 @@ 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 *parent, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > >                                     AcpiCpuHotplug *gpe, uint16_t base)
> > >   {
> > >       CPUState *cpu;
> > >       memory_region_init_io(&gpe->io, owner, &AcpiCpuHotplug_ops,
> > >                             gpe, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
> > > -    memory_region_add_subregion(parent, base, &gpe->io);
> > > +    memory_region_add_subregion(container, base, &gpe->io);
> > >       gpe->device = owner;
> > >       CPU_FOREACH(cpu) {
> > > @@ -100,10 +100,10 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> > >                                   CPUHotplugState *cpuhp_state,
> > >                                   uint16_t io_port)
> > >   {
> > > -    MemoryRegion *parent = pci_address_space_io(PCI_DEVICE(gpe->device));
> > > +    MemoryRegion *container = pci_address_space_io(PCI_DEVICE(gpe->device));
> > > -    memory_region_del_subregion(parent, &gpe->io);
> > > -    cpu_hotplug_hw_init(parent, gpe->device, cpuhp_state, io_port);
> > > +    memory_region_del_subregion(container, &gpe->io);
> > > +    cpu_hotplug_hw_init(container, gpe->device, cpuhp_state, io_port);
> > >   }
> > >   void build_legacy_cpu_hotplug_aml(Aml *ctx, MachineState *machine,
> > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > > index c702d83f27..5595fe5be5 100644
> > > --- a/hw/acpi/piix4.c
> > > +++ b/hw/acpi/piix4.c
> > > @@ -555,15 +555,15 @@ static void piix4_set_cpu_hotplug_legacy(Object *obj, bool value, Error **errp)
> > >       s->cpu_hotplug_legacy = value;
> > >   }
> > > -static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > > +static void piix4_acpi_system_hot_add_init(MemoryRegion *container,
> > >                                              PCIBus *bus, PIIX4PMState *s)
> > >   {
> > >       memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s,
> > >                             "acpi-gpe0", GPE_LEN);
> > > -    memory_region_add_subregion(parent, GPE_BASE, &s->io_gpe);
> > > +    memory_region_add_subregion(container, GPE_BASE, &s->io_gpe);
> > >       if (s->use_acpi_hotplug_bridge || s->use_acpi_root_pci_hotplug) {
> > > -        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, parent,
> > > +        acpi_pcihp_init(OBJECT(s), &s->acpi_pci_hotplug, bus, container,
> > >                           s->use_acpi_hotplug_bridge, ACPI_PCIHP_ADDR_PIIX4);
> > >       }
> > > @@ -571,11 +571,11 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
> > >       object_property_add_bool(OBJECT(s), "cpu-hotplug-legacy",
> > >                                piix4_get_cpu_hotplug_legacy,
> > >                                piix4_set_cpu_hotplug_legacy);
> > > -    legacy_acpi_cpu_hotplug_init(parent, OBJECT(s), &s->gpe,
> > > +    legacy_acpi_cpu_hotplug_init(container, OBJECT(s), &s->gpe,
> > >                                    PIIX4_CPU_HOTPLUG_IO_BASE);
> > >       if (s->acpi_memory_hotplug.is_enabled) {
> > > -        acpi_memory_hotplug_init(parent, OBJECT(s), &s->acpi_memory_hotplug,
> > > +        acpi_memory_hotplug_init(container, OBJECT(s), &s->acpi_memory_hotplug,
> > >                                    ACPI_MEMORY_HOTPLUG_BASE);
> > >       }
> > >   }
> > > diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h
> > > index 99c11b7151..5ff24ec417 100644
> > > --- a/include/hw/acpi/cpu_hotplug.h
> > > +++ b/include/hw/acpi/cpu_hotplug.h
> > > @@ -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 *parent, Object *owner,
> > > +void legacy_acpi_cpu_hotplug_init(MemoryRegion *container, Object *owner,
> > >                                     AcpiCpuHotplug *gpe, uint16_t base);
> > >   void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
> > > -- 
> > > 2.38.1
> >