qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v28 03/17] vfio: Add save and load functions for VFIO PCI dev

From: Kirti Wankhede
Subject: Re: [PATCH v28 03/17] vfio: Add save and load functions for VFIO PCI devices
Date: Sun, 25 Oct 2020 01:18:37 +0530
User-agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:68.0) Gecko/20100101 Thunderbird/68.12.1 


On 10/24/2020 7:46 PM, Alex Williamson wrote:

On Sat, 24 Oct 2020 19:53:39 +0800
Yan Zhao <yan.y.zhao@intel.com> wrote:


hi
when I migrating VFs, the PCI_COMMAND is not properly saved. and the
target side would meet below bug
root@tester:~# [  189.360671] ++++++++++>> reset starts here: iavf_reset_task 
!!!
[  199.360798] iavf 0000:00:04.0: Reset never finished (0)
[  199.380504] kernel BUG at drivers/pci/msi.c:352!
[  199.382957] invalid opcode: 0000 [#1] SMP PTI
[  199.384855] CPU: 1 PID: 419 Comm: kworker/1:2 Tainted: G           OE     
5.0.0-13-generic #14-Ubuntu
[  199.388204] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
rel-1.14.0-0-g155821a1990b-prebuilt.qemu.org 04/01/2014
[  199.392401] Workqueue: events iavf_reset_task [iavf]
[  199.393586] RIP: 0010:free_msi_irqs+0x17b/0x1b0
[  199.394659] Code: 84 e1 fe ff ff 45 31 f6 eb 11 41 83 c6 01 44 39 73 14 0f 86 ce 
fe ff ff 8b 7b 10 44 01 f7 e8 3c 7a ba ff 48 83 78 70 00 74 e0 <0f> 0b 49 8d b5 
b0 00 00 00 e8 07 27 bb ff e9 cf fe ff ff 48 8b 78
[  199.399056] RSP: 0018:ffffabd1006cfdb8 EFLAGS: 00010282
[  199.400302] RAX: ffff9e336d8a2800 RBX: ffff9e3333b006c0 RCX: 0000000000000000
[  199.402000] RDX: 0000000000000000 RSI: 0000000000000019 RDI: ffffffffbaa68100
[  199.403168] RBP: ffffabd1006cfde8 R08: ffff9e3375000248 R09: ffff9e3375000338
[  199.404343] R10: 0000000000000000 R11: ffffffffbaa68108 R12: ffff9e3374ef12c0
[  199.405526] R13: ffff9e3374ef1000 R14: 0000000000000000 R15: ffff9e3371f2d018
[  199.406702] FS:  0000000000000000(0000) GS:ffff9e3375b00000(0000) 
knlGS:0000000000000000
[  199.408027] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  199.408987] CR2: 00000000ffffffff CR3: 0000000033266000 CR4: 00000000000006e0
[  199.410155] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  199.411321] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  199.412437] Call Trace:
[  199.412750]  pci_disable_msix+0xf3/0x120
[  199.413227]  iavf_reset_interrupt_capability.part.40+0x19/0x40 [iavf]
[  199.413998]  iavf_reset_task+0x4b3/0x9d0 [iavf]
[  199.414544]  process_one_work+0x20f/0x410
[  199.415026]  worker_thread+0x34/0x400
[  199.415486]  kthread+0x120/0x140
[  199.415876]  ? process_one_work+0x410/0x410
[  199.416380]  ? __kthread_parkme+0x70/0x70
[  199.416864]  ret_from_fork+0x35/0x40



I verified MSIx with SRIOV VF, and I don't see this issue at my end.


I fixed it with below patch.


commit ad3efa0eeea7edb352294bfce35b904b8d3c759c
Author: Yan Zhao <yan.y.zhao@intel.com>
Date:   Sat Oct 24 19:45:01 2020 +0800

     msix fix.
Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> 

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index f63f15b553..92f71bf933 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -2423,8 +2423,14 @@ const VMStateDescription vmstate_vfio_pci_config = {
  static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
  {
      VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+    PCIDevice *pdev = &vdev->pdev;
+    uint16_t pci_cmd;
+
+    pci_cmd = pci_default_read_config(pdev, PCI_COMMAND, 2);
+    qemu_put_be16(f, pci_cmd);
vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL); 
+
  }
