Patchwork [4/6] acpi_piix4: Track PCI hotplug status and allow non-ACPI remove path

login
register
mail settings
Submitter Alex Williamson
Date March 7, 2012, 12:14 a.m.
Message ID <20120307001438.3079.32280.stgit@bling.home>
Download mbox | patch
Permalink /patch/145119/
State New
Headers show

Comments

Alex Williamson - March 7, 2012, 12:14 a.m.
When a guest probes a device, clear the "up" bit in the hotplug
register.  This allows us to enable a non-ACPI remove path for
devices added, but never accessed by the guest.  This is useful
when a guest does not have ACPI PCI hotplug support to avoid losing
devices to a guest.  We also now individually track bits for "up"
and "down" rather than clearing both on each PCI hotplug action.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 hw/acpi_piix4.c |   58 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 46 insertions(+), 12 deletions(-)
Michael S. Tsirkin - March 11, 2012, 9:57 p.m.
On Tue, Mar 06, 2012 at 05:14:51PM -0700, Alex Williamson wrote:
> When a guest probes a device, clear the "up" bit in the hotplug
> register.  This allows us to enable a non-ACPI remove path for
> devices added, but never accessed by the guest.  This is useful
> when a guest does not have ACPI PCI hotplug support to avoid losing
> devices to a guest.  We also now individually track bits for "up"
> and "down" rather than clearing both on each PCI hotplug action.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

There are two features here:
1. Fixing up/down handling

2. non ACPI removal

I think 1 is done correctly here. But 2.
seems something completely unrelated to acpi.
How about tracking access in pci core?

> ---
> 
>  hw/acpi_piix4.c |   58 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 files changed, 46 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
> index 4d88e23..7e766e5 100644
> --- a/hw/acpi_piix4.c
> +++ b/hw/acpi_piix4.c
> @@ -27,6 +27,7 @@
>  #include "sysemu.h"
>  #include "range.h"
>  #include "ioport.h"
> +#include "pci_host.h"
>  
>  //#define DEBUG
>  
> @@ -75,6 +76,7 @@ typedef struct PIIX4PMState {
>      qemu_irq smi_irq;
>      int kvm_enabled;
>      Notifier machine_ready;
> +    Notifier device_probe;
>  
>      /* for pci hotplug */
>      ACPIGPE gpe;
> @@ -336,6 +338,16 @@ static void piix4_pm_machine_ready(Notifier *n, void *opaque)
>  
>  }
>  
> +static void piix4_pm_device_probe(Notifier *n, void *opaque)
> +{
> +    PIIX4PMState *s = container_of(n, PIIX4PMState, device_probe);
> +    PCIDevice *pdev = opaque;
> +
> +    if (pci_find_domain(pdev->bus) == 0 && pci_bus_num(pdev->bus) == 0) {
> +        s->pci0_status.up &= ~(1U << PCI_SLOT(pdev->devfn));
> +    }

Seems ugly.  How about we register notifiers per bus?

> +}
> +
>  static PIIX4PMState *global_piix4_pm_state; /* cpu hotadd */
>  
>  static int piix4_pm_initfn(PCIDevice *dev)
> @@ -383,6 +395,8 @@ static int piix4_pm_initfn(PCIDevice *dev)
>      qemu_add_machine_init_done_notifier(&s->machine_ready);
>      qemu_register_reset(piix4_reset, s);
>      piix4_acpi_system_hot_add_init(dev->bus, s);
> +    s->device_probe.notify = piix4_pm_device_probe;
> +    pci_host_add_dev_probe_notifier(&s->device_probe);
>  
>      return 0;
>  }
> @@ -502,6 +516,7 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
>          PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
>          if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
>              qdev_free(qdev);
> +            s->pci0_status.down &= ~(1U << slot);
>          }
>      }
>  
> @@ -594,16 +609,41 @@ void qemu_system_cpu_hot_add(int cpu, int state)
>  }
>  #endif
>  
> -static void enable_device(PIIX4PMState *s, int slot)
> +static int enable_device(PIIX4PMState *s, int slot)
>  {
> +    uint32_t mask = 1U << slot;
> +
> +    if ((s->pci0_status.up | s->pci0_status.down) & mask) {
> +        return -1;
> +    }
> +
>      s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> -    s->pci0_status.up |= (1 << slot);
> +    s->pci0_status.up |= mask;
> +
> +    pm_update_sci(s);
> +    return 0;
>  }
>  
> -static void disable_device(PIIX4PMState *s, int slot)
> +static int disable_device(PIIX4PMState *s, int slot)
>  {
> +    uint32_t mask = 1U << slot;
> +
> +    if (s->pci0_status.up & mask) {
> +        s->pci0_status.up &= ~mask;
> +        pciej_write(s, PCI_EJ_BASE, mask);
> +
> +        /* Clear GPE PCI hotplug status if nothing left pending */
> +        if (!(s->pci0_status.up | s->pci0_status.down)) {
> +            s->gpe.sts[0] &= ~PIIX4_PCI_HOTPLUG_STATUS;
> +        }
> +        return 0;
> +    }
> +
>      s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
> -    s->pci0_status.down |= (1 << slot);
> +    s->pci0_status.down |= mask;
> +
> +    pm_update_sci(s);
> +    return 0;
>  }
>  
>  static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
> @@ -620,15 +660,9 @@ 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);
> +        return enable_device(s, slot);
>      } else {
> -        disable_device(s, slot);
> +        return disable_device(s, slot);
>      }
> -
> -    pm_update_sci(s);
> -
> -    return 0;
>  }

