* [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
@ 2023-02-03 16:30 Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe Philippe Mathieu-Daudé
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-03 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Igor Mammedov, Philippe Mathieu-Daudé
To ease code review, rename ACPI CPU hotplug variables
to more meaningful names.
Since hotplug parent can't be any QOM object, and must be
a QDev, convert AcpiCpuHotplug::device from Object* to
DeviceState*.
Philippe Mathieu-Daudé (3):
hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
hw/acpi/acpi-cpu-hotplug-stub.c | 8 +++---
hw/acpi/cpu_hotplug.c | 46 ++++++++++++++++-----------------
hw/acpi/ich9.c | 9 ++++---
hw/acpi/piix4.c | 14 +++++-----
include/hw/acpi/cpu_hotplug.h | 12 ++++-----
include/hw/acpi/ich9.h | 2 +-
include/hw/acpi/piix4.h | 2 +-
7 files changed, 47 insertions(+), 46 deletions(-)
--
2.38.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
2023-02-03 16:30 [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
@ 2023-02-03 16:30 ` Philippe Mathieu-Daudé
2023-02-28 21:48 ` Michael S. Tsirkin
2023-02-03 16:30 ` [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container' Philippe Mathieu-Daudé
` (2 subsequent siblings)
3 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-03 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
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;
}
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);
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;
--
2.38.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
2023-02-03 16:30 [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe Philippe Mathieu-Daudé
@ 2023-02-03 16:30 ` Philippe Mathieu-Daudé
2023-02-28 21:43 ` Michael S. Tsirkin
2023-02-03 16:30 ` [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-22 21:34 ` [PATCH 0/3] " Philippe Mathieu-Daudé
3 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-03 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
No logical change, rename for clarity.
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-03 16:30 [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container' Philippe Mathieu-Daudé
@ 2023-02-03 16:30 ` Philippe Mathieu-Daudé
2023-02-28 15:40 ` Igor Mammedov
2023-02-28 21:41 ` Michael S. Tsirkin
2023-02-22 21:34 ` [PATCH 0/3] " Philippe Mathieu-Daudé
3 siblings, 2 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-03 16:30 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Igor Mammedov, Philippe Mathieu-Daudé,
Aurelien Jarno
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-03 16:30 [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
` (2 preceding siblings ...)
2023-02-03 16:30 ` [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
@ 2023-02-22 21:34 ` Philippe Mathieu-Daudé
2023-02-28 13:36 ` Philippe Mathieu-Daudé
3 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-22 21:34 UTC (permalink / raw)
To: qemu-devel
Cc: Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Igor Mammedov, Alex Bennée
On 3/2/23 17:30, Philippe Mathieu-Daudé wrote:
> To ease code review, rename ACPI CPU hotplug variables
> to more meaningful names.
>
> Since hotplug parent can't be any QOM object, and must be
> a QDev, convert AcpiCpuHotplug::device from Object* to
> DeviceState*.
>
> Philippe Mathieu-Daudé (3):
> hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
> hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
> hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
ping
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-22 21:34 ` [PATCH 0/3] " Philippe Mathieu-Daudé
@ 2023-02-28 13:36 ` Philippe Mathieu-Daudé
2023-02-28 15:47 ` Igor Mammedov
0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 13:36 UTC (permalink / raw)
To: qemu-devel, Ani Sinha
Cc: Michael S. Tsirkin, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Alex Bennée, Bernhard Beschow
ping^2
On 22/2/23 22:34, Philippe Mathieu-Daudé wrote:
> On 3/2/23 17:30, Philippe Mathieu-Daudé wrote:
>> To ease code review, rename ACPI CPU hotplug variables
>> to more meaningful names.
>>
>> Since hotplug parent can't be any QOM object, and must be
>> a QDev, convert AcpiCpuHotplug::device from Object* to
>> DeviceState*.
>>
>> Philippe Mathieu-Daudé (3):
>> hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
>> hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
>> hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
>
> ping
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-03 16:30 ` [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
@ 2023-02-28 15:40 ` Igor Mammedov
2023-02-28 21:41 ` Michael S. Tsirkin
1 sibling, 0 replies; 18+ messages in thread
From: Igor Mammedov @ 2023-02-28 15:40 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Aurelien Jarno
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,
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-28 13:36 ` Philippe Mathieu-Daudé
@ 2023-02-28 15:47 ` Igor Mammedov
2023-02-28 21:50 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Igor Mammedov @ 2023-02-28 15:47 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Michael S. Tsirkin, Marcel Apfelbaum,
Markus Armbruster, Alex Bennée, Bernhard Beschow
On Tue, 28 Feb 2023 14:36:43 +0100
Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> ping^2
please use checkpatch before pasting series.
Object -> DeviceState is a nice cleanup,
the rest is just unnecessary churn in my opinion and a matter of taste,
but I fine with it if it makes code easier to read
for someone else.
>
> On 22/2/23 22:34, Philippe Mathieu-Daudé wrote:
> > On 3/2/23 17:30, Philippe Mathieu-Daudé wrote:
> >> To ease code review, rename ACPI CPU hotplug variables
> >> to more meaningful names.
> >>
> >> Since hotplug parent can't be any QOM object, and must be
> >> a QDev, convert AcpiCpuHotplug::device from Object* to
> >> DeviceState*.
> >>
> >> Philippe Mathieu-Daudé (3):
> >> hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
> >> hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
> >> hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
> >
> > ping
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-03 16:30 ` [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-28 15:40 ` Igor Mammedov
@ 2023-02-28 21:41 ` Michael S. Tsirkin
2023-02-28 22:05 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 21:41 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
2023-02-03 16:30 ` [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container' Philippe Mathieu-Daudé
@ 2023-02-28 21:43 ` Michael S. Tsirkin
2023-02-28 22:16 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 21:43 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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.
> ---
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
2023-02-03 16:30 ` [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe Philippe Mathieu-Daudé
@ 2023-02-28 21:48 ` Michael S. Tsirkin
2023-02-28 22:26 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 21:48 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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.
> 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.
> 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.
> --
> 2.38.1
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-28 15:47 ` Igor Mammedov
@ 2023-02-28 21:50 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 21:50 UTC (permalink / raw)
To: Igor Mammedov
Cc: Philippe Mathieu-Daudé,
qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Alex Bennée, Bernhard Beschow
On Tue, Feb 28, 2023 at 04:47:31PM +0100, Igor Mammedov wrote:
> On Tue, 28 Feb 2023 14:36:43 +0100
> Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> > ping^2
>
> please use checkpatch before pasting series.
>
> Object -> DeviceState is a nice cleanup,
I don't like that one either because everyone wants Object so
we just cast it back all the time. Let's carry what we use
if all you call sites cast you know you should keep the type you
cast to.
> the rest is just unnecessary churn in my opinion and a matter of taste,
> but I fine with it if it makes code easier to read
> for someone else.
>
>
> >
> > On 22/2/23 22:34, Philippe Mathieu-Daudé wrote:
> > > On 3/2/23 17:30, Philippe Mathieu-Daudé wrote:
> > >> To ease code review, rename ACPI CPU hotplug variables
> > >> to more meaningful names.
> > >>
> > >> Since hotplug parent can't be any QOM object, and must be
> > >> a QDev, convert AcpiCpuHotplug::device from Object* to
> > >> DeviceState*.
> > >>
> > >> Philippe Mathieu-Daudé (3):
> > >> hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
> > >> hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
> > >> hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
> > >
> > > ping
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-28 21:41 ` Michael S. Tsirkin
@ 2023-02-28 22:05 ` Philippe Mathieu-Daudé
2023-02-28 22:09 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 22:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-28 22:05 ` Philippe Mathieu-Daudé
@ 2023-02-28 22:09 ` Michael S. Tsirkin
2023-02-28 22:13 ` Philippe Mathieu-Daudé
0 siblings, 1 reply; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 22:09 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent'
2023-02-28 22:09 ` Michael S. Tsirkin
@ 2023-02-28 22:13 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 22:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
2023-02-28 21:43 ` Michael S. Tsirkin
@ 2023-02-28 22:16 ` Philippe Mathieu-Daudé
2023-02-28 22:21 ` Michael S. Tsirkin
0 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 22:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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().
I still believe this is a worthwhile change as API style cleanup,
but I agree the commit description lacks this explanation.
>> ---
>> 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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container'
2023-02-28 22:16 ` Philippe Mathieu-Daudé
@ 2023-02-28 22:21 ` Michael S. Tsirkin
0 siblings, 0 replies; 18+ messages in thread
From: Michael S. Tsirkin @ 2023-02-28 22:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe
2023-02-28 21:48 ` Michael S. Tsirkin
@ 2023-02-28 22:26 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2023-02-28 22:26 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Ani Sinha, Marcel Apfelbaum, Markus Armbruster,
Igor Mammedov, Aurelien Jarno
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-02-28 22:26 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-03 16:30 [PATCH 0/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 1/3] hw/acpi/cpu_hotplug: Rename gpe_cpu -> gpe Philippe Mathieu-Daudé
2023-02-28 21:48 ` Michael S. Tsirkin
2023-02-28 22:26 ` Philippe Mathieu-Daudé
2023-02-03 16:30 ` [PATCH 2/3] hw/acpi/cpu_hotplug: Rename 'parent' MemoryRegion as 'container' Philippe Mathieu-Daudé
2023-02-28 21:43 ` Michael S. Tsirkin
2023-02-28 22:16 ` Philippe Mathieu-Daudé
2023-02-28 22:21 ` Michael S. Tsirkin
2023-02-03 16:30 ` [PATCH 3/3] hw/acpi/cpu_hotplug: Convert 'Object *device' -> 'DeviceState *parent' Philippe Mathieu-Daudé
2023-02-28 15:40 ` Igor Mammedov
2023-02-28 21:41 ` Michael S. Tsirkin
2023-02-28 22:05 ` Philippe Mathieu-Daudé
2023-02-28 22:09 ` Michael S. Tsirkin
2023-02-28 22:13 ` Philippe Mathieu-Daudé
2023-02-22 21:34 ` [PATCH 0/3] " Philippe Mathieu-Daudé
2023-02-28 13:36 ` Philippe Mathieu-Daudé
2023-02-28 15:47 ` Igor Mammedov
2023-02-28 21:50 ` Michael S. Tsirkin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.