Re: [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 28/2/23 22:48, Michael S. Tsirkin wrote:
> On Fri, Feb 03, 2023 at 05:30:19PM +0100, Philippe Mathieu-Daudé wrote:
>> Rename 'g' and 'gpe_cpu' variables as 'gpe' to simplify.
>> No logical change.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   hw/acpi/acpi-cpu-hotplug-stub.c |  6 ++---
>>   hw/acpi/cpu_hotplug.c           | 40 ++++++++++++++++-----------------
>>   hw/acpi/ich9.c                  |  8 +++----
>>   hw/acpi/piix4.c                 |  6 ++---
>>   include/hw/acpi/cpu_hotplug.h   |  8 +++----
>>   include/hw/acpi/ich9.h          |  2 +-
>>   include/hw/acpi/piix4.h         |  2 +-
>>   7 files changed, 36 insertions(+), 36 deletions(-)
>>
>> diff --git a/hw/acpi/acpi-cpu-hotplug-stub.c b/hw/acpi/acpi-cpu-hotplug-stub.c
>> index 3fc4b14c26..b590ad57e1 100644
>> --- a/hw/acpi/acpi-cpu-hotplug-stub.c
>> +++ b/hw/acpi/acpi-cpu-hotplug-stub.c
>> @@ -6,7 +6,7 @@
>>   /* Following stubs are all related to ACPI cpu hotplug */
>>   const VMStateDescription vmstate_cpu_hotplug;
>>   
>> -void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>> +void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>>                                   CPUHotplugState *cpuhp_state,
>>                                   uint16_t io_port)
>>   {
>> @@ -14,7 +14,7 @@ void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>>   }
>>   
>>   void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>> -                                  AcpiCpuHotplug *gpe_cpu, uint16_t base)
>> +                                  AcpiCpuHotplug *gpe, uint16_t base)
>>   {
>>       return;
>>   }
>> @@ -31,7 +31,7 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>>   }
>>   
>>   void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>> -                             AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
>> +                             AcpiCpuHotplug *gpe, DeviceState *dev, Error **errp)
>>   {
>>       return;
>>   }
> 
> 
> just a stub here it does not matter at all.

I kept it consistent with the prototype renamed in the header.

Safe to assume you don't mind if I rename s/hotplug_dev/cow/ 
s/errp/success/ s/gpe/cpu/ s/dev/cat/ here. /s

>> diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c
>> index ff14c3f410..3cfa4f45dc 100644
>> --- a/hw/acpi/cpu_hotplug.c
>> +++ b/hw/acpi/cpu_hotplug.c
>> @@ -26,8 +26,8 @@
>>   
>>   static uint64_t cpu_status_read(void *opaque, hwaddr addr, unsigned int size)
>>   {
>> -    AcpiCpuHotplug *cpus = opaque;
>> -    uint64_t val = cpus->sts[addr];
>> +    AcpiCpuHotplug *gpe = opaque;
>> +    uint64_t val = gpe->sts[addr];
>>   
>>       return val;
>>   }
>> @@ -40,8 +40,8 @@ static void cpu_status_write(void *opaque, hwaddr addr, uint64_t data,
>>          mode by writing 0 at the beginning of legacy CPU bitmap
>>        */
>>       if (addr == 0 && data == 0) {
>> -        AcpiCpuHotplug *cpus = opaque;
>> -        object_property_set_bool(cpus->device, "cpu-hotplug-legacy", false,
>> +        AcpiCpuHotplug *gpe = opaque;
>> +        object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
>>                                    &error_abort);
>>       }
>>   }
>> @@ -59,51 +59,51 @@ static const MemoryRegionOps AcpiCpuHotplug_ops = {
>>       },
>>   };
>>   
>> -static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu)
>> +static void acpi_set_cpu_present_bit(AcpiCpuHotplug *gpe, CPUState *cpu)
>>   {
>>       CPUClass *k = CPU_GET_CLASS(cpu);
>>       int64_t cpu_id;
>>   
>>       cpu_id = k->get_arch_id(cpu);
>>       if ((cpu_id / 8) >= ACPI_GPE_PROC_LEN) {
>> -        object_property_set_bool(g->device, "cpu-hotplug-legacy", false,
>> +        object_property_set_bool(gpe->device, "cpu-hotplug-legacy", false,
>>                                    &error_abort);
>>           return;
>>       }
>>   
>> -    g->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
>> +    gpe->sts[cpu_id / 8] |= (1 << (cpu_id % 8));
>>   }
>>   
>> -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>> -                             AcpiCpuHotplug *g, DeviceState *dev, Error **errp)
>> +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, AcpiCpuHotplug *gpe,
>> +                             DeviceState *dev, Error **errp)
>>   {
>> -    acpi_set_cpu_present_bit(g, CPU(dev));
>> +    acpi_set_cpu_present_bit(gpe, CPU(dev));
>>       acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS);
>>   }
>>   
>>   void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner,
>> -                                  AcpiCpuHotplug *gpe_cpu, uint16_t base)
>> +                                  AcpiCpuHotplug *gpe, uint16_t base)
>>   {
>>       CPUState *cpu;
>>   
>> -    memory_region_init_io(&gpe_cpu->io, owner, &AcpiCpuHotplug_ops,
>> -                          gpe_cpu, "acpi-cpu-hotplug", ACPI_GPE_PROC_LEN);
>> -    memory_region_add_subregion(parent, base, &gpe_cpu->io);
>> -    gpe_cpu->device = owner;
>> +    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);
>> +    gpe->device = owner;
>>   
>>       CPU_FOREACH(cpu) {
>> -        acpi_set_cpu_present_bit(gpe_cpu, cpu);
>> +        acpi_set_cpu_present_bit(gpe, cpu);
>>       }
>>   }
>>   
>> -void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>> +void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>>                                   CPUHotplugState *cpuhp_state,
>>                                   uint16_t io_port)
>>   {
>> -    MemoryRegion *parent = pci_address_space_io(PCI_DEVICE(gpe_cpu->device));
>> +    MemoryRegion *parent = pci_address_space_io(PCI_DEVICE(gpe->device));
>>   
>> -    memory_region_del_subregion(parent, &gpe_cpu->io);
>> -    cpu_hotplug_hw_init(parent, gpe_cpu->device, cpuhp_state, io_port);
>> +    memory_region_del_subregion(parent, &gpe->io);
>> +    cpu_hotplug_hw_init(parent, gpe->device, 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 a93c470e9d..4f8385b894 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_cpu.device);
>> +    Object *obj = OBJECT(s->gpe.device);
>>       object_property_set_bool(obj, "cpu-hotplug-legacy", false, &error_abort);
>>       return 0;
>>   }
>> @@ -338,7 +338,7 @@ 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_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>> +        OBJECT(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),
>> @@ -385,7 +385,7 @@ static void ich9_pm_set_cpu_hotplug_legacy(Object *obj, bool value,
>>   
>>       assert(!value);
>>       if (s->pm.cpu_hotplug_legacy && value == false) {
>> -        acpi_switch_to_modern_cphp(&s->pm.gpe_cpu, &s->pm.cpuhp_state,
>> +        acpi_switch_to_modern_cphp(&s->pm.gpe, &s->pm.cpuhp_state,
>>                                      ICH9_CPU_HOTPLUG_IO_BASE);
>>       }
>>       s->pm.cpu_hotplug_legacy = value;
>> @@ -514,7 +514,7 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>>           }
>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>           if (lpc->pm.cpu_hotplug_legacy) {
>> -            legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp);
>> +            legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe, dev, errp);
>>           } else {
>>               acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp);
>>           }
>> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
>> index 724294b378..c702d83f27 100644
>> --- a/hw/acpi/piix4.c
>> +++ b/hw/acpi/piix4.c
>> @@ -353,7 +353,7 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev,
>>           acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp);
>>       } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) {
>>           if (s->cpu_hotplug_legacy) {
>> -            legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp);
>> +            legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe, dev, errp);
>>           } else {
>>               acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp);
>>           }
>> @@ -549,7 +549,7 @@ static void piix4_set_cpu_hotplug_legacy(Object *obj, bool value, Error **errp)
>>   
>>       assert(!value);
>>       if (s->cpu_hotplug_legacy && value == false) {
>> -        acpi_switch_to_modern_cphp(&s->gpe_cpu, &s->cpuhp_state,
>> +        acpi_switch_to_modern_cphp(&s->gpe, &s->cpuhp_state,
>>                                      PIIX4_CPU_HOTPLUG_IO_BASE);
>>       }
>>       s->cpu_hotplug_legacy = value;
>> @@ -571,7 +571,7 @@ 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_cpu,
>> +    legacy_acpi_cpu_hotplug_init(parent, OBJECT(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 3b932abbbb..99c11b7151 100644
>> --- a/include/hw/acpi/cpu_hotplug.h
>> +++ b/include/hw/acpi/cpu_hotplug.h
>> @@ -25,13 +25,13 @@ typedef struct AcpiCpuHotplug {
>>       uint8_t sts[ACPI_GPE_PROC_LEN];
>>   } AcpiCpuHotplug;
>>   
>> -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev,
>> -                             AcpiCpuHotplug *g, DeviceState *dev, Error **errp);
>> +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,
>> -                                  AcpiCpuHotplug *gpe_cpu, uint16_t base);
>> +                                  AcpiCpuHotplug *gpe, uint16_t base);
>>   
>> -void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe_cpu,
>> +void acpi_switch_to_modern_cphp(AcpiCpuHotplug *gpe,
>>                                   CPUHotplugState *cpuhp_state,
>>                                   uint16_t io_port);
>>   
> 
> 
> I don't see how parameter names matter much.

