From patchwork Mon Nov 15 02:23:44 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Isaku Yamahata X-Patchwork-Id: 71159 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [199.232.76.165]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id CC6A5B7108 for ; Mon, 15 Nov 2010 13:24:52 +1100 (EST) Received: from localhost ([127.0.0.1]:43493 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PHok9-0007fU-55 for incoming@patchwork.ozlabs.org; Sun, 14 Nov 2010 21:24:29 -0500 Received: from [140.186.70.92] (port=50213 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PHojY-0007fM-DM for qemu-devel@nongnu.org; Sun, 14 Nov 2010 21:23:54 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PHojU-0000c7-2u for qemu-devel@nongnu.org; Sun, 14 Nov 2010 21:23:52 -0500 Received: from mail.valinux.co.jp ([210.128.90.3]:59052) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PHojT-0000bq-GV for qemu-devel@nongnu.org; Sun, 14 Nov 2010 21:23:48 -0500 Received: from ps.local.valinux.co.jp (vagw.valinux.co.jp [210.128.90.14]) by mail.valinux.co.jp (Postfix) with SMTP id 65ABC8743C; Mon, 15 Nov 2010 11:23:44 +0900 (JST) Received: (nullmailer pid 15884 invoked by uid 1000); Mon, 15 Nov 2010 02:23:44 -0000 Date: Mon, 15 Nov 2010 11:23:44 +0900 From: Isaku Yamahata To: "Michael S\. Tsirkin" Message-ID: <20101115022344.GE27722@valinux.co.jp> References: <20101114141804.GA15504@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20101114141804.GA15504@redhat.com> User-Agent: Mutt/1.5.19 (2009-01-05) X-Virus-Scanned: clamav-milter 0.95.2 at va-mail.local.valinux.co.jp X-Virus-Status: Clean X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6 (newer, 3) Cc: cam@cs.ualberta.ca, qemu-devel@nongnu.org Subject: [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Thank you for catching pcie part. The following fix is necessary. On Sun, Nov 14, 2010 at 04:18:04PM +0200, Michael S. Tsirkin wrote: > This patch fixes 5beb8ad503c88a76f2b8106c3b74b4ce485a60e1 > which broke hotplug removal of cold plugged devices: > > - pass addition/removal state to hotplug callbacks > - use that in piix and pcie > > This also fixes an assert on hotplug removal of coldplugged > express devices. > > Reported-by: by Cam Macdonell . > Signed-off-by: Isaku Yamahata > Signed-off-by: Michael S. Tsirkin > --- > > So I think the below would be the cleanest way > to fix the bug as we keep the hot/cold plug logic > in a central palce. Untested. Comments? Cam? > > > diff --git a/hw/acpi_piix4.c b/hw/acpi_piix4.c > index 66c7885..f549089 100644 > --- a/hw/acpi_piix4.c > +++ b/hw/acpi_piix4.c > @@ -585,7 +585,8 @@ static void pciej_write(void *opaque, uint32_t addr, uint32_t val) > PIIX4_DPRINTF("pciej write %x <== %d\n", addr, val); > } > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state); > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > + PCIHotplugState state); > > static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *s) > { > @@ -615,18 +616,23 @@ static void disable_device(PIIX4PMState *s, int slot) > s->pci0_status.down |= (1 << slot); > } > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int state) > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > + PCIHotplugState state) > { > int slot = PCI_SLOT(dev->devfn); > PIIX4PMState *s = DO_UPCAST(PIIX4PMState, dev, > DO_UPCAST(PCIDevice, qdev, qdev)); > > - if (!dev->qdev.hotplugged) > + /* Don't send event when device is enabled during qemu machine creation: > + * it is present on boot, no hotplug event is necessary. We do send an > + * event when the device is disabled later. */ > + if (state == PCI_COLDPLUG_ENABLED) { > return 0; > + } > > s->pci0_status.up = 0; > s->pci0_status.down = 0; > - if (state) { > + if (state == PCI_HOTPLUG_ENABLED) { > enable_device(s, slot); > } else { > disable_device(s, slot); > diff --git a/hw/pci.c b/hw/pci.c > index 30e1603..316b24f 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1566,8 +1566,11 @@ static int pci_qdev_init(DeviceState *qdev, DeviceInfo *base) > pci_add_option_rom(pci_dev); > > if (bus->hotplug) { > - /* lower layer must check qdev->hotplugged */ > - rc = bus->hotplug(bus->hotplug_qdev, pci_dev, 1); > + /* Let buses differentiate between hotplug and when device is > + * enabled during qemu machine creation. */ > + rc = bus->hotplug(bus->hotplug_qdev, pci_dev, > + qdev->hotplugged ? PCI_HOTPLUG_ENABLED: > + PCI_COLDPLUG_ENABLED); > if (rc != 0) { > int r = pci_unregister_device(&pci_dev->qdev); > assert(!r); > @@ -1581,7 +1584,8 @@ static int pci_unplug_device(DeviceState *qdev) > { > PCIDevice *dev = DO_UPCAST(PCIDevice, qdev, qdev); > > - return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0); > + return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, > + PCI_HOTPLUG_DISABLED); > } > > void pci_qdev_register(PCIDeviceInfo *info) > diff --git a/hw/pci.h b/hw/pci.h > index 7100804..09b3e4c 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -214,7 +214,15 @@ int pci_device_load(PCIDevice *s, QEMUFile *f); > > typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); > typedef int (*pci_map_irq_fn)(PCIDevice *pci_dev, int irq_num); > -typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, int state); > + > +typedef enum { > + PCI_HOTPLUG_DISABLED, > + PCI_HOTPLUG_ENABLED, > + PCI_COLDPLUG_ENABLED, > +} PCIHotplugState; > + > +typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, > + PCIHotplugState state); > void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent, > const char *name, int devfn_min); > PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_min); > diff --git a/hw/pcie.c b/hw/pcie.c > index 35918f7..4df48b8 100644 > --- a/hw/pcie.c > +++ b/hw/pcie.c > @@ -192,14 +192,16 @@ static void pcie_cap_slot_event(PCIDevice *dev, PCIExpressHotPlugEvent event) > } > > static int pcie_cap_slot_hotplug(DeviceState *qdev, > - PCIDevice *pci_dev, int state) > + PCIDevice *pci_dev, PCIHotplugState state) > { > PCIDevice *d = DO_UPCAST(PCIDevice, qdev, qdev); > uint8_t *exp_cap = d->config + d->exp.exp_cap; > uint16_t sltsta = pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > - if (!pci_dev->qdev.hotplugged) { > - assert(state); /* this case only happens at machine creation. */ > + /* Don't send event when device is enabled during qemu machine creation: > + * it is present on boot, no hotplug event is necessary. We do send an > + * event when the device is disabled later. */ > + if (state == PCI_COLDPLUG_ENABLED) { > pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > PCI_EXP_SLTSTA_PDS); > return 0; > diff --git a/hw/pcie.c b/hw/pcie.c index 4df48b8..f461c1c 100644 --- a/hw/pcie.c +++ b/hw/pcie.c @@ -221,7 +221,7 @@ static int pcie_cap_slot_hotplug(DeviceState *qdev, */ assert(PCI_FUNC(pci_dev->devfn) == 0); - if (state) { + if (state == PCI_HOTPLUG_ENABLED) { pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, PCI_EXP_SLTSTA_PDS); pcie_cap_slot_event(d, PCI_EXP_HP_EV_PDC);