[PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Cindy Lu]

Add support for setting the MAC address using the VDPA tool.
This feature will allow setting the MAC address using the VDPA tool.
For example, in vdpa_sim_net, the implementation sets the MAC address
to the config space. However, for other drivers, they can implement their
own function, not limited to the config space.

Changelog v2
 - Changed the function name to prevent misunderstanding
 - Added check for blk device
 - Addressed the comments
Changelog v3
 - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
 - Add a lock for the network device's dev_set_attr operation
 - Address the comments

Cindy Lu (2):
  vdpa: support set mac address from vdpa tool
  vdpa_sim_net: Add the support of set mac address

 drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
 include/linux/vdpa.h                 |  9 ++++
 include/uapi/linux/vdpa.h            |  1 +
 4 files changed, 109 insertions(+), 1 deletion(-)

-- 
2.45.0

[PATCH v3 1/2] vdpa: support set mac address from vdpa tool

[Cindy Lu]

Add new UAPI to support the mac address from vdpa tool
Function vdpa_nl_cmd_dev_attr_set_doit() will get the
new MAC address from the vdpa tool and then set it to the device.

The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**

Here is example:
root@L1# vdpa -jp dev config show vdpa0
{
    "config": {
        "vdpa0": {
            "mac": "82:4d:e9:5d:d7:e6",
            "link ": "up",
            "link_announce ": false,
            "mtu": 1500
        }
    }
}

root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55

root@L1# vdpa -jp dev config show vdpa0
{
    "config": {
        "vdpa0": {
            "mac": "00:11:22:33:44:55",
            "link ": "up",
            "link_announce ": false,
            "mtu": 1500
        }
    }
}

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa.c       | 81 +++++++++++++++++++++++++++++++++++++++
 include/linux/vdpa.h      |  9 +++++
 include/uapi/linux/vdpa.h |  1 +
 3 files changed, 91 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index a7612e0783b3..ca97ad43e1bd 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -1149,6 +1149,82 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
 	return err;
 }
 
+static int vdpa_dev_net_device_attr_set(struct vdpa_device *vdev,
+					struct genl_info *info)
+{
+	struct vdpa_dev_set_config set_config = {};
+	const u8 *macaddr;
+	struct vdpa_mgmt_dev *mdev = vdev->mdev;
+	struct nlattr **nl_attrs = info->attrs;
+	int err;
+
+	if (!vdev->mdev)
+		return -EINVAL;
+
+	down_write(&vdev->cf_lock);
+	if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
+	    nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
+		set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+		macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
+		memcpy(set_config.net.mac, macaddr, ETH_ALEN);
+
+		if (mdev->ops->dev_set_attr) {
+			err = mdev->ops->dev_set_attr(mdev, vdev, &set_config);
+		} else {
+			NL_SET_ERR_MSG_FMT_MOD(info->extack,
+					       "features 0x%llx not supported",
+					       BIT_ULL(VIRTIO_NET_F_MAC));
+			err = -EINVAL;
+		}
+	}
+	up_write(&vdev->cf_lock);
+
+	return err;
+}
+static int vdpa_nl_cmd_dev_attr_set_doit(struct sk_buff *skb,
+					 struct genl_info *info)
+{
+	const char *name;
+	int err = 0;
+	struct device *dev;
+	struct vdpa_device *vdev;
+	u64 classes;
+
+	if (!info->attrs[VDPA_ATTR_DEV_NAME])
+		return -EINVAL;
+
+	name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
+
+	down_write(&vdpa_dev_lock);
+	dev = bus_find_device(&vdpa_bus, NULL, name, 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,
+			"Fail to find the specified management device");
+		err = -EINVAL;
+		goto mdev_err;
+	}
+	classes = vdpa_mgmtdev_get_classes(vdev->mdev, NULL);
+	if (classes & BIT_ULL(VIRTIO_ID_NET)) {
+		err = vdpa_dev_net_device_attr_set(vdev, info);
+	} else {
+		NL_SET_ERR_MSG_FMT_MOD(info->extack, "%s device not supported",
+				       name);
+	}
+
+mdev_err:
+	put_device(dev);
+dev_err:
+	up_write(&vdpa_dev_lock);
+	return err;
+}
+
 static int vdpa_dev_config_dump(struct device *dev, void *data)
 {
 	struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev);
@@ -1285,6 +1361,11 @@ static const struct genl_ops vdpa_nl_ops[] = {
 		.doit = vdpa_nl_cmd_dev_stats_get_doit,
 		.flags = GENL_ADMIN_PERM,
 	},
+	{
+		.cmd = VDPA_CMD_DEV_ATTR_SET,
+		.doit = vdpa_nl_cmd_dev_attr_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 db15ac07f8a6..14afa6d59098 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -576,11 +576,20 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status);
  *	     @dev: vdpa device to remove
  *	     Driver need to remove the specified device by calling
  *	     _vdpa_unregister_device().
