[PATCH linux-next 0/9] vdpa: Enable user to set mac address,

[PATCH linux-next 0/9] vdpa: Enable user to set mac address,

[Parav Pandit]

Currently user cannot set the mac address and mtu of the vdpa device.
This patchset enables users to set the mac address and mtu of the vdpa
device once the device is created.
If a vendor driver supports such configuration user can set it otherwise
user gets unsupported error.

vdpa mac address and mtu are device configuration layout fields.
To keep interface generic enough for multiple types of vdpa devices, mac
address and mtu setting is implemented as configuration layout config
knobs.
This enables to use similar config layout for other virtio devices.

An example of query & set of config layout fields for vdpa_sim_net
driver:

Configuration layout fields are set after device is created.
This enables user to change such fields at later point without destroying and
recreating the device for new config.

$ vdpa mgmtdev show
vdpasim_net:
  supported_classes net

Add the device:
$ vdpa dev add name bar mgmtdev vdpasim_net

Configure mac address and mtu:
$ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000

In above command only mac address or only mtu can also be set.

View the config after setting:
$ vdpa dev config show
bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0

Patch summary:
Patch-1 uses read only features bit to detect endianness
Patch-2 implements new config layout query command
Patch-3 implements callback for setting vdpa net config fields
Patch-4 extends vdpa_sim_net driver to implement mac, mtu setting
Patch-5 removed redundant get_config callback
Patch-6 mlx5 vdpa driver migrates to user created vdpa device
Patch-7 mlx5 vdpa driver uses random mac address when not configured
Patch-8 mlx5 vdpa driver supports user provided mac config
Patch-9 mlx5 vdpa driver uses user provided mac during rx flow steering

Eli Cohen (4):
  vdpa/mlx5: Enable user to add/delete vdpa device
  vdpa/mlx5: Provide device generated random MAC address
  vdpa/mlx5: Support configuration of MAC
  vdpa/mlx5: Forward only packets with allowed MAC address

Parav Pandit (5):
  vdpa_sim: Consider read only supported features instead of current
  vdpa: Introduce query of device config layout
  vdpa: Enable user to set mac and mtu of vdpa device
  vdpa_sim_net: Enable user to set mac address and mtu
  vdpa_sim_net: Remove redundant get_config callback

 drivers/vdpa/mlx5/net/mlx5_vnet.c    | 185 ++++++++++++++----
 drivers/vdpa/vdpa.c                  | 270 +++++++++++++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim.h     |   4 +-
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  49 +++--
 include/linux/vdpa.h                 |  16 ++
 include/uapi/linux/vdpa.h            |  12 ++
 6 files changed, 476 insertions(+), 60 deletions(-)

-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current

[Parav Pandit]

To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
consider the read only supported features bit instead of current
features bit which can be modified by the driver.

This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
enough just after vdpasim device creation in subsequent patch.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
index 6d75444f9948..176d641a0939 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
@@ -11,6 +11,7 @@
 #include <linux/virtio_byteorder.h>
 #include <linux/vhost_iotlb.h>
 #include <uapi/linux/virtio_config.h>
+#include <linux/bits.h>
 
 #define VDPASIM_FEATURES	((1ULL << VIRTIO_F_ANY_LAYOUT) | \
 				 (1ULL << VIRTIO_F_VERSION_1)  | \
@@ -71,7 +72,8 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr);
 static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
 {
 	return virtio_legacy_is_little_endian() ||
-		(vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
+		(vdpasim->dev_attr.supported_features &
+		 BIT_ULL(VIRTIO_F_VERSION_1));
 }
 
 static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit wrote:
> To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
> consider the read only supported features bit instead of current
> features bit which can be modified by the driver.
> 
> This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
> enough just after vdpasim device creation in subsequent patch.
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>

Well that works for legacy and modern devices but not for transitional
ones. Without transitional device support vendors are reluctant to add
modern features since that will break old guests ...  I suspect we need
to either add a new ioctl enabling modern mode, or abuse SET_FEATURES
and call it from qemu on first config space access.

> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> index 6d75444f9948..176d641a0939 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
> @@ -11,6 +11,7 @@
>  #include <linux/virtio_byteorder.h>
>  #include <linux/vhost_iotlb.h>
>  #include <uapi/linux/virtio_config.h>
> +#include <linux/bits.h>
>  
>  #define VDPASIM_FEATURES	((1ULL << VIRTIO_F_ANY_LAYOUT) | \
>  				 (1ULL << VIRTIO_F_VERSION_1)  | \
> @@ -71,7 +72,8 @@ struct vdpasim *vdpasim_create(struct vdpasim_dev_attr *attr);
>  static inline bool vdpasim_is_little_endian(struct vdpasim *vdpasim)
>  {
>  	return virtio_legacy_is_little_endian() ||
> -		(vdpasim->features & (1ULL << VIRTIO_F_VERSION_1));
> +		(vdpasim->dev_attr.supported_features &
> +		 BIT_ULL(VIRTIO_F_VERSION_1));
>  }
>  
>  static inline u16 vdpasim16_to_cpu(struct vdpasim *vdpasim, __virtio16 val)
> -- 
> 2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current

[Parav Pandit]

Hi Michael, Jason,

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 24, 2021 12:40 PM
> 
> On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit wrote:
> > To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
> > consider the read only supported features bit instead of current
> > features bit which can be modified by the driver.
> >
> > This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
> > enough just after vdpasim device creation in subsequent patch.
> >
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > Reviewed-by: Eli Cohen <elic@nvidia.com>
> 
> Well that works for legacy and modern devices but not for transitional ones.
> Without transitional device support vendors are reluctant to add modern
> features since that will break old guests ...  I suspect we need to either add a
> new ioctl enabling modern mode, or abuse SET_FEATURES and call it from
> qemu on first config space access.
> 

From mgmt tool kernel point of view,
(a) config space decoding depends on current feature version bit
(b) feature get/set and config read are not atomic callbacks 

Mgmt. tool kernel code need to read them in single call from the vendor driver.
I need to add mgmt_dev->ops->get_features_config(struct virtio_features_config*) calllback that returns value of both atomically in structure like below.

struct virtio_features_config {
	u64 features;
	union {
		struct virtio_net_config net;
		struct virtio_block_config blk;
	} u;
}

What do you think?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 1/9] vdpa_sim: Consider read only supported features instead of current

[Jason Wang]

On 2021/2/26 3:36 下午, Parav Pandit wrote:
> Hi Michael, Jason,
>
>> From: Michael S. Tsirkin <mst@redhat.com>
>> Sent: Wednesday, February 24, 2021 12:40 PM
>>
>> On Wed, Feb 24, 2021 at 08:18:36AM +0200, Parav Pandit wrote:
>>> To honor VIRTIO_F_VERSION_1 feature bit, during endianness detection,
>>> consider the read only supported features bit instead of current
>>> features bit which can be modified by the driver.
>>>
>>> This enables vdpa_sim_net driver to invoke cpu_to_vdpasim16() early
>>> enough just after vdpasim device creation in subsequent patch.
>>>
>>> Signed-off-by: Parav Pandit <parav@nvidia.com>
>>> Reviewed-by: Eli Cohen <elic@nvidia.com>
>> Well that works for legacy and modern devices but not for transitional ones.
>> Without transitional device support vendors are reluctant to add modern
>> features since that will break old guests ...  I suspect we need to either add a
>> new ioctl enabling modern mode, or abuse SET_FEATURES and call it from
>> qemu on first config space access.
>>
>  From mgmt tool kernel point of view,
> (a) config space decoding depends on current feature version bit
> (b) feature get/set and config read are not atomic callbacks
>
> Mgmt. tool kernel code need to read them in single call from the vendor driver.
> I need to add mgmt_dev->ops->get_features_config(struct virtio_features_config*) calllback that returns value of both atomically in structure like below.
>
> struct virtio_features_config {
> 	u64 features;
> 	union {
> 		struct virtio_net_config net;
> 		struct virtio_block_config blk;
> 	} u;
> }
>
> What do you think?


I'm wait for Michael to clairfy how the dependency will look like. E.g 
without multiqueue, should we consider the config as:

struct virtio_net_config {
         u8 mac[6];
         le16 status;
         le16 reserved;
         le16 mtu;
};

or

struct virtio_net_config {
         u8 mac[6];
         le16 status;
         le16 mtu;
};

And I wonder whether or not to reuse config is the best choice which 
needs to deal with feature.

Thanks


>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Parav Pandit]

Introduce a command to query a device config layout.

An example query of network vdpa device:

$ vdpa dev add name bar mgmtdev vdpasim_net

$ vdpa dev config show
bar: mac 00:35:09:19:48:05 link up link_announce false mtu 1500 speed 0 duplex 0

$ vdpa dev config show -jp
{
    "config": {
        "bar": {
            "mac": "00:35:09:19:48:05",
            "link ": "up",
            "link_announce ": false,
            "mtu": 1500,
            "speed": 0,
            "duplex": 0
        }
    }
}

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
changelog:
v1->v2:
 - read whole net config layout instead of individual fields
 - added error extack for unmanaged vdpa device
---
 drivers/vdpa/vdpa.c       | 181 ++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vdpa.h |  11 +++
 2 files changed, 192 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3d997b389345..cebbba500638 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -14,6 +14,8 @@
 #include <uapi/linux/vdpa.h>
 #include <net/genetlink.h>
 #include <linux/mod_devicetable.h>
+#include <linux/virtio_net.h>
+#include <linux/virtio_ids.h>
 
 static LIST_HEAD(mdev_head);
 /* A global mutex that protects vdpa management device and device level operations. */
@@ -603,6 +605,179 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
 	return msg->len;
 }
 
+static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
+				       struct sk_buff *msg,
+				       const struct virtio_net_config *config)
+{
+	u32 hash_types;
+	u16 rss_field;
+	u64 features;
+
+	features = vdev->config->get_features(vdev);
+	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
+		return 0;
+
+	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
+			config->max_virtqueue_pairs))
+		return -EMSGSIZE;
+	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
+		       config->rss_max_key_size))
+		return -EMSGSIZE;
+
+	rss_field = le16_to_cpu(config->rss_max_key_size);
+	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, rss_field))
+		return -EMSGSIZE;
+
+	hash_types = le32_to_cpu(config->supported_hash_types);
+	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
+			config->supported_hash_types))
+		return -EMSGSIZE;
+	return 0;
+}
+
+static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
+{
+	struct virtio_net_config config = {};
+
+	vdev->config->get_config(vdev, 0, &config, sizeof(config));
+	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), config.mac))
+		return -EMSGSIZE;
+	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
+		return -EMSGSIZE;
+	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
+		return -EMSGSIZE;
+	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, config.speed))
+		return -EMSGSIZE;
+	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, config.duplex))
+		return -EMSGSIZE;
+
+	return vdpa_dev_net_mq_config_fill(vdev, msg, &config);
+}
+
+static int
+vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
+		     int flags, struct netlink_ext_ack *extack)
+{
+	u32 device_id;
+	void *hdr;
+	int err;
+
+	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
+			  VDPA_CMD_DEV_CONFIG_GET);
+	if (!hdr)
+		return -EMSGSIZE;
+
+	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
+		err = -EMSGSIZE;
+		goto msg_err;
+	}
+
+	device_id = vdev->config->get_device_id(vdev);
+	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
+		err = -EMSGSIZE;
+		goto msg_err;
+	}
+
+	switch (device_id) {
+	case VIRTIO_ID_NET:
+		err = vdpa_dev_net_config_fill(vdev, msg);
+		break;
+	default:
+		err = -EOPNOTSUPP;
+		break;
+	}
+	if (err)
+		goto msg_err;
+
+	genlmsg_end(msg, hdr);
+	return 0;
+
+msg_err:
+	genlmsg_cancel(msg, hdr);
+	return err;
+}
+
+static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct vdpa_device *vdev;
+	struct sk_buff *msg;
+	const char *devname;
+	struct device *dev;
+	int err;
+
+	if (!info->attrs[VDPA_ATTR_DEV_NAME])
+		return -EINVAL;
+	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
+	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
+	if (!msg)
+		return -ENOMEM;
+
+	mutex_lock(&vdpa_dev_mutex);
+	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
+	if (!dev) {
+		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
+		err = -ENODEV;
+		goto dev_err;
+	}
+	vdev = container_of(dev, struct vdpa_device, dev);
+	if (!vdev->mdev) {
+		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
+		err = -EINVAL;
+		goto mdev_err;
+	}
+	err = vdpa_dev_config_fill(vdev, msg, info->snd_portid, info->snd_seq,
+				   0, info->extack);
+	if (!err)
+		err = genlmsg_reply(msg, info);
+
+mdev_err:
+	put_device(dev);
+dev_err:
+	mutex_unlock(&vdpa_dev_mutex);
+	if (err)
+		nlmsg_free(msg);
+	return err;
+}
+
+static int vdpa_dev_config_dump(struct device *dev, void *data)
+{
+	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
+	struct vdpa_dev_dump_info *info = data;
+	int err;
+
+	if (!vdev->mdev)
+		return 0;
+	if (info->idx < info->start_idx) {
+		info->idx++;
+		return 0;
+	}
+	err = vdpa_dev_config_fill(vdev, info->msg, NETLINK_CB(info->cb->skb).portid,
+				   info->cb->nlh->nlmsg_seq, NLM_F_MULTI,
+				   info->cb->extack);
+	if (err)
+		return err;
+
+	info->idx++;
+	return 0;
+}
+
+static int
+vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
+{
+	struct vdpa_dev_dump_info info;
+
+	info.msg = msg;
+	info.cb = cb;
+	info.start_idx = cb->args[0];
+	info.idx = 0;
+
+	mutex_lock(&vdpa_dev_mutex);
+	bus_for_each_dev(&vdpa_bus, NULL, &info, vdpa_dev_config_dump);
+	mutex_unlock(&vdpa_dev_mutex);
+	cb->args[0] = info.idx;
+	return msg->len;
+}
+
 static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
 	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
@@ -634,6 +809,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
 		.doit = vdpa_nl_cmd_dev_get_doit,
 		.dumpit = vdpa_nl_cmd_dev_get_dumpit,
 	},
+	{
+		.cmd = VDPA_CMD_DEV_CONFIG_GET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = vdpa_nl_cmd_dev_config_get_doit,
+		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
+	},
 };
 
 static struct genl_family vdpa_nl_family __ro_after_init = {
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 66a41e4ec163..5c31ecc3b956 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -17,6 +17,7 @@ enum vdpa_command {
 	VDPA_CMD_DEV_NEW,
 	VDPA_CMD_DEV_DEL,
 	VDPA_CMD_DEV_GET,		/* can dump */
+	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
 };
 
 enum vdpa_attr {
@@ -33,6 +34,16 @@ enum vdpa_attr {
 	VDPA_ATTR_DEV_MAX_VQS,			/* u32 */
 	VDPA_ATTR_DEV_MAX_VQ_SIZE,		/* u16 */
 
+	VDPA_ATTR_DEV_NET_CFG_MACADDR,		/* binary */
+	VDPA_ATTR_DEV_NET_STATUS,		/* u8 */
+	VDPA_ATTR_DEV_NET_CFG_MAX_VQP,		/* u16 */
+	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
+	VDPA_ATTR_DEV_NET_CFG_SPEED,		/* u16 */
+	VDPA_ATTR_DEV_NET_CFG_DUPLEX,		/* u16 */
+	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,	/* u8 */
+	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,	/* u16 */
+	VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,	/* u32 */
+
 	/* new attributes must be added above here */
 	VDPA_ATTR_MAX,
 };
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 08:18:37AM +0200, Parav Pandit wrote:
> Introduce a command to query a device config layout.
> 
> An example query of network vdpa device:
> 
> $ vdpa dev add name bar mgmtdev vdpasim_net
> 
> $ vdpa dev config show
> bar: mac 00:35:09:19:48:05 link up link_announce false mtu 1500 speed 0 duplex 0
> 
> $ vdpa dev config show -jp
> {
>     "config": {
>         "bar": {
>             "mac": "00:35:09:19:48:05",
>             "link ": "up",
>             "link_announce ": false,
>             "mtu": 1500,
>             "speed": 0,
>             "duplex": 0
>         }
>     }
> }
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>
> ---
> changelog:
> v1->v2:
>  - read whole net config layout instead of individual fields
>  - added error extack for unmanaged vdpa device
> ---
>  drivers/vdpa/vdpa.c       | 181 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vdpa.h |  11 +++
>  2 files changed, 192 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3d997b389345..cebbba500638 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -14,6 +14,8 @@
>  #include <uapi/linux/vdpa.h>
>  #include <net/genetlink.h>
>  #include <linux/mod_devicetable.h>
> +#include <linux/virtio_net.h>
> +#include <linux/virtio_ids.h>
>  
>  static LIST_HEAD(mdev_head);
>  /* A global mutex that protects vdpa management device and device level operations. */
> @@ -603,6 +605,179 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
>  	return msg->len;
>  }
>  
> +static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> +				       struct sk_buff *msg,
> +				       const struct virtio_net_config *config)
> +{
> +	u32 hash_types;
> +	u16 rss_field;
> +	u64 features;
> +
> +	features = vdev->config->get_features(vdev);
> +	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
> +		return 0;
> +
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> +			config->max_virtqueue_pairs))
> +		return -EMSGSIZE;
> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> +		       config->rss_max_key_size))

Why is it ok to poke at RSS fields without checking the relevant feature bits?

> +		return -EMSGSIZE;

Did you check this with sparse?
max_virtqueue_pairs is __virtio16.

> +
> +	rss_field = le16_to_cpu(config->rss_max_key_size);
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, rss_field))
> +		return -EMSGSIZE;
> +
> +	hash_types = le32_to_cpu(config->supported_hash_types);

