Addresses get set by .set_vq_address. hw vq addresses will be updated on
next modify_virtqueue.
Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
Reviewed-by: Gal Pressman <gal@nvidia.com>
Acked-by: Eugenio Pérez <eperezma@redhat.com>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
2 files changed, 10 insertions(+)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index f8f088cced50..80e066de0866 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
bool state_change = false;
void *obj_context;
void *cmd_hdr;
+ void *vq_ctx;
void *in;
int err;
@@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
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);
+ vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
@@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
state_change = true;
}
+ if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
+ MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
+ MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
+ MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
+ }
+
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));
if (err)
@@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
mvq->desc_addr = desc_area;
mvq->device_addr = device_area;
mvq->driver_addr = driver_area;
+ mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
return 0;
}
diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
index b86d51a855f6..9594ac405740 100644
--- a/include/linux/mlx5/mlx5_ifc_vdpa.h
+++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
@@ -145,6 +145,7 @@ enum {
MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
+ MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
};
--
2.42.0
On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> Addresses get set by .set_vq_address. hw vq addresses will be updated on
> next modify_virtqueue.
>
> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> Reviewed-by: Gal Pressman <gal@nvidia.com>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
I'm kind of ok with this patch and the next one about state, but I
didn't ack them in the previous series.
My main concern is that it is not valid to change the vq address after
DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
change at this moment. I'm not sure about vq state in vDPA, but vhost
forbids changing it with an active backend.
Suspend is not defined in VirtIO at this moment though, so maybe it is
ok to decide that all of these parameters may change during suspend.
Maybe the best thing is to protect this with a vDPA feature flag.
Jason, what do you think?
Thanks!
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> 2 files changed, 10 insertions(+)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index f8f088cced50..80e066de0866 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> bool state_change = false;
> void *obj_context;
> void *cmd_hdr;
> + void *vq_ctx;
> void *in;
> int err;
>
> @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> 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);
> + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
>
> if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> state_change = true;
> }
>
> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> + }
> +
> 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));
> if (err)
> @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> mvq->desc_addr = desc_area;
> mvq->device_addr = device_area;
> mvq->driver_addr = driver_area;
> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> return 0;
> }
>
> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> index b86d51a855f6..9594ac405740 100644
> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> @@ -145,6 +145,7 @@ enum {
> MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> };
>
> --
> 2.42.0
>
On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>> Addresses get set by .set_vq_address. hw vq addresses will be updated on
>> next modify_virtqueue.
>>
>> Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
>> Reviewed-by: Gal Pressman <gal@nvidia.com>
>> Acked-by: Eugenio Pérez <eperezma@redhat.com>
> I'm kind of ok with this patch and the next one about state, but I
> didn't ack them in the previous series.
>
> My main concern is that it is not valid to change the vq address after
> DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> change at this moment. I'm not sure about vq state in vDPA, but vhost
> forbids changing it with an active backend.
>
> Suspend is not defined in VirtIO at this moment though, so maybe it is
> ok to decide that all of these parameters may change during suspend.
> Maybe the best thing is to protect this with a vDPA feature flag.
I think protect with vDPA feature flag could work, while on the other
hand vDPA means vendor specific optimization is possible around suspend
and resume (in case it helps performance), which doesn't have to be
backed by virtio spec. Same applies to vhost user backend features,
variations there were not backed by spec either. Of course, we should
try best to make the default behavior backward compatible with
virtio-based backend, but that circles back to no suspend definition in
the current virtio spec, for which I hope we don't cease development on
vDPA indefinitely. After all, the virtio based vdap backend can well
define its own feature flag to describe (minor difference in) the
suspend behavior based on the later spec once it is formed in future.
Regards,
-Siwei
>
> Jason, what do you think?
>
> Thanks!
>
>> ---
>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
>> include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
>> 2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index f8f088cced50..80e066de0866 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
>> bool state_change = false;
>> void *obj_context;
>> void *cmd_hdr;
>> + void *vq_ctx;
>> void *in;
>> int err;
>>
>> @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
>> 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);
>> + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
>>
>> if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
>> if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
>> @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
>> state_change = true;
>> }
>>
>> + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
>> + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
>> + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
>> + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
>> + }
>> +
>> 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));
>> if (err)
>> @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
>> mvq->desc_addr = desc_area;
>> mvq->device_addr = device_area;
>> mvq->driver_addr = driver_area;
>> + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
>> return 0;
>> }
>>
>> diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
>> index b86d51a855f6..9594ac405740 100644
>> --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
>> +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
>> @@ -145,6 +145,7 @@ enum {
>> MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
>> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
>> MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
>> + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
>> MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
>> };
>>
>> --
>> 2.42.0
>>
On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote:
>
> On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > > next modify_virtqueue.
> > >
> > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > I'm kind of ok with this patch and the next one about state, but I
> > didn't ack them in the previous series.
> >
> > My main concern is that it is not valid to change the vq address after
> > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> > change at this moment. I'm not sure about vq state in vDPA, but vhost
> > forbids changing it with an active backend.
> >
> > Suspend is not defined in VirtIO at this moment though, so maybe it is
> > ok to decide that all of these parameters may change during suspend.
> > Maybe the best thing is to protect this with a vDPA feature flag.
> I think protect with vDPA feature flag could work, while on the other
> hand vDPA means vendor specific optimization is possible around suspend
> and resume (in case it helps performance), which doesn't have to be
> backed by virtio spec. Same applies to vhost user backend features,
> variations there were not backed by spec either. Of course, we should
> try best to make the default behavior backward compatible with
> virtio-based backend, but that circles back to no suspend definition in
> the current virtio spec, for which I hope we don't cease development on
> vDPA indefinitely. After all, the virtio based vdap backend can well
> define its own feature flag to describe (minor difference in) the
> suspend behavior based on the later spec once it is formed in future.
>
So what is the way forward here? From what I understand the options are:
1) Add a vdpa feature flag for changing device properties while suspended.
2) Drop these 2 patches from the series for now. Not sure if this makes sense as
this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this
code won't work anymore. This means the series would be less well tested.
Are there other possible options? What do you think?
[0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip
Thanks,
Dragos
> Regards,
> -Siwei
>
>
>
> >
> > Jason, what do you think?
> >
> > Thanks!
> >
> > > ---
> > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > > 2 files changed, 10 insertions(+)
> > >
> > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > index f8f088cced50..80e066de0866 100644
> > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > bool state_change = false;
> > > void *obj_context;
> > > void *cmd_hdr;
> > > + void *vq_ctx;
> > > void *in;
> > > int err;
> > >
> > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > 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);
> > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
> > >
> > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > state_change = true;
> > > }
> > >
> > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > > + }
> > > +
> > > 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));
> > > if (err)
> > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> > > mvq->desc_addr = desc_area;
> > > mvq->device_addr = device_area;
> > > mvq->driver_addr = driver_area;
> > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > > return 0;
> > > }
> > >
> > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > index b86d51a855f6..9594ac405740 100644
> > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > @@ -145,6 +145,7 @@ enum {
> > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> > > };
> > >
> > > --
> > > 2.42.0
> > >
>
On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote:
> On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote:
> >
> > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > > > next modify_virtqueue.
> > > >
> > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > I'm kind of ok with this patch and the next one about state, but I
> > > didn't ack them in the previous series.
> > >
> > > My main concern is that it is not valid to change the vq address after
> > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> > > change at this moment. I'm not sure about vq state in vDPA, but vhost
> > > forbids changing it with an active backend.
> > >
> > > Suspend is not defined in VirtIO at this moment though, so maybe it is
> > > ok to decide that all of these parameters may change during suspend.
> > > Maybe the best thing is to protect this with a vDPA feature flag.
> > I think protect with vDPA feature flag could work, while on the other
> > hand vDPA means vendor specific optimization is possible around suspend
> > and resume (in case it helps performance), which doesn't have to be
> > backed by virtio spec. Same applies to vhost user backend features,
> > variations there were not backed by spec either. Of course, we should
> > try best to make the default behavior backward compatible with
> > virtio-based backend, but that circles back to no suspend definition in
> > the current virtio spec, for which I hope we don't cease development on
> > vDPA indefinitely. After all, the virtio based vdap backend can well
> > define its own feature flag to describe (minor difference in) the
> > suspend behavior based on the later spec once it is formed in future.
> >
> So what is the way forward here? From what I understand the options are:
>
> 1) Add a vdpa feature flag for changing device properties while suspended.
>
> 2) Drop these 2 patches from the series for now. Not sure if this makes sense as
> this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this
> code won't work anymore. This means the series would be less well tested.
>
> Are there other possible options? What do you think?
>
> [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip
I am fine with either of these.
> Thanks,
> Dragos
>
> > Regards,
> > -Siwei
> >
> >
> >
> > >
> > > Jason, what do you think?
> > >
> > > Thanks!
> > >
> > > > ---
> > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > > > 2 files changed, 10 insertions(+)
> > > >
> > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > index f8f088cced50..80e066de0866 100644
> > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > bool state_change = false;
> > > > void *obj_context;
> > > > void *cmd_hdr;
> > > > + void *vq_ctx;
> > > > void *in;
> > > > int err;
> > > >
> > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > 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);
> > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
> > > >
> > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > state_change = true;
> > > > }
> > > >
> > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > > > + }
> > > > +
> > > > 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));
> > > > if (err)
> > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> > > > mvq->desc_addr = desc_area;
> > > > mvq->device_addr = device_area;
> > > > mvq->driver_addr = driver_area;
> > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > > > return 0;
> > > > }
> > > >
> > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > index b86d51a855f6..9594ac405740 100644
> > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > @@ -145,6 +145,7 @@ enum {
> > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> > > > };
> > > >
> > > > --
> > > > 2.42.0
> > > >
> >
>
On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote:
> On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote:
> > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote:
> > >
> > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > > > > next modify_virtqueue.
> > > > >
> > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > > I'm kind of ok with this patch and the next one about state, but I
> > > > didn't ack them in the previous series.
> > > >
> > > > My main concern is that it is not valid to change the vq address after
> > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> > > > change at this moment. I'm not sure about vq state in vDPA, but vhost
> > > > forbids changing it with an active backend.
> > > >
> > > > Suspend is not defined in VirtIO at this moment though, so maybe it is
> > > > ok to decide that all of these parameters may change during suspend.
> > > > Maybe the best thing is to protect this with a vDPA feature flag.
> > > I think protect with vDPA feature flag could work, while on the other
> > > hand vDPA means vendor specific optimization is possible around suspend
> > > and resume (in case it helps performance), which doesn't have to be
> > > backed by virtio spec. Same applies to vhost user backend features,
> > > variations there were not backed by spec either. Of course, we should
> > > try best to make the default behavior backward compatible with
> > > virtio-based backend, but that circles back to no suspend definition in
> > > the current virtio spec, for which I hope we don't cease development on
> > > vDPA indefinitely. After all, the virtio based vdap backend can well
> > > define its own feature flag to describe (minor difference in) the
> > > suspend behavior based on the later spec once it is formed in future.
> > >
> > So what is the way forward here? From what I understand the options are:
> >
> > 1) Add a vdpa feature flag for changing device properties while suspended.
> >
> > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as
> > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this
> > code won't work anymore. This means the series would be less well tested.
> >
> > Are there other possible options? What do you think?
> >
> > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip
>
> I am fine with either of these.
>
How about allowing the change only under the following conditions:
vhost_vdpa_can_suspend && vhost_vdpa_can_resume &&
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set
?
Thanks,
Dragos
> > Thanks,
> > Dragos
> >
> > > Regards,
> > > -Siwei
> > >
> > >
> > >
> > > >
> > > > Jason, what do you think?
> > > >
> > > > Thanks!
> > > >
> > > > > ---
> > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > > > > 2 files changed, 10 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > index f8f088cced50..80e066de0866 100644
> > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > bool state_change = false;
> > > > > void *obj_context;
> > > > > void *cmd_hdr;
> > > > > + void *vq_ctx;
> > > > > void *in;
> > > > > int err;
> > > > >
> > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > 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);
> > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
> > > > >
> > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > state_change = true;
> > > > > }
> > > > >
> > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > > > > + }
> > > > > +
> > > > > 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));
> > > > > if (err)
> > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> > > > > mvq->desc_addr = desc_area;
> > > > > mvq->device_addr = device_area;
> > > > > mvq->driver_addr = driver_area;
> > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > > > > return 0;
> > > > > }
> > > > >
> > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > index b86d51a855f6..9594ac405740 100644
> > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > @@ -145,6 +145,7 @@ enum {
> > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> > > > > };
> > > > >
> > > > > --
> > > > > 2.42.0
> > > > >
> > >
> >
>
On Thu, Dec 14, 2023 at 4:51 PM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> On Thu, 2023-12-14 at 08:45 -0500, Michael S. Tsirkin wrote:
> > On Thu, Dec 14, 2023 at 01:39:55PM +0000, Dragos Tatulea wrote:
> > > On Tue, 2023-12-12 at 15:44 -0800, Si-Wei Liu wrote:
> > > >
> > > > On 12/12/2023 11:21 AM, Eugenio Perez Martin wrote:
> > > > > On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> > > > > > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > > > > > next modify_virtqueue.
> > > > > >
> > > > > > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > > > > > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > > > > > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > I'm kind of ok with this patch and the next one about state, but I
> > > > > didn't ack them in the previous series.
> > > > >
> > > > > My main concern is that it is not valid to change the vq address after
> > > > > DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> > > > > change at this moment. I'm not sure about vq state in vDPA, but vhost
> > > > > forbids changing it with an active backend.
> > > > >
> > > > > Suspend is not defined in VirtIO at this moment though, so maybe it is
> > > > > ok to decide that all of these parameters may change during suspend.
> > > > > Maybe the best thing is to protect this with a vDPA feature flag.
> > > > I think protect with vDPA feature flag could work, while on the other
> > > > hand vDPA means vendor specific optimization is possible around suspend
> > > > and resume (in case it helps performance), which doesn't have to be
> > > > backed by virtio spec. Same applies to vhost user backend features,
> > > > variations there were not backed by spec either. Of course, we should
> > > > try best to make the default behavior backward compatible with
> > > > virtio-based backend, but that circles back to no suspend definition in
> > > > the current virtio spec, for which I hope we don't cease development on
> > > > vDPA indefinitely. After all, the virtio based vdap backend can well
> > > > define its own feature flag to describe (minor difference in) the
> > > > suspend behavior based on the later spec once it is formed in future.
> > > >
> > > So what is the way forward here? From what I understand the options are:
> > >
> > > 1) Add a vdpa feature flag for changing device properties while suspended.
> > >
> > > 2) Drop these 2 patches from the series for now. Not sure if this makes sense as
> > > this. But then Si-Wei's qemu device suspend/resume poc [0] that exercises this
> > > code won't work anymore. This means the series would be less well tested.
> > >
> > > Are there other possible options? What do you think?
> > >
> > > [0] https://github.com/siwliu-kernel/qemu/tree/svq-resume-wip
> >
> > I am fine with either of these.
> >
> How about allowing the change only under the following conditions:
> vhost_vdpa_can_suspend && vhost_vdpa_can_resume &&
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is set
>
> ?
I think the best option by far is 1, as there is no hint in the
combination of these 3 indicating that you can change device
properties in the suspended state.
>
> Thanks,
> Dragos
>
> > > Thanks,
> > > Dragos
> > >
> > > > Regards,
> > > > -Siwei
> > > >
> > > >
> > > >
> > > > >
> > > > > Jason, what do you think?
> > > > >
> > > > > Thanks!
> > > > >
> > > > > > ---
> > > > > > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > > > > > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > > > > > 2 files changed, 10 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > index f8f088cced50..80e066de0866 100644
> > > > > > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > > > > > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > bool state_change = false;
> > > > > > void *obj_context;
> > > > > > void *cmd_hdr;
> > > > > > + void *vq_ctx;
> > > > > > void *in;
> > > > > > int err;
> > > > > >
> > > > > > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > 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);
> > > > > > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
> > > > > >
> > > > > > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > > > > > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> > > > > > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > > > > > state_change = true;
> > > > > > }
> > > > > >
> > > > > > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > > > > > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > > > > > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > > > > > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > > > > > + }
> > > > > > +
> > > > > > 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));
> > > > > > if (err)
> > > > > > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> > > > > > mvq->desc_addr = desc_area;
> > > > > > mvq->device_addr = device_area;
> > > > > > mvq->driver_addr = driver_area;
> > > > > > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > index b86d51a855f6..9594ac405740 100644
> > > > > > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > > > > > @@ -145,6 +145,7 @@ enum {
> > > > > > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> > > > > > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> > > > > > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> > > > > > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> > > > > > };
> > > > > >
> > > > > > --
> > > > > > 2.42.0
> > > > > >
> > > >
> > >
> >
>
On Tue, 2023-12-12 at 20:21 +0100, Eugenio Perez Martin wrote:
> On Tue, Dec 5, 2023 at 11:46 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >
> > Addresses get set by .set_vq_address. hw vq addresses will be updated on
> > next modify_virtqueue.
> >
> > Signed-off-by: Dragos Tatulea <dtatulea@nvidia.com>
> > Reviewed-by: Gal Pressman <gal@nvidia.com>
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> I'm kind of ok with this patch and the next one about state, but I
> didn't ack them in the previous series.
>
Sorry about the Ack misplacement. I got confused.
> My main concern is that it is not valid to change the vq address after
> DRIVER_OK in VirtIO, which vDPA follows. Only memory maps are ok to
> change at this moment. I'm not sure about vq state in vDPA, but vhost
> forbids changing it with an active backend.
>
> Suspend is not defined in VirtIO at this moment though, so maybe it is
> ok to decide that all of these parameters may change during suspend.
> Maybe the best thing is to protect this with a vDPA feature flag.
>
> Jason, what do you think?
>
> Thanks!
>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 9 +++++++++
> > include/linux/mlx5/mlx5_ifc_vdpa.h | 1 +
> > 2 files changed, 10 insertions(+)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index f8f088cced50..80e066de0866 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -1209,6 +1209,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > bool state_change = false;
> > void *obj_context;
> > void *cmd_hdr;
> > + void *vq_ctx;
> > void *in;
> > int err;
> >
> > @@ -1230,6 +1231,7 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > 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);
> > + vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context);
> >
> > if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_STATE) {
> > if (!is_valid_state_change(mvq->fw_state, state, is_resumable(ndev))) {
> > @@ -1241,6 +1243,12 @@ static int modify_virtqueue(struct mlx5_vdpa_net *ndev,
> > state_change = true;
> > }
> >
> > + if (mvq->modified_fields & MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS) {
> > + MLX5_SET64(virtio_q, vq_ctx, desc_addr, mvq->desc_addr);
> > + MLX5_SET64(virtio_q, vq_ctx, used_addr, mvq->device_addr);
> > + MLX5_SET64(virtio_q, vq_ctx, available_addr, mvq->driver_addr);
> > + }
> > +
> > 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));
> > if (err)
> > @@ -2202,6 +2210,7 @@ static int mlx5_vdpa_set_vq_address(struct vdpa_device *vdev, u16 idx, u64 desc_
> > mvq->desc_addr = desc_area;
> > mvq->device_addr = device_area;
> > mvq->driver_addr = driver_area;
> > + mvq->modified_fields |= MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS;
> > return 0;
> > }
> >
> > diff --git a/include/linux/mlx5/mlx5_ifc_vdpa.h b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > index b86d51a855f6..9594ac405740 100644
> > --- a/include/linux/mlx5/mlx5_ifc_vdpa.h
> > +++ b/include/linux/mlx5/mlx5_ifc_vdpa.h
> > @@ -145,6 +145,7 @@ enum {
> > MLX5_VIRTQ_MODIFY_MASK_STATE = (u64)1 << 0,
> > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_PARAMS = (u64)1 << 3,
> > MLX5_VIRTQ_MODIFY_MASK_DIRTY_BITMAP_DUMP_ENABLE = (u64)1 << 4,
> > + MLX5_VIRTQ_MODIFY_MASK_VIRTIO_Q_ADDRS = (u64)1 << 6,
> > MLX5_VIRTQ_MODIFY_MASK_DESC_GROUP_MKEY = (u64)1 << 14,
> > };
> >
> > --
> > 2.42.0
> >
>
© 2016 - 2025 Red Hat, Inc.