'gpe' is slightly more meaningful than 'g'. Why not name 
s/hotplug_dev/h/, s/errp/e/, or actually, why do we name variables
in prototype? \s

>> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
>> index d41866a229..3ec706c0d7 100644
>> --- a/include/hw/acpi/ich9.h
>> +++ b/include/hw/acpi/ich9.h
>> @@ -53,7 +53,7 @@ typedef struct ICH9LPCPMRegs {
>>       Notifier powerdown_notifier;
>>   
>>       bool cpu_hotplug_legacy;
>> -    AcpiCpuHotplug gpe_cpu;
>> +    AcpiCpuHotplug gpe;
>>       CPUHotplugState cpuhp_state;
>>   
>>       bool keep_pci_slot_hpc;
>> diff --git a/include/hw/acpi/piix4.h b/include/hw/acpi/piix4.h
>> index be1f8ea80e..b88ef92455 100644
>> --- a/include/hw/acpi/piix4.h
>> +++ b/include/hw/acpi/piix4.h
>> @@ -66,7 +66,7 @@ struct PIIX4PMState {
>>       uint8_t s4_val;
>>   
>>       bool cpu_hotplug_legacy;
>> -    AcpiCpuHotplug gpe_cpu;
>> +    AcpiCpuHotplug gpe;
>>       CPUHotplugState cpuhp_state;
>>   
>>       MemHotplugState acpi_memory_hotplug;
> 
> 
> gpe to me reads like ACPIGPE. I think using it for AcpiCpuHotplug is
> confusing.

SIGPIPE?

Anyway I'm glad these prototypes are maintained, let's keep them as
their maintainer like to see them, even if it is opaque to neophytes.

Dropping this series and stopping trying to improve / clarify the API
on your subsystem, I'm just wasting both your / mine time, and that
is certainly not what I aimed at first.