Re: [Xen-devel] [PATCH 1/1 V3] x86/AMD-Vi: Add additional check for invalid special->handle

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.13 at 19:00, <suravee.suthikulpanit@xxxxxxx> wrote:
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -664,19 +664,46 @@ static void __init parse_ivrs_hpet(char *str)
>  
>      ASSERT(*s == '[');
>      id = simple_strtoul(s + 1, &s, 0);
> -    if ( id != (typeof(hpet_sbdf.id))id || *s != ']' || *++s != '=' )
> +    if ( (*s != ']') || (*++s != '=') )

No, unless you have a very good reason.

>          return;
>  
>      s = parse_pci(s + 1, &seg, &bus, &dev, &func);
>      if ( !s || *s )
>          return;
>  
> +    hpet_sbdf.id = id;

In essence this is the only change not contained in the patch I sent.
So I'd be inclined to commit this (and perhaps the one debug
message adjustment below) under your name, and my proposed
change as a separate one.

>      hpet_sbdf.bdf = PCI_BDF(bus, dev, func);
>      hpet_sbdf.seg = seg;
>      hpet_sbdf.cmdline = 1;
>  }
>  custom_param("ivrs_hpet[", parse_ivrs_hpet);
>  
> +static bool_t is_ioapic_overidden(u16 seg, u16 bdf, u8 handle)

Missing __init annotation. And anyway, I can't really see why
putting this in a separate function is a significant benefit. It's
only being used in one place afaics.

> +{
> +    bool_t ret = 0;
> +    int apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
> +
> +    while ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +    {
> +        if ( ioapic_sbdf[apic].bdf == bdf &&
> +             ioapic_sbdf[apic].seg == seg )
> +            break;
> +        apic = find_next_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf),
> +                             apic + 1);
> +    }
> +
> +    if ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +    {
> +        AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x 
> "
> +                        "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
> +                        apic, handle, seg, PCI_BUS(bdf),
> +                        PCI_SLOT(bdf), PCI_FUNC(bdf));
> +        ret = 1;
> +    }
> +
> +    return ret;
> +}
> +
>  static u16 __init parse_ivhd_device_special(
>      const struct acpi_ivrs_device8c *special, u16 seg,
>      u16 header_length, u16 block_length, struct amd_iommu *iommu)
> @@ -698,16 +725,18 @@ static u16 __init parse_ivhd_device_special(
>          return 0;
>      }
>  
> -    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle 
> %#x\n",
> +    AMD_IOMMU_DEBUG("IVHD Special: %04x:%02x:%02x.%u variety %#x handle %#x 
> used_id %#x\n",
>                      seg, PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf),
> -                    special->variety, special->handle);
> +                    special->variety, special->handle, special->used_id);
>      add_ivrs_mapping_entry(bdf, bdf, special->header.data_setting, iommu);
>  
>      switch ( special->variety )
>      {
>      case ACPI_IVHD_IOAPIC:
> -        if ( !iommu_intremap )
> +        if ( !iommu_intremap ||
> +             is_ioapic_overidden(seg, bdf, special->handle) )
>              break;
> +
>          /*
>           * Some BIOSes have IOAPIC broken entries so we check for IVRS
>           * consistency here --- whether entry's IOAPIC ID is valid and
> @@ -725,10 +754,7 @@ static u16 __init parse_ivhd_device_special(
>                  return 0;
>              }
>  
> -            if ( test_bit(special->handle, ioapic_cmdline) )
> -                AMD_IOMMU_DEBUG("IVHD: Command line override present for 
> IO-APIC %#x\n",
> -                                special->handle);
> -            else if ( ioapic_sbdf[special->handle].pin_2_idx )
> +            if ( ioapic_sbdf[special->handle].pin_2_idx )

Again - no, unless you have a very good reason.

>              {
>                  if ( ioapic_sbdf[special->handle].bdf == bdf &&
>                       ioapic_sbdf[special->handle].seg == seg )
> @@ -770,6 +796,16 @@ static u16 __init parse_ivhd_device_special(
>          }
>          break;
>      case ACPI_IVHD_HPET:
> +        if ( hpet_sbdf.cmdline )
> +        {
> +            AMD_IOMMU_DEBUG("IVHD: Command line override present for HPET 
> %#x "
> +                            "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
> +                            hpet_sbdf.id, special->handle, seg, PCI_BUS(bdf),
> +                            PCI_SLOT(bdf), PCI_FUNC(bdf));
> +            hpet_sbdf.iommu = iommu;
> +            break;
> +        }
> +
>          /* set device id of hpet */
>          if ( hpet_sbdf.iommu ||
>               (hpet_sbdf.cmdline && hpet_sbdf.id != special->handle) )
> @@ -777,12 +813,10 @@ static u16 __init parse_ivhd_device_special(
>              printk(XENLOG_WARNING "Only one IVHD HPET entry is supported\n");
>              break;
>          }
> +
>          hpet_sbdf.id = special->handle;
> -        if ( !hpet_sbdf.cmdline )
> -        {
> -            hpet_sbdf.bdf = bdf;
> -            hpet_sbdf.seg = seg;
> -        }
> +        hpet_sbdf.bdf = bdf;
> +        hpet_sbdf.seg = seg;

I don't see what benefit these HPET related changes provide. Or
if there is any that I overlook, then the previous uses of
hpet_sbdf.cmdline should all be eliminated.

Jan

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