diff mbox series

[RFC] vdpa_sim: implement .reset_map support

Message ID 1697185420-27213-1-git-send-email-si-wei.liu@oracle.com
State New
Headers
Series [RFC] vdpa_sim: implement .reset_map support |

Commit Message

Si-Wei Liu Oct. 13, 2023, 8:23 a.m. UTC 
  RFC only. Not tested on vdpa-sim-blk with user virtual address.
Works fine with vdpa-sim-net which uses physical address to map.

This patch is based on top of [1].

[1] https://lore.kernel.org/virtualization/1696928580-7520-1-git-send-email-si-wei.liu@oracle.com/

Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
 drivers/vdpa/vdpa_sim/vdpa_sim.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

Comments

Stefano Garzarella Oct. 13, 2023, 9:22 a.m. UTC | #1 
Hi Si-Wei,

On Fri, Oct 13, 2023 at 01:23:40AM -0700, Si-Wei Liu wrote:
>RFC only. Not tested on vdpa-sim-blk with user virtual address.

I can test it, but what I should stress?

>Works fine with vdpa-sim-net which uses physical address to map.

Can you share your tests? so I'll try to do the same with blk.

>
>This patch is based on top of [1].
>
>[1] 
>https://lore.kernel.org/virtualization/1696928580-7520-1-git-send-email-si-wei.liu@oracle.com/

The series does not apply well on master or vhost tree.
Where should I apply it?

If you have a tree with all of them applied, will be easy for me ;-)

Thanks,
Stefano
Si-Wei Liu Oct. 13, 2023, 5:29 p.m. UTC | #2 
Hi Stefano,

On 10/13/2023 2:22 AM, Stefano Garzarella wrote:
> Hi Si-Wei,
>
> On Fri, Oct 13, 2023 at 01:23:40AM -0700, Si-Wei Liu wrote:
>> RFC only. Not tested on vdpa-sim-blk with user virtual address.
>
> I can test it, but what I should stress?
Great, thank you! As you see, my patch moved vhost_iotlb_reset out of 
vdpasim_reset for the sake of decoupling mapping from vdpa device reset. 
For hardware devices this decoupling makes sense as platform IOMMU 
already did it. But I'm not sure if there's something in the software 
device (esp. with vdpa-blk and the userspace library stack) that may 
have to rely on the current .reset behavior that clears the vhost_iotlb. 
So perhaps you can try to exercise every possible case involving blk 
device reset, and see if anything (related to mapping) breaks?

>
>> Works fine with vdpa-sim-net which uses physical address to map.
>
> Can you share your tests? so I'll try to do the same with blk.
Basically everything involving virtio device reset in the guest, e.g. 
reboot the VM, remove/unbind then reprobe/bind the virtio-net 
module/driver, then see if device I/O (which needs mapping properly) is 
still flowing as expected. And then everything else that could trigger 
QEMU's vhost_dev_start/stop paths ending up as passive vhos-vdpa backend 
reset, for e.g. link status change, suspend/hibernate, SVQ switch and 
live migration. I am not sure if vdpa-blk supports live migration 
through SVQ or not, if not you don't need to worry about.

>
>>
>> This patch is based on top of [1].
>>
>> [1] 
>> https://lore.kernel.org/virtualization/1696928580-7520-1-git-send-email-si-wei.liu@oracle.com/
>
> The series does not apply well on master or vhost tree.
> Where should I apply it?
Sent the link through another email offline.

Thanks,
-Siwei

>
> If you have a tree with all of them applied, will be easy for me ;-)
>
> Thanks,
> Stefano
>
Stefano Garzarella Oct. 17, 2023, 1:44 p.m. UTC | #3 
On Fri, Oct 13, 2023 at 10:29:26AM -0700, Si-Wei Liu wrote:
>Hi Stefano,
>
>On 10/13/2023 2:22 AM, Stefano Garzarella wrote:
>>Hi Si-Wei,
>>
>>On Fri, Oct 13, 2023 at 01:23:40AM -0700, Si-Wei Liu wrote:
>>>RFC only. Not tested on vdpa-sim-blk with user virtual address.
>>
>>I can test it, but what I should stress?
>Great, thank you! As you see, my patch moved vhost_iotlb_reset out of 
>vdpasim_reset for the sake of decoupling mapping from vdpa device 
>reset. For hardware devices this decoupling makes sense as platform 
>IOMMU already did it. But I'm not sure if there's something in the 
>software device (esp. with vdpa-blk and the userspace library stack) 
>that may have to rely on the current .reset behavior that clears the 
>vhost_iotlb. So perhaps you can try to exercise every possible case 
>involving blk device reset, and see if anything (related to mapping) 
>breaks?

