Hi Peter, On 2/18/21 6:48 PM, Peter Xu wrote: > On Thu, Feb 18, 2021 at 06:18:22PM +0100, Auger Eric wrote: >> Hi Peter, >> >> On 2/18/21 5:42 PM, Peter Xu wrote: >>> Eric, >>> >>> On Thu, Feb 18, 2021 at 03:16:50PM +0100, Eric Auger wrote: >>>> @@ -164,12 +166,27 @@ static void >>>> virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start, >>>> >>>> event.type = IOMMU_NOTIFIER_UNMAP; >>>> event.entry.target_as = &address_space_memory; >>>> - event.entry.addr_mask = virt_end - virt_start; >>>> - event.entry.iova = virt_start; >>>> event.entry.perm = IOMMU_NONE; >>>> event.entry.translated_addr = 0; >>>> + event.entry.addr_mask = mask; >>>> + event.entry.iova = virt_start; >>>> >>>> - memory_region_notify_iommu(mr, 0, event); >>>> + if (mask == UINT64_MAX) { >>>> + memory_region_notify_iommu(mr, 0, event); >>>> + } >>>> + >>>> + size = mask + 1; >>>> + >>>> + while (size) { >>>> + uint8_t highest_bit = 64 - clz64(size) - 1; >>> >>> I'm not sure fetching highest bit would work right. E.g., with start=0x11000 >>> and size=0x11000 (then we need to unmap 0x11000-0x22000), current code will >>> first try to invalidate range (0x11000, 0x10000), that seems still invalid >>> since 0x11000 is not aligned to 0x10000 page mask. >> >> Hum I thought aligning the size was sufficient. Where is it checked exactly? > > I don't remember all the context either.. :) > > Firstly - It makes sense to do that since hardware does it, and emulation code > would make sense to follow that.> > There's some more info where I looked into the src of when vt-d got introduced > with the similar change: > > https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html > > I'm not 100% certain anything will break if we don't use page mask but > arbitrary length as vhost did in iotlb msg. However what Yan Zhao reported is > definitely worse than that since we (vt-d) used to unmap outside the range of > the range just for page mask alignment.
OK. I read again the get_naturally_aligned_size() code and indeed it matches the need while taking into account the largest page size bigger than the start @ alignment. This is a common pattern anyway. I am not sure either this would break with vhost and vfio notifications but better to use that function too in virtio-iommu and smmu (I sent a similar patch for smmu). Thank you for reminding me of that piece of code! Eric > > Thanks, >