piix: fix up/down races
PATCH v3 1/1 piix: fix up/down races
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 three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the
ACPI interface, to make the semantics explicit.
Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old qemu introduce a race: the registers were writeable there, so if a register changes between read and write, we'll clobber the new bits losing events. As that version tends to lose events anyway, and as the better than complicating the interface. If someone cares enough, the fix can go on the stable branch.
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above.
Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.
---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation update
docs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)
docs/specs/acpi_pci_hotplug.txt +47 -8
@@ -7,31 +7,70 @@ 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. +When written by Guest, this register implements write one to clear behaviour. Read by ACPI BIOS GPE.1 handler to notify OS of injection events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it is immediately enabled +and a bit in this register is set. +Writes into this register are used to acknowledge the injection notification +and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access): +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. +When written by Guest, this register implements write one to clear behaviour. Read by ACPI BIOS GPE.1 handler to notify OS of removal events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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. +Writes into this register are used to acknowledge the hotunplug request, +and do not not affect the slot status. +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. +guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set are discarded. Also in +practice writing the value 0 into this register has no effect. + +Semantics: selected hotunpluggable slot is 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.
hw/acpi_piix4.c +42 -16
99 lines not on this page · raw message
Signature · 1 line
1.7.9.111.gf3fb0
Quoted 36 lines · Michael S. Tsirkin
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slotif 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 respondinterrupt is reset and guest will never eject the device.
To fix this, we implement three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the
ACPI interface, to make the semantics explicit.Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
Quoted 7 lines
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old qemu introduce a race: the registers were writeable there, so if a register changes between read and write, we'll clobber the new bits losing events. As that version tends to lose events anyway, and as the better than complicating the interface. If someone cares enough, the fix can go on the stable branch.
Seems like it'd be easy to have seabios probe this on boot, ex. write <value> to 0xae00, read 0xae00 to see if value is reflected. If it is, old qemu, modify dsdt to avoid writes.
Quoted 5 lines
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above. Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.
Doesn't windows poll these bits. How can we say that introducing a bus rescan every poll interval is harmless?
Quoted 21 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation updatedocs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)docs/specs/acpi_pci_hotplug.txt +0
-0@@ -7,31 +7,70 @@ describes the interface between QEMU and the ACPI BIOS. ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 15
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2----------------------------------------- -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.
Or if your bios doesn't clear it, 1 to 1.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 20
Quoted 6 lines
docs/specs/acpi_pci_hotplug.txt +4
-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. +When written by Guest, this register implements write one to clear behaviour.
So we're writing the mask of the bits (slots) to be cleared? Can multiple slots be cleared in one write?
Quoted 7 lines
docs/specs/acpi_pci_hotplug.txt +5
-0Read by ACPI BIOS GPE.1 handler to notify OS of injection events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it is immediately enabled +and a bit in this register is set.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 36
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +6
-2+Writes into this register are used to acknowledge the injection notification +and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access): +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. +When written by Guest, this register implements write one to clear behaviour.
Same clarification as above.
Quoted 9 lines
docs/specs/acpi_pci_hotplug.txt +6
-0Read by ACPI BIOS GPE.1 handler to notify OS of removal events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 55
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +5
-1+Writes into this register are used to acknowledge the hotunplug request, +and do not not affect the slot status. +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.
Isn't mentioning the read value creating a defacto standard? If we're not defining we should say reads are undefined.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 58
Quoted 3 lines
docs/specs/acpi_pci_hotplug.txt +3
-0+guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set are discarded. Also in
practice = standard, define it.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 59
Quoted 1 line
docs/specs/acpi_pci_hotplug.txt +1
-0+practice writing the value 0 into this register has no effect.
ditto.
Quoted 5 lines
docs/specs/acpi_pci_hotplug.txt +5
-0+Semantics: selected hotunpluggable slot is 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.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 73
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2PCI 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.
We should define writes as not supported. "Should not, but ok if you do" is not a specification.
Quoted 12 lines
docs/specs/acpi_pci_hotplug.txt +2
-0+Semantics: selected slots support hotplug. Contents of this register +do not change while guest is running.hw/acpi_piix4.c +2
-0@@ -287,6 +287,28 @@ static void piix4_update_hotplug(PIIX4PMState *s) } } +static void acpi_piix_eject_slot (PIIX4PMState *s,unsigned slots) +{
hw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 10 lines
hw/acpi_piix4.c +10
-0+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);
Should we clear "up" here too?
Quoted 35 lines
hw/acpi_piix4.c +23
-0+ + 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 +324,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)@@ -472,10 +507,10 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)struct pci_status *g = opaque;
hw/acpi_piix4.c static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val) · line 515
Quoted 10 lines
hw/acpi_piix4.c +2
-2switch (addr) {case PCI_BASE : - g->up = val; + g->up &= ~val;break ;case PCI_BASE +4 : - g->down = val; + g->down &= ~val;break ; }
Ok, so it is a bit(slot) mask and can clear multiple bits.
Quoted 35 lines
hw/acpi_piix4.c +5
-12@@ -490,19 +525,12 @@ 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); }@@ -532,8 +560,8 @@ 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);
hw/acpi_piix4.c static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, · line 600
Quoted 8 lines
hw/acpi_piix4.c +0
-2@@ -567,8 +595,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 {
So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding? Hmm, can't say I'm excited about this hack either. Thanks,
Alex
Quoted 45 lines · Alex Williamson
Quoted 36 lines · Michael S. Tsirkin
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slotif 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 respondinterrupt is reset and guest will never eject the device.
To fix this, we implement three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the
ACPI interface, to make the semantics explicit.Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
Quoted 7 lines
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old qemu introduce a race: the registers were writeable there, so if a register changes between read and write, we'll clobber the new bits losing events. As that version tends to lose events anyway, and as the better than complicating the interface. If someone cares enough, the fix can go on the stable branch.
Quoted 3 lines
Seems like it'd be easy to have seabios probe this on boot, ex. write <value> to
0xae00, read0xae00to see if value is reflected. If it is, old qemu, modify dsdt to avoid writes.
This won't help at all: the issue is when we migrate to old qemu after boot.
Quoted 7 lines
Quoted 5 lines
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above. Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.Doesn't windows poll these bits.
No.
Quoted 2 lines
How can we say that introducing a bus
rescan every poll interval is harmless?
It's not done every poll interval - all this does is cause extra rescans when system interrupt is asserted.
Quoted 240 lines
Quoted 21 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation updatedocs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)docs/specs/acpi_pci_hotplug.txt +0
-0@@ -7,31 +7,70 @@ describes the interface between QEMU and the ACPI BIOS. ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 15
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2----------------------------------------- -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.Or if your bios doesn't clear it, 1 to 1.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 20
Quoted 6 lines
docs/specs/acpi_pci_hotplug.txt +4
-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. +When written by Guest, this register implements write one to clear behaviour.So we're writing the mask of the bits (slots) to be cleared? Can multiple slots be cleared in one write?
Quoted 7 lines
docs/specs/acpi_pci_hotplug.txt +5
-0Read by ACPI BIOS GPE.1 handler to notify OS of injection events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it is immediately enabled +and a bit in this register is set.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 36
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +6
-2+Writes into this register are used to acknowledge the injection notification +and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access): +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. +When written by Guest, this register implements write one to clear behaviour.Same clarification as above.
Quoted 9 lines
docs/specs/acpi_pci_hotplug.txt +6
-0Read by ACPI BIOS GPE.1 handler to notify OS of removal events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 55
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +5
-1+Writes into this register are used to acknowledge the hotunplug request, +and do not not affect the slot status. +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.Isn't mentioning the read value creating a defacto standard? If we're not defining we should say reads are undefined.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 58
Quoted 3 lines
docs/specs/acpi_pci_hotplug.txt +3
-0+guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set are discarded. Also inpractice = standard, define it.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 59
Quoted 1 line
docs/specs/acpi_pci_hotplug.txt +1
-0+practice writing the value 0 into this register has no effect.ditto.
Quoted 5 lines
docs/specs/acpi_pci_hotplug.txt +5
-0+Semantics: selected hotunpluggable slot is 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.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 73
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2PCI 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.We should define writes as not supported. "Should not, but ok if you do" is not a specification.
Quoted 12 lines
docs/specs/acpi_pci_hotplug.txt +2
-0+Semantics: selected slots support hotplug. Contents of this register +do not change while guest is running.hw/acpi_piix4.c +2
-0@@ -287,6 +287,28 @@ static void piix4_update_hotplug(PIIX4PMState *s) } } +static void acpi_piix_eject_slot (PIIX4PMState *s,unsigned slots) +{hw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 10 lines
hw/acpi_piix4.c +10
-0+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);Should we clear "up" here too?
Quoted 35 lines
hw/acpi_piix4.c +23
-0+ + 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 +324,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)@@ -472,10 +507,10 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)struct pci_status *g = opaque;hw/acpi_piix4.c static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val) · line 515
Quoted 10 lines
hw/acpi_piix4.c +2
-2switch (addr) {case PCI_BASE : - g->up = val; + g->up &= ~val;break ;case PCI_BASE +4 : - g->down = val; + g->down &= ~val;break ; }Ok, so it is a bit(slot) mask and can clear multiple bits.
Quoted 35 lines
hw/acpi_piix4.c +5
-12@@ -490,19 +525,12 @@ 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); }@@ -532,8 +560,8 @@ 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);hw/acpi_piix4.c static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, · line 600
Quoted 8 lines
hw/acpi_piix4.c +0
-2@@ -567,8 +595,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 {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding? Hmm, can't say I'm excited about this hack either. Thanks,
Alex
----- Original Message -----
Quoted 76 lines
From: "Michael S. Tsirkin" <[email protected]>
To: "Alex Williamson" <[email protected]>
Cc: [email protected], "Anthony Liguori" <[email protected]>, "Gerd Hoffmann" <[email protected]>, "Isaku Yamahata" <[email protected]>, "Aurelien Jarno" <[email protected]>, "Paolo Bonzini" <[email protected]>, [email protected], [email protected]
Sent: Monday, April 2, 2012 9:20:32 PM
Subject: Re: [PATCHv3] piix: fix up/down racesQuoted 50 lines · Alex Williamson
Quoted 36 lines · Michael S. Tsirkin
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slotif 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 respondinterrupt is reset and guest will never eject the device.
To fix this, we implement three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the ACPI interface, to make the semantics explicit.
Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
Quoted 12 lines
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old
qemu introduce a race: the registers were writeable there, so if a
register changes between read and write, we'll clobber the new bits
losing events. As that version tends to lose events anyway, and as the
better than complicating the interface. If someone cares enough, the fix
can go on the stable branch.Quoted 5 lines
Seems like it'd be easy to have seabios probe this on boot, ex. write
<value> to0xae00, read0xae00to see if value is reflected. If it is,
old qemu, modify dsdt to avoid writes.This won't help at all: the issue is when we migrate to old qemu after boot.
Quoted 7 lines
Quoted 5 lines
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above. Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.Doesn't windows poll these bits.
Quoted 7 lines
No.
Quoted 2 lines
How can we say that introducing a bus
rescan every poll interval is harmless?It's not done every poll interval - all this does is cause extra rescans when system interrupt is asserted.
it will do rescan only when PIIX4_PCI_HOTPLUG_STATUS in gpe.sts is set.
Quoted 284 lines
Quoted 283 lines
Quoted 34 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation updatedocs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)docs/specs/acpi_pci_hotplug.txt +0
-0@@ -7,31 +7,70 @@ describes the interface between QEMU and theACPI 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.Quoted 7 lines
-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.Or if your bios doesn't clear it, 1 to 1.
Quoted 8 lines
docs/specs/acpi_pci_hotplug.txt +2
-0+ +Pending PCI slot injection notification (IO port 0xae00-0xae03,4-byte access):
---------------------------------------------------------------docs/specs/acpi_pci_hotplug.txt +2
-1-Slot injection notification pending. One bit per slot. +Slot injection notification is pending. One bit per slot. +When written by Guest, this register implements write one toclear behaviour.
So we're writing the mask of the bits (slots) to be cleared? Can multiple slots be cleared in one write?
Quoted 21 lines
Read by ACPI BIOS GPE.1 handler to notify OS of injection events.
docs/specs/acpi_pci_hotplug.txt +4
-0+Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it isimmediately enabled
docs/specs/acpi_pci_hotplug.txt +2
-0+and a bit in this register is set. +Writes into this register are used to acknowledge the injectionnotification
docs/specs/acpi_pci_hotplug.txt +2
-1+and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byteaccess):
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
-----------------------------------------------------docs/specs/acpi_pci_hotplug.txt +2
-1-Slot removal notification pending. One bit per slot. +Slot removal notification is pending. One bit per slot. +When written by Guest, this register implements write one toQuoted 1 line
clear behaviour.
Same clarification as above.
Quoted 22 lines
Read by ACPI BIOS GPE.1 handler to notify OS of removal events.
docs/specs/acpi_pci_hotplug.txt +5
-0+Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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 slotremains enabled
docs/specs/acpi_pci_hotplug.txt +2
-0+until eject. +Writes into this register are used to acknowledge the hotunplugrequest,
docs/specs/acpi_pci_hotplug.txt +4
-1+and do not not affect the slot status. +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 returnQuoted 1 line
0.
Isn't mentioning the read value creating a defacto standard? If we're
not defining we should say reads are undefined.Quoted 4 lines
docs/specs/acpi_pci_hotplug.txt +3
-0+guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set arediscarded. Also in
practice = standard, define it.
Quoted 1 line
+practice writing the value 0 into this register has no effect.
ditto.
Quoted 12 lines
+Semantics: selected hotunpluggable slot is disabled and powered off.
docs/specs/acpi_pci_hotplug.txt +4
-1+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 toOS. One
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 73
Quoted 5 lines
docs/specs/acpi_pci_hotplug.txt +4
-1-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.We should define writes as not supported. "Should not, but ok if you
do" is not a specification.Quoted 24 lines
+Semantics: selected slots support hotplug. Contents of this register
docs/specs/acpi_pci_hotplug.txt +1
-0+do not change while guest is running.hw/acpi_piix4.c +8
-0@@ -287,6 +287,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 toCLR_DOWNbecause of VM reset, and for
compatibility withhw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 3 lines
hw/acpi_piix4.c +3
-0+* old guests which do not have CLR_DOWN. +*/ + s->pci0_status.down &= ~(1U << slot);Should we clear "up" here too?
Quoted 38 lines
hw/acpi_piix4.c +20
-0+ + 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 +324,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
hw/acpi_piix4.c +3
-0+* past events now. +*/ + s->pci0_status.up =0 ; }static void piix4_powerdown (void *opaque,int irq,int power_failing)@@ -472,10 +507,10 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)struct pci_status *g = opaque;hw/acpi_piix4.c static void pcihotplug_write(void *opaque, · line 516
Quoted 10 lines
hw/acpi_piix4.c +2
-2switch (addr) {case PCI_BASE : - g->up = val; + g->up &= ~val;break ;case PCI_BASE +4 : - g->down = val; + g->down &= ~val;break ; }Ok, so it is a bit(slot) mask and can clear multiple bits.
Quoted 48 lines
hw/acpi_piix4.c +3
-10@@ -490,19 +525,12 @@ 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);}
hw/acpi_piix4.c +2
-2@@ -532,8 +560,8 @@ 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);hw/acpi_piix4.c +0
-2@@ -567,8 +595,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);Quoted 1 line
} else {So if we have an old bios and do an add, followed by a remove, guest
ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and
bailing wire from exploding? Hmm, can't say I'm excited about this hack
either. Thanks,Alex
----- Original Message -----
Quoted 114 lines
From: "Alex Williamson" <[email protected]>
To: "Michael S. Tsirkin" <[email protected]>
Cc: [email protected], "Anthony Liguori" <[email protected]>, "Gerd Hoffmann" <[email protected]>, "Isaku Yamahata" <[email protected]>, "Aurelien Jarno" <[email protected]>, "Paolo Bonzini" <[email protected]>, [email protected], [email protected]
Sent: Monday, April 2, 2012 8:59:54 PM
Subject: Re: [PATCHv3] piix: fix up/down racesQuoted 36 lines · Michael S. Tsirkin
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slotif 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 respondinterrupt is reset and guest will never eject the device.
To fix this, we implement three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the
ACPI interface, to make the semantics explicit.Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
Quoted 10 lines
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old
qemu introduce a race: the registers were writeable there, so if a register changes between read and write, we'll clobber the new bits losing events. As that version tends to lose events anyway, and as the
better than complicating the interface. If someone cares enough, the fix
can go on the stable branch.Seems like it'd be easy to have seabios probe this on boot, ex. write <value> to
0xae00, read0xae00to see if value is reflected. If it is,
old qemu, modify dsdt to avoid writes.Quoted 5 lines
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above. Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.Doesn't windows poll these bits. How can we say that introducing a bus
rescan every poll interval is harmless?Quoted 33 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation updatedocs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)docs/specs/acpi_pci_hotplug.txt +0
-0@@ -7,31 +7,70 @@ describes the interface between QEMU and the ACPIBIOS.
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.Quoted 7 lines
-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.Or if your bios doesn't clear it, 1 to 1.
Quoted 11 lines
Quoted 8 lines
docs/specs/acpi_pci_hotplug.txt +2
-0+ +Pending PCI slot injection notification (IO port 0xae00-0xae03,4-byte access):
---------------------------------------------------------------docs/specs/acpi_pci_hotplug.txt +2
-1-Slot injection notification pending. One bit per slot. +Slot injection notification is pending. One bit per slot. +When written by Guest, this register implements write one to clearbehaviour.
So we're writing the mask of the bits (slots) to be cleared? Can multiple slots be cleared in one write?
Probably here we should refer to gpe.sts reg behaviour described in acpi spec. and the same for other writeable regs.
Quoted 11 lines
Quoted 11 lines
Read by ACPI BIOS GPE.1 handler to notify OS of injection events.
docs/specs/acpi_pci_hotplug.txt +4
-0+Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it is immediatelyenabled
docs/specs/acpi_pci_hotplug.txt +2
-0+and a bit in this register is set. +Writes into this register are used to acknowledge the injectionnotification
maybe "clear the injection notification events in specified sots" is better?
Quoted 86 lines
Quoted 11 lines
docs/specs/acpi_pci_hotplug.txt +2
-1+and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byteaccess):
+Pending PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access):
-----------------------------------------------------docs/specs/acpi_pci_hotplug.txt +2
-1-Slot removal notification pending. One bit per slot. +Slot removal notification is pending. One bit per slot. +When written by Guest, this register implements write one to clearQuoted 1 line
behaviour.
Same clarification as above.
Quoted 12 lines
Read by ACPI BIOS GPE.1 handler to notify OS of removal events.
docs/specs/acpi_pci_hotplug.txt +5
-0+Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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 slotremains enabled
docs/specs/acpi_pci_hotplug.txt +2
-0+until eject. +Writes into this register are used to acknowledge the hotunplugrequest,
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 55
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-1+and do not not affect the slot status. +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.Isn't mentioning the read value creating a defacto standard? If we're
not defining we should say reads are undefined.Quoted 4 lines
docs/specs/acpi_pci_hotplug.txt +3
-0+guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set arediscarded. Also in
practice = standard, define it.
Quoted 1 line
+practice writing the value 0 into this register has no effect.
ditto.
Quoted 12 lines
+Semantics: selected hotunpluggable slot is disabled and powered off.
docs/specs/acpi_pci_hotplug.txt +4
-1+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 toOS. One
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 73
Quoted 5 lines
docs/specs/acpi_pci_hotplug.txt +4
-1-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.We should define writes as not supported. "Should not, but ok if you do" is not a specification.
Quoted 12 lines
+Semantics: selected slots support hotplug. Contents of this register
docs/specs/acpi_pci_hotplug.txt +1
-0+do not change while guest is running.hw/acpi_piix4.c +0
-0@@ -287,6 +287,28 @@ static void piix4_update_hotplug(PIIX4PMState *s) } }
Quoted 12 lines
Quoted 8 lines
hw/acpi_piix4.c +8
-0+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 Quoted 4 lines
can't
+ * write toCLR_DOWNbecause of VM reset, and for
compatibility with
+ * old guests which do not haveCLR_DOWN.
fix comment, there is no CLR_DOWN any more
Quoted 4 lines
hw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 2 lines
hw/acpi_piix4.c +2
-0+*/ + s->pci0_status.down &= ~(1U << slot);Should we clear "up" here too?
Is it possible to create pci dev for not yet freed pci slot? If not then we should not care, otherwise we should fix that.
Quoted 110 lines
Quoted 38 lines
hw/acpi_piix4.c +23
-0+ + 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 +324,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)hw/acpi_piix4.c +0
-0@@ -472,10 +507,10 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)struct pci_status *g = opaque;hw/acpi_piix4.c static void pcihotplug_write(void *opaque, · line 516
Quoted 10 lines
hw/acpi_piix4.c +2
-2switch (addr) {case PCI_BASE : - g->up = val; + g->up &= ~val;break ;case PCI_BASE +4 : - g->down = val; + g->down &= ~val;break ; }Ok, so it is a bit(slot) mask and can clear multiple bits.
Quoted 47 lines
hw/acpi_piix4.c +5
-12@@ -490,19 +525,12 @@ 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); }@@ -532,8 +560,8 @@ 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);hw/acpi_piix4.c +0
-2@@ -567,8 +595,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);Quoted 1 line
} else {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding? Hmm, can't say I'm excited about this hack
either. Thanks,Alex
Quoted 5 lines · Igor Mammedov
Quoted 4 lines
hw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 2 lines
hw/acpi_piix4.c +2
-0+*/ + s->pci0_status.down &= ~(1U << slot);Should we clear "up" here too?
Is it possible to create pci dev for not yet freed pci slot?
It's not possible ATM.
Quoted 16 lines
If not then we should not care, otherwise we should fix that.
Quoted 15 lines
Quoted 10 lines
hw/acpi_piix4.c +0
-2@@ -567,8 +595,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 {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding?
In fact UP triggers a rescan - there is no injection event in ACPI. So it seems you can check the events in any order.
Quoted 7 lines
Quoted 7 lines
Hmm, can't say I'm excited about this
hack
either. Thanks,Alex
Quoted 26 lines · Michael S. Tsirkin
Quoted 5 lines · Igor Mammedov
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ */ + s->pci0_status.down &= ~(1U << slot);Should we clear "up" here too?
Is it possible to create pci dev for not yet freed pci slot?
It's not possible ATM.
Quoted 16 lines
If not then we should not care, otherwise we should fix that.
Quoted 15 lines
Quoted 10 lines
(quoted diff) +0
-2@@ -567,8 +595,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 {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding?
Quoted 2 lines
In fact UP triggers a rescan - there is no injection event in ACPI. So it seems you can check the events in any order.
So, we're not concerned with extra device checks and we can clear "down" on eject. What's then the practical advantage of your patch over:
--
hw/acpi_piix4.c +12 -30
@@ -79,6 +79,7 @@ typedef struct PIIX4PMState { } PIIX4PMState;static void piix4_acpi_system_hot_add_init (PCIBus *bus,PIIX4PMState *s); +static void pciej_write (void *opaque,uint32_t addr,uint32_t val);#define ACPI_ENABLE 0xf1#define ACPI_DISABLE 0xf0
@@ -291,6 +292,10 @@ static void piix4_update_hotplug(PIIX4PMState *s) s->pci0_hotplug_enable &= ~(1 << slot); } } + +while (s->pci0_status.down) { +pciej_write (s,PCI_BASE +4 , s->pci0_status.down); + } }static void piix4_reset (void *opaque)
@@ -470,38 +475,15 @@ static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)static uint32_t pcihotplug_read (void *opaque,uint32_t addr) { -uint32_t val =0 ; +uint32_t val;struct pci_status *g = opaque; -switch (addr) { -case PCI_BASE : - val = g->up; -break ; -case PCI_BASE +4 : - val = g->down; -break ; -default : -break ; - } + + val = g->down;PIIX4_DPRINTF ("pcihotplug read %x == %x\n" , addr, val);return val; } -static void pcihotplug_write (void *opaque,uint32_t addr,uint32_t val) -{ -struct pci_status *g = opaque; -switch (addr) { -case PCI_BASE : - g->up = val; -break ; -case PCI_BASE +4 : - g->down = val; -break ; - } - -PIIX4_DPRINTF ("pcihotplug write %x <== %d\n" , addr, val); -} -static uint32_t pciej_read (void *opaque,uint32_t addr) {PIIX4_DPRINTF ("pciej read %x\n" , addr);
@@ -514,6 +496,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)DeviceState *qdev, *next;int slot =ffs (val) -1 ; + 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);
@@ -560,8 +544,8 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)register_ioport_write (PROC_BASE ,32 ,1 , gpe_writeb, s);register_ioport_read (PROC_BASE ,32 ,1 , gpe_readb, 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_read (PCI_BASE ,4 ,4 , pcirmv_read, s); +register_ioport_read (PCI_BASE +4 ,4 ,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);
@@ -640,8 +624,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 {
ie. Up statically returns all hotplug slots, check them all. Down is cleared on eject and reset. Then there's no bios compatibility issue, no bios update even. Clearing down from PCNT doesn't really buy us anything. Thanks,
Alex
Quoted 27 lines · Alex Williamson
Quoted 26 lines · Michael S. Tsirkin
Quoted 5 lines · Igor Mammedov
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ */ + s->pci0_status.down &= ~(1U << slot);Should we clear "up" here too?
Is it possible to create pci dev for not yet freed pci slot?
It's not possible ATM.
Quoted 16 lines
If not then we should not care, otherwise we should fix that.
Quoted 15 lines
Quoted 10 lines
hw/acpi_piix4.c +0
-2@@ -567,8 +595,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 {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding?
Quoted 7 lines
Quoted 2 lines
In fact UP triggers a rescan - there is no injection event in ACPI. So it seems you can check the events in any order.
So, we're not concerned with extra device checks and we can clear "down" on eject. What's then the practical advantage of your patch over:
Signed-off-by Alex Williamson <[email protected]>
For old bios there is no advantage. But new BIOS can avoid a storm of notifications by clearing the register properly.
Quoted 97 lines
--
hw/acpi_piix4.c +12
-30@@ -79,6 +79,7 @@ typedef struct PIIX4PMState { } PIIX4PMState;static void piix4_acpi_system_hot_add_init (PCIBus *bus,PIIX4PMState *s); +static void pciej_write (void *opaque,uint32_t addr,uint32_t val);#define ACPI_ENABLE 0xf1#define ACPI_DISABLE 0xf0@@ -291,6 +292,10 @@ static void piix4_update_hotplug(PIIX4PMState *s) s->pci0_hotplug_enable &= ~(1 << slot); } } + +while (s->pci0_status.down) { +pciej_write (s,PCI_BASE +4 , s->pci0_status.down); + } }static void piix4_reset (void *opaque)@@ -470,38 +475,15 @@ static void gpe_writeb(void *opaque, uint32_t addr, uint32_t val)static uint32_t pcihotplug_read (void *opaque,uint32_t addr) { -uint32_t val =0 ; +uint32_t val;struct pci_status *g = opaque; -switch (addr) { -case PCI_BASE : - val = g->up; -break ; -case PCI_BASE +4 : - val = g->down; -break ; -default : -break ; - } + + val = g->down;PIIX4_DPRINTF ("pcihotplug read %x == %x\n" , addr, val);return val; } -static void pcihotplug_write (void *opaque,uint32_t addr,uint32_t val) -{ -struct pci_status *g = opaque; -switch (addr) { -case PCI_BASE : - g->up = val; -break ; -case PCI_BASE +4 : - g->down = val; -break ; - } - -PIIX4_DPRINTF ("pcihotplug write %x <== %d\n" , addr, val); -} -static uint32_t pciej_read (void *opaque,uint32_t addr) {PIIX4_DPRINTF ("pciej read %x\n" , addr);@@ -514,6 +496,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)DeviceState *qdev, *next;int slot =ffs (val) -1 ; + 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);@@ -560,8 +544,8 @@ static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)register_ioport_write (PROC_BASE ,32 ,1 , gpe_writeb, s);register_ioport_read (PROC_BASE ,32 ,1 , gpe_readb, 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_read (PCI_BASE ,4 ,4 , pcirmv_read, s); +register_ioport_read (PCI_BASE +4 ,4 ,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);@@ -640,8 +624,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 {
Quoted 3 lines
ie. Up statically returns all hotplug slots, check them all. Down is cleared on eject and reset. Then there's no bios compatibility issue, no bios update even.
What you suggest will work but it's a hack.
I do keep the hack around for compatibility but document as such, so we can get rid of it in qemu 2.0 or whenever we are ready to drop migration compatibility.
Quoted 4 lines
Clearing down from PCNT doesn't really buy us
anything. Thanks,Alex
If you don't you get multiple eject notifications. Yes they are harmless but ugly, new bios should be able to get a single notification in a clean way.
Signature · 1 line
MST
Quoted 184 lines · Alex Williamson
Quoted 36 lines · Michael S. Tsirkin
piix acpi interface suffers from the following 2 issues:
1.
- delete device a
- quickly add device b in another slotif 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 respondinterrupt is reset and guest will never eject the device.
To fix this, we implement three changes:
1. Do not clear up/down registers on each hotplug event. this ensures we don't lose events.
2. Make existing up/down registers write 1 to clear; bios will now write back to these the value it read.
3. 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 extends the documentation for the
ACPI interface, to make the semantics explicit.Compatibility notes: migrating guests across qemu versions can cause bios from one qemu realease running on another qemu release. The following documents the behaviour in this case:
Quoted 7 lines
A. new bios running on old qemu
A bios implementing the change mentioned above will if running on an old qemu introduce a race: the registers were writeable there, so if a register changes between read and write, we'll clobber the new bits losing events. As that version tends to lose events anyway, and as the better than complicating the interface. If someone cares enough, the fix can go on the stable branch.Seems like it'd be easy to have seabios probe this on boot, ex. write <value> to
0xae00, read0xae00to see if value is reflected. If it is, old qemu, modify dsdt to avoid writes.Quoted 5 lines
B. old bios running on new qemu
Down bits will get cleared on eject because of 3 above. Up bits only get cleared on reset.
This will cause unnecessary notifications and bus rescans, but they are harmless.Doesn't windows poll these bits. How can we say that introducing a bus rescan every poll interval is harmless?
Quoted 21 lines
Signed-off-by Michael S. Tsirkin <[email protected]>---
Changes from v2:
- Use existing register instead of adding a new one, and update documentation
Changes from v1:
- tweaked a comment about clearing down register on reset
- documentation updatedocs/specs/acpi_pci_hotplug.txt | 55 +++++++++++++++++++++++++++++++----- hw/acpi_piix4.c | 58 ++++++++++++++++++++++++++++----------- 2 files changed, 89 insertions(+), 24 deletions(-)docs/specs/acpi_pci_hotplug.txt +0
-0@@ -7,31 +7,70 @@ describes the interface between QEMU and the ACPI BIOS. ACPI GPE block (IO ports 0xafe0-0xafe3, byte access):docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 15
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2----------------------------------------- -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.Or if your bios doesn't clear it, 1 to 1.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 20
Quoted 6 lines
docs/specs/acpi_pci_hotplug.txt +4
-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. +When written by Guest, this register implements write one to clear behaviour.So we're writing the mask of the bits (slots) to be cleared? Can multiple slots be cleared in one write?
Quoted 7 lines
docs/specs/acpi_pci_hotplug.txt +5
-0Read by ACPI BIOS GPE.1 handler to notify OS of injection events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying OS of injection events. + +Semantics: after a slot is populated by hotplug, it is immediately enabled +and a bit in this register is set.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 36
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +6
-2+Writes into this register are used to acknowledge the injection notification +and do not not affect the slot status. +Reset value: 0 -PCI slot removal notification (IO port 0xae04-0xae07, 4-byte access): +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. +When written by Guest, this register implements write one to clear behaviour.Same clarification as above.
Quoted 9 lines
docs/specs/acpi_pci_hotplug.txt +6
-0Read by ACPI BIOS GPE.1 handler to notify OS of removal events. +Written to by ACPI BIOS GPE.1 handler after reading +and before notifying 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.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 55
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +5
-1+Writes into this register are used to acknowledge the hotunplug request, +and do not not affect the slot status. +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.Isn't mentioning the read value creating a defacto standard? If we're not defining we should say reads are undefined.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 58
Quoted 3 lines
docs/specs/acpi_pci_hotplug.txt +3
-0+guests should write values with exactly one bit set, selecting +the slot to eject. +In practice all bits except the lowest bit that is set are discarded. Also inpractice = standard, define it.
docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 59
Quoted 1 line
docs/specs/acpi_pci_hotplug.txt +1
-0+practice writing the value 0 into this register has no effect.ditto.
Quoted 5 lines
docs/specs/acpi_pci_hotplug.txt +5
-0+Semantics: selected hotunpluggable slot is 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.docs/specs/acpi_pci_hotplug.txt describes the interface between QEMU and the ACPI BIOS. · line 73
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +4
-2PCI 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.We should define writes as not supported. "Should not, but ok if you do" is not a specification.
Quoted 10 lines
docs/specs/acpi_pci_hotplug.txt +2
-0+Semantics: selected slots support hotplug. Contents of this register +do not change while guest is running.hw/acpi_piix4.c +0
-0@@ -287,6 +287,28 @@ static void piix4_update_hotplug(PIIX4PMState *s) } }
Quoted 14 lines
hw/acpi_piix4.c static void piix4_update_hotplug(PIIX4PMState *s) · line 301
Quoted 12 lines
hw/acpi_piix4.c +12
-0+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);Should we clear "up" here too?
On eject clearing "up" is probably OK. However I'm not sure what purpose would it serve, and it seems the less hacks we do the better.
Quoted 86 lines
Quoted 35 lines
hw/acpi_piix4.c +23
-0+ + 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 +324,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)@@ -472,10 +507,10 @@ static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val)struct pci_status *g = opaque;hw/acpi_piix4.c static void pcihotplug_write(void *opaque, uint32_t addr, uint32_t val) · line 515
Quoted 10 lines
hw/acpi_piix4.c +2
-2switch (addr) {case PCI_BASE : - g->up = val; + g->up &= ~val;break ;case PCI_BASE +4 : - g->down = val; + g->down &= ~val;break ; }Ok, so it is a bit(slot) mask and can clear multiple bits.
Quoted 38 lines
hw/acpi_piix4.c +5
-12@@ -490,19 +525,12 @@ 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); }@@ -532,8 +560,8 @@ 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);@@ -567,8 +595,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,return 0 ; }
Quoted 10 lines
hw/acpi_piix4.c static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, · line 600
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 {So if we have an old bios and do an add, followed by a remove, guest ACPI finds both "up" and "down" set for the slot and we rely on the ordering of the AML checking up before down to keep the duct tape and bailing wire from exploding?
Sounds like an overstatement. If bios did it the other way around, it would lose the down event. This is exactly what happened originally anyway.
Quoted 4 lines
Hmm, can't say I'm excited about this hack
either. Thanks,Alex
No need to get excited, but I'll add this to the documentation.
Signature · 1 line
MST
Quoted 7 lines · Michael S. Tsirkin
Quoted 7 lines
Quoted 6 lines
hw/acpi_piix4.c +6
-0+/* +* 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);
Quoted 5 lines
Quoted 1 line
Should we clear "up" here too?
On eject clearing "up" is probably OK. However I'm not sure what purpose would it serve, and it seems the less hacks we do the better.
I added a comment to clarify this.