[Qemu-devel] [PATCH v2 0/2] intel_iommu: Fix unexpected unmaps during global unmap

[Qemu-devel] [PATCH v2 0/2] intel_iommu: Fix unexpected unmaps during global unmap

[Peter Xu]

v2:
- rename helper to get_naturally_aligned_size(), simplify the
  codes as suggested [Paolo]
- check against vtd page size when looping over for unmaps [Yan]
- add r-b for Eric

Please review, thanks.

Peter Xu (1):
  intel_iommu: Fix unexpected unmaps during global unmap

Yan Zhao (1):
  intel_iommu: Fix incorrect "end" for vtd_address_space_unmap

 hw/i386/intel_iommu.c | 71 ++++++++++++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 28 deletions(-)

-- 
2.21.0

[Qemu-devel] [PATCH v2 1/2] intel_iommu: Fix incorrect "end" for vtd_address_space_unmap

[Peter Xu]

From: Yan Zhao <yan.y.zhao@intel.com>

IOMMUNotifier is with inclusive ranges, so we should check
against (VTD_ADDRESS_SIZE(s->aw_bits) - 1).

Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
[peterx: split from another bigger patch]
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 44b1231157..719ce19ab3 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3379,12 +3379,12 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
      * VT-d spec), otherwise we need to consider overflow of 64 bits.
      */
 
-    if (end > VTD_ADDRESS_SIZE(s->aw_bits)) {
+    if (end > VTD_ADDRESS_SIZE(s->aw_bits) - 1) {
         /*
          * Don't need to unmap regions that is bigger than the whole
          * VT-d supported address space size
          */
-        end = VTD_ADDRESS_SIZE(s->aw_bits);
+        end = VTD_ADDRESS_SIZE(s->aw_bits) - 1;
     }
 
     assert(start <= end);
-- 
2.21.0

[Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Peter Xu]

This is an replacement work of Yan Zhao's patch:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html

vtd_address_space_unmap() will do proper page mask alignment to make
sure each IOTLB message will have correct masks for notification
messages (2^N-1), but sometimes it can be expanded to even supercede
the registered range.  That could lead to unexpected UNMAP of already
mapped regions in some other notifiers.

Instead of doing mindless expension of the start address and address
mask, we split the range into smaller ones and guarantee that each
small range will have correct masks (2^N-1) and at the same time we
should also try our best to generate as less IOTLB messages as
possible.

Reported-by: Yan Zhao <yan.y.zhao@intel.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 26 deletions(-)

diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
index 719ce19ab3..de86f53b4e 100644
--- a/hw/i386/intel_iommu.c
+++ b/hw/i386/intel_iommu.c
@@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
     return vtd_dev_as;
 }
 
+static uint64_t get_naturally_aligned_size(uint64_t start,
+                                           uint64_t size, int gaw)
+{
+    uint64_t max_mask = 1ULL << gaw;
+    uint64_t alignment = start ? start & -start : max_mask;
+
+    alignment = MIN(alignment, max_mask);
+    size = MIN(size, max_mask);
+
+    if (alignment <= size) {
+        /* Increase the alignment of start */
+        return alignment;
+    } else {
+        /* Find the largest page mask from size */
+        return 1ULL << (63 - clz64(size));
+    }
+}
+
 /* Unmap the whole range in the notifier's scope. */
 static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
 {
-    IOMMUTLBEntry entry;
-    hwaddr size;
+    hwaddr size, remain;
     hwaddr start = n->start;
     hwaddr end = n->end;
     IntelIOMMUState *s = as->iommu_state;
@@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
     }
 
     assert(start <= end);
-    size = end - start;
+    size = remain = end - start + 1;
 
-    if (ctpop64(size) != 1) {
-        /*
-         * This size cannot format a correct mask. Let's enlarge it to
-         * suite the minimum available mask.
-         */
-        int n = 64 - clz64(size);
-        if (n > s->aw_bits) {
-            /* should not happen, but in case it happens, limit it */
-            n = s->aw_bits;
-        }
-        size = 1ULL << n;
+    while (remain >= VTD_PAGE_SIZE) {
+        IOMMUTLBEntry entry;
+        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
+
+        assert(mask);
+
+        entry.iova = start;
+        entry.addr_mask = mask - 1;
+        entry.target_as = &address_space_memory;
+        entry.perm = IOMMU_NONE;
+        /* This field is meaningless for unmap */
+        entry.translated_addr = 0;
+
+        memory_region_notify_one(n, &entry);
+
+        start += mask;
+        remain -= mask;
     }
 
-    entry.target_as = &address_space_memory;
-    /* Adjust iova for the size */
-    entry.iova = n->start & ~(size - 1);
-    /* This field is meaningless for unmap */
-    entry.translated_addr = 0;
-    entry.perm = IOMMU_NONE;
-    entry.addr_mask = size - 1;
+    assert(!remain);
 
     trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
                              VTD_PCI_SLOT(as->devfn),
                              VTD_PCI_FUNC(as->devfn),
-                             entry.iova, size);
+                             n->start, size);
 
