[v3,05/14] AMD/IOMMU: pass IOMMU to iterate_ivrs_entries() callback
diff mbox series

Message ID e2072315-7c8c-2f82-99f4-795cc93f1fa8@suse.com
State New
Headers show
Series
  • [v3,01/14] AMD/IOMMU: free more memory when cleaning up after error
Related show

Commit Message

Jan Beulich July 16, 2019, 4:37 p.m. UTC
Both users will want to know IOMMU properties (specifically the IRTE
size) subsequently. Leverage this to avoid pointless calls to the
callback when IVRS mapping table entries are unpopulated. To avoid
leaking interrupt remapping tables (bogusly) allocated for IOMMUs
themselves, this requires suppressing their allocation in the first
place, taking a step further what commit 757122c0cf ('AMD/IOMMU: don't
"add" IOMMUs') had done.

Additionally suppress the call for alias entries, as again both users
don't care about these anyway. In fact this eliminates a fair bit of
redundancy from dump output.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v3: New.
---
TBD: Along the lines of avoiding the IRT allocation for the IOMMUs, is
      there a way to recognize the many CPU-provided devices many of
      which can't generate interrupts anyway, and avoid allocations for
      them as well? It's 32k per device, after all. Another option might
      be on-demand allocation of the tables, but quite possibly we'd get
      into trouble with error handling there.

Comments

Andrew Cooper July 19, 2019, 4:32 p.m. UTC | #1
On 16/07/2019 17:37, Jan Beulich wrote:
> Both users will want to know IOMMU properties (specifically the IRTE
> size) subsequently. Leverage this to avoid pointless calls to the
> callback when IVRS mapping table entries are unpopulated. To avoid
> leaking interrupt remapping tables (bogusly) allocated for IOMMUs
> themselves, this requires suppressing their allocation in the first
> place, taking a step further what commit 757122c0cf ('AMD/IOMMU: don't
> "add" IOMMUs') had done.
>
> Additionally suppress the call for alias entries, as again both users
> don't care about these anyway. In fact this eliminates a fair bit of
> redundancy from dump output.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Woods, Brian July 19, 2019, 6:26 p.m. UTC | #2
On Tue, Jul 16, 2019 at 04:37:04PM +0000, Jan Beulich wrote:
> Both users will want to know IOMMU properties (specifically the IRTE
> size) subsequently. Leverage this to avoid pointless calls to the
> callback when IVRS mapping table entries are unpopulated. To avoid
> leaking interrupt remapping tables (bogusly) allocated for IOMMUs
> themselves, this requires suppressing their allocation in the first
> place, taking a step further what commit 757122c0cf ('AMD/IOMMU: don't
> "add" IOMMUs') had done.
> 
> Additionally suppress the call for alias entries, as again both users
> don't care about these anyway. In fact this eliminates a fair bit of
> redundancy from dump output.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Brian Woods <brian.woods@amd.com>

> ---
> v3: New.
> ---
> TBD: Along the lines of avoiding the IRT allocation for the IOMMUs, is
>       there a way to recognize the many CPU-provided devices many of
>       which can't generate interrupts anyway, and avoid allocations for
>       them as well? It's 32k per device, after all. Another option might
>       be on-demand allocation of the tables, but quite possibly we'd get
>       into trouble with error handling there.
> 
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -65,7 +65,11 @@ static void __init add_ivrs_mapping_entr
>       /* override flags for range of devices */
>       ivrs_mappings[bdf].device_flags = flags;
>   
> -    if (ivrs_mappings[alias_id].intremap_table == NULL )
> +    /* Don't map an IOMMU by itself. */
> +    if ( iommu->bdf == bdf )
> +        return;
> +
> +    if ( !ivrs_mappings[alias_id].intremap_table )
>       {
>            /* allocate per-device interrupt remapping table */
>            if ( amd_iommu_perdev_intremap )
> @@ -81,8 +85,9 @@ static void __init add_ivrs_mapping_entr
>                ivrs_mappings[alias_id].intremap_inuse = shared_intremap_inuse;
>            }
>       }
> -    /* Assign IOMMU hardware, but don't map an IOMMU by itself. */
> -    ivrs_mappings[bdf].iommu = iommu->bdf != bdf ? iommu : NULL;
> +
> +    /* Assign IOMMU hardware. */
> +    ivrs_mappings[bdf].iommu = iommu;
>   }
>   
>   static struct amd_iommu * __init find_iommu_from_bdf_cap(
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -1069,7 +1069,8 @@ int iterate_ivrs_mappings(int (*handler)
>       return rc;
>   }
>   
> -int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
> +int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *,
> +                                        struct ivrs_mappings *))
>   {
>       u16 seg = 0;
>       int rc = 0;
> @@ -1082,7 +1083,12 @@ int iterate_ivrs_entries(int (*handler)(
>               break;
>           seg = IVRS_MAPPINGS_SEG(map);
>           for ( bdf = 0; !rc && bdf < ivrs_bdf_entries; ++bdf )
> -            rc = handler(seg, map + bdf);
> +        {
> +            const struct amd_iommu *iommu = map[bdf].iommu;
> +
> +            if ( iommu && map[bdf].dte_requestor_id == bdf )
> +                rc = handler(iommu, &map[bdf]);
> +        }
>       } while ( !rc && ++seg );
>   
>       return rc;
> --- a/xen/drivers/passthrough/amd/iommu_intr.c
> +++ b/xen/drivers/passthrough/amd/iommu_intr.c
> @@ -617,7 +617,7 @@ void amd_iommu_read_msi_from_ire(
>   }
>   
>   int __init amd_iommu_free_intremap_table(
> -    u16 seg, struct ivrs_mappings *ivrs_mapping)
> +    const struct amd_iommu *iommu, struct ivrs_mappings *ivrs_mapping)
>   {
>       void *tb = ivrs_mapping->intremap_table;
>   
> @@ -693,14 +693,15 @@ static void dump_intremap_table(const u3
>       }
>   }
>   
> -static int dump_intremap_mapping(u16 seg, struct ivrs_mappings *ivrs_mapping)
> +static int dump_intremap_mapping(const struct amd_iommu *iommu,
> +                                 struct ivrs_mappings *ivrs_mapping)
>   {
>       unsigned long flags;
>   
>       if ( !ivrs_mapping )
>           return 0;
>   
> -    printk("  %04x:%02x:%02x:%u:\n", seg,
> +    printk("  %04x:%02x:%02x:%u:\n", iommu->seg,
>              PCI_BUS(ivrs_mapping->dte_requestor_id),
>              PCI_SLOT(ivrs_mapping->dte_requestor_id),
>              PCI_FUNC(ivrs_mapping->dte_requestor_id));
> --- a/xen/include/asm-x86/amd-iommu.h
> +++ b/xen/include/asm-x86/amd-iommu.h
> @@ -129,7 +129,8 @@ extern u8 ivhd_type;
>   
>   struct ivrs_mappings *get_ivrs_mappings(u16 seg);
>   int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
> -int iterate_ivrs_entries(int (*)(u16 seg, struct ivrs_mappings *));
> +int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
> +                                 struct ivrs_mappings *));
>   
>   /* iommu tables in guest space */
>   struct mmio_reg {
> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -98,7 +98,8 @@ struct amd_iommu *find_iommu_for_device(
>   /* interrupt remapping */
>   int amd_iommu_setup_ioapic_remapping(void);
>   void *amd_iommu_alloc_intremap_table(unsigned long **);
> -int amd_iommu_free_intremap_table(u16 seg, struct ivrs_mappings *);
> +int amd_iommu_free_intremap_table(
> +    const struct amd_iommu *, struct ivrs_mappings *);
>   void amd_iommu_ioapic_update_ire(
>       unsigned int apic, unsigned int reg, unsigned int value);
>   unsigned int amd_iommu_read_ioapic_from_ire(
>

Patch
diff mbox series

--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -65,7 +65,11 @@  static void __init add_ivrs_mapping_entr
      /* override flags for range of devices */
      ivrs_mappings[bdf].device_flags = flags;
  
-    if (ivrs_mappings[alias_id].intremap_table == NULL )
+    /* Don't map an IOMMU by itself. */
+    if ( iommu->bdf == bdf )
+        return;
+
+    if ( !ivrs_mappings[alias_id].intremap_table )
      {
           /* allocate per-device interrupt remapping table */
           if ( amd_iommu_perdev_intremap )
@@ -81,8 +85,9 @@  static void __init add_ivrs_mapping_entr
               ivrs_mappings[alias_id].intremap_inuse = shared_intremap_inuse;
           }
      }
-    /* Assign IOMMU hardware, but don't map an IOMMU by itself. */
-    ivrs_mappings[bdf].iommu = iommu->bdf != bdf ? iommu : NULL;
+
+    /* Assign IOMMU hardware. */
+    ivrs_mappings[bdf].iommu = iommu;
  }
  
  static struct amd_iommu * __init find_iommu_from_bdf_cap(
--- a/xen/drivers/passthrough/amd/iommu_init.c
+++ b/xen/drivers/passthrough/amd/iommu_init.c
@@ -1069,7 +1069,8 @@  int iterate_ivrs_mappings(int (*handler)
      return rc;
  }
  
-int iterate_ivrs_entries(int (*handler)(u16 seg, struct ivrs_mappings *))
+int iterate_ivrs_entries(int (*handler)(const struct amd_iommu *,
+                                        struct ivrs_mappings *))
  {
      u16 seg = 0;
      int rc = 0;
@@ -1082,7 +1083,12 @@  int iterate_ivrs_entries(int (*handler)(
              break;
          seg = IVRS_MAPPINGS_SEG(map);
          for ( bdf = 0; !rc && bdf < ivrs_bdf_entries; ++bdf )
-            rc = handler(seg, map + bdf);
+        {
+            const struct amd_iommu *iommu = map[bdf].iommu;
+
+            if ( iommu && map[bdf].dte_requestor_id == bdf )
+                rc = handler(iommu, &map[bdf]);
+        }
      } while ( !rc && ++seg );
  
      return rc;
--- a/xen/drivers/passthrough/amd/iommu_intr.c
+++ b/xen/drivers/passthrough/amd/iommu_intr.c
@@ -617,7 +617,7 @@  void amd_iommu_read_msi_from_ire(
  }
  
  int __init amd_iommu_free_intremap_table(
-    u16 seg, struct ivrs_mappings *ivrs_mapping)
+    const struct amd_iommu *iommu, struct ivrs_mappings *ivrs_mapping)
  {
      void *tb = ivrs_mapping->intremap_table;
  
@@ -693,14 +693,15 @@  static void dump_intremap_table(const u3
      }
  }
  
-static int dump_intremap_mapping(u16 seg, struct ivrs_mappings *ivrs_mapping)
+static int dump_intremap_mapping(const struct amd_iommu *iommu,
+                                 struct ivrs_mappings *ivrs_mapping)
  {
      unsigned long flags;
  
      if ( !ivrs_mapping )
          return 0;
  
-    printk("  %04x:%02x:%02x:%u:\n", seg,
+    printk("  %04x:%02x:%02x:%u:\n", iommu->seg,
             PCI_BUS(ivrs_mapping->dte_requestor_id),
             PCI_SLOT(ivrs_mapping->dte_requestor_id),
             PCI_FUNC(ivrs_mapping->dte_requestor_id));
--- a/xen/include/asm-x86/amd-iommu.h
+++ b/xen/include/asm-x86/amd-iommu.h
@@ -129,7 +129,8 @@  extern u8 ivhd_type;
  
  struct ivrs_mappings *get_ivrs_mappings(u16 seg);
  int iterate_ivrs_mappings(int (*)(u16 seg, struct ivrs_mappings *));
-int iterate_ivrs_entries(int (*)(u16 seg, struct ivrs_mappings *));
+int iterate_ivrs_entries(int (*)(const struct amd_iommu *,
+                                 struct ivrs_mappings *));
  
  /* iommu tables in guest space */
  struct mmio_reg {
--- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
+++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
@@ -98,7 +98,8 @@  struct amd_iommu *find_iommu_for_device(
  /* interrupt remapping */
  int amd_iommu_setup_ioapic_remapping(void);
  void *amd_iommu_alloc_intremap_table(unsigned long **);
-int amd_iommu_free_intremap_table(u16 seg, struct ivrs_mappings *);
+int amd_iommu_free_intremap_table(
+    const struct amd_iommu *, struct ivrs_mappings *);
  void amd_iommu_ioapic_update_ire(
      unsigned int apic, unsigned int reg, unsigned int value);
  unsigned int amd_iommu_read_ioapic_from_ire(