unused variable

> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> +			config->supported_hash_types))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> +{
> +	struct virtio_net_config config = {};
> +
> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
> +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), config.mac))
> +		return -EMSGSIZE;
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
> +		return -EMSGSIZE;
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
> +		return -EMSGSIZE;
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, config.speed))
> +		return -EMSGSIZE;

looks like a bunch of endian-ness/sparse errors to me, and
a bunch of fields checked without checking the feature bits.

> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, config.duplex))
> +		return -EMSGSIZE;
> +
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, &config);
> +}
> +
> +static int
> +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
> +		     int flags, struct netlink_ext_ack *extack)
> +{
> +	u32 device_id;
> +	void *hdr;
> +	int err;
> +
> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> +			  VDPA_CMD_DEV_CONFIG_GET);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> +		err = -EMSGSIZE;
> +		goto msg_err;
> +	}
> +
> +	device_id = vdev->config->get_device_id(vdev);
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> +		err = -EMSGSIZE;
> +		goto msg_err;
> +	}
> +
> +	switch (device_id) {
> +	case VIRTIO_ID_NET:
> +		err = vdpa_dev_net_config_fill(vdev, msg);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +	if (err)
> +		goto msg_err;
> +
> +	genlmsg_end(msg, hdr);
> +	return 0;
> +
> +msg_err:
> +	genlmsg_cancel(msg, hdr);
> +	return err;
> +}
> +
> +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct vdpa_device *vdev;
> +	struct sk_buff *msg;
> +	const char *devname;
> +	struct device *dev;
> +	int err;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> +		return -EINVAL;
> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vdpa_dev_mutex);
> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> +	if (!dev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> +		err = -ENODEV;
> +		goto dev_err;
> +	}
> +	vdev = container_of(dev, struct vdpa_device, dev);
> +	if (!vdev->mdev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> +		err = -EINVAL;
> +		goto mdev_err;
> +	}
> +	err = vdpa_dev_config_fill(vdev, msg, info->snd_portid, info->snd_seq,
> +				   0, info->extack);
> +	if (!err)
> +		err = genlmsg_reply(msg, info);
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	mutex_unlock(&vdpa_dev_mutex);
> +	if (err)
> +		nlmsg_free(msg);
> +	return err;
> +}
> +
> +static int vdpa_dev_config_dump(struct device *dev, void *data)
> +{
> +	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> +	struct vdpa_dev_dump_info *info = data;
> +	int err;
> +
> +	if (!vdev->mdev)
> +		return 0;
> +	if (info->idx < info->start_idx) {
> +		info->idx++;
> +		return 0;
> +	}
> +	err = vdpa_dev_config_fill(vdev, info->msg, NETLINK_CB(info->cb->skb).portid,
> +				   info->cb->nlh->nlmsg_seq, NLM_F_MULTI,
> +				   info->cb->extack);
> +	if (err)
> +		return err;
> +
> +	info->idx++;
> +	return 0;
> +}
> +
> +static int
> +vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
> +{
> +	struct vdpa_dev_dump_info info;
> +
> +	info.msg = msg;
> +	info.cb = cb;
> +	info.start_idx = cb->args[0];
> +	info.idx = 0;
> +
> +	mutex_lock(&vdpa_dev_mutex);
> +	bus_for_each_dev(&vdpa_bus, NULL, &info, vdpa_dev_config_dump);
> +	mutex_unlock(&vdpa_dev_mutex);
> +	cb->args[0] = info.idx;
> +	return msg->len;
> +}
> +
>  static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>  	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>  	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> @@ -634,6 +809,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>  		.doit = vdpa_nl_cmd_dev_get_doit,
>  		.dumpit = vdpa_nl_cmd_dev_get_dumpit,
>  	},
> +	{
> +		.cmd = VDPA_CMD_DEV_CONFIG_GET,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = vdpa_nl_cmd_dev_config_get_doit,
> +		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> +	},
>  };
>  
>  static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 66a41e4ec163..5c31ecc3b956 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -17,6 +17,7 @@ enum vdpa_command {
>  	VDPA_CMD_DEV_NEW,
>  	VDPA_CMD_DEV_DEL,
>  	VDPA_CMD_DEV_GET,		/* can dump */
> +	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>  };
>  
>  enum vdpa_attr {
> @@ -33,6 +34,16 @@ enum vdpa_attr {
>  	VDPA_ATTR_DEV_MAX_VQS,			/* u32 */
>  	VDPA_ATTR_DEV_MAX_VQ_SIZE,		/* u16 */
>  
> +	VDPA_ATTR_DEV_NET_CFG_MACADDR,		/* binary */
> +	VDPA_ATTR_DEV_NET_STATUS,		/* u8 */
> +	VDPA_ATTR_DEV_NET_CFG_MAX_VQP,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_SPEED,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_DUPLEX,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,	/* u8 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,	/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,	/* u32 */
> +
>  	/* new attributes must be added above here */
>  	VDPA_ATTR_MAX,
>  };
> -- 
> 2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 24, 2021 12:33 PM
> 
> > +
> > +	features = vdev->config->get_features(vdev);
> > +	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
> > +		return 0;
> > +
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> > +			config->max_virtqueue_pairs))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u8(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> > +		       config->rss_max_key_size))
> 
> Why is it ok to poke at RSS fields without checking the relevant feature bits?
> 
It should be checked.
I relied on _MQ feature bit but yes, even with MQ, RSS can be off.
Will fix to read RSS feature bit.

> > +		return -EMSGSIZE;
> 
> Did you check this with sparse?
> max_virtqueue_pairs is __virtio16.
I will check and go through the types.

> 
> > +
> > +	rss_field = le16_to_cpu(config->rss_max_key_size);
> > +	if (nla_put_u16(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, rss_field))
> > +		return -EMSGSIZE;
> > +
> > +	hash_types = le32_to_cpu(config->supported_hash_types);
> 
> unused variable
Will remove.

> 
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> > +			config->supported_hash_types))
> > +		return -EMSGSIZE;
> > +	return 0;
> > +}
> > +
> > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct
> > +sk_buff *msg) {
> > +	struct virtio_net_config config = {};
> > +
> > +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
> > +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> sizeof(config.mac), config.mac))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU,
> config.mtu))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED,
> config.speed))
> > +		return -EMSGSIZE;
> 
> looks like a bunch of endian-ness/sparse errors to me, and a bunch of fields
> checked without checking the feature bits.
Yes, few are missed out.
Fixing them in v2.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Jason Wang]

On 2021/2/24 2:18 下午, Parav Pandit wrote:
> Introduce a command to query a device config layout.
>
> An example query of network vdpa device:
>
> $ vdpa dev add name bar mgmtdev vdpasim_net
>
> $ vdpa dev config show
> bar: mac 00:35:09:19:48:05 link up link_announce false mtu 1500 speed 0 duplex 0
>
> $ vdpa dev config show -jp
> {
>      "config": {
>          "bar": {
>              "mac": "00:35:09:19:48:05",
>              "link ": "up",
>              "link_announce ": false,
>              "mtu": 1500,
>              "speed": 0,
>              "duplex": 0
>          }
>      }
> }
>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>
> ---
> changelog:
> v1->v2:
>   - read whole net config layout instead of individual fields
>   - added error extack for unmanaged vdpa device
> ---
>   drivers/vdpa/vdpa.c       | 181 ++++++++++++++++++++++++++++++++++++++
>   include/uapi/linux/vdpa.h |  11 +++
>   2 files changed, 192 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3d997b389345..cebbba500638 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -14,6 +14,8 @@
>   #include <uapi/linux/vdpa.h>
>   #include <net/genetlink.h>
>   #include <linux/mod_devicetable.h>
> +#include <linux/virtio_net.h>
> +#include <linux/virtio_ids.h>
>   
>   static LIST_HEAD(mdev_head);
>   /* A global mutex that protects vdpa management device and device level operations. */
> @@ -603,6 +605,179 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
>   	return msg->len;
>   }
>   
> +static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> +				       struct sk_buff *msg,
> +				       const struct virtio_net_config *config)
> +{
> +	u32 hash_types;
> +	u16 rss_field;
> +	u64 features;
> +
> +	features = vdev->config->get_features(vdev);
> +	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
> +		return 0;
> +
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> +			config->max_virtqueue_pairs))
> +		return -EMSGSIZE;


We probably need to check VIRTIO_NET_F_RSS here.


> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> +		       config->rss_max_key_size))
> +		return -EMSGSIZE;
> +
> +	rss_field = le16_to_cpu(config->rss_max_key_size);
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, rss_field))
> +		return -EMSGSIZE;
> +
> +	hash_types = le32_to_cpu(config->supported_hash_types);
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> +			config->supported_hash_types))
> +		return -EMSGSIZE;
> +	return 0;
> +}
> +
> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> +{
> +	struct virtio_net_config config = {};
> +
> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));


Do we need to sync with other possible get_config calls here?


> +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), config.mac))
> +		return -EMSGSIZE;
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
> +		return -EMSGSIZE;
> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
> +		return -EMSGSIZE;


And check VIRTIO_NET_F_SPEED_DUPLEX.


> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, config.speed))
> +		return -EMSGSIZE;
> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, config.duplex))
> +		return -EMSGSIZE;
> +
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, &config);
> +}
> +
> +static int
> +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
> +		     int flags, struct netlink_ext_ack *extack)
> +{
> +	u32 device_id;
> +	void *hdr;
> +	int err;
> +
> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> +			  VDPA_CMD_DEV_CONFIG_GET);
> +	if (!hdr)
> +		return -EMSGSIZE;
> +
> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev->dev))) {
> +		err = -EMSGSIZE;
> +		goto msg_err;
> +	}
> +
> +	device_id = vdev->config->get_device_id(vdev);
> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> +		err = -EMSGSIZE;
> +		goto msg_err;
> +	}
> +
> +	switch (device_id) {
> +	case VIRTIO_ID_NET:
> +		err = vdpa_dev_net_config_fill(vdev, msg);
> +		break;
> +	default:
> +		err = -EOPNOTSUPP;
> +		break;
> +	}
> +	if (err)
> +		goto msg_err;
> +
> +	genlmsg_end(msg, hdr);
> +	return 0;
> +
> +msg_err:
> +	genlmsg_cancel(msg, hdr);
> +	return err;
> +}
> +
> +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct vdpa_device *vdev;
> +	struct sk_buff *msg;
> +	const char *devname;
> +	struct device *dev;
> +	int err;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> +		return -EINVAL;
> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> +	if (!msg)
> +		return -ENOMEM;
> +
> +	mutex_lock(&vdpa_dev_mutex);
> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> +	if (!dev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> +		err = -ENODEV;
> +		goto dev_err;
> +	}
> +	vdev = container_of(dev, struct vdpa_device, dev);
> +	if (!vdev->mdev) {
> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa device");
> +		err = -EINVAL;
> +		goto mdev_err;
> +	}


Though it's fine but it looks to me mdev is not required to work here.


> +	err = vdpa_dev_config_fill(vdev, msg, info->snd_portid, info->snd_seq,
> +				   0, info->extack);
> +	if (!err)
> +		err = genlmsg_reply(msg, info);
> +
> +mdev_err:
> +	put_device(dev);
> +dev_err:
> +	mutex_unlock(&vdpa_dev_mutex);
> +	if (err)
> +		nlmsg_free(msg);
> +	return err;
> +}
> +
> +static int vdpa_dev_config_dump(struct device *dev, void *data)
> +{
> +	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
> +	struct vdpa_dev_dump_info *info = data;
> +	int err;
> +
> +	if (!vdev->mdev)
> +		return 0;
> +	if (info->idx < info->start_idx) {
> +		info->idx++;
> +		return 0;
> +	}
> +	err = vdpa_dev_config_fill(vdev, info->msg, NETLINK_CB(info->cb->skb).portid,
> +				   info->cb->nlh->nlmsg_seq, NLM_F_MULTI,
> +				   info->cb->extack);
> +	if (err)
> +		return err;
> +
> +	info->idx++;
> +	return 0;
> +}
> +
> +static int
> +vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
> +{
> +	struct vdpa_dev_dump_info info;
> +
> +	info.msg = msg;
> +	info.cb = cb;
> +	info.start_idx = cb->args[0];
> +	info.idx = 0;
> +
> +	mutex_lock(&vdpa_dev_mutex);
> +	bus_for_each_dev(&vdpa_bus, NULL, &info, vdpa_dev_config_dump);
> +	mutex_unlock(&vdpa_dev_mutex);
> +	cb->args[0] = info.idx;
> +	return msg->len;
> +}
> +
>   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
> @@ -634,6 +809,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>   		.doit = vdpa_nl_cmd_dev_get_doit,
>   		.dumpit = vdpa_nl_cmd_dev_get_dumpit,
>   	},
> +	{
> +		.cmd = VDPA_CMD_DEV_CONFIG_GET,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = vdpa_nl_cmd_dev_config_get_doit,
> +		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> +	},
>   };
>   
>   static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 66a41e4ec163..5c31ecc3b956 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -17,6 +17,7 @@ enum vdpa_command {
>   	VDPA_CMD_DEV_NEW,
>   	VDPA_CMD_DEV_DEL,
>   	VDPA_CMD_DEV_GET,		/* can dump */
> +	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
>   };
>   
>   enum vdpa_attr {
> @@ -33,6 +34,16 @@ enum vdpa_attr {
>   	VDPA_ATTR_DEV_MAX_VQS,			/* u32 */
>   	VDPA_ATTR_DEV_MAX_VQ_SIZE,		/* u16 */
>   
> +	VDPA_ATTR_DEV_NET_CFG_MACADDR,		/* binary */
> +	VDPA_ATTR_DEV_NET_STATUS,		/* u8 */
> +	VDPA_ATTR_DEV_NET_CFG_MAX_VQP,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_SPEED,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_DUPLEX,		/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,	/* u8 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,	/* u16 */
> +	VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,	/* u32 */
> +
>   	/* new attributes must be added above here */
>   	VDPA_ATTR_MAX,
>   };

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, February 24, 2021 2:18 PM
> 
> On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > +
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> > +			config->max_virtqueue_pairs))
> > +		return -EMSGSIZE;
> 
> 
> We probably need to check VIRTIO_NET_F_RSS here.

Yes. Will use it in v2.

> 
> 
> > +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> > +		       config->rss_max_key_size))
> > +		return -EMSGSIZE;
> > +
> > +	rss_field = le16_to_cpu(config->rss_max_key_size);
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
> rss_field))
> > +		return -EMSGSIZE;
> > +
> > +	hash_types = le32_to_cpu(config->supported_hash_types);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> > +			config->supported_hash_types))
> > +		return -EMSGSIZE;
> > +	return 0;
> > +}
> > +
> > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct
> > +sk_buff *msg) {
> > +	struct virtio_net_config config = {};
> > +
> > +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
> 
> 
> Do we need to sync with other possible get_config calls here?

To readers of get_config() is ok. No need to sync.
However, I think set_config() and get_config() should sync otherwise get_config can get partial value.
Will probably have to add vdpa_device->config_access_lock().

> 
> 
> > +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> sizeof(config.mac), config.mac))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
> > +		return -EMSGSIZE;
> 
> 
> And check VIRTIO_NET_F_SPEED_DUPLEX.

Yes, will do.

> 
> 
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED,
> config.speed))
> > +		return -EMSGSIZE;
> > +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX,
> config.duplex))
> > +		return -EMSGSIZE;
> > +
> > +	return vdpa_dev_net_mq_config_fill(vdev, msg, &config); }
> > +
> > +static int
> > +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32
> portid, u32 seq,
> > +		     int flags, struct netlink_ext_ack *extack) {
> > +	u32 device_id;
> > +	void *hdr;
> > +	int err;
> > +
> > +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > +			  VDPA_CMD_DEV_CONFIG_GET);
> > +	if (!hdr)
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev-
> >dev))) {
> > +		err = -EMSGSIZE;
> > +		goto msg_err;
> > +	}
> > +
> > +	device_id = vdev->config->get_device_id(vdev);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > +		err = -EMSGSIZE;
> > +		goto msg_err;
> > +	}
> > +
> > +	switch (device_id) {
> > +	case VIRTIO_ID_NET:
> > +		err = vdpa_dev_net_config_fill(vdev, msg);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +		break;
> > +	}
> > +	if (err)
> > +		goto msg_err;
> > +
> > +	genlmsg_end(msg, hdr);
> > +	return 0;
> > +
> > +msg_err:
> > +	genlmsg_cancel(msg, hdr);
> > +	return err;
> > +}
> > +
> > +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb,
> > +struct genl_info *info) {
> > +	struct vdpa_device *vdev;
> > +	struct sk_buff *msg;
> > +	const char *devname;
> > +	struct device *dev;
> > +	int err;
> > +
> > +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> > +		return -EINVAL;
> > +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> > +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	if (!msg)
> > +		return -ENOMEM;
> > +
> > +	mutex_lock(&vdpa_dev_mutex);
> > +	dev = bus_find_device(&vdpa_bus, NULL, devname,
> vdpa_name_match);
> > +	if (!dev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> > +		err = -ENODEV;
> > +		goto dev_err;
> > +	}
> > +	vdev = container_of(dev, struct vdpa_device, dev);
> > +	if (!vdev->mdev) {
> > +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa
> device");
> > +		err = -EINVAL;
> > +		goto mdev_err;
> > +	}
> 
> 
> Though it's fine but it looks to me mdev is not required to work here.
> 
Yes, mdev is not required here. However it was little weird to allow $ vdpa dev config show, but not allow $ vdpa dev show.
It transition phase right now. Subsequently will able to allow this as well.
After this series only ifc driver will be left to convert to user created devices.
At that point, this checks will have chance to simplify.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Jason Wang]

