Message ID | 1333461589.3799.183.camel@bling.home |
---|---|
State | New |
Headers | show |
On Tue, Apr 03, 2012 at 07:59:49AM -0600, Alex Williamson wrote: > On Tue, 2012-04-03 at 13:05 +0300, Michael S. Tsirkin wrote: > > On Tue, Apr 03, 2012 at 02:40:41AM -0400, Igor Mammedov wrote: > > > > > + */ > > > > > + 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. > > > > > If not then we should not care, otherwise we should fix that. > > > > > @@ -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. > > 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 <alex.williamson@redhat.com> For old bios there is no advantage. But new BIOS can avoid a storm of notifications by clearing the register properly. > -- > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 0c77730..050061c 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -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. 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. > 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.
diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c index 0c77730..050061c 100644 --- a/hw/acpi_piix4.c +++ b/hw/acpi_piix4.c @@ -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 {