[Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Victor Kaplansky]

Unlike the kernel, vhost-user application accesses log table by
mmaping it to its user space. This change adds two new fields to
VhostUserMsg payload: mmap_size, and mmap_offset and make QEMU to
pass the to vhost-user application in VHOST_USER_SET_LOG_BASE
request.

Signed-off-by: Victor Kaplansky <victork@redhat.com>

---
 hw/virtio/vhost-user.c    | 11 +++++++++--
 tests/vhost-user-test.c   |  8 ++++++++
 docs/specs/vhost-user.txt |  8 +++++++-
 3 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 83c84f1..46c63bc 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -75,6 +75,11 @@ typedef struct VhostUserMemory {
     VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
 } VhostUserMemory;
 
+typedef struct VhostUserLog {
+    uint64_t mmap_size;
+    uint64_t mmap_offset;
+} VhostUserLog;
+
 typedef struct VhostUserMsg {
     VhostUserRequest request;
 
@@ -89,6 +94,7 @@ typedef struct VhostUserMsg {
         struct vhost_vring_state state;
         struct vhost_vring_addr addr;
         VhostUserMemory memory;
+        VhostUserLog log;
     } payload;
 } QEMU_PACKED VhostUserMsg;
 
@@ -200,8 +206,9 @@ static int vhost_user_set_log_base(struct vhost_dev *dev, uint64_t base,
     VhostUserMsg msg = {
         .request = VHOST_USER_SET_LOG_BASE,
         .flags = VHOST_USER_VERSION,
-        .payload.u64 = base,
-        .size = sizeof(msg.payload.u64),
+        .payload.log.mmap_size = log->size,
+        .payload.log.mmap_offset = 0,
+        .size = sizeof(msg.payload.log),
     };
 
     if (shmfd && log->fd != -1) {
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index b6dde75..f005ecf 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -86,6 +86,11 @@ typedef struct VhostUserMemory {
     VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
 } VhostUserMemory;
 
+typedef struct VhostUserLog {
+    uint64_t mmap_size;
+    uint64_t mmap_offset;
+} VhostUserLog;
+
 typedef struct VhostUserMsg {
     VhostUserRequest request;
 
@@ -94,10 +99,13 @@ typedef struct VhostUserMsg {
     uint32_t flags;
     uint32_t size; /* the following payload size */
     union {
+#define VHOST_USER_VRING_IDX_MASK   (0xff)
+#define VHOST_USER_VRING_NOFD_MASK  (0x1<<8)
         uint64_t u64;
         struct vhost_vring_state state;
         struct vhost_vring_addr addr;
         VhostUserMemory memory;
+        VhostUserLog log;
     } payload;
 } QEMU_PACKED VhostUserMsg;
 
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index e0d71e2..eb8f2b2 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -98,6 +98,7 @@ typedef struct VhostUserMsg {
         struct vhost_vring_state state;
         struct vhost_vring_addr addr;
         VhostUserMemory memory;
+        VhostUserLog log;
     };
 } QEMU_PACKED VhostUserMsg;
 
@@ -282,7 +283,12 @@ Message types
       Master payload: u64
       Slave payload: N/A
 
-      Sets the logging base address.
+      Sets logging shared memory space.
+      When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
+      feature, the log memory fd is provided in the ancillary data of
+      VHOST_USER_SET_LOG_BASE message, the size and offset of shared
+      memory area provided in the message.
+
 
  * VHOST_USER_SET_LOG_FD
 
-- 
--Victor

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Marc-André Lureau]

Hi

----- Original Message -----
> Unlike the kernel, vhost-user application accesses log table by
> mmaping it to its user space. This change adds two new fields to
> VhostUserMsg payload: mmap_size, and mmap_offset and make QEMU to
> pass the to vhost-user application in VHOST_USER_SET_LOG_BASE
> request.
> 
> Signed-off-by: Victor Kaplansky <victork@redhat.com>

What's the motivation for doing this? The offset is always 0, and the size must at least match the size of VM memory.

> 
> ---
>  hw/virtio/vhost-user.c    | 11 +++++++++--
>  tests/vhost-user-test.c   |  8 ++++++++
>  docs/specs/vhost-user.txt |  8 +++++++-
>  3 files changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 83c84f1..46c63bc 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -75,6 +75,11 @@ typedef struct VhostUserMemory {
>      VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
>  } VhostUserMemory;
>  
> +typedef struct VhostUserLog {
> +    uint64_t mmap_size;
> +    uint64_t mmap_offset;
> +} VhostUserLog;
> +
>  typedef struct VhostUserMsg {
>      VhostUserRequest request;
>  
> @@ -89,6 +94,7 @@ typedef struct VhostUserMsg {
>          struct vhost_vring_state state;
>          struct vhost_vring_addr addr;
>          VhostUserMemory memory;
> +        VhostUserLog log;
>      } payload;
>  } QEMU_PACKED VhostUserMsg;
>  
> @@ -200,8 +206,9 @@ static int vhost_user_set_log_base(struct vhost_dev *dev,
> uint64_t base,
>      VhostUserMsg msg = {
>          .request = VHOST_USER_SET_LOG_BASE,
>          .flags = VHOST_USER_VERSION,
> -        .payload.u64 = base,
> -        .size = sizeof(msg.payload.u64),
> +        .payload.log.mmap_size = log->size,
> +        .payload.log.mmap_offset = 0,
> +        .size = sizeof(msg.payload.log),
>      };
>  
>      if (shmfd && log->fd != -1) {
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index b6dde75..f005ecf 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -86,6 +86,11 @@ typedef struct VhostUserMemory {
>      VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
>  } VhostUserMemory;
>  
> +typedef struct VhostUserLog {
> +    uint64_t mmap_size;
> +    uint64_t mmap_offset;
> +} VhostUserLog;
> +
>  typedef struct VhostUserMsg {
>      VhostUserRequest request;
>  
> @@ -94,10 +99,13 @@ typedef struct VhostUserMsg {
>      uint32_t flags;
>      uint32_t size; /* the following payload size */
>      union {
> +#define VHOST_USER_VRING_IDX_MASK   (0xff)
> +#define VHOST_USER_VRING_NOFD_MASK  (0x1<<8)
>          uint64_t u64;
>          struct vhost_vring_state state;
>          struct vhost_vring_addr addr;
>          VhostUserMemory memory;
> +        VhostUserLog log;
>      } payload;
>  } QEMU_PACKED VhostUserMsg;
>  
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index e0d71e2..eb8f2b2 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -98,6 +98,7 @@ typedef struct VhostUserMsg {
>          struct vhost_vring_state state;
>          struct vhost_vring_addr addr;
>          VhostUserMemory memory;
> +        VhostUserLog log;
>      };
>  } QEMU_PACKED VhostUserMsg;
>  
> @@ -282,7 +283,12 @@ Message types
>        Master payload: u64
>        Slave payload: N/A
>  
> -      Sets the logging base address.
> +      Sets logging shared memory space.
> +      When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
> +      feature, the log memory fd is provided in the ancillary data of
> +      VHOST_USER_SET_LOG_BASE message, the size and offset of shared
> +      memory area provided in the message.
> +
>  
>   * VHOST_USER_SET_LOG_FD
>  
> --
> --Victor
> 

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 09:32:17AM -0500, Marc-André Lureau wrote:
> Hi
> 
> ----- Original Message -----
> > Unlike the kernel, vhost-user application accesses log table by
> > mmaping it to its user space. This change adds two new fields to
> > VhostUserMsg payload: mmap_size, and mmap_offset and make QEMU to
> > pass the to vhost-user application in VHOST_USER_SET_LOG_BASE
> > request.
> > 
> > Signed-off-by: Victor Kaplansky <victork@redhat.com>
> 
> What's the motivation for doing this? The offset is always 0, and the size must at least match the size of VM memory.

Remote doesn't know the size though, and offset is only there
in the current implementation.

> > 
> > ---
> >  hw/virtio/vhost-user.c    | 11 +++++++++--
> >  tests/vhost-user-test.c   |  8 ++++++++
> >  docs/specs/vhost-user.txt |  8 +++++++-
> >  3 files changed, 24 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 83c84f1..46c63bc 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -75,6 +75,11 @@ typedef struct VhostUserMemory {
> >      VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
> >  } VhostUserMemory;
> >  
> > +typedef struct VhostUserLog {
> > +    uint64_t mmap_size;
> > +    uint64_t mmap_offset;
> > +} VhostUserLog;
> > +
> >  typedef struct VhostUserMsg {
> >      VhostUserRequest request;
> >  
> > @@ -89,6 +94,7 @@ typedef struct VhostUserMsg {
> >          struct vhost_vring_state state;
> >          struct vhost_vring_addr addr;
> >          VhostUserMemory memory;
> > +        VhostUserLog log;
> >      } payload;
> >  } QEMU_PACKED VhostUserMsg;
> >  
> > @@ -200,8 +206,9 @@ static int vhost_user_set_log_base(struct vhost_dev *dev,
> > uint64_t base,
> >      VhostUserMsg msg = {
> >          .request = VHOST_USER_SET_LOG_BASE,
> >          .flags = VHOST_USER_VERSION,
> > -        .payload.u64 = base,
> > -        .size = sizeof(msg.payload.u64),
> > +        .payload.log.mmap_size = log->size,
> > +        .payload.log.mmap_offset = 0,
> > +        .size = sizeof(msg.payload.log),
> >      };
> >  
> >      if (shmfd && log->fd != -1) {
> > diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> > index b6dde75..f005ecf 100644
> > --- a/tests/vhost-user-test.c
> > +++ b/tests/vhost-user-test.c
> > @@ -86,6 +86,11 @@ typedef struct VhostUserMemory {
> >      VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS];
> >  } VhostUserMemory;
> >  
> > +typedef struct VhostUserLog {
> > +    uint64_t mmap_size;
> > +    uint64_t mmap_offset;
> > +} VhostUserLog;
> > +
> >  typedef struct VhostUserMsg {
> >      VhostUserRequest request;
> >  
> > @@ -94,10 +99,13 @@ typedef struct VhostUserMsg {
> >      uint32_t flags;
> >      uint32_t size; /* the following payload size */
> >      union {
> > +#define VHOST_USER_VRING_IDX_MASK   (0xff)
> > +#define VHOST_USER_VRING_NOFD_MASK  (0x1<<8)
> >          uint64_t u64;
> >          struct vhost_vring_state state;
> >          struct vhost_vring_addr addr;
> >          VhostUserMemory memory;
> > +        VhostUserLog log;
> >      } payload;
> >  } QEMU_PACKED VhostUserMsg;
> >  
> > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > index e0d71e2..eb8f2b2 100644
> > --- a/docs/specs/vhost-user.txt
> > +++ b/docs/specs/vhost-user.txt
> > @@ -98,6 +98,7 @@ typedef struct VhostUserMsg {
> >          struct vhost_vring_state state;
> >          struct vhost_vring_addr addr;
> >          VhostUserMemory memory;
> > +        VhostUserLog log;
> >      };
> >  } QEMU_PACKED VhostUserMsg;
> >  
> > @@ -282,7 +283,12 @@ Message types
> >        Master payload: u64
> >        Slave payload: N/A
> >  
> > -      Sets the logging base address.
> > +      Sets logging shared memory space.
> > +      When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
> > +      feature, the log memory fd is provided in the ancillary data of
> > +      VHOST_USER_SET_LOG_BASE message, the size and offset of shared
> > +      memory area provided in the message.
> > +
> >  
> >   * VHOST_USER_SET_LOG_FD
> >  
> > --
> > --Victor
> > 

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Marc-André Lureau]

Hi

On Wed, Nov 11, 2015 at 4:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> Remote doesn't know the size though, and offset is only there
> in the current implementation.

It can compute the size based on the mem tables. But it could be more
future-proof to add these informations.

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 04:23:33PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Nov 11, 2015 at 4:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> > Remote doesn't know the size though, and offset is only there
> > in the current implementation.
> 
> It can compute the size based on the mem tables. But it could be more
> future-proof to add these informations.

Right. Doing it now means we won't need a separate feature flag for it.

> -- 
> Marc-André Lureau

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Marc-André Lureau]

Hi

On Wed, Nov 11, 2015 at 3:26 PM, Victor Kaplansky <victork@redhat.com> wrote:
>
> -      Sets the logging base address.
> +      Sets logging shared memory space.
> +      When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
> +      feature, the log memory fd is provided in the ancillary data of
> +      VHOST_USER_SET_LOG_BASE message, the size and offset of shared
> +      memory area provided in the message.

I think this extra payload needs a better description, in payload
description above. Something like

 * A pair of 64-bit integers
   -------------
   | u64 | u64 |
   -------------

   u64: a 64-bit unsigned integer

"the size and offset of shared memory area provided in the message as
a pair of 64-bit integers."

-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vhost-user: modify SET_LOG_BASE to pass mmap size and offset

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 04:33:05PM +0100, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Nov 11, 2015 at 3:26 PM, Victor Kaplansky <victork@redhat.com> wrote:
> >
> > -      Sets the logging base address.
> > +      Sets logging shared memory space.
> > +      When slave has VHOST_USER_PROTOCOL_F_LOG_SHMFD protocol
> > +      feature, the log memory fd is provided in the ancillary data of
> > +      VHOST_USER_SET_LOG_BASE message, the size and offset of shared
> > +      memory area provided in the message.
> 
> I think this extra payload needs a better description, in payload
> description above. Something like
> 
>  * A pair of 64-bit integers
>    -------------
>    | u64 | u64 |
>    -------------
> 
>    u64: a 64-bit unsigned integer
> 
> "the size and offset of shared memory area provided in the message as
> a pair of 64-bit integers."

Sounds good. I've queued this one up, pls tweak docs
with patches on top.

> -- 
> Marc-André Lureau