+  * @dev_set_attr: change a vdpa device's attr after it was create
+ *	     @mdev: parent device to use for device
+ *	     @dev: vdpa device structure
+ *	     @config:Attributes to be set for the device.
+ *	     The driver needs to check the mask of the structure and then set
+ *	     the related information to the vdpa device. The driver must return 0
+ *	     if set successfully.
  */
 struct vdpa_mgmtdev_ops {
 	int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name,
 		       const struct vdpa_dev_set_config *config);
 	void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev);
+	int (*dev_set_attr)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
+			    const struct vdpa_dev_set_config *config);
 };
 
 /**
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 54b649ab0f22..76b3a1fd13f3 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -19,6 +19,7 @@ enum vdpa_command {
 	VDPA_CMD_DEV_GET,		/* can dump */
 	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
 	VDPA_CMD_DEV_VSTATS_GET,
+	VDPA_CMD_DEV_ATTR_SET,
 };
 
 enum vdpa_attr {
-- 
2.45.0

Re: [PATCH v3 1/2] vdpa: support set mac address from vdpa tool

[Jason Wang]

On Mon, Jul 8, 2024 at 2:48 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add new UAPI to support the mac address from vdpa tool
> Function vdpa_nl_cmd_dev_attr_set_doit() will get the
> new MAC address from the vdpa tool and then set it to the device.
>
> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>
> Here is example:
> root@L1# vdpa -jp dev config show vdpa0
> {
>     "config": {
>         "vdpa0": {
>             "mac": "82:4d:e9:5d:d7:e6",
>             "link ": "up",
>             "link_announce ": false,
>             "mtu": 1500
>         }
>     }
> }
>
> root@L1# vdpa dev set name vdpa0 mac 00:11:22:33:44:55
>
> root@L1# vdpa -jp dev config show vdpa0
> {
>     "config": {
>         "vdpa0": {
>             "mac": "00:11:22:33:44:55",
>             "link ": "up",
>             "link_announce ": false,
>             "mtu": 1500
>         }
>     }
> }
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>

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

Thanks

Re: [PATCH v3 1/2] vdpa: support set mac address from vdpa tool

[kernel test robot]

Hi Cindy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on horms-ipvs/master v6.10-rc7 next-20240703]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cindy-Lu/vdpa-support-set-mac-address-from-vdpa-tool/20240708-144942
base:   linus/master
patch link:    https://lore.kernel.org/r/20240708064820.88955-2-lulu%40redhat.com
patch subject: [PATCH v3 1/2] vdpa: support set mac address from vdpa tool
config: i386-buildonly-randconfig-005-20240708 (https://download.01.org/0day-ci/archive/20240708/202407081733.FCiMubD8-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240708/202407081733.FCiMubD8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407081733.FCiMubD8-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/vdpa/vdpa.c:1377:6: warning: variable 'err' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
    1377 |         if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1378 |             nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/vdpa/vdpa.c:1394:9: note: uninitialized use occurs here
    1394 |         return err;
         |                ^~~
   drivers/vdpa/vdpa.c:1377:2: note: remove the 'if' if its condition is always true
    1377 |         if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    1378 |             nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
         |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/vdpa/vdpa.c:1377:6: warning: variable 'err' is used uninitialized whenever '&&' condition is false [-Wsometimes-uninitialized]
    1377 |         if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/vdpa/vdpa.c:1394:9: note: uninitialized use occurs here
    1394 |         return err;
         |                ^~~
   drivers/vdpa/vdpa.c:1377:6: note: remove the '&&' if its condition is always true
    1377 |         if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
         |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/vdpa/vdpa.c:1371:9: note: initialize the variable 'err' to silence this warning
    1371 |         int err;
         |                ^
         |                 = 0
   2 warnings generated.


vim +1377 drivers/vdpa/vdpa.c

  1363	
  1364	static int vdpa_dev_net_device_attr_set(struct vdpa_device *vdev,
  1365						struct genl_info *info)
  1366	{
  1367		struct vdpa_dev_set_config set_config = {};
  1368		const u8 *macaddr;
  1369		struct vdpa_mgmt_dev *mdev = vdev->mdev;
  1370		struct nlattr **nl_attrs = info->attrs;
  1371		int err;
  1372	
  1373		if (!vdev->mdev)
  1374			return -EINVAL;
  1375	
  1376		down_write(&vdev->cf_lock);
> 1377		if ((mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC)) &&
  1378		    nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
  1379			set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
  1380			macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
  1381			memcpy(set_config.net.mac, macaddr, ETH_ALEN);
  1382	
  1383			if (mdev->ops->dev_set_attr) {
  1384				err = mdev->ops->dev_set_attr(mdev, vdev, &set_config);
  1385			} else {
  1386				NL_SET_ERR_MSG_FMT_MOD(info->extack,
  1387						       "features 0x%llx not supported",
  1388						       BIT_ULL(VIRTIO_NET_F_MAC));
  1389				err = -EINVAL;
  1390			}
  1391		}
  1392		up_write(&vdev->cf_lock);
  1393	
  1394		return err;
  1395	}
  1396	static int vdpa_nl_cmd_dev_attr_set_doit(struct sk_buff *skb,
  1397						 struct genl_info *info)
  1398	{
  1399		const char *name;
  1400		int err = 0;
  1401		struct device *dev;
  1402		struct vdpa_device *vdev;
  1403		u64 classes;
  1404	
  1405		if (!info->attrs[VDPA_ATTR_DEV_NAME])
  1406			return -EINVAL;
  1407	
  1408		name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
  1409	
  1410		down_write(&vdpa_dev_lock);
  1411		dev = bus_find_device(&vdpa_bus, NULL, name, vdpa_name_match);
  1412		if (!dev) {
  1413			NL_SET_ERR_MSG_MOD(info->extack, "device not found");
  1414			err = -ENODEV;
  1415			goto dev_err;
  1416		}
  1417		vdev = container_of(dev, struct vdpa_device, dev);
  1418		if (!vdev->mdev) {
  1419			NL_SET_ERR_MSG_MOD(
  1420				info->extack,
  1421				"Fail to find the specified management device");
  1422			err = -EINVAL;
  1423			goto mdev_err;
  1424		}
  1425		classes = vdpa_mgmtdev_get_classes(vdev->mdev, NULL);
  1426		if (classes & BIT_ULL(VIRTIO_ID_NET)) {
  1427			err = vdpa_dev_net_device_attr_set(vdev, info);
  1428		} else {
  1429			NL_SET_ERR_MSG_FMT_MOD(info->extack, "%s device not supported",
  1430					       name);
  1431		}
  1432	
  1433	mdev_err:
  1434		put_device(dev);
  1435	dev_err:
  1436		up_write(&vdpa_dev_lock);
  1437		return err;
  1438	}
  1439	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v3 2/2] vdpa_sim_net: Add the support of set mac address

