From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by dpdk.org (Postfix) with ESMTP id 3B0E51B19 for ; Tue, 27 Feb 2018 15:04:39 +0100 (CET) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C159A4026648; Tue, 27 Feb 2018 14:04:38 +0000 (UTC) Received: from [10.36.112.12] (unknown [10.36.112.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5E8BC213AEF5; Tue, 27 Feb 2018 14:04:37 +0000 (UTC) To: Jens Freimann Cc: jianfeng.tan@intel.com, stefanha@redhat.com, tiwei.bie@intel.com, dev@dpdk.org References: <20180222181910.23134-1-maxime.coquelin@redhat.com> <20180222181910.23134-3-maxime.coquelin@redhat.com> <20180227131005.a7cqbagkl65qtajn@dhcp-192-241.str.redhat.com> From: Maxime Coquelin Message-ID: Date: Tue, 27 Feb 2018 15:04:35 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180227131005.a7cqbagkl65qtajn@dhcp-192-241.str.redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 27 Feb 2018 14:04:38 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Tue, 27 Feb 2018 14:04:38 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'maxime.coquelin@redhat.com' RCPT:'' Subject: Re: [dpdk-dev] [RFC 2/3] vhost: add SET_VIRTIO_STATUS support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 27 Feb 2018 14:04:39 -0000 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 >> --- >> 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 > regards, > Jens Thanks, Maxime