Add new UAPI to support the mac address from vdpa tool
Function vdpa_nl_cmd_dev_config_set_doit() will get the
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 sample:
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 <[email protected]>
---
drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++
include/linux/vdpa.h | 2 ++
include/uapi/linux/vdpa.h | 1 +
3 files changed, 74 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index a7612e0783b3..347ae6e7749d 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
return err;
}
+static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
+ struct genl_info *info)
+{
+ struct vdpa_dev_set_config set_config = {};
+ struct nlattr **nl_attrs = info->attrs;
+ struct vdpa_mgmt_dev *mdev;
+ const u8 *macaddr;
+ const char *name;
+ int err = 0;
+ struct device *dev;
+ struct vdpa_device *vdev;
+
+ 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;
+ }
+ mdev = vdev->mdev;
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
+ if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
+ NL_SET_ERR_MSG_FMT_MOD(
+ info->extack,
+ "Missing features 0x%llx for provided attributes",
+ BIT_ULL(VIRTIO_NET_F_MAC));
+ err = -EINVAL;
+ goto mdev_err;
+ }
+ macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
+ memcpy(set_config.net.mac, macaddr, ETH_ALEN);
+ set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
+ if (mdev->ops->set_mac) {
+ err = mdev->ops->set_mac(mdev, vdev, &set_config);
+ } else {
+ NL_SET_ERR_MSG_FMT_MOD(
+ info->extack,
+ "%s device not support set mac address ", name);
+ }
+
+ } else {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "%s device not support this config ",
+ 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 +1351,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_CONFIG_SET,
+ .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 db15ac07f8a6..c97f4f1da753 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h
@@ -581,6 +581,8 @@ 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 (*set_mac)(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..53f249fb26bc 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_CONFIG_SET,
};
enum vdpa_attr {
--
2.45.0
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 <[email protected]>
---
drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 18 +++++++++++++++++-
1 file changed, 17 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..e961a08341c2 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c
@@ -414,6 +414,21 @@ 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_mac(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)) {
+ 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 +525,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,
+ .set_mac = vdpasim_net_set_mac
};
static struct virtio_device_id id_table[] = {
--
2.45.0
On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> MAC address from the vdpa tool and then set it to the device.
>
> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
Why don't you use devlink?
On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
The patch does not do what commit log says.
Instead there's an internal API to set mac and
a UAPI to write into config space.
> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> 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 sample:
> 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 <[email protected]>
> ---
> drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++
> include/linux/vdpa.h | 2 ++
> include/uapi/linux/vdpa.h | 1 +
> 3 files changed, 74 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index a7612e0783b3..347ae6e7749d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
> return err;
> }
>
> +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + struct nlattr **nl_attrs = info->attrs;
> + struct vdpa_mgmt_dev *mdev;
> + const u8 *macaddr;
> + const char *name;
> + int err = 0;
> + struct device *dev;
> + struct vdpa_device *vdev;
> +
> + 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;
> + }
> + mdev = vdev->mdev;
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
Seems to poke at a device without even making sure it's a network
device.
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "Missing features 0x%llx for provided attributes",
> + BIT_ULL(VIRTIO_NET_F_MAC));
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + if (mdev->ops->set_mac) {
> + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "%s device not support set mac address ", name);
> + }
> +
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "%s device not support this config ",
> + 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 +1351,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_CONFIG_SET,
> + .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 db15ac07f8a6..c97f4f1da753 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -581,6 +581,8 @@ 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 (*set_mac)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
> + const struct vdpa_dev_set_config *config);
Well, now vdpa_mgmtdev_ops which was completely generic is growing
a net specific interface. Which begs a question - how is this
going to work with other device types?
> };
>
> /**
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 54b649ab0f22..53f249fb26bc 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_CONFIG_SET,
> };
>
> enum vdpa_attr {
> --
> 2.45.0
On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
> >> Add new UAPI to support the mac address from vdpa tool
> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> >> MAC address from the vdpa tool and then set it to the device.
> >>
> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
> >
> >Why don't you use devlink?
>
> Fair question. Why does vdpa-specific uapi even exist? To have
> driver-specific uapi Does not make any sense to me :/
I am not sure which uapi do you refer to? The one this patch proposes or
the existing one?
--
MST
Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
>On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
>> Add new UAPI to support the mac address from vdpa tool
>> Function vdpa_nl_cmd_dev_config_set_doit() will get the
>> MAC address from the vdpa tool and then set it to the device.
>>
>> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>
>Why don't you use devlink?
Fair question. Why does vdpa-specific uapi even exist? To have
driver-specific uapi Does not make any sense to me :/
Wed, Jun 12, 2024 at 09:15:44AM CEST, [email protected] wrote:
>On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
>> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
>> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
>> >> Add new UAPI to support the mac address from vdpa tool
>> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
>> >> MAC address from the vdpa tool and then set it to the device.
>> >>
>> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>> >
>> >Why don't you use devlink?
>>
>> Fair question. Why does vdpa-specific uapi even exist? To have
>> driver-specific uapi Does not make any sense to me :/
>
>I am not sure which uapi do you refer to? The one this patch proposes or
>the existing one?
Sure, I'm sure pointing out, that devlink should have been the answer
instead of vdpa netlink introduction. That ship is sailed, now we have
unfortunate api duplication which leads to questions like Jakub's one.
That's all :/
On Wed, Jun 12, 2024 at 3:12 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> > Add new UAPI to support the mac address from vdpa tool
>
> The patch does not do what commit log says.
> Instead there's an internal API to set mac and
> a UAPI to write into config space.
>
thanks, Michael, I will rewrite this part
Thanks
cindy
> > Function vdpa_nl_cmd_dev_config_set_doit() will get the
> > 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 sample:
> > 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 <[email protected]>
> > ---
> > drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++
> > include/linux/vdpa.h | 2 ++
> > include/uapi/linux/vdpa.h | 1 +
> > 3 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index a7612e0783b3..347ae6e7749d 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
> > return err;
> > }
> >
> > +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> > + struct genl_info *info)
> > +{
> > + struct vdpa_dev_set_config set_config = {};
> > + struct nlattr **nl_attrs = info->attrs;
> > + struct vdpa_mgmt_dev *mdev;
> > + const u8 *macaddr;
> > + const char *name;
> > + int err = 0;
> > + struct device *dev;
> > + struct vdpa_device *vdev;
> > +
> > + 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;
> > + }
> > + mdev = vdev->mdev;
> > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
>
>
> Seems to poke at a device without even making sure it's a network
> device.
>
sure ,will add the check
Thansk
cindy
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "Missing features 0x%llx for provided attributes",
> > + BIT_ULL(VIRTIO_NET_F_MAC));
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> > + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> > + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> > + if (mdev->ops->set_mac) {
> > + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "%s device not support set mac address ", name);
> > + }
> > +
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > + "%s device not support this config ",
> > + 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 +1351,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_CONFIG_SET,
> > + .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 db15ac07f8a6..c97f4f1da753 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -581,6 +581,8 @@ 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 (*set_mac)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev,
> > + const struct vdpa_dev_set_config *config);
>
>
> Well, now vdpa_mgmtdev_ops which was completely generic is growing
> a net specific interface. Which begs a question - how is this
> going to work with other device types?
>
Thanks Micheal, I will rewrite this part
Thanks
Cindy
> > };
> >
> > /**
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 54b649ab0f22..53f249fb26bc 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_CONFIG_SET,
> > };
> >
> > enum vdpa_attr {
> > --
> > 2.45.0
>
On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote:
> Wed, Jun 12, 2024 at 09:15:44AM CEST, [email protected] wrote:
> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
> >> >> Add new UAPI to support the mac address from vdpa tool
> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> >> >> MAC address from the vdpa tool and then set it to the device.
> >> >>
> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
> >> >
> >> >Why don't you use devlink?
> >>
> >> Fair question. Why does vdpa-specific uapi even exist? To have
> >> driver-specific uapi Does not make any sense to me :/
> >
> >I am not sure which uapi do you refer to? The one this patch proposes or
> >the existing one?
>
> Sure, I'm sure pointing out, that devlink should have been the answer
> instead of vdpa netlink introduction. That ship is sailed,
> now we have
> unfortunate api duplication which leads to questions like Jakub's one.
> That's all :/
Yea there's no point to argue now, there were arguments this and that
way. I don't think we currently have a lot
of duplication, do we?
--
MST
On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> Add new UAPI to support the mac address from vdpa tool
> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> 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 sample:
> 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 <[email protected]>
I think actually the idea of allowing provisioning
by specifying config of the device is actually valid.
However
- the name SET_CONFIG makes people think this allows
writing even when e.g. device is assigned to guest
- having the internal api be mac specific is weird
Shouldn't config be an attribute maybe, not a new command?
> ---
> drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++
> include/linux/vdpa.h | 2 ++
> include/uapi/linux/vdpa.h | 1 +
> 3 files changed, 74 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index a7612e0783b3..347ae6e7749d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
> return err;
> }
>
> +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> + struct genl_info *info)
> +{
> + struct vdpa_dev_set_config set_config = {};
> + struct nlattr **nl_attrs = info->attrs;
> + struct vdpa_mgmt_dev *mdev;
> + const u8 *macaddr;
> + const char *name;
> + int err = 0;
> + struct device *dev;
> + struct vdpa_device *vdev;
> +
> + 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;
> + }
> + mdev = vdev->mdev;
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "Missing features 0x%llx for provided attributes",
> + BIT_ULL(VIRTIO_NET_F_MAC));
> + err = -EINVAL;
> + goto mdev_err;
> + }
> + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> + if (mdev->ops->set_mac) {
> + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(
> + info->extack,
> + "%s device not support set mac address ", name);
> + }
> +
> + } else {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "%s device not support this config ",
> + 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 +1351,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_CONFIG_SET,
> + .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 db15ac07f8a6..c97f4f1da753 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -581,6 +581,8 @@ 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 (*set_mac)(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..53f249fb26bc 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_CONFIG_SET,
> };
>
> enum vdpa_attr {
> --
> 2.45.0
Thu, Jun 13, 2024 at 08:49:25AM CEST, [email protected] wrote:
>On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote:
>> Wed, Jun 12, 2024 at 09:15:44AM CEST, [email protected] wrote:
>> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
>> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
>> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
>> >> >> Add new UAPI to support the mac address from vdpa tool
>> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
>> >> >> MAC address from the vdpa tool and then set it to the device.
>> >> >>
>> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>> >> >
>> >> >Why don't you use devlink?
>> >>
>> >> Fair question. Why does vdpa-specific uapi even exist? To have
>> >> driver-specific uapi Does not make any sense to me :/
>> >
>> >I am not sure which uapi do you refer to? The one this patch proposes or
>> >the existing one?
>>
>> Sure, I'm sure pointing out, that devlink should have been the answer
>> instead of vdpa netlink introduction. That ship is sailed,
>
>> now we have
>> unfortunate api duplication which leads to questions like Jakub's one.
>> That's all :/
>
>
>
>Yea there's no point to argue now, there were arguments this and that
>way. I don't think we currently have a lot
>of duplication, do we?
True. I think it would be good to establish guidelines for api
extensions in this area.
>
>--
>MST
>
On Thu, Jun 13, 2024 at 09:21:07AM +0200, Jiri Pirko wrote:
> Thu, Jun 13, 2024 at 08:49:25AM CEST, [email protected] wrote:
> >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote:
> >> Wed, Jun 12, 2024 at 09:15:44AM CEST, [email protected] wrote:
> >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
> >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
> >> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
> >> >> >> Add new UAPI to support the mac address from vdpa tool
> >> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
> >> >> >> MAC address from the vdpa tool and then set it to the device.
> >> >> >>
> >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
> >> >> >
> >> >> >Why don't you use devlink?
> >> >>
> >> >> Fair question. Why does vdpa-specific uapi even exist? To have
> >> >> driver-specific uapi Does not make any sense to me :/
> >> >
> >> >I am not sure which uapi do you refer to? The one this patch proposes or
> >> >the existing one?
> >>
> >> Sure, I'm sure pointing out, that devlink should have been the answer
> >> instead of vdpa netlink introduction. That ship is sailed,
> >
> >> now we have
> >> unfortunate api duplication which leads to questions like Jakub's one.
> >> That's all :/
> >
> >
> >
> >Yea there's no point to argue now, there were arguments this and that
> >way. I don't think we currently have a lot
> >of duplication, do we?
>
> True. I think it would be good to establish guidelines for api
> extensions in this area.
>
> >
> >--
> >MST
> >
Guidelines are good, are there existing examples of such guidelines in
Linux to follow though? Specifically after reviewing this some more, I
think what Cindy is trying to do is actually provisioning as opposed to
programming.
--
MST
On Thu, Jun 13, 2024 at 2:59 PM Michael S. Tsirkin <[email protected]> wrote:
>
> On Tue, Jun 11, 2024 at 01:32:32PM +0800, Cindy Lu wrote:
> > Add new UAPI to support the mac address from vdpa tool
> > Function vdpa_nl_cmd_dev_config_set_doit() will get the
> > 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 sample:
> > 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 <[email protected]>
>
>
>
> I think actually the idea of allowing provisioning
> by specifying config of the device is actually valid.
> However
> - the name SET_CONFIG makes people think this allows
> writing even when e.g. device is assigned to guest
> - having the internal api be mac specific is weird
>
> Shouldn't config be an attribute maybe, not a new command?
>
Got it. Thanks, Michael. I will change this.
Thanks
Cindy
>
> > ---
> > drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++
> > include/linux/vdpa.h | 2 ++
> > include/uapi/linux/vdpa.h | 1 +
> > 3 files changed, 74 insertions(+)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index a7612e0783b3..347ae6e7749d 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -1149,6 +1149,72 @@ static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct genl_info
> > return err;
> > }
> >
> > +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb,
> > + struct genl_info *info)
> > +{
> > + struct vdpa_dev_set_config set_config = {};
> > + struct nlattr **nl_attrs = info->attrs;
> > + struct vdpa_mgmt_dev *mdev;
> > + const u8 *macaddr;
> > + const char *name;
> > + int err = 0;
> > + struct device *dev;
> > + struct vdpa_device *vdev;
> > +
> > + 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;
> > + }
> > + mdev = vdev->mdev;
> > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > + if (!(mdev->supported_features & BIT_ULL(VIRTIO_NET_F_MAC))) {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "Missing features 0x%llx for provided attributes",
> > + BIT_ULL(VIRTIO_NET_F_MAC));
> > + err = -EINVAL;
> > + goto mdev_err;
> > + }
> > + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]);
> > + memcpy(set_config.net.mac, macaddr, ETH_ALEN);
> > + set_config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR);
> > + if (mdev->ops->set_mac) {
> > + err = mdev->ops->set_mac(mdev, vdev, &set_config);
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(
> > + info->extack,
> > + "%s device not support set mac address ", name);
> > + }
> > +
> > + } else {
> > + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> > + "%s device not support this config ",
> > + 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 +1351,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_CONFIG_SET,
> > + .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 db15ac07f8a6..c97f4f1da753 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -581,6 +581,8 @@ 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 (*set_mac)(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..53f249fb26bc 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_CONFIG_SET,
> > };
> >
> > enum vdpa_attr {
> > --
> > 2.45.0
>
Thu, Jun 13, 2024 at 09:50:54AM CEST, [email protected] wrote:
>On Thu, Jun 13, 2024 at 09:21:07AM +0200, Jiri Pirko wrote:
>> Thu, Jun 13, 2024 at 08:49:25AM CEST, [email protected] wrote:
>> >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote:
>> >> Wed, Jun 12, 2024 at 09:15:44AM CEST, [email protected] wrote:
>> >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote:
>> >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, [email protected] wrote:
>> >> >> >On Tue, 11 Jun 2024 13:32:32 +0800 Cindy Lu wrote:
>> >> >> >> Add new UAPI to support the mac address from vdpa tool
>> >> >> >> Function vdpa_nl_cmd_dev_config_set_doit() will get the
>> >> >> >> MAC address from the vdpa tool and then set it to the device.
>> >> >> >>
>> >> >> >> The usage is: vdpa dev set name vdpa_name mac **:**:**:**:**:**
>> >> >> >
>> >> >> >Why don't you use devlink?
>> >> >>
>> >> >> Fair question. Why does vdpa-specific uapi even exist? To have
>> >> >> driver-specific uapi Does not make any sense to me :/
>> >> >
>> >> >I am not sure which uapi do you refer to? The one this patch proposes or
>> >> >the existing one?
>> >>
>> >> Sure, I'm sure pointing out, that devlink should have been the answer
>> >> instead of vdpa netlink introduction. That ship is sailed,
>> >
>> >> now we have
>> >> unfortunate api duplication which leads to questions like Jakub's one.
>> >> That's all :/
>> >
>> >
>> >
>> >Yea there's no point to argue now, there were arguments this and that
>> >way. I don't think we currently have a lot
>> >of duplication, do we?
>>
>> True. I think it would be good to establish guidelines for api
>> extensions in this area.
>>
>> >
>> >--
>> >MST
>> >
>
>
>Guidelines are good, are there existing examples of such guidelines in
>Linux to follow though? Specifically after reviewing this some more, I
Documentation directory in general.
>think what Cindy is trying to do is actually provisioning as opposed to
>programming.
>
>--
>MST
>