* [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files
@ 2013-09-20 18:34 Damien Millescamps
2013-09-20 20:45 ` Benoît Canet
0 siblings, 1 reply; 3+ messages in thread
From: Damien Millescamps @ 2013-09-20 18:34 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-trivial, mjt
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);
- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files
2013-09-20 18:34 [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files Damien Millescamps
@ 2013-09-20 20:45 ` Benoît Canet
2013-09-20 21:01 ` Damien Millescamps
0 siblings, 1 reply; 3+ messages in thread
From: Benoît Canet @ 2013-09-20 20:45 UTC (permalink / raw)
To: Damien Millescamps; +Cc: qemu-trivial, mjt, qemu-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files
2013-09-20 20:45 ` Benoît Canet
@ 2013-09-20 21:01 ` Damien Millescamps
0 siblings, 0 replies; 3+ messages in thread
From: Damien Millescamps @ 2013-09-20 21:01 UTC (permalink / raw)
To: Benoît Canet; +Cc: qemu-trivial, mjt, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 8145 bytes --]
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,
--
Damien
[-- Attachment #2: Type: text/html, Size: 8593 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2013-09-20 21:03 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-20 18:34 [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files Damien Millescamps
2013-09-20 20:45 ` Benoît Canet
2013-09-20 21:01 ` Damien Millescamps
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).