On 2021/2/26 1:32 下午, Parav Pandit wrote:
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Wednesday, February 24, 2021 2:18 PM
>>
>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>> +
>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>> +			config->max_virtqueue_pairs))
>>> +		return -EMSGSIZE;
>>
>> We probably need to check VIRTIO_NET_F_RSS here.
> Yes. Will use it in v2.
>
>>
>>> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>> +		       config->rss_max_key_size))
>>> +		return -EMSGSIZE;
>>> +
>>> +	rss_field = le16_to_cpu(config->rss_max_key_size);
>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>> rss_field))
>>> +		return -EMSGSIZE;
>>> +
>>> +	hash_types = le32_to_cpu(config->supported_hash_types);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>> +			config->supported_hash_types))
>>> +		return -EMSGSIZE;
>>> +	return 0;
>>> +}
>>> +
>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct
>>> +sk_buff *msg) {
>>> +	struct virtio_net_config config = {};
>>> +
>>> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>
>> Do we need to sync with other possible get_config calls here?
> To readers of get_config() is ok. No need to sync.
> However, I think set_config() and get_config() should sync otherwise get_config can get partial value.
> Will probably have to add vdpa_device->config_access_lock().


Probably. And the set_config() should be synchronized with the requrest 
that comes from vDPA bus. This makes me think whether we should go back 
to two phases method, create and enable.

The vDPA device is only registred after enabling, then there's no need 
to sync with vDPA bus, and mgmt is not allowed to change config after 
enalbing?

>
>>
>>> +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
>> sizeof(config.mac), config.mac))
>>> +		return -EMSGSIZE;
>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status))
>>> +		return -EMSGSIZE;
>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu))
>>> +		return -EMSGSIZE;
>>
>> And check VIRTIO_NET_F_SPEED_DUPLEX.
> Yes, will do.
>
>>
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED,
>> config.speed))
>>> +		return -EMSGSIZE;
>>> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX,
>> config.duplex))
>>> +		return -EMSGSIZE;
>>> +
>>> +	return vdpa_dev_net_mq_config_fill(vdev, msg, &config); }
>>> +
>>> +static int
>>> +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32
>> portid, u32 seq,
>>> +		     int flags, struct netlink_ext_ack *extack) {
>>> +	u32 device_id;
>>> +	void *hdr;
>>> +	int err;
>>> +
>>> +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>> +			  VDPA_CMD_DEV_CONFIG_GET);
>>> +	if (!hdr)
>>> +		return -EMSGSIZE;
>>> +
>>> +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev-
>>> dev))) {
>>> +		err = -EMSGSIZE;
>>> +		goto msg_err;
>>> +	}
>>> +
>>> +	device_id = vdev->config->get_device_id(vdev);
>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
>>> +		err = -EMSGSIZE;
>>> +		goto msg_err;
>>> +	}
>>> +
>>> +	switch (device_id) {
>>> +	case VIRTIO_ID_NET:
>>> +		err = vdpa_dev_net_config_fill(vdev, msg);
>>> +		break;
>>> +	default:
>>> +		err = -EOPNOTSUPP;
>>> +		break;
>>> +	}
>>> +	if (err)
>>> +		goto msg_err;
>>> +
>>> +	genlmsg_end(msg, hdr);
>>> +	return 0;
>>> +
>>> +msg_err:
>>> +	genlmsg_cancel(msg, hdr);
>>> +	return err;
>>> +}
>>> +
>>> +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb,
>>> +struct genl_info *info) {
>>> +	struct vdpa_device *vdev;
>>> +	struct sk_buff *msg;
>>> +	const char *devname;
>>> +	struct device *dev;
>>> +	int err;
>>> +
>>> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
>>> +		return -EINVAL;
>>> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
>>> +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
>>> +	if (!msg)
>>> +		return -ENOMEM;
>>> +
>>> +	mutex_lock(&vdpa_dev_mutex);
>>> +	dev = bus_find_device(&vdpa_bus, NULL, devname,
>> vdpa_name_match);
>>> +	if (!dev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
>>> +		err = -ENODEV;
>>> +		goto dev_err;
>>> +	}
>>> +	vdev = container_of(dev, struct vdpa_device, dev);
>>> +	if (!vdev->mdev) {
>>> +		NL_SET_ERR_MSG_MOD(info->extack, "unmanaged vdpa
>> device");
>>> +		err = -EINVAL;
>>> +		goto mdev_err;
>>> +	}
>>
>> Though it's fine but it looks to me mdev is not required to work here.
>>
> Yes, mdev is not required here. However it was little weird to allow $ vdpa dev config show, but not allow $ vdpa dev show.
> It transition phase right now. Subsequently will able to allow this as well.
> After this series only ifc driver will be left to convert to user created devices.
> At that point, this checks will have chance to simplify.


I agree.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Friday, February 26, 2021 1:56 PM
> 
> 
> On 2021/2/26 1:32 下午, Parav Pandit wrote:
> >
> >> From: Jason Wang <jasowang@redhat.com>
> >> Sent: Wednesday, February 24, 2021 2:18 PM
> >>
> >> On 2021/2/24 2:18 下午, Parav Pandit wrote:
> >>> +
> >>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> >>> +			config->max_virtqueue_pairs))
> >>> +		return -EMSGSIZE;
> >>
> >> We probably need to check VIRTIO_NET_F_RSS here.
> > Yes. Will use it in v2.
> >
> >>
> >>> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> >>> +		       config->rss_max_key_size))
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	rss_field = le16_to_cpu(config->rss_max_key_size);
> >>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
> >> rss_field))
> >>> +		return -EMSGSIZE;
> >>> +
> >>> +	hash_types = le32_to_cpu(config->supported_hash_types);
> >>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> >>> +			config->supported_hash_types))
> >>> +		return -EMSGSIZE;
> >>> +	return 0;
> >>> +}
> >>> +
> >>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
> >>> +struct sk_buff *msg) {
> >>> +	struct virtio_net_config config = {};
> >>> +
> >>> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>
> >> Do we need to sync with other possible get_config calls here?
> > To readers of get_config() is ok. No need to sync.
> > However, I think set_config() and get_config() should sync otherwise
> get_config can get partial value.
> > Will probably have to add vdpa_device->config_access_lock().
> 
> 
> Probably. And the set_config() should be synchronized with the requrest that
> comes from vDPA bus. 
Yes, a rw semaphore is good enough.
Device config fields can be change from the device side anyway, such as link status anyway.
Sync using lock shouldn’t be problem. 

> This makes me think whether we should go back to
> two phases method, create and enable.
> 
> The vDPA device is only registred after enabling, then there's no need to sync
> with vDPA bus, and mgmt is not allowed to change config after enalbing?
> 
In that case we should be able to disable it as well. Disable should take the device of the bus.
I find it weird to have this model to linger around the device without anchoring on the bus.
For example device is not yet enabled, so it is not anchored on the bus, but its name is still have to remain unique.

Do we have to anchor vdpa device on the bus? Can vdpa be a class?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Jason Wang]

On 2021/2/26 4:50 下午, Parav Pandit wrote:
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Friday, February 26, 2021 1:56 PM
>>
>>
>> On 2021/2/26 1:32 下午, Parav Pandit wrote:
>>>> From: Jason Wang <jasowang@redhat.com>
>>>> Sent: Wednesday, February 24, 2021 2:18 PM
>>>>
>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>> +
>>>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>>> +			config->max_virtqueue_pairs))
>>>>> +		return -EMSGSIZE;
>>>> We probably need to check VIRTIO_NET_F_RSS here.
>>> Yes. Will use it in v2.
>>>
>>>>> +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>>>> +		       config->rss_max_key_size))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	rss_field = le16_to_cpu(config->rss_max_key_size);
>>>>> +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>>>> rss_field))
>>>>> +		return -EMSGSIZE;
>>>>> +
>>>>> +	hash_types = le32_to_cpu(config->supported_hash_types);
>>>>> +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>>>> +			config->supported_hash_types))
>>>>> +		return -EMSGSIZE;
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>>>> +struct sk_buff *msg) {
>>>>> +	struct virtio_net_config config = {};
>>>>> +
>>>>> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>> Do we need to sync with other possible get_config calls here?
>>> To readers of get_config() is ok. No need to sync.
>>> However, I think set_config() and get_config() should sync otherwise
>> get_config can get partial value.
>>> Will probably have to add vdpa_device->config_access_lock().
>>
>> Probably. And the set_config() should be synchronized with the requrest that
>> comes from vDPA bus.
> Yes, a rw semaphore is good enough.
> Device config fields can be change from the device side anyway, such as link status anyway.
> Sync using lock shouldn’t be problem.
>
>> This makes me think whether we should go back to
>> two phases method, create and enable.
>>
>> The vDPA device is only registred after enabling, then there's no need to sync
>> with vDPA bus, and mgmt is not allowed to change config after enalbing?
>>
> In that case we should be able to disable it as well. Disable should take the device of the bus.
> I find it weird to have this model to linger around the device without anchoring on the bus.
> For example device is not yet enabled, so it is not anchored on the bus, but its name is still have to remain unique.


Can we do some synchornization between vdpa device allocation and vdpa 
device registier to solve the naming issue?


>
> Do we have to anchor vdpa device on the bus? Can vdpa be a class?


I think not, it's a bus that is expected to be bound to different driver 
instead of a subsystem.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Parav Pandit]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Monday, March 1, 2021 9:29 AM
> 
> On 2021/2/26 4:50 下午, Parav Pandit wrote:
> >
> >> From: Jason Wang <jasowang@redhat.com>
> >> Sent: Friday, February 26, 2021 1:56 PM
> >>
> >>
> >> On 2021/2/26 1:32 下午, Parav Pandit wrote:
> >>>> From: Jason Wang <jasowang@redhat.com>
> >>>> Sent: Wednesday, February 24, 2021 2:18 PM
> >>>>
> >>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
> >>>>> +
> >>>>> +	if (nla_put_u16(msg,
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> >>>>> +			config->max_virtqueue_pairs))
> >>>>> +		return -EMSGSIZE;
> >>>> We probably need to check VIRTIO_NET_F_RSS here.
> >>> Yes. Will use it in v2.
> >>>
> >>>>> +	if (nla_put_u8(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> >>>>> +		       config->rss_max_key_size))
> >>>>> +		return -EMSGSIZE;
> >>>>> +
> >>>>> +	rss_field = le16_to_cpu(config->rss_max_key_size);
> >>>>> +	if (nla_put_u16(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
> >>>> rss_field))
> >>>>> +		return -EMSGSIZE;
> >>>>> +
> >>>>> +	hash_types = le32_to_cpu(config->supported_hash_types);
> >>>>> +	if (nla_put_u32(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> >>>>> +			config->supported_hash_types))
> >>>>> +		return -EMSGSIZE;
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
> >>>>> +struct sk_buff *msg) {
> >>>>> +	struct virtio_net_config config = {};
> >>>>> +
> >>>>> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>>> Do we need to sync with other possible get_config calls here?
> >>> To readers of get_config() is ok. No need to sync.
> >>> However, I think set_config() and get_config() should sync otherwise
> >> get_config can get partial value.
> >>> Will probably have to add vdpa_device->config_access_lock().
> >>
> >> Probably. And the set_config() should be synchronized with the
> >> requrest that comes from vDPA bus.
> > Yes, a rw semaphore is good enough.
> > Device config fields can be change from the device side anyway, such as
> link status anyway.
> > Sync using lock shouldn’t be problem.
> >
> >> This makes me think whether we should go back to two phases method,
> >> create and enable.
> >>
> >> The vDPA device is only registred after enabling, then there's no
> >> need to sync with vDPA bus, and mgmt is not allowed to change config
> after enalbing?
> >>
> > In that case we should be able to disable it as well. Disable should take the
> device of the bus.
> > I find it weird to have this model to linger around the device without
> anchoring on the bus.
> > For example device is not yet enabled, so it is not anchored on the bus, but
> its name is still have to remain unique.
> 
> 
> Can we do some synchornization between vdpa device allocation and vdpa
> device registier to solve the naming issue?
mgmt tool querying the device config space after vdpa device is in use is real.
So I don't see solving it any differently.

Now upper layers of vdpa to not access the device on the placed on the vdpa bus, can be taken care by existing driver code using
echo 0 > /sys/bus/vdpa/drivers_autoprobe

By default vdpa device placed on the bus wont bind to any driver (net/vhost etc).
1. Decision to bind to which driver is done after config of the vdpa device is done.
2. orchestration sw does create and config before it binds to the right driver
3. which driver to bind to decided based on the use case of that individual vdpa device
For example, 
(a) vdpa0 binds to net driver to have Netdev for container
(b) vdpa1 binds to vhost driver to map vdpa device to VM
 
> >
> > Do we have to anchor vdpa device on the bus? Can vdpa be a class?
> 
> 
> I think not, it's a bus that is expected to be bound to different driver
> instead of a subsystem.
Yeah, I realized it lately after writing the previous email.
Lets see if above scheme works or not to use existing drivers_autoprobe file.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Jason Wang]

On 2021/3/1 3:29 下午, Parav Pandit wrote:
>
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Monday, March 1, 2021 9:29 AM
>>
>> On 2021/2/26 4:50 下午, Parav Pandit wrote:
>>>> From: Jason Wang <jasowang@redhat.com>
>>>> Sent: Friday, February 26, 2021 1:56 PM
>>>>
>>>>
>>>> On 2021/2/26 1:32 下午, Parav Pandit wrote:
>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>> Sent: Wednesday, February 24, 2021 2:18 PM
>>>>>>
>>>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>>>> +
>>>>>>> +	if (nla_put_u16(msg,
>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>>>>> +			config->max_virtqueue_pairs))
>>>>>>> +		return -EMSGSIZE;
>>>>>> We probably need to check VIRTIO_NET_F_RSS here.
>>>>> Yes. Will use it in v2.
>>>>>
>>>>>>> +	if (nla_put_u8(msg,
>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>>>>>> +		       config->rss_max_key_size))
>>>>>>> +		return -EMSGSIZE;
>>>>>>> +
>>>>>>> +	rss_field = le16_to_cpu(config->rss_max_key_size);
>>>>>>> +	if (nla_put_u16(msg,
>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>>>>>> rss_field))
>>>>>>> +		return -EMSGSIZE;
>>>>>>> +
>>>>>>> +	hash_types = le32_to_cpu(config->supported_hash_types);
>>>>>>> +	if (nla_put_u32(msg,
>> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>>>>>> +			config->supported_hash_types))
>>>>>>> +		return -EMSGSIZE;
>>>>>>> +	return 0;
>>>>>>> +}
>>>>>>> +
>>>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>>>>>> +struct sk_buff *msg) {
>>>>>>> +	struct virtio_net_config config = {};
>>>>>>> +
>>>>>>> +	vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>>> Do we need to sync with other possible get_config calls here?
>>>>> To readers of get_config() is ok. No need to sync.
>>>>> However, I think set_config() and get_config() should sync otherwise
>>>> get_config can get partial value.
>>>>> Will probably have to add vdpa_device->config_access_lock().
>>>> Probably. And the set_config() should be synchronized with the
>>>> requrest that comes from vDPA bus.
>>> Yes, a rw semaphore is good enough.
>>> Device config fields can be change from the device side anyway, such as
>> link status anyway.
>>> Sync using lock shouldn’t be problem.
>>>
>>>> This makes me think whether we should go back to two phases method,
>>>> create and enable.
>>>>
>>>> The vDPA device is only registred after enabling, then there's no
>>>> need to sync with vDPA bus, and mgmt is not allowed to change config
>> after enalbing?
>>> In that case we should be able to disable it as well. Disable should take the
>> device of the bus.
>>> I find it weird to have this model to linger around the device without
>> anchoring on the bus.
>>> For example device is not yet enabled, so it is not anchored on the bus, but
>> its name is still have to remain unique.
>>
>>
>> Can we do some synchornization between vdpa device allocation and vdpa
>> device registier to solve the naming issue?
> mgmt tool querying the device config space after vdpa device is in use is real.
> So I don't see solving it any differently.
>
> Now upper layers of vdpa to not access the device on the placed on the vdpa bus, can be taken care by existing driver code using
> echo 0 > /sys/bus/vdpa/drivers_autoprobe
>
> By default vdpa device placed on the bus wont bind to any driver (net/vhost etc).
> 1. Decision to bind to which driver is done after config of the vdpa device is done.
> 2. orchestration sw does create and config before it binds to the right driver
> 3. which driver to bind to decided based on the use case of that individual vdpa device
> For example,
> (a) vdpa0 binds to net driver to have Netdev for container
> (b) vdpa1 binds to vhost driver to map vdpa device to VM


I think it should work.

Adding Adrian, Maxime and Sean for more comments.


>   
>>> Do we have to anchor vdpa device on the bus? Can vdpa be a class?
>>
>> I think not, it's a bus that is expected to be bound to different driver
>> instead of a subsystem.
> Yeah, I realized it lately after writing the previous email.
> Lets see if above scheme works or not to use existing drivers_autoprobe file.


Yes.

Thanks


_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Adrian Moreno]

