[PATCH] virtio-iommu: Handle non power of 2 range invalidations

[PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Eric Auger]

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(-)

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,
-- 
2.26.2

Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Auger Eric]

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

Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Peter Xu]

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

Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations

[Auger Eric]

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