Re: [RFC v6 00/13] memory: Delete assertion in memory_region_unregister_iommu_notifier

[Eugenio Perez Martin]

Hi!

Sending v6 to see if that is on the same page as what you meant.
Making each setting of "type" explicitly IOMMU_IOTLB_NONE if not used
in notifications. This is done in different commits in case this helps
review of different architectures.

I think that this way we have too much freedom between entry flags
(currently only access type, RW) and notification type. Since not all
of them are valid nor used in the same context, I think this adds
complexity. I'm wondering if:

Option a) We could make it private to memory.c, and make it a flag of
memory_region_notify_iommu (like "bool deviotlb_type)". IOW, instead
of making it a member of IOMMUTLBEntry, wrap the "entry" parameter of
memory_region_notify_iommu in a new private structure defined in
memory.c that adds that flag.

Option b) We could keep the IOMMUTLBNotificationType enum (open to
suggestions for a better name :)), but not embed it in the struct,
like:

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 477c3af24c..d9150e7b7e 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -72,7 +72,8 @@ typedef enum {
     IOMMU_RO   = 1,
     IOMMU_WO   = 2,
     IOMMU_RW   = 3,
-} IOMMUAccessFlags;
+    IOMMU_DEVIOTLB = 4,
+} IOMMUEntryFlags;

 #define IOMMU_ACCESS_FLAG(r, w) (((r) ? IOMMU_RO : 0) | ((w) ? IOMMU_WO : 0))

@@ -81,10 +82,18 @@ struct IOMMUTLBEntry {
     hwaddr                   iova;
     hwaddr                   translated_addr;
     hwaddr                   addr_mask;  /* 0xfff = 4k translation */
-    IOMMUAccessFlags         perm;
+    IOMMUEntryFlags          flags;
     IOMMUTLBNotificationType type; /* Only valid if it is a notification */
 };

+IOMMUTLBNotificationType iommu_tlb_entry_type(struct IOMMUTLBEntry *s) {
+    if (s->flags & IOMMU_DEVIOTLB)
+        return IOMMU_DEVIOTLB_UNMAP;
+    else if (s->flags & IOMMU_RW)
+        return IOMMU_IOTLB_MAP;
+    return IOMMU_IOTLB_UNMAP;
+}
+
--

Thanks!

