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