On 3/1/21 8:50 AM, Jason Wang wrote:
> 
> On 2021/3/1 3:29 下午, Parav Pandit wrote:
>>
>>> From: Jason Wang <jasowang@redhat.com>
>>> Sent: Monday, March 1, 2021 9:29 AM
>>>
>>> On 2021/2/26 4:50 下午, Parav Pandit wrote:
>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>> Sent: Friday, February 26, 2021 1:56 PM
>>>>>
>>>>>
>>>>> On 2021/2/26 1:32 下午, Parav Pandit wrote:
>>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>>> Sent: Wednesday, February 24, 2021 2:18 PM
>>>>>>>
>>>>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>>>>> +
>>>>>>>> +    if (nla_put_u16(msg,
>>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>>>>>> +            config->max_virtqueue_pairs))
>>>>>>>> +        return -EMSGSIZE;
>>>>>>> We probably need to check VIRTIO_NET_F_RSS here.
>>>>>> Yes. Will use it in v2.
>>>>>>
>>>>>>>> +    if (nla_put_u8(msg,
>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>>>>>>> +               config->rss_max_key_size))
>>>>>>>> +        return -EMSGSIZE;
>>>>>>>> +
>>>>>>>> +    rss_field = le16_to_cpu(config->rss_max_key_size);
>>>>>>>> +    if (nla_put_u16(msg,
>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>>>>>>> rss_field))
>>>>>>>> +        return -EMSGSIZE;
>>>>>>>> +
>>>>>>>> +    hash_types = le32_to_cpu(config->supported_hash_types);
>>>>>>>> +    if (nla_put_u32(msg,
>>> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>>>>>>> +            config->supported_hash_types))
>>>>>>>> +        return -EMSGSIZE;
>>>>>>>> +    return 0;
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>>>>>>> +struct sk_buff *msg) {
>>>>>>>> +    struct virtio_net_config config = {};
>>>>>>>> +
>>>>>>>> +    vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>>>> Do we need to sync with other possible get_config calls here?
>>>>>> To readers of get_config() is ok. No need to sync.
>>>>>> However, I think set_config() and get_config() should sync otherwise
>>>>> get_config can get partial value.
>>>>>> Will probably have to add vdpa_device->config_access_lock().
>>>>> Probably. And the set_config() should be synchronized with the
>>>>> requrest that comes from vDPA bus.
>>>> Yes, a rw semaphore is good enough.
>>>> Device config fields can be change from the device side anyway, such as
>>> link status anyway.
>>>> Sync using lock shouldn’t be problem.
>>>>
>>>>> This makes me think whether we should go back to two phases method,
>>>>> create and enable.
>>>>>
>>>>> The vDPA device is only registred after enabling, then there's no
>>>>> need to sync with vDPA bus, and mgmt is not allowed to change config
>>> after enalbing?
>>>> In that case we should be able to disable it as well. Disable should take the
>>> device of the bus.
>>>> I find it weird to have this model to linger around the device without
>>> anchoring on the bus.
>>>> For example device is not yet enabled, so it is not anchored on the bus, but
>>> its name is still have to remain unique.
>>>
>>>
>>> Can we do some synchornization between vdpa device allocation and vdpa
>>> device registier to solve the naming issue?
>> mgmt tool querying the device config space after vdpa device is in use is real.
>> So I don't see solving it any differently.
>>
>> Now upper layers of vdpa to not access the device on the placed on the vdpa
>> bus, can be taken care by existing driver code using
>> echo 0 > /sys/bus/vdpa/drivers_autoprobe
>>
>> By default vdpa device placed on the bus wont bind to any driver (net/vhost etc).
>> 1. Decision to bind to which driver is done after config of the vdpa device is
>> done.
>> 2. orchestration sw does create and config before it binds to the right driver
>> 3. which driver to bind to decided based on the use case of that individual
>> vdpa device
>> For example,
>> (a) vdpa0 binds to net driver to have Netdev for container
>> (b) vdpa1 binds to vhost driver to map vdpa device to VM
> 
> 
> I think it should work.
> > Adding Adrian, Maxime and Sean for more comments.

Is this a strong requirement or just a optional operation?
This might break some existing logic that expects netdev or vhost-vdpa device to
exist before the some config (e.g:mac address) is set.


Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Jason Wang]

On 2021/3/1 8:06 下午, Sean Mooney wrote:
> On Mon, 2021-03-01 at 11:28 +0100, Adrian Moreno wrote:
>> On 3/1/21 8:50 AM, Jason Wang wrote:
>>> On 2021/3/1 3:29 下午, Parav Pandit wrote:
>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>> Sent: Monday, March 1, 2021 9:29 AM
>>>>>
>>>>> On 2021/2/26 4:50 下午, Parav Pandit wrote:
>>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>>> Sent: Friday, February 26, 2021 1:56 PM
>>>>>>>
>>>>>>>
>>>>>>> On 2021/2/26 1:32 下午, Parav Pandit wrote:
>>>>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>>>>> Sent: Wednesday, February 24, 2021 2:18 PM
>>>>>>>>>
>>>>>>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>>>>>>> +
>>>>>>>>>> +    if (nla_put_u16(msg,
>>>>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>>>>>>>> +            config->max_virtqueue_pairs))
>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>> We probably need to check VIRTIO_NET_F_RSS here.
>>>>>>>> Yes. Will use it in v2.
>>>>>>>>
>>>>>>>>>> +    if (nla_put_u8(msg,
>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>>>>>>>>> +               config->rss_max_key_size))
>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>> +
>>>>>>>>>> +    rss_field = le16_to_cpu(config->rss_max_key_size);
>>>>>>>>>> +    if (nla_put_u16(msg,
>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>>>>>>>>> rss_field))
>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>> +
>>>>>>>>>> +    hash_types = le32_to_cpu(config->supported_hash_types);
>>>>>>>>>> +    if (nla_put_u32(msg,
>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>>>>>>>>> +            config->supported_hash_types))
>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>> +    return 0;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>>>>>>>>> +struct sk_buff *msg) {
>>>>>>>>>> +    struct virtio_net_config config = {};
>>>>>>>>>> +
>>>>>>>>>> +    vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>>>>>> Do we need to sync with other possible get_config calls here?
>>>>>>>> To readers of get_config() is ok. No need to sync.
>>>>>>>> However, I think set_config() and get_config() should sync otherwise
>>>>>>> get_config can get partial value.
>>>>>>>> Will probably have to add vdpa_device->config_access_lock().
>>>>>>> Probably. And the set_config() should be synchronized with the
>>>>>>> requrest that comes from vDPA bus.
>>>>>> Yes, a rw semaphore is good enough.
>>>>>> Device config fields can be change from the device side anyway, such as
>>>>> link status anyway.
>>>>>> Sync using lock shouldn’t be problem.
>>>>>>
>>>>>>> This makes me think whether we should go back to two phases method,
>>>>>>> create and enable.
>>>>>>>
>>>>>>> The vDPA device is only registred after enabling, then there's no
>>>>>>> need to sync with vDPA bus, and mgmt is not allowed to change config
>>>>> after enalbing?
>>>>>> In that case we should be able to disable it as well. Disable should take the
>>>>> device of the bus.
>>>>>> I find it weird to have this model to linger around the device without
>>>>> anchoring on the bus.
>>>>>> For example device is not yet enabled, so it is not anchored on the bus, but
>>>>> its name is still have to remain unique.
>>>>>
>>>>>
>>>>> Can we do some synchornization between vdpa device allocation and vdpa
>>>>> device registier to solve the naming issue?
>>>> mgmt tool querying the device config space after vdpa device is in use is real.
>>>> So I don't see solving it any differently.
>>>>
>>>> Now upper layers of vdpa to not access the device on the placed on the vdpa
>>>> bus, can be taken care by existing driver code using
>>>> echo 0 > /sys/bus/vdpa/drivers_autoprobe
>>>>
>>>> By default vdpa device placed on the bus wont bind to any driver (net/vhost etc).
>>>> 1. Decision to bind to which driver is done after config of the vdpa device is
>>>> done.
>>>> 2. orchestration sw does create and config before it binds to the right driver
>>>> 3. which driver to bind to decided based on the use case of that individual
>>>> vdpa device
>>>> For example,
>>>> (a) vdpa0 binds to net driver to have Netdev for container
>>>> (b) vdpa1 binds to vhost driver to map vdpa device to VM
>>>
>>> I think it should work.
>>>> Adding Adrian, Maxime and Sean for more comments.
>> Is this a strong requirement or just a optional operation?


So my understanding is that something needs to be done to make sure:

1) the vpda device is not bound to virtio-vdpa
2) config is not changed if it has been set by management


>> This might break some existing logic that expects netdev or vhost-vdpa device to
>> exist before the some config (e.g:mac address) is set.
> the orcestration software should not require permission to do dirver binding and in the case of openstack will
> not have the capablity to bind the vdpa device to a dirvier in the current proposed design.
> the ordering we are expecting to support in openstack at least initally is that the adminsitrator
> will use either systemd service files or udev rules to allocate the VF and vdpa device staictly at boot.
>
> in the case of openstack we will expect all vdpa devies that will be used by openstack(nova) for assignment
> to vms will be prebound by the adminstrators to vhost-vdpa. openstack will simply track the /dev/vhost* paths
> corallated to the vdpa devices whose parent VF are allowed by the pci passthough whitelist.
>
> if the adminstrator binds the vdpa device to virtio-vdpa so that virtio-net-pci is used on the host nova and libvirt
> will not correct that and the vm boot will fail. but provided you do not list that parent VF in the whitelist nova
> will also ignore it. so if you want to mix and match the vdpa device driver that should not break anything provided
> you do not list its parent in the pci whitelist.
>
> but one thing to keep in mind is whlie we are not planning to create VF/VDPA devices dynamiclaly at lesat not for the initall
> implentation we do expact to be able to configure attibute of the vdpa device after tehy are create like any other nic.
> for example the mac, mtu, link-state should all be configureable at runtime via normal ip route2 commands.


So in the case of vDPA, this task is expected to be done by vdpa tool. 
(There's no guarantee that vDPA is a VF).


>
> at the morment we are encounterng issue with the mac adress specificly which qemu is hopefully going to work around since
> libvirt and nova cannot currently configure the mac address of a vhost-vdpa device so we are relying on qemu
> to take the mac adress form the qemu command line so that it does not end up as all 0 macs in the guest.


Note that a kernel patch is posted by Eli[1] which assign a randomized 
mac. This should solve the issue in another way.


> as some one that works on orcestration level i would have expect that setting the mac/mtu on either the vf or the netdev
> representor to match the deireed mac/mtu in the guest would have been sufficent to alter the vdpa device confirutation.
> likely  the netdev representor would make the most sence to alter to match the desired mac so that the mac adress see on the ovs
> bridge matches the mac of the vm.


Speaking for genreal vDPA, it's not guaranteed that the vendor will 
implmenet the device with a representor. So vDPA tool is expected to be 
the only way to configure the device.

Thanks

[1] https://www.spinics.net/lists/linux-virtualization/msg48451.html


> the current case where the vdpa representor netdev seams to have no bareing on the vdpa device
> config makes debugging much more difficalt and is likely to confuse monitoring software. fortunetly openstack is not matching on the mac
> address in this spefic case to identify the netdev represntor port but we have other backend that do look at the mac. in the case of ovs
> we store the uuid of the neutron port in the ovsdb when adding a port to ovs so we rely on that not the mac but  for legacy mode
> sriov which we hope to support with vdpa eventulally after we comple the hardware offloaed ovs support, currenlty relys on the mac in some cases.
>
> i hope that answered your questions?
> we are aware that we likely will have to support vdpa device creation at some point if we intend to add support for vdpa devices
> created form subfunctions or intels siov howver that was not ready when we started this work so it was declared out of scope and
> the curernt static approch was taken as the only path we could enable before code free in 10 days.
>
> regards sean.
>
>>
>> Thanks
>>
>
> https://www.spinics.net/lists/linux-virtualization/msg48451.html

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 2/9] vdpa: Introduce query of device config layout

[Adrian Moreno]

On 3/2/21 5:25 AM, Jason Wang wrote:
> 
> On 2021/3/1 8:06 下午, Sean Mooney wrote:
>> On Mon, 2021-03-01 at 11:28 +0100, Adrian Moreno wrote:
>>> On 3/1/21 8:50 AM, Jason Wang wrote:
>>>> On 2021/3/1 3:29 下午, Parav Pandit wrote:
>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>> Sent: Monday, March 1, 2021 9:29 AM
>>>>>>
>>>>>> On 2021/2/26 4:50 下午, Parav Pandit wrote:
>>>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>>>> Sent: Friday, February 26, 2021 1:56 PM
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2021/2/26 1:32 下午, Parav Pandit wrote:
>>>>>>>>>> From: Jason Wang <jasowang@redhat.com>
>>>>>>>>>> Sent: Wednesday, February 24, 2021 2:18 PM
>>>>>>>>>>
>>>>>>>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>>>>>>>> +
>>>>>>>>>>> +    if (nla_put_u16(msg,
>>>>>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>>>>>>>>>>> +            config->max_virtqueue_pairs))
>>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>> We probably need to check VIRTIO_NET_F_RSS here.
>>>>>>>>> Yes. Will use it in v2.
>>>>>>>>>
>>>>>>>>>>> +    if (nla_put_u8(msg,
>>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
>>>>>>>>>>> +               config->rss_max_key_size))
>>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>>> +
>>>>>>>>>>> +    rss_field = le16_to_cpu(config->rss_max_key_size);
>>>>>>>>>>> +    if (nla_put_u16(msg,
>>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,
>>>>>>>>>> rss_field))
>>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>>> +
>>>>>>>>>>> +    hash_types = le32_to_cpu(config->supported_hash_types);
>>>>>>>>>>> +    if (nla_put_u32(msg,
>>>>>> VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
>>>>>>>>>>> +            config->supported_hash_types))
>>>>>>>>>>> +        return -EMSGSIZE;
>>>>>>>>>>> +    return 0;
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>>>>>>>>>> +struct sk_buff *msg) {
>>>>>>>>>>> +    struct virtio_net_config config = {};
>>>>>>>>>>> +
>>>>>>>>>>> +    vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>>>>>>> Do we need to sync with other possible get_config calls here?
>>>>>>>>> To readers of get_config() is ok. No need to sync.
>>>>>>>>> However, I think set_config() and get_config() should sync otherwise
>>>>>>>> get_config can get partial value.
>>>>>>>>> Will probably have to add vdpa_device->config_access_lock().
>>>>>>>> Probably. And the set_config() should be synchronized with the
>>>>>>>> requrest that comes from vDPA bus.
>>>>>>> Yes, a rw semaphore is good enough.
>>>>>>> Device config fields can be change from the device side anyway, such as
>>>>>> link status anyway.
>>>>>>> Sync using lock shouldn’t be problem.
>>>>>>>
>>>>>>>> This makes me think whether we should go back to two phases method,
>>>>>>>> create and enable.
>>>>>>>>
>>>>>>>> The vDPA device is only registred after enabling, then there's no
>>>>>>>> need to sync with vDPA bus, and mgmt is not allowed to change config
>>>>>> after enalbing?
>>>>>>> In that case we should be able to disable it as well. Disable should take
>>>>>>> the
>>>>>> device of the bus.
>>>>>>> I find it weird to have this model to linger around the device without
>>>>>> anchoring on the bus.
>>>>>>> For example device is not yet enabled, so it is not anchored on the bus, but
>>>>>> its name is still have to remain unique.
>>>>>>
>>>>>>
>>>>>> Can we do some synchornization between vdpa device allocation and vdpa
>>>>>> device registier to solve the naming issue?
>>>>> mgmt tool querying the device config space after vdpa device is in use is
>>>>> real.
>>>>> So I don't see solving it any differently.
>>>>>
>>>>> Now upper layers of vdpa to not access the device on the placed on the vdpa
>>>>> bus, can be taken care by existing driver code using
>>>>> echo 0 > /sys/bus/vdpa/drivers_autoprobe
>>>>>
>>>>> By default vdpa device placed on the bus wont bind to any driver (net/vhost
>>>>> etc).
>>>>> 1. Decision to bind to which driver is done after config of the vdpa device is
>>>>> done.
>>>>> 2. orchestration sw does create and config before it binds to the right driver
>>>>> 3. which driver to bind to decided based on the use case of that individual
>>>>> vdpa device
>>>>> For example,
>>>>> (a) vdpa0 binds to net driver to have Netdev for container
>>>>> (b) vdpa1 binds to vhost driver to map vdpa device to VM
>>>>
>>>> I think it should work.
>>>>> Adding Adrian, Maxime and Sean for more comments.
>>> Is this a strong requirement or just a optional operation?
> 
> 
> So my understanding is that something needs to be done to make sure:
> 

> 1) the vpda device is not bound to virtio-vdpaThis is a problem of api not being unified, right? If vdpa device is bound to
virtio-vdpa I would expect some net-specific options (like mac or tso) would be
modifiable through net_device_ops if the needed capabilities are held.

> 2) config is not changed if it has been set by management
>For virtio-vdpa (virtio-net) devices, that would be controlled via CAP_NET_ADMIN.
For vhost-vdpa case, are you thinking of a way of stopping a user from sending
VHOST_VDPA_SET_CONFIG messages? or making it fail if management has made some
config at the vdpa level?

