[Qemu-devel] [RFC v2 0/2] Add live migration support in the PVRDMA device

[Qemu-devel] [RFC v2 0/2] Add live migration support in the PVRDMA device

[Sukrit Bhatnagar]

Changes in v2:

* Modify load_dsr() such that dsr mapping is not performed if dsr value
  is non-NULL. Also move free_dsr() out of load_dsr() and call it right
  before if needed. These two changes will allow us to call load_dsr()
  even when we have already done dsr mapping and would like to go on
  with the rest of mappings.

* Use VMStateDescription instead of SaveVMHandlers to describe migration
  state. Also add fields for parent PCI object and MSIX.

* Use a temporary structure (struct PVRDMAMigTmp) to hold some fields
  during migration. These fields, such as cmd_slot_dma and resp_slot_dma
  inside dsr, do not fit into VMSTATE macros as their container
  (dsr_info->dsr) will not be ready until it is mapped on the dest.

* Perform mappings to CQ and event notification rings after the state is
  loaded. This is an extension to the mappings performed in v1;
  following the flow of load_dsr(). All the mappings are succesfully
  done on the dest on state load.

Link(s) to v1:
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04924.html
https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04923.html


Things working now (were not working at the time of v1):

* vmxnet3 is migrating successfully. The issue was in the migration of
  its PCI configuration space, and is solved by the patch Marcel had sent:
  https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01500.html

* There is no problem due to BounceBuffers which were failing the dma mapping
  calls in state load logic earlier. Not sure exactly how it went away. I am
  guessing that adding the PCI and MSIX state to migration solved the issue.


What is still needed:

* A workaround to get libvirt to support same-host migration. Since
  the problems faced in v1 (mentioned above) are out of the way, we
  can move further, and in doing so, we will need this.

Sukrit Bhatnagar (2):
  hw/pvrdma: make DSR mapping idempotent in load_dsr()
  hw/pvrdma: add live migration support

 hw/rdma/vmw/pvrdma_main.c | 104 +++++++++++++++++++++++++++++++++++---
 1 file changed, 96 insertions(+), 8 deletions(-)

-- 
2.21.0

[Qemu-devel] [RFC v2 1/2] hw/pvrdma: make DSR mapping idempotent in load_dsr()

[Sukrit Bhatnagar]

Map to DSR only when there is no mapping done already i.e., when
dev->dsr_info.dsr is NULL. This allows the rest of mappings and
ring inits to be done by calling load_dsr() when DSR has already
been mapped to, somewhere else.

Move free_dsr() out of load_dsr() and call it before the latter
as and when needed. This aids the case where load_dsr() is called
having DSR mapping already done, but the rest of map and init
operations are pending, and prevents an unmap of the DSR.

Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Yuval Shaia <yuval.shaia@oracle.com>
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---
 hw/rdma/vmw/pvrdma_main.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index adcf79cd63..6c90db96f9 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -172,15 +172,15 @@ static int load_dsr(PVRDMADev *dev)
     DSRInfo *dsr_info;
     struct pvrdma_device_shared_region *dsr;
 
-    free_dsr(dev);
-
-    /* Map to DSR */
-    dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
-                              sizeof(struct pvrdma_device_shared_region));
     if (!dev->dsr_info.dsr) {
-        rdma_error_report("Failed to map to DSR");
-        rc = -ENOMEM;
-        goto out;
+        /* Map to DSR */
+        dev->dsr_info.dsr = rdma_pci_dma_map(pci_dev, dev->dsr_info.dma,
+                                  sizeof(struct pvrdma_device_shared_region));
+        if (!dev->dsr_info.dsr) {
+            rdma_error_report("Failed to map to DSR");
+            rc = -ENOMEM;
+            goto out;
+        }
     }
 
     /* Shortcuts */
@@ -402,6 +402,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, uint64_t val,
     case PVRDMA_REG_DSRHIGH:
         trace_pvrdma_regs_write(addr, val, "DSRHIGH", "");
         dev->dsr_info.dma |= val << 32;
+        free_dsr(dev);
         load_dsr(dev);
         init_dsr_dev_caps(dev);
         break;
-- 
2.21.0

[Qemu-devel] [RFC v2 2/2] hw/pvrdma: add live migration support

[Sukrit Bhatnagar]

Use VMStateDescription for migrating device state. Currently,
'vmstate_pvrdma' describes the PCI and MSIX state for pvrdma and
'vmstate_pvrdma_dsr_dma' describes a temporary state containing
some values obtained only after mapping to dsr in the source.
Since the dsr will not be available on dest until we map to the
dma address we had on source, these values cannot be migrated
directly.

Add PVRDMAMigTmp to store this temporary state which consists of
dma addresses and ring page information. The 'parent' member is
used to refer to the device state (PVRDMADev) so that parent PCI
device object is accessible, which is needed to remap to DSR.

