On Fri, Mar 30, 2012 at 06:05:26PM +0200, Igor Mammedov wrote:
> On 03/29/2012 02:51 PM, 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.
> >
> >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<m...@redhat.com>
> >---
> >
> >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(-)
> >
> >diff --git a/docs/specs/acpi_pci_hotplug.txt
> >b/docs/specs/acpi_pci_hotplug.txt
> >index f0f74a7..a8398f9 100644
> >--- a/docs/specs/acpi_pci_hotplug.txt
> >+++ b/docs/specs/acpi_pci_hotplug.txt
> >@@ -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.
> >diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> >index 797ed24..50553fd 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,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);
> >+}
> >
> 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.
> > 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 {
>
> 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.
> -----
> Igor
