diff mbox

[29/38] ivshmem: Implement shm=... with a memory backend

Message ID 1456771254-17511-30-git-send-email-armbru@redhat.com
State New
Headers show

Commit Message

Markus Armbruster Feb. 29, 2016, 6:40 p.m. UTC
ivshmem has its very own code to create and map shared memory.
Replace that with an implicitly created memory backend.  Reduces the
number of ways we create BAR 2 from three to two.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 hw/misc/ivshmem.c | 89 +++++++++++++++++++++----------------------------------
 1 file changed, 33 insertions(+), 56 deletions(-)

Comments

Paolo Bonzini March 1, 2016, 11:37 a.m. UTC | #1
On 29/02/2016 19:40, Markus Armbruster wrote:
> ivshmem has its very own code to create and map shared memory.
> Replace that with an implicitly created memory backend.  Reduces the
> number of ways we create BAR 2 from three to two.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Very appreciated, but do not use user_creatable_add_opts.  Instead,
create the object with object_initialize, object_property_set_* and
user_creatable_complete.  After the object_initialize, add it with
object_property_add_child *under the ivshmem device itself*, giving it a
name like "internal-shm-backend".

This matches what virtio-blk dataplane used to do for x-dataplane (now
removed).

Thanks,

Paolo

> +static HostMemoryBackend *desugar_shm(const char *shm, size_t size)
> +{
> +    /* TODO avoid the detour through QemuOpts */
> +    static int counter;
> +    QemuOpts *opts = qemu_opts_create(qemu_find_opts("object"),
> +                                      NULL, 0, &error_abort);
> +    char *path;
> +    Object *obj;
> +
> +    qemu_opt_set(opts, "qom-type", "memory-backend-file",
> +    &error_abort);
> +    /* FIXME need a better way to make up an ID */
> +    qemu_opts_set_id(opts, g_strdup_printf("ivshmem-backend-%d", counter++));
> +    path = g_strdup_printf("/dev/shm/%s", shm);
> +    qemu_opt_set(opts, "mem-path", path, &error_abort);
> +    qemu_opt_set_number(opts, "size", size, &error_abort);
> +    qemu_opt_set_bool(opts, "share", true, &error_abort);
> +    g_free(path);
> +
> +    obj = user_creatable_add_opts(opts, &error_abort);
> +    qemu_opts_del(opts);
> +
> +    user_creatable_complete(obj, &error_abort);
> +
> +    return MEMORY_BACKEND(obj);
> +}
> +
>  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>  {
>      IVShmemState *s = IVSHMEM(dev);
> @@ -911,6 +914,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>          attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
>      }
>  
> +    if (s->shmobj) {
> +        s->hostmem = desugar_shm(s->shmobj, s->ivshmem_size);
> +    }
> +
>      if (s->hostmem != NULL) {
>          MemoryRegion *mr;
>  
> @@ -921,7 +928,7 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>          vmstate_register_ram(mr, DEVICE(s));
>          memory_region_add_subregion(&s->bar, 0, mr);
>          pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
> -    } else if (s->server_chr != NULL) {
> +    } else {
>          IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
>                          s->server_chr->filename);
>  
> @@ -948,36 +955,6 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
>              error_setg(errp, "failed to initialize interrupts");
>              return;
>          }
> -    } else {
> -        /* just map the file immediately, we're not using a server */
> -        int fd;
> -
> -        IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
> -
> -        /* try opening with O_EXCL and if it succeeds zero the memory
> -         * by truncating to 0 */
> -        if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
> -                        S_IRWXU|S_IRWXG|S_IRWXO)) > 0) {
> -           /* truncate file to length PCI device's memory */
> -            if (ftruncate(fd, s->ivshmem_size) != 0) {
> -                error_report("could not truncate shared file");
> -            }
> -
> -        } else if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR,
> -                        S_IRWXU|S_IRWXG|S_IRWXO)) < 0) {
> -            error_setg(errp, "could not open shared file");
> -            return;
> -        }
> -
> -        if (check_shm_size(s, fd, errp) == -1) {
> -            return;
> -        }
> -
> -        create_shared_memory_BAR(s, fd, attr, &err);
> -        if (err) {
> -            error_propagate(errp, err);
> -            return;
> -        }
>      }
>  
>      if (s->role_val == IVSHMEM_PEER) {
>
Markus Armbruster March 1, 2016, 12:08 p.m. UTC | #2
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 29/02/2016 19:40, Markus Armbruster wrote:
>> ivshmem has its very own code to create and map shared memory.
>> Replace that with an implicitly created memory backend.  Reduces the
>> number of ways we create BAR 2 from three to two.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>
> Very appreciated, but do not use user_creatable_add_opts.  Instead,
> create the object with object_initialize, object_property_set_* and
> user_creatable_complete.  After the object_initialize, add it with
> object_property_add_child *under the ivshmem device itself*, giving it a
> name like "internal-shm-backend".

Will do.

> This matches what virtio-blk dataplane used to do for x-dataplane (now
> removed).

Note to self: commit a616fb7, try to steal that code.

Thanks!
diff mbox

Patch

diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
index 8d54fa9..9931d5e 100644
--- a/hw/misc/ivshmem.c
+++ b/hw/misc/ivshmem.c
@@ -26,6 +26,7 @@ 
 #include "migration/migration.h"
 #include "qemu/error-report.h"
 #include "qemu/event_notifier.h"
+#include "qom/object_interfaces.h"
 #include "sysemu/char.h"
 #include "sysemu/hostmem.h"
 #include "qapi/visitor.h"
@@ -369,31 +370,6 @@  static int check_shm_size(IVShmemState *s, int fd, Error **errp)
     }
 }
 
-/* create the shared memory BAR when we are not using the server, so we can
- * create the BAR and map the memory immediately */
-static int create_shared_memory_BAR(IVShmemState *s, int fd, uint8_t attr,
-                                    Error **errp)
-{
-    void * ptr;
-
-    ptr = mmap(0, s->ivshmem_size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
-    if (ptr == MAP_FAILED) {
-        error_setg_errno(errp, errno, "Failed to mmap shared memory");
-        return -1;
-    }
-
-    memory_region_init_ram_ptr(&s->ivshmem, OBJECT(s), "ivshmem.bar2",
-                               s->ivshmem_size, ptr);
-    qemu_set_ram_fd(s->ivshmem.ram_addr, fd);
-    vmstate_register_ram(&s->ivshmem, DEVICE(s));
-    memory_region_add_subregion(&s->bar, 0, &s->ivshmem);
-
-    /* region for shared memory */
-    pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
-
-    return 0;
-}
-
 static void ivshmem_add_eventfd(IVShmemState *s, int posn, int i)
 {
     memory_region_add_eventfd(&s->ivshmem_mmio,
@@ -833,6 +809,33 @@  static void ivshmem_write_config(PCIDevice *pdev, uint32_t address,
     }
 }
 
+static HostMemoryBackend *desugar_shm(const char *shm, size_t size)
+{
+    /* TODO avoid the detour through QemuOpts */
+    static int counter;
+    QemuOpts *opts = qemu_opts_create(qemu_find_opts("object"),
+                                      NULL, 0, &error_abort);
+    char *path;
+    Object *obj;
+
+    qemu_opt_set(opts, "qom-type", "memory-backend-file",
+    &error_abort);
+    /* FIXME need a better way to make up an ID */
+    qemu_opts_set_id(opts, g_strdup_printf("ivshmem-backend-%d", counter++));
+    path = g_strdup_printf("/dev/shm/%s", shm);
+    qemu_opt_set(opts, "mem-path", path, &error_abort);
+    qemu_opt_set_number(opts, "size", size, &error_abort);
+    qemu_opt_set_bool(opts, "share", true, &error_abort);
+    g_free(path);
+
+    obj = user_creatable_add_opts(opts, &error_abort);
+    qemu_opts_del(opts);
+
+    user_creatable_complete(obj, &error_abort);
+
+    return MEMORY_BACKEND(obj);
+}
+
 static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
 {
     IVShmemState *s = IVSHMEM(dev);
@@ -911,6 +914,10 @@  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
         attr |= PCI_BASE_ADDRESS_MEM_TYPE_64;
     }
 
+    if (s->shmobj) {
+        s->hostmem = desugar_shm(s->shmobj, s->ivshmem_size);
+    }
+
     if (s->hostmem != NULL) {
         MemoryRegion *mr;
 
@@ -921,7 +928,7 @@  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
         vmstate_register_ram(mr, DEVICE(s));
         memory_region_add_subregion(&s->bar, 0, mr);
         pci_register_bar(PCI_DEVICE(s), 2, attr, &s->bar);
-    } else if (s->server_chr != NULL) {
+    } else {
         IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n",
                         s->server_chr->filename);
 
@@ -948,36 +955,6 @@  static void pci_ivshmem_realize(PCIDevice *dev, Error **errp)
             error_setg(errp, "failed to initialize interrupts");
             return;
         }
-    } else {
-        /* just map the file immediately, we're not using a server */
-        int fd;
-
-        IVSHMEM_DPRINTF("using shm_open (shm object = %s)\n", s->shmobj);
-
-        /* try opening with O_EXCL and if it succeeds zero the memory
-         * by truncating to 0 */
-        if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL,
-                        S_IRWXU|S_IRWXG|S_IRWXO)) > 0) {
-           /* truncate file to length PCI device's memory */
-            if (ftruncate(fd, s->ivshmem_size) != 0) {
-                error_report("could not truncate shared file");
-            }
-
-        } else if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR,
-                        S_IRWXU|S_IRWXG|S_IRWXO)) < 0) {
-            error_setg(errp, "could not open shared file");
-            return;
-        }
-
-        if (check_shm_size(s, fd, errp) == -1) {
-            return;
-        }
-
-        create_shared_memory_BAR(s, fd, attr, &err);
-        if (err) {
-            error_propagate(errp, err);
-            return;
-        }
     }
 
     if (s->role_val == IVSHMEM_PEER) {