Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
>>> On 05.12.16 at 05:30, wrote: > On 12/1/16 18:58, Jan Beulich wrote: > On 01.12.16 at 12:04, wrote: >> 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()? > > Isn't the ivrs_ioapic option get parsed before the nr_ioapic_entries are > created? Yes, it is - this would need dealing with of course, perhaps by a 2nd (__initdata) array. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
Hi Jan, On 12/1/16 18:58, Jan Beulich wrote: On 01.12.16 at 12:04, wrote: @@ -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()? I'll fix this. Thanks. 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()? Isn't the ivrs_ioapic option get parsed before the nr_ioapic_entries are created? Thanks, Suravee ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
>>> On 02.12.16 at 04:48, wrote: > On 12/01/2016 06:58 PM, Jan Beulich wrote: > On 01.12.16 at 12:04, 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 ... > > Yes, it would, and that's what I originally tried. However, I was > thinking that it would be unnecessarily waste of space since that would > affect serveral structures i.e. > * mp_ioapic_routing[MAX_IO_APICS] > * mp_ioapics[MAX_IO_APICS] > * nr_ioapic_entries[MAX_IO_APICS] > * vector_map[MAX_IO_APICS] I don't understand - these arrays aren't indexed by IO-APIC ID, so why would they need to grow? Note that I specifically did not suggest to bump MAX_IO_APICS, but only to ... > * ioapic_sbdf[MAX_IO_APICS] ... grow this one array (or alternatively index it the same way the others are being indexed). > If you think this is a reasonable change, I can simplify the patch. The > number 256 should be reasonable for x1APIC since it only supports 8-bit > APIC ID. However, this might be an issue later on with 32-bit APIC ID w/ > x2APIC. How would such a wider ID be communicated? I don't see any MADT sub-table structure other than type 1, which has an 8-bit ID field. Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
Hi Jan, On 12/01/2016 06:58 PM, Jan Beulich wrote: On 01.12.16 at 12:04, 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 ... Yes, it would, and that's what I originally tried. However, I was thinking that it would be unnecessarily waste of space since that would affect serveral structures i.e. * mp_ioapic_routing[MAX_IO_APICS] * mp_ioapics[MAX_IO_APICS] * nr_ioapic_entries[MAX_IO_APICS] * vector_map[MAX_IO_APICS] * ioapic_sbdf[MAX_IO_APICS] If you think this is a reasonable change, I can simplify the patch. The number 256 should be reasonable for x1APIC since it only supports 8-bit APIC ID. However, this might be an issue later on with 32-bit APIC ID w/ x2APIC. Thanks, Suravee ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
>>> On 01.12.16 at 12:04, 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
[Xen-devel] [PATCH] AMD IOMMU: Support IOAPIC IDs larger than 128
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. 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, to identify if the entry is created during ivrs_ioapic command-line parsing. Signed-off-by: Suravee Suthikulpanit Cc: Jan Beulich Cc: Boris Ostrovsky --- xen/drivers/passthrough/amd/iommu_acpi.c | 80 +++ xen/drivers/passthrough/amd/iommu_intr.c | 65 ++ xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 6 ++ 3 files changed, 105 insertions(+), 46 deletions(-) diff --git a/xen/drivers/passthrough/amd/iommu_acpi.c b/xen/drivers/passthrough/amd/iommu_acpi.c index b92c8ad..8c13471 100644 --- a/xen/drivers/passthrough/amd/iommu_acpi.c +++ b/xen/drivers/passthrough/amd/iommu_acpi.c @@ -633,26 +633,36 @@ static u16 __init parse_ivhd_device_extended_range( return dev_length; } -static DECLARE_BITMAP(ioapic_cmdline, ARRAY_SIZE(ioapic_sbdf)) __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; s = parse_pci(s + 1, &seg, &bus, &dev, &func); if ( !s || *s ) return; -ioapic_sbdf[id].bdf = PCI_BDF(bus, dev, func); -ioapic_sbdf[id].seg = seg; -__set_bit(id, ioapic_cmdline); +idx = get_next_ioapic_bdf_index(); +if ( idx < 0 ) +return; + +ioapic_sbdf[idx].bdf = PCI_BDF(bus, dev, func); +ioapic_sbdf[idx].seg = seg; +ioapic_sbdf[idx].id = id; +ioapic_sbdf[idx].cmd = true; } custom_param("ivrs_ioapic[", parse_ivrs_ioapic); @@ -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; } for ( apic = 0; apic < nr_ioapics; apic++ ) { +int idx; + if ( IO_APIC_ID(apic) != special->handle ) continue; -if ( special->handle >= ARRAY_SIZE(ioapic_sbdf) ) -{ -printk(XENLOG_ERR "IVHD Error: IO-APIC %#x entry beyond bounds\n", - special->handle); -return 0; -} - -if ( test_bit(special->handle, ioapic_cmdline) ) +idx = ioapic_id_to_index(special->handle); +if ( idx >= 0 && ioapic_sbdf[idx].cmd ) AMD_IOMMU_DEBUG("IVHD: Command line override present for IO-APIC %#x\n", special->handle); -else if ( ioapic_sbdf[special->handle].pin_2_idx ) +else if ( idx >= 0 && ioapic_sbdf[idx].pin_2_idx ) { -if ( ioapic_sbdf[special->handle].bdf == bdf && - ioapic_sbdf[special->handle].seg == seg ) +if ( ioapic_sbdf[idx].bdf == bdf && + ioapic_sbdf[idx].seg == seg ) AMD_IOMMU_DEBUG("IVHD Warning: Duplicate IO-APIC %#x entries\n", special->handle); else @@ -763,19 +766,24 @@ static u16 __init parse_ivhd_device_special( } else { +idx = get_next_ioapic_bdf_index(); +if