vhost-user payload union restrictions ?

vhost-user payload union restrictions ?

[Dr. David Alan Gilbert]

(Resend, remembering to add list)
Hi,
  I'm trying to understand what restrictions there are on the
payload that's part of VhostUserMsg; and am confused by
inconsistencies.

Lets start with this version:

subprojects/libvhost-user/libvhost-user.h :
typedef struct VhostUserMsg {
    int request;

#define VHOST_USER_VERSION_MASK     (0x3)
#define VHOST_USER_REPLY_MASK       (0x1 << 2)
#define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
    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;
        VhostUserMemRegMsg memreg;
        VhostUserLog log;
        VhostUserConfig config;
        VhostUserVringArea area;
        VhostUserInflight inflight;
    } payload;

    int fds[VHOST_MEMORY_BASELINE_NREGIONS];
    int fd_num;
    uint8_t *data;
} VU_PACKED VhostUserMsg;

note the 'fds' array after the payload but before
the end of the structure.

But then there's the version in:
hw/virtio/vhost-user.c
typedef 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;
        VhostUserMemRegMsg mem_reg;
        VhostUserLog log;
        struct vhost_iotlb_msg iotlb;
        VhostUserConfig config;
        VhostUserCryptoSession session;
        VhostUserVringArea area;
        VhostUserInflight inflight;
} VhostUserPayload;

typedef struct VhostUserMsg {
    VhostUserHeader hdr;
    VhostUserPayload payload;
} QEMU_PACKED VhostUserMsg;

which hasn't got the 'fds' section.
Yet they're both marked as 'packed'.

That's a bit unfortunate for two structures with the same name.

Am I right in thinking that the vhost-user.c version is sent over
the wire, while the libvhost-user.h one is really just an interface?

Is it safe for me to add a new, larger entry in the payload union
without breaking existing clients?

I ended up at this question after trying to add a variable length
entry to the union:

typedef struct {
    VhostUserFSSlaveMsg fs;
    VhostUserFSSlaveMsgEntry entries[VHOST_USER_FS_SLAVE_MAX_ENTRIES];
} QEMU_PACKED VhostUserFSSlaveMsgMax;

...
union ....
        VhostUserFSSlaveMsg fs;
        VhostUserFSSlaveMsgMax fs_max; /* Never actually used */
} VhostUserPayload;

and in the .h I had:
typedef struct {
    /* Generic flags for the overall message */
    uint32_t flags;
    /* Number of entries */
    uint16_t count;
    /* Spare */
    uint16_t align;

    VhostUserFSSlaveMsgEntry entries[];
} VhostUserFSSlaveMsg;

    union {
...
        VhostUserInflight inflight;
        VhostUserFSSlaveMsg fs;
    } payload;

which is apparently OK in the .c version, and gcc is happy with the same
in the libvhost-user.h version; but clang gets upset by the .h
version because it doesn't like the variable length structure
before the end of the struct - which I have sympathy for.

Dave

-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: vhost-user payload union restrictions ?

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3955 bytes --]

Hi

On Wed, May 5, 2021 at 4:38 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
wrote:

> (Resend, remembering to add list)
> Hi,
>   I'm trying to understand what restrictions there are on the
> payload that's part of VhostUserMsg; and am confused by
> inconsistencies.
>
> Lets start with this version:
>
> subprojects/libvhost-user/libvhost-user.h :
> typedef struct VhostUserMsg {
>     int request;
>
> #define VHOST_USER_VERSION_MASK     (0x3)
> #define VHOST_USER_REPLY_MASK       (0x1 << 2)
> #define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
>     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;
>         VhostUserMemRegMsg memreg;
>         VhostUserLog log;
>         VhostUserConfig config;
>         VhostUserVringArea area;
>         VhostUserInflight inflight;
>     } payload;
>
>     int fds[VHOST_MEMORY_BASELINE_NREGIONS];
>     int fd_num;
>     uint8_t *data;
> } VU_PACKED VhostUserMsg;
>
> note the 'fds' array after the payload but before
> the end of the structure.
>
> But then there's the version in:
> hw/virtio/vhost-user.c
> typedef 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;
>         VhostUserMemRegMsg mem_reg;
>         VhostUserLog log;
>         struct vhost_iotlb_msg iotlb;
>         VhostUserConfig config;
>         VhostUserCryptoSession session;
>         VhostUserVringArea area;
>         VhostUserInflight inflight;
> } VhostUserPayload;
>
> typedef struct VhostUserMsg {
>     VhostUserHeader hdr;
>     VhostUserPayload payload;
> } QEMU_PACKED VhostUserMsg;
>
> which hasn't got the 'fds' section.
> Yet they're both marked as 'packed'.
>

They are packed, because both are used to serialize/deserialize the stream.


> That's a bit unfortunate for two structures with the same name.
>
>
Yes, maybe it's time to have a canonical system header used by both?


> Am I right in thinking that the vhost-user.c version is sent over
> the wire, while the libvhost-user.h one is really just an interface?
>
>
I believe the extra fields are not used for serializing the message, but
just a convenient way to group related data.


> Is it safe for me to add a new, larger entry in the payload union
> without breaking existing clients?
>

It should be.