On Wed, Aug 26, 2020 at 4:38 PM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> I am able to hit this assertion when a Red Hat 7 guest virtio_net device
> raises an "Invalidation" of all the TLB entries. This happens in the
> guest's startup if 'intel_iommu=on' argument is passed to the guest
> kernel and right IOMMU/ATS devices are declared in qemu's command line.
>
> Command line:
> /home/qemu/x86_64-softmmu/qemu-system-x86_64 -name \
> guest=rhel7-test,debug-threads=on -machine \
> pc-q35-5.1,accel=kvm,usb=off,dump-guest-core=off,kernel_irqchip=split \
> -cpu \
> Broadwell,vme=on,ss=on,vmx=on,f16c=on,rdrand=on,hypervisor=on,arat=on,tsc-adjust=on,umip=on,arch-capabilities=on,xsaveopt=on,pdpe1gb=on,abm=on,skip-l1dfl-vmentry=on,rtm=on,hle=on
>  \
> -m 8096 -realtime mlock=off -smp 2,sockets=2,cores=1,threads=1 -uuid \
> d022ecbf-679e-4755-87ce-eb87fc5bbc5d -display none -no-user-config \
> -nodefaults -rtc base=utc,driftfix=slew -global \
> kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global \
> ICH9-LPC.disable_s3=1 -global ICH9-LPC.disable_s4=1 -boot strict=on \
> -device intel-iommu,intremap=on,device-iotlb=on -device \
> pcie-root-port,port=0x8,chassis=1,id=pci.1,bus=pcie.0,multifunction=on,addr=0x1
>  \
> -device \
> pcie-root-port,port=0x9,chassis=2,id=pci.2,bus=pcie.0,addr=0x1.0x1 \
> -device \
> pcie-root-port,port=0xa,chassis=3,id=pci.3,bus=pcie.0,addr=0x1.0x2 \
> -device \
> pcie-root-port,port=0xb,chassis=4,id=pci.4,bus=pcie.0,addr=0x1.0x3 \
> -device \
> pcie-root-port,port=0xc,chassis=5,id=pci.5,bus=pcie.0,addr=0x1.0x4 \
> -device \
> pcie-root-port,port=0xd,chassis=6,id=pci.6,bus=pcie.0,addr=0x1.0x5 \
> -device \
> pcie-root-port,port=0xe,chassis=7,id=pci.7,bus=pcie.0,addr=0x1.0x6 \
> -device qemu-xhci,p2=15,p3=15,id=usb,bus=pci.2,addr=0x0 -device \
> virtio-serial-pci,id=virtio-serial0,bus=pci.3,addr=0x0 -drive \
> file=/home/virtio-test2.qcow2,format=qcow2,if=none,id=drive-virtio-disk0 \
> -device \
> virtio-blk-pci,scsi=off,bus=pci.4,addr=0x0,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1
>  \
> -netdev tap,id=hostnet0,vhost=on,vhostforce=on -device \
> virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:0d:1d:f2,bus=pci.1,addr=0x0,iommu_platform=on,ats=on
>  \
> -device virtio-balloon-pci,id=balloon0,bus=pci.5,addr=0x0 -object \
> rng-random,id=objrng0,filename=/dev/urandom -device \
> virtio-rng-pci,rng=objrng0,id=rng0,bus=pci.6,addr=0x0 -s -msg \
> timestamp=on
>
> Full backtrace:
>
> #0  0x00007ffff521370f in raise () at /lib64/libc.so.6
> #1  0x00007ffff51fdb25 in abort () at /lib64/libc.so.6
> #2  0x00007ffff51fd9f9 in _nl_load_domain.cold.0 () at /lib64/libc.so.6
> #3  0x00007ffff520bcc6 in .annobin_assert.c_end () at /lib64/libc.so.6
> #4  0x0000555555888171 in memory_region_notify_one (notifier=0x7ffde0487fa8,
>                                                     entry=0x7ffde5dfe200)
>                           at /home/qemu/memory.c:1918
> #5  0x0000555555888247 in memory_region_notify_iommu (iommu_mr=0x555556f6c0b0,
>                                                       iommu_idx=0, entry=...)
>                           at /home/qemu/memory.c:1941
> #6  0x0000555555951c8d in vtd_process_device_iotlb_desc (s=0x555557609000,
>                                                        
> inv_desc=0x7ffde5dfe2d0)
>                           at /home/qemu/hw/i386/intel_iommu.c:2468
> #7  0x0000555555951e6a in vtd_process_inv_desc (s=0x555557609000)
>                           at /home/qemu/hw/i386/intel_iommu.c:2531
> #8  0x0000555555951fa5 in vtd_fetch_inv_desc (s=0x555557609000)
>                           at /home/qemu/hw/i386/intel_iommu.c:2563
> #9  0x00005555559520e5 in vtd_handle_iqt_write (s=0x555557609000)
>                           at /home/qemu/hw/i386/intel_iommu.c:2590
> #10 0x0000555555952b45 in vtd_mem_write (opaque=0x555557609000, addr=136,
>                                          val=2688, size=4)
>                           at /home/qemu/hw/i386/intel_iommu.c:2837
> #11 0x0000555555883e17 in memory_region_write_accessor (mr=0x555557609330,
>                                                         addr=136,
>                                                         value=0x7ffde5dfe478,
>                                                         size=4,
>                                                         shift=0,
>                                                         mask=4294967295,
>                                                         attrs=...)
>                          at /home/qemu/memory.c:483
> #12 0x000055555588401d in access_with_adjusted_size (addr=136,
>                        value=0x7ffde5dfe478,
>                        size=4,
>                        access_size_min=4,
>                        access_size_max=8,
>                        access_fn=0x555555883d38 
> <memory_region_write_accessor>,
>                        mr=0x555557609330,
>                        attrs=...)
>                        at /home/qemu/memory.c:544
> #13 0x0000555555886f37 in memory_region_dispatch_write (mr=0x555557609330,
>                                                        addr=136,
>                                                        data=2688,
>                                                        op=MO_32,
>                                                        attrs=...)
>                          at /home/qemu/memory.c:1476
> #14 0x0000555555827a03 in flatview_write_continue (fv=0x7ffdd8503150,
>                                                    addr=4275634312,
>                                                    attrs=...,
>                                                    ptr=0x7ffff7ff0028,
>                                                    len=4,
>                                                    addr1=136,
>                                                    l=4,
>                                                    mr=0x555557609330)
>                           at /home/qemu/exec.c:3146
> #15 0x0000555555827b48 in flatview_write (fv=0x7ffdd8503150,
>                                           addr=4275634312,
>                                           attrs=...,
>                                           buf=0x7ffff7ff0028,
>                                           len=4)
>                           at /home/qemu/exec.c:3186
> #16 0x0000555555827e9d in address_space_write (
>                                       as=0x5555567ca640 
> <address_space_memory>,
>                                       addr=4275634312,
>                                       attrs=...,
>                                       buf=0x7ffff7ff0028,
>                                       len=4)
>                           at /home/qemu/exec.c:3277
> #17 0x0000555555827f0a in address_space_rw (
>                                       as=0x5555567ca640 
> <address_space_memory>,
>                                       addr=4275634312,
>                                       attrs=...,
>                                       buf=0x7ffff7ff0028,
>                                       len=4,
>                                       is_write=true)
>                           at /home/qemu/exec.c:3287
> #18 0x000055555589b633 in kvm_cpu_exec (cpu=0x555556b65640)
>                                at /home/qemu/accel/kvm/kvm-all.c:2511
> #19 0x0000555555876ba8 in qemu_kvm_cpu_thread_fn (arg=0x555556b65640)
>                                at /home/qemu/cpus.c:1284
> #20 0x0000555555dafff1 in qemu_thread_start (args=0x555556b8c3b0)
>                                at util/qemu-thread-posix.c:521
> #21 0x00007ffff55a62de in start_thread () at /lib64/libpthread.so.0
> #22 0x00007ffff52d7e83 in clone () at /lib64/libc.so.6
>
> (gdb) frame 4
> #4  0x0000555555888171 in memory_region_notify_one
>                       (notifier=0x7ffde0487fa8, entry=0x7ffde5dfe200)
>                       at /home/qemu/memory.c:1918
> 1918        assert(entry->iova >= notifier->start && entry_end <=
> notifier->end);
> (gdb) p *entry
> $1 = {target_as = 0x555556f6c050, iova = 0, translated_addr = 0,
> addr_mask = 18446744073709551615, perm = IOMMU_NONE}
> --
>
> Tested with vhost-net, host<->guest communication. Still more testing
> needed, since it touches a few architectures and configurations.
>
> v6: Introduce "type" field for IOMMUTLBEntry. Fill in all uses.
>     Update tests reports with more fine-tuning (CPU, RPS/XPS tunning).
>
> v5: Skip regular IOTLB notifications in dev_iotlb notifiers
>
> v4: Rename IOMMU_NOTIFIER_IOTLB -> IOMMU_NOTIFIER_DEVIOTLB.
>     Make vhost-net notifier just IOMMU_NOTIFIER_DEVIOTLB, not
>     IOMMU_NOTIFIER_UNMAP
>
> v3: Skip the assertion in case notifier is a IOTLB one, since they can manage
>     arbitrary ranges. Using a flag in the notifier for now, as Peter 
> suggested.
>
> v2: Actually delete assertion instead of just commenting out using C99
>
> Eugenio Pérez (13):
>   memory: Rename memory_region_notify_one to
>     memory_region_notify_iommu_one
>   memory: Add IOMMUTLBNotificationType to IOMMUTLBEntry
>   hw/alpha/typhoon: Mark all IOMMUTLBEntry as IOMMU_IOTLB_NONE type
>   amd_iommu: Mark all IOMMUTLBEntry as IOMMU_IOTLB_NONE type
>   hw/arm/smmu: Fill IOMMUTLBEntry notifier type
>   dma/rc4030: Mark all IOMMUTLBEntry as IOMMU_IOTLB_NONE type
>   intel_iommu: Mark IOMMUTLBEntry of page notification as
>     IOMMU_IOTLB_UNMAP type
>   virtio-iommu: Mark virtio_iommu_translate IOTLB as IOMMU_IOTLB_NONE
>     type
>   intel_iommu: Set IOMMUTLBEntry type in vtd_page_walk_level
>   memory: Notify IOMMU IOTLB based on entry type, not permissions
>   memory: Add IOMMU_DEVIOTLB_UNMAP IOMMUTLBNotificationType
>   intel_iommu: Do not notify regular iotlb to device-iotlb notifiers
>   memory: Skip bad range assertion if notifier is DEVIOTLB type
>
>  hw/alpha/typhoon.c       |  1 +
>  hw/arm/smmu-common.c     |  4 +++-
>  hw/arm/smmuv3.c          |  4 +++-
>  hw/dma/rc4030.c          |  1 +
>  hw/i386/amd_iommu.c      |  7 ++++++-
>  hw/i386/intel_iommu.c    | 14 +++++++++++--
>  hw/virtio/vhost.c        |  2 +-
>  hw/virtio/virtio-iommu.c |  1 +
>  include/exec/memory.h    | 26 ++++++++++++++++-------
>  softmmu/memory.c         | 45 ++++++++++++++++++++++++++++++----------
>  10 files changed, 80 insertions(+), 25 deletions(-)
>
> --
> 2.18.1
>
>