Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@xxxxxxx> wrote:
> Currently, the driver uses the APIC ID to index into the ioapic_sbdf array.
> The current MAX_IO_APICS is 128, which causes the driver initialization
> to fail on the system with IOAPIC ID >= 128.
> 
> Instead, this patch adds APIC ID in the struct ioapic_sbdf,
> which is used to match the entry when searching through the array.

Wouldn't it have been a lot simpler to just bump the array size to
256? I'll comment on the rest of the patch anyway ...

> Also, this patch removes the use of ioapic_cmdline bit-map, which is
> used to track the ivrs_ioapic options via command line.
> Instead, it introduces the cmd flag in the struct ioapic_cmdline,

... in struct ioapic_sbdf, afaics.

Note that one of the reasons not to do it this way from the
beginning was that ioapic_sbdf[] is permanent, whereas
ioapic_cmdline is __initdata.

>  static void __init parse_ivrs_ioapic(char *str)
>  {
>      const char *s = str;
>      unsigned long id;
>      unsigned int seg, bus, dev, func;
> +    int idx;
>  
>      ASSERT(*s == '[');
>      id = simple_strtoul(s + 1, &s, 0);
> -    if ( id >= ARRAY_SIZE(ioapic_sbdf) || *s != ']' || *++s != '=' )
> +
> +    if ( *s != ']' || *++s != '=' )
> +        return;
> +
> +    idx = ioapic_id_to_index(id);
> +    /* If the entry exist, just ignore the option. */
> +    if ( idx >= 0 )
>          return;

This is a change in behavior, which I don't think we want: So far later
command line options were allowed to override earlier ones.

> @@ -714,43 +724,36 @@ static u16 __init parse_ivhd_device_special(
>           * consistency here --- whether entry's IOAPIC ID is valid and
>           * whether there are conflicting/duplicated entries.
>           */
> -        apic = find_first_bit(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf));
> -        while ( apic < ARRAY_SIZE(ioapic_sbdf) )
> +        for ( apic = 0 ; apic < nr_ioapic_sbdf; apic++ )
>          {
>              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) )
> +        if ( apic < nr_ioapic_sbdf )
>          {
>              AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC 
> %#x"
>                              "(IVRS: %#x devID %04x:%02x:%02x.%u)\n",
> -                            apic, special->handle, seg, PCI_BUS(bdf),
> -                            PCI_SLOT(bdf), PCI_FUNC(bdf));
> +                            ioapic_sbdf[apic].id, special->handle, seg,
> +                            PCI_BUS(bdf), PCI_SLOT(bdf), PCI_FUNC(bdf));
>              break;

Note the comment visible as context at the beginning of this hunk:
How can you now tell apart duplicate entries from ones specified
on the command line?

> @@ -1028,15 +1036,15 @@ static int __init parse_ivrs_table(struct 
> acpi_table_header *table)
>          if ( !nr_ioapic_entries[apic] )
>              continue;
>  
> -        if ( !ioapic_sbdf[IO_APIC_ID(apic)].seg &&
> +        if ( !ioapic_sbdf[apic].seg &&

Can apic really be used as array index here? Don't you need to look
up the index via ioapic_id_to_index()? Or otherwise wouldn't it make
sense to use the same array slots for a particular IO-APIC in both
nr_ioapic_entries[] and ioapic_sbdf[], instead of allocating them via
get_next_ioapic_bdf_index()?

> +int ioapic_id_to_index(unsigned int apic_id)
> +{
> +    int idx;
> +
> +    if ( !nr_ioapic_sbdf )
> +        return -EINVAL;
> +
> +    for ( idx = 0 ; idx < nr_ioapic_sbdf; idx++ )

Due to the use as array index I think idx should be unsigned int, no
matter that it's also used as return value of the function. As the
function would only ever return -EINVAL as error indicator, it may
even be worth to consider it having an unsigned return value, with
e.g. MAX_IO_APICS being the error indicator, to avoid using signed
array indexes elsewhere.

> +int get_next_ioapic_bdf_index(void)

__init

> +{
> +    if ( nr_ioapic_sbdf < MAX_IO_APICS )
> +         return nr_ioapic_sbdf++;

Stray hard tab.

> --- a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> +++ b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h
> @@ -104,8 +104,14 @@ int amd_setup_hpet_msi(struct msi_desc *msi_desc);
>  extern struct ioapic_sbdf {
>      u16 bdf, seg;
>      u16 *pin_2_idx;
> +    u8 id;
> +    bool cmd;

I'd prefer if you used cmdline. Please fit both fields into the 4-byte
hole ahead of the pointer.

Jan

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