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

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 2/1/2023 9:05 PM, Parav Pandit wrote:
>
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> 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@oracle.com>
>> ---
>>   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.
It's not I can't do this, but Jason wanted to have clear semantics 
around migration compatibility for the driver, and for that users have 
to explicitly provide device_features that we may define new driver 
behavior (rather that inheritance which is implicit and not uniformly 
define across drivers) for compatibility using the new uAPI.

Thanks,
-Siwei




_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization