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

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

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.

> +        }
>           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


-boris