On 3/12/2021 1:52 PM, Jason Wang wrote:
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.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@xxxxxxxxx> --- 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.cindex 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.hindex 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.cindex 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?ThanksHi 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_PLATFORMYes, we always support ACCESS_PLATFORM, so it is hard coded in the macro IFCVF_SUPPORTED_FEATURES.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.That's not what I read from the code: features = ifcvf_get_features(vf) & IFCVF_SUPPORTED_FEATURES;
Agree, will check the features bit to set instead of device feature bits. Thanks!Now we check whether device support this feature bit as a double conformation, are you suggesting we should check whether ACCESS_PLATFORM & IFCVF_SUPPORTED_FEATURESin 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.ThanksThanks!ThanksThanks！+ if (ret) + return ret; vf->req_features = features;_______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
