* [Qemu-devel] [PATCH RFC] piix: fix up/down races
@ 2012-03-27 17:59 Michael S. Tsirkin
2012-03-28 15:32 ` Michael S. Tsirkin
2012-03-29 7:32 ` Gleb Natapov
0 siblings, 2 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2012-03-27 17:59 UTC (permalink / raw)
To: qemu-devel
Cc: Isaku Yamahata, Anthony Liguori, Gerd Hoffmann, Aurelien Jarno, gleb
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slot
if we do this before guest reads the down register,
the down event is discarded and device will never
be deleted.
2.
- delete device a
- quickly reset before guest can respond
interrupt is reset and guest will never eject the device.
To fix this, we implement two changes:
1. Add two new registers:
CLR_UP
CLR_DOWN
bios will now write to these the value it read from UP/DOWN
2. on reset, remove all devices which have DOWN bit set
For compatibility with old guests, we also clear
the DOWN bit on write to EJ0 for a device.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
Warning: untested.
Posting for early feedback/flames.
---
hw/acpi_piix4.c | 72 ++++++++++++++++++++++++++++++++++++++++++++----------
1 files changed, 58 insertions(+), 14 deletions(-)
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 797ed24..a155358 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -43,6 +43,8 @@
#define PCI_BASE 0xae00
#define PCI_EJ_BASE 0xae08
#define PCI_RMV_BASE 0xae0c
+#define PCI_CLR_UP_BASE 0xae10
+#define PCI_CLR_DOWN_BASE 0xae14
#define PIIX4_PCI_HOTPLUG_STATUS 2
@@ -287,6 +289,25 @@ static void piix4_update_hotplug(PIIX4PMState *s)
}
}
+static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
+{
+ DeviceState *qdev, *next;
+ BusState *bus = qdev_get_parent_bus(&s->dev.qdev);
+ int slot = ffs(slots) - 1;
+
+ /* Clear down register here too - this is good for
+ * compatibility with old guests which do not have CLR_DOWN. */
+ s->pci0_status.down &= ~(1U << slot);
+
+ QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
+ PCIDevice *dev = PCI_DEVICE(qdev);
+ PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
+ if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
+ qdev_free(qdev);
+ }
+ }
+}
+
static void piix4_reset(void *opaque)
{
PIIX4PMState *s = opaque;
@@ -302,6 +323,19 @@ static void piix4_reset(void *opaque)
pci_conf[0x5B] = 0x02;
}
piix4_update_hotplug(s);
+ /*
+ * Guest lost remove events if any.
+ * As it's being reset it's safe to remove the device now.
+ */
+ while (s->pci0_status.down) {
+ acpi_piix_eject_slot(s, s->pci0_status.down);
+ }
+ /*
+ * Guest lost add events if any.
+ * As it's being reset and will rescan the bus we cann discard
+ * past events now.
+ */
+ s->pci0_status.up = 0;
}
static void piix4_powerdown(void *opaque, int irq, int power_failing)
@@ -490,22 +524,31 @@ static uint32_t pciej_read(void *opaque, uint32_t addr)
static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
{
- BusState *bus = opaque;
- DeviceState *qdev, *next;
- int slot = ffs(val) - 1;
+ PIIX4PMState *s = opaque;
- QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
- PCIDevice *dev = PCI_DEVICE(qdev);
- PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
- if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
- qdev_free(qdev);
- }
+ if (val) {
+ acpi_piix_eject_slot(s, val);
}
-
PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
}
+static void pci_clr_up_write(void *opaque, uint32_t addr, uint32_t val)
+{
+ PIIX4PMState *s = opaque;
+ s->pci0_status.up &= ~val;
+
+ PIIX4_DPRINTF("pci_clr_up write %x <== %d\n", addr, val);
+}
+
+static void pci_clr_down_write(void *opaque, uint32_t addr, uint32_t val)
+{
+ PIIX4PMState *s = opaque;
+ s->pci0_status.down &= ~val;
+
+ PIIX4_DPRINTF("pci_clr_down write %x <== %d\n", addr, val);
+}
+
static uint32_t pcirmv_read(void *opaque, uint32_t addr)
{
PIIX4PMState *s = opaque;
@@ -532,12 +575,15 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
register_ioport_read(PCI_BASE, 8, 4, pcihotplug_read, pci0_status);
- register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
- register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus);
+ register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
+ register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, s);
register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s);
+ register_ioport_write(PCI_CLR_UP_BASE, 4, 4, pci_clr_up_write, s);
+ register_ioport_write(PCI_CLR_DOWN_BASE, 4, 4, pci_clr_down_write, s);
+
pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
}
@@ -567,8 +613,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
return 0;
}
- s->pci0_status.up = 0;
- s->pci0_status.down = 0;
if (state == PCI_HOTPLUG_ENABLED) {
enable_device(s, slot);
} else {
--
1.7.9.111.gf3fb0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] piix: fix up/down races
2012-03-27 17:59 [Qemu-devel] [PATCH RFC] piix: fix up/down races Michael S. Tsirkin
@ 2012-03-28 15:32 ` Michael S. Tsirkin
2012-03-29 7:32 ` Gleb Natapov
1 sibling, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2012-03-28 15:32 UTC (permalink / raw)
To: qemu-devel
Cc: Isaku Yamahata, Anthony Liguori, Gerd Hoffmann, Aurelien Jarno, gleb
On Tue, Mar 27, 2012 at 07:59:07PM +0200, Michael S. Tsirkin wrote:
> piix acpi interface suffers from the following 2 issues:
>
> 1.
> - delete device a
> - quickly add device b in another slot
>
> if we do this before guest reads the down register,
> the down event is discarded and device will never
> be deleted.
>
> 2.
> - delete device a
> - quickly reset before guest can respond
>
> interrupt is reset and guest will never eject the device.
>
> To fix this, we implement two changes:
>
> 1. Add two new registers:
> CLR_UP
> CLR_DOWN
> bios will now write to these the value it read from UP/DOWN
>
> 2. on reset, remove all devices which have DOWN bit set
>
> For compatibility with old guests, we also clear
> the DOWN bit on write to EJ0 for a device.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Warning: untested.
> Posting for early feedback/flames.
OK, I've now tested this with a windows and linux guests.
seabios patch posted on the seabios mailing list.
I'll apply this if no one objects ...
> ---
> hw/acpi_piix4.c | 72 ++++++++++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 797ed24..a155358 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -43,6 +43,8 @@
> #define PCI_BASE 0xae00
> #define PCI_EJ_BASE 0xae08
> #define PCI_RMV_BASE 0xae0c
> +#define PCI_CLR_UP_BASE 0xae10
> +#define PCI_CLR_DOWN_BASE 0xae14
>
> #define PIIX4_PCI_HOTPLUG_STATUS 2
>
> @@ -287,6 +289,25 @@ static void piix4_update_hotplug(PIIX4PMState *s)
> }
> }
>
> +static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
> +{
> + DeviceState *qdev, *next;
> + BusState *bus = qdev_get_parent_bus(&s->dev.qdev);
> + int slot = ffs(slots) - 1;
> +
> + /* Clear down register here too - this is good for
> + * compatibility with old guests which do not have CLR_DOWN. */
> + s->pci0_status.down &= ~(1U << slot);
> +
> + QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
> + PCIDevice *dev = PCI_DEVICE(qdev);
> + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> + if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
> + qdev_free(qdev);
> + }
> + }
> +}
> +
> static void piix4_reset(void *opaque)
> {
> PIIX4PMState *s = opaque;
> @@ -302,6 +323,19 @@ static void piix4_reset(void *opaque)
> pci_conf[0x5B] = 0x02;
> }
> piix4_update_hotplug(s);
> + /*
> + * Guest lost remove events if any.
> + * As it's being reset it's safe to remove the device now.
> + */
> + while (s->pci0_status.down) {
> + acpi_piix_eject_slot(s, s->pci0_status.down);
> + }
> + /*
> + * Guest lost add events if any.
> + * As it's being reset and will rescan the bus we cann discard
> + * past events now.
> + */
> + s->pci0_status.up = 0;
> }
>
> static void piix4_powerdown(void *opaque, int irq, int power_failing)
> @@ -490,22 +524,31 @@ static uint32_t pciej_read(void *opaque, uint32_t addr)
>
> static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
> {
> - BusState *bus = opaque;
> - DeviceState *qdev, *next;
> - int slot = ffs(val) - 1;
> + PIIX4PMState *s = opaque;
>
> - QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
> - PCIDevice *dev = PCI_DEVICE(qdev);
> - PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> - if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
> - qdev_free(qdev);
> - }
> + if (val) {
> + acpi_piix_eject_slot(s, val);
> }
>
> -
> PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
> }
>
> +static void pci_clr_up_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> + PIIX4PMState *s = opaque;
> + s->pci0_status.up &= ~val;
> +
> + PIIX4_DPRINTF("pci_clr_up write %x <== %d\n", addr, val);
> +}
> +
> +static void pci_clr_down_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> + PIIX4PMState *s = opaque;
> + s->pci0_status.down &= ~val;
> +
> + PIIX4_DPRINTF("pci_clr_down write %x <== %d\n", addr, val);
> +}
> +
> static uint32_t pcirmv_read(void *opaque, uint32_t addr)
> {
> PIIX4PMState *s = opaque;
> @@ -532,12 +575,15 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
> register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
> register_ioport_read(PCI_BASE, 8, 4, pcihotplug_read, pci0_status);
>
> - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
> - register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus);
> + register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
> + register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, s);
>
> register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
> register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s);
>
> + register_ioport_write(PCI_CLR_UP_BASE, 4, 4, pci_clr_up_write, s);
> + register_ioport_write(PCI_CLR_DOWN_BASE, 4, 4, pci_clr_down_write, s);
> +
> pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> }
>
> @@ -567,8 +613,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> return 0;
> }
>
> - s->pci0_status.up = 0;
> - s->pci0_status.down = 0;
> if (state == PCI_HOTPLUG_ENABLED) {
> enable_device(s, slot);
> } else {
> --
> 1.7.9.111.gf3fb0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH RFC] piix: fix up/down races
2012-03-27 17:59 [Qemu-devel] [PATCH RFC] piix: fix up/down races Michael S. Tsirkin
2012-03-28 15:32 ` Michael S. Tsirkin
@ 2012-03-29 7:32 ` Gleb Natapov
1 sibling, 0 replies; 3+ messages in thread
From: Gleb Natapov @ 2012-03-29 7:32 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Isaku Yamahata, Anthony Liguori, qemu-devel, Aurelien Jarno,
Gerd Hoffmann
On Tue, Mar 27, 2012 at 07:59:08PM +0200, Michael S. Tsirkin wrote:
> piix acpi interface suffers from the following 2 issues:
>
> 1.
> - delete device a
> - quickly add device b in another slot
>
> if we do this before guest reads the down register,
> the down event is discarded and device will never
> be deleted.
>
> 2.
> - delete device a
> - quickly reset before guest can respond
>
> interrupt is reset and guest will never eject the device.
>
> To fix this, we implement two changes:
>
> 1. Add two new registers:
> CLR_UP
> CLR_DOWN
> bios will now write to these the value it read from UP/DOWN
>
> 2. on reset, remove all devices which have DOWN bit set
>
> For compatibility with old guests, we also clear
> the DOWN bit on write to EJ0 for a device.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Warning: untested.
> Posting for early feedback/flames.
>
> ---
> hw/acpi_piix4.c | 72 ++++++++++++++++++++++++++++++++++++++++++++----------
docs/specs/acpi_pci_hotplug.txt +++++++
> 1 files changed, 58 insertions(+), 14 deletions(-)
>
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 797ed24..a155358 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -43,6 +43,8 @@
> #define PCI_BASE 0xae00
> #define PCI_EJ_BASE 0xae08
> #define PCI_RMV_BASE 0xae0c
> +#define PCI_CLR_UP_BASE 0xae10
> +#define PCI_CLR_DOWN_BASE 0xae14
>
> #define PIIX4_PCI_HOTPLUG_STATUS 2
>
> @@ -287,6 +289,25 @@ static void piix4_update_hotplug(PIIX4PMState *s)
> }
> }
>
> +static void acpi_piix_eject_slot(PIIX4PMState *s, unsigned slots)
> +{
> + DeviceState *qdev, *next;
> + BusState *bus = qdev_get_parent_bus(&s->dev.qdev);
> + int slot = ffs(slots) - 1;
> +
Funny how AML code makes bitmap from a slot number and here we do the
opposite. I wish we could fix it.
> + /* Clear down register here too - this is good for
> + * compatibility with old guests which do not have CLR_DOWN. */
> + s->pci0_status.down &= ~(1U << slot);
Strictly speaking the comment is incorrect since reset handler below
relies on this too.
> +
> + QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
> + PCIDevice *dev = PCI_DEVICE(qdev);
> + PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> + if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
> + qdev_free(qdev);
> + }
> + }
> +}
> +
> static void piix4_reset(void *opaque)
> {
> PIIX4PMState *s = opaque;
> @@ -302,6 +323,19 @@ static void piix4_reset(void *opaque)
> pci_conf[0x5B] = 0x02;
> }
> piix4_update_hotplug(s);
> + /*
> + * Guest lost remove events if any.
> + * As it's being reset it's safe to remove the device now.
> + */
> + while (s->pci0_status.down) {
> + acpi_piix_eject_slot(s, s->pci0_status.down);
> + }
> + /*
> + * Guest lost add events if any.
> + * As it's being reset and will rescan the bus we cann discard
> + * past events now.
> + */
> + s->pci0_status.up = 0;
> }
>
> static void piix4_powerdown(void *opaque, int irq, int power_failing)
> @@ -490,22 +524,31 @@ static uint32_t pciej_read(void *opaque, uint32_t addr)
>
> static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
> {
> - BusState *bus = opaque;
> - DeviceState *qdev, *next;
> - int slot = ffs(val) - 1;
> + PIIX4PMState *s = opaque;
>
> - QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
> - PCIDevice *dev = PCI_DEVICE(qdev);
> - PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
> - if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
> - qdev_free(qdev);
> - }
> + if (val) {
> + acpi_piix_eject_slot(s, val);
> }
>
> -
> PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val);
> }
>
> +static void pci_clr_up_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> + PIIX4PMState *s = opaque;
> + s->pci0_status.up &= ~val;
> +
> + PIIX4_DPRINTF("pci_clr_up write %x <== %d\n", addr, val);
> +}
> +
> +static void pci_clr_down_write(void *opaque, uint32_t addr, uint32_t val)
> +{
> + PIIX4PMState *s = opaque;
> + s->pci0_status.down &= ~val;
> +
> + PIIX4_DPRINTF("pci_clr_down write %x <== %d\n", addr, val);
> +}
> +
> static uint32_t pcirmv_read(void *opaque, uint32_t addr)
> {
> PIIX4PMState *s = opaque;
> @@ -532,12 +575,15 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
> register_ioport_write(PCI_BASE, 8, 4, pcihotplug_write, pci0_status);
> register_ioport_read(PCI_BASE, 8, 4, pcihotplug_read, pci0_status);
>
> - register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, bus);
> - register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, bus);
> + register_ioport_write(PCI_EJ_BASE, 4, 4, pciej_write, s);
> + register_ioport_read(PCI_EJ_BASE, 4, 4, pciej_read, s);
>
> register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
> register_ioport_read(PCI_RMV_BASE, 4, 4, pcirmv_read, s);
>
> + register_ioport_write(PCI_CLR_UP_BASE, 4, 4, pci_clr_up_write, s);
> + register_ioport_write(PCI_CLR_DOWN_BASE, 4, 4, pci_clr_down_write, s);
> +
> pci_bus_hotplug(bus, piix4_device_hotplug, &s->dev.qdev);
> }
>
> @@ -567,8 +613,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> return 0;
> }
>
> - s->pci0_status.up = 0;
> - s->pci0_status.down = 0;
> if (state == PCI_HOTPLUG_ENABLED) {
> enable_device(s, slot);
> } else {
> --
> 1.7.9.111.gf3fb0
--
Gleb.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-03-29 7:34 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-03-27 17:59 [Qemu-devel] [PATCH RFC] piix: fix up/down races Michael S. Tsirkin
2012-03-28 15:32 ` Michael S. Tsirkin
2012-03-29 7:32 ` Gleb Natapov
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.