On 12/1/2021 2:03 AM, Eli Cohen wrote:> On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:
>>
>> On 11/30/2021 1:48 AM, Eli Cohen wrote:
>>> Allow to configure the max virtqueues for a device.
>>>
>>> Signed-off-by: Eli Cohen <elic at nvidia.com>
>>> ---
>>> drivers/vdpa/vdpa.c | 16 +++++++++++++++-
>>> include/linux/vdpa.h | 1 +
>>> 2 files changed, 16 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 7332a74a4b00..e185ec2ee851 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff
*msg, struct netlink_callback *cb)
>>> }
>>> #define VDPA_DEV_NET_ATTRS_MASK ((1 <<
VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
>>> - (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
>>> + (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
>> It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data
virtqueues
>> instead of # of data virtqueue pairs)? Not sure what's possible use
of
>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to dump/display the config space
>> max_virtqueue_pairs value (u16, 1-32768) for virtio-net? Why
there's such
>> quasi-duplicate attribute introduced in the first place?
>>
> VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals whatever
> passed to _vdpa_register_device(). The latter depends on the value
> provided by (struct vdpa_dev_set_config).max_virtqueues. It's the only
> way to let the user know if there are other virtqueues besides the data
> virtqueues. For example, if we support control virtqueue and configured
> 2 data QPs, we will return 5.
That's right. Then let VDPA_ATTR_DEV_MAX_VQS continue to be read-only as
is.>
> Maybe we should add attributes to add aditional virtqueues like control
> virtqueue and their index. They could be returned by
> vdpa_dev_net_config_fill().
Probably this info would need to return to QEMU for proper virtq
modeling via vDPA ioctls rather than just netlink API. Agreed it's
virtio-net specific config.> VDPA_ATTR_DEV_NET_CFG_MAX_VQP seems the right choice since it is used to
> provid the requested number of data virtqueues when creating the device.
> Maybe we need to change the name to VDPA_ATTR_DEV_NET_CFG_MAX_VQ and
> then this patch makes more sense.
I would just keep the name for VDPA_ATTR_DEV_NET_CFG_MAX_VQP and have
user configure the number of data queue pairs instead. Very few virtio
types come with virtqueue in pairs, so this has to be vdpa net specific
config.
Thanks,
-Siwei
>
>
> I will post another series to address all these issues.
>
>> Not even sure VDPA_ATTR_DEV_MAX_VQS by definition should include other
>> virtqueues as well: such as control virtqueue or event virtqueue. Hence
the
>> name will be more applicable to vdpa devices of other virtio type than
just
>> virtio-net. Otherwise I would think this attribute is slightly misnamed
>> (max_data_vqs might be a proper name).
>>
>>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb,
struct genl_info *info)
>>> {
>>> @@ -506,6 +507,19 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
sk_buff *skb, struct genl_info *i
>>> nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
>>> config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
>>> }
>>> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
>>> + config.max_virtqueues =
nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
>>> + if (config.max_virtqueues < 2) {
>>> + NL_SET_ERR_MSG_MOD(info->extack, "At least two
virtqueues are required");
>>> + return -EINVAL;
>>> + }
>>> + if ((config.max_virtqueues - 1) & config.max_virtqueues) {
>>> + NL_SET_ERR_MSG_MOD(info->extack,
>>> + "Must provide power of two number of
virtqueues");
>> Why there's such limitation for the number of vDPA virtqueues? I
thought the
>> software virtio doesn't have this limitation (power of two).
> Right, the limitation comes from mlx5_vdpa. I will post a patch later to
> remove that limitation.
>
>> -Siwei
>>
>>> + return -EINVAL;
>>> + }
>>> + config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>>> + }
>>> /* Skip checking capability if user didn't prefer to
configure any
>>> * device networking attributes. It is likely that user might
have used
>>> diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
>>> index c3011ccda430..2f0b09c6d1ae 100644
>>> --- a/include/linux/vdpa.h
>>> +++ b/include/linux/vdpa.h
>>> @@ -101,6 +101,7 @@ struct vdpa_dev_set_config {
>>> u16 mtu;
>>> } net;
>>> u64 mask;
>>> + u16 max_virtqueues;
>>> };
>>> /**