Jason Wang
2021-Mar-12 07:00 UTC
[PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA
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 at 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 bitsFor "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> 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 at lists.linux-foundation.org >>>>>> https://lists.linuxfoundation.org/mailman/listinfo/virtualization >>>>> >>>> >>> >> >