Jason Wang
2021-Oct-25 07:01 UTC
[PATCH linux-next v4 4/8] vdpa: Enable user to set mac and mtu of vdpa device
? 2021/10/22 ??12:35, Parav Pandit ??:> $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu 9000 > > $ vdpa dev config show > bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 > > $ vdpa dev config show -jp > { > "config": { > "bar": { > "mac": "00:11:22:33:44:55", > "link ": "up", > "link_announce ": false, > "mtu": 9000, > } > } > } > > Signed-off-by: Parav Pandit <parav at nvidia.com> > Reviewed-by: Eli Cohen <elic at nvidia.com> > --- > changelog: > v3->v4: > - provide config attributes during device addition time > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- > drivers/vdpa/vdpa.c | 33 ++++++++++++++++++++++++++-- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > include/linux/vdpa.h | 17 +++++++++++++- > 7 files changed, 57 insertions(+), 8 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index dcd648e1f7e7..6dc75ca70b37 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -499,7 +499,8 @@ static u32 get_dev_type(struct pci_dev *pdev) > return dev_type; > } > > -static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) > +static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > + const struct vdpa_dev_set_config *config) > { > struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; > struct ifcvf_adapter *adapter; > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index bd56de7484dc..ca05f69054b6 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -2404,7 +2404,8 @@ struct mlx5_vdpa_mgmtdev { > struct mlx5_vdpa_net *ndev; > }; > > -static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name) > +static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > + const struct vdpa_dev_set_config *add_config) > { > struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev); > struct virtio_net_config *config; > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index a50a6aa1cfc4..daa34a61c898 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -14,7 +14,6 @@ > #include <uapi/linux/vdpa.h> > #include <net/genetlink.h> > #include <linux/mod_devicetable.h> > -#include <linux/virtio_net.h> > #include <linux/virtio_ids.h> > > static LIST_HEAD(mdev_head); > @@ -472,9 +471,15 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb) > return msg->len; > } > > +#define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \ > + (1 << VDPA_ATTR_DEV_NET_CFG_MTU)) > + > static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info) > { > + struct vdpa_dev_set_config config = {}; > + struct nlattr **nl_attrs = info->attrs; > struct vdpa_mgmt_dev *mdev; > + const u8 *macaddr; > const char *name; > int err = 0; > > @@ -483,6 +488,21 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i > > name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]); > > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) { > + macaddr = nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]); > + memcpy(config.net.mac, macaddr, sizeof(config.net.mac)); > + config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR); > + } > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) { > + config.net.mtu > + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]); > + config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU); > + } > + > + if ((config.mask & VDPA_DEV_NET_ATTRS_MASK) && > + !netlink_capable(skb, CAP_NET_ADMIN)) > + return -EPERM;This deserves a independent patch. And do we need backport it to stable? Another question is that, do need the cap if not attrs were specified?> + > mutex_lock(&vdpa_dev_mutex); > mdev = vdpa_mgmtdev_get_from_attr(info->attrs); > if (IS_ERR(mdev)) { > @@ -490,8 +510,14 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i > err = PTR_ERR(mdev); > goto err; > } > + if ((config.mask & mdev->config_attr_mask) != config.mask) { > + NL_SET_ERR_MSG_MOD(info->extack, > + "All provided attributes are not supported"); > + err = -EOPNOTSUPP; > + goto err; > + } > > - err = mdev->ops->dev_add(mdev, name); > + err = mdev->ops->dev_add(mdev, name, &config); > err: > mutex_unlock(&vdpa_dev_mutex); > return err; > @@ -822,6 +848,9 @@ static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = { > [VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING }, > [VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING }, > [VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING }, > + [VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR, > + /* virtio spec 1.1 section 5.1.4.1 for valid MTU range */ > + [VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68), > }; > > static const struct genl_ops vdpa_nl_ops[] = { > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > index a790903f243e..42d401d43911 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > @@ -248,7 +248,8 @@ static struct device vdpasim_blk_mgmtdev = { > .release = vdpasim_blk_mgmtdev_release, > }; > > -static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) > +static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > + const struct vdpa_dev_set_config *config) > { > struct vdpasim_dev_attr dev_attr = {}; > struct vdpasim *simdev; > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index a1ab6163f7d1..d681e423e64f 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -126,7 +126,8 @@ static struct device vdpasim_net_mgmtdev = { > .release = vdpasim_net_mgmtdev_release, > }; > > -static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) > +static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > + const struct vdpa_dev_set_config *config) > { > struct vdpasim_dev_attr dev_attr = {}; > struct vdpasim *simdev; > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index 841667a896dd..c9204c62f339 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -1503,7 +1503,8 @@ static int vduse_dev_init_vdpa(struct vduse_dev *dev, const char *name) > return 0; > } > > -static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) > +static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > + const struct vdpa_dev_set_config *config) > { > struct vduse_dev *dev; > int ret; > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > index 111153c9ee71..315da5f918dc 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -6,6 +6,8 @@ > #include <linux/device.h> > #include <linux/interrupt.h> > #include <linux/vhost_iotlb.h> > +#include <linux/virtio_net.h> > +#include <linux/if_ether.h> > > /** > * struct vdpa_calllback - vDPA callback definition. > @@ -93,6 +95,14 @@ struct vdpa_iova_range { > u64 last; > }; > > +struct vdpa_dev_set_config { > + struct { > + u8 mac[ETH_ALEN]; > + u16 mtu; > + } net;If we want to add block device, I guess we need a union as a container?> + u64 mask; > +}; > + > /** > * Corresponding file area for device memory mapping > * @file: vma->vm_file for the mapping > @@ -393,6 +403,7 @@ void vdpa_set_config(struct vdpa_device *dev, unsigned int offset, > * @dev_add: Add a vdpa device using alloc and register > * @mdev: parent device to use for device addition > * @name: name of the new vdpa device > + * @config: config attributes to apply to the device under creation > * Driver need to add a new device using _vdpa_register_device() > * after fully initializing the vdpa device. Driver must return 0 > * on success or appropriate error code. > @@ -403,7 +414,8 @@ void vdpa_set_config(struct vdpa_device *dev, unsigned int offset, > * _vdpa_unregister_device(). > */ > struct vdpa_mgmtdev_ops { > - int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name); > + 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); > }; > > @@ -412,12 +424,15 @@ struct vdpa_mgmtdev_ops { > * @device: Management parent device > * @ops: operations supported by management device > * @id_table: Pointer to device id table of supported ids > + * @config_attr_mask: bit mask of attributes of type enum vdpa_attr that > + * management devie support during dev_add callback > * @list: list entry > */ > struct vdpa_mgmt_dev { > struct device *device; > const struct vdpa_mgmtdev_ops *ops; > const struct virtio_device_id *id_table; > + u64 config_attr_mask; > struct list_head list; > }; >
Parav Pandit
2021-Oct-25 07:06 UTC
[PATCH linux-next v4 4/8] vdpa: Enable user to set mac and mtu of vdpa device
> From: Jason Wang <jasowang at redhat.com> > Sent: Monday, October 25, 2021 12:31 PM > > ? 2021/10/22 ??12:35, Parav Pandit ??: > > $ vdpa dev add name bar mgmtdev vdpasim_net mac 00:11:22:33:44:55 mtu > > 9000 > > > > $ vdpa dev config show > > bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 > > > > $ vdpa dev config show -jp > > { > > "config": { > > "bar": { > > "mac": "00:11:22:33:44:55", > > "link ": "up", > > "link_announce ": false, > > "mtu": 9000, > > } > > } > > } > > > > Signed-off-by: Parav Pandit <parav at nvidia.com> > > Reviewed-by: Eli Cohen <elic at nvidia.com> > > --- > > changelog: > > v3->v4: > > - provide config attributes during device addition time > > --- > > drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- > > drivers/vdpa/vdpa.c | 33 ++++++++++++++++++++++++++-- > > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- > > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- > > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > > include/linux/vdpa.h | 17 +++++++++++++- > > 7 files changed, 57 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c > > b/drivers/vdpa/ifcvf/ifcvf_main.c index dcd648e1f7e7..6dc75ca70b37 > > 100644 > > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > > @@ -499,7 +499,8 @@ static u32 get_dev_type(struct pci_dev *pdev) > > return dev_type; > > } > > > > -static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char > > *name) > > +static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char > *name, > > + const struct vdpa_dev_set_config *config) > > { > > struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; > > struct ifcvf_adapter *adapter; > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > index bd56de7484dc..ca05f69054b6 100644 > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > > @@ -2404,7 +2404,8 @@ struct mlx5_vdpa_mgmtdev { > > struct mlx5_vdpa_net *ndev; > > }; > > > > -static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char > > *name) > > +static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char > *name, > > + const struct vdpa_dev_set_config *add_config) > > { > > struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct > mlx5_vdpa_mgmtdev, mgtdev); > > struct virtio_net_config *config; > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > > a50a6aa1cfc4..daa34a61c898 100644 > > --- a/drivers/vdpa/vdpa.c > > +++ b/drivers/vdpa/vdpa.c > > @@ -14,7 +14,6 @@ > > #include <uapi/linux/vdpa.h> > > #include <net/genetlink.h> > > #include <linux/mod_devicetable.h> > > -#include <linux/virtio_net.h> > > #include <linux/virtio_ids.h> > > > > static LIST_HEAD(mdev_head); > > @@ -472,9 +471,15 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff > *msg, struct netlink_callback *cb) > > return msg->len; > > } > > > > +#define VDPA_DEV_NET_ATTRS_MASK ((1 << > VDPA_ATTR_DEV_NET_CFG_MACADDR) | \ > > + (1 << VDPA_ATTR_DEV_NET_CFG_MTU)) > > + > > static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct > genl_info *info) > > { > > + struct vdpa_dev_set_config config = {}; > > + struct nlattr **nl_attrs = info->attrs; > > struct vdpa_mgmt_dev *mdev; > > + const u8 *macaddr; > > const char *name; > > int err = 0; > > > > @@ -483,6 +488,21 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct > > sk_buff *skb, struct genl_info *i > > > > name = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]); > > > > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]) { > > + macaddr > nla_data(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR]); > > + memcpy(config.net.mac, macaddr, sizeof(config.net.mac)); > > + config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR); > > + } > > + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) { > > + config.net.mtu > > + > nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]); > > + config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU); > > + } > > + > > + if ((config.mask & VDPA_DEV_NET_ATTRS_MASK) && > > + !netlink_capable(skb, CAP_NET_ADMIN)) > > + return -EPERM; > > > This deserves a independent patch. And do we need backport it to stable?This patch is adding the ability to configure mac and mtu. Hence, it is in this patch. It cannot be a different patch after this.> > Another question is that, do need the cap if not attrs were specified?I am not sure. A user is adding the vpda device anchored on the bus. We likely need different capability check than the NET_ADMIN.> > > > + > > mutex_lock(&vdpa_dev_mutex); > > mdev = vdpa_mgmtdev_get_from_attr(info->attrs); > > if (IS_ERR(mdev)) { > > @@ -490,8 +510,14 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct > sk_buff *skb, struct genl_info *i > > err = PTR_ERR(mdev); > > goto err; > > } > > + if ((config.mask & mdev->config_attr_mask) != config.mask) { > > + NL_SET_ERR_MSG_MOD(info->extack, > > + "All provided attributes are not supported"); > > + err = -EOPNOTSUPP; > > + goto err; > > + } > > > > - err = mdev->ops->dev_add(mdev, name); > > + err = mdev->ops->dev_add(mdev, name, &config); > > err: > > mutex_unlock(&vdpa_dev_mutex); > > return err; > > @@ -822,6 +848,9 @@ static const struct nla_policy > vdpa_nl_policy[VDPA_ATTR_MAX + 1] = { > > [VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING }, > > [VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING }, > > [VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING }, > > + [VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_ETH_ADDR, > > + /* virtio spec 1.1 section 5.1.4.1 for valid MTU range */ > > + [VDPA_ATTR_DEV_NET_CFG_MTU] = NLA_POLICY_MIN(NLA_U16, 68), > > }; > > > > static const struct genl_ops vdpa_nl_ops[] = { diff --git > > a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > > b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > > index a790903f243e..42d401d43911 100644 > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > > @@ -248,7 +248,8 @@ static struct device vdpasim_blk_mgmtdev = { > > .release = vdpasim_blk_mgmtdev_release, > > }; > > > > -static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char > > *name) > > +static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char > *name, > > + const struct vdpa_dev_set_config *config) > > { > > struct vdpasim_dev_attr dev_attr = {}; > > struct vdpasim *simdev; > > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > > index a1ab6163f7d1..d681e423e64f 100644 > > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > > @@ -126,7 +126,8 @@ static struct device vdpasim_net_mgmtdev = { > > .release = vdpasim_net_mgmtdev_release, > > }; > > > > -static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char > > *name) > > +static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char > *name, > > + const struct vdpa_dev_set_config *config) > > { > > struct vdpasim_dev_attr dev_attr = {}; > > struct vdpasim *simdev; > > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c > > b/drivers/vdpa/vdpa_user/vduse_dev.c > > index 841667a896dd..c9204c62f339 100644 > > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > > @@ -1503,7 +1503,8 @@ static int vduse_dev_init_vdpa(struct vduse_dev > *dev, const char *name) > > return 0; > > } > > > > -static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name) > > +static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > > + const struct vdpa_dev_set_config *config) > > { > > struct vduse_dev *dev; > > int ret; > > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index > > 111153c9ee71..315da5f918dc 100644 > > --- a/include/linux/vdpa.h > > +++ b/include/linux/vdpa.h > > @@ -6,6 +6,8 @@ > > #include <linux/device.h> > > #include <linux/interrupt.h> > > #include <linux/vhost_iotlb.h> > > +#include <linux/virtio_net.h> > > +#include <linux/if_ether.h> > > > > /** > > * struct vdpa_calllback - vDPA callback definition. > > @@ -93,6 +95,14 @@ struct vdpa_iova_range { > > u64 last; > > }; > > > > +struct vdpa_dev_set_config { > > + struct { > > + u8 mac[ETH_ALEN]; > > + u16 mtu; > > + } net; > > > If we want to add block device, I guess we need a union as a container?Right. When that occurs in future, there will be union to contain both.