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

["Zhu, Lingshan" <lingshan.zhu@intel.com>]

On 3/12/2021 3:00 PM, Jason Wang wrote:
>
> On 2021/3/12 2:40 下午, Zhu, Lingshan wrote:
>>
>>
>> On 3/12/2021 1:52 PM, Jason Wang wrote:
>>>
>>> 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;
>> ifcvf_get_features() reads device feature bits(which should always 
>> has ACCSSS_PLATFORM) and IFCVF_SUPPORTED_FEATURES is the driver 
>> supported feature bits 
>
>
> For "driver" you probably mean IFCVF. So there's some misunderstanding 
> before, what I meant for "driver" is virtio driver that do feature 
> negotaitation with the device.
>
> I wonder what features are supported by the device but not the IFCVF 
> driver?
>
> Thanks
we did not use TSO hardware feature bits in IFCVF driver for now. 
Anyway, we will check the features bits to set in set_features than 
hw/ifcvf driver feature bits.

THanks!
>
>
>> which hard coded ACCESS_PLATFORM, so the intersection should include 
>> ACCESS_PLATFORM.
>> the intersection "features" is returned in get_features(), qemu 
>> should set features according to it.
>>>
>>>
>>>> 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.
>> Agree, will check the features bit to set instead of device feature 
>> bits. Thanks!
>>>
>>>
>>>
>>>> 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
>>>>>>
>>>>>
>>>>
>>>
>>
>