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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@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.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


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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization






_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




[Index of Archives]     [KVM Development]     [Libvirt Development]     [Libvirt Users]     [CentOS Virtualization]     [Netdev]     [Ethernet Bridging]     [Linux Wireless]     [Kernel Newbies]     [Security]     [Linux for Hams]     [Netfilter]     [Bugtraq]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux RAID]     [Linux Admin]     [Samba]

  Powered by Linux