[Qemu-devel] [PATCH] ivshmem: Fix unplug of device "ivshmem-plain"

Markus Armbruster posted 1 patch 5 years, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180926163709.22876-1-armbru@redhat.com
Test docker-clang@ubuntu failed
Test checkpatch passed
hw/misc/ivshmem.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
[Qemu-devel] [PATCH] ivshmem: Fix unplug of device "ivshmem-plain"
Posted by Markus Armbruster 5 years, 6 months ago
Commit 2aece63c8a "hostmem: detect host backend memory is being used
properly" fixed "ivshmem-plain" to reject memory backends that are
already in use, and to block their deletion while in use.  Two bugs
escaped review:

* New ivshmem_plain_exit() fails to call ivshmem_exit().  This breaks
  unplug.  Reproducer: migration after unplug still fails with
  "Migration is disabled when using feature 'peer mode' in device
  'ivshmem'".

* It failed to update legacy "ivshmem".  Harmless, because it creates
  the memory backend itself, and nothing else should use it.

Fix by moving the two host_memory_backend_set_mapped() calls into
ivshmem_common_realize() and ivshmem_exit(), guarded by s->hostmem.

Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 6febbabcaa..8cb17b9dd4 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -911,6 +911,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
         IVSHMEM_DPRINTF("using hostmem\n");
 
         s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
+        host_memory_backend_set_mapped(s->hostmem, true);
     } else {
         Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
         assert(chr);
@@ -993,6 +994,10 @@ static void ivshmem_exit(PCIDevice *dev)
         vmstate_unregister_ram(s->ivshmem_bar2, DEVICE(dev));
     }
 
+    if (s->hostmem) {
+        host_memory_backend_set_mapped(s->hostmem, false);
+    }
+
     if (s->peers) {
         for (i = 0; i < s->nb_peers; i++) {
             close_peer_eventfds(s, i);
@@ -1101,14 +1106,6 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
     }
 
     ivshmem_common_realize(dev, errp);
-    host_memory_backend_set_mapped(s->hostmem, true);
-}
-
-static void ivshmem_plain_exit(PCIDevice *pci_dev)
-{
-    IVShmemState *s = IVSHMEM_COMMON(pci_dev);
-
-    host_memory_backend_set_mapped(s->hostmem, false);
 }
 
 static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
@@ -1117,7 +1114,6 @@ static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
     k->realize = ivshmem_plain_realize;
-    k->exit = ivshmem_plain_exit;
     dc->props = ivshmem_plain_properties;
     dc->vmsd = &ivshmem_plain_vmsd;
 }
-- 
2.17.1


Re: [Qemu-devel] [PATCH] ivshmem: Fix unplug of device "ivshmem-plain"
Posted by Paolo Bonzini 5 years, 5 months ago
On 26/09/2018 18:37, Markus Armbruster wrote:
> Commit 2aece63c8a "hostmem: detect host backend memory is being used
> properly" fixed "ivshmem-plain" to reject memory backends that are
> already in use, and to block their deletion while in use.  Two bugs
> escaped review:
> 
> * New ivshmem_plain_exit() fails to call ivshmem_exit().  This breaks
>   unplug.  Reproducer: migration after unplug still fails with
>   "Migration is disabled when using feature 'peer mode' in device
>   'ivshmem'".
> 
> * It failed to update legacy "ivshmem".  Harmless, because it creates
>   the memory backend itself, and nothing else should use it.
> 
> Fix by moving the two host_memory_backend_set_mapped() calls into
> ivshmem_common_realize() and ivshmem_exit(), guarded by s->hostmem.
> 
> Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/misc/ivshmem.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 6febbabcaa..8cb17b9dd4 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -911,6 +911,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>          IVSHMEM_DPRINTF("using hostmem\n");
>  
>          s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
> +        host_memory_backend_set_mapped(s->hostmem, true);
>      } else {
>          Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
>          assert(chr);
> @@ -993,6 +994,10 @@ static void ivshmem_exit(PCIDevice *dev)
>          vmstate_unregister_ram(s->ivshmem_bar2, DEVICE(dev));
>      }
>  
> +    if (s->hostmem) {
> +        host_memory_backend_set_mapped(s->hostmem, false);
> +    }
> +
>      if (s->peers) {
>          for (i = 0; i < s->nb_peers; i++) {
>              close_peer_eventfds(s, i);
> @@ -1101,14 +1106,6 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
>      }
>  
>      ivshmem_common_realize(dev, errp);
> -    host_memory_backend_set_mapped(s->hostmem, true);
> -}
> -
> -static void ivshmem_plain_exit(PCIDevice *pci_dev)
> -{
> -    IVShmemState *s = IVSHMEM_COMMON(pci_dev);
> -
> -    host_memory_backend_set_mapped(s->hostmem, false);
>  }
>  
>  static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
> @@ -1117,7 +1114,6 @@ static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>  
>      k->realize = ivshmem_plain_realize;
> -    k->exit = ivshmem_plain_exit;
>      dc->props = ivshmem_plain_properties;
>      dc->vmsd = &ivshmem_plain_vmsd;
>  }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Thanks for the fix!

