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