Si-Wei Liu
2021-Dec-14 00:47 UTC
[PATCH v1 3/7] vdpa: Allow to configure max data virtqueues
On 12/13/2021 5:07 AM, Eli Cohen wrote:> On Mon, Dec 13, 2021 at 08:44:53AM +0200, Eli Cohen wrote: >> On Fri, Dec 10, 2021 at 10:29:43AM +0800, Jason Wang wrote: >>> On Fri, Dec 10, 2021 at 5:51 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote: >>>> >>>> >>>> On 12/8/2021 9:36 PM, Jason Wang wrote: >>>>> On Thu, Dec 9, 2021 at 8:25 AM Si-Wei Liu <si-wei.liu at oracle.com> wrote: >>>>>> >>>>>> On 12/8/2021 12:14 PM, Eli Cohen wrote: >>>>>>> Add netlink support to configure the max virtqueue pairs for a device. >>>>>>> At least one pair is required. The maximum is dictated by the device. >>>>>>> >>>>>>> Example: >>>>>>> >>>>>>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 5 >>>>>> Not this patch, but I think there should be a mega attribute defined >>>>>> ahead to specify the virtio class/type to create vdpa instance with. >>>>>> Like the existing ones e.g. mac_addr and mtu, max_vqp is also vdpa net >>>>>> specific attribute. >>>>> Probably, but this only works for the case when a single parent is >>>>> allowed to create different types of devices. It looks to me the >>>>> current model to have a per type parent. >>>> Yes, that is the current situation and implementation, although the >>>> model does allow multi-type parent through >>>> VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES. >>> Right, so I agree with you, we need to specify the class first. >>> >> Or maybe fail device creation if the parent device does not support net >> class. > BTW, we already have the mechanism in place to enforce that. If a device > does not support configuration of max_vqp, it will not set > VDPA_ATTR_DEV_NET_CFG_MACADDR in its config_attr_mask so you will not be > able add it if you attempt to specify max_vqp upon creation.Yes, but that is another level of validation down to the specific class of vdpa config. It should be completely fine for user not to specify max_vqp or mac address when vdpa is created. Though they may not create the expected vdpa class as they wish in the first place if the class validation is missing. Having said, I guess it would be great if users who want to create vdpa can get known of the parent's supported class and capability ahead through some mgmtdev command, like what I suggested earlier: $ vdpa mgmtdev capab show pci/0000:41:00.2: ? class: net ???? mac: off ???? mtu: on ???? max_mtu: 9000 ???? max_vqp: 1 auxiliary/mlx5_core.sf.1: ? class: net ???? mac: on ???? mtu: off ???? max_mtu: 1500 ???? max_vqp: 256 Regards, -Siwei> >> You check first the supported class with "vdpa mgmtdev show" and if net >> is supported you may use max_vqp. >> >>>> Regardless, even though with >>>> single-type parent, so far on vdpa creation there's no validation done >>>> yet against which class/type the parent can support. The max-vqp is >>>> essentially vdpa network device only, but command line users don't have >>>> a means to infer it is structured this way, and/or which vdpa parent may >>>> support this config attribute. That said, were the command line usage >>>> structured like below, I would have less worry. >>>> >>>> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 net.max_vqp 5 >>> It might be easier to specify class as a dedicated parameter, since we >>> may want to specify mtu and mac. >>> >>> Thanks >>> >>>> Alternatively, if the goal is to keep the attribute flat, we may show >>>> the mapping for the parent capability and the supported class: >>>> >>>> $ vdpa mgmtdev capab show >>>> pci/0000:41:00.2: >>>> class: net >>>> mac: off >>>> mtu: on >>>> max_mtu: 9000 >>>> max_vqp: 1 >>>> auxiliary/mlx5_core.sf.1: >>>> class: net >>>> mac: on >>>> mtu: off >>>> max_mtu: 1500 >>>> max_vqp: 256 >>>> >>>> Thanks, >>>> -Siwei >>>> >>>>> Thanks >>>>> >>>>>> -Siwei >>>>>> >>>>>>> Signed-off-by: Eli Cohen <elic at nvidia.com> >>>>>>> --- >>>>>>> v0 -> v1: >>>>>>> 1. fix typo >>>>>>> 2. move max_vq_pairs to net specific struct >>>>>>> >>>>>>> drivers/vdpa/vdpa.c | 14 +++++++++++++- >>>>>>> include/linux/vdpa.h | 1 + >>>>>>> 2 files changed, 14 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c >>>>>>> index c37d384c0f33..3bf016e03512 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)) >>>>>>> >>>>>>> static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info) >>>>>>> { >>>>>>> @@ -506,6 +507,17 @@ 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.net.max_vq_pairs >>>>>>> + nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]); >>>>>>> + if (!config.net.max_vq_pairs) { >>>>>>> + NL_SET_ERR_MSG_MOD(info->extack, >>>>>>> + "At least one pair of VQs is required"); >>>>>>> + err = -EINVAL; >>>>>>> + goto err; >>>>>>> + } >>>>>>> + 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 db24317d61b6..b62032573780 100644 >>>>>>> --- a/include/linux/vdpa.h >>>>>>> +++ b/include/linux/vdpa.h >>>>>>> @@ -99,6 +99,7 @@ struct vdpa_dev_set_config { >>>>>>> struct { >>>>>>> u8 mac[ETH_ALEN]; >>>>>>> u16 mtu; >>>>>>> + u16 max_vq_pairs; >>>>>>> } net; >>>>>>> u64 mask; >>>>>>> };