Re: [Qemu-trivial] [Qemu-devel] [PATCH v4] ivshmem: allow the sharing of mmap'd files

[Benoît Canet]

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.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:

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