Re: [dpdk-dev] [RFC 2/3] vhost: add SET_VIRTIO_STATUS support

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 02/27/2018 02:10 PM, Jens Freimann wrote:
> On Thu, Feb 22, 2018 at 07:19:09PM +0100, Maxime Coquelin wrote:
>> This patch implements support for the new SET_VIRTIO_STATUS
>> vhost-user request.
>>
>> The main use for this new request is for the backend to know
>> when the driver sets the DRIVER_OK status bit. Starting Virtio
>> 1.0, we know that once the the bit is set, no more queues will
>> be initialized.
>> When it happens, this patch removes all queues starting from
>> the first uninitialized one, so that the port starts even if
>> the guest driver does not use all the queues provided by QEMU.
>> This is for example the case with Windows driver, which only
>> initializes as much queue pairs as vCPUs.
>>
>> The second use for this request is when the status changes to
>> reset or failed state, the vhost port is stopped and virtqueues
>> cleaned and freed.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> lib/librte_vhost/vhost_user.c | 98 
>> +++++++++++++++++++++++++++++++++++++++++++
>> lib/librte_vhost/vhost_user.h |  5 ++-
>> 2 files changed, 102 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c 
>> b/lib/librte_vhost/vhost_user.c
>> index c256ebb06..7ab02c44b 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -67,6 +67,7 @@ static const char *vhost_message_str[VHOST_USER_MAX] 
>> = {
>>     [VHOST_USER_NET_SET_MTU]  = "VHOST_USER_NET_SET_MTU",
>>     [VHOST_USER_SET_SLAVE_REQ_FD]  = "VHOST_USER_SET_SLAVE_REQ_FD",
>>     [VHOST_USER_IOTLB_MSG]  = "VHOST_USER_IOTLB_MSG",
>> +    [VHOST_USER_SET_VIRTIO_STATUS]  = "VHOST_USER_SET_VIRTIO_STATUS",
>> };
>>
>> static uint64_t
>> @@ -1244,6 +1245,100 @@ vhost_user_iotlb_msg(struct virtio_net **pdev, 
>> struct VhostUserMsg *msg)
>>     return 0;
>> }
>>
>> +static int
>> +vhost_user_set_virtio_status(struct virtio_net *dev, struct 
>> VhostUserMsg *msg)
>> +{
>> +    uint8_t old_status, new_status;
>> +    uint32_t i;
>> +
>> +    /* As per Virtio spec, the Virtio device status is 8 bits wide */
>> +    if (msg->payload.u64 != (uint8_t)msg->payload.u64) {
>> +        RTE_LOG(ERR, VHOST_CONFIG,
>> +                "Invalid Virtio dev status value (%lx)\n",
>> +                msg->payload.u64);
>> +        return -1;
>> +    }
>> +
>> +    new_status = msg->payload.u64;
>> +    old_status = dev->virtio_status;
>> +
>> +    if (new_status == old_status)
>> +        return 0;
>> +
>> +    RTE_LOG(DEBUG, VHOST_CONFIG,
>> +            "New Virtio device status %x (was %x)\n",
>> +            new_status, old_status);
>> +
>> +    dev->virtio_status = new_status;
>> +
>> +    if (new_status == 0 || new_status & VIRTIO_CONFIG_S_FAILED) {
>> +        /*
>> +         * The device moved to reset  or failed state,
>> +         * stop processing the virtqueues
>> +         */
>> +        if (dev->flags & VIRTIO_DEV_RUNNING) {
>> +            dev->flags &= ~VIRTIO_DEV_RUNNING;
>> +            dev->notify_ops->destroy_device(dev->vid);
>> +        }
>> +
>> +        while (dev->nr_vring > 0) {
>> +            struct vhost_virtqueue *vq;
>> +
>> +            vq = dev->virtqueue[--dev->nr_vring];
>> +            if (!vq)
>> +                continue;
>> +
>> +            dev->virtqueue[dev->nr_vring] = NULL;
>> +            cleanup_vq(dev, vq, 1);
>> +            free_vq(vq);
>> +        }
>> +
>> +        return 0;
>> +    }
>> +
>> +    if ((dev->features & (1ULL << VIRTIO_F_VERSION_1)) &&
>> +            (new_status & VIRTIO_CONFIG_S_DRIVER_OK) &&
>> +            !virtio_is_ready(dev)) {
>> +        /*
>> +         * Since Virtio 1.0, we know that no more queues will be
>> +         * setup after guest sets DRIVER_OK. So let's remove
>> +         * uinitialized queues.
>> +         */
>> +        RTE_LOG(INFO, VHOST_CONFIG,
>> +                "Driver is ready, but some queues aren't 
>> initialized\n");
>> +
>> +        /*
>> +         * Find the first uninitialized queue.
>> +         *
>> +         * Note: Ideally the backend implementation should
>> +         * support sparsed virtqueues, but as long as it is
>> +         * not the case, let's remove all queues after the
>> +         * first uninitialized one.
>> +         */
>> +        for (i = 0; i < dev->nr_vring; i++) {
>> +            if (!vq_is_ready(dev->virtqueue[i]))
>> +                break;
>> +        }
>> +
>> +        while (dev->nr_vring >= i) {
>> +            struct vhost_virtqueue *vq;
>> +
>> +            vq = dev->virtqueue[--dev->nr_vring];
> 
> If i is 0, we could access an array element out of bounds, no?


Thanks for spotting this off-by-one error, it should be:
+        while (dev->nr_vring > i) {

> With this fixed,
> 
> Reviewed-by: Jens Freimann <jfreimann@redhat.com>
> regards,
> Jens

Thanks,
Maxime