[Qemu-devel] Hotplug vs. multifunction regression

[Qemu-devel] Hotplug vs. multifunction regression

[Nigel Kukard]

Hi guys,

Starting from the patch series in 9f117d41 the below error is
observed... (reverting to the patch right before it works fine a1483f88)

Here is the error:
qemu-system-x86_64: hw/pci/pcie.c:240: pcie_cap_slot_hotplug_common:
Assertion `((pci_dev->devfn) & 0x07) == 0' failed.

Here is the command that generated it...

CMDLINE: $VAR1 = [
          'qemu-system-x86_64',
          '-m',
          '8G',
          '-machine',
          'q35',
          '-cpu',
          'qemu64',
          '-smp',
          '4,sockets=1,cores=4,threads=1',
          '-enable-kvm',
          '-device',
         
'ioh3420,bus=pcie.0,addr=1c.0,multifunction=on,port=1,chassis=1,id=root.1',
          '-device',
         
'vfio-pci,host=03:00.0,bus=root.1,addr=0.0,multifunction=on,x-vga=on',
          '-device',
          'vfio-pci,host=03:00.1,bus=root.1,addr=0.1',
          '-device',
          'ide-drive,drive=sata-disk0,bus=ide.0',
          '-drive',
         
'file=510-drive_sda.qcow2,id=sata-disk0,format=qcow2,cache=writeback,if=none',
          '-vga',
          'none',
          '-nographic',
          '-boot',
          'menu=on',
        ];

It seems the hotplug interface doesn't know how to handle multifunction
devices.

-N

[Qemu-devel] [PATCH] PCIE: fix regression with coldplugged multifunction device

[Igor Mammedov]

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(-)

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

Re: [Qemu-devel] [PATCH] PCIE: fix regression with coldplugged multifunction device

[Michael S. Tsirkin]

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

Re: [Qemu-devel] Hotplug vs. multifunction regression

[Alex Williamson]

[cc +igor, +mst]

Appears that it's no longer possible to use multifunction devices at
PCIe at all due to hotplug being an integral part of device realization
now.  Thanks,

Alex

On Sun, 2014-02-16 at 15:57 +0000, Nigel Kukard wrote:
> Hi guys,
> 
> Starting from the patch series in 9f117d41 the below error is
> observed... (reverting to the patch right before it works fine a1483f88)
> 
> Here is the error:
> qemu-system-x86_64: hw/pci/pcie.c:240: pcie_cap_slot_hotplug_common:
> Assertion `((pci_dev->devfn) & 0x07) == 0' failed.
> 
> Here is the command that generated it...
> 
> CMDLINE: $VAR1 = [
>           'qemu-system-x86_64',
>           '-m',
>           '8G',
>           '-machine',
>           'q35',
>           '-cpu',
>           'qemu64',
>           '-smp',
>           '4,sockets=1,cores=4,threads=1',
>           '-enable-kvm',
>           '-device',
>          
> 'ioh3420,bus=pcie.0,addr=1c.0,multifunction=on,port=1,chassis=1,id=root.1',
>           '-device',
>          
> 'vfio-pci,host=03:00.0,bus=root.1,addr=0.0,multifunction=on,x-vga=on',
>           '-device',
>           'vfio-pci,host=03:00.1,bus=root.1,addr=0.1',
>           '-device',
>           'ide-drive,drive=sata-disk0,bus=ide.0',
>           '-drive',
>          
> 'file=510-drive_sda.qcow2,id=sata-disk0,format=qcow2,cache=writeback,if=none',
>           '-vga',
>           'none',
>           '-nographic',
>           '-boot',
>           'menu=on',
>         ];
> 
> It seems the hotplug interface doesn't know how to handle multifunction
> devices.
> 
> -N
> 

Re: [Qemu-devel] Hotplug vs. multifunction regression

[Michael S. Tsirkin]

On Mon, Feb 17, 2014 at 07:10:51AM -0700, Alex Williamson wrote:
> [cc +igor, +mst]
> 
> Appears that it's no longer possible to use multifunction devices at
> PCIe at all due to hotplug being an integral part of device realization
> now.  Thanks,
> 
> Alex

Yes, Igor just fixed it.

> On Sun, 2014-02-16 at 15:57 +0000, Nigel Kukard wrote:
> > Hi guys,
> > 
> > Starting from the patch series in 9f117d41 the below error is
> > observed... (reverting to the patch right before it works fine a1483f88)
> > 
> > Here is the error:
> > qemu-system-x86_64: hw/pci/pcie.c:240: pcie_cap_slot_hotplug_common:
> > Assertion `((pci_dev->devfn) & 0x07) == 0' failed.
> > 
> > Here is the command that generated it...
> > 
> > CMDLINE: $VAR1 = [
> >           'qemu-system-x86_64',
> >           '-m',
> >           '8G',
> >           '-machine',
> >           'q35',
> >           '-cpu',
> >           'qemu64',
> >           '-smp',
> >           '4,sockets=1,cores=4,threads=1',
> >           '-enable-kvm',
> >           '-device',
> >          
> > 'ioh3420,bus=pcie.0,addr=1c.0,multifunction=on,port=1,chassis=1,id=root.1',
> >           '-device',
> >          
> > 'vfio-pci,host=03:00.0,bus=root.1,addr=0.0,multifunction=on,x-vga=on',
> >           '-device',
> >           'vfio-pci,host=03:00.1,bus=root.1,addr=0.1',
> >           '-device',
> >           'ide-drive,drive=sata-disk0,bus=ide.0',
> >           '-drive',
> >          
> > 'file=510-drive_sda.qcow2,id=sata-disk0,format=qcow2,cache=writeback,if=none',
> >           '-vga',
> >           'none',
> >           '-nographic',
> >           '-boot',
> >           'menu=on',
> >         ];
> > 
> > It seems the hotplug interface doesn't know how to handle multifunction
> > devices.
> > 
> > -N
> > 
> 
>