[Cindy Lu]

Add the function to support setting the MAC address.
For vdpa_sim_net, the driver will write the MAC address
to the config space, and other devices can implement
their own functions to support this.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
index cfe962911804..a472c3c43bfd 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -414,6 +414,22 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
 	net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
 }
 
+static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev,
+				struct vdpa_device *dev,
+				const struct vdpa_dev_set_config *config)
+{
+	struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
+
+	struct virtio_net_config *vio_config = vdpasim->config;
+	if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
+		if (!is_zero_ether_addr(config->net.mac)) {
+			memcpy(vio_config->mac, config->net.mac, ETH_ALEN);
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+
 static void vdpasim_net_setup_config(struct vdpasim *vdpasim,
 				     const struct vdpa_dev_set_config *config)
 {
@@ -510,7 +526,8 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
 
 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_set_attr = vdpasim_net_set_attr
 };
 
 static struct virtio_device_id id_table[] = {
-- 
2.45.0

Re: [PATCH v3 2/2] vdpa_sim_net: Add the support of set mac address

[Jason Wang]

On Mon, Jul 8, 2024 at 2:48 PM Cindy Lu <lulu@redhat.com> wrote:
>
> Add the function to support setting the MAC address.
> For vdpa_sim_net, the driver will write the MAC address
> to the config space, and other devices can implement
> their own functions to support this.
>
> Signed-off-by: Cindy Lu <lulu@redhat.com>
> ---
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> index cfe962911804..a472c3c43bfd 100644
> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> @@ -414,6 +414,22 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
>         net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
>  }
>
> +static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev,
> +                               struct vdpa_device *dev,
> +                               const struct vdpa_dev_set_config *config)
> +{
> +       struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
> +
> +       struct virtio_net_config *vio_config = vdpasim->config;
> +       if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> +               if (!is_zero_ether_addr(config->net.mac)) {
> +                       memcpy(vio_config->mac, config->net.mac, ETH_ALEN);
> +                       return 0;
> +               }
> +       }
> +       return -EINVAL;

I think in the previous version, we agreed to have a lock to
synchronize the writing here?

Thanks

> +}
> +
>  static void vdpasim_net_setup_config(struct vdpasim *vdpasim,
>                                      const struct vdpa_dev_set_config *config)
>  {
> @@ -510,7 +526,8 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
>
>  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_set_attr = vdpasim_net_set_attr
>  };
>
>  static struct virtio_device_id id_table[] = {
> --
> 2.45.0
>

Re: [PATCH v3 2/2] vdpa_sim_net: Add the support of set mac address

[Cindy Lu]

On Mon, 8 Jul 2024 at 15:06, Jason Wang <jasowang@redhat.com> wrote:
>
> On Mon, Jul 8, 2024 at 2:48 PM Cindy Lu <lulu@redhat.com> wrote:
> >
> > Add the function to support setting the MAC address.
> > For vdpa_sim_net, the driver will write the MAC address
> > to the config space, and other devices can implement
> > their own functions to support this.
> >
> > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > ---
> >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > index cfe962911804..a472c3c43bfd 100644
> > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > @@ -414,6 +414,22 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
> >         net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> >  }
> >
> > +static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev,
> > +                               struct vdpa_device *dev,
> > +                               const struct vdpa_dev_set_config *config)
> > +{
> > +       struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
> > +
> > +       struct virtio_net_config *vio_config = vdpasim->config;
> > +       if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> > +               if (!is_zero_ether_addr(config->net.mac)) {
> > +                       memcpy(vio_config->mac, config->net.mac, ETH_ALEN);
> > +                       return 0;
> > +               }
> > +       }
> > +       return -EINVAL;
>
> I think in the previous version, we agreed to have a lock to
> synchronize the writing here?
>
> Thanks
>
Hi Jason
I have moved the down_write(&vdev->cf_lock) and
up_write(&vdev->cf_lock) to the function vdpa_dev_net_device_attr_set
in vdpa/vdpa.c. Then the device itself doesn't need to call it again.
Do you think this is ok?
Thanks
Cindy
> > +}
> > +
> >  static void vdpasim_net_setup_config(struct vdpasim *vdpasim,
> >                                      const struct vdpa_dev_set_config *config)
> >  {
> > @@ -510,7 +526,8 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
> >
> >  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_set_attr = vdpasim_net_set_attr
> >  };
> >
> >  static struct virtio_device_id id_table[] = {
> > --
> > 2.45.0
> >
>

Re: [PATCH v3 2/2] vdpa_sim_net: Add the support of set mac address

[Jason Wang]

On Mon, Jul 8, 2024 at 3:19 PM Cindy Lu <lulu@redhat.com> wrote:
>
> On Mon, 8 Jul 2024 at 15:06, Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Mon, Jul 8, 2024 at 2:48 PM Cindy Lu <lulu@redhat.com> wrote:
> > >
> > > Add the function to support setting the MAC address.
> > > For vdpa_sim_net, the driver will write the MAC address
> > > to the config space, and other devices can implement
> > > their own functions to support this.
> > >
> > > Signed-off-by: Cindy Lu <lulu@redhat.com>
> > > ---
> > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++++++++++++++-
> > >  1 file changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > > index cfe962911804..a472c3c43bfd 100644
> > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
> > > @@ -414,6 +414,22 @@ static void vdpasim_net_get_config(struct vdpasim *vdpasim, void *config)
> > >         net_config->status = cpu_to_vdpasim16(vdpasim, VIRTIO_NET_S_LINK_UP);
> > >  }
> > >
> > > +static int vdpasim_net_set_attr(struct vdpa_mgmt_dev *mdev,
> > > +                               struct vdpa_device *dev,
> > > +                               const struct vdpa_dev_set_config *config)
> > > +{
> > > +       struct vdpasim *vdpasim = container_of(dev, struct vdpasim, vdpa);
> > > +
> > > +       struct virtio_net_config *vio_config = vdpasim->config;
> > > +       if (config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> > > +               if (!is_zero_ether_addr(config->net.mac)) {
> > > +                       memcpy(vio_config->mac, config->net.mac, ETH_ALEN);
> > > +                       return 0;
> > > +               }
> > > +       }
> > > +       return -EINVAL;
> >
> > I think in the previous version, we agreed to have a lock to
> > synchronize the writing here?
> >
> > Thanks
> >
> Hi Jason
> I have moved the down_write(&vdev->cf_lock) and
> up_write(&vdev->cf_lock) to the function vdpa_dev_net_device_attr_set
> in vdpa/vdpa.c. Then the device itself doesn't need to call it again.
> Do you think this is ok?

I meant we have another path to modify the mac:

static virtio_net_ctrl_ack vdpasim_handle_ctrl_mac(struct vdpasim *vdpasim,
                                                   u8 cmd)
{
        struct virtio_net_config *vio_config = vdpasim->config;
        struct vdpasim_virtqueue *cvq = &vdpasim->vqs[2];
        virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
        size_t read;

        switch (cmd) {
case VIRTIO_NET_CTRL_MAC_ADDR_SET:
                read = vringh_iov_pull_iotlb(&cvq->vring, &cvq->in_iov,
                                             vio_config->mac, ETH_ALEN);
                if (read == ETH_ALEN)
            status = VIRTIO_NET_OK;
        break;
        default:
                break;
        }

        return status;
}

We need to serialize between them.

Thanks

> Thanks
> Cindy
> > > +}
> > > +
> > >  static void vdpasim_net_setup_config(struct vdpasim *vdpasim,
> > >                                      const struct vdpa_dev_set_config *config)
> > >  {
> > > @@ -510,7 +526,8 @@ static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev,
> > >
> > >  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_set_attr = vdpasim_net_set_attr
> > >  };
> > >
> > >  static struct virtio_device_id id_table[] = {
> > > --
> > > 2.45.0
> > >
> >
>

RE: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Parav Pandit]

