Re: [PATCH linux-next v2 05/14] vdpa: Enable user to set mac and mtu of vdpa device

[Jason Wang <jasowang@redhat.com>]

在 2021/4/7 上午1:04, Parav Pandit 写道:
> $ vdpa dev add name bar mgmtdev vdpasim_net
>
> $ vdpa dev config set bar mac 00:11:22:33:44:55 mtu 9000
>
> $ vdpa dev config show
> bar: mac 00:11:22:33:44:55 link up link_announce false mtu 9000 speed 0 duplex 0
>
> $ vdpa dev config show -jp
> {
>      "config": {
>          "bar": {
>              "mac": "00:11:22:33:44:55",
>              "link ": "up",
>              "link_announce ": false,
>              "mtu": 9000,
>              "speed": 0,
>              "duplex": 0
>          }
>      }
> }
>
> Signed-off-by: Parav Pandit <parav@nvidia.com>
> Reviewed-by: Eli Cohen <elic@nvidia.com>
> ---
> changelog:
> v1->v2:
>   - fixed mtu range checking for max
>   - using NLA_POLICY_ETH_ADDR
>   - set config moved to device ops instead of mgmtdev ops
>   - merged build and set to single routine
>   - ensuring that user has NET_ADMIN capability for configuring network
>     attributes
> ---
>   drivers/vdpa/vdpa.c       | 90 +++++++++++++++++++++++++++++++++++++++
>   include/linux/vdpa.h      | 15 +++++++
>   include/uapi/linux/vdpa.h |  1 +
>   3 files changed, 106 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 9da8deb8c0f2..6a6ef1a085e8 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -881,10 +881,94 @@ vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct netlink_callback *
>   	return msg->len;
>   }
>   
> +static int vdpa_dev_net_config_set(struct vdpa_device *vdev,
> +				   struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct nlattr **nl_attrs = info->attrs;
> +	struct vdpa_dev_set_config config = {};
> +	const u8 *macaddr;
> +	int err;
> +
> +	if (!vdev->config->set_ce_config)
> +		return -EOPNOTSUPP;
> +
> +	if (!netlink_capable(skb, CAP_NET_ADMIN))
> +		return -EPERM;
> +
> +	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.net_mask.mac_valid = true;
> +	}
> +	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]) {
> +		config.net.mtu =
> +			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
> +		config.net_mask.mtu_valid = true;
> +	}


I think it's better to aovid any device specific stuffs in vdpa.c. Can 
we just make the config data opaque and simply pass them to the driver?


> +
> +	mutex_lock(&vdev->cf_mutex);
> +	err = vdev->config->set_ce_config(vdev, &config);
> +	mutex_unlock(&vdev->cf_mutex);
> +	return err;
> +}
> +
> +static int vdpa_dev_config_set(struct vdpa_device *vdev, struct sk_buff *skb,
> +			       struct genl_info *info)
> +{
> +	int err = -EOPNOTSUPP;
> +	u32 device_id;
> +
> +	if (!vdev->mdev)
> +		return -EOPNOTSUPP;
> +
> +	device_id = vdev->config->get_device_id(vdev);
> +	switch (device_id) {
> +	case VIRTIO_ID_NET:
> +		err = vdpa_dev_net_config_set(vdev, skb, info);
> +		break;
> +	default:
> +		break;
> +	}
> +	return err;
> +}
> +
> +static int vdpa_nl_cmd_dev_config_set_doit(struct sk_buff *skb, struct genl_info *info)
> +{
> +	struct vdpa_device *vdev;
> +	const char *devname;
> +	struct device *dev;
> +	int err;
> +
> +	if (!info->attrs[VDPA_ATTR_DEV_NAME])
> +		return -EINVAL;
> +	devname = nla_data(info->attrs[VDPA_ATTR_DEV_NAME]);
> +
> +	mutex_lock(&vdpa_dev_mutex);
> +	dev = bus_find_device(&vdpa_bus, NULL, devname, vdpa_name_match);
> +	if (!dev) {
> +		mutex_unlock(&vdpa_dev_mutex);
> +		NL_SET_ERR_MSG_MOD(info->extack, "device not found");
> +		return -ENODEV;
> +	}
> +	vdev = container_of(dev, struct vdpa_device, dev);
> +	if (!vdev->mdev) {
> +		mutex_unlock(&vdpa_dev_mutex);
> +		put_device(dev);
> +		return -EINVAL;
> +	}
> +	err = vdpa_dev_config_set(vdev, skb, info);
> +	put_device(dev);
> +	mutex_unlock(&vdpa_dev_mutex);
> +	return err;
> +}
> +
>   static const struct nla_policy vdpa_nl_policy[VDPA_ATTR_MAX + 1] = {
>   	[VDPA_ATTR_MGMTDEV_BUS_NAME] = { .type = NLA_NUL_STRING },
>   	[VDPA_ATTR_MGMTDEV_DEV_NAME] = { .type = NLA_STRING },
>   	[VDPA_ATTR_DEV_NAME] = { .type = NLA_STRING },
> +	[VDPA_ATTR_DEV_NET_CFG_MACADDR] = NLA_POLICY_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[] = {
> @@ -919,6 +1003,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
>   		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
>   		.flags = GENL_ADMIN_PERM,
>   	},
> +	{
> +		.cmd = VDPA_CMD_DEV_CONFIG_SET,
> +		.validate = GENL_DONT_VALIDATE_STRICT | GENL_DONT_VALIDATE_DUMP,
> +		.doit = vdpa_nl_cmd_dev_config_set_doit,
> +		.flags = GENL_ADMIN_PERM,
> +	},
>   };
>   
>   static struct genl_family vdpa_nl_family __ro_after_init = {
> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> index dcbbecb5dea8..b59e1a214161 100644
> --- a/include/linux/vdpa.h
> +++ b/include/linux/vdpa.h
> @@ -104,6 +104,14 @@ struct vdpa_dev_config {
>   	struct vdpa_net_dev_config net;
>   };
>   
> +struct vdpa_dev_set_config {
> +	struct vdpa_net_dev_config net;
> +	struct {
> +		u8 mac_valid: 1;
> +		u8 mtu_valid: 1;
> +	} net_mask;
> +};
> +
>   /**
>    * struct vdpa_config_ops - operations for configuring a vDPA device.
>    * Note: vDPA device drivers are required to implement all of the
> @@ -204,6 +212,11 @@ struct vdpa_dev_config {
>    *				cpu endianness.
>    *				@vdev: vdpa device
>    *				@config: pointer to config buffer used to read to
> + * @set_ce_config:		Set one or more device configuration in
> + *				cpu endianness.
> + *				@vdev: vdpa device
> + *				@config: configuration to update
> + *				Returns 0 on success or error code


Can we simply use set_config here?

Thanks


>    * @get_generation:		Get device config generation (optional)
>    *				@vdev: vdpa device
>    *				Returns u32: device generation
> @@ -277,6 +290,8 @@ struct vdpa_config_ops {
>   			   const void *buf, unsigned int len);
>   	void (*get_ce_config)(struct vdpa_device *vdev,
>   			      struct vdpa_dev_config *config);
> +	int (*set_ce_config)(struct vdpa_device *vdev,
> +			     const struct vdpa_dev_set_config *config);
>   	u32 (*get_generation)(struct vdpa_device *vdev);
>   	struct vdpa_iova_range (*get_iova_range)(struct vdpa_device *vdev);
>   
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 5c31ecc3b956..ec349789b8d1 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -18,6 +18,7 @@ enum vdpa_command {
>   	VDPA_CMD_DEV_DEL,
>   	VDPA_CMD_DEV_GET,		/* can dump */
>   	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> +	VDPA_CMD_DEV_CONFIG_SET,
>   };
>   
>   enum vdpa_attr {

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