-    map.iova = entry.iova;
-    map.size = entry.addr_mask;
+    map.iova = n->start;
+    map.size = size;
     iova_tree_remove(as->iova_tree, &map);
-
-    memory_region_notify_one(n, &entry);
 }
 
 static void vtd_address_space_unmap_all(IntelIOMMUState *s)
-- 
2.21.0

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Auger Eric]

Hi Peter,

On 6/24/19 11:18 AM, Peter Xu wrote:
> This is an replacement work of Yan Zhao's patch:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> 
> vtd_address_space_unmap() will do proper page mask alignment to make
> sure each IOTLB message will have correct masks for notification
> messages (2^N-1), but sometimes it can be expanded to even supercede
> the registered range.  That could lead to unexpected UNMAP of already
> mapped regions in some other notifiers.
> 
> Instead of doing mindless expension of the start address and address
> mask, we split the range into smaller ones and guarantee that each
> small range will have correct masks (2^N-1) and at the same time we
> should also try our best to generate as less IOTLB messages as
> possible.
> 
> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 719ce19ab3..de86f53b4e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>      return vtd_dev_as;
>  }
>  
> +static uint64_t get_naturally_aligned_size(uint64_t start,
> +                                           uint64_t size, int gaw)
> +{
> +    uint64_t max_mask = 1ULL << gaw;
> +    uint64_t alignment = start ? start & -start : max_mask;
> +
> +    alignment = MIN(alignment, max_mask);
> +    size = MIN(size, max_mask);
this does not not prevent from invalidating beyond gaw if start != 0, right?
> +
> +    if (alignment <= size) {
> +        /* Increase the alignment of start */
I don't really get this comment
> +        return alignment;
> +    } else {
> +        /* Find the largest page mask from size */
> +        return 1ULL << (63 - clz64(size));
> +    }> +}
> +
>  /* Unmap the whole range in the notifier's scope. */
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  {
> -    IOMMUTLBEntry entry;
> -    hwaddr size;
> +    hwaddr size, remain;
>      hwaddr start = n->start;
>      hwaddr end = n->end;
>      IntelIOMMUState *s = as->iommu_state;
> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>      }
>  
>      assert(start <= end);
> -    size = end - start;
> +    size = remain = end - start + 1;
>  
> -    if (ctpop64(size) != 1) {
> -        /*
> -         * This size cannot format a correct mask. Let's enlarge it to
> -         * suite the minimum available mask.
> -         */
> -        int n = 64 - clz64(size);
> -        if (n > s->aw_bits) {
> -            /* should not happen, but in case it happens, limit it */
> -            n = s->aw_bits;
> -        }
> -        size = 1ULL << n;
> +    while (remain >= VTD_PAGE_SIZE) {
Can't we stop as soon as entry.iova exceeds gaw as well?
> +        IOMMUTLBEntry entry;
> +        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
> +
> +        assert(mask);
> +
> +        entry.iova = start;
> +        entry.addr_mask = mask - 1;
> +        entry.target_as = &address_space_memory;
> +        entry.perm = IOMMU_NONE;
> +        /* This field is meaningless for unmap */
> +        entry.translated_addr = 0;
> +
> +        memory_region_notify_one(n, &entry);
> +
> +        start += mask;
> +        remain -= mask;
>      }
>  
> -    entry.target_as = &address_space_memory;
> -    /* Adjust iova for the size */
> -    entry.iova = n->start & ~(size - 1);
> -    /* This field is meaningless for unmap */
> -    entry.translated_addr = 0;
> -    entry.perm = IOMMU_NONE;
> -    entry.addr_mask = size - 1;
> +    assert(!remain);
>  
>      trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                               VTD_PCI_SLOT(as->devfn),
>                               VTD_PCI_FUNC(as->devfn),
> -                             entry.iova, size);
> +                             n->start, size);
>  
> -    map.iova = entry.iova;
> -    map.size = entry.addr_mask;
> +    map.iova = n->start;
> +    map.size = size;
>      iova_tree_remove(as->iova_tree, &map);
> -
> -    memory_region_notify_one(n, &entry);
>  }
>  
>  static void vtd_address_space_unmap_all(IntelIOMMUState *s)
> 

