Re: [PATCH 2/3] mlx5_vdpa: fix feature negotiation across device reset

On Mon, Feb 08, 2021 at 05:20:11PM -0800, Si-Wei Liu wrote:

On 2/7/2021 9:35 PM, Eli Cohen wrote:

quoted

On Sat, Feb 06, 2021 at 04:29:23AM -0800, Si-Wei Liu wrote:

quoted

The mlx_features denotes the capability for which
set of virtio features is supported by device. In
principle, this field needs not be cleared during
virtio device reset, as this capability is static
and does not change across reset.

In fact, the current code may have the assumption
that mlx_features can be reloaded from firmware
via the .get_features ops after device is reset
(via the .set_status ops), which is unfortunately
not true. The userspace VMM might save a copy
of backend capable features and won't call into
kernel again to get it on reset. This causes all
virtio features getting disabled on newly created
virtqs after device reset, while guest would hold
mismatched view of available features. For e.g.,
the guest may still assume tx checksum offload
is available after reset and feature negotiation,
causing frames with bogus (incomplete) checksum
transmitted on the wire.

Signed-off-by: Si-Wei Liu <redacted>
---
  drivers/vdpa/mlx5/net/mlx5_vnet.c | 1 -
  1 file changed, 1 deletion(-)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b8416c4..aa6f8cd 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c

@@ -1788,7 +1788,6 @@ static void mlx5_vdpa_set_status(struct vdpa_device *vdev, u8 status)
  		clear_virtqueues(ndev);
  		mlx5_vdpa_destroy_mr(&ndev->mvdev);
  		ndev->mvdev.status = 0;
-		ndev->mvdev.mlx_features = 0;
  		++mvdev->generation;
  		return;
  	}

Since we assume that device capabilities don't change, I think I would
get the features through a call done in mlx5v_probe after the netdev
object is created and change mlx5_vdpa_get_features() to just return
ndev->mvdev.mlx_features.

Yep, it makes sense. Will post a revised patch.

So I'm waiting for v2 of this patchset. Please make sure to post a cover letter
with an overall description.

If vdpa tool allows
reconfiguration post probing, the code has to be reconciled then.

quoted

Did you actually see this issue in action? If you did, can you share
with us how you trigerred this?

Issue is indeed seen in action. The mismatched tx-checksum offload as
described in the commit message was one of such examples. You would need a
guest reboot though (triggering device reset via the .set_status ops and
zero'ed mlx_features was loaded to deduce new actual_features for creating
the h/w virtq object) for repro.

-Siwei

quoted

quoted

-- 
1.8.3.1