Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA

From: Jason Wang <jasowang@redhat.com>
To: "Zhu, Lingshan" <lingshan.zhu@intel.com>,
	Zhu Lingshan <lingshan.zhu@linux.intel.com>,
	mst@redhat.com, lulu@redhat.com, leonro@nvidia.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	kvm@vger.kernel.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
Date: Fri, 12 Mar 2021 13:52:34 +0800	[thread overview]
Message-ID: <67be60b6-bf30-de85-ed42-d9fad974f42b@redhat.com> (raw)
In-Reply-To: <a1f346cc-c9fd-6d16-39d7-b59965a18b0a@intel.com>

On 2021/3/11 3:19 下午, Zhu, Lingshan wrote:
>
>
> On 3/11/2021 2:20 PM, Jason Wang wrote:
>>
>> On 2021/3/11 12:16 下午, Zhu Lingshan wrote:
>>>
>>>
>>> On 3/11/2021 11:20 AM, Jason Wang wrote:
>>>>
>>>> On 2021/3/10 5:00 下午, Zhu Lingshan wrote:
>>>>> vDPA requres VIRTIO_F_ACCESS_PLATFORM as a must, this commit
>>>>> examines this when set features.
>>>>>
>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> ---
>>>>>   drivers/vdpa/ifcvf/ifcvf_base.c | 8 ++++++++
>>>>>   drivers/vdpa/ifcvf/ifcvf_base.h | 1 +
>>>>>   drivers/vdpa/ifcvf/ifcvf_main.c | 5 +++++
>>>>>   3 files changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> index ea6a78791c9b..58f47fdce385 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>>>>> @@ -224,6 +224,14 @@ u64 ifcvf_get_features(struct ifcvf_hw *hw)
>>>>>       return hw->hw_features;
>>>>>   }
>>>>>   +int ifcvf_verify_min_features(struct ifcvf_hw *hw)
>>>>> +{
>>>>> +    if (!(hw->hw_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>>>> +        return -EINVAL;
>>>>> +
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   void ifcvf_read_net_config(struct ifcvf_hw *hw, u64 offset,
>>>>>                  void *dst, int length)
>>>>>   {
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> index dbb8c10aa3b1..91c5735d4dc9 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.h
>>>>> @@ -123,6 +123,7 @@ void io_write64_twopart(u64 val, u32 *lo, u32 
>>>>> *hi);
>>>>>   void ifcvf_reset(struct ifcvf_hw *hw);
>>>>>   u64 ifcvf_get_features(struct ifcvf_hw *hw);
>>>>>   u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
>>>>> +int ifcvf_verify_min_features(struct ifcvf_hw *hw);
>>>>>   u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
>>>>>   int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
>>>>>   struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
>>>>> diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c 
>>>>> b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> index 25fb9dfe23f0..f624f202447d 100644
>>>>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>>>>> @@ -179,6 +179,11 @@ static u64 ifcvf_vdpa_get_features(struct 
>>>>> vdpa_device *vdpa_dev)
>>>>>   static int ifcvf_vdpa_set_features(struct vdpa_device *vdpa_dev, 
>>>>> u64 features)
>>>>>   {
>>>>>       struct ifcvf_hw *vf = vdpa_to_vf(vdpa_dev);
>>>>> +    int ret;
>>>>> +
>>>>> +    ret = ifcvf_verify_min_features(vf);
>>>>
>>>>
>>>> So this validate device features instead of driver which is the one 
>>>> we really want to check?
>>>>
>>>> Thanks
>>>
>>> Hi Jason,
>>>
>>> Here we check device feature bits to make sure the device support 
>>> ACCESS_PLATFORM. 
>>
>>
>> If you want to check device features, you need to do that during 
>> probe() and fail the probing if without the feature. But I think you 
>> won't ship cards without ACCESS_PLATFORM.
> Yes, there are no reasons ship a card without ACCESS_PLATFORM
>>
>>
>>> In get_features(),
>>> it will return a intersection of device features bit and driver 
>>> supported features bits(which includes ACCESS_PLATFORM).
>>> Other components like QEMU should not set features bits more than 
>>> this intersection of bits. so we can make sure if this
>>> ifcvf_verify_min_features() passed, both device and driver support 
>>> ACCESS_PLATFORM.
>>>
>>> Are you suggesting check driver feature bits in 
>>> ifcvf_verify_min_features() in the meantime as well?
>>
>>
>> So it really depends on your hardware. If you hardware can always 
>> offer ACCESS_PLATFORM, you just need to check driver features. This 
>> is how vdpa_sim and mlx5_vdpa work.
> Yes, we always support ACCESS_PLATFORM, so it is hard coded in the 
> macro IFCVF_SUPPORTED_FEATURES.

That's not what I read from the code:

         features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;

> Now we check whether device support this feature bit as a double 
> conformation, are you suggesting we should check whether 
> ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURES
> in set_features() as well?

If we know device will always offer ACCESS_PLATFORM, there's no need to 
check it again. What we should check if whether driver set that, and if 
it doesn't we need to fail set_features(). I think there's little chance 
that IFCVF can work when IOMMU_PLATFORM is not negotiated.

> I prefer check both device and IFCVF_SUPPORTED_FEATURES both, more 
> reliable.

So again, if you want to check device features, set_features() is not 
the proper place. We need to fail the probe in this case.

Thanks

>
> Thanks!
>>
>> Thanks
>>
>>
>>>
>>> Thanks！
>>>>
>>>>
>>>>> +    if (ret)
>>>>> +        return ret;
>>>>>         vf->req_features = features;
>>>>
>>>> _______________________________________________
>>>> Virtualization mailing list
>>>> Virtualization@lists.linux-foundation.org
>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>>>
>>
>