Re: [RFC v2 1/1] memory: Delete assertion in memory_region_unregister_iommu_notifier

On 2020/6/30 下午11:39, Peter Xu wrote: > On Tue, Jun 30, 2020 at 10:41:10AM +0800, Jason Wang wrote: >>>       /* According to ATS spec table 2.4: >>>        * S = 0, bits 15:12 = xxxx     range size: 4K >>>        * S = 1, bits 15:12 = xxx0     range size: 8K >>>        * S = 1, bits 15:12 = xx01     range size: 16K >>>        * S = 1, bits 15:12 = x011     range size: 32K >>>        * S = 1, bits 15:12 = 0111     range size: 64K >>>        * ... >>>        */ >> >> Right, but the comment is probably misleading here, since it's for >> the PCI-E >> transaction between IOMMU and device not for the device IOTLB >> invalidation >> descriptor. >> >> For device IOTLB invalidation descriptor, spec allows a [0, ~0ULL] >> invalidation: >> >> " >> >> 6.5.2.5 Device-TLB Invalidate Descriptor >> >> ... >> >> Size (S): The size field indicates the number of consecutive pages >> targeted >> by this invalidation >> request. If S field is zero, a single page at page address specified by >> Address [63:12] is requested >> to be invalidated. If S field is Set, the least significant bit in the >> Address field with value 0b >> indicates the invalidation address range. For example, if S field is >> Set and >> Address[12] is Clear, it >> indicates an 8KB invalidation address range with base address in >> Address >> [63:13]. If S field and >> Address[12] is Set and bit 13 is Clear, it indicates a 16KB >> invalidation >> address range with base >> address in Address [63:14], etc. >> >> " >> >> So if we receive an address whose [63] is 0 and the rest is all 1, >> it's then >> a [0, ~0ULL] invalidation. > Yes.  I think invalidating the whole range is always fine.  It's > still not > arbitrary, right?  E.g., we can't even invalidate (0x1000, 0x3000) with > device-iotlb because of the address mask, not to say sub-pages. Yes. > >> >>>>>> How about just convert to use a range [start, end] for any >>>>>> notifier and move >>>>>> the checks (e.g the assert) into the actual notifier implemented >>>>>> (vhost or >>>>>> vfio)? >>>>> IOMMUTLBEntry itself is the abstraction layer of TLB entry.  >>>>> Hardware TLB entry >>>>> is definitely not arbitrary range either (because AFAICT the >>>>> hardware should >>>>> only cache PFN rather than address, so at least PAGE_SIZE aligned). >>>>> Introducing this flag will already make this trickier just to >>>>> avoid introducing >>>>> another similar struct to IOMMUTLBEntry, but I really don't want >>>>> to make it a >>>>> default option...  Not to mention I probably have no reason to >>>>> urge the rest >>>>> iommu notifier users (tcg, vfio) to change their existing good >>>>> code to suite >>>>> any of the backend who can cooperate with arbitrary address >>>>> ranges... >>>> Ok, so it looks like we need a dedicated notifiers to device IOTLB. >>> Or we can also make a new flag for device iotlb just like current >>> UNMAP? Then >>> we replace the vhost type from UNMAP to DEVICE_IOTLB.  But IMHO >>> using the >>> ARBITRARY_LENGTH flag would work in a similar way. DEVICE_IOTLB >>> flag could >>> also allow virtio/vhost to only receive one invalidation (now IIUC >>> it'll >>> receive both iotlb and device-iotlb for unmapping a page when >>> ats=on), but then >>> ats=on will be a must and it could break some old (misconfiged) >>> qemu because >>> afaict previously virtio/vhost could even work with vIOMMU >>> (accidentally) even >>> without ats=on. >> >> That's a bug and I don't think we need to workaround >> mis-configurated qemu >> :) > IMHO it depends on the strictness we want on the qemu cmdline API. :) > > We should at least check libvirt to make sure it's using ats=on > always, then I > agree maybe we can avoid considering the rest... > > Thanks, Cc libvirt list, but I think we should fix libvirt if they don't provide "ats=on". Thanks