Thanks

Eric

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Paolo Bonzini]

On 24/06/19 11:18, Peter Xu wrote:
> This is an replacement work of Yan Zhao's patch:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> 
> vtd_address_space_unmap() will do proper page mask alignment to make
> sure each IOTLB message will have correct masks for notification
> messages (2^N-1), but sometimes it can be expanded to even supercede
> the registered range.  That could lead to unexpected UNMAP of already
> mapped regions in some other notifiers.
> 
> Instead of doing mindless expension of the start address and address
> mask, we split the range into smaller ones and guarantee that each
> small range will have correct masks (2^N-1) and at the same time we
> should also try our best to generate as less IOTLB messages as
> possible.
> 
> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 719ce19ab3..de86f53b4e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>      return vtd_dev_as;
>  }
>  
> +static uint64_t get_naturally_aligned_size(uint64_t start,
> +                                           uint64_t size, int gaw)
> +{
> +    uint64_t max_mask = 1ULL << gaw;
> +    uint64_t alignment = start ? start & -start : max_mask;
> +
> +    alignment = MIN(alignment, max_mask);
> +    size = MIN(size, max_mask);
> +
> +    if (alignment <= size) {
> +        /* Increase the alignment of start */
> +        return alignment;
> +    } else {
> +        /* Find the largest page mask from size */
> +        return 1ULL << (63 - clz64(size));
> +    }
> +}
> +
>  /* Unmap the whole range in the notifier's scope. */
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  {
> -    IOMMUTLBEntry entry;
> -    hwaddr size;
> +    hwaddr size, remain;
>      hwaddr start = n->start;
>      hwaddr end = n->end;
>      IntelIOMMUState *s = as->iommu_state;
> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>      }
>  
>      assert(start <= end);
> -    size = end - start;
> +    size = remain = end - start + 1;
>  
> -    if (ctpop64(size) != 1) {
> -        /*
> -         * This size cannot format a correct mask. Let's enlarge it to
> -         * suite the minimum available mask.
> -         */
> -        int n = 64 - clz64(size);
> -        if (n > s->aw_bits) {
> -            /* should not happen, but in case it happens, limit it */
> -            n = s->aw_bits;
> -        }
> -        size = 1ULL << n;
> +    while (remain >= VTD_PAGE_SIZE) {
> +        IOMMUTLBEntry entry;
> +        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
> +
> +        assert(mask);
> +
> +        entry.iova = start;
> +        entry.addr_mask = mask - 1;
> +        entry.target_as = &address_space_memory;
> +        entry.perm = IOMMU_NONE;
> +        /* This field is meaningless for unmap */
> +        entry.translated_addr = 0;
> +
> +        memory_region_notify_one(n, &entry);
> +
> +        start += mask;
> +        remain -= mask;
>      }
>  
> -    entry.target_as = &address_space_memory;
> -    /* Adjust iova for the size */
> -    entry.iova = n->start & ~(size - 1);
> -    /* This field is meaningless for unmap */
> -    entry.translated_addr = 0;
> -    entry.perm = IOMMU_NONE;
> -    entry.addr_mask = size - 1;
> +    assert(!remain);
>  
>      trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                               VTD_PCI_SLOT(as->devfn),
>                               VTD_PCI_FUNC(as->devfn),
> -                             entry.iova, size);
> +                             n->start, size);
>  
> -    map.iova = entry.iova;
> -    map.size = entry.addr_mask;
> +    map.iova = n->start;
> +    map.size = size;
>      iova_tree_remove(as->iova_tree, &map);
> -
> -    memory_region_notify_one(n, &entry);
>  }
>  
>  static void vtd_address_space_unmap_all(IntelIOMMUState *s)
> 

Looks good, ignore my previous message.

Paolo

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Peter Xu]