> I ended up at this question after trying to add a variable length
> entry to the union:
>
> typedef struct {
>     VhostUserFSSlaveMsg fs;
>     VhostUserFSSlaveMsgEntry entries[VHOST_USER_FS_SLAVE_MAX_ENTRIES];
> } QEMU_PACKED VhostUserFSSlaveMsgMax;
>
> ...
> union ....
>         VhostUserFSSlaveMsg fs;
>         VhostUserFSSlaveMsgMax fs_max; /* Never actually used */
> } VhostUserPayload;
>
> and in the .h I had:
> typedef struct {
>     /* Generic flags for the overall message */
>     uint32_t flags;
>     /* Number of entries */
>     uint16_t count;
>     /* Spare */
>     uint16_t align;
>
>     VhostUserFSSlaveMsgEntry entries[];
> } VhostUserFSSlaveMsg;
>
>     union {
> ...
>         VhostUserInflight inflight;
>         VhostUserFSSlaveMsg fs;
>     } payload;
>
> which is apparently OK in the .c version, and gcc is happy with the same
> in the libvhost-user.h version; but clang gets upset by the .h
> version because it doesn't like the variable length structure
> before the end of the struct - which I have sympathy for.
>
>
Indeed, we probably want to allocate the message separately then.

thanks

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 5509 bytes --]

Re: vhost-user payload union restrictions ?

[Dr. David Alan Gilbert]

* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Wed, May 5, 2021 at 4:38 PM Dr. David Alan Gilbert <dgilbert@redhat.com>
> wrote:
> 
> > (Resend, remembering to add list)
> > Hi,
> >   I'm trying to understand what restrictions there are on the
> > payload that's part of VhostUserMsg; and am confused by
> > inconsistencies.
> >
> > Lets start with this version:
> >
> > subprojects/libvhost-user/libvhost-user.h :
> > typedef struct VhostUserMsg {
> >     int request;
> >
> > #define VHOST_USER_VERSION_MASK     (0x3)
> > #define VHOST_USER_REPLY_MASK       (0x1 << 2)
> > #define VHOST_USER_NEED_REPLY_MASK  (0x1 << 3)
> >     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;
> >         VhostUserMemRegMsg memreg;
> >         VhostUserLog log;
> >         VhostUserConfig config;
> >         VhostUserVringArea area;
> >         VhostUserInflight inflight;
> >     } payload;
> >
> >     int fds[VHOST_MEMORY_BASELINE_NREGIONS];
> >     int fd_num;
> >     uint8_t *data;
> > } VU_PACKED VhostUserMsg;
> >
> > note the 'fds' array after the payload but before
> > the end of the structure.
> >
> > But then there's the version in:
> > hw/virtio/vhost-user.c
> > typedef 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;
> >         VhostUserMemRegMsg mem_reg;
> >         VhostUserLog log;
> >         struct vhost_iotlb_msg iotlb;
> >         VhostUserConfig config;
> >         VhostUserCryptoSession session;
> >         VhostUserVringArea area;
> >         VhostUserInflight inflight;
> > } VhostUserPayload;
> >
> > typedef struct VhostUserMsg {
> >     VhostUserHeader hdr;
> >     VhostUserPayload payload;
> > } QEMU_PACKED VhostUserMsg;
> >
> > which hasn't got the 'fds' section.
> > Yet they're both marked as 'packed'.
> >
> 
> They are packed, because both are used to serialize/deserialize the stream.

The header is (de)serialized and the payload is; but we don't ever try
and deal with both at the same time do we ? We read the header, check
the length, read the payload; so isn't it really each part is packed and
not the whole?

> 
> > That's a bit unfortunate for two structures with the same name.
> >
> >
> Yes, maybe it's time to have a canonical system header used by both?

Any idea where that would live?  I think the problem is that in some
respects the libvhost_user.h is the right place, but it's now formally a
separate/sub project.

> > Am I right in thinking that the vhost-user.c version is sent over
> > the wire, while the libvhost-user.h one is really just an interface?
> >
> >
> I believe the extra fields are not used for serializing the message, but
> just a convenient way to group related data.

OK

> > Is it safe for me to add a new, larger entry in the payload union
> > without breaking existing clients?
> >
> 
> It should be.

Good.

> > I ended up at this question after trying to add a variable length
> > entry to the union:
> >
> > typedef struct {
> >     VhostUserFSSlaveMsg fs;
> >     VhostUserFSSlaveMsgEntry entries[VHOST_USER_FS_SLAVE_MAX_ENTRIES];
> > } QEMU_PACKED VhostUserFSSlaveMsgMax;
> >
> > ...
> > union ....
> >         VhostUserFSSlaveMsg fs;
> >         VhostUserFSSlaveMsgMax fs_max; /* Never actually used */
> > } VhostUserPayload;
> >
> > and in the .h I had:
> > typedef struct {
> >     /* Generic flags for the overall message */
> >     uint32_t flags;
> >     /* Number of entries */
> >     uint16_t count;
> >     /* Spare */
> >     uint16_t align;
> >
> >     VhostUserFSSlaveMsgEntry entries[];
> > } VhostUserFSSlaveMsg;
> >
> >     union {
> > ...
> >         VhostUserInflight inflight;
> >         VhostUserFSSlaveMsg fs;
> >     } payload;
> >
> > which is apparently OK in the .c version, and gcc is happy with the same
> > in the libvhost-user.h version; but clang gets upset by the .h
> > version because it doesn't like the variable length structure
> > before the end of the struct - which I have sympathy for.
> >
> >
> Indeed, we probably want to allocate the message separately then.

I'm thinking that wecould change the libvhost-user.h to be:

(as the C file)

typedef struct VhostUserMsg {
    VhostUserHeader hdr;
    VhostUserPayload payload;
} VU_PACKED VhostUserMsgWire;

typedef struct VhostUserMsgExt {
    int fds[VHOST_MEMORY_BASELINE_NREGIONS];
    int fd_num;
    uint8_t *data;
    VhostUserMsgWire msg;
} VhostUserMsg;

Dave


> thanks
> 
> -- 
> Marc-André Lureau
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK