Re: [Xen-devel] [PATCH v3 05/14] AMD/IOMMU: pass IOMMU to iterate_ivrs_entries() callback

["Woods, Brian" <Brian.Woods@xxxxxxx>]

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@xxxxxxxx>

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

> ---
> 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(
> 

-- 
Brian Woods

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel