Re: [Xen-devel] [PATCH 1/2] AMD IOMMU: also spot missing IO-APIC entries in IVRS table

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-02-06 at 13:12 +0000, Jan Beulich wrote:
> Apart from dealing duplicate conflicting entries, we also have to
> handle firmware omitting IO-APIC entries in IVRS altogether. Not doing
> so has resulted in c/s 26517:601139e2b0db to crash such systems during
> boot (whereas with the change here the IOMMU gets disabled just as is
> being done in the other cases, i.e. unless global tables are being
> used).
> 
> Debugging this issue has also pointed out that the debug log output is
> pretty ugly to look at - consolidate the output, and add one extra
> item for the IVHD special entries, so that future issues are easier
> to analyze.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> Tested-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>

Not really my area but the change looks reasonably straight forward and
fixes a real error observed in the field,
Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

> 
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -354,9 +354,8 @@ static int __init parse_ivmd_block(const
>      base = start_addr & PAGE_MASK;
>      limit = (start_addr + mem_length - 1) & PAGE_MASK;
>  
> -    AMD_IOMMU_DEBUG("IVMD Block: Type %#x\n",ivmd_block->header.type);
> -    AMD_IOMMU_DEBUG(" Start_Addr_Phys %#lx\n", start_addr);
> -    AMD_IOMMU_DEBUG(" Mem_Length %#lx\n", mem_length);
> +    AMD_IOMMU_DEBUG("IVMD Block: type %#x phys %#lx len %#lx\n",
> +                    ivmd_block->header.type, start_addr, mem_length);
>  
>      if ( ivmd_block->header.flags & ACPI_IVMD_EXCLUSION_RANGE )
>          iw = ir = IOMMU_CONTROL_ENABLED;
> @@ -551,8 +550,8 @@ static u16 __init parse_ivhd_device_alia
>          return 0;
>      }
>  
> -    AMD_IOMMU_DEBUG(" Dev_Id Range: %#x -> %#x\n", first_bdf, last_bdf);
> -    AMD_IOMMU_DEBUG(" Dev_Id Alias: %#x\n", alias_id);
> +    AMD_IOMMU_DEBUG(" Dev_Id Range: %#x -> %#x alias %#x\n",
> +                    first_bdf, last_bdf, alias_id);
>  
>      for ( bdf = first_bdf; bdf <= last_bdf; bdf++ )
>          add_ivrs_mapping_entry(bdf, alias_id, 
> range->alias.header.data_setting,
> @@ -654,6 +653,9 @@ static u16 __init parse_ivhd_device_spec
>          return 0;
>      }
>  
> +    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle 
> %#x\n",
> +                    seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> +                    special->variety, special->handle);
>      add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
>  
>      switch ( special->variety )
> @@ -758,10 +760,9 @@ static int __init parse_ivhd_block(const
>      {
>          ivhd_device = (const void *)((const u8 *)ivhd_block + block_length);
>  
> -        AMD_IOMMU_DEBUG( "IVHD Device Entry:\n");
> -        AMD_IOMMU_DEBUG( " Type %#x\n", ivhd_device->header.type);
> -        AMD_IOMMU_DEBUG( " Dev_Id %#x\n", ivhd_device->header.id);
> -        AMD_IOMMU_DEBUG( " Flags %#x\n", ivhd_device->header.data_setting);
> +        AMD_IOMMU_DEBUG("IVHD Device Entry: type %#x id %#x flags %#x\n",
> +                        ivhd_device->header.type, ivhd_device->header.id,
> +                        ivhd_device->header.data_setting);
>  
>          switch ( ivhd_device->header.type )
>          {
> @@ -890,6 +891,7 @@ static int __init parse_ivrs_table(struc
>  {
>      const struct acpi_ivrs_header *ivrs_block;
>      unsigned long length;
> +    unsigned int apic;
>      int error = 0;
>  
>      BUG_ON(!table);
> @@ -903,11 +905,9 @@ static int __init parse_ivrs_table(struc
>      {
>          ivrs_block = (struct acpi_ivrs_header *)((u8 *)table + length);
>  
> -        AMD_IOMMU_DEBUG("IVRS Block:\n");
> -        AMD_IOMMU_DEBUG(" Type %#x\n", ivrs_block->type);
> -        AMD_IOMMU_DEBUG(" Flags %#x\n", ivrs_block->flags);
> -        AMD_IOMMU_DEBUG(" Length %#x\n", ivrs_block->length);
> -        AMD_IOMMU_DEBUG(" Dev_Id %#x\n", ivrs_block->device_id);
> +        AMD_IOMMU_DEBUG("IVRS Block: type %#x flags %#x len %#x id %#x\n",
> +                        ivrs_block->type, ivrs_block->flags,
> +                        ivrs_block->length, ivrs_block->device_id);
>  
>          if ( table->length < (length + ivrs_block->length) )
>          {
> @@ -922,6 +922,29 @@ static int __init parse_ivrs_table(struc
>          length += ivrs_block->length;
>      }
>  
> +    /* Each IO-APIC must have been mentioned in the table. */
> +    for ( apic = 0; !error && apic < nr_ioapics; ++apic )
> +    {
> +        if ( !nr_ioapic_entries[apic] ||
> +             ioapic_sbdf[IO_APIC_ID(apic)].pin_setup )
> +            continue;
> +
> +        printk(XENLOG_ERR "IVHD Error: no information for IO-APIC %#x\n",
> +               IO_APIC_ID(apic));
> +        if ( amd_iommu_perdev_intremap )
> +            error = -ENXIO;
> +        else
> +        {
> +            ioapic_sbdf[IO_APIC_ID(apic)].pin_setup = xzalloc_array(
> +                unsigned long, BITS_TO_LONGS(nr_ioapic_entries[apic]));
> +            if ( !ioapic_sbdf[IO_APIC_ID(apic)].pin_setup )
> +            {
> +                printk(XENLOG_ERR "IVHD Error: Out of memory\n");
> +                error = -ENOMEM;
> +            }
> +        }
> +    }
> +
>      return error;
>  }
>  
> 
> 



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