Virtualization - Mar 2021 - [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

> From: Jason Wang <jasowang at redhat.com>
> Sent: Friday, February 26, 2021 1:56 PM
> 
> 
> 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. 
Yes, a rw semaphore is good enough.
Device config fields can be change from the device side anyway, such as link
status anyway.
Sync using lock shouldn?t be problem. 
> 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?
> 
In that case we should be able to disable it as well. Disable should take the
device of the bus.
I find it weird to have this model to linger around the device without anchoring
on the bus.
For example device is not yet enabled, so it is not anchored on the bus, but its
name is still have to remain unique.

Do we have to anchor vdpa device on the bus? Can vdpa be a class?

On 2021/2/26 4:50 ??, Parav Pandit wrote:
>
>> From: Jason Wang <jasowang at redhat.com>
>> Sent: Friday, February 26, 2021 1:56 PM
>>
>>
>> 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.
> Yes, a rw semaphore is good enough.
> Device config fields can be change from the device side anyway, such as
link status anyway.
> Sync using lock shouldn?t be problem.
>
>> 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?
>>
> In that case we should be able to disable it as well. Disable should take
the device of the bus.
> I find it weird to have this model to linger around the device without
anchoring on the bus.
> For example device is not yet enabled, so it is not anchored on the bus,
but its name is still have to remain unique.

Can we do some synchornization between vdpa device allocation and vdpa 
device registier to solve the naming issue?

>
> Do we have to anchor vdpa device on the bus? Can vdpa be a class?

I think not, it's a bus that is expected to be bound to different driver 
instead of a subsystem.

Thanks