Re: [dpdk-dev] [PATCH 3/5] vhost: do not inline unlikely fragmented buffers code

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

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.

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