|
From: | Damien Millescamps |
Subject: | Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files |
Date: | Fri, 20 Sep 2013 23:01:19 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux i686 on x86_64; rv:17.0) Gecko/20130801 Thunderbird/17.0.8 |
On 09/20/2013 10:45 PM, Benoît Canet
wrote:
Well, I am actually a bit confused by the rule:Le Friday 20 Sep 2013 à 20:34:47 (+0200), Damien Millescamps a écrit :This patch permits to share memory areas that do not specifically belong to /dev/shm. In such case, the file must be already present when launching qemu. A new parameter 'file' has been added to specify the file to use. A use case for this patch is sharing huge pages available through a hugetlbfs mountpoint. Signed-off-by: Damien Millescamps <address@hidden> --- docs/specs/ivshmem_device_spec.txt | 7 +++- hw/misc/ivshmem.c | 56 +++++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 21 deletions(-) diff --git a/docs/specs/ivshmem_device_spec.txt b/docs/specs/ivshmem_device_spec.txt index 667a862..cb7d310 100644 --- a/docs/specs/ivshmem_device_spec.txt +++ b/docs/specs/ivshmem_device_spec.txt @@ -4,8 +4,11 @@ Device Specification for Inter-VM shared memory device The Inter-VM shared memory device is designed to share a region of memory to userspace in multiple virtual guests. The memory region does not belong to any -guest, but is a POSIX memory object on the host. Optionally, the device may -support sending interrupts to other guests sharing the same memory region. +guest, but is a either a POSIX memory object or a mmap'd file (including +hugepage-backed file) on the host. + +Optionally, the device may support sending interrupts to other guests sharing +the same memory region. The Inter-VM PCI device diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c index 2838866..5d991cf 100644 --- a/hw/misc/ivshmem.c +++ b/hw/misc/ivshmem.c @@ -98,6 +98,7 @@ typedef struct IVShmemState { Error *migration_blocker; char * shmobj; + char * fileobj; char * sizearg; char * role; int role_val; /* scalar to avoid multiple string comparisons */ @@ -715,9 +716,10 @@ static int pci_ivshmem_init(PCIDevice *dev) /* if we get a UNIX socket as the parameter we will talk * to the ivshmem server to receive the memory region */ - if (s->shmobj != NULL) { - fprintf(stderr, "WARNING: do not specify both 'chardev' " - "and 'shm' with ivshmem\n"); + if (s->shmobj != NULL || s->fileobj != NULL) { + fprintf(stderr, "WARNING: both 'chardev' and '%s' specified.\n" + "Falling back to 'chardev' only.\n", + s->shmobj ? "shm" : "file"); } IVSHMEM_DPRINTF("using shared memory server (socket = %s)\n", @@ -743,28 +745,43 @@ static int pci_ivshmem_init(PCIDevice *dev) } else { /* just map the file immediately, we're not using a server */ int fd; + int is_shm = !!(s->shmobj != NULL); + int is_file = !!(s->fileobj != NULL);Out of curiosity why doing !! on a boolean which would be converted either to zero or one by implicit casting ?- if (s->shmobj == NULL) { - fprintf(stderr, "Must specify 'chardev' or 'shm' to ivshmem\n"); + if (!(is_shm ^ is_file)) { + fprintf(stderr, "ERROR: both 'file' and 'shm' specified for the " + "same ivshmem device.\n"); exit(1); } - 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) { - fprintf(stderr, "ivshmem: could not truncate shared file\n"); + if (is_shm) { + 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) { + fprintf(stderr, "ivshmem: could not truncate" + " shared file: %m\n"); + exit(-1); + } + + } else if ((fd = shm_open(s->shmobj, O_RDWR, + S_IRWXU|S_IRWXG|S_IRWXO)) < 0) { + fprintf(stderr, "ivshmem: could not open shared file: %m\n"); + exit(-1); } + } else { + IVSHMEM_DPRINTF("using open (file object = %s)\n", s->fileobj); - } else if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR, - S_IRWXU|S_IRWXG|S_IRWXO)) < 0) { - fprintf(stderr, "ivshmem: could not open shared file\n"); - exit(-1); - + /* try opening a mmap's file. This file must have been previously + * created on the host */ + if ((fd = open(s->fileobj, O_RDWR)) < 0) { + fprintf(stderr, "ivshmem: could not open mmap'd file: %m\n"); + exit(-1); + } } if (check_shm_size(s, fd) == -1) { @@ -804,6 +821,7 @@ static Property ivshmem_properties[] = { DEFINE_PROP_BIT("ioeventfd", IVShmemState, features, IVSHMEM_IOEVENTFD, false), DEFINE_PROP_BIT("msi", IVShmemState, features, IVSHMEM_MSI, true), DEFINE_PROP_STRING("shm", IVShmemState, shmobj), + DEFINE_PROP_STRING("file", IVShmemState, fileobj), DEFINE_PROP_STRING("role", IVShmemState, role), DEFINE_PROP_UINT32("use64", IVShmemState, ivshmem_64bit, 1), DEFINE_PROP_END_OF_LIST(), -- 1.7.2.5Hi, I won't be able to help you much on the semantic of the patch. However you should ./script/checkpatch.pl before posting: address@hidden:~/qemu (quorum_reboot2)$ ./scripts/checkpatch.pl /tmp/onx ERROR: "foo * bar" should be "foo *bar" #115: FILE: hw/misc/ivshmem.c:101: + char * fileobj; WARNING: suspect code indent for conditional statements (12, 15) #162: FILE: hw/misc/ivshmem.c:762: + if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL, [...] + /* truncate file to length PCI device's memory */ ERROR: do not use assignment in if condition #162: FILE: hw/misc/ivshmem.c:762: + if ((fd = shm_open(s->shmobj, O_CREAT|O_RDWR|O_EXCL, ERROR: do not use assignment in if condition #171: FILE: hw/misc/ivshmem.c:771: + } else if ((fd = shm_open(s->shmobj, O_RDWR, ERROR: do not use assignment in if condition #186: FILE: hw/misc/ivshmem.c:781: + if ((fd = open(s->fileobj, O_RDWR)) < 0) { total: 4 errors, 1 warnings, 99 lines checked /tmp/patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Best regards Benoît Follow the coding style and run scripts/checkpatch.pl <patchfile> before submitting. from the submit patch guidelines, since I assume that the coding style is referring to the file coding style ? The whole file might need a complete clean up, but I understand that it should be in a separate patch from this rule, since the whole file is written like that: Don't include irrelevant changes. In particular, don't include formatting, coding style or whitespace changes to bits of code that would otherwise not be touched by the patch. This is a just an indentation modification to put it inside a if/else... Cheers, -- Damien |
[Prev in Thread] | Current Thread | [Next in Thread] |