| Message ID | 20210218141650.424967-1-eric.auger@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-iommu: Handle non power of 2 range invalidations | expand |
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. I think the same trick in vtd_address_space_unmap() would work. If you agree, maybe we can generalize that get_naturally_aligned_size() out, but maybe with a better name as a helper? Thanks,
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 think the same trick in vtd_address_space_unmap() would work. If you agree, > maybe we can generalize that get_naturally_aligned_size() out, but maybe with a > better name as a helper? Yep I need to read the code again ;-) Thank you! Eric > > Thanks, >
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,
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, >
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index c2883a2f6c..a4104c99ad 100644 --- a/hw/virtio/virtio-iommu.c +++ b/hw/virtio/virtio-iommu.c @@ -155,6 +155,8 @@ static void virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start, hwaddr virt_end) { IOMMUTLBEvent event; + uint64_t mask = virt_end - virt_start; + uint64_t size; if (!(mr->iommu_notify_flags & IOMMU_NOTIFIER_UNMAP)) { return; @@ -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; + uint64_t pow2size = BIT_ULL(highest_bit); + + event.entry.addr_mask = pow2size - 1; + event.entry.iova = virt_start; + memory_region_notify_iommu(mr, 0, event); + size &= ~pow2size; + virt_start += pow2size; + } } static gboolean virtio_iommu_notify_unmap_cb(gpointer key, gpointer value,
Unmap notifiers work with an address mask assuming an invalidation range of a power of 2. Nothing mandates this in the VIRTIO-IOMMU spec. So in case the range is not a power of 2, split it into several invalidations. Signed-off-by: Eric Auger <eric.auger@redhat.com> --- hw/virtio/virtio-iommu.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-)