From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by dpdk.space (Postfix) with ESMTP id C88AFA05D3 for ; Thu, 23 May 2019 17:17:42 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 9DC6B1B948; Thu, 23 May 2019 17:17:41 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by dpdk.org (Postfix) with ESMTP id F0EC51B945 for ; Thu, 23 May 2019 17:17:39 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 75E8E40003 for ; Thu, 23 May 2019 17:17:39 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 6119440004; Thu, 23 May 2019 17:17:39 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on bernadotte.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-0.9 required=5.0 tests=ALL_TRUSTED,AWL autolearn=disabled version=3.4.1 X-Spam-Score: -0.9 Received: from [192.168.1.59] (host-90-232-127-248.mobileonline.telia.com [90.232.127.248]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 286FC40003; Thu, 23 May 2019 17:17:37 +0200 (CEST) To: Maxime Coquelin , dev@dpdk.org, tiwei.bie@intel.com, jfreimann@redhat.com, zhihong.wang@intel.com, bruce.richardson@intel.com, konstantin.ananyev@intel.com References: <20190517122220.31283-1-maxime.coquelin@redhat.com> <20190517122220.31283-4-maxime.coquelin@redhat.com> <2cfed6b0-8a7d-ec05-4b67-f40bd9409a8b@ericsson.com> <25057af9-8d1c-5e97-b648-6d344724291e@redhat.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= Message-ID: <4a91dd95-16bf-7b73-6cce-2ea8490e64d9@ericsson.com> Date: Thu, 23 May 2019 17:17:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <25057af9-8d1c-5e97-b648-6d344724291e@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP Subject: Re: [dpdk-dev] [PATCH 3/5] vhost: do not inline unlikely fragmented buffers code 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 2019-05-23 16:30, Maxime Coquelin wrote: > Hi Mattias, > > On 5/21/19 9:43 PM, Mattias Rönnblom wrote: >> On 2019-05-17 14:22, Maxime Coquelin wrote: >>> Handling of fragmented virtio-net header and indirect descriptors >>> tables was implemented to fix CVE-2018-1059. It should not never >>> happen with healthy guests and so are already considered as >>> unlikely code path. >>> >>> This patch moves these bits into non-inline dedicated functions >>> to reduce the I-cache pressure. >>> >>> Signed-off-by: Maxime Coquelin >>> --- >>>   lib/librte_vhost/vhost.c      |  33 +++++++++++ >>>   lib/librte_vhost/vhost.h      |  35 +----------- >>>   lib/librte_vhost/virtio_net.c | 102 +++++++++++++++++++--------------- >>>   3 files changed, 91 insertions(+), 79 deletions(-) >>> >>> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c >>> index 4a54ad6bd1..8a4379bc13 100644 >>> --- a/lib/librte_vhost/vhost.c >>> +++ b/lib/librte_vhost/vhost.c >>> @@ -201,6 +201,39 @@ __vhost_log_cache_write(struct virtio_net *dev, >>> struct vhost_virtqueue *vq, >>>   } >>> +void * >>> +alloc_copy_ind_table(struct virtio_net *dev, struct vhost_virtqueue >>> *vq, >> >> This function should have a prefix. > > This function is just moved from vhost.h to vhost.c, so not the purpose > of the patch. > It was declared "static inline" in the header file, and thus only affected those who included the file, as opposed to polluting the whole DPDK library name space. > But I agree your comment, I'll send a patch to add a prefix. > >> >>> +        uint64_t desc_addr, uint64_t desc_len) >>> +{ >>> +    void *idesc; >>> +    uint64_t src, dst; >>> +    uint64_t len, remain = desc_len; >>> + >>> +    idesc = rte_malloc(__func__, desc_len, 0); >>> +    if (unlikely(!idesc)) >> >> if (idesc == NULL) > > Ditto, that is not the purpose of the patch that is just moving the > function. > > I agree this is not matching the coding rules specified in the > documentation, though. > >> >>> +        return NULL; >>> + >>> +    dst = (uint64_t)(uintptr_t)idesc; >>> + >>> +    while (remain) { >> remain > 0 > > Ditto. > >>> +        len = remain; >>> +        src = vhost_iova_to_vva(dev, vq, desc_addr, &len, >>> +                VHOST_ACCESS_RO); >>> +        if (unlikely(!src || !len)) { >>> +            rte_free(idesc); >>> +            return NULL; >>> +        } >>> + >>> +        rte_memcpy((void *)(uintptr_t)dst, (void *)(uintptr_t)src, >>> len); >> >> Just for my understanding: what difference does that (uintptr_t) cast do? > > This is required to build 32bits (-Werror=int-to-pointer-cast) > Ah. Thanks. >>> + >>> +        remain -= len; >>> +        dst += len; >>> +        desc_addr += len; >>> +    } >>> + >>> +    return idesc; >>> +} >>> + >>>   void >>>   cleanup_vq(struct vhost_virtqueue *vq, int destroy) >>>   { >>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h >>> index 3ab7b4950f..ab26454e1c 100644 >>> --- a/lib/librte_vhost/vhost.h >>> +++ b/lib/librte_vhost/vhost.h >>> @@ -488,6 +488,8 @@ void vhost_backend_cleanup(struct virtio_net *dev); >>>   uint64_t __vhost_iova_to_vva(struct virtio_net *dev, struct >>> vhost_virtqueue *vq, >>>               uint64_t iova, uint64_t *len, uint8_t perm); >>> +void *alloc_copy_ind_table(struct virtio_net *dev, struct >>> vhost_virtqueue *vq, >>> +            uint64_t desc_addr, uint64_t desc_len); >>>   int vring_translate(struct virtio_net *dev, struct vhost_virtqueue >>> *vq); >>>   void vring_invalidate(struct virtio_net *dev, struct >>> vhost_virtqueue *vq); >>> @@ -601,39 +603,6 @@ vhost_vring_call_packed(struct virtio_net *dev, >>> struct vhost_virtqueue *vq) >>>           eventfd_write(vq->callfd, (eventfd_t)1); >>>   } >>> -static __rte_always_inline void * >>> -alloc_copy_ind_table(struct virtio_net *dev, struct vhost_virtqueue >>> *vq, >>> -        uint64_t desc_addr, uint64_t desc_len) >>> -{ >>> -    void *idesc; >>> -    uint64_t src, dst; >>> -    uint64_t len, remain = desc_len; >>> - >>> -    idesc = rte_malloc(__func__, desc_len, 0); >>> -    if (unlikely(!idesc)) >>> -        return 0; >>> - >>> -    dst = (uint64_t)(uintptr_t)idesc; >>> - >>> -    while (remain) { >>> -        len = remain; >>> -        src = vhost_iova_to_vva(dev, vq, desc_addr, &len, >>> -                VHOST_ACCESS_RO); >>> -        if (unlikely(!src || !len)) { >>> -            rte_free(idesc); >>> -            return 0; >>> -        } >>> - >>> -        rte_memcpy((void *)(uintptr_t)dst, (void *)(uintptr_t)src, >>> len); >>> - >>> -        remain -= len; >>> -        dst += len; >>> -        desc_addr += len; >>> -    } >>> - >>> -    return idesc; >>> -} >>> - >>>   static __rte_always_inline void >>>   free_ind_table(void *idesc) >>>   { >>> diff --git a/lib/librte_vhost/virtio_net.c >>> b/lib/librte_vhost/virtio_net.c >>> index 35ae4992c2..494dd9957e 100644 >>> --- a/lib/librte_vhost/virtio_net.c >>> +++ b/lib/librte_vhost/virtio_net.c >>> @@ -610,6 +610,35 @@ reserve_avail_buf_packed(struct virtio_net *dev, >>> struct vhost_virtqueue *vq, >>>       return 0; >>>   } >>> +static void >>> +copy_vnet_hdr_to_desc(struct virtio_net *dev, struct vhost_virtqueue >>> *vq, >> >> __rte_noinline? Or you don't care about this function being inlined or >> not? > > Right, I'll add it here and there in next revision. > > I'll try to send a patch to fix the kind of style issues you reported. > If you want to do it that would be great, just let me know. > I just figured it made sense to address some style issues when you were shuffling things around.