From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=42577 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1PI2we-0004iy-P5 for qemu-devel@nongnu.org; Mon, 15 Nov 2010 12:34:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1PI2dg-0002LO-CI for qemu-devel@nongnu.org; Mon, 15 Nov 2010 12:14:48 -0500 Received: from mail-wy0-f173.google.com ([74.125.82.173]:61768) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1PI2dg-0002L5-0d for qemu-devel@nongnu.org; Mon, 15 Nov 2010 12:14:44 -0500 Received: by wyj26 with SMTP id 26so6264051wyj.4 for ; Mon, 15 Nov 2010 09:14:41 -0800 (PST) MIME-Version: 1.0 Sender: camm@ualberta.ca In-Reply-To: <20101114141804.GA15504@redhat.com> References: <20101114141804.GA15504@redhat.com> Date: Mon, 15 Nov 2010 10:14:39 -0700 Message-ID: From: Cam Macdonell Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] Re: [PATCH] pci: allow hotplug removal of cold-plugged devices List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: yamahata@valinux.co.jp, qemu-devel@nongnu.org On Sun, Nov 14, 2010 at 7:18 AM, 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 Acked-by: Cam Macdonell > --- > > 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? Yes, it seems to fix the problem. > > > 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) > =A0 =A0 PIIX4_DPRINTF("pciej write %x <=3D=3D %d\n", addr, val); > =A0} > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int s= tate); > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PCIHotpl= ugState state); > > =A0static void piix4_acpi_system_hot_add_init(PCIBus *bus, PIIX4PMState *= s) > =A0{ > @@ -615,18 +616,23 @@ static void disable_device(PIIX4PMState *s, int slo= t) > =A0 =A0 s->pci0_status.down |=3D (1 << slot); > =A0} > > -static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, int s= tate) > +static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCIHotplugS= tate state) > =A0{ > =A0 =A0 int slot =3D PCI_SLOT(dev->devfn); > =A0 =A0 PIIX4PMState *s =3D DO_UPCAST(PIIX4PMState, dev, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DO_UPCAST= (PCIDevice, qdev, qdev)); > > - =A0 =A0if (!dev->qdev.hotplugged) > + =A0 =A0/* Don't send event when device is enabled during qemu machine c= reation: > + =A0 =A0 * it is present on boot, no hotplug event is necessary. We do s= end an > + =A0 =A0 * event when the device is disabled later. */ > + =A0 =A0if (state =3D=3D PCI_COLDPLUG_ENABLED) { > =A0 =A0 =A0 =A0 return 0; > + =A0 =A0} > > =A0 =A0 s->pci0_status.up =3D 0; > =A0 =A0 s->pci0_status.down =3D 0; > - =A0 =A0if (state) { > + =A0 =A0if (state =3D=3D PCI_HOTPLUG_ENABLED) { > =A0 =A0 =A0 =A0 enable_device(s, slot); > =A0 =A0 } else { > =A0 =A0 =A0 =A0 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, Device= Info *base) > =A0 =A0 pci_add_option_rom(pci_dev); > > =A0 =A0 if (bus->hotplug) { > - =A0 =A0 =A0 =A0/* lower layer must check qdev->hotplugged */ > - =A0 =A0 =A0 =A0rc =3D bus->hotplug(bus->hotplug_qdev, pci_dev, 1); > + =A0 =A0 =A0 =A0/* Let buses differentiate between hotplug and when devi= ce is > + =A0 =A0 =A0 =A0 * enabled during qemu machine creation. */ > + =A0 =A0 =A0 =A0rc =3D bus->hotplug(bus->hotplug_qdev, pci_dev, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qdev->hotplugged ? P= CI_HOTPLUG_ENABLED: > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PCI_COLDPLUG_ENABLED= ); > =A0 =A0 =A0 =A0 if (rc !=3D 0) { > =A0 =A0 =A0 =A0 =A0 =A0 int r =3D pci_unregister_device(&pci_dev->qdev); > =A0 =A0 =A0 =A0 =A0 =A0 assert(!r); > @@ -1581,7 +1584,8 @@ static int pci_unplug_device(DeviceState *qdev) > =A0{ > =A0 =A0 PCIDevice *dev =3D DO_UPCAST(PCIDevice, qdev, qdev); > > - =A0 =A0return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, 0); > + =A0 =A0return dev->bus->hotplug(dev->bus->hotplug_qdev, dev, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCI_HOTPLUG_DIS= ABLED); > =A0} > > =A0void 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); > > =A0typedef void (*pci_set_irq_fn)(void *opaque, int irq_num, int level); > =A0typedef 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 { > + =A0 =A0PCI_HOTPLUG_DISABLED, > + =A0 =A0PCI_HOTPLUG_ENABLED, > + =A0 =A0PCI_COLDPLUG_ENABLED, > +} PCIHotplugState; > + > +typedef int (*pci_hotplug_fn)(DeviceState *qdev, PCIDevice *pci_dev, > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PCIHotplugSt= ate state); > =A0void pci_bus_new_inplace(PCIBus *bus, DeviceState *parent, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0const char *name, int = devfn_min); > =A0PCIBus *pci_bus_new(DeviceState *parent, const char *name, int devfn_m= in); > 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, PCI= ExpressHotPlugEvent event) > =A0} > > =A0static int pcie_cap_slot_hotplug(DeviceState *qdev, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCIDevi= ce *pci_dev, int state) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 PCIDevi= ce *pci_dev, PCIHotplugState state) > =A0{ > =A0 =A0 PCIDevice *d =3D DO_UPCAST(PCIDevice, qdev, qdev); > =A0 =A0 uint8_t *exp_cap =3D d->config + d->exp.exp_cap; > =A0 =A0 uint16_t sltsta =3D pci_get_word(exp_cap + PCI_EXP_SLTSTA); > > - =A0 =A0if (!pci_dev->qdev.hotplugged) { > - =A0 =A0 =A0 =A0assert(state); /* this case only happens at machine crea= tion. */ > + =A0 =A0/* Don't send event when device is enabled during qemu machine c= reation: > + =A0 =A0 * it is present on boot, no hotplug event is necessary. We do s= end an > + =A0 =A0 * event when the device is disabled later. */ > + =A0 =A0if (state =3D=3D PCI_COLDPLUG_ENABLED) { > =A0 =A0 =A0 =A0 pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0PC= I_EXP_SLTSTA_PDS); > =A0 =A0 =A0 =A0 return 0; > >