Hi Cindy,

> From: Cindy Lu <lulu@redhat.com>
> Sent: Monday, July 8, 2024 12:17 PM
> 
> Add support for setting the MAC address using the VDPA tool.
> This feature will allow setting the MAC address using the VDPA tool.
> For example, in vdpa_sim_net, the implementation sets the MAC address to
> the config space. However, for other drivers, they can implement their own
> function, not limited to the config space.
> 
> Changelog v2
>  - Changed the function name to prevent misunderstanding
>  - Added check for blk device
>  - Addressed the comments
> Changelog v3
>  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
>  - Add a lock for the network device's dev_set_attr operation
>  - Address the comments
> 
> Cindy Lu (2):
>   vdpa: support set mac address from vdpa tool
>   vdpa_sim_net: Add the support of set mac address
> 
>  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
>  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
>  include/linux/vdpa.h                 |  9 ++++
>  include/uapi/linux/vdpa.h            |  1 +
>  4 files changed, 109 insertions(+), 1 deletion(-)
> 
> --
> 2.45.0

Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.

The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.

vdpa_sim_net can also be extended for similar way when adding the vdpa device.

Have you considered using the existing tool and kernel in place since 2021?
Such as commit d8ca2fa5be1.

An example of it is, 
$ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Cindy Lu]

