Re: [dpdk-dev] [PATCH] vhost: fix wrong IOTLB initialization

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 5/13/2021 3:38 PM, Kevin Traynor wrote:
> On 13/05/2021 15:11, David Marchand wrote:
>> On Thu, May 13, 2021 at 2:38 PM Chenbo Xia <chenbo.xia@intel.com> wrote:
>>>
>>> This patch fixes an issue of application crash because of vhost iotlb
>>> not initialized when virtio has multiqueue enabled.
>>>
>>> iotlb messages will be sent when some queues are not enabled. If we
>>> initialize iotlb in vhost_user_set_vring_num, it could happen that
>>> iotlb update comes when iotlb pool of disabled queues are not
>>> initialized.
>>
>> This makes the problem I reproduced disappear at init, but I noticed
>> the segfault after restarting testpmd once.
>> And a little bit after this, my vm crashed.
>>
>> This is not systematic, so I guess there is some condition with how
>> the virtio device is initialised in the vm.
>>
> 
> Ok, no point in Red Hat QA testing RC3 yet, if it is still faulty.
> 
> fyi - if you want to fix with a new patch it will likely delay Red Hat
> QA testing RC3 (maybe others?) and probably they will only have cycles
> for one RC3 test run.
> 
> If you choose to revert, we can ask Red Hat QA to test RC3 without
> further delay. Please let us know when you consider the options.
> 

If the patch is not good to go as it is I suggest reverting it, as far as I know
Chenbo will be off for Friday & Monday, so it doesn't leave much time to
update/test a new version.

>>
>> One question below.
>>
>>
>> Bugzilla ID: 703
>>
>>> Fixes: 968bbc7e2e50 ("vhost: avoid IOTLB mempool allocation while IOMMU disabled")
>>>
>>
>> Reported-by: Pei Zhang <pezhang@redhat.com>
>>
>>> Signed-off-by: Chenbo Xia <chenbo.xia@intel.com>
>>> ---
>>>  lib/vhost/vhost_user.c | 13 +++++++++----
>>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>>> index 611ff209e3..ae4df8eb69 100644
>>> --- a/lib/vhost/vhost_user.c
>>> +++ b/lib/vhost/vhost_user.c
>>> @@ -311,6 +311,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>         uint64_t features = msg->payload.u64;
>>>         uint64_t vhost_features = 0;
>>>         struct rte_vdpa_device *vdpa_dev;
>>> +       uint32_t i;
>>>
>>>         if (validate_msg_fds(msg, 0) != 0)
>>>                 return RTE_VHOST_MSG_RESULT_ERR;
>>> @@ -389,6 +390,14 @@ vhost_user_set_features(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>>                 vdpa_dev->ops->set_features(dev->vid);
>>>
>>>         dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>>> +
>>> +       if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) {
>>> +               for (i = 0; i < dev->nr_vring; i++) {
>>
>> I don't know the vhost-user protocol.
>> At this point of the device init/life, are we sure nr_vring is set to
>> the max number of vring?
>> The logs I have tend to say it is the case, but is there a guarantee
>> in the protocol?
>>
>>
>> Another way to fix would be to allocate on the first
>> VHOST_USER_IOTLB_MSG message received for a vring.
>>
>>
>>> +                       if (vhost_user_iotlb_init(dev, i))
>>> +                               return RTE_VHOST_MSG_RESULT_ERR;
>>> +               }
>>> +       }
>>> +
>>>         return RTE_VHOST_MSG_RESULT_OK;
>>>  }
>>>
>>
>>
>