On Mon, Jun 24, 2019 at 12:09:48PM +0200, Auger Eric wrote:
> Hi Peter,
> 
> On 6/24/19 11:18 AM, Peter Xu wrote:
> > This is an replacement work of Yan Zhao's patch:
> > 
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> > 
> > vtd_address_space_unmap() will do proper page mask alignment to make
> > sure each IOTLB message will have correct masks for notification
> > messages (2^N-1), but sometimes it can be expanded to even supercede
> > the registered range.  That could lead to unexpected UNMAP of already
> > mapped regions in some other notifiers.
> > 
> > Instead of doing mindless expension of the start address and address
> > mask, we split the range into smaller ones and guarantee that each
> > small range will have correct masks (2^N-1) and at the same time we
> > should also try our best to generate as less IOTLB messages as
> > possible.
> > 
> > Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
> >  1 file changed, 41 insertions(+), 26 deletions(-)
> > 
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 719ce19ab3..de86f53b4e 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
> >      return vtd_dev_as;
> >  }
> >  
> > +static uint64_t get_naturally_aligned_size(uint64_t start,
> > +                                           uint64_t size, int gaw)
> > +{
> > +    uint64_t max_mask = 1ULL << gaw;
> > +    uint64_t alignment = start ? start & -start : max_mask;
> > +
> > +    alignment = MIN(alignment, max_mask);
> > +    size = MIN(size, max_mask);
> this does not not prevent from invalidating beyond gaw if start != 0, right?

Yes.  But at the start of vtd_address_space_unmap(), we have:

    if (end > VTD_ADDRESS_SIZE(s->aw_bits) - 1) {
        /*
         * Don't need to unmap regions that is bigger than the whole
         * VT-d supported address space size
         */
        end = VTD_ADDRESS_SIZE(s->aw_bits) - 1;
    }

So we don't need to worry about (start+size) exceeding GAW?

[1]

> > +
> > +    if (alignment <= size) {
> > +        /* Increase the alignment of start */
> I don't really get this comment

This comment comes from Paolo, but I'll try to explain - it tries to
mean that this "alignment" will be used as an increasement to "start"
variable, so finally variable "start" will align with larger mask
size.

Better comments welcomed... :)

> > +        return alignment;
> > +    } else {
> > +        /* Find the largest page mask from size */
> > +        return 1ULL << (63 - clz64(size));
> > +    }> +}
> > +
> >  /* Unmap the whole range in the notifier's scope. */
> >  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> >  {
> > -    IOMMUTLBEntry entry;
> > -    hwaddr size;
> > +    hwaddr size, remain;
> >      hwaddr start = n->start;
> >      hwaddr end = n->end;
> >      IntelIOMMUState *s = as->iommu_state;
> > @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
> >      }
> >  
> >      assert(start <= end);
> > -    size = end - start;
> > +    size = remain = end - start + 1;
> >  
> > -    if (ctpop64(size) != 1) {
> > -        /*
> > -         * This size cannot format a correct mask. Let's enlarge it to
> > -         * suite the minimum available mask.
> > -         */
> > -        int n = 64 - clz64(size);
> > -        if (n > s->aw_bits) {
> > -            /* should not happen, but in case it happens, limit it */
> > -            n = s->aw_bits;
> > -        }
> > -        size = 1ULL << n;
> > +    while (remain >= VTD_PAGE_SIZE) {
> Can't we stop as soon as entry.iova exceeds gaw as well?

As explained at [1], I think we've already checked it.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Auger Eric]

On 6/24/19 1:10 PM, Peter Xu wrote:
> On Mon, Jun 24, 2019 at 12:09:48PM +0200, Auger Eric wrote:
>> Hi Peter,
>>
>> On 6/24/19 11:18 AM, Peter Xu wrote:
>>> This is an replacement work of Yan Zhao's patch:
>>>
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
>>>
>>> vtd_address_space_unmap() will do proper page mask alignment to make
>>> sure each IOTLB message will have correct masks for notification
>>> messages (2^N-1), but sometimes it can be expanded to even supercede
>>> the registered range.  That could lead to unexpected UNMAP of already
>>> mapped regions in some other notifiers.
>>>
>>> Instead of doing mindless expension of the start address and address
>>> mask, we split the range into smaller ones and guarantee that each
>>> small range will have correct masks (2^N-1) and at the same time we
>>> should also try our best to generate as less IOTLB messages as
>>> possible.
>>>
>>> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>>>  1 file changed, 41 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
>>> index 719ce19ab3..de86f53b4e 100644
>>> --- a/hw/i386/intel_iommu.c
>>> +++ b/hw/i386/intel_iommu.c
>>> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>>>      return vtd_dev_as;
>>>  }
>>>  
>>> +static uint64_t get_naturally_aligned_size(uint64_t start,
>>> +                                           uint64_t size, int gaw)
>>> +{
>>> +    uint64_t max_mask = 1ULL << gaw;
>>> +    uint64_t alignment = start ? start & -start : max_mask;
>>> +
>>> +    alignment = MIN(alignment, max_mask);
>>> +    size = MIN(size, max_mask);
>> this does not not prevent from invalidating beyond gaw if start != 0, right?
> 
> Yes.  But at the start of vtd_address_space_unmap(), we have:
> 
>     if (end > VTD_ADDRESS_SIZE(s->aw_bits) - 1) {
>         /*
>          * Don't need to unmap regions that is bigger than the whole
>          * VT-d supported address space size
>          */
>         end = VTD_ADDRESS_SIZE(s->aw_bits) - 1;
>     }
> 
> So we don't need to worry about (start+size) exceeding GAW?
Hum yes. Reviewed the previous patch with blinkers ...
> 
> [1]
> 
>>> +
>>> +    if (alignment <= size) {
>>> +        /* Increase the alignment of start */
>> I don't really get this comment
> 
> This comment comes from Paolo, but I'll try to explain - it tries to
> mean that this "alignment" will be used as an increasement to "start"
> variable, so finally variable "start" will align with larger mask
> size.
> 
> Better comments welcomed... :)
smallest page mask from @start or gaw?
> 
>>> +        return alignment;
>>> +    } else {
>>> +        /* Find the largest page mask from size */
>>> +        return 1ULL << (63 - clz64(size));
>>> +    }> +}
>>> +
>>>  /* Unmap the whole range in the notifier's scope. */
>>>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>>>  {
>>> -    IOMMUTLBEntry entry;
>>> -    hwaddr size;
>>> +    hwaddr size, remain;
>>>      hwaddr start = n->start;
>>>      hwaddr end = n->end;
>>>      IntelIOMMUState *s = as->iommu_state;
>>> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>>>      }
>>>  
>>>      assert(start <= end);
>>> -    size = end - start;
>>> +    size = remain = end - start + 1;
>>>  
>>> -    if (ctpop64(size) != 1) {
>>> -        /*
>>> -         * This size cannot format a correct mask. Let's enlarge it to
>>> -         * suite the minimum available mask.
>>> -         */
>>> -        int n = 64 - clz64(size);
>>> -        if (n > s->aw_bits) {
>>> -            /* should not happen, but in case it happens, limit it */
>>> -            n = s->aw_bits;
>>> -        }
>>> -        size = 1ULL << n;
>>> +    while (remain >= VTD_PAGE_SIZE) {
>> Can't we stop as soon as entry.iova exceeds gaw as well?
> 
> As explained at [1], I think we've already checked it.
OK

Thanks

Eric
> 
> Thanks,
> 

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Paolo Bonzini]

On 24/06/19 14:48, Auger Eric wrote:
>>>> +    if (alignment <= size) {
>>>> +        /* Increase the alignment of start */
>>> I don't really get this comment
>> This comment comes from Paolo, but I'll try to explain - it tries to
>> mean that this "alignment" will be used as an increasement to "start"
>> variable, so finally variable "start" will align with larger mask
>> size.
>>
>> Better comments welcomed... :)
> smallest page mask from @start or gaw?

What it means is that there will be "more 0 bits" at the beginning of
start.  Perhaps "On the next iteration start will be aligned to a bigger
power of two"?  I can do this when applying.

Paolo

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Yan Zhao]

Tested-by: Yan Zhao <yan.y.zhao@intel.com>

On Mon, Jun 24, 2019 at 05:18:11PM +0800, Peter Xu wrote:
> This is an replacement work of Yan Zhao's patch:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> 
> vtd_address_space_unmap() will do proper page mask alignment to make
> sure each IOTLB message will have correct masks for notification
> messages (2^N-1), but sometimes it can be expanded to even supercede
> the registered range.  That could lead to unexpected UNMAP of already
> mapped regions in some other notifiers.
> 
> Instead of doing mindless expension of the start address and address
> mask, we split the range into smaller ones and guarantee that each
> small range will have correct masks (2^N-1) and at the same time we
> should also try our best to generate as less IOTLB messages as
> possible.
> 
> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 719ce19ab3..de86f53b4e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>      return vtd_dev_as;
>  }
>  
> +static uint64_t get_naturally_aligned_size(uint64_t start,
> +                                           uint64_t size, int gaw)
> +{
> +    uint64_t max_mask = 1ULL << gaw;
> +    uint64_t alignment = start ? start & -start : max_mask;
> +
> +    alignment = MIN(alignment, max_mask);
> +    size = MIN(size, max_mask);
> +
> +    if (alignment <= size) {
> +        /* Increase the alignment of start */
> +        return alignment;
> +    } else {
> +        /* Find the largest page mask from size */
> +        return 1ULL << (63 - clz64(size));
> +    }
> +}
> +
>  /* Unmap the whole range in the notifier's scope. */
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  {
> -    IOMMUTLBEntry entry;
> -    hwaddr size;
> +    hwaddr size, remain;
>      hwaddr start = n->start;
>      hwaddr end = n->end;
>      IntelIOMMUState *s = as->iommu_state;
> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>      }
>  
>      assert(start <= end);
> -    size = end - start;
> +    size = remain = end - start + 1;
>  
> -    if (ctpop64(size) != 1) {
> -        /*
> -         * This size cannot format a correct mask. Let's enlarge it to
> -         * suite the minimum available mask.
> -         */
> -        int n = 64 - clz64(size);
> -        if (n > s->aw_bits) {
> -            /* should not happen, but in case it happens, limit it */
> -            n = s->aw_bits;
> -        }
> -        size = 1ULL << n;
> +    while (remain >= VTD_PAGE_SIZE) {
> +        IOMMUTLBEntry entry;
> +        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
> +
> +        assert(mask);
> +
> +        entry.iova = start;
> +        entry.addr_mask = mask - 1;
> +        entry.target_as = &address_space_memory;
> +        entry.perm = IOMMU_NONE;
> +        /* This field is meaningless for unmap */
> +        entry.translated_addr = 0;
> +
> +        memory_region_notify_one(n, &entry);
> +
> +        start += mask;
> +        remain -= mask;
>      }
>  
> -    entry.target_as = &address_space_memory;
> -    /* Adjust iova for the size */
> -    entry.iova = n->start & ~(size - 1);
> -    /* This field is meaningless for unmap */
> -    entry.translated_addr = 0;
> -    entry.perm = IOMMU_NONE;
> -    entry.addr_mask = size - 1;
> +    assert(!remain);
>  
>      trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                               VTD_PCI_SLOT(as->devfn),
>                               VTD_PCI_FUNC(as->devfn),
> -                             entry.iova, size);
> +                             n->start, size);
>  
> -    map.iova = entry.iova;
> -    map.size = entry.addr_mask;
> +    map.iova = n->start;
> +    map.size = size;
>      iova_tree_remove(as->iova_tree, &map);
> -
> -    memory_region_notify_one(n, &entry);
>  }
>  
>  static void vtd_address_space_unmap_all(IntelIOMMUState *s)
> -- 
> 2.21.0
> 

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Auger Eric]

Hi Peter,

On 6/24/19 11:18 AM, Peter Xu wrote:
> This is an replacement work of Yan Zhao's patch:
> 
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> 
> vtd_address_space_unmap() will do proper page mask alignment to make
> sure each IOTLB message will have correct masks for notification
> messages (2^N-1), but sometimes it can be expanded to even supercede
> the registered range.  That could lead to unexpected UNMAP of already
> mapped regions in some other notifiers.
> 
> Instead of doing mindless expension of the start address and address
> mask, we split the range into smaller ones and guarantee that each
> small range will have correct masks (2^N-1) and at the same time we
> should also try our best to generate as less IOTLB messages as
> possible.
> 
> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> ---
>  hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>  1 file changed, 41 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 719ce19ab3..de86f53b4e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>      return vtd_dev_as;
>  }
>  
> +static uint64_t get_naturally_aligned_size(uint64_t start,
> +                                           uint64_t size, int gaw)
> +{
> +    uint64_t max_mask = 1ULL << gaw;
> +    uint64_t alignment = start ? start & -start : max_mask;
> +
> +    alignment = MIN(alignment, max_mask);
> +    size = MIN(size, max_mask);
> +
> +    if (alignment <= size) {
> +        /* Increase the alignment of start */
> +        return alignment;
> +    } else {
> +        /* Find the largest page mask from size */
> +        return 1ULL << (63 - clz64(size));
> +    }
> +}
> +
>  /* Unmap the whole range in the notifier's scope. */
>  static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>  {
> -    IOMMUTLBEntry entry;
> -    hwaddr size;
> +    hwaddr size, remain;
>      hwaddr start = n->start;
>      hwaddr end = n->end;
>      IntelIOMMUState *s = as->iommu_state;
> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>      }
>  
>      assert(start <= end);
> -    size = end - start;
> +    size = remain = end - start + 1;
>  
> -    if (ctpop64(size) != 1) {
> -        /*
> -         * This size cannot format a correct mask. Let's enlarge it to
> -         * suite the minimum available mask.
> -         */
> -        int n = 64 - clz64(size);
> -        if (n > s->aw_bits) {
> -            /* should not happen, but in case it happens, limit it */
> -            n = s->aw_bits;
> -        }
> -        size = 1ULL << n;
> +    while (remain >= VTD_PAGE_SIZE) {
> +        IOMMUTLBEntry entry;
> +        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
> +
> +        assert(mask);
> +
> +        entry.iova = start;
> +        entry.addr_mask = mask - 1;
> +        entry.target_as = &address_space_memory;
> +        entry.perm = IOMMU_NONE;
> +        /* This field is meaningless for unmap */
> +        entry.translated_addr = 0;
> +
> +        memory_region_notify_one(n, &entry);
> +
> +        start += mask;
> +        remain -= mask;
>      }
>  
> -    entry.target_as = &address_space_memory;
> -    /* Adjust iova for the size */
> -    entry.iova = n->start & ~(size - 1);
> -    /* This field is meaningless for unmap */
> -    entry.translated_addr = 0;
> -    entry.perm = IOMMU_NONE;
> -    entry.addr_mask = size - 1;
> +    assert(!remain);
>  
>      trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                               VTD_PCI_SLOT(as->devfn),
>                               VTD_PCI_FUNC(as->devfn),
> -                             entry.iova, size);
> +                             n->start, size);
>  
> -    map.iova = entry.iova;
> -    map.size = entry.addr_mask;
> +    map.iova = n->start;
> +    map.size = size;
>      iova_tree_remove(as->iova_tree, &map);
> -
> -    memory_region_notify_one(n, &entry);
>  }
>  
>  static void vtd_address_space_unmap_all(IntelIOMMUState *s)
> 

Re: [Qemu-devel] [PATCH v2 0/2] intel_iommu: Fix unexpected unmaps during global unmap

[Michael S. Tsirkin]

On Mon, Jun 24, 2019 at 05:18:09PM +0800, Peter Xu wrote:
> v2:
> - rename helper to get_naturally_aligned_size(), simplify the
>   codes as suggested [Paolo]
> - check against vtd page size when looping over for unmaps [Yan]
> - add r-b for Eric
> 
> Please review, thanks.

Series:
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

> Peter Xu (1):
>   intel_iommu: Fix unexpected unmaps during global unmap
> 
> Yan Zhao (1):
>   intel_iommu: Fix incorrect "end" for vtd_address_space_unmap
> 
>  hw/i386/intel_iommu.c | 71 ++++++++++++++++++++++++++-----------------
>  1 file changed, 43 insertions(+), 28 deletions(-)
> 
> -- 
> 2.21.0

Re: [Qemu-devel] [PATCH v2 1/2] intel_iommu: Fix incorrect "end" for vtd_address_space_unmap

[Jason Wang]

On 2019/6/24 下午5:18, Peter Xu wrote:
> From: Yan Zhao <yan.y.zhao@intel.com>
>
> IOMMUNotifier is with inclusive ranges, so we should check
> against (VTD_ADDRESS_SIZE(s->aw_bits) - 1).
>
> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com>
> [peterx: split from another bigger patch]
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 44b1231157..719ce19ab3 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3379,12 +3379,12 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>        * VT-d spec), otherwise we need to consider overflow of 64 bits.
>        */
>   
> -    if (end > VTD_ADDRESS_SIZE(s->aw_bits)) {
> +    if (end > VTD_ADDRESS_SIZE(s->aw_bits) - 1) {
>           /*
>            * Don't need to unmap regions that is bigger than the whole
>            * VT-d supported address space size
>            */
> -        end = VTD_ADDRESS_SIZE(s->aw_bits);
> +        end = VTD_ADDRESS_SIZE(s->aw_bits) - 1;
>       }
>   
>       assert(start <= end);


Acked-by: Jason Wang <jasowang@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Jason Wang]

On 2019/6/24 下午5:18, Peter Xu wrote:
> This is an replacement work of Yan Zhao's patch:
>
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
>
> vtd_address_space_unmap() will do proper page mask alignment to make
> sure each IOTLB message will have correct masks for notification
> messages (2^N-1), but sometimes it can be expanded to even supercede
> the registered range.


I wonder under what circumstance that could we meet this?

Thanks


>   That could lead to unexpected UNMAP of already
> mapped regions in some other notifiers.
>
> Instead of doing mindless expension of the start address and address
> mask, we split the range into smaller ones and guarantee that each
> small range will have correct masks (2^N-1) and at the same time we
> should also try our best to generate as less IOTLB messages as
> possible.
>
> Reported-by: Yan Zhao <yan.y.zhao@intel.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/i386/intel_iommu.c | 67 ++++++++++++++++++++++++++-----------------
>   1 file changed, 41 insertions(+), 26 deletions(-)
>
> diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> index 719ce19ab3..de86f53b4e 100644
> --- a/hw/i386/intel_iommu.c
> +++ b/hw/i386/intel_iommu.c
> @@ -3363,11 +3363,28 @@ VTDAddressSpace *vtd_find_add_as(IntelIOMMUState *s, PCIBus *bus, int devfn)
>       return vtd_dev_as;
>   }
>   
> +static uint64_t get_naturally_aligned_size(uint64_t start,
> +                                           uint64_t size, int gaw)
> +{
> +    uint64_t max_mask = 1ULL << gaw;
> +    uint64_t alignment = start ? start & -start : max_mask;
> +
> +    alignment = MIN(alignment, max_mask);
> +    size = MIN(size, max_mask);
> +
> +    if (alignment <= size) {
> +        /* Increase the alignment of start */
> +        return alignment;
> +    } else {
> +        /* Find the largest page mask from size */
> +        return 1ULL << (63 - clz64(size));
> +    }
> +}
> +
>   /* Unmap the whole range in the notifier's scope. */
>   static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>   {
> -    IOMMUTLBEntry entry;
> -    hwaddr size;
> +    hwaddr size, remain;
>       hwaddr start = n->start;
>       hwaddr end = n->end;
>       IntelIOMMUState *s = as->iommu_state;
> @@ -3388,39 +3405,37 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n)
>       }
>   
>       assert(start <= end);
> -    size = end - start;
> +    size = remain = end - start + 1;
>   
> -    if (ctpop64(size) != 1) {
> -        /*
> -         * This size cannot format a correct mask. Let's enlarge it to
> -         * suite the minimum available mask.
> -         */
> -        int n = 64 - clz64(size);
> -        if (n > s->aw_bits) {
> -            /* should not happen, but in case it happens, limit it */
> -            n = s->aw_bits;
> -        }
> -        size = 1ULL << n;
> +    while (remain >= VTD_PAGE_SIZE) {
> +        IOMMUTLBEntry entry;
> +        uint64_t mask = get_naturally_aligned_size(start, remain, s->aw_bits);
> +
> +        assert(mask);
> +
> +        entry.iova = start;
> +        entry.addr_mask = mask - 1;
> +        entry.target_as = &address_space_memory;
> +        entry.perm = IOMMU_NONE;
> +        /* This field is meaningless for unmap */
> +        entry.translated_addr = 0;
> +
> +        memory_region_notify_one(n, &entry);
> +
> +        start += mask;
> +        remain -= mask;
>       }
>   
> -    entry.target_as = &address_space_memory;
> -    /* Adjust iova for the size */
> -    entry.iova = n->start & ~(size - 1);
> -    /* This field is meaningless for unmap */
> -    entry.translated_addr = 0;
> -    entry.perm = IOMMU_NONE;
> -    entry.addr_mask = size - 1;
> +    assert(!remain);
>   
>       trace_vtd_as_unmap_whole(pci_bus_num(as->bus),
>                                VTD_PCI_SLOT(as->devfn),
>                                VTD_PCI_FUNC(as->devfn),
> -                             entry.iova, size);
> +                             n->start, size);
>   
> -    map.iova = entry.iova;
> -    map.size = entry.addr_mask;
> +    map.iova = n->start;
> +    map.size = size;
>       iova_tree_remove(as->iova_tree, &map);
> -
> -    memory_region_notify_one(n, &entry);
>   }
>   
>   static void vtd_address_space_unmap_all(IntelIOMMUState *s)

Re: [Qemu-devel] [PATCH v2 2/2] intel_iommu: Fix unexpected unmaps during global unmap

[Peter Xu]

On Thu, Jul 04, 2019 at 01:45:41PM +0800, Jason Wang wrote:
> 
> On 2019/6/24 下午5:18, Peter Xu wrote:
> > This is an replacement work of Yan Zhao's patch:
> > 
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg625340.html
> > 
> > vtd_address_space_unmap() will do proper page mask alignment to make
> > sure each IOTLB message will have correct masks for notification
> > messages (2^N-1), but sometimes it can be expanded to even supercede
> > the registered range.
> 
> 
> I wonder under what circumstance that could we meet this?

Sorry I forgot to reply-all just now...

I've asked a similar question, and Yan's answer is here:

https://www.mail-archive.com/qemu-devel@nongnu.org/msg625597.html

Regards,

-- 
Peter Xu