piix: fix up/down races + document

PATCH Superseded Michael S. Tsirkin 4 participants 14 messages Mar 29, 2012 – Apr 1, 2012

lore

Share & tools

lore

Michael S. Tsirkin

author

2012-03-29 12:51 UTC #0 lore raw

PATCH v2 1/1 piix: fix up/down races + document

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.

This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.

Signed-off-by Michael S. Tsirkin <[email protected]>

---

Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update

 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)

docs/specs/acpi_pci_hotplug.txt +60 -8

index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

121 lines not on this page · raw message

Signature · 1 line

1.7.9.111.gf3fb0

Alex Williamson

2012-03-29 16:54 UTC #3 lore raw

In reply to Michael S. Tsirkin

Quoted 56 lines · Michael S. Tsirkin

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.
This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update
 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)
docs/specs/acpi_pci_hotplug.txt +1 -2
index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):

docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 21

Quoted 10 lines

docs/specs/acpi_pci_hotplug.txt +8 -1

+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.

Sorry if I'm behind on the discussion, but why do we need to define 0xae0c & 0xae10 below for write-only access when these registers (0xae00 & 0xae04) are effectively read-only (yes, write is there, but it should *never* be used in the current implementation)? Thanks,

Alex

Quoted 203 lines

docs/specs/acpi_pci_hotplug.txt +51 -5

 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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 +578,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 +616,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 {

Michael S. Tsirkin

author

2012-03-29 20:08 UTC #4 lore raw

Quoted 73 lines · Alex Williamson

Quoted 56 lines · Michael S. Tsirkin
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.
This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update
 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)
docs/specs/acpi_pci_hotplug.txt +1 -2
index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 21
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +8 -1
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
Sorry if I'm behind on the discussion, but why do we need to define 0xae0c & 0xae10 below for write-only access when these registers (0xae00 & 0xae04) are effectively read-only (yes, write is there, but it should *never* be used in the current implementation)? Thanks,

Quoted 1 line

Alex

Because otherwise a new bios will be broken on an old qemu. We should have made the registers read-only in the old qemu then we won't have the problem now.

Yes, it's probably not a big deal, but it's just as easy to create a new register as it is to redefine the old one.

Quoted 205 lines

Quoted 203 lines

docs/specs/acpi_pci_hotplug.txt +51 -5

 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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 +578,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 +616,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 {

Alex Williamson

2012-03-29 19:52 UTC #5 lore raw

Quoted 74 lines · Michael S. Tsirkin

Quoted 73 lines · Alex Williamson
Quoted 56 lines · Michael S. Tsirkin
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.
This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update
 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)
docs/specs/acpi_pci_hotplug.txt +1 -2
index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 21
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +8 -1
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
Sorry if I'm behind on the discussion, but why do we need to define 0xae0c & 0xae10 below for write-only access when these registers (0xae00 & 0xae04) are effectively read-only (yes, write is there, but it should *never* be used in the current implementation)? Thanks,

Quoted 5 lines

Quoted 1 line
Alex
Because otherwise a new bios will be broken on an old qemu. We should have made the registers read-only in the old qemu then we won't have the problem now.

If you think there might be rouge bioses out there that ever wrote to these registers, but I doubt that's the case.

Quoted 2 lines

Yes, it's probably not a big deal, but it's just as easy to create a new register as it is to redefine the old one.

It could just as easily be called a spec clarification that writes were never intended previously. It's easy to add two more registers... oh wait, we only have 16bits of ioport space... Thanks,

Alex

Quoted 205 lines

Quoted 203 lines

docs/specs/acpi_pci_hotplug.txt +51 -5

 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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 +578,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 +616,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 {

Michael S. Tsirkin

author

2012-03-30 12:19 UTC #6 lore raw

Quoted 75 lines · Alex Williamson

Quoted 74 lines · Michael S. Tsirkin
Quoted 73 lines · Alex Williamson
Quoted 56 lines · Michael S. Tsirkin
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.
This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.
Signed-off-by Michael S. Tsirkin <[email protected]>
---
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update
 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)
