On 04.10.2018 18:06, Maxime Coquelin wrote:
>
>
> On 10/04/2018 04:59 PM, Ilya Maximets wrote:
>> On 04.10.2018 11:13, Maxime Coquelin wrote:
>>> When the memory table gets updated, the rings addresses need
>>> to be translated again. If it fails, we need to exit cleanly
>>> by unmapping memory regions.
>>>
>>> Fixes: d5022533c20a ("vhost: retranslate vring addr when memory table
>>> changes")
>>> Cc: sta...@dpdk.org
>>>
>>> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
>>> ---
>>
>> Acked-by: Ilya Maximets <i.maxim...@samsung.com>
>>
>> Minor comments inline.
>>
>>> lib/librte_vhost/vhost_user.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 8ffe5aa66..b6eae8dc5 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -964,7 +964,8 @@ vhost_user_set_mem_table(struct virtio_net **pdev,
>>> struct VhostUserMsg *msg)
>>> dev = translate_ring_addresses(dev, i);
>>> if (!dev)
>>> - return VH_RESULT_ERR;
>>> + goto err_mmap;
>>> +
>>
>> 1. No need to have two empty lines. (You could fix this while applying)
>
> Right. I will either fix it while applying, or it will be in next
> revision, if any.
>
>> 2. In current code, error on message handling will cause disconnect and
>> memory regions will be freed anyway. So, the change is not very
>> important for master (maybe just for consistency with surrounding
>> code) but it could be important for stable versions.
>
> I may not disconnect directly, as if reply-ack feature is negotiated[0],
> the slave would reply with a NACK and function handler will return 0.
> I guess that in that case the master (QEMU) will disconnect anyway, but
> this is just an assumption from slave point of view.
Yes, you're right. Thanks.
>
> [0]: REPLY_ACK protocol feature is only advertised when IOMMU support is
> requested because of a bug in an old upstream QEMU version.
>
> Thanks!
> Maxime
>
>>> *pdev = dev;
>>> }
>>>
>
>
