Re: [Qemu-devel] [PATCH v5 1/4] vhost-user: add new vhost user messages to support virtio config space

[Liu, Changpeng]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden
> Sent: Tuesday, November 21, 2017 4:45 AM
> To: Stefan Hajnoczi <address@hidden>
> Cc: Liu, Changpeng <address@hidden>; address@hidden;
> address@hidden; address@hidden; address@hidden;
> Harris, James R <address@hidden>
> Subject: Re: [PATCH v5 1/4] vhost-user: add new vhost user messages to support
> virtio config space
> 
> On Mon, Nov 20, 2017 at 04:26:31PM +0000, Stefan Hajnoczi wrote:
> > On Fri, Nov 17, 2017 at 04:44:37AM +0800, Changpeng Liu wrote:
> > > Add VHOST_USER_GET_CONFIG/VHOST_USER_SET_CONFIG messages which
> can be
> > > used for live migration of vhost user devices, also vhost user devices
> > > can benefit from the messages to get/set virtio config space from/to the
> > > I/O target. For the purpose to support virtio config space change,
> > > VHOST_USER_SET_CONFIG_FD message is added as the event notifier
> > > in case virtio config space change in the I/O target.
> > >
> > > Signed-off-by: Changpeng Liu <address@hidden>
> > > ---
> > >  docs/interop/vhost-user.txt       | 39 ++++++++++++++++
> > >  hw/virtio/vhost-user.c            | 98
> +++++++++++++++++++++++++++++++++++++++
> > >  hw/virtio/vhost.c                 | 63 +++++++++++++++++++++++++
> > >  include/hw/virtio/vhost-backend.h |  8 ++++
> > >  include/hw/virtio/vhost.h         | 16 +++++++
> > >  5 files changed, 224 insertions(+)
> > >
> > > diff --git a/docs/interop/vhost-user.txt b/docs/interop/vhost-user.txt
> > > index 954771d..1b98388 100644
> > > --- a/docs/interop/vhost-user.txt
> > > +++ b/docs/interop/vhost-user.txt
> > > @@ -116,6 +116,16 @@ Depending on the request type, payload can be:
> > >      - 3: IOTLB invalidate
> > >      - 4: IOTLB access fail
> > >
> > > + * Virtio device config space
> > > +   ---------------------------
> > > +   | offset | size | payload |
> > > +   ---------------------------
> > > +
> > > +   Offset: a 32-bit offset of virtio device's configuration space
> >
> > s/of/in the/
> >
> > > +   Size: a 32-bit size of configuration space that master wanted to 
> > > change
> >
> > Is this also used for GET_CONFIG?  If yes, I suggest "a 32-bit
> > configuration space access size in bytes".
> >
> > Please mention that Size must be <= 256 bytes.
> >
> > > +   Payload: a 256-bytes array holding the contents of the virtio
> > > +       device's configuration space
> >
> > What about bytes outside the [offset, offset+size) range?  I guess they
> > must be 0 and are ignored by the master/slave.
> >
> > Would it be cleaner to make Payload a variable-sized field with Size
> > bytes?  That way it's not necessary to transfer 0s and memcpy() a subset
> > of the payload array.
> >
> > > +
> > >  In QEMU the vhost-user message is implemented with the following struct:
> > >
> > >  typedef struct VhostUserMsg {
> > > @@ -129,6 +139,7 @@ typedef struct VhostUserMsg {
> > >          VhostUserMemory memory;
> > >          VhostUserLog log;
> > >          struct vhost_iotlb_msg iotlb;
> > > +        VhostUserConfig config;
> > >      };
> > >  } QEMU_PACKED VhostUserMsg;
> > >
> > > @@ -596,6 +607,34 @@ Master message types
> > >        and expect this message once (per VQ) during device configuration
> > >        (ie. before the master starts the VQ).
> > >
> > > + * VHOST_USER_GET_CONFIG
> > > +      Id: 24
> > > +      Equivalent ioctl: N/A
> > > +      Master payload: virtio device config space
> > > +
> > > +      Submitted by the vhost-user master to fetch the contents of the 
> > > virtio
> > > +      device configuration space. The vhost-user master may cache the 
> > > contents
> > > +      to avoid repeated VHOST_USER_GET_CONFIG calls.
> > > +
> > > +* VHOST_USER_SET_CONFIG
> > > +      Id: 25
> > > +      Equivalent ioctl: N/A
> > > +      Master payload: virtio device config space
> > > +
> > > +      Submitted by the vhost-user master when the Guest changes the 
> > > virtio
> > > +      device configuration space and also can be used for live migration
> > > +      on the destination host.
> >
> > There might be security issues if the vhost slave cannot tell whether
> > SET_CONFIG is coming from the guest driver or from the master process
> > (live migration).  Typically certain fields are read-only for the guest
> > driver.  Maybe those fields need to be set by the master after live
> > migration.
> >
> > One way to solve this is adding a flags field to the message.  A special
> > flag can be used for live migration so the slave knows that this
> > SET_CONFIG message is allowed to write to read-only fields.
> >
> > It's also worth documenting that slaves MUST NOT accept SET_CONFIG for
> > read-only configuration space fields unless the live migration bit is
> > set.  Hopefully this will remind implementors to think through the
> > security issues.
> 
> Live migrations is supposed to be migrating guest writeable state too.
> If you mean migrating RO fields like size, then
> I don't think it's a good idea to reuse SET_CONFIG for that.
> SET_CONFIG should obey exactly the virtio semantics.
> 
> And I agree, it should say that slave must treat it as a write,
> and get config as a read according to virtio semantics.
> 
> If someone needs to pass configuration from qemu to
> slave, let's add specific messages with precisely defined semantics.
> 
> Which reminds me.
> 
> virtio 1 changed endian-ness for config.
> 
> I think we should specify it's all virtio 1 format, and
> just disallow vhost blk for virtio 0 guests.
Will add the limitation to this txt file.
> 
> --
> MST