diff mbox

[02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices

Message ID 1385001528-12003-3-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Nov. 21, 2013, 2:38 a.m. UTC
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
resolving conflict with pcihp: s/PCIHotplugState/HotplugState/
perhaps this patch should go before  pcihp patches to avoid
unnecessary code churn??
---
 hw/acpi/pcihp.c          |    6 +++---
 hw/acpi/piix4.c          |   22 ++++++++++++----------
 hw/pci/pci.c             |   12 ++++++------
 hw/pci/pcie.c            |    7 ++++---
 hw/pci/shpc.c            |    9 +++++----
 include/hw/acpi/pcihp.h  |    2 +-
 include/hw/pci/pci.h     |   11 +----------
 include/hw/pci/pci_bus.h |    2 +-
 include/hw/qdev-core.h   |    9 +++++++++
 9 files changed, 42 insertions(+), 38 deletions(-)

Comments

Paolo Bonzini Nov. 25, 2013, 12:49 p.m. UTC | #1
Il 21/11/2013 03:38, Igor Mammedov ha scritto:
> +typedef enum {
> +    HOTPLUG_DISABLED,
> +    HOTPLUG_ENABLED,
> +    COLDPLUG_ENABLED,
> +} HotplugState;
> +
> +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev,
> +                          HotplugState state);

I don't think this is a particularly useful interface, because it
reinvents a lot of what already exists in qdev/qbus.

The cold/hotplug_enabled differentiation is not particularly useful
when dev->hotplugged already exists, and the interface leaves one to
wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED
(until one looks at the code).

Take for example this:

static void enable_device(PIIX4PMState *s, int slot)
{
    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);

    /* 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) {
        s->pci0_slot_device_present |= (1U << slot);
        return 0;
    }

    if (state == PCI_HOTPLUG_ENABLED) {
        enable_device(s, slot);
    } else {
        disable_device(s, slot);
    }

    pm_update_sci(s);

    return 0;
}


In my opinion, it is much clearer this way---separating enable/disabled
rather than hotplug/coldplug:

static void piix4_pci_send_event(PIIX4PMState *s)
{
    s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
    pm_update_sci(s);
}

static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev)
{
    PCIDevice *pci = PCI_DEVICE(dev);
    int slot = PCI_SLOT(pci->devfn);
    PIIX4PMState *s = PIIX4_PM(qdev);

    s->pci0_slot_device_present |= (1U << slot);
    /* 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 (dev->hotplugged) {
        piix4_pci_send_event(s);
    }
    return 0;
}

static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev)
{
    PCIDevice *pci = PCI_DEVICE(dev);
    int slot = PCI_SLOT(pci->devfn);
    PIIX4PMState *s = PIIX4_PM(qdev);

    s->pci0_status.down |= (1U << slot);
    piix4_pci_send_event(s);
    return 0;
}

This is of course just an example, I'm not asking you to reimplement hotplug
for all buses in QEMU.  However, my point is that this shouldn't be a "core"
enum and interface.  If you want to have a core interface in BusClass, I'd
rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI
bus does it (note: I only maintain it, I didn't write that code) and so
that BusClass's methods match ->init/->exit on the device side.

Paolo
Paolo Bonzini Nov. 25, 2013, 1:11 p.m. UTC | #2
Il 25/11/2013 13:49, Paolo Bonzini ha scritto:
> Il 21/11/2013 03:38, Igor Mammedov ha scritto:
>> +typedef enum {
>> +    HOTPLUG_DISABLED,
>> +    HOTPLUG_ENABLED,
>> +    COLDPLUG_ENABLED,
>> +} HotplugState;
>> +
>> +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev,
>> +                          HotplugState state);
> 
> I don't think this is a particularly useful interface, because it
> reinvents a lot of what already exists in qdev/qbus.
> 
> The cold/hotplug_enabled differentiation is not particularly useful
> when dev->hotplugged already exists, and the interface leaves one to
> wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED
> (until one looks at the code).
> 
> Take for example this:
> 
> static void enable_device(PIIX4PMState *s, int slot)
> {
>     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);
> 
>     /* 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) {
>         s->pci0_slot_device_present |= (1U << slot);
>         return 0;
>     }
> 
>     if (state == PCI_HOTPLUG_ENABLED) {
>         enable_device(s, slot);
>     } else {
>         disable_device(s, slot);
>     }
> 
>     pm_update_sci(s);
> 
>     return 0;
> }
> 
> 
> In my opinion, it is much clearer this way---separating enable/disabled
> rather than hotplug/coldplug:
> 
> static void piix4_pci_send_event(PIIX4PMState *s)
> {
>     s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
>     pm_update_sci(s);
> }
> 
> static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_slot_device_present |= (1U << slot);
>     /* 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 (dev->hotplugged) {
>         piix4_pci_send_event(s);
>     }
>     return 0;
> }
> 
> static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_status.down |= (1U << slot);
>     piix4_pci_send_event(s);
>     return 0;
> }
> 
> This is of course just an example, I'm not asking you to reimplement hotplug
> for all buses in QEMU.  However, my point is that this shouldn't be a "core"
> enum and interface.  If you want to have a core interface in BusClass, I'd
> rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI
> bus does it (note: I only maintain it, I didn't write that code) and so
> that BusClass's methods match ->init/->exit on the device side.

I reviewed the user now.

I understand why you want to reuse the same idiom.  Perhaps you can move
this to include/hw/hotplug.h instead?  I would still prefer DimmBus or
BusClass to use the more common hotplug/hot_unplug interface, but
ACPIHotpluggableDimmBus can map it to an hotplug_fn.

Paolo
Igor Mammedov Nov. 25, 2013, 3:57 p.m. UTC | #3
On Mon, 25 Nov 2013 13:49:29 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Il 21/11/2013 03:38, Igor Mammedov ha scritto:
> > +typedef enum {
> > +    HOTPLUG_DISABLED,
> > +    HOTPLUG_ENABLED,
> > +    COLDPLUG_ENABLED,
> > +} HotplugState;
> > +
> > +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev,
> > +                          HotplugState state);
> 
> I don't think this is a particularly useful interface, because it
> reinvents a lot of what already exists in qdev/qbus.
> 
> The cold/hotplug_enabled differentiation is not particularly useful
> when dev->hotplugged already exists, and the interface leaves one to
> wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED
> (until one looks at the code).
Yes, it's a bit weird but I've opted in favor of unifying currently
used by piix4/ich9_acpi approach so it could be reused vs rewriting
yet another part of QEMU and Michael also uses this in his new PCIEHP
patches.
Maybe rewriting it as you suggest should be made as a separate series.


But I have unrelated to this question, regarding patch
 "[PATCH 01/27] acpi: factor out common pm_update_sci() into acpi core"

in old pm_update_sci() we have:
    sci_level = (((pmsts & s->ar.pm1.evt.en) &
                  (ACPI_BITMASK_RT_CLOCK_ENABLE |
                   ACPI_BITMASK_POWER_BUTTON_ENABLE |
                   ACPI_BITMASK_GLOBAL_LOCK_ENABLE |
                   ACPI_BITMASK_TIMER_ENABLE)) != 0) ||
        (((s->ar.gpe.sts[0] & s->ar.gpe.en[0]) &
          (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS)) != 0);

where we mask out currently not supported GPE status bits,
can we just drop (PIIX4_PCI_HOTPLUG_STATUS | PIIX4_CPU_HOTPLUG_STATUS) mask
altogether and rely on gpe.sts & gpe.en only for rising SCI?
Why there were need to mask SCI at all?

> Take for example this:
> 
> static void enable_device(PIIX4PMState *s, int slot)
> {
>     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);
> 
>     /* 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) {
>         s->pci0_slot_device_present |= (1U << slot);
>         return 0;
>     }
> 
>     if (state == PCI_HOTPLUG_ENABLED) {
>         enable_device(s, slot);
>     } else {
>         disable_device(s, slot);
>     }
> 
>     pm_update_sci(s);
> 
>     return 0;
> }
> 
> 
> In my opinion, it is much clearer this way---separating enable/disabled
> rather than hotplug/coldplug:
> 
> static void piix4_pci_send_event(PIIX4PMState *s)
> {
>     s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
>     pm_update_sci(s);
> }
> 
> static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_slot_device_present |= (1U << slot);
>     /* 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 (dev->hotplugged) {
>         piix4_pci_send_event(s);
>     }
>     return 0;
> }
> 
> static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev)
> {
>     PCIDevice *pci = PCI_DEVICE(dev);
>     int slot = PCI_SLOT(pci->devfn);
>     PIIX4PMState *s = PIIX4_PM(qdev);
> 
>     s->pci0_status.down |= (1U << slot);
>     piix4_pci_send_event(s);
>     return 0;
> }
> 
> This is of course just an example, I'm not asking you to reimplement hotplug
> for all buses in QEMU.  However, my point is that this shouldn't be a "core"
> enum and interface.  If you want to have a core interface in BusClass, I'd
> rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI
> bus does it (note: I only maintain it, I didn't write that code) and so
> that BusClass's methods match ->init/->exit on the device side.
> 
> Paolo
diff mbox

Patch

diff --git a/hw/acpi/pcihp.c b/hw/acpi/pcihp.c
index 8a0822e..04d39e9 100644
--- a/hw/acpi/pcihp.c
+++ b/hw/acpi/pcihp.c
@@ -195,7 +195,7 @@  static void disable_device(AcpiPciHpState *s, unsigned bsel, int slot)
 }
 
 int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *dev,
-                              PCIHotplugState state)
+                              HotplugState state)
 {
     int slot = PCI_SLOT(dev->devfn);
     int bsel = acpi_pcihp_get_bsel(dev->bus);
@@ -206,12 +206,12 @@  int acpi_pcihp_device_hotplug(AcpiPciHpState *s, PCIDevice *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 (state == COLDPLUG_ENABLED) {
         s->acpi_pcihp_pci_status[bsel].device_present |= (1U << slot);
         return 0;
     }
 
-    if (state == PCI_HOTPLUG_ENABLED) {
+    if (state == HOTPLUG_ENABLED) {
         enable_device(s, bsel, slot);
     } else {
         disable_device(s, bsel, slot);
diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index b6dfa71..cf42f13 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -401,11 +401,12 @@  static void piix4_pm_powerdown_req(Notifier *n, void *opaque)
     acpi_pm1_evt_power_down(&s->ar);
 }
 
-static int piix4_acpi_pci_hotplug(DeviceState *qdev, PCIDevice *dev,
-                                  PCIHotplugState state)
+static int piix4_acpi_pci_hotplug(DeviceState *qdev, DeviceState *dev,
+                                  HotplugState state)
 {
     PIIX4PMState *s = PIIX4_PM(qdev);
-    int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, dev, state);
+    int ret = acpi_pcihp_device_hotplug(&s->acpi_pci_hotplug, PCI_DEVICE(dev),
+                                        state);
     if (ret < 0) {
         return ret;
     }
@@ -740,8 +741,8 @@  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 int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev,
+                                HotplugState state);
 
 static void piix4_acpi_system_hot_add_init(MemoryRegion *parent,
                                            PCIBus *bus, PIIX4PMState *s)
@@ -788,21 +789,22 @@  static void disable_device(PIIX4PMState *s, int slot)
     s->pci0_status.down |= (1U << slot);
 }
 
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
-				PCIHotplugState state)
+static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev,
+                                HotplugState state)
 {
-    int slot = PCI_SLOT(dev->devfn);
+    PCIDevice *d = PCI_DEVICE(dev);
+    int slot = PCI_SLOT(d->devfn);
     PIIX4PMState *s = PIIX4_PM(qdev);
 
     /* 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 (state == COLDPLUG_ENABLED) {
         s->pci0_slot_device_present |= (1U << slot);
         return 0;
     }
 
-    if (state == PCI_HOTPLUG_ENABLED) {
+    if (state == HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
         disable_device(s, slot);
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 31165cf..0713a1b 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -346,7 +346,7 @@  void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
     bus->irq_count = g_malloc0(nirq * sizeof(bus->irq_count[0]));
 }
 
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *qdev)
+void pci_bus_hotplug(PCIBus *bus, hotplug_fn hotplug, DeviceState *qdev)
 {
     bus->qbus.allow_hotplug = 1;
     bus->hotplug = hotplug;
@@ -1779,9 +1779,9 @@  static int pci_qdev_init(DeviceState *qdev)
     if (bus->hotplug) {
         /* Let buses differentiate between hotplug and when device is
          * enabled during qemu machine creation. */
-        rc = bus->hotplug(bus->hotplug_qdev, pci_dev,
-                          qdev->hotplugged ? PCI_HOTPLUG_ENABLED:
-                          PCI_COLDPLUG_ENABLED);
+        rc = bus->hotplug(bus->hotplug_qdev, DEVICE(pci_dev),
+                          qdev->hotplugged ? HOTPLUG_ENABLED :
+                          COLDPLUG_ENABLED);
         if (rc != 0) {
             int r = pci_unregister_device(&pci_dev->qdev);
             assert(!r);
@@ -1800,8 +1800,8 @@  static int pci_unplug_device(DeviceState *qdev)
         qerror_report(QERR_DEVICE_NO_HOTPLUG, object_get_typename(OBJECT(dev)));
         return -1;
     }
-    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
-                             PCI_HOTPLUG_DISABLED);
+    return dev->bus->hotplug(dev->bus->hotplug_qdev, qdev,
+                             HOTPLUG_DISABLED);
 }
 
 PCIDevice *pci_create_multifunction(PCIBus *bus, int devfn, bool multifunction,
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index ca60cf2..64500c1 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -217,16 +217,17 @@  static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
 }
 
 static int pcie_cap_slot_hotplug(DeviceState *qdev,
-                                 PCIDevice *pci_dev, PCIHotplugState state)
+                                 DeviceState *dev, HotplugState state)
 {
     PCIDevice *d = PCI_DEVICE(qdev);
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
     uint8_t *exp_cap = d->config + d->exp.exp_cap;
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 
     /* 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 (state == COLDPLUG_ENABLED) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                    PCI_EXP_SLTSTA_PDS);
         return 0;
@@ -246,7 +247,7 @@  static int pcie_cap_slot_hotplug(DeviceState *qdev,
      */
     assert(PCI_FUNC(pci_dev->devfn) == 0);
 
-    if (state == PCI_HOTPLUG_ENABLED) {
+    if (state == HOTPLUG_ENABLED) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                    PCI_EXP_SLTSTA_PDS);
         pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);
diff --git a/hw/pci/shpc.c b/hw/pci/shpc.c
index 576244b..e7efe63 100644
--- a/hw/pci/shpc.c
+++ b/hw/pci/shpc.c
@@ -491,9 +491,10 @@  static const MemoryRegionOps shpc_mmio_ops = {
     },
 };
 
-static int shpc_device_hotplug(DeviceState *qdev, PCIDevice *affected_dev,
-                               PCIHotplugState hotplug_state)
+static int shpc_device_hotplug(DeviceState *qdev, DeviceState *dev,
+                               HotplugState hotplug_state)
 {
+    PCIDevice *affected_dev = PCI_DEVICE(dev);
     int pci_slot = PCI_SLOT(affected_dev->devfn);
     uint8_t state;
     uint8_t led;
@@ -510,13 +511,13 @@  static int shpc_device_hotplug(DeviceState *qdev, PCIDevice *affected_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 (hotplug_state == PCI_COLDPLUG_ENABLED) {
+    if (hotplug_state == COLDPLUG_ENABLED) {
         shpc_set_status(shpc, slot, 0, SHPC_SLOT_STATUS_MRL_OPEN);
         shpc_set_status(shpc, slot, SHPC_SLOT_STATUS_PRSNT_7_5W,
                         SHPC_SLOT_STATUS_PRSNT_MASK);
         return 0;
     }
-    if (hotplug_state == PCI_HOTPLUG_DISABLED) {
+    if (hotplug_state == HOTPLUG_DISABLED) {
         shpc->config[SHPC_SLOT_EVENT_LATCH(slot)] |= SHPC_SLOT_EVENT_BUTTON;
         state = shpc_get_status(shpc, slot, SHPC_SLOT_STATE_MASK);
         led = shpc_get_status(shpc, slot, SHPC_SLOT_PWR_LED_MASK);
diff --git a/include/hw/acpi/pcihp.h b/include/hw/acpi/pcihp.h
index 6230e60..aa16bff 100644
--- a/include/hw/acpi/pcihp.h
+++ b/include/hw/acpi/pcihp.h
@@ -53,7 +53,7 @@  void acpi_pcihp_init(AcpiPciHpState *, PCIBus *root,
 
 /* Invoke on device hotplug */
 int acpi_pcihp_device_hotplug(AcpiPciHpState *, PCIDevice *,
-                              PCIHotplugState state);
+                              HotplugState state);
 
 /* Called on reset */
 void acpi_pcihp_reset(AcpiPciHpState *s);
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index 81fdddc..0eb89be 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -330,15 +330,6 @@  typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level);
 typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num);
 typedef PCIINTxRoute (*pci_route_irq_fn)(void *opaque, int pin);
 
-typedef enum {
-    PCI_HOTPLUG_DISABLED,
-    PCI_HOTPLUG_ENABLED,
-    PCI_COLDPLUG_ENABLED,
-} PCIHotplugState;
-
-typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
-                              PCIHotplugState state);
-
 #define TYPE_PCI_BUS "PCI"
 #define PCI_BUS(obj) OBJECT_CHECK(PCIBus, (obj), TYPE_PCI_BUS)
 #define TYPE_PCIE_BUS "PCIE"
@@ -357,7 +348,7 @@  PCIBus *pci_bus_new(DeviceState *parent, const char *name,
 void pci_bus_irqs(PCIBus *bus, pci_set_irq_fn set_irq, pci_map_irq_fn map_irq,
                   void *irq_opaque, int nirq);
 int pci_bus_get_irq_level(PCIBus *bus, int irq_num);
-void pci_bus_hotplug(PCIBus *bus, pci_hotplug_fn hotplug, DeviceState *dev);
+void pci_bus_hotplug(PCIBus *bus, hotplug_fn hotplug, DeviceState *dev);
 /* 0 <= pin <= 3 0 = INTA, 1 = INTB, 2 = INTC, 3 = INTD */
 int pci_swizzle_map_irq_fn(PCIDevice *pci_dev, int pin);
 PCIBus *pci_register_bus(DeviceState *parent, const char *name,
diff --git a/include/hw/pci/pci_bus.h b/include/hw/pci/pci_bus.h
index 9df1788..ac5eb6a 100644
--- a/include/hw/pci/pci_bus.h
+++ b/include/hw/pci/pci_bus.h
@@ -16,7 +16,7 @@  struct PCIBus {
     pci_set_irq_fn set_irq;
     pci_map_irq_fn map_irq;
     pci_route_irq_fn route_intx_to_irq;
-    pci_hotplug_fn hotplug;
+    hotplug_fn hotplug;
     DeviceState *hotplug_qdev;
     void *irq_opaque;
     PCIDevice *devices[PCI_SLOT_MAX * PCI_FUNC_MAX];
diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index f2043a6..1eb2028 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -307,4 +307,13 @@  extern int qdev_hotplug;
 
 char *qdev_get_dev_path(DeviceState *dev);
 
+typedef enum {
+    HOTPLUG_DISABLED,
+    HOTPLUG_ENABLED,
+    COLDPLUG_ENABLED,
+} HotplugState;
+
+typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev,
+                          HotplugState state);
+
 #endif