On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
>
> Hi Cindy,
>
> > From: Cindy Lu <lulu@redhat.com>
> > Sent: Monday, July 8, 2024 12:17 PM
> >
> > Add support for setting the MAC address using the VDPA tool.
> > This feature will allow setting the MAC address using the VDPA tool.
> > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > the config space. However, for other drivers, they can implement their own
> > function, not limited to the config space.
> >
> > Changelog v2
> >  - Changed the function name to prevent misunderstanding
> >  - Added check for blk device
> >  - Addressed the comments
> > Changelog v3
> >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> >  - Add a lock for the network device's dev_set_attr operation
> >  - Address the comments
> >
> > Cindy Lu (2):
> >   vdpa: support set mac address from vdpa tool
> >   vdpa_sim_net: Add the support of set mac address
> >
> >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> >  include/linux/vdpa.h                 |  9 ++++
> >  include/uapi/linux/vdpa.h            |  1 +
> >  4 files changed, 109 insertions(+), 1 deletion(-)
> >
> > --
> > 2.45.0
>
> Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
>
> The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
>
> vdpa_sim_net can also be extended for similar way when adding the vdpa device.
>
> Have you considered using the existing tool and kernel in place since 2021?
> Such as commit d8ca2fa5be1.
>
> An example of it is,
> $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
>
Hi Parav
Really thanks for your comments. The reason for adding this function
is to support Kubevirt.
the problem we meet is that kubevirt chooses one random vdpa device
from the pool and we don't know which one it going to pick. That means
we can't get to know the Mac address before it is created. So we plan
to have this function to change the mac address after it is created
Thanks
cindy

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Michael S. Tsirkin]

On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> >
> > Hi Cindy,
> >
> > > From: Cindy Lu <lulu@redhat.com>
> > > Sent: Monday, July 8, 2024 12:17 PM
> > >
> > > Add support for setting the MAC address using the VDPA tool.
> > > This feature will allow setting the MAC address using the VDPA tool.
> > > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > > the config space. However, for other drivers, they can implement their own
> > > function, not limited to the config space.
> > >
> > > Changelog v2
> > >  - Changed the function name to prevent misunderstanding
> > >  - Added check for blk device
> > >  - Addressed the comments
> > > Changelog v3
> > >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> > >  - Add a lock for the network device's dev_set_attr operation
> > >  - Address the comments
> > >
> > > Cindy Lu (2):
> > >   vdpa: support set mac address from vdpa tool
> > >   vdpa_sim_net: Add the support of set mac address
> > >
> > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > >  include/linux/vdpa.h                 |  9 ++++
> > >  include/uapi/linux/vdpa.h            |  1 +
> > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > >
> > > --
> > > 2.45.0
> >
> > Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> > Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> > So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
> >
> > The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
> >
> > vdpa_sim_net can also be extended for similar way when adding the vdpa device.
> >
> > Have you considered using the existing tool and kernel in place since 2021?
> > Such as commit d8ca2fa5be1.
> >
> > An example of it is,
> > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
> >
> Hi Parav
> Really thanks for your comments. The reason for adding this function
> is to support Kubevirt.
> the problem we meet is that kubevirt chooses one random vdpa device
> from the pool and we don't know which one it going to pick. That means
> we can't get to know the Mac address before it is created. So we plan
> to have this function to change the mac address after it is created
> Thanks
> cindy

Well you will need to change kubevirt to teach it to set
mac address, right?

-- 
MST

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Jason Wang]

