[Qemu-devel] [PATCH 0/2] acpi: robust notifications

[Qemu-devel] [PATCH 0/2] acpi: robust notifications

[Michael S. Tsirkin]

acpi currently clears up/down registers on each hotplug
request. This loses events if they are done quickly.
It's easy to clear down on ej write.
To detect that guest was notified of device addition,
we track configuration accesses since system reset.

Michael S. Tsirkin (2):
  pci: track function accesses
  acpi: don't clear up/down on each host change

 hw/acpi_piix4.c |   32 +++++++++++++++++++++++++++-----
 hw/pci.c        |   26 ++++++++++++++++++++++++++
 hw/pci.h        |    3 +++
 hw/pci_host.c   |    2 ++
 4 files changed, 58 insertions(+), 5 deletions(-)

-- 
1.7.9.111.gf3fb0

[Qemu-devel] [PATCH 1/2] pci: track function accesses

[Michael S. Tsirkin]

Set a flag when function is first accessed after system reset.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/pci.c      |   26 ++++++++++++++++++++++++++
 hw/pci.h      |    3 +++
 hw/pci_host.c |    2 ++
 3 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index b706e69..2aaa45e 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -407,6 +407,17 @@ static VMStateInfo vmstate_info_pci_irq_state = {
     .put  = put_pci_irq_state,
 };
 
+static const VMStateDescription pci_vmstate_accessed_since_reset ={
+    .name = "pci/accessed_since_reset",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField []) {
+        VMSTATE_BOOL(accessed_since_reset, PCIDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_pci_device = {
     .name = "PCIDevice",
     .version_id = 2,
@@ -421,6 +432,12 @@ const VMStateDescription vmstate_pci_device = {
 				   vmstate_info_pci_irq_state,
 				   PCI_NUM_PINS * sizeof(int32_t)),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &pci_vmstate_accessed_since_reset,
+        },
+        VMSTATE_END_OF_LIST()
     }
 };
 
@@ -738,6 +755,12 @@ static void pci_config_free(PCIDevice *pci_dev)
     g_free(pci_dev->used);
 }
 
+static void pci_system_reset_fn(void *opaque)
+{
+    PCIDevice *dev = opaque;
+    dev->accessed_since_reset = false;
+}
+
 /* -1 for devfn means auto assign */
 static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
                                          const char *name, int devfn)
@@ -805,11 +828,14 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
     bus->devices[devfn] = pci_dev;
     pci_dev->irq = qemu_allocate_irqs(pci_set_irq, pci_dev, PCI_NUM_PINS);
     pci_dev->version_id = 2; /* Current pci device vmstate version */
+    pci_dev->accessed_since_reset = false;
+    qemu_register_reset(pci_system_reset_fn, pci_dev);
     return pci_dev;
 }
 
 static void do_pci_unregister_device(PCIDevice *pci_dev)
 {
+    qemu_unregister_reset(pci_system_reset_fn, pci_dev);
     qemu_free_irqs(pci_dev->irq);
     pci_dev->bus->devices[pci_dev->devfn] = NULL;
     pci_config_free(pci_dev);
diff --git a/hw/pci.h b/hw/pci.h
index 8d0aa49..3fdd1dc 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -243,6 +243,9 @@ struct PCIDevice {
     bool has_rom;
     MemoryRegion rom;
     uint32_t rom_bar;
+
+    /* Cleared on first access after system reset */
+    bool accessed_since_reset;
 };
 
 void pci_register_bar(PCIDevice *pci_dev, int region_num,
diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..3019d72 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -51,6 +51,7 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
                                   uint32_t limit, uint32_t val, uint32_t len)
 {
     assert(len <= 4);
+    pci_dev->accessed_since_reset = true;
     pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
 }
 
@@ -58,6 +59,7 @@ uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
                                      uint32_t limit, uint32_t len)
 {
     assert(len <= 4);
+    pci_dev->accessed_since_reset = true;
     return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
 }
 
-- 
1.7.9.111.gf3fb0

[Qemu-devel] [PATCH 2/2] acpi: don't clear up/down on each host change

[Michael S. Tsirkin]

Instead of clearing up slot on each change, we clear:
- up: when slot is accessed
- down: when device is ejected

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/acpi_piix4.c |   32 +++++++++++++++++++++++++++-----
 1 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 797ed24..b6bb486 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -49,6 +49,7 @@
 struct pci_status {
     uint32_t up;
     uint32_t down;
+    BusState *bus;
 };
 
 typedef struct PIIX4PMState {
@@ -268,6 +269,21 @@ static const VMStateDescription vmstate_acpi = {
     }
 };
 
