RE: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute

[Parav Pandit]

> From: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> Sent: Tuesday, January 31, 2023 6:22 PM
> 
> With device feature provisioning, there's a chance for misconfiguration that the
> vdpa feature attribute supplied in 'vdpa dev add' command doesn't get
> selected on the device_features to be provisioned. For instance, when a @mac
> attribute is specified, the corresponding feature bit _F_MAC in device_features
> should be set for consistency. If there's conflict on provisioned features against
> the attribute, it should be treated as an error to fail the ambiguous command.
> Noted the opposite is not necessarily true, for e.g. it's okay to have _F_MAC set
> in device_features without providing a corresponding @mac attribute, in which
> case the vdpa vendor driver could load certain default value for attribute that is
> not explicitly specified.
> 
> Generalize this check in vdpa core so that there's no duplicate code in each
> vendor driver.
> 
> Signed-off-by: Si-Wei Liu <si-wei.liu@xxxxxxxxxx>
> ---
>  drivers/vdpa/vdpa.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 21c8aa3..1eba978
> 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct
> sk_buff *skb, struct genl_info *i
>  		config.mask |=
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
>  	}
>  	if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
> +		u64 missing = 0x0ULL;
> +
>  		config.device_features =
>  			nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
> +			missing |= BIT_ULL(VIRTIO_NET_F_MAC);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
> +			missing |= BIT_ULL(VIRTIO_NET_F_MTU);
> +		if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
> +		    config.net.max_vq_pairs > 1 &&
> +		    !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> +			missing |= BIT_ULL(VIRTIO_NET_F_MQ);
> +		if (missing) {
> +			NL_SET_ERR_MSG_FMT_MOD(info->extack,
> +					       "Missing features 0x%llx for
> provided attributes",
> +					       missing);
> +			return -EINVAL;
> +		}
>  		config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
>  	}
> 
> --
> 1.8.3.1
Vdpa this layer can likely derive the feature bits for the supplied config fields so that user doesn't need to keep track of both.
Only those feature bits which are unrelated to any config, is what user should be setting.