All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] virtio-iommu: Handle non power of 2 range invalidations
@ 2021-02-18 14:16 Eric Auger
  2021-02-18 16:42 ` Peter Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Auger @ 2021-02-18 14:16 UTC (permalink / raw)
  To: eric.auger.pro, eric.auger, qemu-devel, qemu-arm, peter.maydell,
	jean-philippe
  Cc: vivek.gautam, jasowang, peterx

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



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations
  2021-02-18 14:16 [PATCH] virtio-iommu: Handle non power of 2 range invalidations Eric Auger
@ 2021-02-18 16:42 ` Peter Xu
  2021-02-18 17:18   ` Auger Eric
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Xu @ 2021-02-18 16:42 UTC (permalink / raw)
  To: Eric Auger
  Cc: peter.maydell, jean-philippe, jasowang, qemu-devel, vivek.gautam,
	qemu-arm, eric.auger.pro

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations
  2021-02-18 16:42 ` Peter Xu
@ 2021-02-18 17:18   ` Auger Eric
  2021-02-18 17:48     ` Peter Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Auger Eric @ 2021-02-18 17:18 UTC (permalink / raw)
  To: Peter Xu
  Cc: peter.maydell, jean-philippe, jasowang, qemu-devel, vivek.gautam,
	qemu-arm, eric.auger.pro

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations
  2021-02-18 17:18   ` Auger Eric
@ 2021-02-18 17:48     ` Peter Xu
  2021-02-18 18:06       ` Auger Eric
  0 siblings, 1 reply; 5+ messages in thread
From: Peter Xu @ 2021-02-18 17:48 UTC (permalink / raw)
  To: Auger Eric
  Cc: peter.maydell, jean-philippe, jasowang, qemu-devel, vivek.gautam,
	qemu-arm, eric.auger.pro

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] virtio-iommu: Handle non power of 2 range invalidations
  2021-02-18 17:48     ` Peter Xu
@ 2021-02-18 18:06       ` Auger Eric
  0 siblings, 0 replies; 5+ messages in thread
From: Auger Eric @ 2021-02-18 18:06 UTC (permalink / raw)
  To: Peter Xu
  Cc: peter.maydell, jean-philippe, jasowang, qemu-devel, vivek.gautam,
	qemu-arm, eric.auger.pro

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



^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-02-18 18:08 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-18 14:16 [PATCH] virtio-iommu: Handle non power of 2 range invalidations Eric Auger
2021-02-18 16:42 ` Peter Xu
2021-02-18 17:18   ` Auger Eric
2021-02-18 17:48     ` Peter Xu
2021-02-18 18:06       ` Auger Eric

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.