diff mbox series

hw/virtio/virtio-iommu: Enforce power-of-two notify for both MAP and UNMAP

Message ID 20220714095418.261387-1-jean-philippe@linaro.org (mailing list archive)
State New, archived
Headers show
Series hw/virtio/virtio-iommu: Enforce power-of-two notify for both MAP and UNMAP | expand

Commit Message

Jean-Philippe Brucker July 14, 2022, 9:54 a.m. UTC
Currently we only enforce power-of-two mappings (required by the QEMU
notifier) for UNMAP requests. A MAP request not aligned on a
power-of-two may be successfully handled by VFIO, and then the
corresponding UNMAP notify will fail because it will attempt to split
that mapping. Ensure MAP and UNMAP notifications are consistent.

Fixes: dde3f08b5cab ("virtio-iommu: Handle non power of 2 range invalidations")
Reported-by: Tina Zhang <tina.zhang@intel.com>
Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/virtio/virtio-iommu.c | 44 +++++++++++++++++++++++-----------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

tina.zhang July 15, 2022, 12:09 a.m. UTC | #1
> -----Original Message-----
> From: Jean-Philippe Brucker <jean-philippe@linaro.org>
> Sent: Thursday, July 14, 2022 5:54 PM
> To: eric.auger@redhat.com
> Cc: mst@redhat.com; qemu-devel@nongnu.org; Jean-Philippe Brucker
> <jean-philippe@linaro.org>; Zhang, Tina <tina.zhang@intel.com>
> Subject: [PATCH] hw/virtio/virtio-iommu: Enforce power-of-two notify for
> both MAP and UNMAP
> 
> Currently we only enforce power-of-two mappings (required by the QEMU
> notifier) for UNMAP requests. A MAP request not aligned on a power-of-two
> may be successfully handled by VFIO, and then the corresponding UNMAP
> notify will fail because it will attempt to split that mapping. Ensure MAP and
> UNMAP notifications are consistent.
> 
> Fixes: dde3f08b5cab ("virtio-iommu: Handle non power of 2 range
> invalidations")
> Reported-by: Tina Zhang <tina.zhang@intel.com>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
> ---
>  hw/virtio/virtio-iommu.c | 44 +++++++++++++++++++++++-----------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c index
> 281152d338..f3ecbc71af 100644
> --- a/hw/virtio/virtio-iommu.c
> +++ b/hw/virtio/virtio-iommu.c
> @@ -197,6 +197,29 @@ static gint interval_cmp(gconstpointer a,
> gconstpointer b, gpointer user_data)
>      }
>  }
> 
> +static void virtio_iommu_notify_map_unmap(IOMMUMemoryRegion *mr,
> +                                          IOMMUTLBEvent *event,
> +                                          hwaddr virt_start, hwaddr
> +virt_end) {
> +    uint64_t delta = virt_end - virt_start;
> +
> +    event->entry.iova = virt_start;
> +    event->entry.addr_mask = delta;
> +
> +    if (delta == UINT64_MAX) {
> +        memory_region_notify_iommu(mr, 0, *event);
> +    }
> +
> +    while (virt_start != virt_end + 1) {
> +        uint64_t mask = dma_aligned_pow2_mask(virt_start, virt_end,
> + 64);
> +
> +        event->entry.addr_mask = mask;
> +        event->entry.iova = virt_start;
> +        memory_region_notify_iommu(mr, 0, *event);
> +        virt_start += mask + 1;

Hi Jean, 

We also need to increase the event->translated_addr for the map request here.

Thanks,
Tina

> +    }
> +}
> +
>  static void virtio_iommu_notify_map(IOMMUMemoryRegion *mr, hwaddr
> virt_start,
>                                      hwaddr virt_end, hwaddr paddr,
>                                      uint32_t flags) @@ -215,19 +238,16 @@ static void
> virtio_iommu_notify_map(IOMMUMemoryRegion *mr, hwaddr virt_start,
> 
>      event.type = IOMMU_NOTIFIER_MAP;
>      event.entry.target_as = &address_space_memory;
> -    event.entry.addr_mask = virt_end - virt_start;
> -    event.entry.iova = virt_start;
>      event.entry.perm = perm;
>      event.entry.translated_addr = paddr;
> 
> -    memory_region_notify_iommu(mr, 0, event);
> +    virtio_iommu_notify_map_unmap(mr, &event, virt_start, virt_end);
>  }
> 
>  static void virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr,
> hwaddr virt_start,
>                                        hwaddr virt_end)  {
>      IOMMUTLBEvent event;
> -    uint64_t delta = virt_end - virt_start;
> 
>      if (!(mr->iommu_notify_flags & IOMMU_NOTIFIER_UNMAP)) {
>          return;
> @@ -239,22 +259,8 @@ static void
> virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start,
>      event.entry.target_as = &address_space_memory;
>      event.entry.perm = IOMMU_NONE;
>      event.entry.translated_addr = 0;
> -    event.entry.addr_mask = delta;
> -    event.entry.iova = virt_start;
> -
> -    if (delta == UINT64_MAX) {
> -        memory_region_notify_iommu(mr, 0, event);
> -    }
> 
> -
> -    while (virt_start != virt_end + 1) {
> -        uint64_t mask = dma_aligned_pow2_mask(virt_start, virt_end, 64);
> -
> -        event.entry.addr_mask = mask;
> -        event.entry.iova = virt_start;
> -        memory_region_notify_iommu(mr, 0, event);
> -        virt_start += mask + 1;
> -    }
> +    virtio_iommu_notify_map_unmap(mr, &event, virt_start, virt_end);
>  }
> 
>  static gboolean virtio_iommu_notify_unmap_cb(gpointer key, gpointer value,
> --
> 2.36.1
Jean-Philippe Brucker July 18, 2022, 1:45 p.m. UTC | #2
Hi Tina,

On Fri, Jul 15, 2022 at 12:09:23AM +0000, Zhang, Tina wrote:
> > +static void virtio_iommu_notify_map_unmap(IOMMUMemoryRegion *mr,
> > +                                          IOMMUTLBEvent *event,
> > +                                          hwaddr virt_start, hwaddr
> > +virt_end) {
> > +    uint64_t delta = virt_end - virt_start;
> > +
> > +    event->entry.iova = virt_start;
> > +    event->entry.addr_mask = delta;
> > +
> > +    if (delta == UINT64_MAX) {
> > +        memory_region_notify_iommu(mr, 0, *event);
> > +    }
> > +
> > +    while (virt_start != virt_end + 1) {
> > +        uint64_t mask = dma_aligned_pow2_mask(virt_start, virt_end,
> > + 64);
> > +
> > +        event->entry.addr_mask = mask;
> > +        event->entry.iova = virt_start;
> > +        memory_region_notify_iommu(mr, 0, *event);
> > +        virt_start += mask + 1;
> 
> Hi Jean, 
> 
> We also need to increase the event->translated_addr for the map request here.

Ah right, I'll fix this

Thanks,
Jean
diff mbox series

Patch

diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 281152d338..f3ecbc71af 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -197,6 +197,29 @@  static gint interval_cmp(gconstpointer a, gconstpointer b, gpointer user_data)
     }
 }
 
+static void virtio_iommu_notify_map_unmap(IOMMUMemoryRegion *mr,
+                                          IOMMUTLBEvent *event,
+                                          hwaddr virt_start, hwaddr virt_end)
+{
+    uint64_t delta = virt_end - virt_start;
+
+    event->entry.iova = virt_start;
+    event->entry.addr_mask = delta;
+
+    if (delta == UINT64_MAX) {
+        memory_region_notify_iommu(mr, 0, *event);
+    }
+
+    while (virt_start != virt_end + 1) {
+        uint64_t mask = dma_aligned_pow2_mask(virt_start, virt_end, 64);
+
+        event->entry.addr_mask = mask;
+        event->entry.iova = virt_start;
+        memory_region_notify_iommu(mr, 0, *event);
+        virt_start += mask + 1;
+    }
+}
+
 static void virtio_iommu_notify_map(IOMMUMemoryRegion *mr, hwaddr virt_start,
                                     hwaddr virt_end, hwaddr paddr,
                                     uint32_t flags)
@@ -215,19 +238,16 @@  static void virtio_iommu_notify_map(IOMMUMemoryRegion *mr, hwaddr virt_start,
 
     event.type = IOMMU_NOTIFIER_MAP;
     event.entry.target_as = &address_space_memory;
-    event.entry.addr_mask = virt_end - virt_start;
-    event.entry.iova = virt_start;
     event.entry.perm = perm;
     event.entry.translated_addr = paddr;
 
-    memory_region_notify_iommu(mr, 0, event);
+    virtio_iommu_notify_map_unmap(mr, &event, virt_start, virt_end);
 }
 
 static void virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start,
                                       hwaddr virt_end)
 {
     IOMMUTLBEvent event;
-    uint64_t delta = virt_end - virt_start;
 
     if (!(mr->iommu_notify_flags & IOMMU_NOTIFIER_UNMAP)) {
         return;
@@ -239,22 +259,8 @@  static void virtio_iommu_notify_unmap(IOMMUMemoryRegion *mr, hwaddr virt_start,
     event.entry.target_as = &address_space_memory;
     event.entry.perm = IOMMU_NONE;
     event.entry.translated_addr = 0;
-    event.entry.addr_mask = delta;
-    event.entry.iova = virt_start;
-
-    if (delta == UINT64_MAX) {
-        memory_region_notify_iommu(mr, 0, event);
-    }
 
-
-    while (virt_start != virt_end + 1) {
-        uint64_t mask = dma_aligned_pow2_mask(virt_start, virt_end, 64);
-
-        event.entry.addr_mask = mask;
-        event.entry.iova = virt_start;
-        memory_region_notify_iommu(mr, 0, event);
-        virt_start += mask + 1;
-    }
+    virtio_iommu_notify_map_unmap(mr, &event, virt_start, virt_end);
 }
 
 static gboolean virtio_iommu_notify_unmap_cb(gpointer key, gpointer value,