> 
>>> This might break some existing logic that expects netdev or vhost-vdpa device to
>>> exist before the some config (e.g:mac address) is set.
>> the orcestration software should not require permission to do dirver binding
>> and in the case of openstack will
>> not have the capablity to bind the vdpa device to a dirvier in the current
>> proposed design.
>> the ordering we are expecting to support in openstack at least initally is
>> that the adminsitrator
>> will use either systemd service files or udev rules to allocate the VF and
>> vdpa device staictly at boot.
>>
>> in the case of openstack we will expect all vdpa devies that will be used by
>> openstack(nova) for assignment
>> to vms will be prebound by the adminstrators to vhost-vdpa. openstack will
>> simply track the /dev/vhost* paths
>> corallated to the vdpa devices whose parent VF are allowed by the pci
>> passthough whitelist.
>>
>> if the adminstrator binds the vdpa device to virtio-vdpa so that
>> virtio-net-pci is used on the host nova and libvirt
>> will not correct that and the vm boot will fail. but provided you do not list
>> that parent VF in the whitelist nova
>> will also ignore it. so if you want to mix and match the vdpa device driver
>> that should not break anything provided
>> you do not list its parent in the pci whitelist.
>>
>> but one thing to keep in mind is whlie we are not planning to create VF/VDPA
>> devices dynamiclaly at lesat not for the initall
>> implentation we do expact to be able to configure attibute of the vdpa device
>> after tehy are create like any other nic.
>> for example the mac, mtu, link-state should all be configureable at runtime
>> via normal ip route2 commands.
> 
> 
> So in the case of vDPA, this task is expected to be done by vdpa tool. (There's
> no guarantee that vDPA is a VF).
> 
> 
>>
>> at the morment we are encounterng issue with the mac adress specificly which
>> qemu is hopefully going to work around since
>> libvirt and nova cannot currently configure the mac address of a vhost-vdpa
>> device so we are relying on qemu
>> to take the mac adress form the qemu command line so that it does not end up
>> as all 0 macs in the guest.
> 
> 
> Note that a kernel patch is posted by Eli[1] which assign a randomized mac. This
> should solve the issue in another way.
> 
> 
>> as some one that works on orcestration level i would have expect that setting
>> the mac/mtu on either the vf or the netdev
>> representor to match the deireed mac/mtu in the guest would have been
>> sufficent to alter the vdpa device confirutation.
>> likely  the netdev representor would make the most sence to alter to match the
>> desired mac so that the mac adress see on the ovs
>> bridge matches the mac of the vm.
> 
> 
> Speaking for genreal vDPA, it's not guaranteed that the vendor will implmenet
> the device with a representor. So vDPA tool is expected to be the only way to
> configure the device.
> 
> Thanks
> 
> [1] https://www.spinics.net/lists/linux-virtualization/msg48451.html
> 
> 
>> the current case where the vdpa representor netdev seams to have no bareing on
>> the vdpa device
>> config makes debugging much more difficalt and is likely to confuse monitoring
>> software. fortunetly openstack is not matching on the mac
>> address in this spefic case to identify the netdev represntor port but we have
>> other backend that do look at the mac. in the case of ovs
>> we store the uuid of the neutron port in the ovsdb when adding a port to ovs
>> so we rely on that not the mac but  for legacy mode
>> sriov which we hope to support with vdpa eventulally after we comple the
>> hardware offloaed ovs support, currenlty relys on the mac in some cases.
>>
>> i hope that answered your questions?
>> we are aware that we likely will have to support vdpa device creation at some
>> point if we intend to add support for vdpa devices
>> created form subfunctions or intels siov howver that was not ready when we
>> started this work so it was declared out of scope and
>> the curernt static approch was taken as the only path we could enable before
>> code free in 10 days.
>>
>> regards sean.
>>
>>>
>>> Thanks
>>>
>>
>> https://www.spinics.net/lists/linux-virtualization/msg48451.html
> 

-- 
Adrián Moreno

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 3/9] vdpa: Enable user to set mac and mtu of vdpa device

[Parav Pandit]

$ vdpa dev add name bar mgmtdev vdpasim_net

$ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000

$ vdpa dev config show
bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0

$ vdpa dev config show -jp
{
    "config": {
        "bar": {
            "mac": "00:11:22:33:44:55",
            "link ": "up",
            "link_announce ": false,
            "mtu": 9000,
            "speed": 0,
            "duplex": 0
        }
    }
}

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa.c       | 89 +++++++++++++++++++++++++++++++++++++++
 include/linux/vdpa.h      | 16 +++++++
 include/uapi/linux/vdpa.h |  1 +
 3 files changed, 106 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index cebbba500638..ae4656dcea16 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -778,10 +778,93 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
 	return msg->len;
 }
 
+static void build_dev_net_config_attrs(struct genl_info *info,
+				       struct vdpa_dev_config_set_attr *attrs)
+{
+	struct nlattr **nl_attrs = info->attrs;
+	const u8 *macaddr;
+
+	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
+		macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
+		memcpy(attrs->cfg.mac, macaddr, sizeof(attrs->cfg.mac));
+		attrs->mask.mac_valid = true;
+	}
+	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) {
+		attrs->cfg.mtu =
+			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
+		attrs->mask.mtu_valid = true;
+	}
+}
+
+static int
+vdpa_dev_net_config_set(struct vdpa_device *vdev,
+			const struct vdpa_dev_config_set_attr *attrs)
+{
+	struct vdpa_mgmt_dev *mdev = vdev->mdev;
+
+	if (!mdev->ops->dev_config_set)
+		return -EOPNOTSUPP;
+	return mdev->ops->dev_config_set(mdev, vdev, attrs);
+}
+
+static int vdpa_dev_config_set(struct vdpa_device *vdev, struct genl_info *info)
+{
+	struct vdpa_dev_config_set_attr attrs = {};
+	int err = -EOPNOTSUPP;
+	u32 device_id;
+
+	if (!vdev->mdev)
+		return -EOPNOTSUPP;
+
+	device_id = vdev->config->get_device_id(vdev);
+	switch (device_id) {
+	case VIRTIO_ID_NET:
+		build_dev_net_config_attrs(info, &attrs);
+		err = vdpa_dev_net_config_set(vdev, &attrs);
+		break;
+	default:
+		break;
+	}
+	return err;
+}
+
+static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb, struct genl_info *info)
+{
+	struct vdpa_device *vdev;
+	const char *devname;
+	struct device *dev;
+	int err;
+
+	if (!info->attrs[VDPA_ATTR_DEV_NAME])
+		return -EINVAL;
+	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
+
+	mutex_lock(&vdpa_dev_mutex);
+	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
+	if (!dev) {
+		mutex_unlock(&vdpa_dev_mutex);
+		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
+		return -ENODEV;
+	}
+	vdev = container_of(dev, struct vdpa_device, dev);
+	if (!vdev->mdev) {
+		mutex_unlock(&vdpa_dev_mutex);
+		put_device(dev);
+		return -EINVAL;
+	}
+	err = vdpa_dev_config_set(vdev, info);
+	put_device(dev);
+	mutex_unlock(&vdpa_dev_mutex);
+	return err;
+}
+
 static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
 	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
 	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
 	[VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING },
+	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_EXACT_LEN(ETH_ALEN),
+	/* virtio spec 1.1 section 5.1.4.1 for valid MTU range */
+	[VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_RANGE(NLA_U16, 68, 65535),
 };
 
 static const struct genl_ops vdpa_nl_ops[] = {
@@ -815,6 +898,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
 		.doit = vdpa_nl_cmd_dev_config_get_doit,
 		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
 	},
+	{
+		.cmd = VDPA_CMD_DEV_CONFIG_SET,
+		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
+		.doit = vdpa_nl_cmd_dev_config_set_doit,
+		.flags = GENL_ADMIN_PERM,
+	},
 };
 
 static struct genl_family vdpa_nl_family __ro_after_init = {
diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 4ab5494503a8..c43a9e86c0ee 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -6,6 +6,7 @@
 #include <linux/device.h>
 #include <linux/interrupt.h>
 #include <linux/vhost_iotlb.h>
+#include <linux/virtio_net.h>
 
 /**
  * vDPA callback definition.
@@ -343,6 +344,14 @@ static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
 	ops->get_config(vdev, offset, buf, len);
 }
 
+struct vdpa_dev_config_set_attr {
+	struct virtio_net_config cfg;
+	struct {
+		u8 mac_valid : 1;
+		u8 mtu_valid : 1;
+	} mask;
+};
+
 /**
  * vdpa_mgmtdev_ops - vdpa device ops
  * @dev_add:	Add a vdpa device using alloc and register
@@ -356,10 +365,17 @@ static inline void vdpa_get_config(struct vdpa_device *vdev, unsigned offset,
  *		@dev: vdpa device to remove
  *		Driver need to remove the specified device by calling
  *		_vdpa_unregister_device().
+ * @dev_config_set: Setup one or more fields of the device configuration layout
+ *		    @mdev: management device of the vdpa device
+ *		    @dev: vdpa device whose config fields to setup/modify
+ *		    @attrs: attributes to updated
  */
 struct vdpa_mgmtdev_ops {
 	int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name);
 	void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev);
+	int (*dev_config_set)(struct vdpa_mgmt_dev *mdev,
+			      struct vdpa_device *dev,
+			      const struct vdpa_dev_config_set_attr *attrs);
 };
 
 struct vdpa_mgmt_dev {
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 5c31ecc3b956..ec349789b8d1 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -18,6 +18,7 @@ enum vdpa_command {
 	VDPA_CMD_DEV_DEL,
 	VDPA_CMD_DEV_GET,		/* can dump */
 	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
+	VDPA_CMD_DEV_CONFIG_SET,
 };
 
 enum vdpa_attr {
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 4/9] vdpa_sim_net: Enable user to set mac address and mtu

[Parav Pandit]

Enable user to set the mac address and mtu so that each vdpa device
can have its own user specified mac address and mtu.
This is done by implementing the management device's configuration
layout fields setting callback routine.

Now that user is enabled to set the mac address, remove the module
parameter for same.

And example of setting mac addr and mtu:
$ vdpa mgmtdev show

$ vdpa dev add name bar mgmtdev vdpasim_net
$ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000

View the config after setting:
$ vdpa dev config show
bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 39 ++++++++++++++++------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 240a5f1306b5..6e941b0e7935 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -29,12 +29,6 @@
 
 #define VDPASIM_NET_VQ_NUM	2
 
-static char *macaddr;
-module_param(macaddr, charp, 0);
-MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
-
-static u8 macaddr_buf[ETH_ALEN];
-
 static void vdpasim_net_work(struct work_struct *work)
 {
 	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
@@ -113,9 +107,7 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
 	struct virtio_net_config *net_config =
 		(struct virtio_net_config *)config;
 
-	net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
 	net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
-	memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
 }
 
 static void vdpasim_net_mgmtdev_release(struct device *dev)
@@ -134,6 +126,7 @@ static struct device vdpasim_net_mgmtdev_dummy = {
 
 static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
 {
+	struct virtio_net_config *cfg;
 	struct vdpasim_dev_attr dev_attr = {};
 	struct vdpasim *simdev;
 	int ret;
@@ -152,6 +145,10 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
 	if (IS_ERR(simdev))
 		return PTR_ERR(simdev);
 
+	cfg = simdev->config;
+	eth_random_addr(cfg->mac);
+	cfg->mtu = cpu_to_vdpasim16(simdev, 1500);
+
 	ret = _vdpa_register_device(&simdev->vdpa);
 	if (ret)
 		goto reg_err;
@@ -171,9 +168,25 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
 	_vdpa_unregister_device(&simdev->vdpa);
 }
 
+static int
+vdpasim_net_dev_config_set(struct vdpa_mgmt_dev *mdev,
+			   struct vdpa_device *dev,
+			   const struct vdpa_dev_config_set_attr *attrs)
+{
+	struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa);
+	struct virtio_net_config *dev_cfg = simdev->config;
+
+	if (attrs->mask.mac_valid)
+		memcpy(dev_cfg->mac, attrs->cfg.mac, sizeof(dev_cfg->mac));
+	if (attrs->mask.mtu_valid)
+		dev_cfg->mtu = cpu_to_vdpasim16(simdev, attrs->cfg.mtu);
+	return 0;
+}
+
 static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = {
 	.dev_add = vdpasim_net_dev_add,
-	.dev_del = vdpasim_net_dev_del
+	.dev_del = vdpasim_net_dev_del,
+	.dev_config_set = vdpasim_net_dev_config_set,
 };
 
 static struct virtio_device_id id_table[] = {
@@ -198,14 +211,6 @@ static int __init vdpasim_net_init(void)
 {
 	int ret;
 
-	if (macaddr) {
-		mac_pton(macaddr, macaddr_buf);
-		if (!is_valid_ether_addr(macaddr_buf))
-			return -EADDRNOTAVAIL;
-	} else {
-		eth_random_addr(macaddr_buf);
-	}
-
 	ret = device_register(&vdpasim_net_mgmtdev);
 	if (ret)
 		return ret;
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 4/9] vdpa_sim_net: Enable user to set mac address and mtu

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 08:18:39AM +0200, Parav Pandit wrote:
> Enable user to set the mac address and mtu so that each vdpa device
> can have its own user specified mac address and mtu.
> This is done by implementing the management device's configuration
> layout fields setting callback routine.
> 
> Now that user is enabled to set the mac address, remove the module
> parameter for same.

Will likely break some testing setups ...
Not too hard to keep it around, is it?

> 
> And example of setting mac addr and mtu:
> $ vdpa mgmtdev show
> 
> $ vdpa dev add name bar mgmtdev vdpasim_net
> $ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000
> 
> View the config after setting:
> $ vdpa dev config show
> bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0
> 
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 39 ++++++++++++++++------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index 240a5f1306b5..6e941b0e7935 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -29,12 +29,6 @@
>  
>  #define VDPASIM_NET_VQ_NUM	2
>  
> -static char *macaddr;
> -module_param(macaddr, charp, 0);
> -MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
> -
> -static u8 macaddr_buf[ETH_ALEN];
> -
>  static void vdpasim_net_work(struct work_struct *work)
>  {
>  	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> @@ -113,9 +107,7 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
>  	struct virtio_net_config *net_config =
>  		(struct virtio_net_config *)config;
>  
> -	net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
>  	net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> -	memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
>  }
>  
>  static void vdpasim_net_mgmtdev_release(struct device *dev)
> @@ -134,6 +126,7 @@ static struct device vdpasim_net_mgmtdev_dummy = {
>  
>  static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
>  {
> +	struct virtio_net_config *cfg;
>  	struct vdpasim_dev_attr dev_attr = {};
>  	struct vdpasim *simdev;
>  	int ret;
> @@ -152,6 +145,10 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
>  	if (IS_ERR(simdev))
>  		return PTR_ERR(simdev);
>  
> +	cfg = simdev->config;
> +	eth_random_addr(cfg->mac);
> +	cfg->mtu = cpu_to_vdpasim16(simdev, 1500);
> +
>  	ret = _vdpa_register_device(&simdev->vdpa);
>  	if (ret)
>  		goto reg_err;

Hmm moving it here is problematic:
this part happens before set_features so I suspect
endian-ness will be wrong for BE hosts ...


> @@ -171,9 +168,25 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
>  	_vdpa_unregister_device(&simdev->vdpa);
>  }
>  
> +static int
> +vdpasim_net_dev_config_set(struct vdpa_mgmt_dev *mdev,
> +			   struct vdpa_device *dev,
> +			   const struct vdpa_dev_config_set_attr *attrs)
> +{
> +	struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa);
> +	struct virtio_net_config *dev_cfg = simdev->config;
> +
> +	if (attrs->mask.mac_valid)
> +		memcpy(dev_cfg->mac, attrs->cfg.mac, sizeof(dev_cfg->mac));
> +	if (attrs->mask.mtu_valid)
> +		dev_cfg->mtu = cpu_to_vdpasim16(simdev, attrs->cfg.mtu);
> +	return 0;
> +}
> +
>  static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = {
>  	.dev_add = vdpasim_net_dev_add,
> -	.dev_del = vdpasim_net_dev_del
> +	.dev_del = vdpasim_net_dev_del,
> +	.dev_config_set = vdpasim_net_dev_config_set,
>  };
>  
>  static struct virtio_device_id id_table[] = {
> @@ -198,14 +211,6 @@ static int __init vdpasim_net_init(void)
>  {
>  	int ret;
>  
> -	if (macaddr) {
> -		mac_pton(macaddr, macaddr_buf);
> -		if (!is_valid_ether_addr(macaddr_buf))
> -			return -EADDRNOTAVAIL;
> -	} else {
> -		eth_random_addr(macaddr_buf);
> -	}
> -
>  	ret = device_register(&vdpasim_net_mgmtdev);
>  	if (ret)
>  		return ret;
> -- 
> 2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 4/9] vdpa_sim_net: Enable user to set mac address and mtu

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 24, 2021 12:27 PM
> 
> On Wed, Feb 24, 2021 at 08:18:39AM +0200, Parav Pandit wrote:
> > Enable user to set the mac address and mtu so that each vdpa device
> > can have its own user specified mac address and mtu.
> > This is done by implementing the management device's configuration
> > layout fields setting callback routine.
> >
> > Now that user is enabled to set the mac address, remove the module
> > parameter for same.
> 
> Will likely break some testing setups ...
> Not too hard to keep it around, is it?
>
Mostly not. In previous series, we moved away from default device to user created device without an additional module param.
So this shouldn't break it.
It gets confusing which one to use and module param applies same mac to all the vdpa devices.
So lets shift to user provided mac.

> >
> > And example of setting mac addr and mtu:
> > $ vdpa mgmtdev show
> >
> > $ vdpa dev add name bar mgmtdev vdpasim_net $ vdpa dev config set bar
> > mac 00:11:22:33:44:55 mtu 9000
> >
> > View the config after setting:
> > $ vdpa dev config show
> > bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed
> > 0 duplex 0
> >
> > Signed-off-by: Parav Pandit <parav@nvidia.com>
> > Reviewed-by: Eli Cohen <elic@nvidia.com>
> > ---
> >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 39
> > ++++++++++++++++------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index 240a5f1306b5..6e941b0e7935 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -29,12 +29,6 @@
> >
> >  #define VDPASIM_NET_VQ_NUM	2
> >
> > -static char *macaddr;
> > -module_param(macaddr, charp, 0);
> > -MODULE_PARM_DESC(macaddr, "Ethernet MAC address");
> > -
> > -static u8 macaddr_buf[ETH_ALEN];
> > -
> >  static void vdpasim_net_work(struct work_struct *work)  {
> >  	struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
> > @@ -113,9 +107,7 @@ static void vdpasim_net_get_config(struct vdpasim
> *vdpasim, void *config)
> >  	struct virtio_net_config *net_config =
> >  		(struct virtio_net_config *)config;
> >
> > -	net_config->mtu = cpu_to_vdpasim16(vdpasim, 1500);
> >  	net_config->status = cpu_to_vdpasim16(vdpasim,
> VIRTIO_NET_S_LINK_UP);
> > -	memcpy(net_config->mac, macaddr_buf, ETH_ALEN);
> >  }
> >
> >  static void vdpasim_net_mgmtdev_release(struct device *dev) @@ -134,6
> > +126,7 @@ static struct device vdpasim_net_mgmtdev_dummy = {
> >
> >  static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char
> > *name)  {
> > +	struct virtio_net_config *cfg;
> >  	struct vdpasim_dev_attr dev_attr = {};
> >  	struct vdpasim *simdev;
> >  	int ret;
> > @@ -152,6 +145,10 @@ static int vdpasim_net_dev_add(struct
> vdpa_mgmt_dev *mdev, const char *name)
> >  	if (IS_ERR(simdev))
> >  		return PTR_ERR(simdev);
> >
> > +	cfg = simdev->config;
> > +	eth_random_addr(cfg->mac);
> > +	cfg->mtu = cpu_to_vdpasim16(simdev, 1500);
> > +
> >  	ret = _vdpa_register_device(&simdev->vdpa);
> >  	if (ret)
> >  		goto reg_err;
> 
> Hmm moving it here is problematic:
> this part happens before set_features so I suspect endian-ness will be wrong
> for BE hosts ...
>
Hmm. I see it. Looking for solution to it now.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 5/9] vdpa_sim_net: Remove redundant get_config callback

[Parav Pandit]

Now that mac address and mtu are set in the vdpa_sim allocated config
space and read from the vdpa_sim maintained memory area, remove
get_config callback implementation.

Link status is setup only once, set up once after config space is
allocated by the vdpa_sim.

Signed-off-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Eli Cohen <elic@nvidia.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index 6e941b0e7935..61f1d37d8d60 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -102,14 +102,6 @@ static void vdpasim_net_work(struct work_struct *work)
 	spin_unlock(&vdpasim->lock);
 }
 
-static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
-{
-	struct virtio_net_config *net_config =
-		(struct virtio_net_config *)config;
-
-	net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
-}
-
 static void vdpasim_net_mgmtdev_release(struct device *dev)
 {
 }
@@ -137,7 +129,6 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
 	dev_attr.supported_features = VDPASIM_NET_FEATURES;
 	dev_attr.nvqs = VDPASIM_NET_VQ_NUM;
 	dev_attr.config_size = sizeof(struct virtio_net_config);
-	dev_attr.get_config = vdpasim_net_get_config;
 	dev_attr.work_fn = vdpasim_net_work;
 	dev_attr.buffer_size = PAGE_SIZE;
 
@@ -148,6 +139,7 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name)
 	cfg = simdev->config;
 	eth_random_addr(cfg->mac);
 	cfg->mtu = cpu_to_vdpasim16(simdev, 1500);
+	cfg->status = cpu_to_vdpasim16(simdev, VIRTIO_NET_S_LINK_UP);
 
 	ret = _vdpa_register_device(&simdev->vdpa);
 	if (ret)
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 6/9] vdpa/mlx5: Enable user to add/delete vdpa device

