Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1

3 messages, 2 authors, 2021-09-15 · open the first message on its own page

Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1

From: "Michael S. Tsirkin" <mst@redhat.com>
Date: 2021-09-15 07:31:02 

On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
quoted
On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
quoted
This new attribute advertises whether the vdpa device is legacy or not.
Users can pick right virtqueue size if the vdpa device is legacy which
doesn't support to change virtqueue size.

Signed-off-by: Wu Zongyong <redacted>
So if we are bothering with legacy, I think there are
several things to do when building the interface
- support transitional devices, that is allow userspace
  to tell device it's in legacy mode
- support reporting/setting supporting endian-ness
It's true if we try to implement a general drvier for legacy.
But this series is dedicated to implement a driver only for ENI. Is it
necessary to implement what you said here in this series?
To a certain degree, yes.

I am thinking about the UAPI here. The new attribute is part of that.
E.g. userspace consuming this needs to be more or less hardware agnostic
and not depend on specifics of ENI.

Otherwise if userspace assumes legacy==eni then it will break with
other hardware.
 
One way to test how generic it all is would be adding legacy support in
the simulator.
quoted
quoted
---
 drivers/vdpa/vdpa.c          | 6 ++++++
 drivers/virtio/virtio_vdpa.c | 7 ++++++-
 include/uapi/linux/vdpa.h    | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1dc121a07a93..533d7f589eee 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/vdpa.h>
 #include <uapi/linux/vdpa.h>
+#include <uapi/linux/virtio_config.h>
 #include <net/genetlink.h>
 #include <linux/mod_devicetable.h>
 
@@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
 	u16 max_vq_size;
 	u32 device_id;
 	u32 vendor_id;
+	u64 features;
 	void *hdr;
 	int err;
 
@@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
 	device_id = vdev->config->get_device_id(vdev);
 	vendor_id = vdev->config->get_vendor_id(vdev);
 	max_vq_size = vdev->config->get_vq_num_max(vdev);
+	features = vdev->config->get_features(vdev);
 
 	err = -EMSGSIZE;
 	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev)))
@@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
 		goto msg_err;
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
 		goto msg_err;
+	if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
+	    nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
+		goto msg_err;
 
 	genlmsg_end(msg, hdr);
 	return 0;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 72eaef2caeb1..1cba957c4cdc 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -7,6 +7,7 @@
  *
  */
 
+#include "linux/virtio_config.h"
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/device.h>
@@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 	/* Assume split virtqueue, switch to packed if necessary */
 	struct vdpa_vq_state state = {0};
 	unsigned long flags;
+	bool may_reduce_num = false;
 	u32 align, num;
 	int err;
 
@@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
 		goto error_new_virtqueue;
 	}
 
