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

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.

> >>>>> 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,