On Tue, Aug 11, 2020 at 9:28 PM Peter Xu <pet...@redhat.com> wrote: > > Hi, Eugenio, > > On Tue, Aug 11, 2020 at 08:10:44PM +0200, Eugenio Perez Martin wrote: > > Using this patch as a reference, I'm having problems to understand: > > > > - I'm not sure that the flag name expresses clearly the notifier capability. > > The old code is kind of messed up for dev-iotlb invalidations, by always > sending UNMAP notifications for both iotlb and dev-iotlb invalidations. > > Now if we introduce the new DEV_IOTLB type, we can separate the two: > > - We notify IOMMU_NOTIFIER_UNMAP for iotlb invalidations > > - We notify IOMMU_NOTIFIER_DEV_IOTLB for dev-iotlb invalidations > > Vhost should always be with ats=on when vIOMMU enabled, so it will enable > device iotlb. Then it does not need iotlb (UNMAP) invalidation any more > because they'll normally be duplicated (one is to invalidate vIOMMU cache, one > is to invalidate device cache). > > Also, we can drop UNMAP type for vhost if we introduce DEV_IOTLB. It works > just like on the real hardwares - the device won't be able to receive iotlb > invalidation messages, but only device iotlb invalidation messages. Here > vhost (or, virtio-pci) is the device. > > > - What would be the advantages of using another field (NotifierType?) > > in the notifier to express that it accepts arbitrary ranges for > > unmapping? (If I understood correctly Jason's proposal) > > (Please refer to above too..) > > > - Is it possible (or advisable) to skip all the page splitting in > > vtd_page_walk if the memory range notifier supports these arbitrary > > ranges? What would be the disadvantages? (Maybe in a future patch). It > > seems it is advisable to me, but I would like to double confirm. > > vtd_page_walk is not used for dev-iotlb, we don't need to change that. We > also > want to explicitly keep the page granularity of vtd_page_walk for the other > IOMMU notifiers, e.g. vfio. >
I'm not sure if I'm understanding it. I have here a backtrace in a regular call (not [0,~0ULL]): #0 0x000055555597ca63 in memory_region_notify_one_iommu (notifier=0x7fffe4976f08, entry=0x7fffddff9d20) at /home/qemu/softmmu/memory.c:1895 #1 0x000055555597cc87 in memory_region_notify_iommu (iommu_mr=0x55555728f2e0, iommu_idx=0, entry=...) at /home/qemu/softmmu/memory.c:1938 #2 0x000055555594b95a in vtd_sync_shadow_page_hook (entry=0x7fffddff9e70, private=0x55555728f2e0) at /home/qemu/hw/i386/intel_iommu.c:1436 #3 0x000055555594af7b in vtd_page_walk_one (entry=0x7fffddff9e70, info=0x7fffddffa140) at /home/qemu/hw/i386/intel_iommu.c:1173 #4 0x000055555594b2b3 in vtd_page_walk_level (addr=10531758080, start=4292870144, end=4294967296, level=1, read=true, write=true, info=0x7fffddffa140) at /home/qemu/hw/i386/intel_iommu.c:1254 #5 0x000055555594b225 in vtd_page_walk_level (addr=10530922496, start=3221225472, end=4294967296, level=2, read=true, write=true, info=0x7fffddffa140) at /home/qemu/hw/i386/intel_iommu.c:1236 #6 0x000055555594b225 in vtd_page_walk_level (addr=10529021952, start=0, end=549755813888, level=3, read=true, write=true, info=0x7fffddffa140) at /home/qemu/hw/i386/intel_iommu.c:1236 #7 0x000055555594b3f8 in vtd_page_walk (s=0x555557565210, ce=0x7fffddffa1a0, start=0, end=549755813888, info=0x7fffddffa140) at /home/qemu/hw/i386/intel_iommu.c:1293 #8 0x000055555594ba77 in vtd_sync_shadow_page_table_range (vtd_as=0x55555728f270, ce=0x7fffddffa1a0, addr=0, size=18446744073709551615) at /home/qemu/hw/i386/intel_iommu.c:1467 #9 0x000055555594bb50 in vtd_sync_shadow_page_table (vtd_as=0x55555728f270) at /home/qemu/hw/i386/intel_iommu.c:1498 #10 0x000055555594cc5f in vtd_iotlb_domain_invalidate (s=0x555557565210, domain_id=3) at /home/qemu/hw/i386/intel_iommu.c:1965 #11 0x000055555594dbae in vtd_process_iotlb_desc (s=0x555557565210, inv_desc=0x7fffddffa2b0) at /home/qemu/hw/i386/intel_iommu.c:2371 #12 0x000055555594dfd3 in vtd_process_inv_desc (s=0x555557565210) at /home/qemu/hw/i386/intel_iommu.c:2499 #13 0x000055555594e1e9 in vtd_fetch_inv_desc (s=0x555557565210) at /home/qemu/hw/i386/intel_iommu.c:2568 #14 0x000055555594e330 in vtd_handle_iqt_write (s=0x555557565210) at /home/qemu/hw/i386/intel_iommu.c:2595 #15 0x000055555594ed90 in vtd_mem_write (opaque=0x555557565210, addr=136, val=1888, size=4) at /home/qemu/hw/i386/intel_iommu.c:2842 #16 0x00005555559787b9 in memory_region_write_accessor (mr=0x555557565540, addr=136, value=0x7fffddffa478, size=4, shift=0, mask=4294967295, attrs=...) at /home/qemu/softmmu/memory.c:483 #17 0x00005555559789d7 in access_with_adjusted_size (addr=136, value=0x7fffddffa478, size=4, access_size_min=4, access_size_max=8, access_fn= 0x5555559786da <memory_region_write_accessor>, mr=0x555557565540, attrs=...) at /home/qemu/softmmu/memory.c:544 #18 0x000055555597b8a5 in memory_region_dispatch_write (mr=0x555557565540, addr=136, data=1888, op=MO_32, attrs=...) at /home/qemu/softmmu/memory.c:1465 #19 0x000055555582b1bf in flatview_write_continue (fv=0x7fffc447c470, addr=4275634312, attrs=..., ptr=0x7ffff7dfd028, len=4, addr1=136, l=4, mr=0x555557565540) at /home/qemu/exec.c:3176 #20 0x000055555582b304 in flatview_write (fv=0x7fffc447c470, addr=4275634312, attrs=..., buf=0x7ffff7dfd028, len=4) at /home/qemu/exec.c:3216 #21 0x000055555582b659 in address_space_write (as=0x5555567a9380 <address_space_memory>, addr=4275634312, attrs=..., buf=0x7ffff7dfd028, len=4) at /home/qemu/exec.c:3307 #22 0x000055555582b6c6 in address_space_rw (as=0x5555567a9380 <address_space_memory>, addr=4275634312, attrs=..., buf=0x7ffff7dfd028, len=4, is_write=true) at /home/qemu/exec.c:3317 #23 0x000055555588e3b8 in kvm_cpu_exec (cpu=0x555556bfe9f0) at /home/qemu/accel/kvm/kvm-all.c:2518 #24 0x0000555555972bcf in qemu_kvm_cpu_thread_fn (arg=0x555556bfe9f0) at /home/qemu/softmmu/cpus.c:1188 #25 0x0000555555e08fbd in qemu_thread_start (args=0x555556c24c60) at util/qemu-thread-posix.c:521 #26 0x00007ffff55a714a in start_thread () at /lib64/libpthread.so.0 #27 0x00007ffff52d8f23 in clone () at /lib64/libc.so.6 with entry = {target_as = 0x5555567a9380, iova = 0xfff0b000, translated_addr = 0x0, addr_mask = 0xfff, perm = 0x0} Here we got 3 levels of vtd_page_walk (frames #4-#6). The #6 parameters are: (addr=10529021952, start=0, end=549755813888, level=3, read=true, write=true, info=0x7fffddffa140) If I understand correctly, the while (iova < end) {} loop in vtd_page_walk will break the big range in small pages (4K because of level=1, and (end - iova) / subpage_size = 245 pages or iterations). That could be a lot of write(2) in vhost_kernel_send_device_iotlb_msg in the worst case, or a lot of useless returns in memory_region_notify_one_iommu because of (notifier->start > entry_end || notifier->end < entry->iova) in the best. Am I right with this? I understand that others notifiers (you mention vfio) need the granularity, but would a check in some vtd_* function for the help with the performance? (not suggesting to introduce it in this patch series). Thank you very much. > Though we'll need to modify vtd_process_device_iotlb_desc() to only send > notifications to the notifiers that registered with DEV_IOTLB flag. > > Hope it helps.. > > Thanks, > > -- > Peter Xu >