[PATCH v3 1/2] qemu: add support for shmem-{plain, doorbell} role

[Wangxin (Alexander)]

> On Mon, Aug 03, 2020 at 09:26:19AM +0000, Wangxin (Alexander) wrote:
> >> On Fri, Jul 24, 2020 at 11:34:11AM +0800, Wang Xin wrote:
> >> >Role(master or peer) controls how the domain behaves on migration.
> >> >For more details about migration with ivshmem, see
> >> >https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/system/ivshmem.rst;hb=HEAD
> >> >
> >> >It's a optional attribute in libvirt, and qemu will choose default
> >> >role for ivshmem device if the user is not specified.
> >> >
> >> >With device property 'role', the value can be 'master' or 'peer'.
> >> > - 'master' (means 'master=on' in qemu), the guest will copy
> >> >   the shared memory on migration to the destination host.
> >> > - 'peer' (means 'master=off' in qemu), the migration is disabled.
> >> >
> >> >Signed-off-by: Martin Kletzander <mkletzan at redhat.com>
> >> >Signed-off-by: Yang Hang <yanghang44 at huawei.com>
> >> >Signed-off-by: Wang Xin <wangxinxin.wang at huawei.com>
> >> >
> >> >diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
> >> >index 2c7bf349c3..e588b74c2f 100644
> >> >--- a/src/qemu/qemu_migration.c
> >> >+++ b/src/qemu/qemu_migration.c
> >> >@@ -1261,10 +1261,22 @@ qemuMigrationSrcIsAllowed(virQEMUDriverPtr driver,
> >> >             }
> >> >         }
> >> >
> >> >-        if (vm->def->nshmems) {
> >> >-            virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> >> >-                           _("migration with shmem device is not supported"));
> >> >-            return false;
> >> >+        for (i = 0; i < vm->def->nshmems; i++) {
> >> >+            virDomainShmemDefPtr shmem = vm->def->shmems[i];
> >> >+
> >> >+            if (shmem->model == VIR_DOMAIN_SHMEM_MODEL_IVSHMEM) {
> >> >+                virReportError(VIR_ERR_OPERATION_INVALID, "%s",
> >> >+                               _("migration with legacy shmem device is not supported"));
> >> >+                return false;
> >> >+            }
> >> >+            if (shmem->role == VIR_DOMAIN_SHMEM_ROLE_PEER) {
> >>
> >> This allows migration with the default role which you leave to be chosen by the
> >> hypervisor.  In that case we need to change this to:
> >>
> >>    if (shmem->role != VIR_DOMAIN_SHMEM_ROLE_MASTER) {
> >
> >In fact, the hypervisor will also check weather the ivshmem device is migratable(by
> >using migration blocker) or not.
> >If 'role' is not specified in libvirt, the default role may 'master' or 'peer', we don't kown it,
> >so, I think it's better to check it in hypervisor.
> >
> 
> We strive to prevent any errors in QEMU "just in case".  Users could start
> doubting our backwards compatibility if their `default` role shmem stopped
> migrating in the future, for example.  And by being strict we avoid any possible
> future issues like this.
> 

Get it, thanks for explaining. :)

> >>
> >> >+                virReportError(VIR_ERR_OPERATION_INVALID,
> >> >+                               _("shmem device '%s' with role='%s', "
> >> >+                                 "cannot be migrated"),
> >>
> >> and this accordingly.
> >>
> >> But that's fine, I can change that before pushing.  With that change:

Sure.

> >>
> >> Reviewed-by: Martin Kletzander <mkletzan at redhat.com>
> >>
> >> but I'll wait after the release since this is a feature.
> >
> >

Thanks for your review!
Best regards, Alexander.