diff mbox

PCIE: fix regression with coldplugged multifunction device

Message ID 1392645606-26352-1-git-send-email-imammedo@redhat.com
State New
Headers show

Commit Message

Igor Mammedov Feb. 17, 2014, 2 p.m. UTC
QEMU abort is caused by misplaced assertion, which should
be checked only when device is hotplugged.

Refernce to regression report:
 http://www.mail-archive.com/qemu-devel@nongnu.org/msg216226.html

Reported-By: Nigel Kukard <nkukard+qemu@lbsd.net>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/pci/pcie.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

Comments

Michael S. Tsirkin Feb. 17, 2014, 2:20 p.m. UTC | #1
On Mon, Feb 17, 2014 at 03:00:06PM +0100, Igor Mammedov wrote:
> QEMU abort is caused by misplaced assertion, which should
> be checked only when device is hotplugged.
> 
> Refernce to regression report:
>  http://www.mail-archive.com/qemu-devel@nongnu.org/msg216226.html
> 
> Reported-By: Nigel Kukard <nkukard+qemu@lbsd.net>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>

Applied, thanks!

BTW we really should make hotplug return an error not assert.

> ---
>  hw/pci/pcie.c |   16 ++++++++--------
>  1 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
> index 8ecd11e..02cde6f 100644
> --- a/hw/pci/pcie.c
> +++ b/hw/pci/pcie.c
> @@ -221,29 +221,23 @@ static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
>                                           DeviceState *dev,
>                                           uint8_t **exp_cap, Error **errp)
>  {
> -    PCIDevice *pci_dev = PCI_DEVICE(dev);
>      *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
>      uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
>  
> -    PCIE_DEV_PRINTF(pci_dev, "hotplug state: %d\n", state);
> +    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: %d\n", state);
>      if (sltsta & PCI_EXP_SLTSTA_EIS) {
>          /* the slot is electromechanically locked.
>           * This error is propagated up to qdev and then to HMP/QMP.
>           */
>          error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
>      }
> -
> -    /* TODO: multifunction hot-plug.
> -     * Right now, only a device of function = 0 is allowed to be
> -     * hot plugged/unplugged.
> -     */
> -    assert(PCI_FUNC(pci_dev->devfn) == 0);
>  }
>  
>  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                Error **errp)
>  {
>      uint8_t *exp_cap;
> +    PCIDevice *pci_dev = PCI_DEVICE(dev);
>  
>      pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
>  
> @@ -256,6 +250,12 @@ void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
>          return;
>      }
>  
> +    /* TODO: multifunction hot-plug.
> +     * Right now, only a device of function = 0 is allowed to be
> +     * hot plugged/unplugged.
> +     */
> +    assert(PCI_FUNC(pci_dev->devfn) == 0);
> +
>      pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
>                                 PCI_EXP_SLTSTA_PDS);
>      pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);
> -- 
> 1.7.1
diff mbox

Patch

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 8ecd11e..02cde6f 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -221,29 +221,23 @@  static void pcie_cap_slot_hotplug_common(PCIDevice *hotplug_dev,
                                          DeviceState *dev,
                                          uint8_t **exp_cap, Error **errp)
 {
-    PCIDevice *pci_dev = PCI_DEVICE(dev);
     *exp_cap = hotplug_dev->config + hotplug_dev->exp.exp_cap;
     uint16_t sltsta = pci_get_word(*exp_cap + PCI_EXP_SLTSTA);
 
-    PCIE_DEV_PRINTF(pci_dev, "hotplug state: %d\n", state);
+    PCIE_DEV_PRINTF(PCI_DEVICE(dev), "hotplug state: %d\n", state);
     if (sltsta & PCI_EXP_SLTSTA_EIS) {
         /* the slot is electromechanically locked.
          * This error is propagated up to qdev and then to HMP/QMP.
          */
         error_setg_errno(errp, -EBUSY, "slot is electromechanically locked");
     }
-
-    /* TODO: multifunction hot-plug.
-     * Right now, only a device of function = 0 is allowed to be
-     * hot plugged/unplugged.
-     */
-    assert(PCI_FUNC(pci_dev->devfn) == 0);
 }
 
 void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
                               Error **errp)
 {
     uint8_t *exp_cap;
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
 
     pcie_cap_slot_hotplug_common(PCI_DEVICE(hotplug_dev), dev, &exp_cap, errp);
 
@@ -256,6 +250,12 @@  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
         return;
     }
 
+    /* TODO: multifunction hot-plug.
+     * Right now, only a device of function = 0 is allowed to be
+     * hot plugged/unplugged.
+     */
+    assert(PCI_FUNC(pci_dev->devfn) == 0);
+
     pci_word_test_and_set_mask(exp_cap + PCI_EXP_SLTSTA,
                                PCI_EXP_SLTSTA_PDS);
     pcie_cap_slot_event(PCI_DEVICE(hotplug_dev), PCI_EXP_HP_EV_PDC);