Patch

diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c
index 4d88e23..7e766e5 100644
--- a/hw/acpi_piix4.c
+++ b/hw/acpi_piix4.c
@@ -27,6 +27,7 @@ 
 #include "sysemu.h"
 #include "range.h"
 #include "ioport.h"
+#include "pci_host.h"
 
 //#define DEBUG
 
@@ -75,6 +76,7 @@  typedef struct PIIX4PMState {
     qemu_irq smi_irq;
     int kvm_enabled;
     Notifier machine_ready;
+    Notifier device_probe;
 
     /* for pci hotplug */
     ACPIGPE gpe;
@@ -336,6 +338,16 @@  static void piix4_pm_machine_ready(Notifier *n, void *opaque)
 
 }
 
+static void piix4_pm_device_probe(Notifier *n, void *opaque)
+{
+    PIIX4PMState *s = container_of(n, PIIX4PMState, device_probe);
+    PCIDevice *pdev = opaque;
+
+    if (pci_find_domain(pdev->bus) == 0 && pci_bus_num(pdev->bus) == 0) {
+        s->pci0_status.up &= ~(1U << PCI_SLOT(pdev->devfn));
+    }
+}
+
 static PIIX4PMState *global_piix4_pm_state; /* cpu hotadd */
 
 static int piix4_pm_initfn(PCIDevice *dev)
@@ -383,6 +395,8 @@  static int piix4_pm_initfn(PCIDevice *dev)
     qemu_add_machine_init_done_notifier(&s->machine_ready);
     qemu_register_reset(piix4_reset, s);
     piix4_acpi_system_hot_add_init(dev->bus, s);
+    s->device_probe.notify = piix4_pm_device_probe;
+    pci_host_add_dev_probe_notifier(&s->device_probe);
 
     return 0;
 }
@@ -502,6 +516,7 @@  static void pciej_write(void *opaque, uint32_t addr, uint32_t val)
         PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
         if (PCI_SLOT(dev->devfn) == slot && !pc->no_hotplug) {
             qdev_free(qdev);
+            s->pci0_status.down &= ~(1U << slot);
         }
     }
 
@@ -594,16 +609,41 @@  void qemu_system_cpu_hot_add(int cpu, int state)
 }
 #endif
 
-static void enable_device(PIIX4PMState *s, int slot)
+static int enable_device(PIIX4PMState *s, int slot)
 {
+    uint32_t mask = 1U << slot;
+
+    if ((s->pci0_status.up | s->pci0_status.down) & mask) {
+        return -1;
+    }
+
     s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.up |= (1 << slot);
+    s->pci0_status.up |= mask;
+
+    pm_update_sci(s);
+    return 0;
 }
 
-static void disable_device(PIIX4PMState *s, int slot)
+static int disable_device(PIIX4PMState *s, int slot)
 {
+    uint32_t mask = 1U << slot;
+
+    if (s->pci0_status.up & mask) {
+        s->pci0_status.up &= ~mask;
+        pciej_write(s, PCI_EJ_BASE, mask);
+
+        /* Clear GPE PCI hotplug status if nothing left pending */
+        if (!(s->pci0_status.up | s->pci0_status.down)) {
+            s->gpe.sts[0] &= ~PIIX4_PCI_HOTPLUG_STATUS;
+        }
+        return 0;
+    }
+
     s->gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS;
-    s->pci0_status.down |= (1 << slot);
+    s->pci0_status.down |= mask;
+
+    pm_update_sci(s);
+    return 0;
 }
 
 static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev,
@@ -620,15 +660,9 @@  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);
+        return enable_device(s, slot);
     } else {
-        disable_device(s, slot);
+        return disable_device(s, slot);
     }
-
-    pm_update_sci(s);
-
-    return 0;
 }