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 2D3FFA05D3 for ; Thu, 23 May 2019 16:31:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 03A471B951; Thu, 23 May 2019 16:31:14 +0200 (CEST) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 5E0145689 for ; Thu, 23 May 2019 16:31:12 +0200 (CEST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B03A63DBC7; Thu, 23 May 2019 14:31:05 +0000 (UTC) Received: from [10.36.112.60] (ovpn-112-60.ams2.redhat.com [10.36.112.60]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 7E47E620DF; Thu, 23 May 2019 14:30:59 +0000 (UTC) To: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= , 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> From: Maxime Coquelin Message-ID: <25057af9-8d1c-5e97-b648-6d344724291e@redhat.com> Date: Thu, 23 May 2019 16:30:57 +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: <2cfed6b0-8a7d-ec05-4b67-f40bd9409a8b@ericsson.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Thu, 23 May 2019 14:31:11 +0000 (UTC) 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" 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. 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) >> + >> +        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. Thanks, Maxime