[Parav Pandit]

From: Eli Cohen <elic@nvidia.com>

Allow to control vdpa device creation and destruction using the vdpa
management tool.

Examples:
1. List the management devices
$ vdpa mgmtdev show
pci/0000:3b:00.1:
  supported_classes net

2. Create vdpa instance
$ vdpa dev add mgmtdev pci/0000:3b:00.1 name vdpa0

3. Show vdpa devices
$ vdpa dev show
vdpa0: type network mgmtdev pci/0000:3b:00.1 vendor_id 5555 max_vqs 16 \
max_vq_size 256

Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 79 +++++++++++++++++++++++++++----
 1 file changed, 70 insertions(+), 9 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 10e9b09932eb..b67bba581dfd 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1966,23 +1966,32 @@ static void init_mvqs(struct mlx5_vdpa_net *ndev)
 	}
 }
 
-static int mlx5v_probe(struct auxiliary_device *adev,
-		       const struct auxiliary_device_id *id)
+struct mlx5_vdpa_mgmtdev {
+	struct vdpa_mgmt_dev mgtdev;
+	struct mlx5_adev *madev;
+	struct mlx5_vdpa_net *ndev;
+};
+
+static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
 {
-	struct mlx5_adev *madev = container_of(adev, struct mlx5_adev, adev);
-	struct mlx5_core_dev *mdev = madev->mdev;
+	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
 	struct virtio_net_config *config;
 	struct mlx5_vdpa_dev *mvdev;
 	struct mlx5_vdpa_net *ndev;
+	struct mlx5_core_dev *mdev;
 	u32 max_vqs;
 	int err;
 
+	if (mgtdev->ndev)
+		return -ENOSPC;
+
+	mdev = mgtdev->madev->mdev;
 	/* we save one virtqueue for control virtqueue should we require it */
 	max_vqs = MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues);
 	max_vqs = min_t(u32, max_vqs, MLX5_MAX_SUPPORTED_VQS);
 
 	ndev = vdpa_alloc_device(struct mlx5_vdpa_net, mvdev.vdev, mdev->device, &mlx5_vdpa_ops,
-				 2 * mlx5_vdpa_max_qps(max_vqs), NULL);
+				 2 * mlx5_vdpa_max_qps(max_vqs), name);
 	if (IS_ERR(ndev))
 		return PTR_ERR(ndev);
 
@@ -2009,11 +2018,12 @@ static int mlx5v_probe(struct auxiliary_device *adev,
 	if (err)
 		goto err_res;
 
-	err = vdpa_register_device(&mvdev->vdev);
+	mvdev->vdev.mdev = &mgtdev->mgtdev;
+	err = _vdpa_register_device(&mvdev->vdev);
 	if (err)
 		goto err_reg;
 
-	dev_set_drvdata(&adev->dev, ndev);
+	mgtdev->ndev = ndev;
 	return 0;
 
 err_reg:
@@ -2026,11 +2036,62 @@ static int mlx5v_probe(struct auxiliary_device *adev,
 	return err;
 }
 
+static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev)
+{
+	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
+
+	_vdpa_unregister_device(dev);
+	mgtdev->ndev = NULL;
+}
+
+static const struct vdpa_mgmtdev_ops mdev_ops = {
+	.dev_add = mlx5_vdpa_dev_add,
+	.dev_del = mlx5_vdpa_dev_del,
+};
+
+static struct virtio_device_id id_table[] = {
+	{ VIRTIO_ID_NET, VIRTIO_DEV_ANY_ID },
+	{ 0 },
+};
+
+static int mlx5v_probe(struct auxiliary_device *adev,
+		       const struct auxiliary_device_id *id)
+
+{
+	struct mlx5_adev *madev = container_of(adev, struct mlx5_adev, adev);
+	struct mlx5_core_dev *mdev = madev->mdev;
+	struct mlx5_vdpa_mgmtdev *mgtdev;
+	int err;
+
+	mgtdev = kzalloc(sizeof(*mgtdev), GFP_KERNEL);
+	if (!mgtdev)
+		return -ENOMEM;
+
+	mgtdev->mgtdev.ops = &mdev_ops;
+	mgtdev->mgtdev.device = mdev->device;
+	mgtdev->mgtdev.id_table = id_table;
+	mgtdev->madev = madev;
+
+	err = vdpa_mgmtdev_register(&mgtdev->mgtdev);
+	if (err)
+		goto reg_err;
+
+	dev_set_drvdata(&adev->dev, mgtdev);
+
+	return 0;
+
+reg_err:
+	kfree(mdev);
+	return err;
+}
+
 static void mlx5v_remove(struct auxiliary_device *adev)
 {
-	struct mlx5_vdpa_dev *mvdev = dev_get_drvdata(&adev->dev);
+	struct mlx5_vdpa_mgmtdev *mgtdev;
 
-	vdpa_unregister_device(&mvdev->vdev);
+	mgtdev = dev_get_drvdata(&adev->dev);
+	vdpa_mgmtdev_unregister(&mgtdev->mgtdev);
+	kfree(mgtdev);
 }
 
 static const struct auxiliary_device_id mlx5v_id_table[] = {
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Parav Pandit]

From: Eli Cohen <elic@nvidia.com>

Use a randomly generated MAC address to be applied in case it is not
configured by management tool.

The value queried through mlx5_query_nic_vport_mac_address() is not
relelavnt to vdpa since it is the mac that should be used by the regular
NIC driver.

Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b67bba581dfd..ece2183e7b20 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
 	if (err)
 		goto err_mtu;
 
-	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
-	if (err)
-		goto err_mtu;
-
+	eth_random_addr(config->mac);
 	mvdev->vdev.dma_dev = mdev->device;
 	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
 	if (err)
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Jason Wang]

On 2021/2/24 2:18 下午, Parav Pandit wrote:
> From: Eli Cohen <elic@nvidia.com>
>
> Use a randomly generated MAC address to be applied in case it is not
> configured by management tool.
>
> The value queried through mlx5_query_nic_vport_mac_address() is not
> relelavnt to vdpa since it is the mac that should be used by the regular
> NIC driver.
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>


Acked-by: Jason Wang <jasowang@redhat.com>


> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b67bba581dfd..ece2183e7b20 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   	if (err)
>   		goto err_mtu;
>   
> -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> -	if (err)
> -		goto err_mtu;
> -
> +	eth_random_addr(config->mac);
>   	mvdev->vdev.dma_dev = mdev->device;
>   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>   	if (err)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Michael S. Tsirkin]

On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > 
> > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > From: Eli Cohen <elic@nvidia.com>
> > > 
> > > Use a randomly generated MAC address to be applied in case it is not
> > > configured by management tool.
> > > 
> > > The value queried through mlx5_query_nic_vport_mac_address() is not
> > > relelavnt to vdpa since it is the mac that should be used by the regular
> > > NIC driver.
> > > 
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > 
> > 
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> > 
> > > ---
> > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index b67bba581dfd..ece2183e7b20 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > >   	if (err)
> > >   		goto err_mtu;
> > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > -	if (err)
> > > -		goto err_mtu;
> > > -
> > > +	eth_random_addr(config->mac);
> 
> I think this patch is missing setting VIRTIO_NET_F_MTU. I will post v2
> with the other fixes in this series.

I don't really understand why this is a good idea.

If userspace wants a random mac it can set it, with this
patch it is impossible to know whether the mac is
a hardware one (which will be persistent e.g. across reboots)
or a random one.

E.g. there is a patch configuring a userspace supplied
mac if the hardware mac is zero.

This patch will break it.

> > >   	mvdev->vdev.dma_dev = mdev->device;
> > >   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > >   	if (err)
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Michael S. Tsirkin]

On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
> On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > > > 
> > > > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > > > From: Eli Cohen <elic@nvidia.com>
> > > > > 
> > > > > Use a randomly generated MAC address to be applied in case it is not
> > > > > configured by management tool.
> > > > > 
> > > > > The value queried through mlx5_query_nic_vport_mac_address() is not
> > > > > relelavnt to vdpa since it is the mac that should be used by the regular
> > > > > NIC driver.
> > > > > 
> > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > 
> > > > 
> > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > 
> > > > 
> > > > > ---
> > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index b67bba581dfd..ece2183e7b20 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
> > > > >   	if (err)
> > > > >   		goto err_mtu;
> > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> > > > > -	if (err)
> > > > > -		goto err_mtu;
> > > > > -
> > > > > +	eth_random_addr(config->mac);
> > > 
> > > I think this patch is missing setting VIRTIO_NET_F_MTU. I will post v2
> > > with the other fixes in this series.
> > 
> > I don't really understand why this is a good idea.
> > 
> > If userspace wants a random mac it can set it, with this
> > patch it is impossible to know whether the mac is
> > a hardware one (which will be persistent e.g. across reboots)
> > or a random one.
> > 
> 
> You can still get a persistent MAC set by the vdpa tool. e.g
> 
> vdpa dev config set vdpa0 mac 00:11:22:33:44:55
> 
> If you don't use vdpa tool, the device assigns a random MAC. This MAC is
> used to filter imcoming unicast traffic (done in a subsequent patch).

Well previously device learned the MAC from outgoing traffic
and used that for the filter.
Is changing that to a random value really all that useful as
a default?  Why not do the randomization in userspace?


> > E.g. there is a patch configuring a userspace supplied
> > mac if the hardware mac is zero.
> > 
> > This patch will break it.
> > 
> > > > >   	mvdev->vdev.dma_dev = mdev->device;
> > > > >   	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> > > > >   	if (err)
> > > > 
> > 

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Jason Wang]

On 2021/3/2 12:12 上午, Michael S. Tsirkin wrote:
> On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
>> On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
>>> On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
>>>> On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
>>>>> On 2021/2/24 2:18 下午, Parav Pandit wrote:
>>>>>> From: Eli Cohen <elic@nvidia.com>
>>>>>>
>>>>>> Use a randomly generated MAC address to be applied in case it is not
>>>>>> configured by management tool.
>>>>>>
>>>>>> The value queried through mlx5_query_nic_vport_mac_address() is not
>>>>>> relelavnt to vdpa since it is the mac that should be used by the regular
>>>>>> NIC driver.
>>>>>>
>>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>>> Reviewed-by: Parav Pandit <parav@nvidia.com>
>>>>>
>>>>> Acked-by: Jason Wang <jasowang@redhat.com>
>>>>>
>>>>>
>>>>>> ---
>>>>>>    drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
>>>>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>> index b67bba581dfd..ece2183e7b20 100644
>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>>> @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>>>>>>    	if (err)
>>>>>>    		goto err_mtu;
>>>>>> -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
>>>>>> -	if (err)
>>>>>> -		goto err_mtu;
>>>>>> -
>>>>>> +	eth_random_addr(config->mac);
>>>> I think this patch is missing setting VIRTIO_NET_F_MTU. I will post v2
>>>> with the other fixes in this series.
>>> I don't really understand why this is a good idea.
>>>
>>> If userspace wants a random mac it can set it, with this
>>> patch it is impossible to know whether the mac is
>>> a hardware one (which will be persistent e.g. across reboots)
>>> or a random one.
>>>
>> You can still get a persistent MAC set by the vdpa tool. e.g
>>
>> vdpa dev config set vdpa0 mac 00:11:22:33:44:55
>>
>> If you don't use vdpa tool, the device assigns a random MAC. This MAC is
>> used to filter imcoming unicast traffic (done in a subsequent patch).
> Well previously device learned the MAC from outgoing traffic
> and used that for the filter.
> Is changing that to a random value really all that useful as
> a default?  Why not do the randomization in userspace?


I think the point is that, if userspace doens't specify a mac, it can 
still work.

Thanks


>
>
>>> E.g. there is a patch configuring a userspace supplied
>>> mac if the hardware mac is zero.
>>>
>>> This patch will break it.
>>>
>>>>>>    	mvdev->vdev.dma_dev = mdev->device;
>>>>>>    	err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
>>>>>>    	if (err)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Parav Pandit]

Hi Eli,

> From: Eli Cohen <elic@nvidia.com>
> Sent: Tuesday, March 2, 2021 11:09 AM
> 
> On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote:
> > On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
> > > On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
> > > > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> > > > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > > > > >
> > > > > > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > > > > > From: Eli Cohen <elic@nvidia.com>
> > > > > > >
> > > > > > > Use a randomly generated MAC address to be applied in case
> > > > > > > it is not configured by management tool.
> > > > > > >
> > > > > > > The value queried through mlx5_query_nic_vport_mac_address()
> > > > > > > is not relelavnt to vdpa since it is the mac that should be
> > > > > > > used by the regular NIC driver.
> > > > > > >
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > >
> > > > > >
> > > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > >
> > > > > >
> > > > > > > ---
> > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > index b67bba581dfd..ece2183e7b20 100644
> > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct
> vdpa_mgmt_dev *v_mdev, const char *name)
> > > > > > >   	if (err)
> > > > > > >   		goto err_mtu;
> > > > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config-
> >mac);
> > > > > > > -	if (err)
> > > > > > > -		goto err_mtu;
> > > > > > > -
> > > > > > > +	eth_random_addr(config->mac);
> > > > >
> > > > > I think this patch is missing setting VIRTIO_NET_F_MTU. I will
> > > > > post v2 with the other fixes in this series.
> > > >
> > > > I don't really understand why this is a good idea.
> > > >
> > > > If userspace wants a random mac it can set it, with this patch it
> > > > is impossible to know whether the mac is a hardware one (which
> > > > will be persistent e.g. across reboots) or a random one.
> > > >
> > >
> > > You can still get a persistent MAC set by the vdpa tool. e.g
> > >
> > > vdpa dev config set vdpa0 mac 00:11:22:33:44:55
> > >
> > > If you don't use vdpa tool, the device assigns a random MAC. This
> > > MAC is used to filter imcoming unicast traffic (done in a subsequent
> patch).
> >
> > Well previously device learned the MAC from outgoing traffic and used
> > that for the filter.
> 
> No, was is added only in the last series that Parav send. Before that the
> device did not filter and forwarded any packet that was forwarded to it buy
> the eswitch.
> 
> > Is changing that to a random value really all that useful as a
> > default?  Why not do the randomization in userspace?
> >
> 
> I think we want management entity to set the MAC, that's the VDPA tool.
> So as things are right now (with the last series applied), the vdpa tool is the
> tool to assign MAC addresses and if it doesn't, a device randomly generated
> MAC applies. Currently MAC addresses set by qemu command line are
> ignored (set_config is not implementd). We can allow this but this will
> override vdpa tool configuration.

