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