On Thu, 2 Jun 2016 14:13:23 +0300 Marcel Apfelbaum <mar...@redhat.com> wrote:
> On 05/31/2016 01:01 PM, Igor Mammedov wrote: > > it reduces number of args passed in handlers by 1 and > > a number of used proxy wrappers saving ~20LOC. > > Also it allows to make cpu/mem hotplug code more > > universal as it would allow ARM to reuse it without > > rewrite by providing its own send_event callback > > to trigger events usiong GPIO instead of GPE > > as fixed hadrware ACPI model doen't have GPE at all. > > > > Signed-off-by: Igor Mammedov <imamm...@redhat.com> > > --- > > v2: > > - make ACPI_SEND_EVENT macro a function > > Marcel Apfelbaum <mar...@redhat.com> > > --- > > hw/acpi/cpu_hotplug.c | 5 ++--- > > hw/acpi/ich9.c | 33 ++++++++++++++++++++------------- > > hw/acpi/memory_hotplug.c | 12 ++++-------- > > hw/acpi/pcihp.c | 10 ++++------ > > hw/acpi/piix4.c | 11 +++++------ > > hw/isa/lpc_ich9.c | 30 +++--------------------------- > > include/hw/acpi/cpu_hotplug.h | 3 ++- > > include/hw/acpi/ich9.h | 9 +++++---- > > include/hw/acpi/memory_hotplug.h | 4 ++-- > > include/hw/acpi/pcihp.h | 5 +++-- > > 10 files changed, 50 insertions(+), 72 deletions(-) > > > > diff --git a/hw/acpi/cpu_hotplug.c b/hw/acpi/cpu_hotplug.c > > index 9d71d2f..fe75bd9 100644 > > --- a/hw/acpi/cpu_hotplug.c > > +++ b/hw/acpi/cpu_hotplug.c > > @@ -62,15 +62,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(ACPIREGS *ar, qemu_irq irq, > > +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > > AcpiCpuHotplug *g, DeviceState *dev, Error > > **errp) > > { > > acpi_set_cpu_present_bit(g, CPU(dev), errp); > > if (*errp != NULL) { > > return; > > } > > - > > - acpi_send_gpe_event(ar, irq, ACPI_CPU_HOTPLUG_STATUS); > > + acpi_send_event(DEVICE(hotplug_dev), ACPI_CPU_HOTPLUG_STATUS); > > > The patch looks good to me. I have only one question. > Why do you cast hotplug_dev to DEVICE and not a the safer expected interface? > Or even HotplugHandler it's not HotplugHandler because acpi events in general is not hotplug related. I wouldn't say that casting to interface any more safer than casting to DEVICE, I'm using DeviceState here because it will save some call sites a trouble of casting to interface. > > Thanks, > Marcel > > > } > > > > void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > > diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c > > index af340d0..853c9c4 100644 > > --- a/hw/acpi/ich9.c > > +++ b/hw/acpi/ich9.c > > @@ -430,40 +430,47 @@ void ich9_pm_add_properties(Object *obj, > > ICH9LPCPMRegs *pm, Error **errp) > > NULL); > > } > > > > -void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error > > **errp) > > +void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > + Error **errp) > > { > > - if (pm->acpi_memory_hotplug.is_enabled && > > + ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > + > > + if (lpc->pm.acpi_memory_hotplug.is_enabled && > > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > - acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, > > &pm->acpi_memory_hotplug, > > + acpi_memory_plug_cb(hotplug_dev, &lpc->pm.acpi_memory_hotplug, > > dev, errp); > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > - legacy_acpi_cpu_plug_cb(&pm->acpi_regs, pm->irq, > > - &pm->gpe_cpu, dev, errp); > > + legacy_acpi_cpu_plug_cb(hotplug_dev, &lpc->pm.gpe_cpu, dev, errp); > > } else { > > error_setg(errp, "acpi: device plug request for not supported > > device" > > " type: %s", object_get_typename(OBJECT(dev))); > > } > > } > > > > -void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev, > > - Error **errp) > > +void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > + DeviceState *dev, Error **errp) > > { > > - if (pm->acpi_memory_hotplug.is_enabled && > > + ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > + > > + if (lpc->pm.acpi_memory_hotplug.is_enabled && > > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > - acpi_memory_unplug_request_cb(&pm->acpi_regs, pm->irq, > > - &pm->acpi_memory_hotplug, dev, errp); > > + acpi_memory_unplug_request_cb(hotplug_dev, > > + &lpc->pm.acpi_memory_hotplug, dev, > > + errp); > > } else { > > error_setg(errp, "acpi: device unplug request for not supported > > device" > > " type: %s", object_get_typename(OBJECT(dev))); > > } > > } > > > > -void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, > > +void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState > > *dev, > > Error **errp) > > { > > - if (pm->acpi_memory_hotplug.is_enabled && > > + ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > + > > + if (lpc->pm.acpi_memory_hotplug.is_enabled && > > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > - acpi_memory_unplug_cb(&pm->acpi_memory_hotplug, dev, errp); > > + acpi_memory_unplug_cb(&lpc->pm.acpi_memory_hotplug, dev, errp); > > } else { > > error_setg(errp, "acpi: device unplug for not supported device" > > " type: %s", object_get_typename(OBJECT(dev))); > > diff --git a/hw/acpi/memory_hotplug.c b/hw/acpi/memory_hotplug.c > > index f65a3a2..ec4e64b 100644 > > --- a/hw/acpi/memory_hotplug.c > > +++ b/hw/acpi/memory_hotplug.c > > @@ -228,7 +228,7 @@ acpi_memory_slot_status(MemHotplugState *mem_st, > > return &mem_st->devs[slot]; > > } > > > > -void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState > > *mem_st, > > +void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState > > *mem_st, > > DeviceState *dev, Error **errp) > > { > > MemStatus *mdev; > > @@ -247,13 +247,11 @@ void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, > > MemHotplugState *mem_st, > > mdev->is_enabled = true; > > if (dev->hotplugged) { > > mdev->is_inserting = true; > > - > > - /* do ACPI magic */ > > - acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS); > > + acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > > } > > } > > > > -void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq, > > +void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_dev, > > MemHotplugState *mem_st, > > DeviceState *dev, Error **errp) > > { > > @@ -265,9 +263,7 @@ void acpi_memory_unplug_request_cb(ACPIREGS *ar, > > qemu_irq irq, > > } > > > > mdev->is_removing = true; > > - > > - /* Do ACPI magic */ > > - acpi_send_gpe_event(ar, irq, ACPI_MEMORY_HOTPLUG_STATUS); > > + acpi_send_event(DEVICE(hotplug_dev), ACPI_MEMORY_HOTPLUG_STATUS); > > } > > > > void acpi_memory_unplug_cb(MemHotplugState *mem_st, > > diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c > > index 71f4c4e..d957d1e 100644 > > --- a/hw/acpi/pcihp.c > > +++ b/hw/acpi/pcihp.c > > @@ -182,7 +182,7 @@ void acpi_pcihp_reset(AcpiPciHpState *s) > > acpi_pcihp_update(s); > > } > > > > -void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState > > *s, > > +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState > > *s, > > DeviceState *dev, Error **errp) > > { > > PCIDevice *pdev = PCI_DEVICE(dev); > > @@ -202,11 +202,10 @@ void acpi_pcihp_device_plug_cb(ACPIREGS *ar, qemu_irq > > irq, AcpiPciHpState *s, > > } > > > > s->acpi_pcihp_pci_status[bsel].up |= (1U << slot); > > - > > - acpi_send_gpe_event(ar, irq, ACPI_PCI_HOTPLUG_STATUS); > > + acpi_send_event(DEVICE(hotplug_dev), ACPI_PCI_HOTPLUG_STATUS); > > } > > > > -void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, > > AcpiPciHpState *s, > > +void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, > > AcpiPciHpState *s, > > DeviceState *dev, Error **errp) > > { > > PCIDevice *pdev = PCI_DEVICE(dev); > > @@ -219,8 +218,7 @@ void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq > > irq, AcpiPciHpState *s, > > } > > > > s->acpi_pcihp_pci_status[bsel].down |= (1U << slot); > > - > > - acpi_send_gpe_event(ar, irq, ACPI_PCI_HOTPLUG_STATUS); > > + acpi_send_event(DEVICE(hotplug_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 5b4fcb5..c48cb1b 100644 > > --- a/hw/acpi/piix4.c > > +++ b/hw/acpi/piix4.c > > @@ -348,12 +348,11 @@ static void piix4_device_plug_cb(HotplugHandler > > *hotplug_dev, > > > > if (s->acpi_memory_hotplug.is_enabled && > > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > - acpi_memory_plug_cb(&s->ar, s->irq, &s->acpi_memory_hotplug, dev, > > errp); > > + acpi_memory_plug_cb(hotplug_dev, &s->acpi_memory_hotplug, dev, > > errp); > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > > - acpi_pcihp_device_plug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, > > dev, > > - errp); > > + acpi_pcihp_device_plug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, > > errp); > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_CPU)) { > > - legacy_acpi_cpu_plug_cb(&s->ar, s->irq, &s->gpe_cpu, dev, errp); > > + legacy_acpi_cpu_plug_cb(hotplug_dev, &s->gpe_cpu, dev, errp); > > } else { > > error_setg(errp, "acpi: device plug request for not supported > > device" > > " type: %s", object_get_typename(OBJECT(dev))); > > @@ -367,10 +366,10 @@ static void > > piix4_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > > > if (s->acpi_memory_hotplug.is_enabled && > > object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) { > > - acpi_memory_unplug_request_cb(&s->ar, s->irq, > > &s->acpi_memory_hotplug, > > + acpi_memory_unplug_request_cb(hotplug_dev, &s->acpi_memory_hotplug, > > dev, errp); > > } else if (object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE)) { > > - acpi_pcihp_device_unplug_cb(&s->ar, s->irq, &s->acpi_pci_hotplug, > > dev, > > + acpi_pcihp_device_unplug_cb(hotplug_dev, &s->acpi_pci_hotplug, dev, > > errp); > > } else { > > error_setg(errp, "acpi: device unplug request for not supported > > device" > > diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c > > index 72d0781..2a2d52e 100644 > > --- a/hw/isa/lpc_ich9.c > > +++ b/hw/isa/lpc_ich9.c > > @@ -637,30 +637,6 @@ static void ich9_lpc_realize(PCIDevice *d, Error > > **errp) > > 1); > > } > > > > -static void ich9_device_plug_cb(HotplugHandler *hotplug_dev, > > - DeviceState *dev, Error **errp) > > -{ > > - ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > - > > - ich9_pm_device_plug_cb(&lpc->pm, dev, errp); > > -} > > - > > -static void ich9_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > - DeviceState *dev, Error **errp) > > -{ > > - ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > - > > - ich9_pm_device_unplug_request_cb(&lpc->pm, dev, errp); > > -} > > - > > -static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev, > > - DeviceState *dev, Error **errp) > > -{ > > - ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev); > > - > > - ich9_pm_device_unplug_cb(&lpc->pm, dev, errp); > > -} > > - > > static bool ich9_rst_cnt_needed(void *opaque) > > { > > ICH9LPCState *lpc = opaque; > > @@ -733,9 +709,9 @@ static void ich9_lpc_class_init(ObjectClass *klass, > > void *data) > > * pc_q35_init() > > */ > > dc->cannot_instantiate_with_device_add_yet = true; > > - hc->plug = ich9_device_plug_cb; > > - hc->unplug_request = ich9_device_unplug_request_cb; > > - hc->unplug = ich9_device_unplug_cb; > > + hc->plug = ich9_pm_device_plug_cb; > > + hc->unplug_request = ich9_pm_device_unplug_request_cb; > > + hc->unplug = ich9_pm_device_unplug_cb; > > adevc->ospm_status = ich9_pm_ospm_status; > > adevc->send_event = ich9_send_gpe; > > } > > diff --git a/include/hw/acpi/cpu_hotplug.h b/include/hw/acpi/cpu_hotplug.h > > index 6d729d8..6fef67e 100644 > > --- a/include/hw/acpi/cpu_hotplug.h > > +++ b/include/hw/acpi/cpu_hotplug.h > > @@ -15,13 +15,14 @@ > > #include "hw/acpi/acpi.h" > > #include "hw/acpi/pc-hotplug.h" > > #include "hw/acpi/aml-build.h" > > +#include "hw/hotplug.h" > > > > typedef struct AcpiCpuHotplug { > > MemoryRegion io; > > uint8_t sts[ACPI_GPE_PROC_LEN]; > > } AcpiCpuHotplug; > > > > -void legacy_acpi_cpu_plug_cb(ACPIREGS *ar, qemu_irq irq, > > +void legacy_acpi_cpu_plug_cb(HotplugHandler *hotplug_dev, > > AcpiCpuHotplug *g, DeviceState *dev, Error > > **errp); > > > > void legacy_acpi_cpu_hotplug_init(MemoryRegion *parent, Object *owner, > > diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h > > index 63fa198..bbd657c 100644 > > --- a/include/hw/acpi/ich9.h > > +++ b/include/hw/acpi/ich9.h > > @@ -69,10 +69,11 @@ extern const VMStateDescription vmstate_ich9_pm; > > > > void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp); > > > > -void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error > > **errp); > > -void ich9_pm_device_unplug_request_cb(ICH9LPCPMRegs *pm, DeviceState *dev, > > - Error **errp); > > -void ich9_pm_device_unplug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, > > +void ich9_pm_device_plug_cb(HotplugHandler *hotplug_dev, DeviceState *dev, > > + Error **errp); > > +void ich9_pm_device_unplug_request_cb(HotplugHandler *hotplug_dev, > > + DeviceState *dev, Error **errp); > > +void ich9_pm_device_unplug_cb(HotplugHandler *hotplug_dev, DeviceState > > *dev, > > Error **errp); > > > > void ich9_pm_ospm_status(AcpiDeviceIf *adev, ACPIOSTInfoList ***list); > > diff --git a/include/hw/acpi/memory_hotplug.h > > b/include/hw/acpi/memory_hotplug.h > > index 3a646b1..d2c7452 100644 > > --- a/include/hw/acpi/memory_hotplug.h > > +++ b/include/hw/acpi/memory_hotplug.h > > @@ -32,9 +32,9 @@ typedef struct MemHotplugState { > > void acpi_memory_hotplug_init(MemoryRegion *as, Object *owner, > > MemHotplugState *state); > > > > -void acpi_memory_plug_cb(ACPIREGS *ar, qemu_irq irq, MemHotplugState > > *mem_st, > > +void acpi_memory_plug_cb(HotplugHandler *hotplug_dev, MemHotplugState > > *mem_st, > > DeviceState *dev, Error **errp); > > -void acpi_memory_unplug_request_cb(ACPIREGS *ar, qemu_irq irq, > > +void acpi_memory_unplug_request_cb(HotplugHandler *hotplug_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 79a4392..04528b7 100644 > > --- a/include/hw/acpi/pcihp.h > > +++ b/include/hw/acpi/pcihp.h > > @@ -29,6 +29,7 @@ > > > > #include "hw/acpi/acpi.h" > > #include "migration/vmstate.h" > > +#include "hw/hotplug.h" > > > > #define ACPI_PCIHP_IO_BASE_PROP "acpi-pcihp-io-base" > > #define ACPI_PCIHP_IO_LEN_PROP "acpi-pcihp-io-len" > > @@ -56,9 +57,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(ACPIREGS *ar, qemu_irq irq, AcpiPciHpState > > *s, > > +void acpi_pcihp_device_plug_cb(HotplugHandler *hotplug_dev, AcpiPciHpState > > *s, > > DeviceState *dev, Error **errp); > > -void acpi_pcihp_device_unplug_cb(ACPIREGS *ar, qemu_irq irq, > > AcpiPciHpState *s, > > +void acpi_pcihp_device_unplug_cb(HotplugHandler *hotplug_dev, > > AcpiPciHpState *s, > > DeviceState *dev, Error **errp); > > > > /* Called on reset */ > > >