[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memo
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memory backend |
Date: |
Wed, 09 Mar 2016 21:59:13 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) |
Marc-André Lureau <address@hidden> writes:
> Hi
>
> On Mon, Mar 7, 2016 at 8:25 PM, Markus Armbruster <address@hidden> 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 <address@hidden>
>> ---
>> 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
>>
>>
- [Qemu-devel] [PATCH v2 24/42] ivshmem: Simplify rejection of invalid peer ID from server, (continued)
- [Qemu-devel] [PATCH v2 24/42] ivshmem: Simplify rejection of invalid peer ID from server, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 36/42] qdev: New DEFINE_PROP_ON_OFF_AUTO, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 25/42] ivshmem: Disentangle ivshmem_read(), Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 40/42] ivshmem: Drop ivshmem property x-memdev, Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 34/42] ivshmem: Simplify memory regions for BAR 2 (shared memory), Markus Armbruster, 2016/03/07
- [Qemu-devel] [PATCH v2 33/42] ivshmem: Implement shm=... with a memory backend, Markus Armbruster, 2016/03/07
[Qemu-devel] [PATCH v2 12/42] ivshmem: Rewrite specification document, Markus Armbruster, 2016/03/07
[Qemu-devel] [PATCH v2 28/42] ivshmem: Propagate errors through ivshmem_recv_setup(), Markus Armbruster, 2016/03/07
[Qemu-devel] [PATCH v2 42/42] contrib/ivshmem-server: Print "not for production" warning, Markus Armbruster, 2016/03/07
[Qemu-devel] [PATCH v2 41/42] ivshmem: Require master to have ID zero, Markus Armbruster, 2016/03/07