Re: [Xen-devel] [PATCH 2/4] AMD IOMMU: cover all functions of a device even if ACPI only tells us of func 0

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 15.02.13 at 18:21, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote:
> On 02/15/2013 11:20 AM, Jan Beulich wrote:
>>   
>>   static int __init get_last_bdf_acpi(struct acpi_table_header *table)
>>   {
>>       const struct acpi_ivrs_header *ivrs_block;
>>       unsigned long length = sizeof(struct acpi_table_ivrs);
>> +    int last_bdf = 0;
>>   
>>       while ( table->length > (length + sizeof(*ivrs_block)) )
>>       {
>>           ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
>>           if ( table->length < (length + ivrs_block->length) )
>>               return -ENODEV;
>> -        if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE &&
>> -             get_last_bdf_ivhd(
>> +        if ( ivrs_block->type == ACPI_IVRS_TYPE_HARDWARE )
>> +        {
>> +            int ret = get_last_bdf_ivhd(
>>                    container_of(ivrs_block, const struct acpi_ivrs_hardware,
>> -                              header)) != 0 )
>> -            return -ENODEV;
>> +                              header));
>> +
>> +            if ( ret < 0 )
>> +                return ret;
>> +            UPDATE_LAST_BDF(ret);
> 
> Why do we need UPDATE_LAST_BDF () here? It is updated in 
> get_last_bdf_ivhd () above.

No, because "last_bdf" now is a local variable.

>> +        }
>>           length += ivrs_block->length;
>>       }
>> -   return 0;
>> +
>> +    return last_bdf;
>>   }
> 
> ...
> 
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -28,12 +28,38 @@
>>   #include <asm/hvm/svm/amd-iommu-proto.h>
>>   #include "../ats.h"
>>   
>> +static bool_t __read_mostly init_done;
>> +
>>   struct amd_iommu *find_iommu_for_device(int seg, int bdf)
>>   {
>>       struct ivrs_mappings *ivrs_mappings = get_ivrs_mappings(seg);
>>   
>> -    return ivrs_mappings && bdf < ivrs_bdf_entries ? 
>> ivrs_mappings[bdf].iommu
>> -                                                   : NULL;
>> +    if ( !ivrs_mappings || bdf >= ivrs_bdf_entries )
>> +        return NULL;
>> +
>> +    if ( unlikely(!ivrs_mappings[bdf].iommu) && likely(init_done) )
>> +    {
>> +        unsigned int bd0 = bdf & ~PCI_FUNC(~0);
>> +
>> +        if ( ivrs_mappings[bd0].iommu )
>> +        {
>> +            struct ivrs_mappings tmp = ivrs_mappings[bd0];
>> +
>> +            tmp.iommu = NULL;
>> +            if ( tmp.dte_requestor_id == bd0 )
>> +                tmp.dte_requestor_id = bdf;
> 
> Is it possible to have tmp.dte_requestor_id != bd0

Sure - when there was an alias entry for it (I assume e.g. in the
case of the device sitting behind a legacy PCI bridge).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel