Message ID | 1386349395-5710-5-git-send-email-imammedo@redhat.com |
---|---|
State | New |
Headers | show |
On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > Split piix4_device_hotplug() into hotplug/unplug callbacks > and register them as "hotplug-device" interface implementation of > PIIX4_PM device. > > Replace pci_bus_hotplug() wiring with setting link on > PCI BUS "hotplug-device" property to PIIX4_PM device. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------ > 1 files changed, 41 insertions(+), 32 deletions(-) > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > index 8084a60..8327d80 100644 > --- a/hw/acpi/piix4.c > +++ b/hw/acpi/piix4.c > @@ -30,6 +30,8 @@ > #include "hw/nvram/fw_cfg.h" > #include "exec/address-spaces.h" > #include "hw/acpi/piix4.h" > +#include "qapi/qmp/qerror.h" > +#include "hw/hotplug.h" > > //#define DEBUG > > @@ -107,7 +109,7 @@ typedef struct PIIX4PMState { > OBJECT_CHECK(PIIX4PMState, (obj), TYPE_PIIX4_PM) > > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > - PCIBus *bus, PIIX4PMState *s); > + BusState *bus, PIIX4PMState *s); > > #define ACPI_ENABLE 0xf1 > #define ACPI_DISABLE 0xf0 > @@ -478,7 +480,7 @@ static int piix4_pm_initfn(PCIDevice *dev) > qemu_add_machine_init_done_notifier(&s->machine_ready); > qemu_register_reset(piix4_reset, s); > > - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), dev->bus, s); > + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), BUS(dev->bus), s); > > piix4_pm_add_propeties(s); > return 0; > @@ -664,13 +666,11 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque) > piix4_cpu_hotplug_req(s, CPU(opaque), PLUG); > } > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > - PCIHotplugState state); > - > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > - PCIBus *bus, PIIX4PMState *s) > + BusState *bus, PIIX4PMState *s) > { > CPUState *cpu; > + Error *local_error = NULL; > > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, > "acpi-gpe0", GPE_LEN); > @@ -680,7 +680,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); > memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, > &s->io_pci); > - pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); > + object_property_set_link(OBJECT(bus), OBJECT(s), > + QDEV_HOTPLUG_DEVICE_PROPERTY, &local_error); > + if (error_is_set(&local_error)) { > + qerror_report_err(local_error); > + error_free(local_error); > + abort(); > + } > + bus->allow_hotplug = 1; I saw the exact same lines in this patch and the next two, I would "recycle" the pci_bus_hotplug (changing the signature and implementation) in order to avoid code duplication. What do you think? Thanks, Marcel > > CPU_FOREACH(cpu) { > CPUClass *cc = CPU_GET_CLASS(cpu); > @@ -696,41 +703,36 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > qemu_register_cpu_added_notifier(&s->cpu_added_notifier); > } > > -static void enable_device(PIIX4PMState *s, int slot) > +static void piix4_pci_device_hotplug_cb(DeviceState *hotplug_dev, > + DeviceState *dev, Error **errp) > { > - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > - s->pci0_slot_device_present |= (1U << slot); > -} > - > -static void disable_device(PIIX4PMState *s, int slot) > -{ > - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > - s->pci0_status.down |= (1U << slot); > -} > - > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > - PCIHotplugState state) > -{ > - int slot = PCI_SLOT(dev->devfn); > - PIIX4PMState *s = PIIX4_PM(qdev); > + PCIDevice *pci_dev = PCI_DEVICE(dev); > + int slot = PCI_SLOT(pci_dev->devfn); > + PIIX4PMState *s = PIIX4_PM(hotplug_dev); > > /* Don't send event when device is enabled during qemu machine creation: > * it is present on boot, no hotplug event is necessary. We do send an > * event when the device is disabled later. */ > - if (state == PCI_COLDPLUG_ENABLED) { > + if (!dev->hotplugged) { > s->pci0_slot_device_present |= (1U << slot); > - return 0; > - } > - > - if (state == PCI_HOTPLUG_ENABLED) { > - enable_device(s, slot); > - } else { > - disable_device(s, slot); > + return; > } > > + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > + s->pci0_slot_device_present |= (1U << slot); > pm_update_sci(s); > +} > > - return 0; > +static void piix4_pci_device_hot_unplug_cb(DeviceState *hotplug_dev, > + DeviceState *dev, Error **errp) > +{ > + PCIDevice *pci_dev = PCI_DEVICE(dev); > + int slot = PCI_SLOT(pci_dev->devfn); > + PIIX4PMState *s = PIIX4_PM(hotplug_dev); > + > + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > + s->pci0_status.down |= (1U << slot); > + pm_update_sci(s); > } > > static Property piix4_pm_properties[] = { > @@ -745,6 +747,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) > { > DeviceClass *dc = DEVICE_CLASS(klass); > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > + HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass); > > k->no_hotplug = 1; > k->init = piix4_pm_initfn; > @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) > dc->no_user = 1; > dc->vmsd = &vmstate_acpi; > dc->props = piix4_pm_properties; > + hc->hotplug = piix4_pci_device_hotplug_cb; > + hc->hot_unplug = piix4_pci_device_hot_unplug_cb; > } > > static const TypeInfo piix4_pm_info = { > @@ -764,6 +769,10 @@ static const TypeInfo piix4_pm_info = { > .parent = TYPE_PCI_DEVICE, > .instance_size = sizeof(PIIX4PMState), > .class_init = piix4_pm_class_init, > + .interfaces = (InterfaceInfo[]) { > + { TYPE_HOTPLUG_DEVICE }, > + { } > + } > }; > > static void piix4_pm_register_types(void)
On Mon, 09 Dec 2013 11:02:29 +0200 Marcel Apfelbaum <marcel.a@redhat.com> wrote: > On Fri, 2013-12-06 at 18:03 +0100, Igor Mammedov wrote: > > Split piix4_device_hotplug() into hotplug/unplug callbacks > > and register them as "hotplug-device" interface implementation of > > PIIX4_PM device. > > > > Replace pci_bus_hotplug() wiring with setting link on > > PCI BUS "hotplug-device" property to PIIX4_PM device. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------ > > 1 files changed, 41 insertions(+), 32 deletions(-) > > > > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c > > index 8084a60..8327d80 100644 > > --- a/hw/acpi/piix4.c > > +++ b/hw/acpi/piix4.c > > @@ -30,6 +30,8 @@ > > #include "hw/nvram/fw_cfg.h" > > #include "exec/address-spaces.h" > > #include "hw/acpi/piix4.h" > > +#include "qapi/qmp/qerror.h" > > +#include "hw/hotplug.h" > > > > //#define DEBUG > > > > @@ -107,7 +109,7 @@ typedef struct PIIX4PMState { > > OBJECT_CHECK(PIIX4PMState, (obj), TYPE_PIIX4_PM) > > > > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > > - PCIBus *bus, PIIX4PMState *s); > > + BusState *bus, PIIX4PMState *s); > > > > #define ACPI_ENABLE 0xf1 > > #define ACPI_DISABLE 0xf0 > > @@ -478,7 +480,7 @@ static int piix4_pm_initfn(PCIDevice *dev) > > qemu_add_machine_init_done_notifier(&s->machine_ready); > > qemu_register_reset(piix4_reset, s); > > > > - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), dev->bus, s); > > + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), BUS(dev->bus), s); > > > > piix4_pm_add_propeties(s); > > return 0; > > @@ -664,13 +666,11 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque) > > piix4_cpu_hotplug_req(s, CPU(opaque), PLUG); > > } > > > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > > - PCIHotplugState state); > > - > > static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > > - PCIBus *bus, PIIX4PMState *s) > > + BusState *bus, PIIX4PMState *s) > > { > > CPUState *cpu; > > + Error *local_error = NULL; > > > > memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, > > "acpi-gpe0", GPE_LEN); > > @@ -680,7 +680,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > > "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); > > memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, > > &s->io_pci); > > - pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); > > + object_property_set_link(OBJECT(bus), OBJECT(s), > > + QDEV_HOTPLUG_DEVICE_PROPERTY, &local_error); > > + if (error_is_set(&local_error)) { > > + qerror_report_err(local_error); > > + error_free(local_error); > > + abort(); > > + } > > + bus->allow_hotplug = 1; > I saw the exact same lines in this patch and the next two, > I would "recycle" the pci_bus_hotplug (changing the signature > and implementation) in order to avoid code duplication. > What do you think? it's not exactly the same, in shpc case we do not abort. I've left it with a bit of code duplication for reason that in abort case error check code disappears one Peter's abort_error patch is merged (it's on my todo list to switch to abort_error once available). And as for "bus->allow_hotplug = 1" it should also be removed once all current users are switched to common hotplug interface, then empty link could serve the same purpose as allow_hotplug. I'm not sure that creating function to replace essentially 2-liner in 2 places justifies trouble considering that it should be obsoleted anyway. > Thanks, > Marcel > > > > > CPU_FOREACH(cpu) { > > CPUClass *cc = CPU_GET_CLASS(cpu); > > @@ -696,41 +703,36 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, > > qemu_register_cpu_added_notifier(&s->cpu_added_notifier); > > } > > > > -static void enable_device(PIIX4PMState *s, int slot) > > +static void piix4_pci_device_hotplug_cb(DeviceState *hotplug_dev, > > + DeviceState *dev, Error **errp) > > { > > - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > > - s->pci0_slot_device_present |= (1U << slot); > > -} > > - > > -static void disable_device(PIIX4PMState *s, int slot) > > -{ > > - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > > - s->pci0_status.down |= (1U << slot); > > -} > > - > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > > - PCIHotplugState state) > > -{ > > - int slot = PCI_SLOT(dev->devfn); > > - PIIX4PMState *s = PIIX4_PM(qdev); > > + PCIDevice *pci_dev = PCI_DEVICE(dev); > > + int slot = PCI_SLOT(pci_dev->devfn); > > + PIIX4PMState *s = PIIX4_PM(hotplug_dev); > > > > /* Don't send event when device is enabled during qemu machine creation: > > * it is present on boot, no hotplug event is necessary. We do send an > > * event when the device is disabled later. */ > > - if (state == PCI_COLDPLUG_ENABLED) { > > + if (!dev->hotplugged) { > > s->pci0_slot_device_present |= (1U << slot); > > - return 0; > > - } > > - > > - if (state == PCI_HOTPLUG_ENABLED) { > > - enable_device(s, slot); > > - } else { > > - disable_device(s, slot); > > + return; > > } > > > > + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > > + s->pci0_slot_device_present |= (1U << slot); > > pm_update_sci(s); > > +} > > > > - return 0; > > +static void piix4_pci_device_hot_unplug_cb(DeviceState *hotplug_dev, > > + DeviceState *dev, Error **errp) > > +{ > > + PCIDevice *pci_dev = PCI_DEVICE(dev); > > + int slot = PCI_SLOT(pci_dev->devfn); > > + PIIX4PMState *s = PIIX4_PM(hotplug_dev); > > + > > + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; > > + s->pci0_status.down |= (1U << slot); > > + pm_update_sci(s); > > } > > > > static Property piix4_pm_properties[] = { > > @@ -745,6 +747,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) > > { > > DeviceClass *dc = DEVICE_CLASS(klass); > > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > > + HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass); > > > > k->no_hotplug = 1; > > k->init = piix4_pm_initfn; > > @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) > > dc->no_user = 1; > > dc->vmsd = &vmstate_acpi; > > dc->props = piix4_pm_properties; > > + hc->hotplug = piix4_pci_device_hotplug_cb; > > + hc->hot_unplug = piix4_pci_device_hot_unplug_cb; > > } > > > > static const TypeInfo piix4_pm_info = { > > @@ -764,6 +769,10 @@ static const TypeInfo piix4_pm_info = { > > .parent = TYPE_PCI_DEVICE, > > .instance_size = sizeof(PIIX4PMState), > > .class_init = piix4_pm_class_init, > > + .interfaces = (InterfaceInfo[]) { > > + { TYPE_HOTPLUG_DEVICE }, > > + { } > > + } > > }; > > > > static void piix4_pm_register_types(void) > > >
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c index 8084a60..8327d80 100644 --- a/hw/acpi/piix4.c +++ b/hw/acpi/piix4.c @@ -30,6 +30,8 @@ #include "hw/nvram/fw_cfg.h" #include "exec/address-spaces.h" #include "hw/acpi/piix4.h" +#include "qapi/qmp/qerror.h" +#include "hw/hotplug.h" //#define DEBUG @@ -107,7 +109,7 @@ typedef struct PIIX4PMState { OBJECT_CHECK(PIIX4PMState, (obj), TYPE_PIIX4_PM) static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, - PCIBus *bus, PIIX4PMState *s); + BusState *bus, PIIX4PMState *s); #define ACPI_ENABLE 0xf1 #define ACPI_DISABLE 0xf0 @@ -478,7 +480,7 @@ static int piix4_pm_initfn(PCIDevice *dev) qemu_add_machine_init_done_notifier(&s->machine_ready); qemu_register_reset(piix4_reset, s); - piix4_acpi_system_hot_add_init(pci_address_space_io(dev), dev->bus, s); + piix4_acpi_system_hot_add_init(pci_address_space_io(dev), BUS(dev->bus), s); piix4_pm_add_propeties(s); return 0; @@ -664,13 +666,11 @@ static void piix4_cpu_added_req(Notifier *n, void *opaque) piix4_cpu_hotplug_req(s, CPU(opaque), PLUG); } -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, - PCIHotplugState state); - static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, - PCIBus *bus, PIIX4PMState *s) + BusState *bus, PIIX4PMState *s) { CPUState *cpu; + Error *local_error = NULL; memory_region_init_io(&s->io_gpe, OBJECT(s), &piix4_gpe_ops, s, "acpi-gpe0", GPE_LEN); @@ -680,7 +680,14 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, "acpi-pci-hotplug", PCI_HOTPLUG_SIZE); memory_region_add_subregion(parent, PCI_HOTPLUG_ADDR, &s->io_pci); - pci_bus_hotplug(bus, piix4_device_hotplug, DEVICE(s)); + object_property_set_link(OBJECT(bus), OBJECT(s), + QDEV_HOTPLUG_DEVICE_PROPERTY, &local_error); + if (error_is_set(&local_error)) { + qerror_report_err(local_error); + error_free(local_error); + abort(); + } + bus->allow_hotplug = 1; CPU_FOREACH(cpu) { CPUClass *cc = CPU_GET_CLASS(cpu); @@ -696,41 +703,36 @@ static void piix4_acpi_system_hot_add_init(MemoryRegion *parent, qemu_register_cpu_added_notifier(&s->cpu_added_notifier); } -static void enable_device(PIIX4PMState *s, int slot) +static void piix4_pci_device_hotplug_cb(DeviceState *hotplug_dev, + DeviceState *dev, Error **errp) { - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; - s->pci0_slot_device_present |= (1U << slot); -} - -static void disable_device(PIIX4PMState *s, int slot) -{ - s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; - s->pci0_status.down |= (1U << slot); -} - -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, - PCIHotplugState state) -{ - int slot = PCI_SLOT(dev->devfn); - PIIX4PMState *s = PIIX4_PM(qdev); + PCIDevice *pci_dev = PCI_DEVICE(dev); + int slot = PCI_SLOT(pci_dev->devfn); + PIIX4PMState *s = PIIX4_PM(hotplug_dev); /* Don't send event when device is enabled during qemu machine creation: * it is present on boot, no hotplug event is necessary. We do send an * event when the device is disabled later. */ - if (state == PCI_COLDPLUG_ENABLED) { + if (!dev->hotplugged) { s->pci0_slot_device_present |= (1U << slot); - return 0; - } - - if (state == PCI_HOTPLUG_ENABLED) { - enable_device(s, slot); - } else { - disable_device(s, slot); + return; } + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; + s->pci0_slot_device_present |= (1U << slot); pm_update_sci(s); +} - return 0; +static void piix4_pci_device_hot_unplug_cb(DeviceState *hotplug_dev, + DeviceState *dev, Error **errp) +{ + PCIDevice *pci_dev = PCI_DEVICE(dev); + int slot = PCI_SLOT(pci_dev->devfn); + PIIX4PMState *s = PIIX4_PM(hotplug_dev); + + s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; + s->pci0_status.down |= (1U << slot); + pm_update_sci(s); } static Property piix4_pm_properties[] = { @@ -745,6 +747,7 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); + HotplugDeviceClass *hc = HOTPLUG_DEVICE_CLASS(klass); k->no_hotplug = 1; k->init = piix4_pm_initfn; @@ -757,6 +760,8 @@ static void piix4_pm_class_init(ObjectClass *klass, void *data) dc->no_user = 1; dc->vmsd = &vmstate_acpi; dc->props = piix4_pm_properties; + hc->hotplug = piix4_pci_device_hotplug_cb; + hc->hot_unplug = piix4_pci_device_hot_unplug_cb; } static const TypeInfo piix4_pm_info = { @@ -764,6 +769,10 @@ static const TypeInfo piix4_pm_info = { .parent = TYPE_PCI_DEVICE, .instance_size = sizeof(PIIX4PMState), .class_init = piix4_pm_class_init, + .interfaces = (InterfaceInfo[]) { + { TYPE_HOTPLUG_DEVICE }, + { } + } }; static void piix4_pm_register_types(void)
Split piix4_device_hotplug() into hotplug/unplug callbacks and register them as "hotplug-device" interface implementation of PIIX4_PM device. Replace pci_bus_hotplug() wiring with setting link on PCI BUS "hotplug-device" property to PIIX4_PM device. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- hw/acpi/piix4.c | 73 +++++++++++++++++++++++++++++++------------------------ 1 files changed, 41 insertions(+), 32 deletions(-)