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.
Jason Wang
2021-Feb-26 08:26 UTC
[PATCH linux-next 2/9] vdpa: Introduce query of device config layout
On 2021/2/26 1:32 ??, Parav Pandit wrote:> >> 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().Probably. And the set_config() should be synchronized with the requrest that comes from vDPA bus. This makes me think whether we should go back to two phases method, create and enable. The vDPA device is only registred after enabling, then there's no need to sync with vDPA bus, and mgmt is not allowed to change config after enalbing?> >> >>> + 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.I agree. Thanks