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@xxxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