On Tue, Jul 9, 2024 at 8:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> > On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> > >
> > > Hi Cindy,
> > >
> > > > From: Cindy Lu <lulu@redhat.com>
> > > > Sent: Monday, July 8, 2024 12:17 PM
> > > >
> > > > Add support for setting the MAC address using the VDPA tool.
> > > > This feature will allow setting the MAC address using the VDPA tool.
> > > > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > > > the config space. However, for other drivers, they can implement their own
> > > > function, not limited to the config space.
> > > >
> > > > Changelog v2
> > > >  - Changed the function name to prevent misunderstanding
> > > >  - Added check for blk device
> > > >  - Addressed the comments
> > > > Changelog v3
> > > >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> > > >  - Add a lock for the network device's dev_set_attr operation
> > > >  - Address the comments
> > > >
> > > > Cindy Lu (2):
> > > >   vdpa: support set mac address from vdpa tool
> > > >   vdpa_sim_net: Add the support of set mac address
> > > >
> > > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > > >  include/linux/vdpa.h                 |  9 ++++
> > > >  include/uapi/linux/vdpa.h            |  1 +
> > > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > > >
> > > > --
> > > > 2.45.0
> > >
> > > Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> > > Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> > > So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
> > >
> > > The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
> > >
> > > vdpa_sim_net can also be extended for similar way when adding the vdpa device.
> > >
> > > Have you considered using the existing tool and kernel in place since 2021?
> > > Such as commit d8ca2fa5be1.
> > >
> > > An example of it is,
> > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
> > >
> > Hi Parav
> > Really thanks for your comments. The reason for adding this function
> > is to support Kubevirt.
> > the problem we meet is that kubevirt chooses one random vdpa device
> > from the pool and we don't know which one it going to pick. That means
> > we can't get to know the Mac address before it is created. So we plan
> > to have this function to change the mac address after it is created
> > Thanks
> > cindy
>
> Well you will need to change kubevirt to teach it to set
> mac address, right?

That's the plan. Adding Leonardo.

Thanks

>
> --
> MST
>

RE: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Parav Pandit]

Hi Cindy,

> From: Jason Wang <jasowang@redhat.com>
> Sent: Wednesday, July 10, 2024 8:36 AM
> 
> On Tue, Jul 9, 2024 at 8:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> > > On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > Hi Cindy,
> > > >
> > > > > From: Cindy Lu <lulu@redhat.com>
> > > > > Sent: Monday, July 8, 2024 12:17 PM
> > > > >
> > > > > Add support for setting the MAC address using the VDPA tool.
> > > > > This feature will allow setting the MAC address using the VDPA tool.
> > > > > For example, in vdpa_sim_net, the implementation sets the MAC
> > > > > address to the config space. However, for other drivers, they
> > > > > can implement their own function, not limited to the config space.
> > > > >
> > > > > Changelog v2
> > > > >  - Changed the function name to prevent misunderstanding
> > > > >  - Added check for blk device
> > > > >  - Addressed the comments
> > > > > Changelog v3
> > > > >  - Split the function of the net device from
> > > > > vdpa_nl_cmd_dev_attr_set_doit
> > > > >  - Add a lock for the network device's dev_set_attr operation
> > > > >  - Address the comments
> > > > >
> > > > > Cindy Lu (2):
> > > > >   vdpa: support set mac address from vdpa tool
> > > > >   vdpa_sim_net: Add the support of set mac address
> > > > >
> > > > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > > > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > > > >  include/linux/vdpa.h                 |  9 ++++
> > > > >  include/uapi/linux/vdpa.h            |  1 +
> > > > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > > > >
> > > > > --
> > > > > 2.45.0
> > > >
> > > > Mlx5 device already allows setting the mac and mtu during the vdpa
> device creation time.
> > > > Once the vdpa device is created, it binds to vdpa bus and other driver
> vhost_vdpa etc bind to it.
> > > > So there was no good reason in the past to support explicit config after
> device add complicate the flow for synchronizing this.
> > > >
> > > > The user who wants a device with new attributes, as well destroy and
> recreate the vdpa device with new desired attributes.
> > > >
> > > > vdpa_sim_net can also be extended for similar way when adding the
> vdpa device.
> > > >
> > > > Have you considered using the existing tool and kernel in place since
> 2021?
> > > > Such as commit d8ca2fa5be1.
> > > >
> > > > An example of it is,
> > > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55
> > > > mtu 9000
> > > >
> > > Hi Parav
> > > Really thanks for your comments. The reason for adding this function
> > > is to support Kubevirt.
> > > the problem we meet is that kubevirt chooses one random vdpa device
> > > from the pool and we don't know which one it going to pick. That
> > > means we can't get to know the Mac address before it is created. So
> > > we plan to have this function to change the mac address after it is
> > > created Thanks cindy
> >
> > Well you will need to change kubevirt to teach it to set mac address,
> > right?
> 
> That's the plan. Adding Leonardo.

Any specific reason to pre-create those large number of vdpa devices of the pool?
I was hoping to create vdpa device with needed attributes, when spawning a kubevirt instance.
K8s DRA infrastructure [1] can be used to create the needed vdpa device. Have you considered using the DRA of [1]?

[1] https://kubernetes.io/docs/concepts/scheduling-eviction/dynamic-resource-allocation/

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Michael S. Tsirkin]

