Date:   Thu, 11 Mar 2021 15:19:14 +0800
From:   "Zhu, Lingshan" <lingshan.zhu@...el.com>
To:     Jason Wang <jasowang@...hat.com>,
        Zhu Lingshan <lingshan.zhu@...ux.intel.com>, mst@...hat.com,
        lulu@...hat.com, leonro@...dia.com
Cc:     netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA



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@...el.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.
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? I prefer check both device and 
IFCVF_SUPPORTED_FEATURES both, more reliable.

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

Powered by blists - more mailing lists