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.william...@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;
> }
