* [Qemu-devel] [PATCH for-3.1 0/2] ACPI type safety cleanup @ 2018-07-23 19:31 Eduardo Habkost 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety Eduardo Habkost 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler Eduardo Habkost 0 siblings, 2 replies; 11+ messages in thread From: Eduardo Habkost @ 2018-07-23 19:31 UTC (permalink / raw) To: qemu-devel Cc: Marcel Apfelbaum, Igor Mammedov, Xiao Guangrong, Ben Warren, Michael S. Tsirkin Small cleanup to make the ACPI callback functions harder to misuse. Eduardo Habkost (2): acpi: Improve acpi_send_event() type safety acpi: Decouple ACPI hotplug callbacks from HotplugHandler include/hw/acpi/acpi_dev_interface.h | 2 +- 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/acpi_interface.c | 5 ++--- 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 ++++++++++-------- hw/acpi/vmgenid.c | 2 +- 15 files changed, 47 insertions(+), 44 deletions(-) -- 2.18.0.rc1.1.g3f1ff2140 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety 2018-07-23 19:31 [Qemu-devel] [PATCH for-3.1 0/2] ACPI type safety cleanup Eduardo Habkost @ 2018-07-23 19:31 ` Eduardo Habkost 2018-07-24 12:27 ` Igor Mammedov 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler Eduardo Habkost 1 sibling, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2018-07-23 19:31 UTC (permalink / raw) To: qemu-devel Cc: Marcel Apfelbaum, Igor Mammedov, Xiao Guangrong, Ben Warren, Michael S. Tsirkin acpi_send_event() requires an object that implements TYPE_ACPI_DEVICE_IF. Indicate that in the function prototype. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- include/hw/acpi/acpi_dev_interface.h | 2 +- hw/acpi/acpi_interface.c | 5 ++--- hw/acpi/cpu.c | 4 ++-- hw/acpi/cpu_hotplug.c | 2 +- hw/acpi/memory_hotplug.c | 4 ++-- hw/acpi/nvdimm.c | 2 +- hw/acpi/pcihp.c | 4 ++-- hw/acpi/vmgenid.c | 2 +- 8 files changed, 12 insertions(+), 13 deletions(-) diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h index dabf4c4fc9..bab71f9d60 100644 --- a/include/hw/acpi/acpi_dev_interface.h +++ b/include/hw/acpi/acpi_dev_interface.h @@ -31,7 +31,7 @@ typedef struct AcpiDeviceIf { Object Parent; } AcpiDeviceIf; -void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event); +void acpi_send_event(AcpiDeviceIf *dev, AcpiEventStatusBits event); /** * AcpiDeviceIfClass: diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c index 6583917b8e..f9b538293e 100644 --- a/hw/acpi/acpi_interface.c +++ b/hw/acpi/acpi_interface.c @@ -2,12 +2,11 @@ #include "hw/acpi/acpi_dev_interface.h" #include "qemu/module.h" -void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event) +void acpi_send_event(AcpiDeviceIf *dev, AcpiEventStatusBits event) { AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(dev); if (adevc->send_event) { - AcpiDeviceIf *adev = ACPI_DEVICE_IF(dev); - adevc->send_event(adev, event); + adevc->send_event(dev, event); } } diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c index 5ae595ecbe..2eb9b5a032 100644 --- a/hw/acpi/cpu.c +++ b/hw/acpi/cpu.c @@ -233,7 +233,7 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, cdev->cpu = CPU(dev); if (dev->hotplugged) { cdev->is_inserting = true; - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); } } @@ -249,7 +249,7 @@ void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, } cdev->is_removing = true; - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_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 5243918125..5a1599796b 100644 --- a/hw/acpi/cpu_hotplug.c +++ b/hw/acpi/cpu_hotplug.c @@ -79,7 +79,7 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, if (*errp != NULL) { return; } - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); } void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c index 0ff1712c4c..24b2aa0301 100644 --- a/hw/acpi/memory_hotplug.c +++ b/hw/acpi/memory_hotplug.c @@ -280,7 +280,7 @@ 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(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); } } @@ -296,7 +296,7 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, } mdev->is_removing = true; - acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_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 27eeb6609f..bdc373696d 100644 --- a/hw/acpi/nvdimm.c +++ b/hw/acpi/nvdimm.c @@ -921,7 +921,7 @@ static const MemoryRegionOps nvdimm_dsm_ops = { void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev) { if (dev->hotplugged) { - acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); } } diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c index 80d42e12ff..b3bb11dac5 100644 --- a/hw/acpi/pcihp.c +++ b/hw/acpi/pcihp.c @@ -237,7 +237,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, } s->acpi_pcihp_pci_status[bsel].up |= (1U << slot); - acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); } void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, @@ -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(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); } static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size) diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index d78b579a20..74caedde79 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -158,7 +158,7 @@ static void vmgenid_update_guest(VmGenIdState *vms) cpu_physical_memory_write(vmgenid_addr, guid_le.data, sizeof(guid_le.data)); /* Send _GPE.E05 event */ - acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS); + acpi_send_event(ACPI_DEVICE_IF(obj), ACPI_VMGENID_CHANGE_STATUS); } } } -- 2.18.0.rc1.1.g3f1ff2140 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety Eduardo Habkost @ 2018-07-24 12:27 ` Igor Mammedov 0 siblings, 0 replies; 11+ messages in thread From: Igor Mammedov @ 2018-07-24 12:27 UTC (permalink / raw) To: Eduardo Habkost Cc: qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren, Michael S. Tsirkin On Mon, 23 Jul 2018 16:31:44 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > acpi_send_event() requires an object that implements > TYPE_ACPI_DEVICE_IF. Indicate that in the function prototype. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> It's not like acpi_send_event would ever need access to more than AcpiDeviceIf, so it's fine to make it more strict. Reviewed-by: Igor Mammedov <imammedo@redhat.com> PS: checkpatch is not happy with this patch > --- > include/hw/acpi/acpi_dev_interface.h | 2 +- > hw/acpi/acpi_interface.c | 5 ++--- > hw/acpi/cpu.c | 4 ++-- > hw/acpi/cpu_hotplug.c | 2 +- > hw/acpi/memory_hotplug.c | 4 ++-- > hw/acpi/nvdimm.c | 2 +- > hw/acpi/pcihp.c | 4 ++-- > hw/acpi/vmgenid.c | 2 +- > 8 files changed, 12 insertions(+), 13 deletions(-) > > diff --git a/include/hw/acpi/acpi_dev_interface.h b/include/hw/acpi/acpi_dev_interface.h > index dabf4c4fc9..bab71f9d60 100644 > --- a/include/hw/acpi/acpi_dev_interface.h > +++ b/include/hw/acpi/acpi_dev_interface.h > @@ -31,7 +31,7 @@ typedef struct AcpiDeviceIf { > Object Parent; > } AcpiDeviceIf; > > -void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event); > +void acpi_send_event(AcpiDeviceIf *dev, AcpiEventStatusBits event); > > /** > * AcpiDeviceIfClass: > diff --git a/hw/acpi/acpi_interface.c b/hw/acpi/acpi_interface.c > index 6583917b8e..f9b538293e 100644 > --- a/hw/acpi/acpi_interface.c > +++ b/hw/acpi/acpi_interface.c > @@ -2,12 +2,11 @@ > #include "hw/acpi/acpi_dev_interface.h" > #include "qemu/module.h" > > -void acpi_send_event(DeviceState *dev, AcpiEventStatusBits event) > +void acpi_send_event(AcpiDeviceIf *dev, AcpiEventStatusBits event) > { > AcpiDeviceIfClass *adevc = ACPI_DEVICE_IF_GET_CLASS(dev); > if (adevc->send_event) { > - AcpiDeviceIf *adev = ACPI_DEVICE_IF(dev); > - adevc->send_event(adev, event); > + adevc->send_event(dev, event); > } > } > > diff --git a/hw/acpi/cpu.c b/hw/acpi/cpu.c > index 5ae595ecbe..2eb9b5a032 100644 > --- a/hw/acpi/cpu.c > +++ b/hw/acpi/cpu.c > @@ -233,7 +233,7 @@ void acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > cdev->cpu = CPU(dev); > if (dev->hotplugged) { > cdev->is_inserting = true; > - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > } > } > > @@ -249,7 +249,7 @@ void acpi_cpu_unplug_request_cb(HotplugHandler *hotplug_dev, > } > > cdev->is_removing = true; > - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_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 5243918125..5a1599796b 100644 > --- a/hw/acpi/cpu_hotplug.c > +++ b/hw/acpi/cpu_hotplug.c > @@ -79,7 +79,7 @@ void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > if (*errp != NULL) { > return; > } > - acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > } > > void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > index 0ff1712c4c..24b2aa0301 100644 > --- a/hw/acpi/memory_hotplug.c > +++ b/hw/acpi/memory_hotplug.c > @@ -280,7 +280,7 @@ 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(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > } > } > > @@ -296,7 +296,7 @@ void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > } > > mdev->is_removing = true; > - acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_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 27eeb6609f..bdc373696d 100644 > --- a/hw/acpi/nvdimm.c > +++ b/hw/acpi/nvdimm.c > @@ -921,7 +921,7 @@ static const MemoryRegionOps nvdimm_dsm_ops = { > void nvdimm_acpi_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev) > { > if (dev->hotplugged) { > - acpi_send_event(DEVICE(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_NVDIMM_HOTPLUG_STATUS); > } > } > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > index 80d42e12ff..b3bb11dac5 100644 > --- a/hw/acpi/pcihp.c > +++ b/hw/acpi/pcihp.c > @@ -237,7 +237,7 @@ void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > } > > s->acpi_pcihp_pci_status[bsel].up |= (1U << slot); > - acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > } > > void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState *s, > @@ -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(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > } > > static uint64_t pci_read(void *opaque, hwaddr addr, unsigned int size) > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index d78b579a20..74caedde79 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -158,7 +158,7 @@ static void vmgenid_update_guest(VmGenIdState *vms) > cpu_physical_memory_write(vmgenid_addr, guid_le.data, > sizeof(guid_le.data)); > /* Send _GPE.E05 event */ > - acpi_send_event(DEVICE(obj), ACPI_VMGENID_CHANGE_STATUS); > + acpi_send_event(ACPI_DEVICE_IF(obj), ACPI_VMGENID_CHANGE_STATUS); > } > } > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-23 19:31 [Qemu-devel] [PATCH for-3.1 0/2] ACPI type safety cleanup Eduardo Habkost 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety Eduardo Habkost @ 2018-07-23 19:31 ` Eduardo Habkost 2018-07-24 12:29 ` Igor Mammedov 1 sibling, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2018-07-23 19:31 UTC (permalink / raw) To: qemu-devel Cc: Marcel Apfelbaum, Igor Mammedov, Xiao Guangrong, Ben Warren, Michael S. Tsirkin 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(-) 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))); -- 2.18.0.rc1.1.g3f1ff2140 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler Eduardo Habkost @ 2018-07-24 12:29 ` Igor Mammedov 2018-07-24 12:39 ` Eduardo Habkost 0 siblings, 1 reply; 11+ messages in thread From: Igor Mammedov @ 2018-07-24 12:29 UTC (permalink / raw) To: Eduardo Habkost Cc: qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren, Michael S. Tsirkin 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))); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-24 12:29 ` Igor Mammedov @ 2018-07-24 12:39 ` Eduardo Habkost 2018-07-24 15:28 ` Igor Mammedov 0 siblings, 1 reply; 11+ messages in thread From: Eduardo Habkost @ 2018-07-24 12:39 UTC (permalink / raw) To: Igor Mammedov Cc: qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren, Michael S. Tsirkin 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? -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-24 12:39 ` Eduardo Habkost @ 2018-07-24 15:28 ` Igor Mammedov 2018-07-24 15:48 ` Eduardo Habkost 2018-07-24 18:14 ` Michael S. Tsirkin 0 siblings, 2 replies; 11+ messages in thread From: Igor Mammedov @ 2018-07-24 15:28 UTC (permalink / raw) To: Eduardo Habkost Cc: qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren, Michael S. Tsirkin 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-24 15:28 ` Igor Mammedov @ 2018-07-24 15:48 ` Eduardo Habkost 2018-07-24 18:14 ` Michael S. Tsirkin 1 sibling, 0 replies; 11+ messages in thread From: Eduardo Habkost @ 2018-07-24 15:48 UTC (permalink / raw) To: Igor Mammedov Cc: qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren, Michael S. Tsirkin 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? -- Eduardo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-24 15:28 ` Igor Mammedov 2018-07-24 15:48 ` Eduardo Habkost @ 2018-07-24 18:14 ` Michael S. Tsirkin 2018-07-25 6:57 ` Igor Mammedov 1 sibling, 1 reply; 11+ messages in thread From: Michael S. Tsirkin @ 2018-07-24 18:14 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren 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? -- MST ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-24 18:14 ` Michael S. Tsirkin @ 2018-07-25 6:57 ` Igor Mammedov 2018-07-25 13:46 ` Michael S. Tsirkin 0 siblings, 1 reply; 11+ messages in thread From: Igor Mammedov @ 2018-07-25 6:57 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Eduardo Habkost, qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler 2018-07-25 6:57 ` Igor Mammedov @ 2018-07-25 13:46 ` Michael S. Tsirkin 0 siblings, 0 replies; 11+ messages in thread From: Michael S. Tsirkin @ 2018-07-25 13:46 UTC (permalink / raw) To: Igor Mammedov Cc: Eduardo Habkost, qemu-devel, Marcel Apfelbaum, Xiao Guangrong, Ben Warren 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. -- MST ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-07-25 13:47 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-23 19:31 [Qemu-devel] [PATCH for-3.1 0/2] ACPI type safety cleanup Eduardo Habkost 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 1/2] acpi: Improve acpi_send_event() type safety Eduardo Habkost 2018-07-24 12:27 ` Igor Mammedov 2018-07-23 19:31 ` [Qemu-devel] [PATCH for-3.1 2/2] acpi: Decouple ACPI hotplug callbacks from HotplugHandler Eduardo Habkost 2018-07-24 12:29 ` Igor Mammedov 2018-07-24 12:39 ` Eduardo Habkost 2018-07-24 15:28 ` Igor Mammedov 2018-07-24 15:48 ` Eduardo Habkost 2018-07-24 18:14 ` Michael S. Tsirkin 2018-07-25 6:57 ` Igor Mammedov 2018-07-25 13:46 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).