+	if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
+		may_reduce_num = true;
+
 	/* Create the vring */
 	align = ops->get_vq_align(vdpa);
 	vq = vring_create_virtqueue(index, num, align, vdev,
-				    true, true, ctx,
+				    true, may_reduce_num, ctx,
 				    virtio_vdpa_notify, callback, name);
 	if (!vq) {
 		err = -ENOMEM;
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 66a41e4ec163..ce0b74276a5b 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -32,6 +32,7 @@ enum vdpa_attr {
 	VDPA_ATTR_DEV_VENDOR_ID,		/* u32 */
 	VDPA_ATTR_DEV_MAX_VQS,			/* u32 */
 	VDPA_ATTR_DEV_MAX_VQ_SIZE,		/* u16 */
+	VDPA_ATTR_DEV_VERSION_1,		/* flag */
 
 	/* new attributes must be added above here */
 	VDPA_ATTR_MAX,
-- 
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization





  

    
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1

    

      From: Jason Wang <hidden>

      Date: 2021-09-15 08:06:19 
    

  

  
On Wed, Sep 15, 2021 at 3:31 PM Michael S. Tsirkin [off-list ref] wrote:
On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
quoted
On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
quoted
On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
quoted
This new attribute advertises whether the vdpa device is legacy or not.
Users can pick right virtqueue size if the vdpa device is legacy which
doesn't support to change virtqueue size.

Signed-off-by: Wu Zongyong <redacted>
So if we are bothering with legacy, I think there are
several things to do when building the interface
- support transitional devices, that is allow userspace
  to tell device it's in legacy mode
- support reporting/setting supporting endian-ness
It's true if we try to implement a general drvier for legacy.
But this series is dedicated to implement a driver only for ENI. Is it
necessary to implement what you said here in this series?
To a certain degree, yes.

I am thinking about the UAPI here. The new attribute is part of that.
E.g. userspace consuming this needs to be more or less hardware agnostic
and not depend on specifics of ENI.

Otherwise if userspace assumes legacy==eni then it will break with
other hardware.

One way to test how generic it all is would be adding legacy support in
the simulator.
I don't get why we need to support legacy devices e.g it doesn't have
ACCESS_PLATFORM support. I think we should re-consider to mandate 1.0
devices.

https://lore.kernel.org/lkml/20210408082648.20145-1-jasowang@redhat.com/

And it will complicate all of the different layers.

Thanks

quoted
quoted
quoted
---
 drivers/vdpa/vdpa.c          | 6 ++++++
 drivers/virtio/virtio_vdpa.c | 7 ++++++-
 include/uapi/linux/vdpa.h    | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1dc121a07a93..533d7f589eee 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/vdpa.h>
 #include <uapi/linux/vdpa.h>
+#include <uapi/linux/virtio_config.h>
 #include <net/genetlink.h>
 #include <linux/mod_devicetable.h>

@@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
  u16 max_vq_size;
  u32 device_id;
  u32 vendor_id;
+ u64 features;
  void *hdr;
  int err;

@@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
  device_id = vdev->config->get_device_id(vdev);
  vendor_id = vdev->config->get_vendor_id(vdev);
  max_vq_size = vdev->config->get_vq_num_max(vdev);
+ features = vdev->config->get_features(vdev);

  err = -EMSGSIZE;
  if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev)))
@@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
          goto msg_err;
  if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
          goto msg_err;
+ if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
+     nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
+         goto msg_err;

  genlmsg_end(msg, hdr);
  return 0;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 72eaef2caeb1..1cba957c4cdc 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -7,6 +7,7 @@
  *
  */

+#include "linux/virtio_config.h"
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/device.h>
@@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
  /* Assume split virtqueue, switch to packed if necessary */
  struct vdpa_vq_state state = {0};
  unsigned long flags;
+ bool may_reduce_num = false;
  u32 align, num;
  int err;

@@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
          goto error_new_virtqueue;
  }

+ if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
+         may_reduce_num = true;
+
  /* Create the vring */
  align = ops->get_vq_align(vdpa);
  vq = vring_create_virtqueue(index, num, align, vdev,
-                             true, true, ctx,
+                             true, may_reduce_num, ctx,
                              virtio_vdpa_notify, callback, name);
  if (!vq) {
          err = -ENOMEM;
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 66a41e4ec163..ce0b74276a5b 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -32,6 +32,7 @@ enum vdpa_attr {
  VDPA_ATTR_DEV_VENDOR_ID,                /* u32 */
  VDPA_ATTR_DEV_MAX_VQS,                  /* u32 */
  VDPA_ATTR_DEV_MAX_VQ_SIZE,              /* u16 */
+ VDPA_ATTR_DEV_VERSION_1,                /* flag */

  /* new attributes must be added above here */
  VDPA_ATTR_MAX,
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization





  

    
Re: [PATCH v2 4/5] vdpa: add new vdpa attribute VDPA_ATTR_DEV_F_VERSION_1

    

      From: "Michael S. Tsirkin" <mst@redhat.com>

      Date: 2021-09-15 11:10:21 
    

  

  
On Wed, Sep 15, 2021 at 04:05:49PM +0800, Jason Wang wrote:
On Wed, Sep 15, 2021 at 3:31 PM Michael S. Tsirkin [off-list ref] wrote:
quoted
On Wed, Sep 15, 2021 at 11:24:53AM +0800, Wu Zongyong wrote:
quoted
On Tue, Sep 14, 2021 at 08:58:28AM -0400, Michael S. Tsirkin wrote:
quoted
On Tue, Sep 14, 2021 at 08:24:51PM +0800, Wu Zongyong wrote:
quoted
This new attribute advertises whether the vdpa device is legacy or not.
Users can pick right virtqueue size if the vdpa device is legacy which
doesn't support to change virtqueue size.

Signed-off-by: Wu Zongyong <redacted>
So if we are bothering with legacy, I think there are
several things to do when building the interface
- support transitional devices, that is allow userspace
  to tell device it's in legacy mode
- support reporting/setting supporting endian-ness
It's true if we try to implement a general drvier for legacy.
But this series is dedicated to implement a driver only for ENI. Is it
necessary to implement what you said here in this series?
To a certain degree, yes.

I am thinking about the UAPI here. The new attribute is part of that.
E.g. userspace consuming this needs to be more or less hardware agnostic
and not depend on specifics of ENI.

Otherwise if userspace assumes legacy==eni then it will break with
other hardware.

One way to test how generic it all is would be adding legacy support in
the simulator.
I don't get why we need to support legacy devices e.g it doesn't have
ACCESS_PLATFORM support. I think we should re-consider to mandate 1.0
devices.

https://lore.kernel.org/lkml/20210408082648.20145-1-jasowang@redhat.com/

And it will complicate all of the different layers.

Thanks

It's not that we have to, it's just that imho, if we do
it's easier to just do it all in the kernel rather than spreading
parts of code around.

quoted
quoted
quoted
quoted
---
 drivers/vdpa/vdpa.c          | 6 ++++++
 drivers/virtio/virtio_vdpa.c | 7 ++++++-
 include/uapi/linux/vdpa.h    | 1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1dc121a07a93..533d7f589eee 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -12,6 +12,7 @@
 #include <linux/slab.h>
 #include <linux/vdpa.h>
 #include <uapi/linux/vdpa.h>
+#include <uapi/linux/virtio_config.h>
 #include <net/genetlink.h>
 #include <linux/mod_devicetable.h>

@@ -494,6 +495,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
  u16 max_vq_size;
  u32 device_id;
  u32 vendor_id;
+ u64 features;
  void *hdr;
  int err;

@@ -508,6 +510,7 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
  device_id = vdev->config->get_device_id(vdev);
  vendor_id = vdev->config->get_vendor_id(vdev);
  max_vq_size = vdev->config->get_vq_num_max(vdev);
+ features = vdev->config->get_features(vdev);

  err = -EMSGSIZE;
  if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev)))