docs/specs/acpi_pci_hotplug.txt +1 -2
index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 21
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +8 -1
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
Sorry if I'm behind on the discussion, but why do we need to define 0xae0c & 0xae10 below for write-only access when these registers (0xae00 & 0xae04) are effectively read-only (yes, write is there, but it should *never* be used in the current implementation)? Thanks,

Quoted 8 lines

Quoted 5 lines
Quoted 1 line
Alex
Because otherwise a new bios will be broken on an old qemu. We should have made the registers read-only in the old qemu then we won't have the problem now.
If you think there might be rouge bioses out there that ever wrote to these registers, but I doubt that's the case.

As I understand it, the issue is not rouge bioses,
it's the old qemus that make these writeable, and we want the new bios to work there. Therefore new bios should avoid writing these registers.

Quoted 5 lines

Quoted 2 lines
Yes, it's probably not a big deal, but it's just as easy to create a new register as it is to redefine the old one.
It could just as easily be called a spec clarification that writes were never intended previously.

As I see it, the issue is with old implementations, not the spec.

Quoted 4 lines

It's easy to add two more registers... oh
wait, we only have 16bits of ioport space... Thanks,
Alex

8 bytes is still cheap ...

Quoted 207 lines

Quoted 205 lines

Quoted 203 lines

docs/specs/acpi_pci_hotplug.txt +51 -5

 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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 +578,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 +616,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 {

Igor Mammedov

2012-03-30 16:05 UTC #7 lore raw

In reply to Michael S. Tsirkin

Quoted 230 lines · Michael S. Tsirkin

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.

This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.

Signed-off-by Michael S. Tsirkin<[email protected]>

---

Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update

  docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
  hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
  2 files changed, 121 insertions(+), 22 deletions(-)

docs/specs/acpi_pci_hotplug.txt +60 -8

index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
  ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
  -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
  event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
  ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
  Read by ACPI BIOS GPE.1 handler to notify OS of injection
  events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
  -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
  Read by ACPI BIOS GPE.1 handler to notify OS of removal
  events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
  PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
  ----------------------------------------
 
  Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
  PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
  -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +46 ~~-10~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
  }
 
  static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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);

hw/acpi_piix4.c static uint32_t pciej_read(void *opaque, uint32_t addr) · line 554

Quoted 10 lines

hw/acpi_piix4.c +9 -0

+}
+
+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);
+}

Why adding PCI_CLR_[UP|DOWN]_BASE instead of using PCI_BASE for this writes?

Quoted 22 lines

hw/acpi_piix4.c +5 -2

  static uint32_t pcirmv_read(void *opaque, uint32_t addr)
  {
      PIIX4PMState *s = opaque;

@@ -532,12 +578,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);
  }

hw/acpi_piix4.c static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, · line 621

Quoted 8 lines

hw/acpi_piix4.c +0 -2

@@ -567,8 +616,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 {

And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.

-----
Igor

Michael S. Tsirkin

author

2012-04-01 08:27 UTC #8 lore raw

Quoted 234 lines · Igor Mammedov

Quoted 233 lines · Michael S. Tsirkin

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.

This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.

Signed-off-by Michael S. Tsirkin<[email protected]>

---

Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update

 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)

docs/specs/acpi_pci_hotplug.txt +60 -8

index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +49 ~~-10~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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)

Quoted 8 lines

hw/acpi_piix4.c static uint32_t pciej_read(void *opaque, uint32_t addr)
Quoted 7 lines
hw/acpi_piix4.c +6 -0
+{
+    PIIX4PMState *s = opaque;
+    s->pci0_status.down&= ~val;
+
+    PIIX4_DPRINTF("pci_clr_down write %x<== %d\n", addr, val);
+}
 
Why adding PCI_CLR_[UP|DOWN]_BASE instead of using PCI_BASE for this writes?

Because symbolic names for specific registers are better than hard-coded offsets. I think we should replace PCI_BASE with PCI_UP_BASE/PCI_DOWN_BASE as well.

Quoted 25 lines

hw/acpi_piix4.c +5 -2

 static uint32_t pcirmv_read(void *opaque, uint32_t addr)
 {
     PIIX4PMState *s = opaque;

@@ -532,12 +578,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 +616,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
         return 0;
     }

Quoted 9 lines

