Messages in this thread | | | Subject | Re: [PATCH V3 6/6] vDPA/ifcvf: verify mandatory feature bits for vDPA | From | "Zhu, Lingshan" <> | Date | Fri, 12 Mar 2021 14:40:30 +0800 |
| |
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 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 >>>> >>> >> >
| |