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