>>> On 01.12.16 at 12:04, <suravee.suthikulpa...@amd.com> 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@lists.xen.org
https://lists.xen.org/xen-devel
