Re: [Qemu-devel] [PATCH 1/5] virtio: Set "start_on_kick" on virtio_set_features()

[Greg Kurz]

On Wed, 29 May 2019 15:09:51 +0800
address@hidden wrote:

> From: Xie Yongji <address@hidden>
> 
> The guest feature is not set correctly on virtio_reset() and
> virtio_init(). So we should not use it to set "start_on_kick" at that
> point. This patch set "start_on_kick" on virtio_set_features() instead.
> 
> Signed-off-by: Xie Yongji <address@hidden>
> ---

Maybe add a Fixes: badaf79cfdbd3 ?

>  hw/virtio/virtio.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 4805727b53..fc8fca81ad 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1214,8 +1214,7 @@ void virtio_reset(void *opaque)
>          k->reset(vdev);
>      }
>  
> -    vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) 
> &&
> -                          !virtio_vdev_has_feature(vdev, 
> VIRTIO_F_VERSION_1));
> +    vdev->start_on_kick = false;
>      vdev->started = false;
>      vdev->broken = false;
>      vdev->guest_features = 0;
> @@ -2069,14 +2068,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t 
> val)
>          return -EINVAL;
>      }
>      ret = virtio_set_features_nocheck(vdev, val);
> -    if (!ret && virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> -        int i;
> -        for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> -            if (vdev->vq[i].vring.num != 0) {
> -                virtio_init_region_cache(vdev, i);
> +    if (!ret) {
> +        if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> +            /* VIRTIO_RING_F_EVENT_IDX changes the size of the caches.  */
> +            int i;
> +            for (i = 0; i < VIRTIO_QUEUE_MAX; i++) {
> +                if (vdev->vq[i].vring.num != 0) {
> +                    virtio_init_region_cache(vdev, i);
> +                }
>              }
>          }
> +
> +        if (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) &&
> +            !virtio_vdev_has_feature(vdev, VIRTIO_F_VERSION_1)) {
> +            vdev->start_on_kick = true;
> +        }
>      }
>      return ret;
>  }
> @@ -2331,8 +2337,7 @@ void virtio_init(VirtIODevice *vdev, const char *name,
>              g_malloc0(sizeof(*vdev->vector_queues) * nvectors);
>      }
>  
> -    vdev->start_on_kick = (virtio_host_has_feature(vdev, VIRTIO_F_VERSION_1) 
> &&
> -                          !virtio_vdev_has_feature(vdev, 
> VIRTIO_F_VERSION_1));
> +    vdev->start_on_kick = false;

virtio_init() is called at realize on an object that was just allocated with
g_malloc0(). You shouldn't need to set anything to 0 or false... or I'm
missing something ?

Anyway, I guess this doesn't hurt, so:

Reviewed-by: Greg Kurz <address@hidden>

>      vdev->started = false;
>      vdev->device_id = device_id;
>      vdev->status = 0;