hw/acpi_piix4.c static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, · line 621
Quoted 5 lines
hw/acpi_piix4.c +0 -2
-    s->pci0_status.up = 0;
-    s->pci0_status.down = 0;
     if (state == PCI_HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.

Which isn't really acceptable.

Quoted 2 lines

-----
Igor

Gleb Natapov

2012-04-01 08:27 UTC #9 lore raw

Quoted 6 lines · Michael S. Tsirkin

Quoted 3 lines
And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.
Which isn't really acceptable.

I do not think it should be taken for granted. We should be able to get rid of a crud once in a while.

--
Gleb.

Michael S. Tsirkin

author

2012-04-01 09:06 UTC #10 lore raw

Quoted 9 lines · Gleb Natapov

Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines
And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.
Which isn't really acceptable.
I do not think it should be taken for granted. We should be able to get rid of a crud once in a while.

In general, yes. It needs to be well planned several years in advance though. Something like
http://www.kernel.org/doc/Documentation/feature-removal-schedule.txt
might be a model.

Breaking compatibility would bring very little
gain in this case though, right?

Quoted 2 lines

--
Gleb.

Gleb Natapov

2012-04-01 09:13 UTC #11 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 7 lines · Gleb Natapov
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines
And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.
Which isn't really acceptable.

Quoted 8 lines

Quoted 2 lines
I do not think it should be taken for granted. We should be able to get rid of a crud once in a while.
In general, yes. It needs to be well planned several years in advance though. Something like
http://www.kernel.org/doc/Documentation/feature-removal-schedule.txt
might be a model.

No nearly the same. We do not drop any features, we just reimplementing it differently. Since QEMU ships with the bios image that suppose to work with it preserving old bios compatibility shouldn't be considered to much IMO. Migration from qemu-1.0 to qemu-1.1 -M pc-1.0 should work though. By adding more craft now we make cleanup harder later.

Quoted 3 lines

Breaking compatibility would bring very little
gain in this case though, right?

It will clean up code instead of adding more and will allow reuse of hotplug code in QEMU and bios between pci/cpu/memory.

--
Gleb.

Gleb Natapov

2012-04-01 09:20 UTC #12 lore raw

Quoted 23 lines · Gleb Natapov

Quoted 8 lines · Michael S. Tsirkin
Quoted 7 lines · Gleb Natapov
Quoted 6 lines · Michael S. Tsirkin
Quoted 3 lines
And if pci-hotplug could be done as cpu-hotplug, then it would simplify pci-hotplug (guest would only need to read PCI_BASE) and probably it would be possible to unify hotplug code for pci/cpu. But that for sure will break compatibility.
Which isn't really acceptable.
Quoted 8 lines
Quoted 2 lines
I do not think it should be taken for granted. We should be able to get rid of a crud once in a while.
In general, yes. It needs to be well planned several years in advance though. Something like
http://www.kernel.org/doc/Documentation/feature-removal-schedule.txt
might be a model.
No nearly the same. We do not drop any features, we just reimplementing it differently. Since QEMU ships with the bios image that suppose to work with it preserving old bios compatibility shouldn't be considered to much IMO. Migration from qemu-1.0 to qemu-1.1 -M pc-1.0 should work though. By adding more craft now we make cleanup harder later.

Quoted 6 lines

Quoted 3 lines
Breaking compatibility would bring very little
gain in this case though, right?
It will clean up code instead of adding more and will allow reuse of hotplug code in QEMU and bios between pci/cpu/memory.

Just to clarify I am not against your approach per se. We discussed both points Igor and Alex are bringing now with you on IRC and you convinced me that your approach is good enough. It shows that IRC is not a proper channel to discuss thous thing since discussion have to be repeated on ML anyway, so I bring my points from that discussion here for consideration by others.

--
Gleb.

Gleb Natapov

Acked-by on v2 1/1

2012-04-01 09:24 UTC #13 lore raw

In reply to Michael S. Tsirkin

Quoted 25 lines · Michael S. Tsirkin

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

Quoted 8 lines

For compatibility with old guests, we also clear the DOWN bit on write to EJ0 for a device.
This patch also adds more detailed documentation for the ACPI interface, including the semantics of both old and new registers.
Signed-off-by Michael S. Tsirkin <[email protected]>

Acked-by Gleb Natapov <[email protected]>

But Michael will have to convince Igor and Alex too.

Quoted 239 lines

---

Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update

 docs/specs/acpi_pci_hotplug.txt |   68 +++++++++++++++++++++++++++++++----
 hw/acpi_piix4.c                 |   75 +++++++++++++++++++++++++++++++-------
 2 files changed, 121 insertions(+), 22 deletions(-)

docs/specs/acpi_pci_hotplug.txt +60 -8

index f0f74a7..a8398f9 100644
@@ -7,31 +7,83 @@ describes the interface between QEMU and the ACPI BIOS.
 ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
 -----------------------------------------
 
-Generic ACPI GPE block. Bit 1 (GPE.1) used to notify PCI hotplug/eject
+Generic ACPI GPE block. Bit 1 (GPE.1) is used to notify PCI hotplug/hotunplug
 event to ACPI BIOS, via SCI interrupt.
 
-PCI slot injection notification pending (IO port 0xae00-0xae03, 4-byte access):
+Semantics: this event occurs each time a bit in either Pending PCI slot
+injection notification or Pending PCI slot removal notification registers
+changes status from 0 to 1.
+
+Pending PCI slot injection notification (IO port 0xae00-0xae03, 4-byte access):
 ---------------------------------------------------------------
-Slot injection notification pending. One bit per slot.
+Slot injection notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of injection
 events.
 
-PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+Semantics: after a slot is populated by hotplug, it is immediately enabled
+and a bit in this register is set.
+Reset value: 0
+
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
 -----------------------------------------------------
-Slot removal notification pending. One bit per slot.
+Slot removal notification is pending. One bit per slot.
+Guests should not write this register, in practice the value
+written overwrites the register.
 
 Read by ACPI BIOS GPE.1 handler to notify OS of removal
 events.
 
+Semantics: upon hotunplug request, a bit in this register is set
+and the OSPM is notified about hotunplug request, but a slot remains enabled
+until eject.
+Reset value: 0
+
 PCI device eject (IO port 0xae08-0xae0b, 4-byte access):
 ----------------------------------------
 
 Used by ACPI BIOS _EJ0 method to request device removal. One bit per slot.
-Reads return 0.
+Guests should not read this register. In practice reads return 0.
+
+Semantics: selected hotunpluggable slots are disabled and powered off.
+Has no effect on non-hotunpluggable slots.
+
+For compatibility with old BIOS, writes to this register
+clear bits in pending slot injection notification register.
 
 PCI removability status (IO port 0xae0c-0xae0f, 4-byte access):
 -----------------------------------------------
 
-Used by ACPI BIOS _RMV method to indicate removability status to OS. One
-bit per slot.
+Used by ACPI BIOS _RMV method to detect removability status
+and report to OS. One bit per slot.
+Guests should not write this register, in practice the value
+written is discarded.
+
+Semantics: selected slots support hotplug. Contents of this register
+do not change while guest is running.
+
+Clear PCI slot injection notification (IO port 0xae10-0xae14, 4-byte access):
+---------------------------------------------------------------
+Clears bits in pending slot injection notification register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot injection notification register and before
+notifying OS of injection events.
+
+Semantics: used to acknowledge the injection notification.
+Does not affect slot status.
+
+Clear PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
+-----------------------------------------------------
+Clears bits in slot removal notification pending register. One bit per slot.
+Guests should not read this register. In practice reads return 0.
+
+Written to by ACPI BIOS GPE.1 handler after reading the
+pending slot removal notification register and before
+notifying OS of removal events.
+
+Semantics: used to acknowledge the hotunplug request.
+Does not affect slot status.

hw/acpi_piix4.c +61 ~~-14~~

index 797ed24..50553fd 100644
@@ -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,28 @@ 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 - this is good for a case where guest can't
+     * write to CLR_DOWN because of VM reset, and 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 +326,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 can discard
+     * past events now.
+     */
+    s->pci0_status.up = 0;
 }
 
 static void piix4_powerdown(void *opaque, int irq, int power_failing)

@@ -490,22 +527,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 +578,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 +616,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 {

Signature · 1 line

1.7.9.111.gf3fb0

--
Gleb.