From: "Mattias Rönnblom" <mattias.ronnblom@ericsson.com>
To: Maxime Coquelin <maxime.coquelin@redhat.com>,
dev@dpdk.org, tiwei.bie@intel.com, jfreimann@redhat.com,
zhihong.wang@intel.com, bruce.richardson@intel.com,
konstantin.ananyev@intel.com
Subject: Re: [dpdk-dev] [PATCH 3/5] vhost: do not inline unlikely fragmented buffers code
Date: Thu, 23 May 2019 17:17:36 +0200 [thread overview]
Message-ID: <4a91dd95-16bf-7b73-6cce-2ea8490e64d9@ericsson.com> (raw)
In-Reply-To: <25057af9-8d1c-5e97-b648-6d344724291e@redhat.com>
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 <maxime.coquelin@redhat.com>
>>> ---
>>> 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.
next prev parent reply other threads:[~2019-05-23 15:17 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-17 12:22 [dpdk-dev] [PATCH 0/5] vhost: I-cache pressure optimizations Maxime Coquelin
2019-05-17 12:22 ` [dpdk-dev] [PATCH 1/5] vhost: un-inline dirty pages logging functions Maxime Coquelin
2019-05-17 12:22 ` [dpdk-dev] [PATCH 2/5] vhost: do not inline packed and split functions Maxime Coquelin
2019-05-17 13:00 ` David Marchand
2019-05-17 14:42 ` Maxime Coquelin
2019-05-17 12:22 ` [dpdk-dev] [PATCH 3/5] vhost: do not inline unlikely fragmented buffers code Maxime Coquelin
2019-05-17 12:57 ` Maxime Coquelin
2019-05-21 19:43 ` Mattias Rönnblom
2019-05-23 14:30 ` Maxime Coquelin
2019-05-23 15:17 ` Mattias Rönnblom [this message]
2019-05-23 17:40 ` Maxime Coquelin
2019-05-17 12:22 ` [dpdk-dev] [PATCH 4/5] vhost: simplify descriptor's buffer prefetching Maxime Coquelin
2019-05-17 12:22 ` [dpdk-dev] [PATCH 5/5] eal/x86: force inlining of all memcpy and mov helpers Maxime Coquelin
2019-05-17 13:04 ` [dpdk-dev] [PATCH 0/5] vhost: I-cache pressure optimizations David Marchand
2019-05-17 14:42 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4a91dd95-16bf-7b73-6cce-2ea8490e64d9@ericsson.com \
--to=mattias.ronnblom@ericsson.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=jfreimann@redhat.com \
--cc=konstantin.ananyev@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=tiwei.bie@intel.com \
--cc=zhihong.wang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).