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 <imamm...@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)
