Jason Wang
2021-Feb-24 08:47 UTC
[PATCH linux-next 2/9] vdpa: Introduce query of device config layout
On 2021/2/24 2:18 ??, Parav Pandit wrote:> Introduce a command to query a device config layout. > > An example query of network vdpa device: > > $ vdpa dev add name bar mgmtdev vdpasim_net > > $ vdpa dev config show > bar: mac 00:35:09:19:48:05 link up link_announce false mtu 1500 speed 0 duplex 0 > > $ vdpa dev config show -jp > { > "config": { > "bar": { > "mac": "00:35:09:19:48:05", > "link ": "up", > "link_announce ": false, > "mtu": 1500, > "speed": 0, > "duplex": 0 > } > } > } > > Signed-off-by: Parav Pandit <parav at nvidia.com> > Reviewed-by: Eli Cohen <elic at nvidia.com> > --- > changelog: > v1->v2: > - read whole net config layout instead of individual fields > - added error extack for unmanaged vdpa device > --- > drivers/vdpa/vdpa.c | 181 ++++++++++++++++++++++++++++++++++++++ > include/uapi/linux/vdpa.h | 11 +++ > 2 files changed, 192 insertions(+) > > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 3d997b389345..cebbba500638 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -14,6 +14,8 @@ > #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); > /* A global mutex that protects vdpa management device and device level operations. */ > @@ -603,6 +605,179 @@ 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, > + const struct virtio_net_config *config) > +{ > + u32 hash_types; > + u16 rss_field; > + u64 features; > + > + features = vdev->config->get_features(vdev); > + if ((features & (1ULL << VIRTIO_NET_F_MQ)) == 0) > + return 0; > + > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > + config->max_virtqueue_pairs)) > + return -EMSGSIZE;We probably need to check VIRTIO_NET_F_RSS here.> + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN, > + config->rss_max_key_size)) > + return -EMSGSIZE; > + > + rss_field = le16_to_cpu(config->rss_max_key_size); > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, rss_field)) > + return -EMSGSIZE; > + > + hash_types = le32_to_cpu(config->supported_hash_types); > + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES, > + config->supported_hash_types)) > + return -EMSGSIZE; > + return 0; > +} > + > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg) > +{ > + struct virtio_net_config config = {}; > + > + vdev->config->get_config(vdev, 0, &config, sizeof(config));Do we need to sync with other possible get_config calls here?> + if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac), config.mac)) > + return -EMSGSIZE; > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status)) > + return -EMSGSIZE; > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu)) > + return -EMSGSIZE;And check VIRTIO_NET_F_SPEED_DUPLEX.> + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, config.speed)) > + return -EMSGSIZE; > + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, config.duplex)) > + return -EMSGSIZE; > + > + return vdpa_dev_net_mq_config_fill(vdev, msg, &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; > + }Though it's fine but it looks to me mdev is not required to work here.> + 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 }, > @@ -634,6 +809,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/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 */ > + > /* new attributes must be added above here */ > VDPA_ATTR_MAX, > };
Parav Pandit
2021-Feb-26 05:32 UTC
[PATCH linux-next 2/9] vdpa: Introduce query of device config layout
> From: Jason Wang <jasowang at redhat.com> > Sent: Wednesday, February 24, 2021 2:18 PM > > On 2021/2/24 2:18 ??, Parav Pandit wrote: > > + > > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, > > + config->max_virtqueue_pairs)) > > + return -EMSGSIZE; > > > We probably need to check VIRTIO_NET_F_RSS here.Yes. Will use it in v2.> > > > + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_KEY_LEN, > > + config->rss_max_key_size)) > > + return -EMSGSIZE; > > + > > + rss_field = le16_to_cpu(config->rss_max_key_size); > > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_RSS_MAX_IT_LEN, > rss_field)) > > + return -EMSGSIZE; > > + > > + hash_types = le32_to_cpu(config->supported_hash_types); > > + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_RSS_HASH_TYPES, > > + config->supported_hash_types)) > > + return -EMSGSIZE; > > + return 0; > > +} > > + > > +static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct > > +sk_buff *msg) { > > + struct virtio_net_config config = {}; > > + > > + vdev->config->get_config(vdev, 0, &config, sizeof(config)); > > > Do we need to sync with other possible get_config calls here?To readers of get_config() is ok. No need to sync. However, I think set_config() and get_config() should sync otherwise get_config can get partial value. Will probably have to add vdpa_device->config_access_lock().> > > > + if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, > sizeof(config.mac), config.mac)) > > + return -EMSGSIZE; > > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, config.status)) > > + return -EMSGSIZE; > > + if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, config.mtu)) > > + return -EMSGSIZE; > > > And check VIRTIO_NET_F_SPEED_DUPLEX.Yes, will do.> > > > + if (nla_put_u32(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, > config.speed)) > > + return -EMSGSIZE; > > + if (nla_put_u8(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, > config.duplex)) > > + return -EMSGSIZE; > > + > > + return vdpa_dev_net_mq_config_fill(vdev, msg, &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; > > + } > > > Though it's fine but it looks to me mdev is not required to work here. >Yes, mdev is not required here. However it was little weird to allow $ vdpa dev config show, but not allow $ vdpa dev show. It transition phase right now. Subsequently will able to allow this as well. After this series only ifc driver will be left to convert to user created devices. At that point, this checks will have chance to simplify.