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

[Michael S. Tsirkin]

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.

-- 
MST