On Mon, Mar 27, 2023 at 03:54:34PM +0300, Eli Cohen wrote:
>
> On 27/03/2023 15:41, Michael S. Tsirkin wrote:
> > On Mon, Mar 20, 2023 at 10:01:05AM +0200, Eli Cohen wrote:
> > > Current code ignores link state updates if VIRTIO_NET_F_STATUS was not
> > > negotiated. However, link state updates could be received before feature
> > > negotiation was completed , therefore causing link state events to be
> > > lost, possibly leaving the link state down.
> > >
> > > Add code to detect if VIRTIO_NET_F_STATUS was set and update the link
> > > state. We add a spinlock to serialize updated to config.status to
> > > maintain its integrity.
> > >
> > > Fixes: 033779a708f0 ("vdpa/mlx5: make MTU/STATUS presence conditional on feature bits")
> > > Signed-off-by: Eli Cohen <elic@xxxxxxxxxx>
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 91 ++++++++++++++++++++-----------
> > > drivers/vdpa/mlx5/net/mlx5_vnet.h | 2 +
> > > 2 files changed, 60 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index 520646ae7fa0..20d415e25aeb 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -2298,10 +2298,62 @@ static void update_cvq_info(struct mlx5_vdpa_dev *mvdev)
> > > }
> > > }
> > > +static bool f_status_was_set(u64 old, u64 new)
> > the name is exact reverse of what it does is it not?
> > it is true if status was not set and is being set.
>
> I am tracking feature negotiation to check if VIRTIO_NET_F_STATUS *just
> became set* and unconditionally update link state.
> If you still think the name choice is not good, I can change it.
this is where you use the function currently.
yes the "new" value is currently the value that
was already saved before the call to the function.
but the function itself does not care.
but it says nothing about what it does itself.
it gets old and new and tells you whether VIRTIO_NET_F_STATUS
flipped from 0 in old to 1 in new.
I am not sure why you need helper, you can write
it in a single statement as:
(~old & new & BIT_ULL(VIRTIO_NET_F_STATUS))
or here practically:
if (~cur_features & ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS))
(maybe use a shorter name for cur_features to fit in 80 columns).
so I would just open-code, but if you do write a helper then it
should make sense by itself and be reusable.
>
> >
> > > +{
> > > + if (!(old & BIT_ULL(VIRTIO_NET_F_STATUS)) &&
> > > + (new & BIT_ULL(VIRTIO_NET_F_STATUS)))
> > > + return true;
> > > +
> > > + return false;
> > > +}
> > > +
> > > +static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16 vport)
> > > +{
> > > + u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> > > + u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> > > + int err;
> > > +
> > > + MLX5_SET(query_vport_state_in, in, opcode, MLX5_CMD_OP_QUERY_VPORT_STATE);
> > > + MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> > > + MLX5_SET(query_vport_state_in, in, vport_number, vport);
> > > + if (vport)
> > > + MLX5_SET(query_vport_state_in, in, other_vport, 1);
> > > +
> > > + err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> > > + if (err)
> > > + return 0;
> > > +
> > > + return MLX5_GET(query_vport_state_out, out, state);
> > > +}
> > > +
> > > +static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> > > +{
> > > + if (query_vport_state(mvdev->mdev, MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) ==
> > > + VPORT_STATE_UP)
> > > + return true;
> > > +
> > > + return false;
> > > +}
> > > +
> > > +static void update_link_state(struct mlx5_vdpa_dev *mvdev)
> > > +{
> > > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + bool up;
> > > +
> > > + up = get_link_state(mvdev);
> > > + spin_lock(&ndev->status_lock);
> > > + if (up)
> > > + ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> > > + else
> > > + ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> > > + spin_unlock(&ndev->status_lock);
> > > +}
> > > +
> > > static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > > {
> > > struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > > struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > > + u64 cur_features;
> > > int err;
> > > print_features(mvdev, features, true);
> > > @@ -2310,7 +2362,11 @@ static int mlx5_vdpa_set_driver_features(struct vdpa_device *vdev, u64 features)
> > > if (err)
> > > return err;
> > > + cur_features = ndev->mvdev.actual_features;
> > > ndev->mvdev.actual_features = features & ndev->mvdev.mlx_features;
> > > + if (f_status_was_set(cur_features, ndev->mvdev.actual_features))
> > > + update_link_state(mvdev);
> > > +
> > > if (ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_MQ))
> > > ndev->rqt_size = mlx5vdpa16_to_cpu(mvdev, ndev->config.max_virtqueue_pairs);
> > > else
> > > @@ -2995,34 +3051,6 @@ struct mlx5_vdpa_mgmtdev {
> > > struct mlx5_vdpa_net *ndev;
> > > };
> > > -static u8 query_vport_state(struct mlx5_core_dev *mdev, u8 opmod, u16 vport)
> > > -{
> > > - u32 out[MLX5_ST_SZ_DW(query_vport_state_out)] = {};
> > > - u32 in[MLX5_ST_SZ_DW(query_vport_state_in)] = {};
> > > - int err;
> > > -
> > > - MLX5_SET(query_vport_state_in, in, opcode, MLX5_CMD_OP_QUERY_VPORT_STATE);
> > > - MLX5_SET(query_vport_state_in, in, op_mod, opmod);
> > > - MLX5_SET(query_vport_state_in, in, vport_number, vport);
> > > - if (vport)
> > > - MLX5_SET(query_vport_state_in, in, other_vport, 1);
> > > -
> > > - err = mlx5_cmd_exec_inout(mdev, query_vport_state, in, out);
> > > - if (err)
> > > - return 0;
> > > -
> > > - return MLX5_GET(query_vport_state_out, out, state);
> > > -}
> > > -
> > > -static bool get_link_state(struct mlx5_vdpa_dev *mvdev)
> > > -{
> > > - if (query_vport_state(mvdev->mdev, MLX5_VPORT_STATE_OP_MOD_VNIC_VPORT, 0) ==
> > > - VPORT_STATE_UP)
> > > - return true;
> > > -
> > > - return false;
> > > -}
> > > -
> > > static void update_carrier(struct work_struct *work)
> > > {
> > > struct mlx5_vdpa_wq_ent *wqent;
> > > @@ -3032,11 +3060,7 @@ static void update_carrier(struct work_struct *work)
> > > wqent = container_of(work, struct mlx5_vdpa_wq_ent, work);
> > > mvdev = wqent->mvdev;
> > > ndev = to_mlx5_vdpa_ndev(mvdev);
> > > - if (get_link_state(mvdev))
> > > - ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> > > - else
> > > - ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> > > -
> > > + update_link_state(mvdev);
> > > if (ndev->nb_registered && ndev->config_cb.callback)
> > > ndev->config_cb.callback(ndev->config_cb.private);
> > > @@ -3172,6 +3196,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> > > init_mvqs(ndev);
> > > init_rwsem(&ndev->reslock);
> > > + spin_lock_init(&ndev->status_lock);
> > > config = &ndev->config;
> > > if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.h b/drivers/vdpa/mlx5/net/mlx5_vnet.h
> > > index c90a89e1de4d..3666bbaa8822 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.h
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.h
> > > @@ -50,6 +50,8 @@ struct mlx5_vdpa_net {
> > > struct mlx5_vdpa_wq_ent cvq_ent;
> > > struct hlist_head macvlan_hash[MLX5V_MACVLAN_SIZE];
> > > struct dentry *debugfs;
> > > + /* serialize link status updates */
> > > + spinlock_t status_lock;
> > > };
> > > struct mlx5_vdpa_counter {
> > > --
> > > 2.38.1
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
