Re: [Xen-devel] [PATCH v6 7/8] AMD/IOMMU: allocate one device table per PCI segment

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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