pvrdma_dsr_dma_pre_save() saves the dsr state into this temporary
representation and pvrdma_dsr_dma_post_load() loads it back.
This load function also remaps to the dsr and and calls
load_dsr() for further map and ring init operations.

Please note that this call to load_dsr() can be removed from the
migration flow and included in pvrdma_regs_write() to perform a
lazy load.
As of now, migration will fail if there in an error in load_dsr().
Also, there might be a considerable amount of pages in the rings,
which will have dma map operations when the init functions are
called.
If this takes noticeable time, it might be better to have lazy
load instead.

Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Cc: Yuval Shaia <yuval.shaia@oracle.com>
Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
---
 hw/rdma/vmw/pvrdma_main.c | 87 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
index 6c90db96f9..4a10bd2fc7 100644
--- a/hw/rdma/vmw/pvrdma_main.c
+++ b/hw/rdma/vmw/pvrdma_main.c
@@ -28,6 +28,7 @@
 #include "sysemu/sysemu.h"
 #include "monitor/monitor.h"
 #include "hw/rdma/rdma.h"
+#include "migration/register.h"
 
 #include "../rdma_rm.h"
 #include "../rdma_backend.h"
@@ -593,6 +594,91 @@ static void pvrdma_shutdown_notifier(Notifier *n, void *opaque)
     pvrdma_fini(pci_dev);
 }
 
+struct PVRDMAMigTmp {
+    PVRDMADev *parent;
+    uint64_t dma;
+    uint64_t cmd_slot_dma;
+    uint64_t resp_slot_dma;
+    uint32_t cq_ring_pages_num_pages;
+    uint64_t cq_ring_pages_pdir_dma;
+    uint32_t async_ring_pages_num_pages;
+    uint64_t async_ring_pages_pdir_dma;
+};
+
+static int pvrdma_dsr_dma_pre_save(void *opaque)
+{
+    struct PVRDMAMigTmp *tmp = opaque;
+    DSRInfo *dsr_info = &tmp->parent->dsr_info;
+    struct pvrdma_device_shared_region *dsr = dsr_info->dsr;
+
+    tmp->dma = dsr_info->dma;
+    tmp->cmd_slot_dma = dsr->cmd_slot_dma;
+    tmp->resp_slot_dma = dsr->resp_slot_dma;
+    tmp->cq_ring_pages_num_pages = dsr->cq_ring_pages.num_pages;
+    tmp->cq_ring_pages_pdir_dma = dsr->cq_ring_pages.pdir_dma;
+    tmp->async_ring_pages_num_pages = dsr->async_ring_pages.num_pages;
+    tmp->async_ring_pages_pdir_dma = dsr->async_ring_pages.pdir_dma;
+
+    return 0;
+}
+
+static int pvrdma_dsr_dma_post_load(void *opaque, int version_id)
+{
+    struct PVRDMAMigTmp *tmp = opaque;
+    PVRDMADev *dev = tmp->parent;
+    PCIDevice *pci_dev = PCI_DEVICE(dev);
+    DSRInfo *dsr_info = &dev->dsr_info;
+    struct pvrdma_device_shared_region *dsr;
+
+    dsr_info->dma = tmp->dma;
+    dsr_info->dsr = rdma_pci_dma_map(pci_dev, dsr_info->dma,
+                                sizeof(struct pvrdma_device_shared_region));
+    if (!dsr_info->dsr) {
+        rdma_error_report("Failed to map to DSR");
+        return -ENOMEM;
+    }
+
+    dsr = dsr_info->dsr;
+    dsr->cmd_slot_dma = tmp->cmd_slot_dma;
+    dsr->resp_slot_dma = tmp->resp_slot_dma;
+    dsr->cq_ring_pages.num_pages = tmp->cq_ring_pages_num_pages;
+    dsr->cq_ring_pages.pdir_dma = tmp->cq_ring_pages_pdir_dma;
+    dsr->async_ring_pages.num_pages = tmp->async_ring_pages_num_pages;
+    dsr->async_ring_pages.pdir_dma = tmp->async_ring_pages_pdir_dma;
+
+    return load_dsr(dev);
+}
+
+static const VMStateDescription vmstate_pvrdma_dsr_dma = {
+    .name = "pvrdma-dsr-dma",
+    .pre_save = pvrdma_dsr_dma_pre_save,
+    .post_load = pvrdma_dsr_dma_post_load,
+    .fields = (VMStateField[]) {
+            VMSTATE_UINT64(dma, struct PVRDMAMigTmp),
+            VMSTATE_UINT64(cmd_slot_dma, struct PVRDMAMigTmp),
+            VMSTATE_UINT64(resp_slot_dma, struct PVRDMAMigTmp),
+            VMSTATE_UINT32(async_ring_pages_num_pages, struct PVRDMAMigTmp),
+            VMSTATE_UINT64(async_ring_pages_pdir_dma, struct PVRDMAMigTmp),
+            VMSTATE_UINT32(cq_ring_pages_num_pages, struct PVRDMAMigTmp),
+            VMSTATE_UINT64(cq_ring_pages_pdir_dma, struct PVRDMAMigTmp),
+            VMSTATE_END_OF_LIST()
+    }
+};
+
+static const VMStateDescription vmstate_pvrdma = {
+    .name = "pvrdma",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+            VMSTATE_PCI_DEVICE(parent_obj, PVRDMADev),
+            VMSTATE_MSIX(parent_obj, PVRDMADev),
+            VMSTATE_WITH_TMP(PVRDMADev,
+                             struct PVRDMAMigTmp,
+                             vmstate_pvrdma_dsr_dma),
+            VMSTATE_END_OF_LIST()
+    }
+};
+
 static void pvrdma_realize(PCIDevice *pdev, Error **errp)
 {
     int rc = 0;
@@ -688,6 +774,7 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
 
     dc->desc = "RDMA Device";
     dc->props = pvrdma_dev_properties;
+    dc->vmsd = &vmstate_pvrdma;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
 
     ir->print_statistics = pvrdma_print_statistics;
-- 
2.21.0

Re: [Qemu-devel] [RFC v2 2/2] hw/pvrdma: add live migration support

[Yuval Shaia]

On Sat, Jul 06, 2019 at 09:39:40AM +0530, Sukrit Bhatnagar wrote:
> Use VMStateDescription for migrating device state. Currently,

What do you mean by 'Currently'?

> 'vmstate_pvrdma' describes the PCI and MSIX state for pvrdma and
> 'vmstate_pvrdma_dsr_dma' describes a temporary state containing
> some values obtained only after mapping to dsr in the source.
> Since the dsr will not be available on dest until we map to the
> dma address we had on source, these values cannot be migrated
> directly.
> 
> Add PVRDMAMigTmp to store this temporary state which consists of
> dma addresses and ring page information. The 'parent' member is
> used to refer to the device state (PVRDMADev) so that parent PCI
> device object is accessible, which is needed to remap to DSR.
> 
> pvrdma_dsr_dma_pre_save() saves the dsr state into this temporary
> representation and pvrdma_dsr_dma_post_load() loads it back.
> This load function also remaps to the dsr and and calls
> load_dsr() for further map and ring init operations.
> 
> Please note that this call to load_dsr() can be removed from the
> migration flow and included in pvrdma_regs_write() to perform a
> lazy load.

The lazy load was suggested to overcome a potential problem with mapping to
addresses while still in migration process. With that been solved i would
suggest to drop the idea of lazy load.

> As of now, migration will fail if there in an error in load_dsr().
> Also, there might be a considerable amount of pages in the rings,
> which will have dma map operations when the init functions are
> called.
> If this takes noticeable time, it might be better to have lazy
> load instead.

Yeah, make sense but i hope we will not get to this.

> 
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Cc: Yuval Shaia <yuval.shaia@oracle.com>
> Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> ---
>  hw/rdma/vmw/pvrdma_main.c | 87 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index 6c90db96f9..4a10bd2fc7 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -28,6 +28,7 @@
>  #include "sysemu/sysemu.h"
>  #include "monitor/monitor.h"
>  #include "hw/rdma/rdma.h"
> +#include "migration/register.h"
>  
>  #include "../rdma_rm.h"
>  #include "../rdma_backend.h"
> @@ -593,6 +594,91 @@ static void pvrdma_shutdown_notifier(Notifier *n, void *opaque)
>      pvrdma_fini(pci_dev);
>  }
>  
> +struct PVRDMAMigTmp {
> +    PVRDMADev *parent;
> +    uint64_t dma;
> +    uint64_t cmd_slot_dma;
> +    uint64_t resp_slot_dma;
> +    uint32_t cq_ring_pages_num_pages;
> +    uint64_t cq_ring_pages_pdir_dma;
> +    uint32_t async_ring_pages_num_pages;
> +    uint64_t async_ring_pages_pdir_dma;
> +};
> +
> +static int pvrdma_dsr_dma_pre_save(void *opaque)
> +{
> +    struct PVRDMAMigTmp *tmp = opaque;

For me tmp sounds like a very bad name, if it is the convention then i can
live with that, anyways suggesting something like mig_tmp_info or something
like that.

> +    DSRInfo *dsr_info = &tmp->parent->dsr_info;

Can you shad some light on how the parent field is initialized with the
pointer to the device object?

> +    struct pvrdma_device_shared_region *dsr = dsr_info->dsr;
> +
> +    tmp->dma = dsr_info->dma;
> +    tmp->cmd_slot_dma = dsr->cmd_slot_dma;
> +    tmp->resp_slot_dma = dsr->resp_slot_dma;
> +    tmp->cq_ring_pages_num_pages = dsr->cq_ring_pages.num_pages;
> +    tmp->cq_ring_pages_pdir_dma = dsr->cq_ring_pages.pdir_dma;
> +    tmp->async_ring_pages_num_pages = dsr->async_ring_pages.num_pages;
> +    tmp->async_ring_pages_pdir_dma = dsr->async_ring_pages.pdir_dma;
> +
> +    return 0;
> +}
> +
> +static int pvrdma_dsr_dma_post_load(void *opaque, int version_id)
> +{
> +    struct PVRDMAMigTmp *tmp = opaque;
> +    PVRDMADev *dev = tmp->parent;
> +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> +    DSRInfo *dsr_info = &dev->dsr_info;
> +    struct pvrdma_device_shared_region *dsr;
> +
> +    dsr_info->dma = tmp->dma;
> +    dsr_info->dsr = rdma_pci_dma_map(pci_dev, dsr_info->dma,
> +                                sizeof(struct pvrdma_device_shared_region));
> +    if (!dsr_info->dsr) {
> +        rdma_error_report("Failed to map to DSR");
> +        return -ENOMEM;

Will this cause the VM on source host to continue functioning besides
failing the migration?

> +    }
> +
> +    dsr = dsr_info->dsr;
> +    dsr->cmd_slot_dma = tmp->cmd_slot_dma;
> +    dsr->resp_slot_dma = tmp->resp_slot_dma;
> +    dsr->cq_ring_pages.num_pages = tmp->cq_ring_pages_num_pages;
> +    dsr->cq_ring_pages.pdir_dma = tmp->cq_ring_pages_pdir_dma;
> +    dsr->async_ring_pages.num_pages = tmp->async_ring_pages_num_pages;
> +    dsr->async_ring_pages.pdir_dma = tmp->async_ring_pages_pdir_dma;

I expect the above 6 fields to be already populated with the correct values
as we just map to driver's DSR that should be migrated as part of memory
copy of source to dest host.
Can you verify it and if my assumptions are correct to remove these
assignments (and the corresponding from pre_save)?

> +
> +    return load_dsr(dev);
> +}
> +
> +static const VMStateDescription vmstate_pvrdma_dsr_dma = {
> +    .name = "pvrdma-dsr-dma",
> +    .pre_save = pvrdma_dsr_dma_pre_save,
> +    .post_load = pvrdma_dsr_dma_post_load,

I'm looking for a hook that is triggered just before leaving the source
host so we can do some needed cleanups such as unmapping the DSR, removing
IP addresses from the host etc.

> +    .fields = (VMStateField[]) {
> +            VMSTATE_UINT64(dma, struct PVRDMAMigTmp),
> +            VMSTATE_UINT64(cmd_slot_dma, struct PVRDMAMigTmp),
> +            VMSTATE_UINT64(resp_slot_dma, struct PVRDMAMigTmp),
> +            VMSTATE_UINT32(async_ring_pages_num_pages, struct PVRDMAMigTmp),
> +            VMSTATE_UINT64(async_ring_pages_pdir_dma, struct PVRDMAMigTmp),
> +            VMSTATE_UINT32(cq_ring_pages_num_pages, struct PVRDMAMigTmp),
> +            VMSTATE_UINT64(cq_ring_pages_pdir_dma, struct PVRDMAMigTmp),
> +            VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static const VMStateDescription vmstate_pvrdma = {
> +    .name = "pvrdma",

Suggesting to use the already defined constant PVRDMA_HW_NAME.

> +    .version_id = 1,
> +    .minimum_version_id = 1,

Hmmm...interesting, what's the use of these fields?

> +    .fields = (VMStateField[]) {
> +            VMSTATE_PCI_DEVICE(parent_obj, PVRDMADev),
> +            VMSTATE_MSIX(parent_obj, PVRDMADev),
> +            VMSTATE_WITH_TMP(PVRDMADev,
> +                             struct PVRDMAMigTmp,
> +                             vmstate_pvrdma_dsr_dma),
> +            VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void pvrdma_realize(PCIDevice *pdev, Error **errp)
>  {
>      int rc = 0;
> @@ -688,6 +774,7 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
>  
>      dc->desc = "RDMA Device";
>      dc->props = pvrdma_dev_properties;
> +    dc->vmsd = &vmstate_pvrdma;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);

Besides the above comments looks like a great job, thanks!

>  
>      ir->print_statistics = pvrdma_print_statistics;
> -- 
> 2.21.0
> 
> 

Re: [Qemu-devel] [RFC v2 2/2] hw/pvrdma: add live migration support

[Sukrit Bhatnagar]

On Mon, 8 Jul 2019 at 10:43, Yuval Shaia <yuval.shaia@oracle.com> wrote:
>
> On Sat, Jul 06, 2019 at 09:39:40AM +0530, Sukrit Bhatnagar wrote:
> > Use VMStateDescription for migrating device state. Currently,
>
> What do you mean by 'Currently'?

I meant that 'vmstate_pvrdma' will contain more fields later. This is
how it looks till now.

> > 'vmstate_pvrdma' describes the PCI and MSIX state for pvrdma and
> > 'vmstate_pvrdma_dsr_dma' describes a temporary state containing
> > some values obtained only after mapping to dsr in the source.
> > Since the dsr will not be available on dest until we map to the
> > dma address we had on source, these values cannot be migrated
> > directly.
> >
> > Add PVRDMAMigTmp to store this temporary state which consists of
> > dma addresses and ring page information. The 'parent' member is
> > used to refer to the device state (PVRDMADev) so that parent PCI
> > device object is accessible, which is needed to remap to DSR.
> >
> > pvrdma_dsr_dma_pre_save() saves the dsr state into this temporary
> > representation and pvrdma_dsr_dma_post_load() loads it back.
> > This load function also remaps to the dsr and and calls
> > load_dsr() for further map and ring init operations.
> >
> > Please note that this call to load_dsr() can be removed from the
> > migration flow and included in pvrdma_regs_write() to perform a
> > lazy load.
>
> The lazy load was suggested to overcome a potential problem with mapping to
> addresses while still in migration process. With that been solved i would
> suggest to drop the idea of lazy load.
>
> > As of now, migration will fail if there in an error in load_dsr().
> > Also, there might be a considerable amount of pages in the rings,
> > which will have dma map operations when the init functions are
> > called.
> > If this takes noticeable time, it might be better to have lazy
> > load instead.
>
> Yeah, make sense but i hope we will not get to this.
>
> >
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Cc: Yuval Shaia <yuval.shaia@oracle.com>
> > Signed-off-by: Sukrit Bhatnagar <skrtbhtngr@gmail.com>
> > ---
> >  hw/rdma/vmw/pvrdma_main.c | 87 +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 87 insertions(+)
> >
> > diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> > index 6c90db96f9..4a10bd2fc7 100644
> > --- a/hw/rdma/vmw/pvrdma_main.c
> > +++ b/hw/rdma/vmw/pvrdma_main.c
> > @@ -28,6 +28,7 @@
> >  #include "sysemu/sysemu.h"
> >  #include "monitor/monitor.h"
> >  #include "hw/rdma/rdma.h"
> > +#include "migration/register.h"
> >
> >  #include "../rdma_rm.h"
> >  #include "../rdma_backend.h"
> > @@ -593,6 +594,91 @@ static void pvrdma_shutdown_notifier(Notifier *n, void *opaque)
> >      pvrdma_fini(pci_dev);
> >  }
> >
> > +struct PVRDMAMigTmp {
> > +    PVRDMADev *parent;
> > +    uint64_t dma;
> > +    uint64_t cmd_slot_dma;
> > +    uint64_t resp_slot_dma;
> > +    uint32_t cq_ring_pages_num_pages;
> > +    uint64_t cq_ring_pages_pdir_dma;
> > +    uint32_t async_ring_pages_num_pages;
> > +    uint64_t async_ring_pages_pdir_dma;
> > +};
> > +
> > +static int pvrdma_dsr_dma_pre_save(void *opaque)
> > +{
> > +    struct PVRDMAMigTmp *tmp = opaque;
>
> For me tmp sounds like a very bad name, if it is the convention then i can
> live with that, anyways suggesting something like mig_tmp_info or something
> like that.

It doesn't seem like a convention, but this is how it is named
in other places in the source. But sure, I'll change it to a more
meaningful name, if it is needed.

> > +    DSRInfo *dsr_info = &tmp->parent->dsr_info;
>
> Can you shad some light on how the parent field is initialized with the
> pointer to the device object?

TL;DR
The trick is on line 567 in migration/vmstate-types.c in the
function 'put_tmp'.


Each VMStateDescription has one or more VMStateField values
which are used to define members of the device state for migration.

VMSTATE_WITH_TMP is a macro which defines a VMStateField for
those types of device state which need an intermediate representation
and cannot be migrated directly. In our case, this might apply to the
dma addresses and pdir information stored within dsr.

Each VMStateField has a corresponding VMStateInfo, which basically
is an interface declaring two functions, get and put. These functions are
used to describe how this certain type of VMStateField has to be
saved/loaded during migration.

VMSTATE_WITH_TMP has its VMStateInfo set to 'vmstate_info_tmp',
which assigns the functions 'put_tmp' and 'get_tmp' for save/load logic.

Lets consider state save during migration.
All the state saving work is done inside 'vmstate_save_state_v'.
It takes a VMStateDescription as 2nd argument and an opaque type
as 3rd argument.

When 'vmstate_save_state_v' is first called during migration, the
VMStateDescription argument is set to 'vmstate_pvrdma', and
opaque is set to our instance of PVRDMADev.

This function iteratively saves the VMStateFields; PCI then MSIX
then the WITH_TMP.
When it reaches WITH_TMP, it falls through the if-else conditions,
and finally reaches field->info->put. [migration/vmstate.c:386]
This put function is set to 'put_tmp' mentioned above.

Now, the 'put_tmp' function is called, with 'pv' parameter set to the
'parent' field of PVRDMAMigTmp, and 'field' set to our WITH_TMP.
This is because 'parent' is the first member of PVRDMAMigTmp,
and both first_elem and curr_elem (inside 'vmstate_save_state_v')
will point to it when the while loop reaches WITH_TMP field.

'put_tmp' will set the first element of 'tmp' to 'pv' and call
'vmstate_save_state*' with opaque parameter set to this 'tmp', and
the VMStateDescription set to 'vmstate_pvrdma_dsr_dma'.

> > +    struct pvrdma_device_shared_region *dsr = dsr_info->dsr;
> > +
> > +    tmp->dma = dsr_info->dma;
> > +    tmp->cmd_slot_dma = dsr->cmd_slot_dma;
> > +    tmp->resp_slot_dma = dsr->resp_slot_dma;
> > +    tmp->cq_ring_pages_num_pages = dsr->cq_ring_pages.num_pages;
> > +    tmp->cq_ring_pages_pdir_dma = dsr->cq_ring_pages.pdir_dma;
> > +    tmp->async_ring_pages_num_pages = dsr->async_ring_pages.num_pages;
> > +    tmp->async_ring_pages_pdir_dma = dsr->async_ring_pages.pdir_dma;
> > +
> > +    return 0;
> > +}
> > +
> > +static int pvrdma_dsr_dma_post_load(void *opaque, int version_id)
> > +{
> > +    struct PVRDMAMigTmp *tmp = opaque;
> > +    PVRDMADev *dev = tmp->parent;
> > +    PCIDevice *pci_dev = PCI_DEVICE(dev);
> > +    DSRInfo *dsr_info = &dev->dsr_info;
> > +    struct pvrdma_device_shared_region *dsr;
> > +
> > +    dsr_info->dma = tmp->dma;
> > +    dsr_info->dsr = rdma_pci_dma_map(pci_dev, dsr_info->dma,
> > +                                sizeof(struct pvrdma_device_shared_region));
> > +    if (!dsr_info->dsr) {
> > +        rdma_error_report("Failed to map to DSR");
> > +        return -ENOMEM;
>
> Will this cause the VM on source host to continue functioning besides
> failing the migration?

No, the VM on the source halts.

> > +    }
> > +
> > +    dsr = dsr_info->dsr;
> > +    dsr->cmd_slot_dma = tmp->cmd_slot_dma;
> > +    dsr->resp_slot_dma = tmp->resp_slot_dma;
> > +    dsr->cq_ring_pages.num_pages = tmp->cq_ring_pages_num_pages;
> > +    dsr->cq_ring_pages.pdir_dma = tmp->cq_ring_pages_pdir_dma;
> > +    dsr->async_ring_pages.num_pages = tmp->async_ring_pages_num_pages;
> > +    dsr->async_ring_pages.pdir_dma = tmp->async_ring_pages_pdir_dma;
>
> I expect the above 6 fields to be already populated with the correct values
> as we just map to driver's DSR that should be migrated as part of memory
> copy of source to dest host.
> Can you verify it and if my assumptions are correct to remove these
> assignments (and the corresponding from pre_save)?

Yes, you are right. We do not need these 6 fields. In fact, we don't need
vmstate_pvrdma_dsr_dma and PVRDMAMigTmp as of now; we just need the
save/load logic in vmstate_pvrdma.

I had added these fields based on the BounceBuffer issue we had earlier.

> > +
> > +    return load_dsr(dev);
> > +}
> > +
> > +static const VMStateDescription vmstate_pvrdma_dsr_dma = {
> > +    .name = "pvrdma-dsr-dma",
> > +    .pre_save = pvrdma_dsr_dma_pre_save,
> > +    .post_load = pvrdma_dsr_dma_post_load,
>
> I'm looking for a hook that is triggered just before leaving the source
> host so we can do some needed cleanups such as unmapping the DSR, removing
> IP addresses from the host etc.

There is a 'post_save' function available which is called on the src when
the fields are saved into the migration stream. We can try that.

> > +    .fields = (VMStateField[]) {
> > +            VMSTATE_UINT64(dma, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT64(cmd_slot_dma, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT64(resp_slot_dma, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT32(async_ring_pages_num_pages, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT64(async_ring_pages_pdir_dma, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT32(cq_ring_pages_num_pages, struct PVRDMAMigTmp),
> > +            VMSTATE_UINT64(cq_ring_pages_pdir_dma, struct PVRDMAMigTmp),
> > +            VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> > +static const VMStateDescription vmstate_pvrdma = {
> > +    .name = "pvrdma",
>
> Suggesting to use the already defined constant PVRDMA_HW_NAME.

Noted.

> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
>
> Hmmm...interesting, what's the use of these fields?

These are used to indicate any major changes to the device state
for migration such that if versions do not match at all, migration
will fail for the device.
Removing these also does not make any difference in the behaviour
of our migration, since we are implementing it from scratch currently.
I had added these because it seemed like convention.

> > +    .fields = (VMStateField[]) {
> > +            VMSTATE_PCI_DEVICE(parent_obj, PVRDMADev),
> > +            VMSTATE_MSIX(parent_obj, PVRDMADev),
> > +            VMSTATE_WITH_TMP(PVRDMADev,
> > +                             struct PVRDMAMigTmp,
> > +                             vmstate_pvrdma_dsr_dma),
> > +            VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static void pvrdma_realize(PCIDevice *pdev, Error **errp)
> >  {
> >      int rc = 0;
> > @@ -688,6 +774,7 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
> >
> >      dc->desc = "RDMA Device";
> >      dc->props = pvrdma_dev_properties;
> > +    dc->vmsd = &vmstate_pvrdma;
> >      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
>
> Besides the above comments looks like a great job, thanks!
>
> >
> >      ir->print_statistics = pvrdma_print_statistics;
> > --
> > 2.21.0
> >
> >

Re: [Qemu-devel] [RFC v2 0/2] Add live migration support in the PVRDMA device

[Marcel Apfelbaum]

Hi Sukrit,

On 7/6/19 7:09 AM, Sukrit Bhatnagar wrote:
> Changes in v2:
>
> * Modify load_dsr() such that dsr mapping is not performed if dsr value
>    is non-NULL. Also move free_dsr() out of load_dsr() and call it right
>    before if needed. These two changes will allow us to call load_dsr()
>    even when we have already done dsr mapping and would like to go on
>    with the rest of mappings.
>
> * Use VMStateDescription instead of SaveVMHandlers to describe migration
>    state. Also add fields for parent PCI object and MSIX.
>
> * Use a temporary structure (struct PVRDMAMigTmp) to hold some fields
>    during migration. These fields, such as cmd_slot_dma and resp_slot_dma
>    inside dsr, do not fit into VMSTATE macros as their container
>    (dsr_info->dsr) will not be ready until it is mapped on the dest.
>
> * Perform mappings to CQ and event notification rings after the state is
>    loaded. This is an extension to the mappings performed in v1;
>    following the flow of load_dsr(). All the mappings are succesfully
>    done on the dest on state load.

Nice!

> Link(s) to v1:
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04924.html
> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04923.html
>
>
> Things working now (were not working at the time of v1):
>
> * vmxnet3 is migrating successfully. The issue was in the migration of
>    its PCI configuration space, and is solved by the patch Marcel had sent:
>    https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01500.html
>
> * There is no problem due to BounceBuffers which were failing the dma mapping
>    calls in state load logic earlier. Not sure exactly how it went away. I am
>    guessing that adding the PCI and MSIX state to migration solved the issue.
>

I am sure it was connected somehow, anyway, I am glad we can continue
with the project.

> What is still needed:
>
> * A workaround to get libvirt to support same-host migration. Since
>    the problems faced in v1 (mentioned above) are out of the way, we
>    can move further, and in doing so, we will need this.

[Adding Daniel  and Michal]
Is there anyway to test live-migration for libvirt domains on the same host?
Even a 'hack' would be enough.

Sukrit, another way you could do it is by enabling nested virtualization
and have 2 Vms as hosts. I suppose the migration will take some time 
though...


Thanks,
Marcel

> Sukrit Bhatnagar (2):
>    hw/pvrdma: make DSR mapping idempotent in load_dsr()
>    hw/pvrdma: add live migration support
>
>   hw/rdma/vmw/pvrdma_main.c | 104 +++++++++++++++++++++++++++++++++++---
>   1 file changed, 96 insertions(+), 8 deletions(-)
>

Re: [Qemu-devel] [RFC v2 0/2] Add live migration support in the PVRDMA device

[Daniel P. Berrangé]

On Sat, Jul 06, 2019 at 10:04:55PM +0300, Marcel Apfelbaum wrote:
> Hi Sukrit,
> 
> On 7/6/19 7:09 AM, Sukrit Bhatnagar wrote:
> > Changes in v2:
> > 
> > * Modify load_dsr() such that dsr mapping is not performed if dsr value
> >    is non-NULL. Also move free_dsr() out of load_dsr() and call it right
> >    before if needed. These two changes will allow us to call load_dsr()
> >    even when we have already done dsr mapping and would like to go on
> >    with the rest of mappings.
> > 
> > * Use VMStateDescription instead of SaveVMHandlers to describe migration
> >    state. Also add fields for parent PCI object and MSIX.
> > 
> > * Use a temporary structure (struct PVRDMAMigTmp) to hold some fields
> >    during migration. These fields, such as cmd_slot_dma and resp_slot_dma
> >    inside dsr, do not fit into VMSTATE macros as their container
> >    (dsr_info->dsr) will not be ready until it is mapped on the dest.
> > 
> > * Perform mappings to CQ and event notification rings after the state is
> >    loaded. This is an extension to the mappings performed in v1;
> >    following the flow of load_dsr(). All the mappings are succesfully
> >    done on the dest on state load.
> 
> Nice!
> 
> > Link(s) to v1:
> > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04924.html
> > https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04923.html
> > 
> > 
> > Things working now (were not working at the time of v1):
> > 
> > * vmxnet3 is migrating successfully. The issue was in the migration of
> >    its PCI configuration space, and is solved by the patch Marcel had sent:
> >    https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01500.html
> > 
> > * There is no problem due to BounceBuffers which were failing the dma mapping
> >    calls in state load logic earlier. Not sure exactly how it went away. I am
> >    guessing that adding the PCI and MSIX state to migration solved the issue.
> > 
> 
> I am sure it was connected somehow, anyway, I am glad we can continue
> with the project.
> 
> > What is still needed:
> > 
> > * A workaround to get libvirt to support same-host migration. Since
> >    the problems faced in v1 (mentioned above) are out of the way, we
> >    can move further, and in doing so, we will need this.
> 
> [Adding Daniel  and Michal]
> Is there anyway to test live-migration for libvirt domains on the same host?
> Even a 'hack' would be enough.

Create two VMs on your host & run inside those. Or create two containers
if you want a lighter weight solution. You must have two completely
independant libvirtd instances, sharing nothing, except optionally where
you store disk images.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [RFC v2 0/2] Add live migration support in the PVRDMA device

[Marcel Apfelbaum]

On 7/8/19 12:38 PM, Daniel P. Berrangé wrote:
> On Sat, Jul 06, 2019 at 10:04:55PM +0300, Marcel Apfelbaum wrote:
>> Hi Sukrit,
>>
>> On 7/6/19 7:09 AM, Sukrit Bhatnagar wrote:
>>> Changes in v2:
>>>
>>> * Modify load_dsr() such that dsr mapping is not performed if dsr value
>>>     is non-NULL. Also move free_dsr() out of load_dsr() and call it right
>>>     before if needed. These two changes will allow us to call load_dsr()
>>>     even when we have already done dsr mapping and would like to go on
>>>     with the rest of mappings.
>>>
>>> * Use VMStateDescription instead of SaveVMHandlers to describe migration
>>>     state. Also add fields for parent PCI object and MSIX.
>>>
>>> * Use a temporary structure (struct PVRDMAMigTmp) to hold some fields
>>>     during migration. These fields, such as cmd_slot_dma and resp_slot_dma
>>>     inside dsr, do not fit into VMSTATE macros as their container
>>>     (dsr_info->dsr) will not be ready until it is mapped on the dest.
>>>
>>> * Perform mappings to CQ and event notification rings after the state is
>>>     loaded. This is an extension to the mappings performed in v1;
>>>     following the flow of load_dsr(). All the mappings are succesfully
>>>     done on the dest on state load.
>> Nice!
>>
>>> Link(s) to v1:
>>> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04924.html
>>> https://lists.gnu.org/archive/html/qemu-devel/2019-06/msg04923.html
>>>
>>>
>>> Things working now (were not working at the time of v1):
>>>
>>> * vmxnet3 is migrating successfully. The issue was in the migration of
>>>     its PCI configuration space, and is solved by the patch Marcel had sent:
>>>     https://lists.gnu.org/archive/html/qemu-devel/2019-07/msg01500.html
>>>
>>> * There is no problem due to BounceBuffers which were failing the dma mapping
>>>     calls in state load logic earlier. Not sure exactly how it went away. I am
>>>     guessing that adding the PCI and MSIX state to migration solved the issue.
>>>
>> I am sure it was connected somehow, anyway, I am glad we can continue
>> with the project.
>>
>>> What is still needed:
>>>
>>> * A workaround to get libvirt to support same-host migration. Since
>>>     the problems faced in v1 (mentioned above) are out of the way, we
>>>     can move further, and in doing so, we will need this.
>> [Adding Daniel  and Michal]
>> Is there anyway to test live-migration for libvirt domains on the same host?
>> Even a 'hack' would be enough.
> Create two VMs on your host & run inside those. Or create two containers
> if you want a lighter weight solution. You must have two completely
> independant libvirtd instances, sharing nothing, except optionally where
> you store disk images.

We'll work with live-cd, no storage is needed.

Thank you for the help!
Marcel

> Regards,
> Daniel