I just tried these steps without using a VM and the host kernel hangs
after adding the device:

[root@f38-vm-build ~]# modprobe virtio-vdpa
[root@f38-vm-build ~]# modprobe vdpa-sim-blk
[root@f38-vm-build ~]# vdpa dev add mgmtdev vdpasim_blk name blk0
[   35.284575][  T563] virtio_blk virtio6: 1/0/0 default/read/poll queues
[   35.286372][  T563] virtio_blk virtio6: [vdb] 262144 512-byte logical blocks (134 MB/128 MiB)
[   35.295271][  T564] vringh:

Reverting this patch (so building "vdpa/mlx5: implement .reset_map 
driver op") worked here.

>
>>
>>>Works fine with vdpa-sim-net which uses physical address to map.
>>
>>Can you share your tests? so I'll try to do the same with blk.
>Basically everything involving virtio device reset in the guest, e.g.  
>reboot the VM, remove/unbind then reprobe/bind the virtio-net 
>module/driver, then see if device I/O (which needs mapping properly) is 
>still flowing as expected. And then everything else that could trigger 
>QEMU's vhost_dev_start/stop paths ending up as passive vhos-vdpa 
>backend reset, for e.g. link status change, suspend/hibernate, SVQ 
>switch and live migration. I am not sure if vdpa-blk supports live 
>migration through SVQ or not, if not you don't need to worry about.
>
>>
>>>
>>>This patch is based on top of [1].
>>>
>>>[1] https://lore.kernel.org/virtualization/1696928580-7520-1-git-send-email-si-wei.liu@oracle.com/
>>
>>The series does not apply well on master or vhost tree.
>>Where should I apply it?
>Sent the link through another email offline.

Received thanks!

Stefano
Si-Wei Liu Oct. 18, 2023, 5:21 a.m. UTC | #4 
Hi Stefano,

On 10/17/2023 6:44 AM, Stefano Garzarella wrote:
> On Fri, Oct 13, 2023 at 10:29:26AM -0700, Si-Wei Liu wrote:
>> Hi Stefano,
>>
>> On 10/13/2023 2:22 AM, Stefano Garzarella wrote:
>>> Hi Si-Wei,
>>>
>>> On Fri, Oct 13, 2023 at 01:23:40AM -0700, Si-Wei Liu wrote:
>>>> RFC only. Not tested on vdpa-sim-blk with user virtual address.
>>>
>>> I can test it, but what I should stress?
>> Great, thank you! As you see, my patch moved vhost_iotlb_reset out of 
>> vdpasim_reset for the sake of decoupling mapping from vdpa device 
>> reset. For hardware devices this decoupling makes sense as platform 
>> IOMMU already did it. But I'm not sure if there's something in the 
>> software device (esp. with vdpa-blk and the userspace library stack) 
>> that may have to rely on the current .reset behavior that clears the 
>> vhost_iotlb. So perhaps you can try to exercise every possible case 
>> involving blk device reset, and see if anything (related to mapping) 
>> breaks?
>
> I just tried these steps without using a VM and the host kernel hangs
> after adding the device:
>
> [root@f38-vm-build ~]# modprobe virtio-vdpa
> [root@f38-vm-build ~]# modprobe vdpa-sim-blk
> [root@f38-vm-build ~]# vdpa dev add mgmtdev vdpasim_blk name blk0
> [   35.284575][  T563] virtio_blk virtio6: 1/0/0 default/read/poll queues
> [   35.286372][  T563] virtio_blk virtio6: [vdb] 262144 512-byte 
> logical blocks (134 MB/128 MiB)
> [   35.295271][  T564] vringh:
>
> Reverting this patch (so building "vdpa/mlx5: implement .reset_map 
> driver op") worked here.
I'm sorry, the previous RFC patch was incomplete - please see the v2 I 
just posted. Tested both use_va and !use_va on vdpa-sim-blk, and raw 
disk copy to the vdpa block simulator using dd seems fine. Just let me 
know how it goes on your side this time.

Thanks,
-Siwei

>
>>
>>>
>>>> Works fine with vdpa-sim-net which uses physical address to map.
>>>
>>> Can you share your tests? so I'll try to do the same with blk.
>> Basically everything involving virtio device reset in the guest, 
>> e.g.  reboot the VM, remove/unbind then reprobe/bind the virtio-net 
>> module/driver, then see if device I/O (which needs mapping properly) 
>> is still flowing as expected. And then everything else that could 
>> trigger QEMU's vhost_dev_start/stop paths ending up as passive 
>> vhos-vdpa backend reset, for e.g. link status change, 
>> suspend/hibernate, SVQ switch and live migration. I am not sure if 
>> vdpa-blk supports live migration through SVQ or not, if not you don't 
>> need to worry about.
>>
>>>
>>>>
>>>> This patch is based on top of [1].
>>>>
>>>> [1] 
>>>> https://lore.kernel.org/virtualization/1696928580-7520-1-git-send-email-si-wei.liu@oracle.com/
>>>
>>> The series does not apply well on master or vhost tree.
>>> Where should I apply it?
>> Sent the link through another email offline.
>
> Received thanks!
>
> Stefano
>
diff mbox series

Patch

diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.c b/drivers/vdpa/vdpa_sim/vdpa_sim.c
index 76d4105..a7455f2 100644
--- a/drivers/vdpa/vdpa_sim/vdpa_sim.c
+++ b/drivers/vdpa/vdpa_sim/vdpa_sim.c
@@ -151,13 +151,6 @@  static void vdpasim_do_reset(struct vdpasim *vdpasim)
 				 &vdpasim->iommu_lock);
 	}
 
