[vhost,3/7] vdpa/mlx5: Allow modifying multiple vq fields in one modify command
Commit Message
Add a bitmask variable that tracks hw vq field changes that
are supposed to be modified on next hw vq change command.
This will be useful to set multiple vq fields when resuming the vq.
The state needs to remain as a parameter as it doesn't make sense to
make it part of the vq struct: fw_state is updated only after a
successful command.
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
1 file changed, 40 insertions(+), 8 deletions(-)
Comments
On Fri, Dec 1, 2023 at 11:49 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> Add a bitmask variable that tracks hw vq field changes that
> are supposed to be modified on next hw vq change command.
>
> This will be useful to set multiple vq fields when resuming the vq.
>
> The state needs to remain as a parameter as it doesn't make sense to
> make it part of the vq struct: fw_state is updated only after a
> successful command.
>
I don't get this paragraph, "modified_fields" is a member of
"mlx5_vdpa_virtqueue". Am I missing something?
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
> 1 file changed, 40 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 12ac3397f39b..d06285e46fe2 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
> u16 avail_idx;
> u16 used_idx;
> int fw_state;
> +
> + u64 modified_fields;
> +
> struct msi_map map;
>
> /* keep last in the struct */
> @@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
> }
> }
>
> -static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
> +static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
> +{
> + /* Only state is always modifiable */
> + if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
> + return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
> + mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
> +
> + return true;
> +}
> +
> +static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> + struct mlx5_vdpa_virtqueue *mvq,
> + int state)
> {
> int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
> u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
> @@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> return 0;
>
> + if (!modifiable_virtqueue_fields(mvq))
> + return -EINVAL;
> +
I'm ok with this change, but since modified_fields is (or will be) a
bitmap tracking changes to state, addresses, mkey, etc, doesn't have
more sense to check it like:
if (modified_fields & 1<<change_1_flag)
// perform change 1
if (modified_fields & 1<<change_2_flag)
// perform change 2
if (modified_fields & 1<<change_3_flag)
// perform change 13
---
Instead of:
if (!modified_fields)
return
if (modified_fields & 1<<change_1_flag)
// perform change 1
if (modified_fields & 1<<change_2_flag)
// perform change 2
// perform change 3, no checking, as it is the only possible value of
modified_fields
---
Or am I missing something?
The rest looks good to me.
> if (!is_valid_state_change(mvq->fw_state, state))
> return -EINVAL;
>
> @@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
>
> obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> - MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
> - MLX5_VIRTQ_MODIFY_MASK_STATE);
> - MLX5_SET(virtio_net_q_object, obj_context, state, state);
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
> + MLX5_SET(virtio_net_q_object, obj_context, state, state);
> +
> + MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> kfree(in);
> if (!err)
> mvq->fw_state = state;
>
> + mvq->modified_fields = 0;
> +
> return err;
> }
>
> +static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
> + struct mlx5_vdpa_virtqueue *mvq,
> + unsigned int state)
> +{
> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
> + return modify_virtqueue(ndev, mvq, state);
> +}
> +
> static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> {
> u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> @@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> goto err_vq;
>
> if (mvq->ready) {
> - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> if (err) {
> mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
> idx, err);
> @@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> return;
>
> - if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> + if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");
>
> if (query_virtqueue(ndev, mvq, &attr)) {
> @@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> return;
>
> suspend_vq(ndev, mvq);
> + mvq->modified_fields = 0;
> destroy_virtqueue(ndev, mvq);
> dealloc_vector(ndev, mvq);
> counter_set_dealloc(ndev, mvq);
> @@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> if (!ready) {
> suspend_vq(ndev, mvq);
> } else {
> - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> if (err) {
> mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
> ready = false;
> @@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> {
> int i;
>
> - for (i = 0; i < ndev->mvdev.max_vqs; i++)
> + for (i = 0; i < ndev->mvdev.max_vqs; i++) {
> ndev->vqs[i].ready = false;
> + ndev->vqs[i].modified_fields = 0;
> + }
>
> ndev->mvdev.cvq.ready = false;
> }
> --
> 2.42.0
>
On 12/01, Eugenio Perez Martin wrote:
> On Fri, Dec 1, 2023 at 11:49 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> > Add a bitmask variable that tracks hw vq field changes that
> > are supposed to be modified on next hw vq change command.
> >
> > This will be useful to set multiple vq fields when resuming the vq.
> >
> > The state needs to remain as a parameter as it doesn't make sense to
> > make it part of the vq struct: fw_state is updated only after a
> > successful command.
> >
>
> I don't get this paragraph, "modified_fields" is a member of
> "mlx5_vdpa_virtqueue". Am I missing something?
>
I think this paragraph adds more confusion than clarification. I meant
to say that the state argument from modified_virtqueue needs to remain
there, as opposed to integrating it into the mlx5_vdpa_virtqueue struct.
>
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 48 +++++++++++++++++++++++++------
> > 1 file changed, 40 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index 12ac3397f39b..d06285e46fe2 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
> > u16 avail_idx;
> > u16 used_idx;
> > int fw_state;
> > +
> > + u64 modified_fields;
> > +
> > struct msi_map map;
> >
> > /* keep last in the struct */
> > @@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
> > }
> > }
> >
> > -static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
> > +static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
> > +{
> > + /* Only state is always modifiable */
> > + if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
> > + return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
> > + mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
> > +
> > + return true;
> > +}
> > +
> > +static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > + struct mlx5_vdpa_virtqueue *mvq,
> > + int state)
> > {
> > int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
> > u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
> > @@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
> > return 0;
> >
> > + if (!modifiable_virtqueue_fields(mvq))
> > + return -EINVAL;
> > +
>
> I'm ok with this change, but since modified_fields is (or will be) a
> bitmap tracking changes to state, addresses, mkey, etc, doesn't have
> more sense to check it like:
>
> if (modified_fields & 1<<change_1_flag)
> // perform change 1
> if (modified_fields & 1<<change_2_flag)
> // perform change 2
> if (modified_fields & 1<<change_3_flag)
> // perform change 13
> ---
>
> Instead of:
> if (!modified_fields)
> return
>
> if (modified_fields & 1<<change_1_flag)
> // perform change 1
> if (modified_fields & 1<<change_2_flag)
> // perform change 2
>
> // perform change 3, no checking, as it is the only possible value of
> modified_fields
> ---
>
> Or am I missing something?
>
modifiable_virtqueue_fields() is meant to check that the modification is
done only in the INIT or SUSPEND state of the queue. Or did I
misunderstand your question?
> The rest looks good to me.
>
Thanks for reviewing my patches Eugenio!
> > if (!is_valid_state_change(mvq->fw_state, state))
> > return -EINVAL;
> >
> > @@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
> > MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
> >
> > obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
> > - MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
> > - MLX5_VIRTQ_MODIFY_MASK_STATE);
> > - MLX5_SET(virtio_net_q_object, obj_context, state, state);
> > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
> > + MLX5_SET(virtio_net_q_object, obj_context, state, state);
> > +
> > + MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
> > err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
> > kfree(in);
> > if (!err)
> > mvq->fw_state = state;
> >
> > + mvq->modified_fields = 0;
> > +
> > return err;
> > }
> >
> > +static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
> > + struct mlx5_vdpa_virtqueue *mvq,
> > + unsigned int state)
> > +{
> > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
> > + return modify_virtqueue(ndev, mvq, state);
> > +}
> > +
> > static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> > {
> > u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
> > @@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
> > goto err_vq;
> >
> > if (mvq->ready) {
> > - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > if (err) {
> > mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
> > idx, err);
> > @@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
> > if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
> > return;
> >
> > - if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> > + if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
> > mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");
> >
> > if (query_virtqueue(ndev, mvq, &attr)) {
> > @@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
> > return;
> >
> > suspend_vq(ndev, mvq);
> > + mvq->modified_fields = 0;
> > destroy_virtqueue(ndev, mvq);
> > dealloc_vector(ndev, mvq);
> > counter_set_dealloc(ndev, mvq);
> > @@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
> > if (!ready) {
> > suspend_vq(ndev, mvq);
> > } else {
> > - err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > + err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
> > if (err) {
> > mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
> > ready = false;
> > @@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
> > {
> > int i;
> >
> > - for (i = 0; i < ndev->mvdev.max_vqs; i++)
> > + for (i = 0; i < ndev->mvdev.max_vqs; i++) {
> > ndev->vqs[i].ready = false;
> > + ndev->vqs[i].modified_fields = 0;
> > + }
> >
> > ndev->mvdev.cvq.ready = false;
> > }
> > --
> > 2.42.0
> >
>
@@ -120,6 +120,9 @@ struct mlx5_vdpa_virtqueue {
u16 avail_idx;
u16 used_idx;
int fw_state;
+
+ u64 modified_fields;
+
struct msi_map map;
/* keep last in the struct */
@@ -1181,7 +1184,19 @@ static bool is_valid_state_change(int oldstate, int newstate)
}
}
-static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, int state)
+static bool modifiable_virtqueue_fields(struct mlx5_vdpa_virtqueue *mvq)
+{
+ /* Only state is always modifiable */
+ if (mvq->modified_fields & ~MLX5_VIRTQ_MODIFY_MASK_STATE)
+ return mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_INIT ||
+ mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND;
+
+ return true;
+}
+
+static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
+ struct mlx5_vdpa_virtqueue *mvq,
+ int state)
{
int inlen = MLX5_ST_SZ_BYTES(modify_virtio_net_q_in);
u32 out[MLX5_ST_SZ_DW(modify_virtio_net_q_out)] = {};
@@ -1193,6 +1208,9 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
if (mvq->fw_state == MLX5_VIRTIO_NET_Q_OBJECT_NONE)
return 0;
+ if (!modifiable_virtqueue_fields(mvq))
+ return -EINVAL;
+
if (!is_valid_state_change(mvq->fw_state, state))
return -EINVAL;
@@ -1208,17 +1226,28 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque
MLX5_SET(general_obj_in_cmd_hdr, cmd_hdr, uid, ndev->mvdev.res.uid);
obj_context = MLX5_ADDR_OF(modify_virtio_net_q_in, in, obj_context);
- MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select,
- MLX5_VIRTQ_MODIFY_MASK_STATE);
- MLX5_SET(virtio_net_q_object, obj_context, state, state);
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE)
+ MLX5_SET(virtio_net_q_object, obj_context, state, state);
+
+ MLX5_SET64(virtio_net_q_object, obj_context, modify_field_select, mvq->modified_fields);
err = mlx5_cmd_exec(ndev->mvdev.mdev, in, inlen, out, sizeof(out));
kfree(in);
if (!err)
mvq->fw_state = state;
+ mvq->modified_fields = 0;
+
return err;
}
+static int modify_virtqueue_state(struct mlx5_vdpa_net *ndev,
+ struct mlx5_vdpa_virtqueue *mvq,
+ unsigned int state)
+{
+ mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_STATE;
+ return modify_virtqueue(ndev, mvq, state);
+}
+
static int counter_set_alloc(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
{
u32 in[MLX5_ST_SZ_DW(create_virtio_q_counters_in)] = {};
@@ -1347,7 +1376,7 @@ static int setup_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq)
goto err_vq;
if (mvq->ready) {
- err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
+ err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
if (err) {
mlx5_vdpa_warn(&ndev->mvdev, "failed to modify to ready vq idx %d(%d)\n",
idx, err);
@@ -1382,7 +1411,7 @@ static void suspend_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m
if (mvq->fw_state != MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY)
return;
- if (modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
+ if (modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_SUSPEND))
mlx5_vdpa_warn(&ndev->mvdev, "modify to suspend failed\n");
if (query_virtqueue(ndev, mvq, &attr)) {
@@ -1407,6 +1436,7 @@ static void teardown_vq(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *
return;
suspend_vq(ndev, mvq);
+ mvq->modified_fields = 0;
destroy_virtqueue(ndev, mvq);
dealloc_vector(ndev, mvq);
counter_set_dealloc(ndev, mvq);
@@ -2207,7 +2237,7 @@ static void mlx5_vdpa_set_vq_ready(struct vdpa_device *vdev, u16 idx, bool ready
if (!ready) {
suspend_vq(ndev, mvq);
} else {
- err = modify_virtqueue(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
+ err = modify_virtqueue_state(ndev, mvq, MLX5_VIRTIO_NET_Q_OBJECT_STATE_RDY);
if (err) {
mlx5_vdpa_warn(mvdev, "modify VQ %d to ready failed (%d)\n", idx, err);
ready = false;
@@ -2804,8 +2834,10 @@ static void clear_vqs_ready(struct mlx5_vdpa_net *ndev)
{
int i;
- for (i = 0; i < ndev->mvdev.max_vqs; i++)
+ for (i = 0; i < ndev->mvdev.max_vqs; i++) {
ndev->vqs[i].ready = false;
+ ndev->vqs[i].modified_fields = 0;
+ }
ndev->mvdev.cvq.ready = false;
}