Michael S. Tsirkin
2022-Jan-13 06:57 UTC
[PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ
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? pls explain the motivation. which config are you trying to fix what is current and expected behaviour.> > 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.> Signed-off-by: Si-Wei Liu<si-wei.liu at 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
Si-Wei Liu
2022-Jan-14 08:51 UTC
[PATCH 2/3] vdpa/mlx5: set_features should nack MQ if no CTRL_VQ
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 at 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