Re: [RFC v3 5/8] vhost: introduce vhost_set_vring_ready method

[Laurent Vivier]

On 29/05/2020 16:06, Cindy Lu wrote:
> From: Jason Wang <jasowang@redhat.com>
> 
> Vhost-vdpa introduces VHOST_VDPA_SET_VRING_ENABLE which complies the
> semantic of queue_enable defined in virtio spec. This method can be
> used for preventing device from executing request for a specific
> virtqueue. This patch introduces the vhost_ops for this.
> 
> Note that, we've already had vhost_set_vring_enable which has different
> semantic which allows to enable or disable a specific virtqueue for
> some kinds of vhost backends. E.g vhost-user use this to changes the
> number of active queue pairs.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Add your S-o-b.

> ---
>  hw/net/vhost_net-stub.c |  4 ++++
>  hw/net/vhost_net.c      | 11 ++++++++++-
>  include/net/vhost_net.h |  1 +
>  3 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/net/vhost_net-stub.c b/hw/net/vhost_net-stub.c
> index aac0e98228..43e93e1a9a 100644
> --- a/hw/net/vhost_net-stub.c
> +++ b/hw/net/vhost_net-stub.c
> @@ -86,6 +86,10 @@ int vhost_set_vring_enable(NetClientState *nc, int enable)
>      return 0;
>  }
>  
> +int vhost_set_vring_ready(NetClientState *nc)
> +{
> +    return 0;
> +}

Add a blank line here.

>  int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
>  {
>      return 0;
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d1d421e3d9..e2bc7de2eb 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -344,7 +344,7 @@ int vhost_net_start(VirtIODevice *dev, NetClientState 
> *ncs,
>              goto err_start;
>          }
>  
> -        if (ncs[i].peer->vring_enable) {
> +        if (peer->vring_enable) {
>              /* restore vring enable state */
>              r = vhost_set_vring_enable(peer, peer->vring_enable);

Move this part to PATCH 2/8

> @@ -455,6 +455,15 @@ int vhost_set_vring_enable(NetClientState *nc, int 
> enable)
>      return 0;
>  }
>  
> +int vhost_set_vring_ready(NetClientState *nc)
> +{
> +    VHostNetState *net = get_vhost_net(nc);
> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
> +    if (vhost_ops && vhost_ops->vhost_set_vring_ready) {

The structure VhostOps doesn't declare the vhost_set_vring_ready field.
Your patch is missing something and it could be not built.

It is defined in PATCH 7/8. If you want to keep this patch you should
move the declaration of "vhost_set_vring_ready_op vhost_set_vring_ready"
(and related) to this patch.

> +        return vhost_ops->vhost_set_vring_ready(&net->dev);
> +    }
> +    return 0;
> +}

Add a blank line.

>  int vhost_net_set_mtu(struct vhost_net *net, uint16_t mtu)
>  {
>      const VhostOps *vhost_ops = net->dev.vhost_ops;
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 77e47398c4..8a6f208189 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -35,6 +35,7 @@ int vhost_net_notify_migration_done(VHostNetState *net, 
> char* mac_addr);
>  VHostNetState *get_vhost_net(NetClientState *nc);
>  
>  int vhost_set_vring_enable(NetClientState * nc, int enable);
> +int vhost_set_vring_ready(NetClientState *nc);
>  
>  uint64_t vhost_net_get_acked_features(VHostNetState *net);
>  
> 

Thanks,
Laurent