[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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 redhat com>
>Signed-off-by: Yang Hang <yanghang44 huawei com>
>Signed-off-by: Wang Xin <wangxinxin wang 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.


>+                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:

Reviewed-by: Martin Kletzander <mkletzan redhat com>

but I'll wait after the release since this is a feature.


Attachment: signature.asc
Description: PGP signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]