| Message ID | 20240611053239.516996-1-lulu@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] vdpa: support set mac address from vdpa tool | expand |
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?
Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/
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 <lulu@redhat.com> > --- > 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, kuba@kernel.org 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?
Wed, Jun 12, 2024 at 09:15:44AM CEST, mst@redhat.com wrote: >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 <mst@redhat.com> 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 <lulu@redhat.com> > > --- > > 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, mst@redhat.com wrote: > >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: > >> Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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?
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 <lulu@redhat.com> 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, mst@redhat.com wrote: >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: >> Wed, Jun 12, 2024 at 09:15:44AM CEST, mst@redhat.com wrote: >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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, mst@redhat.com wrote: > >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: > >> Wed, Jun 12, 2024 at 09:15:44AM CEST, mst@redhat.com wrote: > >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: > >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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.
On Thu, Jun 13, 2024 at 2:59 PM Michael S. Tsirkin <mst@redhat.com> 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 <lulu@redhat.com> > > > > 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, mst@redhat.com wrote: >On Thu, Jun 13, 2024 at 09:21:07AM +0200, Jiri Pirko wrote: >> Thu, Jun 13, 2024 at 08:49:25AM CEST, mst@redhat.com wrote: >> >On Wed, Jun 12, 2024 at 09:22:32AM +0200, Jiri Pirko wrote: >> >> Wed, Jun 12, 2024 at 09:15:44AM CEST, mst@redhat.com wrote: >> >> >On Wed, Jun 12, 2024 at 08:29:53AM +0200, Jiri Pirko wrote: >> >> >> Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 >
On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: > > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ It came with devlink first actually, but switched to a dedicated uAPI. Parav(cced) may explain more here. Thanks >
> From: Jason Wang <jasowang@redhat.com> > Sent: Monday, June 17, 2024 7:18 AM > > On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: > > > > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ > > It came with devlink first actually, but switched to a dedicated uAPI. > > Parav(cced) may explain more here. > Devlink configures function level mac that applies to all protocol devices (vdpa, rdma, netdev) etc. Additionally, vdpa device level mac can be different (an additional one) to apply to only vdpa traffic. Hence dedicated uAPI was added.
Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: > > >> From: Jason Wang <jasowang@redhat.com> >> Sent: Monday, June 17, 2024 7:18 AM >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: >> > >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> Parav(cced) may explain more here. >> >Devlink configures function level mac that applies to all protocol devices (vdpa, rdma, netdev) etc. >Additionally, vdpa device level mac can be different (an additional one) to apply to only vdpa traffic. >Hence dedicated uAPI was added. There is 1:1 relation between vdpa instance and devlink port, isn't it? Then we have: devlink port function set DEV/PORT_INDEX hw_addr ADDR Which does exactly what you need, configure function hw address (mac). When you say VDPA traffic, do you suggest there might be VDPA instance and netdev running on the same VF in parallel. If yes, do we have 2 eswitch port representors to be separately used to steer the traffic? If no, how is that supposed to be working?
> From: Jiri Pirko <jiri@resnulli.us> > Sent: Monday, June 17, 2024 3:09 PM > > Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: > > > > > >> From: Jason Wang <jasowang@redhat.com> > >> Sent: Monday, June 17, 2024 7:18 AM > >> > >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> > > >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ > >> > >> It came with devlink first actually, but switched to a dedicated uAPI. > >> > >> Parav(cced) may explain more here. > >> > >Devlink configures function level mac that applies to all protocol devices > (vdpa, rdma, netdev) etc. > >Additionally, vdpa device level mac can be different (an additional one) to > apply to only vdpa traffic. > >Hence dedicated uAPI was added. > > There is 1:1 relation between vdpa instance and devlink port, isn't it? > Then we have: > devlink port function set DEV/PORT_INDEX hw_addr ADDR > Above command is privilege command done by the hypervisor on the port function. Vpda level setting the mac is similar to a function owner driver setting the mac on the self netdev (even though devlink side has configured some mac for it). For example, $ ip link set dev wlan1 address 00:11:22:33:44:55 > Which does exactly what you need, configure function hw address (mac). > > When you say VDPA traffic, do you suggest there might be VDPA instance and > netdev running on the same VF in parallel. If yes, do we have 2 eswitch port > representors to be separately used to steer the traffic? > If no, how is that supposed to be working? A eswitch may allow incoming and outgoing traffic from multiple mac addresses left to the tc rules to decide. It does not need two eswitch ports.
Mon, Jun 17, 2024 at 11:44:53AM CEST, parav@nvidia.com wrote: > >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Monday, June 17, 2024 3:09 PM >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: >> > >> > >> >> From: Jason Wang <jasowang@redhat.com> >> >> Sent: Monday, June 17, 2024 7:18 AM >> >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> > >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ >> >> >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> >> >> Parav(cced) may explain more here. >> >> >> >Devlink configures function level mac that applies to all protocol devices >> (vdpa, rdma, netdev) etc. >> >Additionally, vdpa device level mac can be different (an additional one) to >> apply to only vdpa traffic. >> >Hence dedicated uAPI was added. >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? >> Then we have: >> devlink port function set DEV/PORT_INDEX hw_addr ADDR >> >Above command is privilege command done by the hypervisor on the port function. >Vpda level setting the mac is similar to a function owner driver setting the mac on the self netdev (even though devlink side has configured some mac for it). >For example, >$ ip link set dev wlan1 address 00:11:22:33:44:55 Hmm, under what sceratio exacly this is needed? I mean, the VM that has VDPA device can actually do that too. That is the actual function owner. > >> Which does exactly what you need, configure function hw address (mac). >> >> When you say VDPA traffic, do you suggest there might be VDPA instance and >> netdev running on the same VF in parallel. If yes, do we have 2 eswitch port >> representors to be separately used to steer the traffic? >> If no, how is that supposed to be working? >A eswitch may allow incoming and outgoing traffic from multiple mac addresses left to the tc rules to decide. >It does not need two eswitch ports. Ugh.
> From: Jiri Pirko <jiri@resnulli.us> > Sent: Monday, June 17, 2024 5:10 PM > > Mon, Jun 17, 2024 at 11:44:53AM CEST, parav@nvidia.com wrote: > > > >> From: Jiri Pirko <jiri@resnulli.us> > >> Sent: Monday, June 17, 2024 3:09 PM > >> > >> Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: > >> > > >> > > >> >> From: Jason Wang <jasowang@redhat.com> > >> >> Sent: Monday, June 17, 2024 7:18 AM > >> >> > >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> > > >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ > >> >> > >> >> It came with devlink first actually, but switched to a dedicated uAPI. > >> >> > >> >> Parav(cced) may explain more here. > >> >> > >> >Devlink configures function level mac that applies to all protocol > >> >devices > >> (vdpa, rdma, netdev) etc. > >> >Additionally, vdpa device level mac can be different (an additional > >> >one) to > >> apply to only vdpa traffic. > >> >Hence dedicated uAPI was added. > >> > >> There is 1:1 relation between vdpa instance and devlink port, isn't it? > >> Then we have: > >> devlink port function set DEV/PORT_INDEX hw_addr ADDR > >> > >Above command is privilege command done by the hypervisor on the port > function. > >Vpda level setting the mac is similar to a function owner driver setting the > mac on the self netdev (even though devlink side has configured some mac for > it). > >For example, > >$ ip link set dev wlan1 address 00:11:22:33:44:55 > > Hmm, under what sceratio exacly this is needed? The administrator on the host creating a vdpa device for the VM wants to configure the mac address for the VM. This administrator may not have the access to the devlink port function. Or he may just prefer a different MAC (theoretical case). > I mean, the VM that has VDPA device can actually do that too. VM cannot do. Virtio spec do not allow modifying the mac address. > That is the actual function owner. vdpa is not mapping a whole VF to the VM. It is getting some synthetic PCI device composed using several software (kernel) and user space layers. so VM is not the function owner.
Mon, Jun 17, 2024 at 01:48:02PM CEST, parav@nvidia.com wrote: > >> From: Jiri Pirko <jiri@resnulli.us> >> Sent: Monday, June 17, 2024 5:10 PM >> >> Mon, Jun 17, 2024 at 11:44:53AM CEST, parav@nvidia.com wrote: >> > >> >> From: Jiri Pirko <jiri@resnulli.us> >> >> Sent: Monday, June 17, 2024 3:09 PM >> >> >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: >> >> > >> >> > >> >> >> From: Jason Wang <jasowang@redhat.com> >> >> >> Sent: Monday, June 17, 2024 7:18 AM >> >> >> >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: >> >> >> > >> >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ >> >> >> >> >> >> It came with devlink first actually, but switched to a dedicated uAPI. >> >> >> >> >> >> Parav(cced) may explain more here. >> >> >> >> >> >Devlink configures function level mac that applies to all protocol >> >> >devices >> >> (vdpa, rdma, netdev) etc. >> >> >Additionally, vdpa device level mac can be different (an additional >> >> >one) to >> >> apply to only vdpa traffic. >> >> >Hence dedicated uAPI was added. >> >> >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? >> >> Then we have: >> >> devlink port function set DEV/PORT_INDEX hw_addr ADDR >> >> >> >Above command is privilege command done by the hypervisor on the port >> function. >> >Vpda level setting the mac is similar to a function owner driver setting the >> mac on the self netdev (even though devlink side has configured some mac for >> it). >> >For example, >> >$ ip link set dev wlan1 address 00:11:22:33:44:55 >> >> Hmm, under what sceratio exacly this is needed? >The administrator on the host creating a vdpa device for the VM wants to configure the mac address for the VM. >This administrator may not have the access to the devlink port function. >Or he may just prefer a different MAC (theoretical case). Right, but that is not reason for new uapi but rather reason to alter existing devlink model to have the "host side". We discussed this many times. > >> I mean, the VM that has VDPA device can actually do that too. >VM cannot do. Virtio spec do not allow modifying the mac address. I see. Any good reason to not allow that? > >> That is the actual function owner. >vdpa is not mapping a whole VF to the VM. >It is getting some synthetic PCI device composed using several software (kernel) and user space layers. >so VM is not the function owner. Sure, but owner of the netdev side, to what the mac is related. That is my point.
On Mon, Jun 17, 2024 at 03:02:43PM +0200, Jiri Pirko wrote: > Mon, Jun 17, 2024 at 01:48:02PM CEST, parav@nvidia.com wrote: > > > >> From: Jiri Pirko <jiri@resnulli.us> > >> Sent: Monday, June 17, 2024 5:10 PM > >> > >> Mon, Jun 17, 2024 at 11:44:53AM CEST, parav@nvidia.com wrote: > >> > > >> >> From: Jiri Pirko <jiri@resnulli.us> > >> >> Sent: Monday, June 17, 2024 3:09 PM > >> >> > >> >> Mon, Jun 17, 2024 at 04:57:23AM CEST, parav@nvidia.com wrote: > >> >> > > >> >> > > >> >> >> From: Jason Wang <jasowang@redhat.com> > >> >> >> Sent: Monday, June 17, 2024 7:18 AM > >> >> >> > >> >> >> On Wed, Jun 12, 2024 at 2:30 PM Jiri Pirko <jiri@resnulli.us> wrote: > >> >> >> > > >> >> >> > Wed, Jun 12, 2024 at 03:58:10AM CEST, kuba@kernel.org 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 :/ > >> >> >> > >> >> >> It came with devlink first actually, but switched to a dedicated uAPI. > >> >> >> > >> >> >> Parav(cced) may explain more here. > >> >> >> > >> >> >Devlink configures function level mac that applies to all protocol > >> >> >devices > >> >> (vdpa, rdma, netdev) etc. > >> >> >Additionally, vdpa device level mac can be different (an additional > >> >> >one) to > >> >> apply to only vdpa traffic. > >> >> >Hence dedicated uAPI was added. > >> >> > >> >> There is 1:1 relation between vdpa instance and devlink port, isn't it? > >> >> Then we have: > >> >> devlink port function set DEV/PORT_INDEX hw_addr ADDR > >> >> > >> >Above command is privilege command done by the hypervisor on the port > >> function. > >> >Vpda level setting the mac is similar to a function owner driver setting the > >> mac on the self netdev (even though devlink side has configured some mac for > >> it). > >> >For example, > >> >$ ip link set dev wlan1 address 00:11:22:33:44:55 > >> > >> Hmm, under what sceratio exacly this is needed? > >The administrator on the host creating a vdpa device for the VM wants to configure the mac address for the VM. > >This administrator may not have the access to the devlink port function. > >Or he may just prefer a different MAC (theoretical case). > > Right, but that is not reason for new uapi but rather reason to alter > existing devlink model to have the "host side". We discussed this many > times. > > > > > >> I mean, the VM that has VDPA device can actually do that too. > >VM cannot do. Virtio spec do not allow modifying the mac address. > > I see. Any good reason to not allow that? > > > > > >> That is the actual function owner. > >vdpa is not mapping a whole VF to the VM. > >It is getting some synthetic PCI device composed using several software (kernel) and user space layers. > >so VM is not the function owner. > > Sure, but owner of the netdev side, to what the mac is related. That is > my point. I don't know what this discussion is about, at this point. For better or worse, vdpa gained interfaces for provisioning new devices. Yes the solution space was wide but it's been there for years so kind of too late to try and make people move to another interface for that. Having said that, vdpa interfaces are all built around virtio spec. Let's try to stick to that.
On Mon, 17 Jun 2024 09:47:21 -0400 Michael S. Tsirkin wrote: > I don't know what this discussion is about, at this point. > For better or worse, vdpa gained interfaces for provisioning > new devices. Yes the solution space was wide but it's been there > for years so kind of too late to try and make people > move to another interface for that. > > Having said that, vdpa interfaces are all built around > virtio spec. Let's try to stick to that. But the virtio spec doesn't allow setting the MAC... I'm probably just lost in the conversation but there's hypervisor side and there is user/VM side, each of them already has an interface to set the MAC. The MAC doesn't matter, but I want to make sure my mental model matches reality in case we start duplicating too much..
On Mon, Jun 17, 2024 at 08:20:02AM -0700, Jakub Kicinski wrote: > On Mon, 17 Jun 2024 09:47:21 -0400 Michael S. Tsirkin wrote: > > I don't know what this discussion is about, at this point. > > For better or worse, vdpa gained interfaces for provisioning > > new devices. Yes the solution space was wide but it's been there > > for years so kind of too late to try and make people > > move to another interface for that. > > > > Having said that, vdpa interfaces are all built around > > virtio spec. Let's try to stick to that. > > But the virtio spec doesn't allow setting the MAC... > I'm probably just lost in the conversation but there's hypervisor side > and there is user/VM side, each of them already has an interface to set > the MAC. The MAC doesn't matter, but I want to make sure my mental model > matches reality in case we start duplicating too much.. An obvious part of provisioning is specifying the config space of the device.
On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > But the virtio spec doesn't allow setting the MAC... > > I'm probably just lost in the conversation but there's hypervisor side > > and there is user/VM side, each of them already has an interface to set > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > matches reality in case we start duplicating too much.. > > An obvious part of provisioning is specifying the config space > of the device. Agreed, that part is obvious. Please go ahead, I don't really care and you clearly don't have time to explain.
On Mon, Jun 17, 2024 at 09:44:21AM -0700, Jakub Kicinski wrote: > On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > > But the virtio spec doesn't allow setting the MAC... > > > I'm probably just lost in the conversation but there's hypervisor side > > > and there is user/VM side, each of them already has an interface to set > > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > > matches reality in case we start duplicating too much.. > > > > An obvious part of provisioning is specifying the config space > > of the device. > > Agreed, that part is obvious. > Please go ahead, I don't really care and you clearly don't have time > to explain. Thanks! Just in case Cindy who is working on it is also confused, here is what I meant: - an interface to provision a device, including its config space, makes sense to me - default mac address is part of config space, and would thus be covered - note how this is different from ability to tweak the mac of an existing device
On Tue, Jun 18, 2024 at 6:39 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Jun 17, 2024 at 09:44:21AM -0700, Jakub Kicinski wrote: > > On Mon, 17 Jun 2024 12:20:19 -0400 Michael S. Tsirkin wrote: > > > > But the virtio spec doesn't allow setting the MAC... > > > > I'm probably just lost in the conversation but there's hypervisor side > > > > and there is user/VM side, each of them already has an interface to set > > > > the MAC. The MAC doesn't matter, but I want to make sure my mental model > > > > matches reality in case we start duplicating too much.. > > > > > > An obvious part of provisioning is specifying the config space > > > of the device. > > > > Agreed, that part is obvious. > > Please go ahead, I don't really care and you clearly don't have time > > to explain. > > Thanks! > Just in case Cindy who is working on it is also confused, > here is what I meant: > > - an interface to provision a device, including its config > space, makes sense to me > - default mac address is part of config space, and would thus be covered > - note how this is different from ability to tweak the mac of an existing > device > Thanks Micheal, I will continue working in this thanks cindy > > -- > MST >
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 {
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 <lulu@redhat.com> --- drivers/vdpa/vdpa.c | 71 +++++++++++++++++++++++++++++++++++++++ include/linux/vdpa.h | 2 ++ include/uapi/linux/vdpa.h | 1 + 3 files changed, 74 insertions(+)