+/* Did guest access any devices in a slot? If yes, clear the up bit. */
+static void piix4_update_up(struct pci_status *s)
+{
+    DeviceState *qdev, *next;
+    int slot;
+
+    QTAILQ_FOREACH_SAFE(qdev, &s->bus->children, sibling, next) {
+        PCIDevice *pdev = PCI_DEVICE(qdev);
+        slot = PCI_SLOT(pdev->devfn);
+        if (pdev->accessed_since_reset) {
+            s->up &= ~(1 << slot);
+        }
+    }
+}
+
 static void piix4_update_hotplug(PIIX4PMState *s)
 {
     PCIDevice *dev = &s->dev;
@@ -290,6 +306,7 @@ static void piix4_update_hotplug(PIIX4PMState *s)
 static void piix4_reset(void *opaque)
 {
     PIIX4PMState *s = opaque;
+    struct pci_status *pci0_status = &s->pci0_status;
     uint8_t *pci_conf = s->dev.config;
 
     pci_conf[0x58] = 0;
@@ -301,6 +318,8 @@ static void piix4_reset(void *opaque)
         /* Mark SMM as already inited (until KVM supports SMM). */
         pci_conf[0x5B] = 0x02;
     }
+    pci0_status->up = 0;
+    pci0_status->down = 0;
     piix4_update_hotplug(s);
 }
 
@@ -454,6 +473,7 @@ static uint32_t pcihotplug_read(void *opaque, uint32_t addr)
     struct pci_status *g = opaque;
     switch (addr) {
         case PCI_BASE:
+            piix4_update_up(g);
             val = g->up;
             break;
         case PCI_BASE + 4:
@@ -490,10 +510,13 @@ 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;
+    struct pci_status *s = opaque;
+    BusState *bus = s->bus;
     DeviceState *qdev, *next;
     int slot = ffs(val) - 1;
 
+    s->down &= ~(1 << slot);
+    s->up &= ~(1 << slot);
     QTAILQ_FOREACH_SAFE(qdev, &bus->children, sibling, next) {
         PCIDevice *dev = PCI_DEVICE(qdev);
         PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
@@ -524,6 +547,7 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
 static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s)
 {
     struct pci_status *pci0_status = &s->pci0_status;
+    pci0_status->bus = qdev_get_parent_bus(&s->dev.qdev);
 
     register_ioport_write(GPE_BASE, GPE_LEN, 1, gpe_writeb, s);
     register_ioport_read(GPE_BASE, GPE_LEN, 1,  gpe_readb, s);
@@ -532,8 +556,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, pci0_status);
+    register_ioport_read(PCI_EJ_BASE, 4, 4,  pciej_read, pci0_status);
 
     register_ioport_write(PCI_RMV_BASE, 4, 4, pcirmv_write, s);
     register_ioport_read(PCI_RMV_BASE, 4, 4,  pcirmv_read, s);
@@ -567,8 +591,6 @@ static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
         return 0;
     }
 
-    s->pci0_status.up = 0;
-    s->pci0_status.down = 0;
     if (state == PCI_HOTPLUG_ENABLED) {
         enable_device(s, slot);
     } else {
-- 
1.7.9.111.gf3fb0

Re: [Qemu-devel] [PATCH 0/2] acpi: robust notifications

[Gleb Natapov]

On Sun, Mar 18, 2012 at 07:06:23PM +0200, Michael S. Tsirkin wrote:
> acpi currently clears up/down registers on each hotplug
> request. This loses events if they are done quickly.
> It's easy to clear down on ej write.
> To detect that guest was notified of device addition,
> we track configuration accesses since system reset.
> 
This is hack. This is not how real HW works, why do we needed it?
If up/down thing is so broken that this is required lets move to how
cpu hot-plug works and maintain slot status bit map instead. BTW
we discussed that with Alex last week. Have you followed that thread.

> Michael S. Tsirkin (2):
>   pci: track function accesses
>   acpi: don't clear up/down on each host change
> 
>  hw/acpi_piix4.c |   32 +++++++++++++++++++++++++++-----
>  hw/pci.c        |   26 ++++++++++++++++++++++++++
>  hw/pci.h        |    3 +++
>  hw/pci_host.c   |    2 ++
>  4 files changed, 58 insertions(+), 5 deletions(-)
> 
> -- 
> 1.7.9.111.gf3fb0

--
			Gleb.