diff mbox series

[v6,7/8] AMD/IOMMU: allocate one device table per PCI segment

Message ID e0a904bf-b6a0-6224-88f6-e89a95867718@suse.com (mailing list archive)
State Superseded
Headers show
Series AMD IOMMU: further improvements | expand

Commit Message

Jan Beulich Sept. 19, 2019, 1:24 p.m. UTC
Having a single device table for all segments can't possibly be right.
(Even worse, the symbol wasn't static despite being used in just one
source file.) Attach the device tables to their respective IVRS mapping
ones.

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

---
 xen/drivers/passthrough/amd/iommu_init.c |   81 ++++++++++++++++---------------
 1 file changed, 43 insertions(+), 38 deletions(-)

Comments

Paul Durrant Sept. 23, 2019, 4:30 p.m. UTC | #1
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> Sent: 19 September 2019 14:25
> To: xen-devel@lists.xenproject.org
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> Subject: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
> 
> Having a single device table for all segments can't possibly be right.

The copy of the spec. I have says (on page 253: Fixed-Length IVHD Blocks) that IVHD entries must have a segment group of 0, so can't the code just require iommu->seg == 0?

  Paul

> (Even worse, the symbol wasn't static despite being used in just one
> source file.) Attach the device tables to their respective IVRS mapping
> ones.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v6: New.
> 
> ---
>  xen/drivers/passthrough/amd/iommu_init.c |   81 ++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 38 deletions(-)
> 
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -39,7 +39,6 @@ unsigned int __read_mostly ivrs_bdf_entr
>  u8 __read_mostly ivhd_type;
>  static struct radix_tree_root ivrs_maps;
>  LIST_HEAD_READ_MOSTLY(amd_iommu_head);
> -struct table_struct device_table;
>  bool_t iommuv2_enabled;
> 
>  static bool iommu_has_ht_flag(struct amd_iommu *iommu, u8 mask)
> @@ -989,6 +988,12 @@ static void disable_iommu(struct amd_iom
>      spin_unlock_irqrestore(&iommu->lock, flags);
>  }
> 
> +static unsigned int __init dt_alloc_size(void)
> +{
> +    return PAGE_SIZE << get_order_from_bytes(ivrs_bdf_entries *
> +                                             IOMMU_DEV_TABLE_ENTRY_SIZE);
> +}
> +
>  static void __init deallocate_buffer(void *buf, uint32_t sz)
>  {
>      int order = 0;
> @@ -999,12 +1004,6 @@ static void __init deallocate_buffer(voi
>      }
>  }
> 
> -static void __init deallocate_device_table(struct table_struct *table)
> -{
> -    deallocate_buffer(table->buffer, table->alloc_size);
> -    table->buffer = NULL;
> -}
> -
>  static void __init deallocate_ring_buffer(struct ring_buffer *ring_buf)
>  {
>      deallocate_buffer(ring_buf->buffer, ring_buf->alloc_size);
> @@ -1068,8 +1067,29 @@ static void * __init allocate_ppr_log(st
>                                  IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log");
>  }
> 
> +/*
> + * Within ivrs_mappings[] we allocate an extra array element to store
> + * - segment number,
> + * - device table.
> + */
> +#define IVRS_MAPPINGS_SEG(m) (m)[ivrs_bdf_entries].dte_requestor_id
> +#define IVRS_MAPPINGS_DEVTAB(m) (m)[ivrs_bdf_entries].intremap_table
> +
> +static void __init free_ivrs_mapping(void *ptr)
> +{
> +    const struct ivrs_mappings *ivrs_mappings = ptr;
> +
> +    if ( IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
> +        deallocate_buffer(IVRS_MAPPINGS_DEVTAB(ivrs_mappings),
> +                          dt_alloc_size());
> +
> +    xfree(ptr);
> +}
> +
>  static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr)
>  {
> +    const struct ivrs_mappings *ivrs_mappings;
> +
>      if ( allocate_cmd_buffer(iommu) == NULL )
>          goto error_out;
> 
> @@ -1082,13 +1102,15 @@ static int __init amd_iommu_init_one(str
>      if ( intr && !set_iommu_interrupt_handler(iommu) )
>          goto error_out;
> 
> -    /* To make sure that device_table.buffer has been successfully allocated */
> -    if ( device_table.buffer == NULL )
> +    /* Make sure that the device table has been successfully allocated. */
> +    ivrs_mappings = get_ivrs_mappings(iommu->seg);
> +    if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
>          goto error_out;
> 
> -    iommu->dev_table.alloc_size = device_table.alloc_size;
> -    iommu->dev_table.entries = device_table.entries;
> -    iommu->dev_table.buffer = device_table.buffer;
> +    iommu->dev_table.alloc_size = dt_alloc_size();
> +    iommu->dev_table.entries = iommu->dev_table.alloc_size /
> +                               IOMMU_DEV_TABLE_ENTRY_SIZE;
> +    iommu->dev_table.buffer = IVRS_MAPPINGS_DEVTAB(ivrs_mappings);
> 
>      enable_iommu(iommu);
>      printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
> @@ -1135,11 +1157,8 @@ static void __init amd_iommu_init_cleanu
>          xfree(iommu);
>      }
> 
> -    /* free device table */
> -    deallocate_device_table(&device_table);
> -
> -    /* free ivrs_mappings[] */
> -    radix_tree_destroy(&ivrs_maps, xfree);
> +    /* Free ivrs_mappings[] and their device tables. */
> +    radix_tree_destroy(&ivrs_maps, free_ivrs_mapping);
> 
>      iommu_enabled = 0;
>      iommu_hwdom_passthrough = false;
> @@ -1147,12 +1166,6 @@ static void __init amd_iommu_init_cleanu
>      iommuv2_enabled = 0;
>  }
> 
> -/*
> - * We allocate an extra array element to store the segment number
> - * (and in the future perhaps other global information).
> - */
> -#define IVRS_MAPPINGS_SEG(m) m[ivrs_bdf_entries].dte_requestor_id
> -
>  struct ivrs_mappings *get_ivrs_mappings(u16 seg)
>  {
>      return radix_tree_lookup(&ivrs_maps, seg);
> @@ -1235,24 +1248,18 @@ static int __init alloc_ivrs_mappings(u1
>  static int __init amd_iommu_setup_device_table(
>      u16 seg, struct ivrs_mappings *ivrs_mappings)
>  {
> +    struct amd_iommu_dte *dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings);
>      unsigned int bdf;
> 
>      BUG_ON( (ivrs_bdf_entries == 0) );
> 
> -    if ( !device_table.buffer )
> +    if ( !dt )
>      {
>          /* allocate 'device table' on a 4K boundary */
> -        device_table.alloc_size = PAGE_SIZE <<
> -                                  get_order_from_bytes(
> -                                  PAGE_ALIGN(ivrs_bdf_entries *
> -                                  IOMMU_DEV_TABLE_ENTRY_SIZE));
> -        device_table.entries = device_table.alloc_size /
> -                               IOMMU_DEV_TABLE_ENTRY_SIZE;
> -
> -        device_table.buffer = allocate_buffer(device_table.alloc_size,
> -                                              "Device Table");
> +        dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) =
> +            allocate_buffer(dt_alloc_size(), "Device Table");
>      }
> -    if ( !device_table.buffer )
> +    if ( !dt )
>          return -ENOMEM;
> 
>      /* Add device table entries */
> @@ -1260,12 +1267,10 @@ static int __init amd_iommu_setup_device
>      {
>          if ( ivrs_mappings[bdf].valid )
>          {
> -            void *dte;
>              const struct pci_dev *pdev = NULL;
> 
>              /* add device table entry */
> -            dte = device_table.buffer + (bdf * IOMMU_DEV_TABLE_ENTRY_SIZE);
> -            iommu_dte_add_device_entry(dte, &ivrs_mappings[bdf]);
> +            iommu_dte_add_device_entry(&dt[bdf], &ivrs_mappings[bdf]);
> 
>              if ( iommu_intremap &&
>                   ivrs_mappings[bdf].dte_requestor_id == bdf &&
> @@ -1308,7 +1313,7 @@ static int __init amd_iommu_setup_device
>              }
> 
>              amd_iommu_set_intremap_table(
> -                dte, ivrs_mappings[bdf].intremap_table,
> +                &dt[bdf], ivrs_mappings[bdf].intremap_table,
>                  ivrs_mappings[bdf].iommu, iommu_intremap);
>          }
>      }
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Jan Beulich Sept. 24, 2019, 9:10 a.m. UTC | #2
On 23.09.2019 18:30, Paul Durrant wrote:
>> -----Original Message-----
>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>> Sent: 19 September 2019 14:25
>> To: xen-devel@lists.xenproject.org
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
>> Subject: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
>>
>> Having a single device table for all segments can't possibly be right.
> 
> The copy of the spec. I have says (on page 253: Fixed-Length IVHD
> Blocks) that IVHD entries must have a segment group of 0, so can't
> the code just require iommu->seg == 0?

The wording in my version is "At this time, only PCI Segment Group 0 is
supported." This suggests to me that it is not a good idea to have logic
baked in that depends on this remaining true. I realize though that there
are more places than just this one where we (have to) assume segment 0
(all in iommu_acpi.c, and all marked with an XXX comment).

Jan
Paul Durrant Sept. 24, 2019, 9:20 a.m. UTC | #3
> -----Original Message-----
> From: Jan Beulich <jbeulich@suse.com>
> Sent: 24 September 2019 10:10
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>
> Subject: Re: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
> 
> On 23.09.2019 18:30, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
> >> Sent: 19 September 2019 14:25
> >> To: xen-devel@lists.xenproject.org
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit
> <suravee.suthikulpanit@amd.com>
> >> Subject: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
> >>
> >> Having a single device table for all segments can't possibly be right.
> >
> > The copy of the spec. I have says (on page 253: Fixed-Length IVHD
> > Blocks) that IVHD entries must have a segment group of 0, so can't
> > the code just require iommu->seg == 0?
> 
> The wording in my version is "At this time, only PCI Segment Group 0 is
> supported." This suggests to me that it is not a good idea to have logic
> baked in that depends on this remaining true. I realize though that there
> are more places than just this one where we (have to) assume segment 0
> (all in iommu_acpi.c, and all marked with an XXX comment).
> 

Ok. Fair enough. I just wasn't sure it was worth doing this change at the moment; but it doesn't hurt, so you can add my R-b.

  Paul

> Jan
Jan Beulich Sept. 24, 2019, 9:30 a.m. UTC | #4
On 24.09.2019 11:20, Paul Durrant wrote:
>> -----Original Message-----
>> From: Jan Beulich <jbeulich@suse.com>
>> Sent: 24 September 2019 10:10
>> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
>> Cc: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>
>> Subject: Re: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
>>
>> On 23.09.2019 18:30, Paul Durrant wrote:
>>>> -----Original Message-----
>>>> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Jan Beulich
>>>> Sent: 19 September 2019 14:25
>>>> To: xen-devel@lists.xenproject.org
>>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Suravee Suthikulpanit
>> <suravee.suthikulpanit@amd.com>
>>>> Subject: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment
>>>>
>>>> Having a single device table for all segments can't possibly be right.
>>>
>>> The copy of the spec. I have says (on page 253: Fixed-Length IVHD
>>> Blocks) that IVHD entries must have a segment group of 0, so can't
>>> the code just require iommu->seg == 0?
>>
>> The wording in my version is "At this time, only PCI Segment Group 0 is
>> supported." This suggests to me that it is not a good idea to have logic
>> baked in that depends on this remaining true. I realize though that there
>> are more places than just this one where we (have to) assume segment 0
>> (all in iommu_acpi.c, and all marked with an XXX comment).
>>
> 
> Ok. Fair enough. I just wasn't sure it was worth doing this change at the
> moment;

The thing that I found really disgusting was the global (i.e. not even
static) variable "device_table". And simply making it static would still
have left things in too ugly a state for my taste.

> but it doesn't hurt, so you can add my R-b.

Thanks.

Jan
Andrew Cooper Sept. 25, 2019, 2:19 p.m. UTC | #5
On 19/09/2019 14:24, Jan Beulich wrote:
> Having a single device table for all segments can't possibly be right.

That depends on your point of view.  Given that there aren't AMD systems
(or really, x86 systems in general) with segments other than zero, a
single device table is reasonable, even if not overly-forward compatible.

> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1068,8 +1067,29 @@ static void * __init allocate_ppr_log(st
>                                  IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log");
>  }
>  
> +/*
> + * Within ivrs_mappings[] we allocate an extra array element to store
> + * - segment number,
> + * - device table.
> + */
> +#define IVRS_MAPPINGS_SEG(m) (m)[ivrs_bdf_entries].dte_requestor_id
> +#define IVRS_MAPPINGS_DEVTAB(m) (m)[ivrs_bdf_entries].intremap_table
> +
> +static void __init free_ivrs_mapping(void *ptr)

A pointer to this function gets stashed in a non-init radix tree, and
gets invoked by a non-init function (radix_tree_destroy).  It shouldn't
be __init.

> +{
> +    const struct ivrs_mappings *ivrs_mappings = ptr;
> +
> +    if ( IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
> +        deallocate_buffer(IVRS_MAPPINGS_DEVTAB(ivrs_mappings),
> +                          dt_alloc_size());
> +
> +    xfree(ptr)
> +}
> +
>  static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr)
>  {
> +    const struct ivrs_mappings *ivrs_mappings;
> +
>      if ( allocate_cmd_buffer(iommu) == NULL )
>          goto error_out;
>  
> @@ -1082,13 +1102,15 @@ static int __init amd_iommu_init_one(str
>      if ( intr && !set_iommu_interrupt_handler(iommu) )
>          goto error_out;
>  
> -    /* To make sure that device_table.buffer has been successfully allocated */
> -    if ( device_table.buffer == NULL )
> +    /* Make sure that the device table has been successfully allocated. */
> +    ivrs_mappings = get_ivrs_mappings(iommu->seg);
> +    if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )

This isn't safe.  get_ivrs_mappings() returns NULL on failure, which
IVRS_MAPPINGS_DEVTAB() dereferences to find the device table pointer.

~Andrew

>          goto error_out;
>  
> -    iommu->dev_table.alloc_size = device_table.alloc_size;
> -    iommu->dev_table.entries = device_table.entries;
> -    iommu->dev_table.buffer = device_table.buffer;
> +    iommu->dev_table.alloc_size = dt_alloc_size();
> +    iommu->dev_table.entries = iommu->dev_table.alloc_size /
> +                               IOMMU_DEV_TABLE_ENTRY_SIZE;
> +    iommu->dev_table.buffer = IVRS_MAPPINGS_DEVTAB(ivrs_mappings);
>  
>      enable_iommu(iommu);
>      printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
>
Jan Beulich Sept. 25, 2019, 2:40 p.m. UTC | #6
On 25.09.2019 16:19, Andrew Cooper wrote:
> On 19/09/2019 14:24, Jan Beulich wrote:
>> Having a single device table for all segments can't possibly be right.
> 
> That depends on your point of view.  Given that there aren't AMD systems
> (or really, x86 systems in general)

You keep saying this, and I can only keep repeating that a couple
of years ago I did end up testing (and making work) Xen on an SGI
system with (iirc) three segments.

> with segments other than zero, a
> single device table is reasonable, even if not overly-forward compatible.

Well, I see what you mean, but my use of plural in "segments" is
intended to mean potentially multiple of them.

>> --- a/xen/drivers/passthrough/amd/iommu_init.c
>> +++ b/xen/drivers/passthrough/amd/iommu_init.c
>> @@ -1068,8 +1067,29 @@ static void * __init allocate_ppr_log(st
>>                                  IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log");
>>  }
>>  
>> +/*
>> + * Within ivrs_mappings[] we allocate an extra array element to store
>> + * - segment number,
>> + * - device table.
>> + */
>> +#define IVRS_MAPPINGS_SEG(m) (m)[ivrs_bdf_entries].dte_requestor_id
>> +#define IVRS_MAPPINGS_DEVTAB(m) (m)[ivrs_bdf_entries].intremap_table
>> +
>> +static void __init free_ivrs_mapping(void *ptr)
> 
> A pointer to this function gets stashed in a non-init radix tree, and
> gets invoked by a non-init function (radix_tree_destroy).  It shouldn't
> be __init.

I don't see the stashing part, and I don't see an issue at all with
passing the function pointer to radix_tree_destroy(): This function
invocation is itself in an __init function (which gets called upon
initialization errors).

>> @@ -1082,13 +1102,15 @@ static int __init amd_iommu_init_one(str
>>      if ( intr && !set_iommu_interrupt_handler(iommu) )
>>          goto error_out;
>>  
>> -    /* To make sure that device_table.buffer has been successfully allocated */
>> -    if ( device_table.buffer == NULL )
>> +    /* Make sure that the device table has been successfully allocated. */
>> +    ivrs_mappings = get_ivrs_mappings(iommu->seg);
>> +    if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
> 
> This isn't safe.  get_ivrs_mappings() returns NULL on failure, which
> IVRS_MAPPINGS_DEVTAB() dereferences to find the device table pointer.

get_ivrs_mappings() can't return NULL here - if the setting up of
that table has failed (in amd_iommu_prepare_one()), we wouldn't make
it here.

Jan
diff mbox series

Patch

--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -39,7 +39,6 @@  unsigned int __read_mostly ivrs_bdf_entr
 u8 __read_mostly ivhd_type;
 static struct radix_tree_root ivrs_maps;
 LIST_HEAD_READ_MOSTLY(amd_iommu_head);
-struct table_struct device_table;
 bool_t iommuv2_enabled;
 
 static bool iommu_has_ht_flag(struct amd_iommu *iommu, u8 mask)
@@ -989,6 +988,12 @@  static void disable_iommu(struct amd_iom
     spin_unlock_irqrestore(&iommu->lock, flags);
 }
 
+static unsigned int __init dt_alloc_size(void)
+{
+    return PAGE_SIZE << get_order_from_bytes(ivrs_bdf_entries *
+                                             IOMMU_DEV_TABLE_ENTRY_SIZE);
+}
+
 static void __init deallocate_buffer(void *buf, uint32_t sz)
 {
     int order = 0;
@@ -999,12 +1004,6 @@  static void __init deallocate_buffer(voi
     }
 }
 
-static void __init deallocate_device_table(struct table_struct *table)
-{
-    deallocate_buffer(table->buffer, table->alloc_size);
-    table->buffer = NULL;
-}
-
 static void __init deallocate_ring_buffer(struct ring_buffer *ring_buf)
 {
     deallocate_buffer(ring_buf->buffer, ring_buf->alloc_size);
@@ -1068,8 +1067,29 @@  static void * __init allocate_ppr_log(st
                                 IOMMU_PPR_LOG_DEFAULT_ENTRIES, "PPR Log");
 }
 
+/*
+ * Within ivrs_mappings[] we allocate an extra array element to store
+ * - segment number,
+ * - device table.
+ */
+#define IVRS_MAPPINGS_SEG(m) (m)[ivrs_bdf_entries].dte_requestor_id
+#define IVRS_MAPPINGS_DEVTAB(m) (m)[ivrs_bdf_entries].intremap_table
+
+static void __init free_ivrs_mapping(void *ptr)
+{
+    const struct ivrs_mappings *ivrs_mappings = ptr;
+
+    if ( IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
+        deallocate_buffer(IVRS_MAPPINGS_DEVTAB(ivrs_mappings),
+                          dt_alloc_size());
+
+    xfree(ptr);
+}
+
 static int __init amd_iommu_init_one(struct amd_iommu *iommu, bool intr)
 {
+    const struct ivrs_mappings *ivrs_mappings;
+
     if ( allocate_cmd_buffer(iommu) == NULL )
         goto error_out;
 
@@ -1082,13 +1102,15 @@  static int __init amd_iommu_init_one(str
     if ( intr && !set_iommu_interrupt_handler(iommu) )
         goto error_out;
 
-    /* To make sure that device_table.buffer has been successfully allocated */
-    if ( device_table.buffer == NULL )
+    /* Make sure that the device table has been successfully allocated. */
+    ivrs_mappings = get_ivrs_mappings(iommu->seg);
+    if ( !IVRS_MAPPINGS_DEVTAB(ivrs_mappings) )
         goto error_out;
 
-    iommu->dev_table.alloc_size = device_table.alloc_size;
-    iommu->dev_table.entries = device_table.entries;
-    iommu->dev_table.buffer = device_table.buffer;
+    iommu->dev_table.alloc_size = dt_alloc_size();
+    iommu->dev_table.entries = iommu->dev_table.alloc_size /
+                               IOMMU_DEV_TABLE_ENTRY_SIZE;
+    iommu->dev_table.buffer = IVRS_MAPPINGS_DEVTAB(ivrs_mappings);
 
     enable_iommu(iommu);
     printk("AMD-Vi: IOMMU %d Enabled.\n", nr_amd_iommus );
@@ -1135,11 +1157,8 @@  static void __init amd_iommu_init_cleanu
         xfree(iommu);
     }
 
-    /* free device table */
-    deallocate_device_table(&device_table);
-
-    /* free ivrs_mappings[] */
-    radix_tree_destroy(&ivrs_maps, xfree);
+    /* Free ivrs_mappings[] and their device tables. */
+    radix_tree_destroy(&ivrs_maps, free_ivrs_mapping);
 
     iommu_enabled = 0;
     iommu_hwdom_passthrough = false;
@@ -1147,12 +1166,6 @@  static void __init amd_iommu_init_cleanu
     iommuv2_enabled = 0;
 }
 
-/*
- * We allocate an extra array element to store the segment number
- * (and in the future perhaps other global information).
- */
-#define IVRS_MAPPINGS_SEG(m) m[ivrs_bdf_entries].dte_requestor_id
-
 struct ivrs_mappings *get_ivrs_mappings(u16 seg)
 {
     return radix_tree_lookup(&ivrs_maps, seg);
@@ -1235,24 +1248,18 @@  static int __init alloc_ivrs_mappings(u1
 static int __init amd_iommu_setup_device_table(
     u16 seg, struct ivrs_mappings *ivrs_mappings)
 {
+    struct amd_iommu_dte *dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings);
     unsigned int bdf;
 
     BUG_ON( (ivrs_bdf_entries == 0) );
 
-    if ( !device_table.buffer )
+    if ( !dt )
     {
         /* allocate 'device table' on a 4K boundary */
-        device_table.alloc_size = PAGE_SIZE <<
-                                  get_order_from_bytes(
-                                  PAGE_ALIGN(ivrs_bdf_entries *
-                                  IOMMU_DEV_TABLE_ENTRY_SIZE));
-        device_table.entries = device_table.alloc_size /
-                               IOMMU_DEV_TABLE_ENTRY_SIZE;
-
-        device_table.buffer = allocate_buffer(device_table.alloc_size,
-                                              "Device Table");
+        dt = IVRS_MAPPINGS_DEVTAB(ivrs_mappings) =
+            allocate_buffer(dt_alloc_size(), "Device Table");
     }
-    if ( !device_table.buffer )
+    if ( !dt )
         return -ENOMEM;
 
     /* Add device table entries */
@@ -1260,12 +1267,10 @@  static int __init amd_iommu_setup_device
     {
         if ( ivrs_mappings[bdf].valid )
         {
-            void *dte;
             const struct pci_dev *pdev = NULL;
 
             /* add device table entry */
-            dte = device_table.buffer + (bdf * IOMMU_DEV_TABLE_ENTRY_SIZE);
-            iommu_dte_add_device_entry(dte, &ivrs_mappings[bdf]);
+            iommu_dte_add_device_entry(&dt[bdf], &ivrs_mappings[bdf]);
 
             if ( iommu_intremap &&
                  ivrs_mappings[bdf].dte_requestor_id == bdf &&
@@ -1308,7 +1313,7 @@  static int __init amd_iommu_setup_device
             }
 
             amd_iommu_set_intremap_table(
-                dte, ivrs_mappings[bdf].intremap_table,
+                &dt[bdf], ivrs_mappings[bdf].intremap_table,
                 ivrs_mappings[bdf].iommu, iommu_intremap);
         }
     }