On Wed, Jul 10, 2024 at 11:05:48AM +0800, Jason Wang wrote:
> On Tue, Jul 9, 2024 at 8:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> > > On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> > > >
> > > > Hi Cindy,
> > > >
> > > > > From: Cindy Lu <lulu@redhat.com>
> > > > > Sent: Monday, July 8, 2024 12:17 PM
> > > > >
> > > > > Add support for setting the MAC address using the VDPA tool.
> > > > > This feature will allow setting the MAC address using the VDPA tool.
> > > > > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > > > > the config space. However, for other drivers, they can implement their own
> > > > > function, not limited to the config space.
> > > > >
> > > > > Changelog v2
> > > > >  - Changed the function name to prevent misunderstanding
> > > > >  - Added check for blk device
> > > > >  - Addressed the comments
> > > > > Changelog v3
> > > > >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> > > > >  - Add a lock for the network device's dev_set_attr operation
> > > > >  - Address the comments
> > > > >
> > > > > Cindy Lu (2):
> > > > >   vdpa: support set mac address from vdpa tool
> > > > >   vdpa_sim_net: Add the support of set mac address
> > > > >
> > > > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > > > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > > > >  include/linux/vdpa.h                 |  9 ++++
> > > > >  include/uapi/linux/vdpa.h            |  1 +
> > > > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > > > >
> > > > > --
> > > > > 2.45.0
> > > >
> > > > Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> > > > Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> > > > So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
> > > >
> > > > The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
> > > >
> > > > vdpa_sim_net can also be extended for similar way when adding the vdpa device.
> > > >
> > > > Have you considered using the existing tool and kernel in place since 2021?
> > > > Such as commit d8ca2fa5be1.
> > > >
> > > > An example of it is,
> > > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
> > > >
> > > Hi Parav
> > > Really thanks for your comments. The reason for adding this function
> > > is to support Kubevirt.
> > > the problem we meet is that kubevirt chooses one random vdpa device
> > > from the pool and we don't know which one it going to pick. That means
> > > we can't get to know the Mac address before it is created. So we plan
> > > to have this function to change the mac address after it is created
> > > Thanks
> > > cindy
> >
> > Well you will need to change kubevirt to teach it to set
> > mac address, right?
> 
> That's the plan. Adding Leonardo.
> 
> Thanks

So given you are going to change kubevirt, can we
change it to create devices as needed with the
existing API?

> >
> > --
> > MST
> >

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Cindy Lu]

On Wed, 10 Jul 2024 at 14:10, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 10, 2024 at 11:05:48AM +0800, Jason Wang wrote:
> > On Tue, Jul 9, 2024 at 8:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> > > > On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> > > > >
> > > > > Hi Cindy,
> > > > >
> > > > > > From: Cindy Lu <lulu@redhat.com>
> > > > > > Sent: Monday, July 8, 2024 12:17 PM
> > > > > >
> > > > > > Add support for setting the MAC address using the VDPA tool.
> > > > > > This feature will allow setting the MAC address using the VDPA tool.
> > > > > > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > > > > > the config space. However, for other drivers, they can implement their own
> > > > > > function, not limited to the config space.
> > > > > >
> > > > > > Changelog v2
> > > > > >  - Changed the function name to prevent misunderstanding
> > > > > >  - Added check for blk device
> > > > > >  - Addressed the comments
> > > > > > Changelog v3
> > > > > >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> > > > > >  - Add a lock for the network device's dev_set_attr operation
> > > > > >  - Address the comments
> > > > > >
> > > > > > Cindy Lu (2):
> > > > > >   vdpa: support set mac address from vdpa tool
> > > > > >   vdpa_sim_net: Add the support of set mac address
> > > > > >
> > > > > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > > > > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > > > > >  include/linux/vdpa.h                 |  9 ++++
> > > > > >  include/uapi/linux/vdpa.h            |  1 +
> > > > > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > --
> > > > > > 2.45.0
> > > > >
> > > > > Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> > > > > Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> > > > > So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
> > > > >
> > > > > The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
> > > > >
> > > > > vdpa_sim_net can also be extended for similar way when adding the vdpa device.
> > > > >
> > > > > Have you considered using the existing tool and kernel in place since 2021?
> > > > > Such as commit d8ca2fa5be1.
> > > > >
> > > > > An example of it is,
> > > > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
> > > > >
> > > > Hi Parav
> > > > Really thanks for your comments. The reason for adding this function
> > > > is to support Kubevirt.
> > > > the problem we meet is that kubevirt chooses one random vdpa device
> > > > from the pool and we don't know which one it going to pick. That means
> > > > we can't get to know the Mac address before it is created. So we plan
> > > > to have this function to change the mac address after it is created
> > > > Thanks
> > > > cindy
> > >
> > > Well you will need to change kubevirt to teach it to set
> > > mac address, right?
> >
> > That's the plan. Adding Leonardo.
> >
> > Thanks
>
> So given you are going to change kubevirt, can we
> change it to create devices as needed with the
> existing API?
>
Hi Micheal and Parav,
I'm really not familiar with kubevirt, hope Leonardo can help answer
these questions
Hi @Leonardo Milleri
would you help answer these questions?

Thanks
Cindy
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH v3 0/2] vdpa: support set mac address from vdpa tool

[Leonardo Milleri]

Sorry for the noise, resending the email in text format

Hi All,

My answers inline below

>> Any specific reason to pre-create those large number of vdpa devices of the pool?
>> I was hoping to create vdpa device with needed attributes, when spawning a kubevirt instance.
>> K8s DRA infrastructure [1] can be used to create the needed vdpa device. Have you considered using the DRA of [1]?

