| Message ID | 20180723193145.24705-3-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI type safety cleanup | expand |
On Mon, 23 Jul 2018 16:31:45 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > The ACPI hotplug callbacks get a HotplugHandler object as > argument. This has two problems: > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > function prototype doesn't indicate that. It's possible to > pass an object that would make the function crash. > 2) The function does not require the object to implement > TYPE_HOTPLUG_HANDLER at all, but the function prototype > imposes that for no reason. > > Change the argument type to AcpiDeviceIf instead of > HotplugHandler. What is the motivation for this patch, do you actually get crashes? > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > include/hw/acpi/cpu.h | 5 ++--- > include/hw/acpi/cpu_hotplug.h | 2 +- > include/hw/acpi/memory_hotplug.h | 4 ++-- > include/hw/acpi/pcihp.h | 4 ++-- > include/hw/mem/nvdimm.h | 3 ++- > hw/acpi/cpu.c | 8 ++++---- > hw/acpi/cpu_hotplug.c | 4 ++-- > hw/acpi/ich9.c | 14 ++++++++------ > hw/acpi/memory_hotplug.c | 8 ++++---- > hw/acpi/nvdimm.c | 4 ++-- > hw/acpi/pcihp.c | 8 ++++---- > hw/acpi/piix4.c | 18 ++++++++++-------- > 12 files changed, 43 insertions(+), 39 deletions(-) > > diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h > index 89ce172941..3ae6504c24 100644 > --- a/include/hw/acpi/cpu.h > +++ b/include/hw/acpi/cpu.h > @@ -15,7 +15,6 @@ > #include "hw/qdev-core.h" > #include "hw/acpi/acpi.h" > #include "hw/acpi/aml-build.h" > -#include "hw/hotplug.h" > > typedef struct AcpiCpuStatus { > struct CPUState *cpu; > @@ -34,10 +33,10 @@ typedef struct CPUHotplugState { > AcpiCpuStatus *devs; > } CPUHotplugState; > > -void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > +void acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, > CPUHotplugState *cpu_st, DeviceState *dev, Error **errp); > > -void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > +void acpi_cpu_unplug_request_cb(AcpiDeviceIf *acpi_dev, > CPUHotplugState *cpu_st, > DeviceState *dev, Error **errp); > > diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h > index 3b932abbbb..8e663b0606 100644 > --- a/include/hw/acpi/cpu_hotplug.h > +++ b/include/hw/acpi/cpu_hotplug.h > @@ -25,7 +25,7 @@ typedef struct AcpiCpuHotplug { > uint8_t sts[ACPI_GPE_PROC_LEN]; > } AcpiCpuHotplug; > > -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > +void legacy_acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, > AcpiCpuHotplug *g, DeviceState *dev, Error **errp); > > void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h > index 77c65765d6..ebd97093dd 100644 > --- a/include/hw/acpi/memory_hotplug.h > +++ b/include/hw/acpi/memory_hotplug.h > @@ -31,9 +31,9 @@ typedef struct MemHotplugState { > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, > MemHotplugState *state, uint16_t io_base); > > -void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > +void acpi_memory_plug_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, > DeviceState *dev, Error **errp); > -void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > +void acpi_memory_unplug_request_cb(AcpiDeviceIf *acpi_dev, > MemHotplugState *mem_st, > DeviceState *dev, Error **errp); > void acpi_memory_unplug_cb(MemHotplugState *mem_st, > diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h > index 8a65f99fc8..bba37b81dc 100644 > --- a/include/hw/acpi/pcihp.h > +++ b/include/hw/acpi/pcihp.h > @@ -56,9 +56,9 @@ typedef struct AcpiPciHpState { > void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root, > MemoryRegion *address_space_io, bool bridges_enabled); > > -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > +void acpi_pcihp_device_plug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp); > -void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > +void acpi_pcihp_device_unplug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp); > > /* Called on reset */ > diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h > index c5c9b3c7f8..0d80497efe 100644 > --- a/include/hw/mem/nvdimm.h > +++ b/include/hw/mem/nvdimm.h > @@ -25,6 +25,7 @@ > > #include "hw/mem/pc-dimm.h" > #include "hw/acpi/bios-linker-loader.h" > +#include "hw/acpi/acpi_dev_interface.h" > > #define NVDIMM_DEBUG 0 > #define nvdimm_debug(fmt, ...) \ > @@ -149,5 +150,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, > BIOSLinker *linker, AcpiNVDIMMState *state, > uint32_t ram_slots); > void nvdimm_plug(AcpiNVDIMMState *state); > -void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev); > +void nvdimm_acpi_plug_cb(AcpiDeviceIf *acpi_dev, DeviceState *dev); > #endif > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 2eb9b5a032..71aec1d655 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -220,7 +220,7 @@ static AcpiCpuStatus *get_cpu_status(CPUHotplugState *cpu_st, DeviceState *dev) > return NULL; > } > > -void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > +void acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, > CPUHotplugState *cpu_st, DeviceState *dev, Error **errp) > { > AcpiCpuStatus *cdev; > @@ -233,11 +233,11 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > cdev->cpu = CPU(dev); > if (dev->hotplugged) { > cdev->is_inserting = true; > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); > } > } > > -void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > +void acpi_cpu_unplug_request_cb(AcpiDeviceIf *acpi_dev, > CPUHotplugState *cpu_st, > DeviceState *dev, Error **errp) > { > @@ -249,7 +249,7 @@ void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > } > > cdev->is_removing = true; > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); > } > > void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st, > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c > index 5a1599796b..d9277e27de 100644 > --- a/hw/acpi/cpu_hotplug.c > +++ b/hw/acpi/cpu_hotplug.c > @@ -72,14 +72,14 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu, > g->sts[cpu_id / 8] |= (1 << (cpu_id % 8)); > } > > -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > +void legacy_acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, > AcpiCpuHotplug *g, DeviceState *dev, Error **errp) > { > acpi_set_cpu_present_bit(g, CPU(dev), errp); > if (*errp != NULL) { > return; > } > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); > } > > void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c > index c5d8646abc..c53b8bb508 100644 > --- a/hw/acpi/ich9.c > +++ b/hw/acpi/ich9.c > @@ -487,20 +487,21 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > Error **errp) > { > ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); > > if (lpc->pm.acpi_memory_hotplug.is_enabled && > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { > - nvdimm_acpi_plug_cb(hotplug_dev, dev); > + nvdimm_acpi_plug_cb(acpi_dev, dev); > } else { > - acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug, > + acpi_memory_plug_cb(acpi_dev, &lpc->pm.acpi_memory_hotplug, > dev, errp); > } > } 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(acpi_dev, &lpc->pm.gpe_cpu, dev, errp); > } else { > - acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp); > + acpi_cpu_plug_cb(acpi_dev, &lpc->pm.cpuhp_state, dev, errp); > } > } else { > error_setg(errp, "acpi: device plug request for not supported device" > @@ -512,15 +513,16 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); > > if (lpc->pm.acpi_memory_hotplug.is_enabled && > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - acpi_memory_unplug_request_cb(hotplug_dev, > + acpi_memory_unplug_request_cb(acpi_dev, > &lpc->pm.acpi_memory_hotplug, dev, > errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) && > !lpc->pm.cpu_hotplug_legacy) { > - acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state, > + acpi_cpu_unplug_request_cb(acpi_dev, &lpc->pm.cpuhp_state, > dev, errp); > } else { > error_setg(errp, "acpi: device unplug request for not supported device" > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index 24b2aa0301..c0a4c39432 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -261,7 +261,7 @@ acpi_memory_slot_status(MemHotplugState *mem_st, > return &mem_st->devs[slot]; > } > > -void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > +void acpi_memory_plug_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, > DeviceState *dev, Error **errp) > { > MemStatus *mdev; > @@ -280,11 +280,11 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, > mdev->is_enabled = true; > if (dev->hotplugged) { > mdev->is_inserting = true; > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_MEMORY_HOTPLUG_STATUS); > } > } > > -void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > +void acpi_memory_unplug_request_cb(AcpiDeviceIf *acpi_dev, > MemHotplugState *mem_st, > DeviceState *dev, Error **errp) > { > @@ -296,7 +296,7 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > } > > mdev->is_removing = true; > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_MEMORY_HOTPLUG_STATUS); > } > > void acpi_memory_unplug_cb(MemHotplugState *mem_st, > diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c > index bdc373696d..b4708dc746 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -918,10 +918,10 @@ static const MemoryRegionOps nvdimm_dsm_ops = { > }, > }; > > -void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev) > +void nvdimm_acpi_plug_cb(AcpiDeviceIf *acpi_dev, DeviceState *dev) > { > if (dev->hotplugged) { > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_NVDIMM_HOTPLUG_STATUS); > } > } > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index b3bb11dac5..c094d86de3 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -217,7 +217,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s) > acpi_pcihp_update(s); > } > > -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > +void acpi_pcihp_device_plug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp) > { > PCIDevice *pdev = PCI_DEVICE(dev); > @@ -237,10 +237,10 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > } > > s->acpi_pcihp_pci_status[bsel].up |= (1U << slot); > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_PCI_HOTPLUG_STATUS); > } > > -void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > +void acpi_pcihp_device_unplug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, > DeviceState *dev, Error **errp) > { > PCIDevice *pdev = PCI_DEVICE(dev); > @@ -253,7 +253,7 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > } > > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > + acpi_send_event(acpi_dev, ACPI_PCI_HOTPLUG_STATUS); > } > > static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size) > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 6404af5f33..c26c66242f 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -374,22 +374,23 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > PIIX4PMState *s = PIIX4_PM(hotplug_dev); > + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); > > if (s->acpi_memory_hotplug.is_enabled && > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { > - nvdimm_acpi_plug_cb(hotplug_dev, dev); > + nvdimm_acpi_plug_cb(acpi_dev, dev); > } else { > - acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, > + acpi_memory_plug_cb(acpi_dev, &s->acpi_memory_hotplug, > dev, errp); > } > } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > - acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp); > + acpi_pcihp_device_plug_cb(acpi_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(acpi_dev, &s->gpe_cpu, dev, errp); > } else { > - acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp); > + acpi_cpu_plug_cb(acpi_dev, &s->cpuhp_state, dev, errp); > } > } else { > error_setg(errp, "acpi: device plug request for not supported device" > @@ -401,17 +402,18 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev, > DeviceState *dev, Error **errp) > { > PIIX4PMState *s = PIIX4_PM(hotplug_dev); > + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); > > if (s->acpi_memory_hotplug.is_enabled && > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > - acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug, > + acpi_memory_unplug_request_cb(acpi_dev, &s->acpi_memory_hotplug, > dev, errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > - acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, > + acpi_pcihp_device_unplug_cb(acpi_dev, &s->acpi_pci_hotplug, dev, > errp); > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) && > !s->cpu_hotplug_legacy) { > - acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp); > + acpi_cpu_unplug_request_cb(acpi_dev, &s->cpuhp_state, dev, errp); > } else { > error_setg(errp, "acpi: device unplug request for not supported device" > " type: %s", object_get_typename(OBJECT(dev)));
On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > On Mon, 23 Jul 2018 16:31:45 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > The ACPI hotplug callbacks get a HotplugHandler object as > > argument. This has two problems: > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > function prototype doesn't indicate that. It's possible to > > pass an object that would make the function crash. > > 2) The function does not require the object to implement > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > imposes that for no reason. > > > > Change the argument type to AcpiDeviceIf instead of > > HotplugHandler. > What is the motivation for this patch, > do you actually get crashes? I didn't get crashes, but the idea for the change came when Michael asked me how to get the HotplugHandler object. I was going to suggest current_machine (which is also a hotplug handler), when I noticed he actually needed an AcpiDeviceIf object. The main motivation, however, is to simply make the function prototypes make sense. Is there a single reason to make the ACPI functions get a HotplugHandler argument instead of AcpiDeviceIf?
On Tue, 24 Jul 2018 09:39:16 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > > On Mon, 23 Jul 2018 16:31:45 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > The ACPI hotplug callbacks get a HotplugHandler object as > > > argument. This has two problems: > > > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > > function prototype doesn't indicate that. It's possible to > > > pass an object that would make the function crash. > > > 2) The function does not require the object to implement > > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > > imposes that for no reason. > > > > > > Change the argument type to AcpiDeviceIf instead of > > > HotplugHandler. > > What is the motivation for this patch, > > do you actually get crashes? > > I didn't get crashes, but the idea for the change came when > Michael asked me how to get the HotplugHandler object. I was > going to suggest current_machine (which is also a hotplug > handler), when I noticed he actually needed an AcpiDeviceIf > object. > > The main motivation, however, is to simply make the function > prototypes make sense. Is there a single reason to make the ACPI > functions get a HotplugHandler argument instead of AcpiDeviceIf? I'd rather to keep current *_cb() functions as is, as we might actually need access to hotplug handler there and in that case keeping more generic hotplug handler would be better and it also sort of documents better what is being passed in. Also we won't have to rewrite it back from AcpiDeviceIf to HotplugHandler again if it's needed.
On Tue, Jul 24, 2018 at 05:28:44PM +0200, Igor Mammedov wrote: > On Tue, 24 Jul 2018 09:39:16 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > > > On Mon, 23 Jul 2018 16:31:45 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > The ACPI hotplug callbacks get a HotplugHandler object as > > > > argument. This has two problems: > > > > > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > > > function prototype doesn't indicate that. It's possible to > > > > pass an object that would make the function crash. > > > > 2) The function does not require the object to implement > > > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > > > imposes that for no reason. > > > > > > > > Change the argument type to AcpiDeviceIf instead of > > > > HotplugHandler. > > > What is the motivation for this patch, > > > do you actually get crashes? > > > > I didn't get crashes, but the idea for the change came when > > Michael asked me how to get the HotplugHandler object. I was > > going to suggest current_machine (which is also a hotplug > > handler), when I noticed he actually needed an AcpiDeviceIf > > object. > > > > The main motivation, however, is to simply make the function > > prototypes make sense. Is there a single reason to make the ACPI > > functions get a HotplugHandler argument instead of AcpiDeviceIf? > I'd rather to keep current *_cb() functions as is, > as we might actually need access to hotplug handler there and in > that case keeping more generic hotplug handler would be better > and it also sort of documents better what is being passed in. I think it doesn't document what is being passed in at all. The function looks like it requires only a HotplugHandler object, but it actually requires an AcpiDeviceIf object. > > Also we won't have to rewrite it back from AcpiDeviceIf to > HotplugHandler again if it's needed. > My main problem is: today there's a hard requirement that HotplugHandler and AcpiDeviceIf must be the same object, but it's not documented anywhere. The proposal in this patch is to remove that requirement, and make the callbacks just require an AcpiDeviceIf object. If you plan to keep that requirement, that may be OK, but the requirement needs to be documented somehow. How do you think the requirement can be enforced and/or documented?
On Tue, Jul 24, 2018 at 05:28:44PM +0200, Igor Mammedov wrote: > On Tue, 24 Jul 2018 09:39:16 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > > > On Mon, 23 Jul 2018 16:31:45 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > The ACPI hotplug callbacks get a HotplugHandler object as > > > > argument. This has two problems: > > > > > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > > > function prototype doesn't indicate that. It's possible to > > > > pass an object that would make the function crash. > > > > 2) The function does not require the object to implement > > > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > > > imposes that for no reason. > > > > > > > > Change the argument type to AcpiDeviceIf instead of > > > > HotplugHandler. > > > What is the motivation for this patch, > > > do you actually get crashes? > > > > I didn't get crashes, but the idea for the change came when > > Michael asked me how to get the HotplugHandler object. I was > > going to suggest current_machine (which is also a hotplug > > handler), when I noticed he actually needed an AcpiDeviceIf > > object. > > > > The main motivation, however, is to simply make the function > > prototypes make sense. Is there a single reason to make the ACPI > > functions get a HotplugHandler argument instead of AcpiDeviceIf? > I'd rather to keep current *_cb() functions as is, > as we might actually need access to hotplug handler there and in > that case keeping more generic hotplug handler would be better > and it also sort of documents better what is being passed in. > > Also we won't have to rewrite it back from AcpiDeviceIf to > HotplugHandler again if it's needed. Personally I think type-safety is important, changing APIs isn't such a big deal. So I'd rather take Eduardo's patch unless the patches that would need to change it back are just round the corner. what's the status there?
On Tue, 24 Jul 2018 21:14:48 +0300 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Tue, Jul 24, 2018 at 05:28:44PM +0200, Igor Mammedov wrote: > > On Tue, 24 Jul 2018 09:39:16 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > > > > On Mon, 23 Jul 2018 16:31:45 -0300 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > The ACPI hotplug callbacks get a HotplugHandler object as > > > > > argument. This has two problems: > > > > > > > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > > > > function prototype doesn't indicate that. It's possible to > > > > > pass an object that would make the function crash. > > > > > 2) The function does not require the object to implement > > > > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > > > > imposes that for no reason. > > > > > > > > > > Change the argument type to AcpiDeviceIf instead of > > > > > HotplugHandler. > > > > What is the motivation for this patch, > > > > do you actually get crashes? > > > > > > I didn't get crashes, but the idea for the change came when > > > Michael asked me how to get the HotplugHandler object. I was > > > going to suggest current_machine (which is also a hotplug > > > handler), when I noticed he actually needed an AcpiDeviceIf > > > object. > > > > > > The main motivation, however, is to simply make the function > > > prototypes make sense. Is there a single reason to make the ACPI > > > functions get a HotplugHandler argument instead of AcpiDeviceIf? > > I'd rather to keep current *_cb() functions as is, > > as we might actually need access to hotplug handler there and in > > that case keeping more generic hotplug handler would be better > > and it also sort of documents better what is being passed in. > > > > Also we won't have to rewrite it back from AcpiDeviceIf to > > HotplugHandler again if it's needed. > > Personally I think type-safety is important, changing APIs > isn't such a big deal. So I'd rather take Eduardo's patch unless > the patches that would need to change it back are just > round the corner. what's the status there? I don't have any patches for this part nor any plan to work on it. Ok, let's go ahead with Eduardo's patches. We can always change it back if need arises.
On Wed, Jul 25, 2018 at 08:57:20AM +0200, Igor Mammedov wrote: > On Tue, 24 Jul 2018 21:14:48 +0300 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > On Tue, Jul 24, 2018 at 05:28:44PM +0200, Igor Mammedov wrote: > > > On Tue, 24 Jul 2018 09:39:16 -0300 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Tue, Jul 24, 2018 at 02:29:49PM +0200, Igor Mammedov wrote: > > > > > On Mon, 23 Jul 2018 16:31:45 -0300 > > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > > > The ACPI hotplug callbacks get a HotplugHandler object as > > > > > > argument. This has two problems: > > > > > > > > > > > > 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the > > > > > > function prototype doesn't indicate that. It's possible to > > > > > > pass an object that would make the function crash. > > > > > > 2) The function does not require the object to implement > > > > > > TYPE_HOTPLUG_HANDLER at all, but the function prototype > > > > > > imposes that for no reason. > > > > > > > > > > > > Change the argument type to AcpiDeviceIf instead of > > > > > > HotplugHandler. > > > > > What is the motivation for this patch, > > > > > do you actually get crashes? > > > > > > > > I didn't get crashes, but the idea for the change came when > > > > Michael asked me how to get the HotplugHandler object. I was > > > > going to suggest current_machine (which is also a hotplug > > > > handler), when I noticed he actually needed an AcpiDeviceIf > > > > object. > > > > > > > > The main motivation, however, is to simply make the function > > > > prototypes make sense. Is there a single reason to make the ACPI > > > > functions get a HotplugHandler argument instead of AcpiDeviceIf? > > > I'd rather to keep current *_cb() functions as is, > > > as we might actually need access to hotplug handler there and in > > > that case keeping more generic hotplug handler would be better > > > and it also sort of documents better what is being passed in. > > > > > > Also we won't have to rewrite it back from AcpiDeviceIf to > > > HotplugHandler again if it's needed. > > > > Personally I think type-safety is important, changing APIs > > isn't such a big deal. So I'd rather take Eduardo's patch unless > > the patches that would need to change it back are just > > round the corner. what's the status there? > I don't have any patches for this part nor any plan to work on it. > Ok, let's go ahead with Eduardo's patches. We can always change it > back if need arises. OK - it's for-3.1 anyway, Eduardo pls remember to repost after the release, if that's still the case I'll merge.
diff --git a/include/hw/acpi/cpu.h b/include/hw/acpi/cpu.h index 89ce172941..3ae6504c24 100644 --- a/include/hw/acpi/cpu.h +++ b/include/hw/acpi/cpu.h @@ -15,7 +15,6 @@ #include "hw/qdev-core.h" #include "hw/acpi/acpi.h" #include "hw/acpi/aml-build.h" -#include "hw/hotplug.h" typedef struct AcpiCpuStatus { struct CPUState *cpu; @@ -34,10 +33,10 @@ typedef struct CPUHotplugState { AcpiCpuStatus *devs; } CPUHotplugState; -void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, +void acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, CPUHotplugState *cpu_st, DeviceState *dev, Error **errp); -void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, +void acpi_cpu_unplug_request_cb(AcpiDeviceIf *acpi_dev, CPUHotplugState *cpu_st, DeviceState *dev, Error **errp); diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h index 3b932abbbb..8e663b0606 100644 --- a/include/hw/acpi/cpu_hotplug.h +++ b/include/hw/acpi/cpu_hotplug.h @@ -25,7 +25,7 @@ typedef struct AcpiCpuHotplug { uint8_t sts[ACPI_GPE_PROC_LEN]; } AcpiCpuHotplug; -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, +void legacy_acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, AcpiCpuHotplug *g, DeviceState *dev, Error **errp); void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, diff --git a/include/hw/acpi/memory_hotplug.h b/include/hw/acpi/memory_hotplug.h index 77c65765d6..ebd97093dd 100644 --- a/include/hw/acpi/memory_hotplug.h +++ b/include/hw/acpi/memory_hotplug.h @@ -31,9 +31,9 @@ typedef struct MemHotplugState { void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, MemHotplugState *state, uint16_t io_base); -void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, +void acpi_memory_plug_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, DeviceState *dev, Error **errp); -void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, +void acpi_memory_unplug_request_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, DeviceState *dev, Error **errp); void acpi_memory_unplug_cb(MemHotplugState *mem_st, diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h index 8a65f99fc8..bba37b81dc 100644 --- a/include/hw/acpi/pcihp.h +++ b/include/hw/acpi/pcihp.h @@ -56,9 +56,9 @@ typedef struct AcpiPciHpState { void acpi_pcihp_init(Object *owner, AcpiPciHpState *, PCIBus *root, MemoryRegion *address_space_io, bool bridges_enabled); -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, +void acpi_pcihp_device_plug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp); -void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, +void acpi_pcihp_device_unplug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp); /* Called on reset */ diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h index c5c9b3c7f8..0d80497efe 100644 --- a/include/hw/mem/nvdimm.h +++ b/include/hw/mem/nvdimm.h @@ -25,6 +25,7 @@ #include "hw/mem/pc-dimm.h" #include "hw/acpi/bios-linker-loader.h" +#include "hw/acpi/acpi_dev_interface.h" #define NVDIMM_DEBUG 0 #define nvdimm_debug(fmt, ...) \ @@ -149,5 +150,5 @@ void nvdimm_build_acpi(GArray *table_offsets, GArray *table_data, BIOSLinker *linker, AcpiNVDIMMState *state, uint32_t ram_slots); void nvdimm_plug(AcpiNVDIMMState *state); -void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev); +void nvdimm_acpi_plug_cb(AcpiDeviceIf *acpi_dev, DeviceState *dev); #endif diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 2eb9b5a032..71aec1d655 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -220,7 +220,7 @@ static AcpiCpuStatus *get_cpu_status(CPUHotplugState *cpu_st, DeviceState *dev) return NULL; } -void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, +void acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, CPUHotplugState *cpu_st, DeviceState *dev, Error **errp) { AcpiCpuStatus *cdev; @@ -233,11 +233,11 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, cdev->cpu = CPU(dev); if (dev->hotplugged) { cdev->is_inserting = true; - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); } } -void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, +void acpi_cpu_unplug_request_cb(AcpiDeviceIf *acpi_dev, CPUHotplugState *cpu_st, DeviceState *dev, Error **errp) { @@ -249,7 +249,7 @@ void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, } cdev->is_removing = true; - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); } void acpi_cpu_unplug_cb(CPUHotplugState *cpu_st, diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c index 5a1599796b..d9277e27de 100644 --- a/hw/acpi/cpu_hotplug.c +++ b/hw/acpi/cpu_hotplug.c @@ -72,14 +72,14 @@ static void acpi_set_cpu_present_bit(AcpiCpuHotplug *g, CPUState *cpu, g->sts[cpu_id / 8] |= (1 << (cpu_id % 8)); } -void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, +void legacy_acpi_cpu_plug_cb(AcpiDeviceIf *acpi_dev, AcpiCpuHotplug *g, DeviceState *dev, Error **errp) { acpi_set_cpu_present_bit(g, CPU(dev), errp); if (*errp != NULL) { return; } - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_CPU_HOTPLUG_STATUS); } void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c index c5d8646abc..c53b8bb508 100644 --- a/hw/acpi/ich9.c +++ b/hw/acpi/ich9.c @@ -487,20 +487,21 @@ void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); if (lpc->pm.acpi_memory_hotplug.is_enabled && object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { - nvdimm_acpi_plug_cb(hotplug_dev, dev); + nvdimm_acpi_plug_cb(acpi_dev, dev); } else { - acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug, + acpi_memory_plug_cb(acpi_dev, &lpc->pm.acpi_memory_hotplug, dev, errp); } } 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(acpi_dev, &lpc->pm.gpe_cpu, dev, errp); } else { - acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.cpuhp_state, dev, errp); + acpi_cpu_plug_cb(acpi_dev, &lpc->pm.cpuhp_state, dev, errp); } } else { error_setg(errp, "acpi: device plug request for not supported device" @@ -512,15 +513,16 @@ void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); if (lpc->pm.acpi_memory_hotplug.is_enabled && object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { - acpi_memory_unplug_request_cb(hotplug_dev, + acpi_memory_unplug_request_cb(acpi_dev, &lpc->pm.acpi_memory_hotplug, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) && !lpc->pm.cpu_hotplug_legacy) { - acpi_cpu_unplug_request_cb(hotplug_dev, &lpc->pm.cpuhp_state, + acpi_cpu_unplug_request_cb(acpi_dev, &lpc->pm.cpuhp_state, dev, errp); } else { error_setg(errp, "acpi: device unplug request for not supported device" diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index 24b2aa0301..c0a4c39432 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -261,7 +261,7 @@ acpi_memory_slot_status(MemHotplugState *mem_st, return &mem_st->devs[slot]; } -void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, +void acpi_memory_plug_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, DeviceState *dev, Error **errp) { MemStatus *mdev; @@ -280,11 +280,11 @@ void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState *mem_st, mdev->is_enabled = true; if (dev->hotplugged) { mdev->is_inserting = true; - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_MEMORY_HOTPLUG_STATUS); } } -void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, +void acpi_memory_unplug_request_cb(AcpiDeviceIf *acpi_dev, MemHotplugState *mem_st, DeviceState *dev, Error **errp) { @@ -296,7 +296,7 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, } mdev->is_removing = true; - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_MEMORY_HOTPLUG_STATUS); } void acpi_memory_unplug_cb(MemHotplugState *mem_st, diff --git a/hw/acpi/nvdimm.c b/hw/acpi/nvdimm.c index bdc373696d..b4708dc746 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -918,10 +918,10 @@ static const MemoryRegionOps nvdimm_dsm_ops = { }, }; -void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev) +void nvdimm_acpi_plug_cb(AcpiDeviceIf *acpi_dev, DeviceState *dev) { if (dev->hotplugged) { - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_NVDIMM_HOTPLUG_STATUS); } } diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index b3bb11dac5..c094d86de3 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -217,7 +217,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s) acpi_pcihp_update(s); } -void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, +void acpi_pcihp_device_plug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp) { PCIDevice *pdev = PCI_DEVICE(dev); @@ -237,10 +237,10 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, } s->acpi_pcihp_pci_status[bsel].up |= (1U << slot); - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_PCI_HOTPLUG_STATUS); } -void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, +void acpi_pcihp_device_unplug_cb(AcpiDeviceIf *acpi_dev, AcpiPciHpState *s, DeviceState *dev, Error **errp) { PCIDevice *pdev = PCI_DEVICE(dev); @@ -253,7 +253,7 @@ void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, } s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); - acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); + acpi_send_event(acpi_dev, ACPI_PCI_HOTPLUG_STATUS); } static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size) diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 6404af5f33..c26c66242f 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -374,22 +374,23 @@ static void piix4_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { PIIX4PMState *s = PIIX4_PM(hotplug_dev); + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); if (s->acpi_memory_hotplug.is_enabled && object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { if (object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM)) { - nvdimm_acpi_plug_cb(hotplug_dev, dev); + nvdimm_acpi_plug_cb(acpi_dev, dev); } else { - acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, + acpi_memory_plug_cb(acpi_dev, &s->acpi_memory_hotplug, dev, errp); } } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { - acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, errp); + acpi_pcihp_device_plug_cb(acpi_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(acpi_dev, &s->gpe_cpu, dev, errp); } else { - acpi_cpu_plug_cb(hotplug_dev, &s->cpuhp_state, dev, errp); + acpi_cpu_plug_cb(acpi_dev, &s->cpuhp_state, dev, errp); } } else { error_setg(errp, "acpi: device plug request for not supported device" @@ -401,17 +402,18 @@ static void piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev, DeviceState *dev, Error **errp) { PIIX4PMState *s = PIIX4_PM(hotplug_dev); + AcpiDeviceIf *acpi_dev = ACPI_DEVICE_IF(hotplug_dev); if (s->acpi_memory_hotplug.is_enabled && object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { - acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug, + acpi_memory_unplug_request_cb(acpi_dev, &s->acpi_memory_hotplug, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { - acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, + acpi_pcihp_device_unplug_cb(acpi_dev, &s->acpi_pci_hotplug, dev, errp); } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU) && !s->cpu_hotplug_legacy) { - acpi_cpu_unplug_request_cb(hotplug_dev, &s->cpuhp_state, dev, errp); + acpi_cpu_unplug_request_cb(acpi_dev, &s->cpuhp_state, dev, errp); } else { error_setg(errp, "acpi: device unplug request for not supported device" " type: %s", object_get_typename(OBJECT(dev)));
The ACPI hotplug callbacks get a HotplugHandler object as argument. This has two problems: 1) The functions require a TYPE_ACPI_DEVICE_IF object, but the function prototype doesn't indicate that. It's possible to pass an object that would make the function crash. 2) The function does not require the object to implement TYPE_HOTPLUG_HANDLER at all, but the function prototype imposes that for no reason. Change the argument type to AcpiDeviceIf instead of HotplugHandler. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- include/hw/acpi/cpu.h | 5 ++--- include/hw/acpi/cpu_hotplug.h | 2 +- include/hw/acpi/memory_hotplug.h | 4 ++-- include/hw/acpi/pcihp.h | 4 ++-- include/hw/mem/nvdimm.h | 3 ++- hw/acpi/cpu.c | 8 ++++---- hw/acpi/cpu_hotplug.c | 4 ++-- hw/acpi/ich9.c | 14 ++++++++------ hw/acpi/memory_hotplug.c | 8 ++++---- hw/acpi/nvdimm.c | 4 ++-- hw/acpi/pcihp.c | 8 ++++---- hw/acpi/piix4.c | 18 ++++++++++-------- 12 files changed, 43 insertions(+), 39 deletions(-)