[RFC,v2,10/13] vhost: add vhost_kernel_set_vring_enable

This method is already present in vhost-user. This commit adapts it to
vhost-net, so SVQ can use.

vhost_kernel_set_enable stops the device, so qemu can ask for its status
(next available idx the device was going to consume). When SVQ starts it
can resume consuming the guest's driver ring, without notice from the
latter. Not stopping the device before of the swapping could imply that
it process more buffers than reported, what would duplicate the device
action.

Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
 hw/virtio/vhost-backend.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

在 2021/3/16 上午3:48, Eugenio Pérez 写道:
> This method is already present in vhost-user. This commit adapts it to
> vhost-net, so SVQ can use.
>
> vhost_kernel_set_enable stops the device, so qemu can ask for its status
> (next available idx the device was going to consume). When SVQ starts it
> can resume consuming the guest's driver ring, without notice from the
> latter. Not stopping the device before of the swapping could imply that
> it process more buffers than reported, what would duplicate the device
> action.


Note that it might not be the case of vDPA (virtio) or at least virtio 
needs some extension to achieve something similar like this. One example 
is virtio-pci which forbids 0 to be wrote to queue_enable.

This is another reason to start from vhost-vDPA.


>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>   hw/virtio/vhost-backend.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 31b33bde37..1ac5c574a9 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -201,6 +201,34 @@ static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
>       return idx - dev->vq_index;
>   }
>   
> +static int vhost_kernel_set_vq_enable(struct vhost_dev *dev, unsigned idx,
> +                                      bool enable)
> +{
> +    struct vhost_vring_file file = {
> +        .index = idx,
> +    };
> +
> +    if (!enable) {
> +        file.fd = -1; /* Pass -1 to unbind from file. */
> +    } else {
> +        struct vhost_net *vn_dev = container_of(dev, struct vhost_net, dev);
> +        file.fd = vn_dev->backend;


This can only work with vhost-net devices but not vsock/scsi etc.

Thanks


> +    }
> +
> +    return vhost_kernel_net_set_backend(dev, &file);
> +}
> +
> +static int vhost_kernel_set_vring_enable(struct vhost_dev *dev, int enable)
> +{
> +    int i;
> +
> +    for (i = 0; i < dev->nvqs; ++i) {
> +        vhost_kernel_set_vq_enable(dev, i, enable);
> +    }
> +
> +    return 0;
> +}
> +
>   #ifdef CONFIG_VHOST_VSOCK
>   static int vhost_kernel_vsock_set_guest_cid(struct vhost_dev *dev,
>                                               uint64_t guest_cid)
> @@ -317,6 +345,7 @@ static const VhostOps kernel_ops = {
>           .vhost_set_owner = vhost_kernel_set_owner,
>           .vhost_reset_device = vhost_kernel_reset_device,
>           .vhost_get_vq_index = vhost_kernel_get_vq_index,
> +        .vhost_set_vring_enable = vhost_kernel_set_vring_enable,
>   #ifdef CONFIG_VHOST_VSOCK
>           .vhost_vsock_set_guest_cid = vhost_kernel_vsock_set_guest_cid,
>           .vhost_vsock_set_running = vhost_kernel_vsock_set_running,

On Tue, Mar 16, 2021 at 8:30 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2021/3/16 上午3:48, Eugenio Pérez 写道:
> > This method is already present in vhost-user. This commit adapts it to
> > vhost-net, so SVQ can use.
> >
> > vhost_kernel_set_enable stops the device, so qemu can ask for its status
> > (next available idx the device was going to consume). When SVQ starts it
> > can resume consuming the guest's driver ring, without notice from the
> > latter. Not stopping the device before of the swapping could imply that
> > it process more buffers than reported, what would duplicate the device
> > action.
>
>
> Note that it might not be the case of vDPA (virtio) or at least virtio
> needs some extension to achieve something similar like this. One example
> is virtio-pci which forbids 0 to be wrote to queue_enable.
>
> This is another reason to start from vhost-vDPA.
>
>
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> >   hw/virtio/vhost-backend.c | 29 +++++++++++++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> >
> > diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> > index 31b33bde37..1ac5c574a9 100644
> > --- a/hw/virtio/vhost-backend.c
> > +++ b/hw/virtio/vhost-backend.c
> > @@ -201,6 +201,34 @@ static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> >       return idx - dev->vq_index;
> >   }
> >
> > +static int vhost_kernel_set_vq_enable(struct vhost_dev *dev, unsigned idx,
> > +                                      bool enable)
> > +{
> > +    struct vhost_vring_file file = {
> > +        .index = idx,
> > +    };
> > +
> > +    if (!enable) {
> > +        file.fd = -1; /* Pass -1 to unbind from file. */
> > +    } else {
> > +        struct vhost_net *vn_dev = container_of(dev, struct vhost_net, dev);
> > +        file.fd = vn_dev->backend;
>
>
> This can only work with vhost-net devices but not vsock/scsi etc.
>

Right. Shadow virtqueue code should also check the return value of the
vhost_set_vring_enable call.

I'm not sure how to solve it without resorting to some ifelse/switch
chain, checking for specific net/vsock/... features, or relaying on
some other qemu class facilities. However, since the main use case is
vDPA live migration, this commit could be left out and SVQ operation
would only be valid for vhost-vdpa and vhost-user.

> Thanks
>
>
> > +    }
> > +
> > +    return vhost_kernel_net_set_backend(dev, &file);
> > +}
> > +
> > +static int vhost_kernel_set_vring_enable(struct vhost_dev *dev, int enable)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < dev->nvqs; ++i) {
> > +        vhost_kernel_set_vq_enable(dev, i, enable);
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >   #ifdef CONFIG_VHOST_VSOCK
> >   static int vhost_kernel_vsock_set_guest_cid(struct vhost_dev *dev,
> >                                               uint64_t guest_cid)
> > @@ -317,6 +345,7 @@ static const VhostOps kernel_ops = {
> >           .vhost_set_owner = vhost_kernel_set_owner,
> >           .vhost_reset_device = vhost_kernel_reset_device,
> >           .vhost_get_vq_index = vhost_kernel_get_vq_index,
> > +        .vhost_set_vring_enable = vhost_kernel_set_vring_enable,
> >   #ifdef CONFIG_VHOST_VSOCK
> >           .vhost_vsock_set_guest_cid = vhost_kernel_vsock_set_guest_cid,
> >           .vhost_vsock_set_running = vhost_kernel_vsock_set_running,
>

在 2021/3/16 下午6:43, Eugenio Perez Martin 写道:
> On Tue, Mar 16, 2021 at 8:30 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 在 2021/3/16 上午3:48, Eugenio Pérez 写道:
>>> This method is already present in vhost-user. This commit adapts it to
>>> vhost-net, so SVQ can use.
>>>
>>> vhost_kernel_set_enable stops the device, so qemu can ask for its status
>>> (next available idx the device was going to consume). When SVQ starts it
>>> can resume consuming the guest's driver ring, without notice from the
>>> latter. Not stopping the device before of the swapping could imply that
>>> it process more buffers than reported, what would duplicate the device
>>> action.
>>
>> Note that it might not be the case of vDPA (virtio) or at least virtio
>> needs some extension to achieve something similar like this. One example
>> is virtio-pci which forbids 0 to be wrote to queue_enable.
>>
>> This is another reason to start from vhost-vDPA.
>>
>>
>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>> ---
>>>    hw/virtio/vhost-backend.c | 29 +++++++++++++++++++++++++++++
>>>    1 file changed, 29 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
>>> index 31b33bde37..1ac5c574a9 100644
>>> --- a/hw/virtio/vhost-backend.c
>>> +++ b/hw/virtio/vhost-backend.c
>>> @@ -201,6 +201,34 @@ static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
>>>        return idx - dev->vq_index;
>>>    }
>>>
>>> +static int vhost_kernel_set_vq_enable(struct vhost_dev *dev, unsigned idx,
>>> +                                      bool enable)
>>> +{
>>> +    struct vhost_vring_file file = {
>>> +        .index = idx,
>>> +    };
>>> +
>>> +    if (!enable) {
>>> +        file.fd = -1; /* Pass -1 to unbind from file. */
>>> +    } else {
>>> +        struct vhost_net *vn_dev = container_of(dev, struct vhost_net, dev);
>>> +        file.fd = vn_dev->backend;
>>
>> This can only work with vhost-net devices but not vsock/scsi etc.
>>
> Right. Shadow virtqueue code should also check the return value of the
> vhost_set_vring_enable call.
>
> I'm not sure how to solve it without resorting to some ifelse/switch
> chain, checking for specific net/vsock/... features, or relaying on
> some other qemu class facilities. However, since the main use case is
> vDPA live migration, this commit could be left out and SVQ operation
> would only be valid for vhost-vdpa and vhost-user.


Yes, that's why I think we can start with vhost-vDPA first.

Thanks


>
>> Thanks
>>
>>
>>> +    }
>>> +
>>> +    return vhost_kernel_net_set_backend(dev, &file);
>>> +}
>>> +
>>> +static int vhost_kernel_set_vring_enable(struct vhost_dev *dev, int enable)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < dev->nvqs; ++i) {
>>> +        vhost_kernel_set_vq_enable(dev, i, enable);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>    #ifdef CONFIG_VHOST_VSOCK
>>>    static int vhost_kernel_vsock_set_guest_cid(struct vhost_dev *dev,
>>>                                                uint64_t guest_cid)
>>> @@ -317,6 +345,7 @@ static const VhostOps kernel_ops = {
>>>            .vhost_set_owner = vhost_kernel_set_owner,
>>>            .vhost_reset_device = vhost_kernel_reset_device,
>>>            .vhost_get_vq_index = vhost_kernel_get_vq_index,
>>> +        .vhost_set_vring_enable = vhost_kernel_set_vring_enable,
>>>    #ifdef CONFIG_VHOST_VSOCK
>>>            .vhost_vsock_set_guest_cid = vhost_kernel_vsock_set_guest_cid,
>>>            .vhost_vsock_set_running = vhost_kernel_vsock_set_running,