I believe its incorrect to always do random generated mac as of this patch.
This is because, doing so ignores any admin config done by the sysadmin on the switch (ovs side) using [1].

So if user choose to configure using eswitch config, mlx5_vnet to honor that.
And if user prefers to override is using vdpa tool or set_config from QEMU side, so be it.
Hence, instead of reporting all zeros, mlx5 should query own vport context and publish that mac in the config layout and rx steering side.

If user choose to override it, config layout and rx rules will have to updated on such config.

[1] devlink port function set pci/0000:03:00.0/<port_id_of_sf/vf>/ hw_addr 00:11:22:33:44:55
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 08:33:50AM +0200, Eli Cohen wrote:
> On Wed, Mar 03, 2021 at 05:59:50AM +0200, Parav Pandit wrote:
> > Hi Eli,
> > 
> > > From: Eli Cohen <elic@nvidia.com>
> > > Sent: Tuesday, March 2, 2021 11:09 AM
> > > 
> > > On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote:
> > > > On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
> > > > > On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> > > > > > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > > > > > > >
> > > > > > > > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > > > > > > > From: Eli Cohen <elic@nvidia.com>
> > > > > > > > >
> > > > > > > > > Use a randomly generated MAC address to be applied in case
> > > > > > > > > it is not configured by management tool.
> > > > > > > > >
> > > > > > > > > The value queried through mlx5_query_nic_vport_mac_address()
> > > > > > > > > is not relelavnt to vdpa since it is the mac that should be
> > > > > > > > > used by the regular NIC driver.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > > > >
> > > > > > > >
> > > > > > > > > ---
> > > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > index b67bba581dfd..ece2183e7b20 100644
> > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct
> > > vdpa_mgmt_dev *v_mdev, const char *name)
> > > > > > > > >   	if (err)
> > > > > > > > >   		goto err_mtu;
> > > > > > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config-
> > > >mac);
> > > > > > > > > -	if (err)
> > > > > > > > > -		goto err_mtu;
> > > > > > > > > -
> > > > > > > > > +	eth_random_addr(config->mac);
> > > > > > >
> > > > > > > I think this patch is missing setting VIRTIO_NET_F_MTU. I will
> > > > > > > post v2 with the other fixes in this series.
> > > > > >
> > > > > > I don't really understand why this is a good idea.
> > > > > >
> > > > > > If userspace wants a random mac it can set it, with this patch it
> > > > > > is impossible to know whether the mac is a hardware one (which
> > > > > > will be persistent e.g. across reboots) or a random one.
> > > > > >
> > > > >
> > > > > You can still get a persistent MAC set by the vdpa tool. e.g
> > > > >
> > > > > vdpa dev config set vdpa0 mac 00:11:22:33:44:55
> > > > >
> > > > > If you don't use vdpa tool, the device assigns a random MAC. This
> > > > > MAC is used to filter imcoming unicast traffic (done in a subsequent
> > > patch).
> > > >
> > > > Well previously device learned the MAC from outgoing traffic and used
> > > > that for the filter.
> > > 
> > > No, was is added only in the last series that Parav send. Before that the
> > > device did not filter and forwarded any packet that was forwarded to it buy
> > > the eswitch.
> > > 
> > > > Is changing that to a random value really all that useful as a
> > > > default?  Why not do the randomization in userspace?
> > > >
> > > 
> > > I think we want management entity to set the MAC, that's the VDPA tool.
> > > So as things are right now (with the last series applied), the vdpa tool is the
> > > tool to assign MAC addresses and if it doesn't, a device randomly generated
> > > MAC applies. Currently MAC addresses set by qemu command line are
> > > ignored (set_config is not implementd). We can allow this but this will
> > > override vdpa tool configuration.
> > 
> > I believe its incorrect to always do random generated mac as of this patch.
> > This is because, doing so ignores any admin config done by the sysadmin on the switch (ovs side) using [1].
> > 
> Well, when this patch was sent, we still had thoughts to let mlx5e NIC
> and the VDPA to co-exist on the same function (VF or SF). Now that we're
> off this idea you can become tempted to use the MAC address configured
> for the NIC but I don't think it's a good idea. We already have a
> dedicated tool to configure MAC for VDPA so let's use it.

Right. And users can decide to reuse the hardware MAC if they want to.

> > So if user choose to configure using eswitch config, mlx5_vnet to honor that.
> > And if user prefers to override is using vdpa tool or set_config from QEMU side, so be it.
> 
> I agree that we should let the user to configure the MAC through qemu
> argument when booting the VM. So I'll add this for the next spin of this
> series.

OK so
- if MAC is configured it is used
- if not configured 0 is reported to userspace

fair summary?

> > Hence, instead of reporting all zeros, mlx5 should query own vport context and publish that mac in the config layout and rx steering side.
> > 
> > If user choose to override it, config layout and rx rules will have to updated on such config.
> > 
> > [1] devlink port function set pci/0000:03:00.0/<port_id_of_sf/vf>/ hw_addr 00:11:22:33:44:55

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, March 3, 2021 2:59 PM
> 
> On Wed, Mar 03, 2021 at 08:33:50AM +0200, Eli Cohen wrote:
> > On Wed, Mar 03, 2021 at 05:59:50AM +0200, Parav Pandit wrote:
> > > Hi Eli,
> > >
> > > > From: Eli Cohen <elic@nvidia.com>
> > > > Sent: Tuesday, March 2, 2021 11:09 AM
> > > >
> > > > On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote:
> > > > > On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
> > > > > > On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> > > > > > > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > > > > > > > >
> > > > > > > > > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > > > > > > > > From: Eli Cohen <elic@nvidia.com>
> > > > > > > > > >
> > > > > > > > > > Use a randomly generated MAC address to be applied in
> > > > > > > > > > case it is not configured by management tool.
> > > > > > > > > >
> > > > > > > > > > The value queried through
> > > > > > > > > > mlx5_query_nic_vport_mac_address()
> > > > > > > > > > is not relelavnt to vdpa since it is the mac that
> > > > > > > > > > should be used by the regular NIC driver.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > > ---
> > > > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > index b67bba581dfd..ece2183e7b20 100644
> > > > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > > > @@ -2005,10 +2005,7 @@ static int
> > > > > > > > > > mlx5_vdpa_dev_add(struct
> > > > vdpa_mgmt_dev *v_mdev, const char *name)
> > > > > > > > > >   	if (err)
> > > > > > > > > >   		goto err_mtu;
> > > > > > > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0,
> 0, config-
> > > > >mac);
> > > > > > > > > > -	if (err)
> > > > > > > > > > -		goto err_mtu;
> > > > > > > > > > -
> > > > > > > > > > +	eth_random_addr(config->mac);
> > > > > > > >
> > > > > > > > I think this patch is missing setting VIRTIO_NET_F_MTU. I
> > > > > > > > will post v2 with the other fixes in this series.
> > > > > > >
> > > > > > > I don't really understand why this is a good idea.
> > > > > > >
> > > > > > > If userspace wants a random mac it can set it, with this
> > > > > > > patch it is impossible to know whether the mac is a hardware
> > > > > > > one (which will be persistent e.g. across reboots) or a random
> one.
> > > > > > >
> > > > > >
> > > > > > You can still get a persistent MAC set by the vdpa tool. e.g
> > > > > >
> > > > > > vdpa dev config set vdpa0 mac 00:11:22:33:44:55
> > > > > >
> > > > > > If you don't use vdpa tool, the device assigns a random MAC.
> > > > > > This MAC is used to filter imcoming unicast traffic (done in a
> > > > > > subsequent
> > > > patch).
> > > > >
> > > > > Well previously device learned the MAC from outgoing traffic and
> > > > > used that for the filter.
> > > >
> > > > No, was is added only in the last series that Parav send. Before
> > > > that the device did not filter and forwarded any packet that was
> > > > forwarded to it buy the eswitch.
> > > >
> > > > > Is changing that to a random value really all that useful as a
> > > > > default?  Why not do the randomization in userspace?
> > > > >
> > > >
> > > > I think we want management entity to set the MAC, that's the VDPA
> tool.
> > > > So as things are right now (with the last series applied), the
> > > > vdpa tool is the tool to assign MAC addresses and if it doesn't, a
> > > > device randomly generated MAC applies. Currently MAC addresses set
> > > > by qemu command line are ignored (set_config is not implementd).
> > > > We can allow this but this will override vdpa tool configuration.
> > >
> > > I believe its incorrect to always do random generated mac as of this
> patch.
> > > This is because, doing so ignores any admin config done by the sysadmin
> on the switch (ovs side) using [1].
> > >
> > Well, when this patch was sent, we still had thoughts to let mlx5e NIC
> > and the VDPA to co-exist on the same function (VF or SF). Now that
> > we're off this idea you can become tempted to use the MAC address
> > configured for the NIC but I don't think it's a good idea. We already
> > have a dedicated tool to configure MAC for VDPA so let's use it.
> 
> Right. And users can decide to reuse the hardware MAC if they want to.
Right.
If user chose to reuse the hw mac set by the eswitch, so let them use it.
When smartnic is used, some users do not trust compute side for network attributes configuration.
So those users to configure the MAC from eswitch along with ovs policy.
I think Sean gave one example of it.

And user can choose to override it via vdpa tool/qemu.

I am not sure doing only one_way fits all.
Switch/ovs side configuring the mac along with policy seems fine to me.
But this may not fit the case for those who doesn't have eswitch.
Jason offline or in previous thread mentioned a use case to create multiple vdpa device from single PF/VF.
At Mellanox at least we don't see that use case at moment given its attached to eswitch.

> 
> > > So if user choose to configure using eswitch config, mlx5_vnet to honor
> that.
> > > And if user prefers to override is using vdpa tool or set_config from
> QEMU side, so be it.
> >
> > I agree that we should let the user to configure the MAC through qemu
> > argument when booting the VM. So I'll add this for the next spin of
> > this series.
> 
> OK so
> - if MAC is configured it is used
> - if not configured 0 is reported to userspace
> 
> fair summary?
> 
LGTM.
Eli?

> > > Hence, instead of reporting all zeros, mlx5 should query own vport
> context and publish that mac in the config layout and rx steering side.
> > >
> > > If user choose to override it, config layout and rx rules will have to
> updated on such config.
> > >
> > > [1] devlink port function set pci/0000:03:00.0/<port_id_of_sf/vf>/
> > > hw_addr 00:11:22:33:44:55

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 7/9] vdpa/mlx5: Provide device generated random MAC address

[Michael S. Tsirkin]

On Wed, Mar 03, 2021 at 03:59:50AM +0000, Parav Pandit wrote:
> Hi Eli,
> 
> > From: Eli Cohen <elic@nvidia.com>
> > Sent: Tuesday, March 2, 2021 11:09 AM
> > 
> > On Mon, Mar 01, 2021 at 11:12:33AM -0500, Michael S. Tsirkin wrote:
> > > On Mon, Mar 01, 2021 at 03:19:51PM +0200, Eli Cohen wrote:
> > > > On Mon, Mar 01, 2021 at 08:09:48AM -0500, Michael S. Tsirkin wrote:
> > > > > On Mon, Mar 01, 2021 at 09:08:28AM +0200, Eli Cohen wrote:
> > > > > > On Wed, Feb 24, 2021 at 05:11:23PM +0800, Jason Wang wrote:
> > > > > > >
> > > > > > > On 2021/2/24 2:18 下午, Parav Pandit wrote:
> > > > > > > > From: Eli Cohen <elic@nvidia.com>
> > > > > > > >
> > > > > > > > Use a randomly generated MAC address to be applied in case
> > > > > > > > it is not configured by management tool.
> > > > > > > >
> > > > > > > > The value queried through mlx5_query_nic_vport_mac_address()
> > > > > > > > is not relelavnt to vdpa since it is the mac that should be
> > > > > > > > used by the regular NIC driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > > >
> > > > > > >
> > > > > > > Acked-by: Jason Wang <jasowang@redhat.com>
> > > > > > >
> > > > > > >
> > > > > > > > ---
> > > > > > > >   drivers/vdpa/mlx5/net/mlx5_vnet.c | 5 +----
> > > > > > > >   1 file changed, 1 insertion(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > index b67bba581dfd..ece2183e7b20 100644
> > > > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > > > @@ -2005,10 +2005,7 @@ static int mlx5_vdpa_dev_add(struct
> > vdpa_mgmt_dev *v_mdev, const char *name)
> > > > > > > >   	if (err)
> > > > > > > >   		goto err_mtu;
> > > > > > > > -	err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config-
> > >mac);
> > > > > > > > -	if (err)
> > > > > > > > -		goto err_mtu;
> > > > > > > > -
> > > > > > > > +	eth_random_addr(config->mac);
> > > > > >
> > > > > > I think this patch is missing setting VIRTIO_NET_F_MTU. I will
> > > > > > post v2 with the other fixes in this series.
> > > > >
> > > > > I don't really understand why this is a good idea.
> > > > >
> > > > > If userspace wants a random mac it can set it, with this patch it
> > > > > is impossible to know whether the mac is a hardware one (which
> > > > > will be persistent e.g. across reboots) or a random one.
> > > > >
> > > >
> > > > You can still get a persistent MAC set by the vdpa tool. e.g
> > > >
> > > > vdpa dev config set vdpa0 mac 00:11:22:33:44:55
> > > >
> > > > If you don't use vdpa tool, the device assigns a random MAC. This
> > > > MAC is used to filter imcoming unicast traffic (done in a subsequent
> > patch).
> > >
> > > Well previously device learned the MAC from outgoing traffic and used
> > > that for the filter.
> > 
> > No, was is added only in the last series that Parav send. Before that the
> > device did not filter and forwarded any packet that was forwarded to it buy
> > the eswitch.
> > 
> > > Is changing that to a random value really all that useful as a
> > > default?  Why not do the randomization in userspace?
> > >
> > 
> > I think we want management entity to set the MAC, that's the VDPA tool.
> > So as things are right now (with the last series applied), the vdpa tool is the
> > tool to assign MAC addresses and if it doesn't, a device randomly generated
> > MAC applies. Currently MAC addresses set by qemu command line are
> > ignored (set_config is not implementd). We can allow this but this will
> > override vdpa tool configuration.
> 
> I believe its incorrect to always do random generated mac as of this patch.
> This is because, doing so ignores any admin config done by the sysadmin on the switch (ovs side) using [1].
> 
> So if user choose to configure using eswitch config, mlx5_vnet to honor that.
> And if user prefers to override is using vdpa tool or set_config from QEMU side, so be it.
> Hence, instead of reporting all zeros, mlx5 should query own vport context and publish that mac in the config layout and rx steering side.
> 
> If user choose to override it, config layout and rx rules will have to updated on such config.
> 
> [1] devlink port function set pci/0000:03:00.0/<port_id_of_sf/vf>/ hw_addr 00:11:22:33:44:55

well it has reported all zeros to mean "learning bridge" for a
so while I suspect changing that is an ABI change, though
a minor one ...

If you do change it please add some other way to find out
whether it still learns from outgoing traffic
(ie whether mac spoofing is enabled).

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 8/9] vdpa/mlx5: Support configuration of MAC

[Parav Pandit]

From: Eli Cohen <elic@nvidia.com>

Add code to accept MAC configuration through vdpa tool. The MAC is
written into the config struct and later can be retrieved through
get_config().

Examples:
1. Configure MAC:
$ vdpa dev config set vdpa0 mac 00:11:22:33:44:55

2. Show configured params:
$ vdpa dev config show
vdpa0: mac 00:11:22:33:44:55 link down link_announce false mtu 0 speed 0 duplex 0

Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index ece2183e7b20..51a3fc4cde4d 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1972,6 +1972,22 @@ struct mlx5_vdpa_mgmtdev {
 	struct mlx5_vdpa_net *ndev;
 };
 
