| Message ID | 1379702087-8653-1-git-send-email-damien.millescamps@6wind.com |
|---|---|
| State | New |
| Headers | show |
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 <damien.millescamps@6wind.com> > --- > 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.5 > > Hi, I won't be able to help you much on the semantic of the patch. However you should ./script/checkpatch.pl before posting: benoit@Laure:~/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
On 09/20/2013 10:45 PM, Benoît Canet wrote: > 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 <damien.millescamps@6wind.com> >> --- >> 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.5 >> >> > Hi, > > I won't be able to help you much on the semantic of the patch. > However you should ./script/checkpatch.pl before posting: > > benoit@Laure:~/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 Well, I am actually a bit confused by the rule: *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,
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); - 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(),
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 <damien.millescamps@6wind.com> --- docs/specs/ivshmem_device_spec.txt | 7 +++- hw/misc/ivshmem.c | 56 +++++++++++++++++++++++------------ 2 files changed, 42 insertions(+), 21 deletions(-)