Re: [virtio-dev] Preferred layout for request/response structs

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Apr 16, 2020 at 08:58:46PM +0900, Keiichi Watanabe wrote:
> Hi, I'm designing virtio-video protocol [1] and thinking about its
> structs' layouts.
> The virtio-video protocol adopts the request-response model like
> virtio-gpu and virtio-fs. While I looked at these two protocols, I
> found they used different designs of having request headers.
> So, Iâm wondering whatâs the best way of having the request / response
> header for virtio-video.
> 
> On the one hand, virtio-gpu's structs for each control and response
> contain the virtio_gpu_ctrl_hdr as one of fields:
> struct virtio_gpu_get_edid {
>   struct virtio_gpu_ctrl_hdr hdr;
>   ...
> };
> struct virtio_gpu_resp_edid {
>   struct virtio_gpu_ctrl_hdr hdr;
>   ...
> };
> 
> On the other hand, virtio-fs separates its header struct and request
> body like the FUSE protocol:
> struct fuse_in_header in;
> union {
>   struct fuse_read_in readin;
>   ...
> };
> 
> When we define a new protocol, which layout is preferred? I'd
> especially like to know why virtio-gpu decided to use this design and
> if the design is nice.

Here are my personal thoughts about request/response struct design:

Make the request/response is a single entity if possible because it's
simpler.  This works for devices where the driver places a
request/response onto the virtqueue as a single element and the device
completes it.

Devices that have rx/tx virtqueue designs work differently, and the
response will need to have fields that can be used to correlate the
response with the request that it is related to.

If the software processing of the request/response is inherently
separate (e.g. the device is a transport for an existing protocol that
has separate request/response messages), then it also makes sense to
include extra header fields in the response - even if the driver places
a single element into the virtqueue and you don't have an rx/tx
virtqueue design.

Regarding whether to specify a single C struct with a union for all
request types, or whether to specify separate top-level C structs like
virtio-gpu, it depends.  If there would be a lot of duplication then I
think the single struct with a union approach is good.  If there is a
lot of variation then defining separate top-level structs is cleaner
because there's not much to share.

> In the latest proposal of the virtio-video [1], we chose the same
> design as virtio-gpu like:
> struct virtio_video_resource_queue {
>   struct virtio_video_cmd_hdr hdr;
>   ...
> };
> struct virtio_video_resource_queue_resp {
>   struct virtio_video_cmd_hdr hdr;

Are these hdr fields actually used by the driver when the request is
completed?

If your request/response is a single virtqueue element then there is no
need to duplicate hdr into the response because the driver already has
the request.

Response headers are usually only necessary in rx/tx virtqueue designs
where the driver needs to correlate a response on the rx queue with a
request that was sent on the tx queue.

>   ...
> };
> However, I thought it's a bit inconvenient that we cannot know the
> actual size of a struct before its header. Actually, our device
> implementation reads the header first to know the size and then reads
> the whole struct including the header.

In the device implementation I would read a struct virtio_video_cmd_hdr
from the out iov list first.  Then either read the request-specific
struct (defined separately) or use (uint8_t *)req + offsetof(struct
virtio_video_resource_queue, field_after_hdr) to read the remainder but
it's ugly.

> If it doesnât make any sense to follow the virtio-gpuâs header design,
> Iâd like to separate the header and the command bodies and make the
> driver send two structs per command:
> struct virtio_video_cmd_hdr hdr; // contains what command follows
> struct virtio_video_resource_queue {
>     ...
> };
> struct virtio_video_resource_queue_resp {
>   ...
> };

Yes, this is cleaner for the device emulation code.  The driver doesn't
need this separation.

Remember that the struct definitions in the VIRTIO specification are
pseudo-C and not necessarily the exact struct definitions that drivers
and device implementations have to use.  As long as you follow the data
layout correctly as per the spec you can arrange your structs any way
you want.

Stefan

Attachment: signature.asc
Description: PGP signature