Jason Wang
2021-Feb-01 07:23 UTC
[PATCH 2/2] vdpa/mlx5: Restore the hardware used index after change map
On 2021/2/1 ??2:38, Eli Cohen wrote:> On Mon, Feb 01, 2021 at 02:00:35PM +0800, Jason Wang wrote: >> On 2021/2/1 ??1:52, Eli Cohen wrote: >>> On Mon, Feb 01, 2021 at 11:36:23AM +0800, Jason Wang wrote: >>>> On 2021/2/1 ??2:55, Eli Cohen wrote: >>>>> On Fri, Jan 29, 2021 at 11:49:45AM +0800, Jason Wang wrote: >>>>>> On 2021/1/28 ??9:41, Eli Cohen wrote: >>>>>>> When a change of memory map occurs, the hardware resources are destroyed >>>>>>> and then re-created again with the new memory map. In such case, we need >>>>>>> to restore the hardware available and used indices. The driver failed to >>>>>>> restore the used index which is added here. >>>>>>> >>>>>>> Fixes 1a86b377aa21 ("vdpa/mlx5: Add VDPA driver for supported mlx5 devices") >>>>>>> Signed-off-by: Eli Cohen <elic at nvidia.com> >>>>>> A question. Does this mean after a vq is suspended, the hw used index is not >>>>>> equal to vq used index? >>>>> Surely there is just one "Used index" for a VQ. What I was trying to say >>>>> is that after the VQ is suspended, I read the used index by querying the >>>>> hardware. The read result is the used index that the hardware wrote to >>>>> memory. >>>> Just to make sure I understand here. So it looks to me we had two index. The >>>> first is the used index which is stored in the memory/virtqueue, the second >>>> is the one that is stored by the device. >>>> >>> It is the structures defined in the virtio spec in 2.6.6 for the >>> available ring and 2.6.8 for the used ring. As you know these the >>> available ring is written to by the driver and read by the device. The >>> opposite happens for the used index. >> >> Right, so for used index it was wrote by device. And the device should have >> an internal used index value that is used to write to the used ring. And the >> code is used to sync the device internal used index if I understand this >> correctly. >> >> >>> The reason I need to restore the last known indices is for the new >>> hardware objects to sync on the last state and take over from there. >> >> Right, after the vq suspending, the questions are: >> >> 1) is hardware internal used index might not be the same with the used index >> in the virtqueue? >> > Generally the answer is no because the hardware is the only one writing > it. New objects start up with the initial value configured to them upon > creation. This value was zero before this change. > You could argue that since the hardware has access to virtqueue memory, > it could just read the value from there but it does not.I see.> >> or >> >> 2) can we simply sync the virtqueue's used index to the hardware's used >> index? >> > Theoretically it could be done but that's not how the hardware works. > One reason is that is not supposed to read from that area. But it is > really hardware implementation detail.I get you now. So Acked-by: Jason Wang <jasowang at redhat.com> Thanks>>>>> After the I create the new hardware object, I need to tell it >>>>> what is the used index (and the available index) as a way to sync it >>>>> with the existing VQ. >>>> For avail index I understand that the hardware index is not synced with the >>>> avail index stored in the memory/virtqueue. The question is used index, if >>>> the hardware one is not synced with the one in the virtqueue. It means after >>>> vq is suspended,? some requests is not completed by the hardware (e.g the >>>> buffer were not put to used ring). >>>> >>>> This may have implications to live migration, it means those unaccomplished >>>> requests needs to be migrated to the destination and resubmitted to the >>>> device. This looks not easy. >>>> >>>> Thanks >>>> >>>> >>>>> This sync is especially important when a change of map occurs while the >>>>> VQ was already used (hence the indices are likely to be non zero). This >>>>> can be triggered by hot adding memory after the VQs have been used. >>>>> >>>>>> Thanks >>>>>> >>>>>> >>>>>>> --- >>>>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 7 +++++++ >>>>>>> 1 file changed, 7 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>> index 549ded074ff3..3fc8588cecae 100644 >>>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c >>>>>>> @@ -87,6 +87,7 @@ struct mlx5_vq_restore_info { >>>>>>> u64 device_addr; >>>>>>> u64 driver_addr; >>>>>>> u16 avail_index; >>>>>>> + u16 used_index; >>>>>>> bool ready; >>>>>>> struct vdpa_callback cb; >>>>>>> bool restore; >>>>>>> @@ -121,6 +122,7 @@ struct mlx5_vdpa_virtqueue { >>>>>>> u32 virtq_id; >>>>>>> struct mlx5_vdpa_net *ndev; >>>>>>> u16 avail_idx; >>>>>>> + u16 used_idx; >>>>>>> int fw_state; >>>>>>> /* keep last in the struct */ >>>>>>> @@ -804,6 +806,7 @@ static int create_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtque >>>>>>> obj_context = MLX5_ADDR_OF(create_virtio_net_q_in, in, obj_context); >>>>>>> MLX5_SET(virtio_net_q_object, obj_context, hw_available_index, mvq->avail_idx); >>>>>>> + MLX5_SET(virtio_net_q_object, obj_context, hw_used_index, mvq->used_idx); >>>>>>> MLX5_SET(virtio_net_q_object, obj_context, queue_feature_bit_mask_12_3, >>>>>>> get_features_12_3(ndev->mvdev.actual_features)); >>>>>>> vq_ctx = MLX5_ADDR_OF(virtio_net_q_object, obj_context, virtio_q_context); >>>>>>> @@ -1022,6 +1025,7 @@ static int connect_qps(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *m >>>>>>> struct mlx5_virtq_attr { >>>>>>> u8 state; >>>>>>> u16 available_index; >>>>>>> + u16 used_index; >>>>>>> }; >>>>>>> static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueue *mvq, >>>>>>> @@ -1052,6 +1056,7 @@ static int query_virtqueue(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqueu >>>>>>> memset(attr, 0, sizeof(*attr)); >>>>>>> attr->state = MLX5_GET(virtio_net_q_object, obj_context, state); >>>>>>> attr->available_index = MLX5_GET(virtio_net_q_object, obj_context, hw_available_index); >>>>>>> + attr->used_index = MLX5_GET(virtio_net_q_object, obj_context, hw_used_index); >>>>>>> kfree(out); >>>>>>> return 0; >>>>>>> @@ -1602,6 +1607,7 @@ static int save_channel_info(struct mlx5_vdpa_net *ndev, struct mlx5_vdpa_virtqu >>>>>>> return err; >>>>>>> ri->avail_index = attr.available_index; >>>>>>> + ri->used_index = attr.used_index; >>>>>>> ri->ready = mvq->ready; >>>>>>> ri->num_ent = mvq->num_ent; >>>>>>> ri->desc_addr = mvq->desc_addr; >>>>>>> @@ -1646,6 +1652,7 @@ static void restore_channels_info(struct mlx5_vdpa_net *ndev) >>>>>>> continue; >>>>>>> mvq->avail_idx = ri->avail_index; >>>>>>> + mvq->used_idx = ri->used_index; >>>>>>> mvq->ready = ri->ready; >>>>>>> mvq->num_ent = ri->num_ent; >>>>>>> mvq->desc_addr = ri->desc_addr;