VT-d: domain ID mapping improvements

[PATCH 6/6] VT-d: avoid allocating domid_{bit,}map[] when possible

Posted by Jan Beulich 4 years, 2 months ago

When an IOMMU implements the full 16 bits worth of DID in context
entries, there's no point going through a memory base translation table.
For IOMMUs not using Caching Mode we can simply use the domain IDs
verbatim, while for Caching Mode we need to avoid DID 0.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
For the case where the memory tables are needed, xvzalloc_array() would
of course be an option to use here as well, despite this being boot time
allocations. Yet the introduction of xvmalloc() et al continues to be
stuck ...

--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -62,11 +62,32 @@ static struct tasklet vtd_fault_tasklet;
 static int setup_hwdom_device(u8 devfn, struct pci_dev *);
 static void setup_hwdom_rmrr(struct domain *d);
 
+static bool domid_mapping(const struct vtd_iommu *iommu)
+{
+    return (const void *)iommu->domid_bitmap != (const void *)iommu->domid_map;
+}
+
+static domid_t convert_domid(const struct vtd_iommu *iommu, domid_t domid)
+{
+    /*
+     * While we need to avoid DID 0 for caching-mode IOMMUs, maintain
+     * the property of the transformation being the same in either
+     * direction. By clipping to 16 bits we ensure that the resulting
+     * DID will fit in the respective context entry field.
+     */
+    BUILD_BUG_ON(sizeof(domid_t) > sizeof(uint16_t));
+
+    return !cap_caching_mode(iommu->cap) ? domid : ~domid;
+}
+
 static int domain_iommu_domid(const struct domain *d,
                               const struct vtd_iommu *iommu)
 {
     unsigned int nr_dom, i;
 
+    if ( !domid_mapping(iommu) )
+        return convert_domid(iommu, d->domain_id);
+
     nr_dom = cap_ndoms(iommu->cap);
     i = find_first_bit(iommu->domid_bitmap, nr_dom);
     while ( i < nr_dom )
@@ -91,26 +112,32 @@ static int context_set_domain_id(struct
                                  const struct domain *d,
                                  struct vtd_iommu *iommu)
 {
-    unsigned int nr_dom, i;
+    unsigned int i;
 
     ASSERT(spin_is_locked(&iommu->lock));
 
-    nr_dom = cap_ndoms(iommu->cap);
-    i = find_first_bit(iommu->domid_bitmap, nr_dom);
-    while ( i < nr_dom && iommu->domid_map[i] != d->domain_id )
-        i = find_next_bit(iommu->domid_bitmap, nr_dom, i + 1);
-
-    if ( i >= nr_dom )
+    if ( domid_mapping(iommu) )
     {
-        i = find_first_zero_bit(iommu->domid_bitmap, nr_dom);
+        unsigned int nr_dom = cap_ndoms(iommu->cap);
+
+        i = find_first_bit(iommu->domid_bitmap, nr_dom);
+        while ( i < nr_dom && iommu->domid_map[i] != d->domain_id )
+            i = find_next_bit(iommu->domid_bitmap, nr_dom, i + 1);
+
         if ( i >= nr_dom )
         {
-            dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no free domain ids\n");
-            return -EBUSY;
+            i = find_first_zero_bit(iommu->domid_bitmap, nr_dom);
+            if ( i >= nr_dom )
+            {
+                dprintk(XENLOG_ERR VTDPREFIX, "IOMMU: no free domain id\n");
+                return -EBUSY;
+            }
+            iommu->domid_map[i] = d->domain_id;
+            set_bit(i, iommu->domid_bitmap);
         }
-        iommu->domid_map[i] = d->domain_id;
-        set_bit(i, iommu->domid_bitmap);
     }
+    else
+        i = convert_domid(iommu, d->domain_id);
 
     context->hi |= (i & ((1 << DID_FIELD_WIDTH) - 1)) << DID_HIGH_OFFSET;
     return 0;
@@ -140,7 +167,12 @@ static int context_get_domain_id(const s
 
 static void cleanup_domid_map(struct domain *domain, struct vtd_iommu *iommu)
 {
-    int iommu_domid = domain_iommu_domid(domain, iommu);
+    int iommu_domid;
+
+    if ( !domid_mapping(iommu) )
+        return;
+
+    iommu_domid = domain_iommu_domid(domain, iommu);
 
     if ( iommu_domid >= 0 )
     {
@@ -196,7 +228,13 @@ static void check_cleanup_domid_map(stru
 
 domid_t did_to_domain_id(const struct vtd_iommu *iommu, unsigned int did)
 {
-    if ( did >= cap_ndoms(iommu->cap) || !test_bit(did, iommu->domid_bitmap) )
+    if ( did >= min(cap_ndoms(iommu->cap), DOMID_MASK + 1) )
+        return DOMID_INVALID;
+
+    if ( !domid_mapping(iommu) )
+        return convert_domid(iommu, did);
+
+    if ( !test_bit(did, iommu->domid_bitmap) )
         return DOMID_INVALID;
 
     return iommu->domid_map[did];
@@ -1296,24 +1334,32 @@ int __init iommu_alloc(struct acpi_drhd_
     if ( !ecap_coherent(iommu->ecap) )
         vtd_ops.sync_cache = sync_cache;
 
-    /* allocate domain id bitmap */
     nr_dom = cap_ndoms(iommu->cap);
-    iommu->domid_bitmap = xzalloc_array(unsigned long, BITS_TO_LONGS(nr_dom));
-    if ( !iommu->domid_bitmap )
-        return -ENOMEM;
 
-    iommu->domid_map = xzalloc_array(domid_t, nr_dom);
-    if ( !iommu->domid_map )
-        return -ENOMEM;
+    if ( nr_dom <= DOMID_MASK + cap_caching_mode(iommu->cap) )
+    {
+        /* Allocate domain id (bit) maps. */
+        iommu->domid_bitmap = xzalloc_array(unsigned long,
+                                            BITS_TO_LONGS(nr_dom));
+        iommu->domid_map = xzalloc_array(domid_t, nr_dom);
+        if ( !iommu->domid_bitmap || !iommu->domid_map )
+            return -ENOMEM;
 
-    /*
-     * If Caching mode is set, then invalid translations are tagged with
-     * domain id 0. Hence reserve bit/slot 0.
-     */
-    if ( cap_caching_mode(iommu->cap) )
+        /*
+         * If Caching mode is set, then invalid translations are tagged
+         * with domain id 0. Hence reserve bit/slot 0.
+         */
+        if ( cap_caching_mode(iommu->cap) )
+        {
+            iommu->domid_map[0] = DOMID_INVALID;
+            __set_bit(0, iommu->domid_bitmap);
+        }
+    }
+    else
     {
-        iommu->domid_map[0] = DOMID_INVALID;
-        __set_bit(0, iommu->domid_bitmap);
+        /* Don't leave dangling NULL pointers. */
+        iommu->domid_bitmap = ZERO_BLOCK_PTR;
+        iommu->domid_map = ZERO_BLOCK_PTR;
     }
 
     return 0;
--- a/xen/drivers/passthrough/vtd/iommu.h
+++ b/xen/drivers/passthrough/vtd/iommu.h
@@ -87,7 +87,7 @@
 #define cap_plmr(c)        (((c) >> 5) & 1)
 #define cap_rwbf(c)        (((c) >> 4) & 1)
 #define cap_afl(c)        (((c) >> 3) & 1)
-#define cap_ndoms(c)        (1 << (4 + 2 * ((c) & 0x7)))
+#define cap_ndoms(c)        (1U << (4 + 2 * ((c) & 0x7)))
 
 /*
  * Extended Capability Register

RE: [PATCH 6/6] VT-d: avoid allocating domid_{bit,}map[] when possible

Posted by Tian, Kevin 4 years, 2 months ago

> From: Jan Beulich <jbeulich@suse.com>
> Sent: Friday, November 12, 2021 5:50 PM
> 
> When an IOMMU implements the full 16 bits worth of DID in context
> entries, there's no point going through a memory base translation table.
> For IOMMUs not using Caching Mode we can simply use the domain IDs
> verbatim, while for Caching Mode we need to avoid DID 0.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> For the case where the memory tables are needed, xvzalloc_array() would
> of course be an option to use here as well, despite this being boot time
> allocations. Yet the introduction of xvmalloc() et al continues to be
> stuck ...
> 
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -62,11 +62,32 @@ static struct tasklet vtd_fault_tasklet;
>  static int setup_hwdom_device(u8 devfn, struct pci_dev *);
>  static void setup_hwdom_rmrr(struct domain *d);
> 
> +static bool domid_mapping(const struct vtd_iommu *iommu)
> +{
> +    return (const void *)iommu->domid_bitmap != (const void *)iommu-
> >domid_map;
> +}
> +
> +static domid_t convert_domid(const struct vtd_iommu *iommu, domid_t
> domid)
> +{
> +    /*
> +     * While we need to avoid DID 0 for caching-mode IOMMUs, maintain
> +     * the property of the transformation being the same in either
> +     * direction. By clipping to 16 bits we ensure that the resulting
> +     * DID will fit in the respective context entry field.
> +     */
> +    BUILD_BUG_ON(sizeof(domid_t) > sizeof(uint16_t));
> +
> +    return !cap_caching_mode(iommu->cap) ? domid : ~domid;

If DOMID_MASK grows to 0xFFFF (though unlikely), then it translates 
to '0' when caching mode is true. We need extend BUILD_BUG_ON() to 
check DOMID_MASK in this case, since caching mode implies
total_size minus one for available domain IDs 

Thanks
Kevin

Re: [PATCH 6/6] VT-d: avoid allocating domid_{bit,}map[] when possible

Posted by Jan Beulich 4 years, 2 months ago

On 15.11.2021 07:18, Tian, Kevin wrote:
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: Friday, November 12, 2021 5:50 PM
>>
>> When an IOMMU implements the full 16 bits worth of DID in context
>> entries, there's no point going through a memory base translation table.
>> For IOMMUs not using Caching Mode we can simply use the domain IDs
>> verbatim, while for Caching Mode we need to avoid DID 0.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> For the case where the memory tables are needed, xvzalloc_array() would
>> of course be an option to use here as well, despite this being boot time
>> allocations. Yet the introduction of xvmalloc() et al continues to be
>> stuck ...
>>
>> --- a/xen/drivers/passthrough/vtd/iommu.c
>> +++ b/xen/drivers/passthrough/vtd/iommu.c
>> @@ -62,11 +62,32 @@ static struct tasklet vtd_fault_tasklet;
>>  static int setup_hwdom_device(u8 devfn, struct pci_dev *);
>>  static void setup_hwdom_rmrr(struct domain *d);
>>
>> +static bool domid_mapping(const struct vtd_iommu *iommu)
>> +{
>> +    return (const void *)iommu->domid_bitmap != (const void *)iommu-
>>> domid_map;
>> +}
>> +
>> +static domid_t convert_domid(const struct vtd_iommu *iommu, domid_t
>> domid)
>> +{
>> +    /*
>> +     * While we need to avoid DID 0 for caching-mode IOMMUs, maintain
>> +     * the property of the transformation being the same in either
>> +     * direction. By clipping to 16 bits we ensure that the resulting
>> +     * DID will fit in the respective context entry field.
>> +     */
>> +    BUILD_BUG_ON(sizeof(domid_t) > sizeof(uint16_t));
>> +
>> +    return !cap_caching_mode(iommu->cap) ? domid : ~domid;
> 
> If DOMID_MASK grows to 0xFFFF (though unlikely),

I did consider this too unlikely to warrant taking care of. Now that
you ask for it anyway, ...

> then it translates 
> to '0' when caching mode is true. We need extend BUILD_BUG_ON() to 
> check DOMID_MASK in this case, since caching mode implies
> total_size minus one for available domain IDs 

... I guess I'd rather replace the BUILD_BUG_ON() than extend it or
add a 2nd one.

Jan

[PATCH 1/6] VT-d: properly reserve DID 0 for caching mode IOMMUs
[PATCH 2/6] VT-d: split domid map cleanup check into a function
[PATCH 3/6] VT-d: don't leak domid mapping on error path
[PATCH 4/6] VT-d: tidy domid map handling
[PATCH 5/6] VT-d: introduce helper to convert DID to domid_t
[PATCH 6/6] VT-d: avoid allocating domid_{bit,}map[] when possible