+static int mlx5_vdpa_net_dev_config_set(struct vdpa_mgmt_dev *v_mdev,
+					struct vdpa_device *vdev,
+					const struct vdpa_dev_config_set_attr *attrs)
+{
+	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
+	struct mlx5_vdpa_net *ndev = mgtdev->ndev;
+
+	if (attrs->mask.mtu_valid)
+		return -EOPNOTSUPP;
+
+	if (attrs->mask.mac_valid)
+		memcpy(ndev->config.mac, attrs->cfg.mac, ETH_ALEN);
+
+	return 0;
+}
+
 static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
 {
 	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
@@ -2044,6 +2060,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
 static const struct vdpa_mgmtdev_ops mdev_ops = {
 	.dev_add = mlx5_vdpa_dev_add,
 	.dev_del = mlx5_vdpa_dev_del,
+	.dev_config_set = mlx5_vdpa_net_dev_config_set,
 };
 
 static struct virtio_device_id id_table[] = {
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 8/9] vdpa/mlx5: Support configuration of MAC

[Jason Wang]

On 2021/2/24 2:18 下午, Parav Pandit wrote:
> From: Eli Cohen <elic@nvidia.com>
>
> Add code to accept MAC configuration through vdpa tool. The MAC is
> written into the config struct and later can be retrieved through
> get_config().
>
> Examples:
> 1. Configure MAC:
> $ vdpa dev config set vdpa0 mac 00:11:22:33:44:55
>
> 2. Show configured params:
> $ vdpa dev config show
> vdpa0: mac 00:11:22:33:44:55 link down link_announce false mtu 0 speed 0 duplex 0
>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---


Acked-by: Jason Wang <jasowang@redhat.com>


>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index ece2183e7b20..51a3fc4cde4d 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1972,6 +1972,22 @@ struct mlx5_vdpa_mgmtdev {
>   	struct mlx5_vdpa_net *ndev;
>   };
>   
> +static int mlx5_vdpa_net_dev_config_set(struct vdpa_mgmt_dev *v_mdev,
> +					struct vdpa_device *vdev,
> +					const struct vdpa_dev_config_set_attr *attrs)
> +{
> +	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> +	struct mlx5_vdpa_net *ndev = mgtdev->ndev;
> +
> +	if (attrs->mask.mtu_valid)
> +		return -EOPNOTSUPP;
> +
> +	if (attrs->mask.mac_valid)
> +		memcpy(ndev->config.mac, attrs->cfg.mac, ETH_ALEN);
> +
> +	return 0;
> +}
> +
>   static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name)
>   {
>   	struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev);
> @@ -2044,6 +2060,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *
>   static const struct vdpa_mgmtdev_ops mdev_ops = {
>   	.dev_add = mlx5_vdpa_dev_add,
>   	.dev_del = mlx5_vdpa_dev_del,
> +	.dev_config_set = mlx5_vdpa_net_dev_config_set,
>   };
>   
>   static struct virtio_device_id id_table[] = {

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH linux-next 9/9] vdpa/mlx5: Forward only packets with allowed MAC address

[Parav Pandit]

From: Eli Cohen <elic@nvidia.com>

Add rules to forward packets to the net device's TIR only if the
destination MAC is equal to the configured MAC. This is required to
prevent the netdevice from receiving traffic not destined to its
configured MAC.

Signed-off-by: Eli Cohen <elic@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 84 +++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 20 deletions(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 51a3fc4cde4d..9b580c67acda 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -147,7 +147,8 @@ struct mlx5_vdpa_net {
 	struct mutex reslock;
 	struct mlx5_flow_table *rxft;
 	struct mlx5_fc *rx_counter;
-	struct mlx5_flow_handle *rx_rule;
+	struct mlx5_flow_handle *rx_rule_ucast;
+	struct mlx5_flow_handle *rx_rule_mcast;
 	bool setup;
 	u16 mtu;
 };
@@ -1294,21 +1295,34 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 	struct mlx5_flow_table_attr ft_attr = {};
 	struct mlx5_flow_act flow_act = {};
 	struct mlx5_flow_namespace *ns;
+	struct mlx5_flow_spec *spec;
+	void *headers_c;
+	void *headers_v;
+	u8 *dmac_c;
+	u8 *dmac_v;
 	int err;
 
-	/* for now, one entry, match all, forward to tir */
-	ft_attr.max_fte = 1;
-	ft_attr.autogroup.max_num_groups = 1;
+	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
+	if (!spec)
+		return -ENOMEM;
+
+	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
+	ft_attr.max_fte = 2;
+	ft_attr.autogroup.max_num_groups = 2;
 
-	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
+	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev,
+				     MLX5_FLOW_NAMESPACE_BYPASS);
 	if (!ns) {
-		mlx5_vdpa_warn(&ndev->mvdev, "get flow namespace\n");
-		return -EOPNOTSUPP;
+		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
+		err = -EOPNOTSUPP;
+		goto err_ns;
 	}
 
 	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
-	if (IS_ERR(ndev->rxft))
-		return PTR_ERR(ndev->rxft);
+	if (IS_ERR(ndev->rxft)) {
+		err = PTR_ERR(ndev->rxft);
+		goto err_ns;
+	}
 
 	ndev->rx_counter = mlx5_fc_create(ndev->mvdev.mdev, false);
 	if (IS_ERR(ndev->rx_counter)) {
@@ -1316,37 +1330,67 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 		goto err_fc;
 	}
 
-	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST | MLX5_FLOW_CONTEXT_ACTION_COUNT;
+	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
+	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
+	memset(dmac_c, 0xff, ETH_ALEN);
+	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
+	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
+	ether_addr_copy(dmac_v, ndev->config.mac);
+
+	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST |
+			  MLX5_FLOW_CONTEXT_ACTION_COUNT;
 	dest[0].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
 	dest[0].tir_num = ndev->res.tirn;
 	dest[1].type = MLX5_FLOW_DESTINATION_TYPE_COUNTER;
 	dest[1].counter_id = mlx5_fc_id(ndev->rx_counter);
-	ndev->rx_rule = mlx5_add_flow_rules(ndev->rxft, NULL, &flow_act, dest, 2);
-	if (IS_ERR(ndev->rx_rule)) {
-		err = PTR_ERR(ndev->rx_rule);
-		ndev->rx_rule = NULL;
-		goto err_rule;
+	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act,
+						  dest, 2);
+
+	if (IS_ERR(ndev->rx_rule_ucast)) {
+		err = PTR_ERR(ndev->rx_rule_ucast);
+		ndev->rx_rule_ucast = NULL;
+		goto err_rule_ucast;
+	}
+
+	memset(dmac_c, 0, ETH_ALEN);
+	memset(dmac_v, 0, ETH_ALEN);
+	dmac_c[0] = 1;
+	dmac_v[0] = 1;
+	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
+	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act,
+						  dest, 1);
+	if (IS_ERR(ndev->rx_rule_mcast)) {
+		err = PTR_ERR(ndev->rx_rule_mcast);
+		ndev->rx_rule_mcast = NULL;
+		goto err_rule_mcast;
 	}
 
+	kvfree(spec);
 	return 0;
 
-err_rule:
+err_rule_mcast:
+	mlx5_del_flow_rules(ndev->rx_rule_ucast);
+	ndev->rx_rule_ucast = NULL;
+err_rule_ucast:
 	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
 err_fc:
 	mlx5_destroy_flow_table(ndev->rxft);
+err_ns:
+	kvfree(spec);
 	return err;
 }
 
 static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
 {
-	if (!ndev->rx_rule)
+	if (!ndev->rx_rule_ucast)
 		return;
 
-	mlx5_del_flow_rules(ndev->rx_rule);
+	mlx5_del_flow_rules(ndev->rx_rule_mcast);
+	ndev->rx_rule_mcast = NULL;
+	mlx5_del_flow_rules(ndev->rx_rule_ucast);
+	ndev->rx_rule_ucast = NULL;
 	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
 	mlx5_destroy_flow_table(ndev->rxft);
-
-	ndev->rx_rule = NULL;
 }
 
 static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)
-- 
2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 9/9] vdpa/mlx5: Forward only packets with allowed MAC address

[Jason Wang]

On 2021/2/24 2:18 下午, Parav Pandit wrote:
> From: Eli Cohen <elic@nvidia.com>
>
> Add rules to forward packets to the net device's TIR only if the
> destination MAC is equal to the configured MAC. This is required to
> prevent the netdevice from receiving traffic not destined to its
> configured MAC.


Just to confirm, this will only apply to unicast packet and we will 
still let multicast/broadcast to go?

Thanks


>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---
>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 84 +++++++++++++++++++++++--------
>   1 file changed, 64 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 51a3fc4cde4d..9b580c67acda 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -147,7 +147,8 @@ struct mlx5_vdpa_net {
>   	struct mutex reslock;
>   	struct mlx5_flow_table *rxft;
>   	struct mlx5_fc *rx_counter;
> -	struct mlx5_flow_handle *rx_rule;
> +	struct mlx5_flow_handle *rx_rule_ucast;
> +	struct mlx5_flow_handle *rx_rule_mcast;
>   	bool setup;
>   	u16 mtu;
>   };
> @@ -1294,21 +1295,34 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   	struct mlx5_flow_table_attr ft_attr = {};
>   	struct mlx5_flow_act flow_act = {};
>   	struct mlx5_flow_namespace *ns;
> +	struct mlx5_flow_spec *spec;
> +	void *headers_c;
> +	void *headers_v;
> +	u8 *dmac_c;
> +	u8 *dmac_v;
>   	int err;
>   
> -	/* for now, one entry, match all, forward to tir */
> -	ft_attr.max_fte = 1;
> -	ft_attr.autogroup.max_num_groups = 1;
> +	spec = kvzalloc(sizeof(*spec), GFP_KERNEL);
> +	if (!spec)
> +		return -ENOMEM;
> +
> +	spec->match_criteria_enable = MLX5_MATCH_OUTER_HEADERS;
> +	ft_attr.max_fte = 2;
> +	ft_attr.autogroup.max_num_groups = 2;
>   
> -	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev, MLX5_FLOW_NAMESPACE_BYPASS);
> +	ns = mlx5_get_flow_namespace(ndev->mvdev.mdev,
> +				     MLX5_FLOW_NAMESPACE_BYPASS);
>   	if (!ns) {
> -		mlx5_vdpa_warn(&ndev->mvdev, "get flow namespace\n");
> -		return -EOPNOTSUPP;
> +		mlx5_vdpa_warn(&ndev->mvdev, "failed to get flow namespace\n");
> +		err = -EOPNOTSUPP;
> +		goto err_ns;
>   	}
>   
>   	ndev->rxft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> -	if (IS_ERR(ndev->rxft))
> -		return PTR_ERR(ndev->rxft);
> +	if (IS_ERR(ndev->rxft)) {
> +		err = PTR_ERR(ndev->rxft);
> +		goto err_ns;
> +	}
>   
>   	ndev->rx_counter = mlx5_fc_create(ndev->mvdev.mdev, false);
>   	if (IS_ERR(ndev->rx_counter)) {
> @@ -1316,37 +1330,67 @@ static int add_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   		goto err_fc;
>   	}
>   
> -	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST | MLX5_FLOW_CONTEXT_ACTION_COUNT;
> +	headers_c = MLX5_ADDR_OF(fte_match_param, spec->match_criteria, outer_headers);
> +	dmac_c = MLX5_ADDR_OF(fte_match_param, headers_c, outer_headers.dmac_47_16);
> +	memset(dmac_c, 0xff, ETH_ALEN);
> +	headers_v = MLX5_ADDR_OF(fte_match_param, spec->match_value, outer_headers);
> +	dmac_v = MLX5_ADDR_OF(fte_match_param, headers_v, outer_headers.dmac_47_16);
> +	ether_addr_copy(dmac_v, ndev->config.mac);
> +
> +	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST |
> +			  MLX5_FLOW_CONTEXT_ACTION_COUNT;
>   	dest[0].type = MLX5_FLOW_DESTINATION_TYPE_TIR;
>   	dest[0].tir_num = ndev->res.tirn;
>   	dest[1].type = MLX5_FLOW_DESTINATION_TYPE_COUNTER;
>   	dest[1].counter_id = mlx5_fc_id(ndev->rx_counter);
> -	ndev->rx_rule = mlx5_add_flow_rules(ndev->rxft, NULL, &flow_act, dest, 2);
> -	if (IS_ERR(ndev->rx_rule)) {
> -		err = PTR_ERR(ndev->rx_rule);
> -		ndev->rx_rule = NULL;
> -		goto err_rule;
> +	ndev->rx_rule_ucast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act,
> +						  dest, 2);
> +
> +	if (IS_ERR(ndev->rx_rule_ucast)) {
> +		err = PTR_ERR(ndev->rx_rule_ucast);
> +		ndev->rx_rule_ucast = NULL;
> +		goto err_rule_ucast;
> +	}
> +
> +	memset(dmac_c, 0, ETH_ALEN);
> +	memset(dmac_v, 0, ETH_ALEN);
> +	dmac_c[0] = 1;
> +	dmac_v[0] = 1;
> +	flow_act.action = MLX5_FLOW_CONTEXT_ACTION_FWD_DEST;
> +	ndev->rx_rule_mcast = mlx5_add_flow_rules(ndev->rxft, spec, &flow_act,
> +						  dest, 1);
> +	if (IS_ERR(ndev->rx_rule_mcast)) {
> +		err = PTR_ERR(ndev->rx_rule_mcast);
> +		ndev->rx_rule_mcast = NULL;
> +		goto err_rule_mcast;
>   	}
>   
> +	kvfree(spec);
>   	return 0;
>   
> -err_rule:
> +err_rule_mcast:
> +	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> +	ndev->rx_rule_ucast = NULL;
> +err_rule_ucast:
>   	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
>   err_fc:
>   	mlx5_destroy_flow_table(ndev->rxft);
> +err_ns:
> +	kvfree(spec);
>   	return err;
>   }
>   
>   static void remove_fwd_to_tir(struct mlx5_vdpa_net *ndev)
>   {
> -	if (!ndev->rx_rule)
> +	if (!ndev->rx_rule_ucast)
>   		return;
>   
> -	mlx5_del_flow_rules(ndev->rx_rule);
> +	mlx5_del_flow_rules(ndev->rx_rule_mcast);
> +	ndev->rx_rule_mcast = NULL;
> +	mlx5_del_flow_rules(ndev->rx_rule_ucast);
> +	ndev->rx_rule_ucast = NULL;
>   	mlx5_fc_destroy(ndev->mvdev.mdev, ndev->rx_counter);
>   	mlx5_destroy_flow_table(ndev->rxft);
> -
> -	ndev->rx_rule = NULL;
>   }
>   
>   static void mlx5_vdpa_kick_vq(struct vdpa_device *vdev, u16 idx)

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH linux-next 0/9] vdpa: Enable user to set mac address,

[Michael S. Tsirkin]

On Wed, Feb 24, 2021 at 08:18:35AM +0200, Parav Pandit wrote:
> Currently user cannot set the mac address and mtu of the vdpa device.
> This patchset enables users to set the mac address and mtu of the vdpa
> device once the device is created.
> If a vendor driver supports such configuration user can set it otherwise
> user gets unsupported error.
> 
> vdpa mac address and mtu are device configuration layout fields.
> To keep interface generic enough for multiple types of vdpa devices, mac
> address and mtu setting is implemented as configuration layout config
> knobs.
> This enables to use similar config layout for other virtio devices.
> 
> An example of query & set of config layout fields for vdpa_sim_net
> driver:
> 
> Configuration layout fields are set after device is created.
> This enables user to change such fields at later point without destroying and
> recreating the device for new config.
> 
> $ vdpa mgmtdev show
> vdpasim_net:
>   supported_classes net
> 
> Add the device:
> $ vdpa dev add name bar mgmtdev vdpasim_net
> 
> Configure mac address and mtu:
> $ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000
> 
> In above command only mac address or only mtu can also be set.
> 
> View the config after setting:
> $ vdpa dev config show
> bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0
> 
> Patch summary:
> Patch-1 uses read only features bit to detect endianness
> Patch-2 implements new config layout query command
> Patch-3 implements callback for setting vdpa net config fields
> Patch-4 extends vdpa_sim_net driver to implement mac, mtu setting
> Patch-5 removed redundant get_config callback
> Patch-6 mlx5 vdpa driver migrates to user created vdpa device
> Patch-7 mlx5 vdpa driver uses random mac address when not configured
> Patch-8 mlx5 vdpa driver supports user provided mac config
> Patch-9 mlx5 vdpa driver uses user provided mac during rx flow steering

which tree is this for? does not seem to apply to linux-next branch of
vhost ...

> Eli Cohen (4):
>   vdpa/mlx5: Enable user to add/delete vdpa device
>   vdpa/mlx5: Provide device generated random MAC address
>   vdpa/mlx5: Support configuration of MAC
>   vdpa/mlx5: Forward only packets with allowed MAC address
> 
> Parav Pandit (5):
>   vdpa_sim: Consider read only supported features instead of current
>   vdpa: Introduce query of device config layout
>   vdpa: Enable user to set mac and mtu of vdpa device
>   vdpa_sim_net: Enable user to set mac address and mtu
>   vdpa_sim_net: Remove redundant get_config callback
> 
>  drivers/vdpa/mlx5/net/mlx5_vnet.c    | 185 ++++++++++++++----
>  drivers/vdpa/vdpa.c                  | 270 +++++++++++++++++++++++++++
>  drivers/vdpa/vdpa_sim/vdpa_sim.h     |   4 +-
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c |  49 +++--
>  include/linux/vdpa.h                 |  16 ++
>  include/uapi/linux/vdpa.h            |  12 ++
>  6 files changed, 476 insertions(+), 60 deletions(-)
> 
> -- 
> 2.26.2

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH linux-next 0/9] vdpa: Enable user to set mac address,

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Wednesday, February 24, 2021 12:21 PM
> 
> On Wed, Feb 24, 2021 at 08:18:35AM +0200, Parav Pandit wrote:
> > Currently user cannot set the mac address and mtu of the vdpa device.
> > This patchset enables users to set the mac address and mtu of the vdpa
[..]

> >
> > Patch summary:
> > Patch-1 uses read only features bit to detect endianness
> > Patch-2 implements new config layout query command
> > Patch-3 implements callback for setting vdpa net config fields
> > Patch-4 extends vdpa_sim_net driver to implement mac, mtu setting
> > Patch-5 removed redundant get_config callback
> > Patch-6 mlx5 vdpa driver migrates to user created vdpa device
> > Patch-7 mlx5 vdpa driver uses random mac address when not configured
> > Patch-8 mlx5 vdpa driver supports user provided mac config
> > Patch-9 mlx5 vdpa driver uses user provided mac during rx flow
> > steering
> 
> which tree is this for? does not seem to apply to linux-next branch of vhost ...

It is for linux-next branch of vhost. However I missed to rebase before sending.
My bad.
Preparing to send v2 after addressing other comments and after rebase.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization