Re: [PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ

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

On 1/12/2022 10:57 PM, Michael S. Tsirkin wrote:
> On Thu, Jan 13, 2022 at 12:10:50AM -0500, Si-Wei Liu wrote:
>> Made corresponding change per spec:
>
>> The device MUST NOT offer a feature which requires another feature
>> which was not offered.
> Says nothing about the driver though, and you seem to be
> doing things to driver features?
Yes, it's about validation for driver features, though the spec doesn't 
have clear way how to deal with this situation. I guess this in reality 
leaves quite some space for the implementation. To step back, in recent 
days with latent spec revision for feature negotiation due to endianness 
and MTU validation, what do we expect device to work if the driver is 
not compliant and comes up with invalid features set? To clear a subset 
of driver features unsupported by the device, such that driver may 
figure out by reading it from device config space later on? Or fail the 
entire features and have driver to re-try a different setting? Do you 
feel its possible for device to clear a subset of invalid or unsupported 
features sent down by the driver, which may allow driver continue to 
work without having to fail the entire feature negotiation?

The current userspace implementation in qemu may filter out invalid 
features from driver by clearing a subset and tailor it to fit what 
host/device can offer. I thought it should be safe to follow the 
existing practice. That way guest driver can get know of the effective 
features during feature negotiation, or after features_ok is set (that's 
what I call by "re-read" of features, sorry if I used the wrong term). 
Did I miss something? I can definitely add more explanation for the 
motivation, remove the reference to spec and delete the Fixes tag to 
avoid confusions. Do you think this would work?

Another option would be just return failure for the 
set_driver_features() call when seeing (MQ && !CTRL_VQ). Simple enough 
and easy to implement. Efficient to indicate which individual feature is 
failing? Probably not, driver has to retry a few times using binary 
search to know.

> pls explain the motivation. which config are you trying to
> fix what is current and expected behaviour.
The current mq code for mlx5_vdpa driver is written in the assumption 
that MQ must come together with CTRL_VQ. I would like to point out that 
right now there's nowhere in the host side even QEMU to guarantee this 
assumption would hold. Were there a malicious driver sending down MQ 
without CTRL_VQ, it would compromise various spots such as 
is_index_valid() and is_ctrl_vq_idx(). This doesn't end up with 
immediate panic or security loophole in the host currently, but still 
the chance for this being taken advantage of is not zero, especially 
when future code change is involved. You can say it's code cleanup, but 
the added check helps harden the crispy assumption and assures peace of 
mind.

>
>> Fixes: 52893733f2c5 ("vdpa/mlx5: Add multiqueue support")
>
> It's all theoretical right? Fixes really means
> "if you have commit ABC then you should pick this one up".
> not really appropriate for theoretical fixes.
Yeah. This was discovered in code review. Didn't see a real issue. I can 
remove the tag.

-Siwei
>
>> Signed-off-by: Si-Wei Liu<si-wei.liu@oracle.com>
>> ---
>>   drivers/vdpa/mlx5/net/mlx5_vnet.c | 16 +++++++++++++---
>>   1 file changed, 13 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b53603d..46d4deb 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1897,11 +1897,21 @@ static u64 mlx5_vdpa_get_device_features(struct vdpa_device *vdev)
>>   	return ndev->mvdev.mlx_features;
>>   }
>>   
>> -static int verify_min_features(struct mlx5_vdpa_dev *mvdev, u64 features)
>> +static int verify_driver_features(struct mlx5_vdpa_dev *mvdev, u64 *features)
>
> Good rename actually but document in commit log with an
> explanation.
>
>>   {
>> -	if (!(features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>> +	/* minimum features to expect */
>> +	if (!(*features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM)))
>>   		return -EOPNOTSUPP;
>>   
>> +	/* Double check features combination sent down by the driver.
>> +	 * NACK invalid feature due to the absence of depended feature.
> Pls rewrite this to make it grammatical.  There's no NACK in spec. What
> does this do? Fails to set FEATURES_OK?
>
>> +	 * Driver is expected to re-read the negotiated features once
>> +	 * return from set_driver_features.
> once return is ungrammatical. What to say here depends on what
> you mean by this, so I'm not sure.
>
>
> Here's text from spec:
>
> \item\label{itm:General Initialization And Device Operation /
> Device Initialization / Read feature bits} Read device feature bits, and write the subset of feature bits
>     understood by the OS and driver to the device.  During this step the
>     driver MAY read (but MUST NOT write) the device-specific configuration fields to check that it can support the device before accepting it.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Set FEATURES-OK} Set the FEATURES_OK status bit.  The driver MUST NOT accept
>     new feature bits after this step.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Re-read FEATURES-OK} Re-read \field{device status} to ensure the FEATURES_OK bit is still
>     set: otherwise, the device does not support our subset of features
>     and the device is unusable.
>
> \item\label{itm:General Initialization And Device Operation / Device Initialization / Device-specific Setup} Perform device-specific setup, including discovery of virtqueues for the
>     device, optional per-bus setup, reading and possibly writing the
>     device's virtio configuration space, and population of virtqueues.
>
> does not seem to talk about re-reading features.
> What did I miss?
>
>
>> +	 */
>
> This comment confuses more than it clarifies. I would
> - quote the spec
> - explain why does code do what it does specifically for these features
>
>> +	if ((*features & (BIT_ULL(VIRTIO_NET_F_MQ) | BIT_ULL(VIRTIO_NET_F_CTRL_VQ))) ==
>> +            BIT_ULL(VIRTIO_NET_F_MQ))
>> +		*features &= ~BIT_ULL(VIRTIO_NET_F_MQ);
>> +
>>   	return 0;
>>   }
>>   
>> @@ -1977,7 +1987,7 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
>>   
>>   	print_features(mvdev, features, true);
>>   
>> -	err = verify_min_features(mvdev, features);
>> +	err = verify_driver_features(mvdev, &features);
>>   	if (err)
>>   		return err;
>>   
>> -- 
>> 1.8.3.1

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