* [Qemu-devel] [PATCH] pci: allow hotplug removal of cold-plugged devices
@ 2010-11-14 14:18 Michael S. Tsirkin
2010-11-15 2:10 ` [Qemu-devel] " Isaku Yamahata
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2010-11-14 14:18 UTC (permalink / raw)
To: yamahata, cam; +Cc: qemu-devel
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?
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;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices
2010-11-14 14:18 [Qemu-devel] [PATCH] pci: allow hotplug removal of cold-plugged devices Michael S. Tsirkin
@ 2010-11-15 2:10 ` Isaku Yamahata
2010-11-15 2:23 ` Isaku Yamahata
2010-11-15 17:14 ` Cam Macdonell
2 siblings, 0 replies; 5+ messages in thread
From: Isaku Yamahata @ 2010-11-15 2:10 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: cam, qemu-devel
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;
>
--
yamahata
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices
2010-11-14 14:18 [Qemu-devel] [PATCH] pci: allow hotplug removal of cold-plugged devices Michael S. Tsirkin
2010-11-15 2:10 ` [Qemu-devel] " Isaku Yamahata
@ 2010-11-15 2:23 ` Isaku Yamahata
2010-11-16 12:49 ` Michael S. Tsirkin
2010-11-15 17:14 ` Cam Macdonell
2 siblings, 1 reply; 5+ messages in thread
From: Isaku Yamahata @ 2010-11-15 2:23 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: cam, qemu-devel
Thank you for catching pcie part.
The following fix is necessary.
diff --git a/hw/pcie.c b/hw/pcie.c
index 4df48b8..f461c1c 100644
--- a/hw/pcie.c
+++ b/hw/pcie.c
@@ -221,7 +221,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
*/
assert(PCI_FUNC(pci_dev->devfn) == 0);
- if (state) {
+ if (state == PCI_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);
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>.
> 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;
>
--
yamahata
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices
2010-11-14 14:18 [Qemu-devel] [PATCH] pci: allow hotplug removal of cold-plugged devices Michael S. Tsirkin
2010-11-15 2:10 ` [Qemu-devel] " Isaku Yamahata
2010-11-15 2:23 ` Isaku Yamahata
@ 2010-11-15 17:14 ` Cam Macdonell
2 siblings, 0 replies; 5+ messages in thread
From: Cam Macdonell @ 2010-11-15 17:14 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: yamahata, qemu-devel
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;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices
2010-11-15 2:23 ` Isaku Yamahata
@ 2010-11-16 12:49 ` Michael S. Tsirkin
0 siblings, 0 replies; 5+ messages in thread
From: Michael S. Tsirkin @ 2010-11-16 12:49 UTC (permalink / raw)
To: Isaku Yamahata; +Cc: cam, qemu-devel
On Mon, Nov 15, 2010 at 11:23:44AM +0900, Isaku Yamahata wrote:
> Thank you for catching pcie part.
> The following fix is necessary.
>
> diff --git a/hw/pcie.c b/hw/pcie.c
> index 4df48b8..f461c1c 100644
> --- a/hw/pcie.c
> +++ b/hw/pcie.c
> @@ -221,7 +221,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev,
> */
> assert(PCI_FUNC(pci_dev->devfn) == 0);
>
> - if (state) {
> + if (state == PCI_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);
>
Okay. Although note that the enum values were carefully
selected to make the old code work as well.
> 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>.
> > 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;
> >
>
> --
> yamahata
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-11-16 12:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-11-14 14:18 [Qemu-devel] [PATCH] pci: allow hotplug removal of cold-plugged devices Michael S. Tsirkin
2010-11-15 2:10 ` [Qemu-devel] " Isaku Yamahata
2010-11-15 2:23 ` Isaku Yamahata
2010-11-16 12:49 ` Michael S. Tsirkin
2010-11-15 17:14 ` Cam Macdonell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.