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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

On Tue, Dec 05, 2017 at 02:27:16PM +0800, Changpeng Liu wrote:
> +* 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. The vhost-user slave must check the flags
> +      filed, and slaves MUST NOT accept SET_CONFIG for read-only

s/filed/field/

> +static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *config,
> +                                 uint32_t offset, uint32_t size, uint32_t flags)
> +{
> +    uint8_t *p;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    VhostUserMsg msg = {
> +        msg.request = VHOST_USER_SET_CONFIG,
> +        msg.flags = VHOST_USER_VERSION,
> +        msg.size = VHOST_USER_CONFIG_HDR_SIZE + size,
> +    };
> +
> +    if (reply_supported) {
> +        msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> +    }
> +
> +    msg.payload.config.offset = offset,
> +    msg.payload.config.size = size,
> +    msg.payload.config.flags = flags,
> +    p = msg.payload.config.region;
> +    memcpy(p, config + offset, size);

This function can be made more general by changing the semantics of the
config argument:

  memcpy(p, config, size);

Now the caller can pass just a single field instead of a whole 256-byte
config buffer.  It might be clearer to name the argument "data" or
"region" instead of "config" though.

> @@ -1505,6 +1508,67 @@ void vhost_ack_features(struct vhost_dev *hdev, const int *feature_bits,
>      }
>  }
>  
> +int vhost_dev_get_config(struct vhost_dev *hdev, uint8_t *config,
> +                         uint32_t config_len)
> +{
> +    assert(hdev->vhost_ops);
> +
> +    if (hdev->vhost_ops->vhost_get_config) {
> +        return hdev->vhost_ops->vhost_get_config(hdev, config, config_len);
> +    }
> +
> +    return 0;
> +}
> +
> +int vhost_dev_set_config(struct vhost_dev *hdev, const uint8_t *config,
> +                         uint32_t offset, uint32_t size, uint32_t flags)
> +{
> +    assert(hdev->vhost_ops);
> +
> +    if (hdev->vhost_ops->vhost_set_config) {
> +        return hdev->vhost_ops->vhost_set_config(hdev, config, offset,
> +                                                 size, flags);
> +    }
> +
> +    return 0;
> +}

Both vhost_dev_get_config() and vhost_dev_set_config() cannot fail
silently.  The device will not work properly if the configuration space
feature is not supported.

Please make these functions return an error if the callback is NULL.

The vhost-blk code should also check that
hdev->vhost_ops->vhost_set_config != NULL during realize.  This way
users see the error when adding the device instead of at runtime when
the function gets called.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]