diff mbox series

[4/9] VT-d: adjust domid map updating when unmapping context

Message ID be453d69-dfa8-1f75-b30f-918229c73d02@suse.com (mailing list archive)
State New
Headers show
Series IOMMU: XSA-373 follow-on | expand

Commit Message

Jan Beulich June 9, 2021, 9:28 a.m. UTC
When an earlier error occurred, cleaning up the domid mapping data is
wrong, as references likely still exist. The only exception to this is
when the actual unmapping worked, but some flush failed (supposedly
impossible after XSA-373). The guest will get crashed in such a case
though, so add fallback cleanup to domain destruction to cover this
case. This in turn makes it desirable to silence the dprintk() in
domain_iommu_domid().

Note that no error will be returned anymore when the lookup fails - in
the common case lookup failure would already have caused
domain_context_unmap_one() to fail, yet even from a more general
perspective it doesn't look right to fail domain_context_unmap() in such
a case when this was the last device, but not when any earlier unmap was
otherwise successful.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Tian, Kevin June 24, 2021, 5:21 a.m. UTC | #1
> From: Jan Beulich <jbeulich@suse.com>
> Sent: Wednesday, June 9, 2021 5:28 PM
> 
> When an earlier error occurred, cleaning up the domid mapping data is
> wrong, as references likely still exist. The only exception to this is
> when the actual unmapping worked, but some flush failed (supposedly
> impossible after XSA-373). The guest will get crashed in such a case
> though, so add fallback cleanup to domain destruction to cover this
> case. This in turn makes it desirable to silence the dprintk() in
> domain_iommu_domid().
> 
> Note that no error will be returned anymore when the lookup fails - in
> the common case lookup failure would already have caused
> domain_context_unmap_one() to fail, yet even from a more general
> perspective it doesn't look right to fail domain_context_unmap() in such
> a case when this was the last device, but not when any earlier unmap was
> otherwise successful.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -80,9 +80,11 @@ static int domain_iommu_domid(struct dom
>          i = find_next_bit(iommu->domid_bitmap, nr_dom, i+1);
>      }
> 
> -    dprintk(XENLOG_ERR VTDPREFIX,
> -            "Cannot get valid iommu domid: domid=%d iommu->index=%d\n",
> -            d->domain_id, iommu->index);
> +    if ( !d->is_dying )
> +        dprintk(XENLOG_ERR VTDPREFIX,
> +                "Cannot get valid iommu %u domid: %pd\n",
> +                iommu->index, d);
> +
>      return -1;
>  }
> 
> @@ -147,6 +149,17 @@ static int context_get_domain_id(struct
>      return domid;
>  }
> 
> +static void cleanup_domid_map(struct domain *domain, struct vtd_iommu
> *iommu)
> +{
> +    int iommu_domid = domain_iommu_domid(domain, iommu);
> +
> +    if ( iommu_domid >= 0 )
> +    {
> +        clear_bit(iommu_domid, iommu->domid_bitmap);
> +        iommu->domid_map[iommu_domid] = 0;
> +    }
> +}
> +
>  static void sync_cache(const void *addr, unsigned int size)
>  {
>      static unsigned long clflush_size = 0;
> @@ -1724,6 +1737,9 @@ static int domain_context_unmap(struct d
>          goto out;
>      }
> 
> +    if ( ret )
> +        goto out;
> +
>      /*
>       * if no other devices under the same iommu owned by this domain,
>       * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
> @@ -1743,19 +1759,8 @@ static int domain_context_unmap(struct d
> 
>      if ( found == 0 )
>      {
> -        int iommu_domid;
> -
>          clear_bit(iommu->index, &dom_iommu(domain)-
> >arch.vtd.iommu_bitmap);
> -
> -        iommu_domid = domain_iommu_domid(domain, iommu);
> -        if ( iommu_domid == -1 )
> -        {
> -            ret = -EINVAL;
> -            goto out;
> -        }
> -
> -        clear_bit(iommu_domid, iommu->domid_bitmap);
> -        iommu->domid_map[iommu_domid] = 0;
> +        cleanup_domid_map(domain, iommu);
>      }
> 
>  out:
> @@ -1775,6 +1780,7 @@ static void iommu_domain_teardown(struct
>  {
>      struct domain_iommu *hd = dom_iommu(d);
>      struct mapped_rmrr *mrmrr, *tmp;
> +    const struct acpi_drhd_unit *drhd;
> 
>      if ( list_empty(&acpi_drhd_units) )
>          return;
> @@ -1786,6 +1792,9 @@ static void iommu_domain_teardown(struct
>      }
> 
>      ASSERT(!hd->arch.vtd.pgd_maddr);
> +
> +    for_each_drhd_unit ( drhd )
> +        cleanup_domid_map(d, drhd->iommu);
>  }
> 
>  static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn,
diff mbox series

Patch

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -80,9 +80,11 @@  static int domain_iommu_domid(struct dom
         i = find_next_bit(iommu->domid_bitmap, nr_dom, i+1);
     }
 
-    dprintk(XENLOG_ERR VTDPREFIX,
-            "Cannot get valid iommu domid: domid=%d iommu->index=%d\n",
-            d->domain_id, iommu->index);
+    if ( !d->is_dying )
+        dprintk(XENLOG_ERR VTDPREFIX,
+                "Cannot get valid iommu %u domid: %pd\n",
+                iommu->index, d);
+
     return -1;
 }
 
@@ -147,6 +149,17 @@  static int context_get_domain_id(struct
     return domid;
 }
 
+static void cleanup_domid_map(struct domain *domain, struct vtd_iommu *iommu)
+{
+    int iommu_domid = domain_iommu_domid(domain, iommu);
+
+    if ( iommu_domid >= 0 )
+    {
+        clear_bit(iommu_domid, iommu->domid_bitmap);
+        iommu->domid_map[iommu_domid] = 0;
+    }
+}
+
 static void sync_cache(const void *addr, unsigned int size)
 {
     static unsigned long clflush_size = 0;
@@ -1724,6 +1737,9 @@  static int domain_context_unmap(struct d
         goto out;
     }
 
+    if ( ret )
+        goto out;
+
     /*
      * if no other devices under the same iommu owned by this domain,
      * clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
@@ -1743,19 +1759,8 @@  static int domain_context_unmap(struct d
 
     if ( found == 0 )
     {
-        int iommu_domid;
-
         clear_bit(iommu->index, &dom_iommu(domain)->arch.vtd.iommu_bitmap);
-
-        iommu_domid = domain_iommu_domid(domain, iommu);
-        if ( iommu_domid == -1 )
-        {
-            ret = -EINVAL;
-            goto out;
-        }
-
-        clear_bit(iommu_domid, iommu->domid_bitmap);
-        iommu->domid_map[iommu_domid] = 0;
+        cleanup_domid_map(domain, iommu);
     }
 
 out:
@@ -1775,6 +1780,7 @@  static void iommu_domain_teardown(struct
 {
     struct domain_iommu *hd = dom_iommu(d);
     struct mapped_rmrr *mrmrr, *tmp;
+    const struct acpi_drhd_unit *drhd;
 
     if ( list_empty(&acpi_drhd_units) )
         return;
@@ -1786,6 +1792,9 @@  static void iommu_domain_teardown(struct
     }
 
     ASSERT(!hd->arch.vtd.pgd_maddr);
+
+    for_each_drhd_unit ( drhd )
+        cleanup_domid_map(d, drhd->iommu);
 }
 
 static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn,