| Message ID | 1457378754-21649-34-git-send-email-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <armbru@redhat.com> 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> > --- > hw/misc/ivshmem.c | 79 ++++++++++++++++--------------------------------------- > 1 file changed, 23 insertions(+), 56 deletions(-) > > diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c > index 1c25621..747f9c3 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,23 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address, > } > } > > +static void desugar_shm(IVShmemState *s) > +{ > + Object *obj; > + char *path; > + > + obj = object_new("memory-backend-file"); > + path = g_strdup_printf("/dev/shm/%s", s->shmobj); > + object_property_set_str(obj, path, "mem-path", &error_abort); > + g_free(path); > + object_property_set_int(obj, s->ivshmem_size, "size", &error_abort); > + object_property_set_bool(obj, true, "share", &error_abort); > + object_property_add_child(OBJECT(s), "internal-shm-backend", obj, > + &error_abort); > + user_creatable_complete(obj, &error_abort); > + s->hostmem = MEMORY_BACKEND(obj); > +} > + > static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) > { > IVShmemState *s = IVSHMEM(dev); > @@ -911,6 +904,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) > attr |= PCI_BASE_ADDRESS_MEM_TYPE_64; > } > > + if (s->shmobj) { > + desugar_shm(s); > + } > + > if (s->hostmem != NULL) { > MemoryRegion *mr; > > @@ -921,7 +918,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 +945,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) { Nice patch, but it's worth pointing out that qemu file_ram_alloc() creates file with open O_CREAT. Here you rely on the fact that /dev/shm is present and file inside maps to POSIX shared memory object names. That's a lot more restrictive than using shm_open(). Furthermore, the permissions are not the same. The current code uses 777 for some reasons, while qemu file_ram_alloc() is 644. I am not convinced the cleanup is worth these braking changes, especially the restriction to Linux only. > - /* 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) { > -- > 2.4.3 > >
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <armbru@redhat.com> 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> >> --- >> hw/misc/ivshmem.c | 79 ++++++++++++++++--------------------------------------- >> 1 file changed, 23 insertions(+), 56 deletions(-) >> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c >> index 1c25621..747f9c3 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,23 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address, >> } >> } >> >> +static void desugar_shm(IVShmemState *s) >> +{ >> + Object *obj; >> + char *path; >> + >> + obj = object_new("memory-backend-file"); >> + path = g_strdup_printf("/dev/shm/%s", s->shmobj); >> + object_property_set_str(obj, path, "mem-path", &error_abort); >> + g_free(path); >> + object_property_set_int(obj, s->ivshmem_size, "size", &error_abort); >> + object_property_set_bool(obj, true, "share", &error_abort); >> + object_property_add_child(OBJECT(s), "internal-shm-backend", obj, >> + &error_abort); >> + user_creatable_complete(obj, &error_abort); >> + s->hostmem = MEMORY_BACKEND(obj); >> +} >> + >> static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) >> { >> IVShmemState *s = IVSHMEM(dev); >> @@ -911,6 +904,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) >> attr |= PCI_BASE_ADDRESS_MEM_TYPE_64; >> } >> >> + if (s->shmobj) { >> + desugar_shm(s); >> + } >> + >> if (s->hostmem != NULL) { >> MemoryRegion *mr; >> >> @@ -921,7 +918,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 +945,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) { > > Nice patch, but it's worth pointing out that qemu file_ram_alloc() > creates file with open O_CREAT. Here you rely on the fact that > /dev/shm is present and file inside maps to POSIX shared memory object > names. That's a lot more restrictive than using shm_open(). > Furthermore, the permissions are not the same. The current code uses > 777 for some reasons, while qemu file_ram_alloc() is 644. I am not > convinced the cleanup is worth these braking changes, especially the > restriction to Linux only. The integrated memory backend has to go. Yes, my desugaring to memory-backend-file assumes Linux and the conventional mount point /dev/shm. For what it's worth, glibc implements shm_open() as a thin wrapper around open() on all targets. On Linux, it looks for other mountpoints when /dev/shm/ isn't there or unsuitable. On other targets, it always uses /dev/shm/. I didn't bother to duplicate glibc's mountpoint search, because distros converged to /dev/shm/ long ago. The proper way to support POSIX shared memory objects on systems where they're not files (and therefore can't be mapped with memory-backend-file) is to create memory-backend-shm. If such systems exist. I didn't do this now, because one, I'm not aware of a system that needs it, and two, ivshmem is Linux-specific anyway. ivshmem-plain could be made more portable, and once that's done, memory-backend-shm might become useful. That leaves permissions. You're right, the patch changes them from 0777 to 0644. I'm inclined to call it a bug fix. I failed to mention it in my commit message, and I'll fix that. We may want to mention it in release notes, too. >> - /* 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) { >> -- >> 2.4.3 >> >>
Hi On Wed, Mar 9, 2016 at 9:59 PM, Markus Armbruster <armbru@redhat.com> wrote: > I didn't do this now, because one, I'm not aware of a system that needs > it, and two, ivshmem is Linux-specific anyway. ivshmem-plain could be > made more portable, and once that's done, memory-backend-shm might > become useful. How is ivshmem (the "plain" part) Linux-specific? Afaik it works on bsd too. > That leaves permissions. You're right, the patch changes them from 0777 > to 0644. I'm inclined to call it a bug fix. I failed to mention it in > my commit message, and I'll fix that. We may want to mention it in > release notes, too. That's good enough for me.
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Wed, Mar 9, 2016 at 9:59 PM, Markus Armbruster <armbru@redhat.com> wrote: >> The integrated memory backend has to go. >> >> Yes, my desugaring to memory-backend-file assumes Linux and the >> conventional mount point /dev/shm. >> >> For what it's worth, glibc implements shm_open() as a thin wrapper >> around open() on all targets. On Linux, it looks for other mountpoints >> when /dev/shm/ isn't there or unsuitable. On other targets, it always >> uses /dev/shm/. I didn't bother to duplicate glibc's mountpoint search, >> because distros converged to /dev/shm/ long ago. >> >> The proper way to support POSIX shared memory objects on systems where >> they're not files (and therefore can't be mapped with >> memory-backend-file) is to create memory-backend-shm. If such systems >> exist. >> >> I didn't do this now, because one, I'm not aware of a system that needs >> it, and two, ivshmem is Linux-specific anyway. ivshmem-plain could be >> made more portable, and once that's done, memory-backend-shm might >> become useful. > > How is ivshmem (the "plain" part) Linux-specific? Afaik it works on bsd too. To make ivshmem-plain more portable, either add suitable #ifdefs to ivshmem.c or split ivshmem-plain off into its own file. As is, we only compile ivshmem.c with CONFIG_EVENTFD. If someone does that work, *and* any of the systems opened up by that can't do use memory-backend-file, then those systems would profit from memory-backend-shm. >> That leaves permissions. You're right, the patch changes them from 0777 >> to 0644. I'm inclined to call it a bug fix. I failed to mention it in >> my commit message, and I'll fix that. We may want to mention it in >> release notes, too. > > That's good enough for me. Okay :)
diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 1c25621..747f9c3 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,23 @@ static void ivshmem_write_config(PCIDevice *pdev, uint32_t address, } } +static void desugar_shm(IVShmemState *s) +{ + Object *obj; + char *path; + + obj = object_new("memory-backend-file"); + path = g_strdup_printf("/dev/shm/%s", s->shmobj); + object_property_set_str(obj, path, "mem-path", &error_abort); + g_free(path); + object_property_set_int(obj, s->ivshmem_size, "size", &error_abort); + object_property_set_bool(obj, true, "share", &error_abort); + object_property_add_child(OBJECT(s), "internal-shm-backend", obj, + &error_abort); + user_creatable_complete(obj, &error_abort); + s->hostmem = MEMORY_BACKEND(obj); +} + static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) { IVShmemState *s = IVSHMEM(dev); @@ -911,6 +904,10 @@ static void pci_ivshmem_realize(PCIDevice *dev, Error **errp) attr |= PCI_BASE_ADDRESS_MEM_TYPE_64; } + if (s->shmobj) { + desugar_shm(s); + } + if (s->hostmem != NULL) { MemoryRegion *mr; @@ -921,7 +918,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 +945,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) {
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 | 79 ++++++++++++++++--------------------------------------- 1 file changed, 23 insertions(+), 56 deletions(-)