static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f) 
@@ -2432,6 +2438,10 @@ static int vfio_pci_load_config(VFIODevice *vbasedev, 
QEMUFile *f)
      VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
      PCIDevice *pdev = &vdev->pdev;
      int ret;
+    uint16_t pci_cmd;
+
+    pci_cmd = qemu_get_be16(f);
+    vfio_pci_write_config(pdev, PCI_COMMAND, pci_cmd, 2);
ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1); 
      if (ret) {




We need to avoid this sort of ad-hoc stuffing random fields into the
config stream.  The command register is already migrated in vconfig, it
only needs to be written through vfio:

vfio_pci_write_config(pdev, PCI_COMMAND,
                      pci_get_word(pdev->config, PCI_COMMAND), 2);



I verified at my end again.
pci command value (using pci_default_read_config()) before vmstate_save_state() is 0x507 and at destination after vmstate_load_state() is also 0x507 - with pci_default_read_config() and the cached config space value using pci_get_word() - both are 0x507. 
VM restores successfully.
Yan, can you share pci command values before and after as above? what exactly is missing? 

Thanks,
Kirti


Thanks,
Alex



On Fri, Oct 23, 2020 at 04:10:29PM +0530, Kirti Wankhede wrote:

Added functions to save and restore PCI device specific data,
specifically config space of PCI device.

Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
Reviewed-by: Neo Jia <cjia@nvidia.com>
---
  hw/vfio/pci.c                 | 48 +++++++++++++++++++++++++++++++++++++++++++
  include/hw/vfio/vfio-common.h |  2 ++
  2 files changed, 50 insertions(+)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index bffd5bfe3b78..92cc25a5489f 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -41,6 +41,7 @@
  #include "trace.h"
  #include "qapi/error.h"
  #include "migration/blocker.h"
+#include "migration/qemu-file.h"
#define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug" @@ -2401,11 +2402,58 @@ static Object *vfio_pci_get_object(VFIODevice *vbasedev) 
      return OBJECT(vdev);
  }
+static bool vfio_msix_present(void *opaque, int version_id) 
+{
+    PCIDevice *pdev = opaque;
+
+    return msix_present(pdev);
+}
+
+const VMStateDescription vmstate_vfio_pci_config = {
+    .name = "VFIOPCIDevice",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
+        VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_present),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+
+    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
+}
+
+static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
+{
+    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
+    PCIDevice *pdev = &vdev->pdev;
+    int ret;
+
+    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
+    if (ret) {
+        return ret;
+    }
+
+    if (msi_enabled(pdev)) {
+        vfio_msi_enable(vdev);
+    } else if (msix_enabled(pdev)) {
+        vfio_msix_enable(vdev);
+    }
+
+    return ret;
+}
+
  static VFIODeviceOps vfio_pci_ops = {
      .vfio_compute_needs_reset = vfio_pci_compute_needs_reset,
      .vfio_hot_reset_multi = vfio_pci_hot_reset_multi,
      .vfio_eoi = vfio_intx_eoi,
      .vfio_get_object = vfio_pci_get_object,
+    .vfio_save_config = vfio_pci_save_config,
+    .vfio_load_config = vfio_pci_load_config,
  };
int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp) 
diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
index fe99c36a693a..ba6169cd926e 100644
--- a/include/hw/vfio/vfio-common.h
+++ b/include/hw/vfio/vfio-common.h
@@ -120,6 +120,8 @@ struct VFIODeviceOps {
      int (*vfio_hot_reset_multi)(VFIODevice *vdev);
      void (*vfio_eoi)(VFIODevice *vdev);
      Object *(*vfio_get_object)(VFIODevice *vdev);
+    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
+    int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
  };
typedef struct VFIOGroup { 
--
2.7.0
reply via email to
[Prev in Thread] Current Thread [Next in Thread]