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 );
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
