Patchwork pci: allow hotplug removal of cold-plugged devices

login
register
mail settings
Submitter Michael S. Tsirkin
Date Nov. 14, 2010, 2:18 p.m.
Message ID <20101114141804.GA15504@redhat.com>
Download mbox | patch
Permalink /patch/71116/
State New
Headers show

Comments

Michael S. Tsirkin - Nov. 14, 2010, 2:18 p.m.
This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
which broke hotplug removal of cold plugged devices:

- pass addition/removal state to hotplug callbacks
- use that in piix and pcie

This also fixes an assert on hotplug removal of coldplugged
express devices.

Reported-by: by Cam Macdonell <cam@cs.ualberta.ca>.
Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---

So I think the below would be the cleanest way
to fix the bug as we keep the hot/cold plug logic
in a central palce. Untested. Comments? Cam?
Isaku Yamahata - Nov. 15, 2010, 2:10 a.m.
Looks good. Thank you for catching pcie case.

On Sun, Nov 14, 2010 at 04:18:04PM +0200, Michael S. Tsirkin wrote:
> This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> which broke hotplug removal of cold plugged devices:
> 
> - pass addition/removal state to hotplug callbacks
> - use that in piix and pcie
> 
> This also fixes an assert on hotplug removal of coldplugged
> express devices.
> 
> Reported-by: by Cam Macdonell <cam@cs.ualberta.ca>.
              ^^^ minor nitpick

> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> 
> So I think the below would be the cleanest way
> to fix the bug as we keep the hot/cold plug logic
> in a central palce. Untested. Comments? Cam?
> 
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 66c7885..f549089 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -585,7 +585,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>      PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
>  }
>  
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state);
> +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> +                                PCIHotplugState state);
>  
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  {
> @@ -615,18 +616,23 @@ static void disable_device(PIIX4PMState *s, int slot)
>      s->pci0_status.down |= (1 << slot);
>  }
>  
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
> +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> +				PCIHotplugState state)
>  {
>      int slot = PCI_SLOT(dev->devfn);
>      PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
>                                  DO_UPCAST(PCIDevice, qdev, qdev));
>  
> -    if (!dev->qdev.hotplugged)
> +    /* 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) {
>          return 0;
> +    }
>  
>      s->pci0_status.up = 0;
>      s->pci0_status.down = 0;
> -    if (state) {
> +    if (state == PCI_HOTPLUG_ENABLED) {
>          enable_device(s, slot);
>      } else {
>          disable_device(s, slot);
> diff --git a/hw/pci.c b/hw/pci.c
> index 30e1603..316b24f 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1566,8 +1566,11 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>      pci_add_option_rom(pci_dev);
>  
>      if (bus->hotplug) {
> -        /* lower layer must check qdev->hotplugged */
> -        rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1);
> +        /* 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);
>          if (rc != 0) {
>              int r = pci_unregister_device(&pci_dev->qdev);
>              assert(!r);
> @@ -1581,7 +1584,8 @@ static int pci_unplug_device(DeviceState *qdev)
>  {
>      PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>  
> -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0);
> +    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> +                             PCI_HOTPLUG_DISABLED);
>  }
>  
>  void pci_qdev_register(PCIDeviceInfo *info)
> diff --git a/hw/pci.h b/hw/pci.h
> index 7100804..09b3e4c 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -214,7 +214,15 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
>  
>  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 int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int state);
> +
> +typedef enum {
> +    PCI_HOTPLUG_DISABLED,
> +    PCI_HOTPLUG_ENABLED,
> +    PCI_COLDPLUG_ENABLED,
> +} PCIHotplugState;
> +
> +typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> +                              PCIHotplugState state);
>  void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>                           const char *name, int devfn_min);
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 35918f7..4df48b8 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -192,14 +192,16 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
>  }
>  
>  static int pcie_cap_slot_hotplug(DeviceState *qdev,
> -                                 PCIDevice *pci_dev, int state)
> +                                 PCIDevice *pci_dev, PCIHotplugState state)
>  {
>      PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
>      uint8_t *exp_cap = d->config + d->exp.exp_cap;
>      uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
>  
> -    if (!pci_dev->qdev.hotplugged) {
> -        assert(state); /* this case only happens at machine creation. */
> +    /* 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) {
>          pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                     PCI_EXP_SLTSTA_PDS);
>          return 0;
>
Cam Macdonell - Nov. 15, 2010, 5:14 p.m.
On Sun, Nov 14, 2010 at 7:18 AM, Michael S. Tsirkin <mst@redhat.com> wrote:
> This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1
> which broke hotplug removal of cold plugged devices:
>
> - pass addition/removal state to hotplug callbacks
> - use that in piix and pcie
>
> This also fixes an assert on hotplug removal of coldplugged
> express devices.
>
> Reported-by: by Cam Macdonell <cam@cs.ualberta.ca>.
> Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Acked-by: Cam Macdonell <cam@cs.ualberta.ca>

> ---
>
> So I think the below would be the cleanest way
> to fix the bug as we keep the hot/cold plug logic
> in a central palce. Untested. Comments? Cam?

Yes, it seems to fix the problem.

>
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 66c7885..f549089 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -585,7 +585,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>     PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
>  }
>
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state);
> +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> +                                PCIHotplugState state);
>
>  static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
>  {
> @@ -615,18 +616,23 @@ static void disable_device(PIIX4PMState *s, int slot)
>     s->pci0_status.down |= (1 << slot);
>  }
>
> -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
> +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> +                               PCIHotplugState state)
>  {
>     int slot = PCI_SLOT(dev->devfn);
>     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
>                                 DO_UPCAST(PCIDevice, qdev, qdev));
>
> -    if (!dev->qdev.hotplugged)
> +    /* 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) {
>         return 0;
> +    }
>
>     s->pci0_status.up = 0;
>     s->pci0_status.down = 0;
> -    if (state) {
> +    if (state == PCI_HOTPLUG_ENABLED) {
>         enable_device(s, slot);
>     } else {
>         disable_device(s, slot);
> diff --git a/hw/pci.c b/hw/pci.c
> index 30e1603..316b24f 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1566,8 +1566,11 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
>     pci_add_option_rom(pci_dev);
>
>     if (bus->hotplug) {
> -        /* lower layer must check qdev->hotplugged */
> -        rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1);
> +        /* 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);
>         if (rc != 0) {
>             int r = pci_unregister_device(&pci_dev->qdev);
>             assert(!r);
> @@ -1581,7 +1584,8 @@ static int pci_unplug_device(DeviceState *qdev)
>  {
>     PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
>
> -    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0);
> +    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
> +                             PCI_HOTPLUG_DISABLED);
>  }
>
>  void pci_qdev_register(PCIDeviceInfo *info)
> diff --git a/hw/pci.h b/hw/pci.h
> index 7100804..09b3e4c 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -214,7 +214,15 @@ int pci_device_load(PCIDevice *s, QEMUFile *f);
>
>  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 int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int state);
> +
> +typedef enum {
> +    PCI_HOTPLUG_DISABLED,
> +    PCI_HOTPLUG_ENABLED,
> +    PCI_COLDPLUG_ENABLED,
> +} PCIHotplugState;
> +
> +typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
> +                              PCIHotplugState state);
>  void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
>                          const char *name, int devfn_min);
>  PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 35918f7..4df48b8 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -192,14 +192,16 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
>  }
>
>  static int pcie_cap_slot_hotplug(DeviceState *qdev,
> -                                 PCIDevice *pci_dev, int state)
> +                                 PCIDevice *pci_dev, PCIHotplugState state)
>  {
>     PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
>     uint8_t *exp_cap = d->config + d->exp.exp_cap;
>     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
>
> -    if (!pci_dev->qdev.hotplugged) {
> -        assert(state); /* this case only happens at machine creation. */
> +    /* 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) {
>         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                    PCI_EXP_SLTSTA_PDS);
>         return 0;
>
>

Patch

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 66c7885..f549089 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -585,7 +585,8 @@  static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
     PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
 }
 
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state);
+static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
+                                PCIHotplugState state);
 
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
@@ -615,18 +616,23 @@  static void disable_device(PIIX4PMState *s, int slot)
     s->pci0_status.down |= (1 << slot);
 }
 
-static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state)
+static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
+				PCIHotplugState state)
 {
     int slot = PCI_SLOT(dev->devfn);
     PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev,
                                 DO_UPCAST(PCIDevice, qdev, qdev));
 
-    if (!dev->qdev.hotplugged)
+    /* 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) {
         return 0;
+    }
 
     s->pci0_status.up = 0;
     s->pci0_status.down = 0;
-    if (state) {
+    if (state == PCI_HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
         disable_device(s, slot);
diff --git a/hw/pci.c b/hw/pci.c
index 30e1603..316b24f 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1566,8 +1566,11 @@  static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base)
     pci_add_option_rom(pci_dev);
 
     if (bus->hotplug) {
-        /* lower layer must check qdev->hotplugged */
-        rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1);
+        /* 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);
         if (rc != 0) {
             int r = pci_unregister_device(&pci_dev->qdev);
             assert(!r);
@@ -1581,7 +1584,8 @@  static int pci_unplug_device(DeviceState *qdev)
 {
     PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev);
 
-    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0);
+    return dev->bus->hotplug(dev->bus->hotplug_qdev, dev,
+                             PCI_HOTPLUG_DISABLED);
 }
 
 void pci_qdev_register(PCIDeviceInfo *info)
diff --git a/hw/pci.h b/hw/pci.h
index 7100804..09b3e4c 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -214,7 +214,15 @@  int pci_device_load(PCIDevice *s, QEMUFile *f);
 
 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 int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int state);
+
+typedef enum {
+    PCI_HOTPLUG_DISABLED,
+    PCI_HOTPLUG_ENABLED,
+    PCI_COLDPLUG_ENABLED,
+} PCIHotplugState;
+
+typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev,
+                              PCIHotplugState state);
 void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent,
                          const char *name, int devfn_min);
 PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min);
diff --git a/hw/pcie.c b/hw/pcie.c
index 35918f7..4df48b8 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -192,14 +192,16 @@  static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event)
 }
 
 static int pcie_cap_slot_hotplug(DeviceState *qdev,
-                                 PCIDevice *pci_dev, int state)
+                                 PCIDevice *pci_dev, PCIHotplugState state)
 {
     PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev);
     uint8_t *exp_cap = d->config + d->exp.exp_cap;
     uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA);
 
-    if (!pci_dev->qdev.hotplugged) {
-        assert(state); /* this case only happens at machine creation. */
+    /* 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) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                    PCI_EXP_SLTSTA_PDS);
         return 0;