Re: [Qemu-devel] [PATCH v4 for-4.0 2/7] vhost-user: Support transferring inflight buffer between qemu and backend

[Yongji Xie]

On Tue, 15 Jan 2019 at 06:25, Michael S. Tsirkin <address@hidden> wrote:
>
> On Wed, Jan 09, 2019 at 07:27:23PM +0800, address@hidden wrote:
> > @@ -382,6 +397,30 @@ If VHOST_USER_PROTOCOL_F_SLAVE_SEND_FD protocol 
> > feature is negotiated,
> >  slave can send file descriptors (at most 8 descriptors in each message)
> >  to master via ancillary data using this fd communication channel.
> >
> > +Inflight I/O tracking
> > +---------------------
> > +
> > +To support slave reconnecting, slave need to track inflight I/O in a
> > +shared memory. VHOST_USER_GET_INFLIGHT_FD and VHOST_USER_SET_INFLIGHT_FD
> > +are used to transfer the memory between master and slave. And to encourage
> > +consistency, we provide a recommended format for this memory:
>
> I think we should make a stronger statement and actually
> just say what the format is. Not recommend it weakly.
>

Okey, will do it.

> > +
> > +offset        width    description
> > +0x0      0x400    region for queue0
> > +0x400    0x400    region for queue1
> > +0x800    0x400    region for queue2
> > +...      ...      ...
> > +
> > +For each virtqueue, we have a 1024 bytes region.
>
>
> Why is the size hardcoded? Why not a function of VQ size?
>

Sorry, I didn't get your point. Should the region's size be fixed? Do
you mean we need to document a function for the region's size?

>
> > The region's format is like:
> > +
> > +offset   width    description
> > +0x0      0x1      descriptor 0 is in use or not
> > +0x1      0x1      descriptor 1 is in use or not
> > +0x2      0x1      descriptor 2 is in use or not
> > +...      ...      ...
> > +
> > +For each descriptor, we use one byte to specify whether it's in use or not.
> > +
> >  Protocol features
> >  -----------------
> >
>
> I think that it's a good idea to have a version in this region.
> Otherwise how are you going to handle compatibility when
> this needs to be extended?
>

I have put the version into the message's payload: VhostUserInflight. Is it OK?

Thanks,
Yongji