Paolo

Re: [Qemu-devel] [PATCH] ivshmem: Fix unplug of device "ivshmem-plain"
Posted by Marc-André Lureau 5 years, 6 months ago
Hi

On Wed, Sep 26, 2018 at 8:37 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Commit 2aece63c8a "hostmem: detect host backend memory is being used
> properly" fixed "ivshmem-plain" to reject memory backends that are
> already in use, and to block their deletion while in use.  Two bugs
> escaped review:
>
> * New ivshmem_plain_exit() fails to call ivshmem_exit().  This breaks
>   unplug.  Reproducer: migration after unplug still fails with
>   "Migration is disabled when using feature 'peer mode' in device
>   'ivshmem'".
>
> * It failed to update legacy "ivshmem".  Harmless, because it creates
>   the memory backend itself, and nothing else should use it.
>
> Fix by moving the two host_memory_backend_set_mapped() calls into
> ivshmem_common_realize() and ivshmem_exit(), guarded by s->hostmem.
>
> Fixes: 2aece63c8a9d2c3a8ff41d2febc4cdeff2633331
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/misc/ivshmem.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
>
> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> index 6febbabcaa..8cb17b9dd4 100644
> --- a/hw/misc/ivshmem.c
> +++ b/hw/misc/ivshmem.c
> @@ -911,6 +911,7 @@ static void ivshmem_common_realize(PCIDevice *dev, Error **errp)
>          IVSHMEM_DPRINTF("using hostmem\n");
>
>          s->ivshmem_bar2 = host_memory_backend_get_memory(s->hostmem);
> +        host_memory_backend_set_mapped(s->hostmem, true);
>      } else {
>          Chardev *chr = qemu_chr_fe_get_driver(&s->server_chr);
>          assert(chr);
> @@ -993,6 +994,10 @@ static void ivshmem_exit(PCIDevice *dev)
>          vmstate_unregister_ram(s->ivshmem_bar2, DEVICE(dev));
>      }
>
> +    if (s->hostmem) {
> +        host_memory_backend_set_mapped(s->hostmem, false);
> +    }
> +
>      if (s->peers) {
>          for (i = 0; i < s->nb_peers; i++) {
>              close_peer_eventfds(s, i);
> @@ -1101,14 +1106,6 @@ static void ivshmem_plain_realize(PCIDevice *dev, Error **errp)
>      }
>
>      ivshmem_common_realize(dev, errp);
> -    host_memory_backend_set_mapped(s->hostmem, true);
> -}
> -
> -static void ivshmem_plain_exit(PCIDevice *pci_dev)
> -{
> -    IVShmemState *s = IVSHMEM_COMMON(pci_dev);
> -
> -    host_memory_backend_set_mapped(s->hostmem, false);
>  }
>
>  static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
> @@ -1117,7 +1114,6 @@ static void ivshmem_plain_class_init(ObjectClass *klass, void *data)
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>
>      k->realize = ivshmem_plain_realize;
> -    k->exit = ivshmem_plain_exit;
>      dc->props = ivshmem_plain_properties;
>      dc->vmsd = &ivshmem_plain_vmsd;
>  }
> --
> 2.17.1
>
>


-- 
Marc-André Lureau