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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature