RE: [PATCH linux-next v3 2/6] vdpa: Introduce query of device config layout

[Parav Pandit <parav@nvidia.com>]

> From: Jason Wang <jasowang@redhat.com>
> Sent: Tuesday, June 22, 2021 12:50 PM
> 

[..]
> >   {
> >   	const struct vdpa_config_ops *ops = vdev->config;
> >
> > +	mutex_lock(&vdev->cf_mutex);
> >   	/*
> >   	 * Config accesses aren't supposed to trigger before features are set.
> >   	 * If it does happen we assume a legacy guest.
> > @@ -303,6 +308,7 @@ void vdpa_get_config(struct vdpa_device *vdev,
> unsigned int offset,
> >   	if (!vdev->features_valid)
> >   		vdpa_set_features(vdev, 0);
> >   	ops->get_config(vdev, offset, buf, len);
> > +	mutex_unlock(&vdev->cf_mutex);
> >   }
> >   EXPORT_SYMBOL_GPL(vdpa_get_config);
> >
> > @@ -316,7 +322,9 @@ EXPORT_SYMBOL_GPL(vdpa_get_config);
> >   void vdpa_set_config(struct vdpa_device *vdev, unsigned int offset,
> >   		     void *buf, unsigned int length)
> >   {
> > +	mutex_lock(&vdev->cf_mutex);
> >   	vdev->config->set_config(vdev, offset, buf, length);
> > +	mutex_unlock(&vdev->cf_mutex);
> 
> 
> I think it's better to use a separate patch to implement the synchronization in
> set_get()/get_config()
> 
Ok. I will split.

> 
> >   }
> >   EXPORT_SYMBOL_GPL(vdpa_set_config);
> >
> > @@ -643,6 +651,204 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct
> sk_buff *msg, struct netlink_callba
> >   	return msg->len;
> >   }
> >
> > +static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
> > +				       struct sk_buff *msg, u64 features,
> > +				       const struct virtio_net_config *config)
> > +{
> > +	u16 val_u16;
> > +
> > +	if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0)
> > +		return 0;
> > +
> > +	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
> > +	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> val_u16);
> > +}
> > +
> > +static int vdpa_dev_net_rss_config_fill(struct vdpa_device *vdev,
> > +					struct sk_buff *msg, u64 features,
> > +					const struct virtio_net_config
> *config)
> > +{
> > +	u16 val_u16;
> > +	u16 val_u32;
> > +
> > +	if ((features & (1ULL << VIRTIO_NET_F_RSS)) == 0)
> > +		return 0;
> > +
> > +	if (nla_put_u8(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,
> > +		       config->rss_max_key_size))
> > +		return -EMSGSIZE;
> > +
> > +	val_u16 = le16_to_cpu(config->rss_max_key_size);
> > +	if (nla_put_u16(msg,
> VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, val_u16))
> > +		return -EMSGSIZE;
> > +
> > +	val_u32 = le32_to_cpu(config->supported_hash_types);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,
> val_u32))
> > +		return -EMSGSIZE;
> > +	return 0;
> > +}
> > +
> > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct
> sk_buff *msg)
> > +{
> > +	struct virtio_net_config config = {};
> > +	u64 features;
> > +	u32 val_u32;
> > +	u16 val_u16;
> > +	int err;
> > +
> > +	vdpa_get_config(vdev, 0, &config, sizeof(config));
> > +
> > +	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
> sizeof(config.mac),
> > +		    config.mac))
> > +		return -EMSGSIZE;
> > +
> > +	val_u16 = le16_to_cpu(config.status);
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> > +		return -EMSGSIZE;
> 
> 
> Note that status field only exist when VIRITO_NET_F_STATUS is
> negotiated. And if not, we need assume the link is up.
> 
Ok. will change.

> 
> > +
> > +	val_u16 = le16_to_cpu(config.mtu);
> > +	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> > +		return -EMSGSIZE;
> > +
> > +	val_u32 = le32_to_cpu(config.speed);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, val_u32))
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX,
> config.duplex))
> > +		return -EMSGSIZE;
> 
> 
> The above two only exists when VIRTIO_NET_F_SPEED_DUPLEX is
> negotiated.
> 
I think I missed fixing this. You or Michael mentioned this in v2.
Will fix.

> 
> > +
> > +	features = vdev->config->get_features(vdev);
> > +
> > +	err = vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > +	if (err)
> > +		return err;
> > +	return vdpa_dev_net_rss_config_fill(vdev, msg, features, &config);
> > +}
> > +
> > +static int
> > +vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32
> portid, u32 seq,
> > +		     int flags, struct netlink_ext_ack *extack)
> > +{
> > +	u32 device_id;
> > +	void *hdr;
> > +	int err;
> > +
> > +	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
> > +			  VDPA_CMD_DEV_CONFIG_GET);
> > +	if (!hdr)
> > +		return -EMSGSIZE;
> > +
> > +	if (nla_put_string(msg, VDPA_ATTR_DEV_NAME, dev_name(&vdev-
> >dev))) {
> > +		err = -EMSGSIZE;
> > +		goto msg_err;
> > +	}
> > +
> > +	device_id = vdev->config->get_device_id(vdev);
> > +	if (nla_put_u32(msg, VDPA_ATTR_DEV_ID, device_id)) {
> > +		err = -EMSGSIZE;
> > +		goto msg_err;
> > +	}
> > +
> > +	switch (device_id) {
> > +	case VIRTIO_ID_NET:
> > +		err = vdpa_dev_net_config_fill(vdev, msg);
> > +		break;
> > +	default:
> > +		err = -EOPNOTSUPP;
> > +		break;
> > +	}
> > +	if (err)
> > +		goto msg_err;
> > +
> > +	genlmsg_end(msg, hdr);
> > +	return 0;
> > +
> > +msg_err:
> > +	genlmsg_cancel(msg, hdr);
> > +	return err;
> > +}
> > +
> > +static int vdpa_nl_cmd_dev_config_get_doit(struct sk_buff *skb, struct
> genl_info *info)
> > +{
> > +	struct vdpa_device *vdev;
> > +	struct sk_buff *msg;
> > +	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]);
> > +	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > +	if (!msg)
> > +		return -ENOMEM;
> > +
> > +	mutex_lock(&vdpa_dev_mutex);
> > +	dev = bus_find_device(&vdpa_bus, NULL, devname,
> 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, "unmanaged vdpa
> device");
> > +		err = -EINVAL;
> > +		goto mdev_err;
> > +	}
> > +	err = vdpa_dev_config_fill(vdev, msg, info->snd_portid, info-
> >snd_seq,
> > +				   0, info->extack);
> > +	if (!err)
> > +		err = genlmsg_reply(msg, info);
> > +
> > +mdev_err:
> > +	put_device(dev);
> > +dev_err:
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +	if (err)
> > +		nlmsg_free(msg);
> > +	return err;
> > +}
> > +
> > +static int vdpa_dev_config_dump(struct device *dev, void *data)
> > +{
> > +	struct vdpa_device *vdev = container_of(dev, struct vdpa_device,
> dev);
> > +	struct vdpa_dev_dump_info *info = data;
> > +	int err;
> > +
> > +	if (!vdev->mdev)
> > +		return 0;
> > +	if (info->idx < info->start_idx) {
> > +		info->idx++;
> > +		return 0;
> > +	}
> > +	err = vdpa_dev_config_fill(vdev, info->msg, NETLINK_CB(info->cb-
> >skb).portid,
> > +				   info->cb->nlh->nlmsg_seq, NLM_F_MULTI,
> > +				   info->cb->extack);
> > +	if (err)
> > +		return err;
> > +
> > +	info->idx++;
> > +	return 0;
> > +}
> > +
> > +static int
> > +vdpa_nl_cmd_dev_config_get_dumpit(struct sk_buff *msg, struct
> netlink_callback *cb)
> > +{
> > +	struct vdpa_dev_dump_info info;
> > +
> > +	info.msg = msg;
> > +	info.cb = cb;
> > +	info.start_idx = cb->args[0];
> > +	info.idx = 0;
> > +
> > +	mutex_lock(&vdpa_dev_mutex);
> > +	bus_for_each_dev(&vdpa_bus, NULL, &info,
> vdpa_dev_config_dump);
> > +	mutex_unlock(&vdpa_dev_mutex);
> > +	cb->args[0] = info.idx;
> > +	return msg->len;
> > +}
> > +
> >   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 },
> > @@ -674,6 +880,12 @@ static const struct genl_ops vdpa_nl_ops[] = {
> >   		.doit = vdpa_nl_cmd_dev_get_doit,
> >   		.dumpit = vdpa_nl_cmd_dev_get_dumpit,
> >   	},
> > +	{
> > +		.cmd = VDPA_CMD_DEV_CONFIG_GET,
> > +		.validate = GENL_DONT_VALIDATE_STRICT |
> GENL_DONT_VALIDATE_DUMP,
> > +		.doit = vdpa_nl_cmd_dev_config_get_doit,
> > +		.dumpit = vdpa_nl_cmd_dev_config_get_dumpit,
> > +	},
> >   };
> >
> >   static struct genl_family vdpa_nl_family __ro_after_init = {
> > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
> > index 993d99519452..bf104f9f461a 100644
> > --- a/include/linux/vdpa.h
> > +++ b/include/linux/vdpa.h
> > @@ -42,6 +42,7 @@ struct vdpa_mgmt_dev;
> >    * @dev: underlying device
> >    * @dma_dev: the actual device that is performing DMA
> >    * @config: the configuration ops for this device.
> > + * @cf_mutex: Protects get and set access to features and configuration
> layout.
> >    * @index: device index
> >    * @features_valid: were features initialized? for legacy guests
> >    * @nvqs: maximum number of supported virtqueues
> > @@ -52,6 +53,7 @@ struct vdpa_device {
> >   	struct device dev;
> >   	struct device *dma_dev;
> >   	const struct vdpa_config_ops *config;
> > +	struct mutex cf_mutex; /* Protects get/set config and features */
> >   	unsigned int index;
> >   	bool features_valid;
> >   	int nvqs;
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 66a41e4ec163..5c31ecc3b956 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -17,6 +17,7 @@ enum vdpa_command {
> >   	VDPA_CMD_DEV_NEW,
> >   	VDPA_CMD_DEV_DEL,
> >   	VDPA_CMD_DEV_GET,		/* can dump */
> > +	VDPA_CMD_DEV_CONFIG_GET,	/* can dump */
> >   };
> >
> >   enum vdpa_attr {
> > @@ -33,6 +34,16 @@ enum vdpa_attr {
> >   	VDPA_ATTR_DEV_MAX_VQS,			/* u32 */
> >   	VDPA_ATTR_DEV_MAX_VQ_SIZE,		/* u16 */
> >
> > +	VDPA_ATTR_DEV_NET_CFG_MACADDR,		/* binary */
> > +	VDPA_ATTR_DEV_NET_STATUS,		/* u8 */
> > +	VDPA_ATTR_DEV_NET_CFG_MAX_VQP,		/* u16 */
> > +	VDPA_ATTR_DEV_NET_CFG_MTU,		/* u16 */
> > +	VDPA_ATTR_DEV_NET_CFG_SPEED,		/* u16 */
> > +	VDPA_ATTR_DEV_NET_CFG_DUPLEX,		/* u16 */
> > +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN,	/* u8 */
> > +	VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN,	/* u16 */
> > +	VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES,	/* u32 */
> 
> 
> Is it better to use a separate enum for net specific attributes?
> 
Yes, because they are only net specific.
I guess it is related to your below question.

> Another question (sorry if it has been asked before). Can we simply
> return the config (binary) to the userspace, then usespace can use the
> existing uAPI like virtio_net_config plus the feature to explain the config?
> 
We did discuss in v2.
Usually returning the whole blob and parsing is not desired via netlink.
Returning individual fields give the full flexibility to return only the valid fields.
Otherwise we need to implement another bitmask too to tell which fields from the struct are valid and share with user space.
Returning individual fields is the widely used approach.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization