* Marc-André Lureau (marcandre.lur...@gmail.com) wrote:
> Hi
>
> On Wed, May 5, 2021 at 4:38 PM Dr. David Alan Gilbert <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