@@ -520,6 +523,9 @@ vdpa_dev_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq
          goto msg_err;
  if (nla_put_u16(msg, VDPA_ATTR_DEV_MAX_VQ_SIZE, max_vq_size))
          goto msg_err;
+ if (features & BIT_ULL(VIRTIO_F_VERSION_1) &&
+     nla_put_flag(msg, VDPA_ATTR_DEV_VERSION_1))
+         goto msg_err;

  genlmsg_end(msg, hdr);
  return 0;
diff --git a/drivers/virtio/virtio_vdpa.c b/drivers/virtio/virtio_vdpa.c
index 72eaef2caeb1..1cba957c4cdc 100644
--- a/drivers/virtio/virtio_vdpa.c
+++ b/drivers/virtio/virtio_vdpa.c
@@ -7,6 +7,7 @@
  *
  */

+#include "linux/virtio_config.h"
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/device.h>
@@ -145,6 +146,7 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
  /* Assume split virtqueue, switch to packed if necessary */
  struct vdpa_vq_state state = {0};
  unsigned long flags;
+ bool may_reduce_num = false;
  u32 align, num;
  int err;

@@ -169,10 +171,13 @@ virtio_vdpa_setup_vq(struct virtio_device *vdev, unsigned int index,
          goto error_new_virtqueue;
  }

+ if (ops->get_features(vdpa) & BIT_ULL(VIRTIO_F_VERSION_1))
+         may_reduce_num = true;
+
  /* Create the vring */
  align = ops->get_vq_align(vdpa);
  vq = vring_create_virtqueue(index, num, align, vdev,
-                             true, true, ctx,
+                             true, may_reduce_num, ctx,
                              virtio_vdpa_notify, callback, name);
  if (!vq) {
          err = -ENOMEM;
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 66a41e4ec163..ce0b74276a5b 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -32,6 +32,7 @@ enum vdpa_attr {
  VDPA_ATTR_DEV_VENDOR_ID,                /* u32 */
  VDPA_ATTR_DEV_MAX_VQS,                  /* u32 */
  VDPA_ATTR_DEV_MAX_VQ_SIZE,              /* u16 */
+ VDPA_ATTR_DEV_VERSION_1,                /* flag */

  /* new attributes must be added above here */
  VDPA_ATTR_MAX,
--
2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help