The vhost-vdpa devices are created in the host before spawning the
kubevirt VM. This is achieved by using:
- sriov-network-operator: load kernel drivers, create vdpa devices
(with MAC address), etc
- sriov-device-plugin: create pool of resources (vdpa devices in this
case), advertise devices to k8s, allocate devices during pod creation
(by the way, isn't this mechanism very similar to DRA?)

Then we create the kubevirt VM by defining an interface with the
following attributes:
- type:vdpa
- mac
- source: vhost-vdpa path

So the issue is, how to make sure the mac in the VM is the same mac of vdpa?
Two options:
- ensure kubevirt interface mac is equal to vdpa mac: this is not
possible because of the device plugin resource pool. You can have a
few devices in the pool and the device plugin picks one randomly.
- change vdpa mac address at a later stage, to make sure it is aligned
with kubevirt interface mac. I don't know if there is already specific
code in kubevirt to do that or need to be implemented.

Hope this helps to clarify

Thanks
Leonardo


On Wed, Jul 10, 2024 at 10:46 AM Cindy Lu <lulu@redhat.com> wrote:
>
> On Wed, 10 Jul 2024 at 14:10, Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Jul 10, 2024 at 11:05:48AM +0800, Jason Wang wrote:
> > > On Tue, Jul 9, 2024 at 8:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Tue, Jul 09, 2024 at 02:19:19PM +0800, Cindy Lu wrote:
> > > > > On Tue, 9 Jul 2024 at 11:59, Parav Pandit <parav@nvidia.com> wrote:
> > > > > >
> > > > > > Hi Cindy,
> > > > > >
> > > > > > > From: Cindy Lu <lulu@redhat.com>
> > > > > > > Sent: Monday, July 8, 2024 12:17 PM
> > > > > > >
> > > > > > > Add support for setting the MAC address using the VDPA tool.
> > > > > > > This feature will allow setting the MAC address using the VDPA tool.
> > > > > > > For example, in vdpa_sim_net, the implementation sets the MAC address to
> > > > > > > the config space. However, for other drivers, they can implement their own
> > > > > > > function, not limited to the config space.
> > > > > > >
> > > > > > > Changelog v2
> > > > > > >  - Changed the function name to prevent misunderstanding
> > > > > > >  - Added check for blk device
> > > > > > >  - Addressed the comments
> > > > > > > Changelog v3
> > > > > > >  - Split the function of the net device from vdpa_nl_cmd_dev_attr_set_doit
> > > > > > >  - Add a lock for the network device's dev_set_attr operation
> > > > > > >  - Address the comments
> > > > > > >
> > > > > > > Cindy Lu (2):
> > > > > > >   vdpa: support set mac address from vdpa tool
> > > > > > >   vdpa_sim_net: Add the support of set mac address
> > > > > > >
> > > > > > >  drivers/vdpa/vdpa.c                  | 81 ++++++++++++++++++++++++++++
> > > > > > >  drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 19 ++++++-
> > > > > > >  include/linux/vdpa.h                 |  9 ++++
> > > > > > >  include/uapi/linux/vdpa.h            |  1 +
> > > > > > >  4 files changed, 109 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > --
> > > > > > > 2.45.0
> > > > > >
> > > > > > Mlx5 device already allows setting the mac and mtu during the vdpa device creation time.
> > > > > > Once the vdpa device is created, it binds to vdpa bus and other driver vhost_vdpa etc bind to it.
> > > > > > So there was no good reason in the past to support explicit config after device add complicate the flow for synchronizing this.
> > > > > >
> > > > > > The user who wants a device with new attributes, as well destroy and recreate the vdpa device with new desired attributes.
> > > > > >
> > > > > > vdpa_sim_net can also be extended for similar way when adding the vdpa device.
> > > > > >
> > > > > > Have you considered using the existing tool and kernel in place since 2021?
> > > > > > Such as commit d8ca2fa5be1.
> > > > > >
> > > > > > An example of it is,
> > > > > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000
> > > > > >
> > > > > Hi Parav
> > > > > Really thanks for your comments. The reason for adding this function
> > > > > is to support Kubevirt.
> > > > > the problem we meet is that kubevirt chooses one random vdpa device
> > > > > from the pool and we don't know which one it going to pick. That means
> > > > > we can't get to know the Mac address before it is created. So we plan
> > > > > to have this function to change the mac address after it is created
> > > > > Thanks
> > > > > cindy
> > > >
> > > > Well you will need to change kubevirt to teach it to set
> > > > mac address, right?
> > >
> > > That's the plan. Adding Leonardo.
> > >
> > > Thanks
> >
> > So given you are going to change kubevirt, can we
> > change it to create devices as needed with the
> > existing API?
> >
> Hi Micheal and Parav,
> I'm really not familiar with kubevirt, hope Leonardo can help answer
> these questions
> Hi @Leonardo Milleri
> would you help answer these questions?
>
> Thanks
> Cindy
> > > >
> > > > --
> > > > MST
> > > >
> >
>