-	for (i = 0; i < vdpasim->dev_attr.nas; i++) {
-		vhost_iotlb_reset(&vdpasim->iommu[i]);
-		vhost_iotlb_add_range(&vdpasim->iommu[i], 0, ULONG_MAX,
-				      0, VHOST_MAP_RW);
-		vdpasim->iommu_pt[i] = true;
-	}
-
 	vdpasim->running = true;
 	spin_unlock(&vdpasim->iommu_lock);
 
@@ -637,6 +630,25 @@  static int vdpasim_set_map(struct vdpa_device *vdpa, unsigned int asid,
 	return ret;
 }
 
+static int vdpasim_reset_map(struct vdpa_device *vdpa, unsigned int asid)
+{
+	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
+
+	if (asid >= vdpasim->dev_attr.nas)
+		return -EINVAL;
+
+	spin_lock(&vdpasim->iommu_lock);
+	if (vdpasim->iommu_pt[asid])
+		goto out;
+	vhost_iotlb_reset(&vdpasim->iommu[asid]);
+	vhost_iotlb_add_range(&vdpasim->iommu[asid], 0, ULONG_MAX,
+			      0, VHOST_MAP_RW);
+	vdpasim->iommu_pt[asid] = true;
+out:
+	spin_unlock(&vdpasim->iommu_lock);
+	return 0;
+}
+
 static int vdpasim_bind_mm(struct vdpa_device *vdpa, struct mm_struct *mm)
 {
 	struct vdpasim *vdpasim = vdpa_to_sim(vdpa);
@@ -759,6 +771,7 @@  static void vdpasim_free(struct vdpa_device *vdpa)
 	.set_group_asid         = vdpasim_set_group_asid,
 	.dma_map                = vdpasim_dma_map,
 	.dma_unmap              = vdpasim_dma_unmap,
+	.reset_map              = vdpasim_reset_map,
 	.bind_mm		= vdpasim_bind_mm,
 	.unbind_mm		= vdpasim_unbind_mm,
 	.free                   = vdpasim_free,
@@ -796,6 +809,7 @@  static void vdpasim_free(struct vdpa_device *vdpa)
 	.get_iova_range         = vdpasim_get_iova_range,
 	.set_group_asid         = vdpasim_set_group_asid,
 	.set_map                = vdpasim_set_map,
+	.reset_map              = vdpasim_reset_map,
 	.bind_mm		= vdpasim_bind_mm,
 	.unbind_mm		= vdpasim_unbind_mm,
 	.free                   = vdpasim_free,