Re: [Qemu-devel] [PATCH 4/6] acpi_piix4: Track PCI hotplug status and allow non-ACPI remove path

[Michael S. Tsirkin]

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 <address@hidden>

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;
>  }