* vhost-user payload union restrictions ?
@ 2021-05-05 12:36 Dr. David Alan Gilbert
2021-05-05 12:59 ` Marc-André Lureau
0 siblings, 1 reply; 3+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-05 12:36 UTC (permalink / raw)
To: marcandre.lureau, mst; +Cc: vgoyal, stefanha, qemu-devel
(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
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: vhost-user payload union restrictions ?
2021-05-05 12:36 vhost-user payload union restrictions ? Dr. David Alan Gilbert
@ 2021-05-05 12:59 ` Marc-André Lureau
2021-05-05 17:54 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 3+ messages in thread
From: Marc-André Lureau @ 2021-05-05 12:59 UTC (permalink / raw)
To: Dr. David Alan Gilbert; +Cc: QEMU, Stefan Hajnoczi, vgoyal, Michael S. Tsirkin
[-- 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 --]
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: vhost-user payload union restrictions ?
2021-05-05 12:59 ` Marc-André Lureau
@ 2021-05-05 17:54 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 3+ messages in thread
From: Dr. David Alan Gilbert @ 2021-05-05 17:54 UTC (permalink / raw)
To: Marc-André Lureau; +Cc: QEMU, Stefan Hajnoczi, vgoyal, Michael S. Tsirkin
* 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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-05 17:56 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-05 12:36 vhost-user payload union restrictions ? Dr. David Alan Gilbert
2021-05-05 12:59 ` Marc-André Lureau
2021-05-05 17:54 ` Dr. David Alan Gilbert
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).