| Message ID | 1480590296-2534-1-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@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
Hi Jan, On 12/01/2016 06:58 PM, Jan Beulich wrote: >>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@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 ... 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
>>> On 02.12.16 at 04:48, <Suravee.Suthikulpanit@amd.com> wrote: > On 12/01/2016 06:58 PM, Jan Beulich wrote: >>>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@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 ... > > 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
Hi Jan, On 12/1/16 18:58, Jan Beulich wrote: >>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@amd.com> 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
>>> On 05.12.16 at 05:30, <Suravee.Suthikulpanit@amd.com> wrote: > On 12/1/16 18:58, Jan Beulich wrote: >>>>> On 01.12.16 at 12:04, <suravee.suthikulpanit@amd.com> 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
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 ( idx < 0 ) + return 0; + /* set device id of ioapic */ - ioapic_sbdf[special->handle].bdf = bdf; - ioapic_sbdf[special->handle].seg = seg; + ioapic_sbdf[idx].bdf = bdf; + ioapic_sbdf[idx].seg = seg; + ioapic_sbdf[idx].id = special->handle; - ioapic_sbdf[special->handle].pin_2_idx = xmalloc_array( + ioapic_sbdf[idx].pin_2_idx = xmalloc_array( u16, nr_ioapic_entries[apic]); if ( nr_ioapic_entries[apic] && - !ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx ) + !ioapic_sbdf[idx].pin_2_idx ) { printk(XENLOG_ERR "IVHD Error: Out of memory\n"); return 0; } - memset(ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx, -1, + memset(ioapic_sbdf[idx].pin_2_idx, -1, nr_ioapic_entries[apic] * sizeof(*ioapic_sbdf->pin_2_idx)); } @@ -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 && /* SB IO-APIC is always on this device in AMD systems. */ - ioapic_sbdf[IO_APIC_ID(apic)].bdf == PCI_BDF(0, 0x14, 0) ) + ioapic_sbdf[apic].bdf == PCI_BDF(0, 0x14, 0) ) sb_ioapic = 1; - if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx ) + if ( ioapic_sbdf[apic].pin_2_idx ) continue; - if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) ) + if ( ioapic_id_to_index(IO_APIC_ID(apic)) < 0 ) { printk(XENLOG_ERR "IVHD Error: no information for IO-APIC %#x\n", IO_APIC_ID(apic)); @@ -1044,10 +1052,10 @@ static int __init parse_ivrs_table(struct acpi_table_header *table) return -ENXIO; } - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx = xmalloc_array( + ioapic_sbdf[apic].pin_2_idx = xmalloc_array( u16, nr_ioapic_entries[apic]); - if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx ) - memset(ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx, -1, + if ( ioapic_sbdf[apic].pin_2_idx ) + memset(ioapic_sbdf[apic].pin_2_idx, -1, nr_ioapic_entries[apic] * sizeof(*ioapic_sbdf->pin_2_idx)); else { diff --git a/xen/drivers/passthrough/amd/iommu_intr.c b/xen/drivers/passthrough/amd/iommu_intr.c index 0a9f22f..438c78f 100644 --- a/xen/drivers/passthrough/amd/iommu_intr.c +++ b/xen/drivers/passthrough/amd/iommu_intr.c @@ -32,9 +32,36 @@ struct hpet_sbdf hpet_sbdf; void *shared_intremap_table; unsigned long *shared_intremap_inuse; static DEFINE_SPINLOCK(shared_intremap_lock); +unsigned int nr_ioapic_sbdf; static void dump_intremap_tables(unsigned char key); +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++ ) + if ( ioapic_sbdf[idx].id == apic_id ) + break; + + if ( idx == nr_ioapic_sbdf ) + return -EINVAL; + + return idx; +} + +int get_next_ioapic_bdf_index(void) +{ + if ( nr_ioapic_sbdf < MAX_IO_APICS ) + return nr_ioapic_sbdf++; + + printk(XENLOG_ERR "IVHD Error: Too many IO APICs.\n"); + return -EINVAL; +} + static spinlock_t* get_intremap_lock(int seg, int req_id) { return (amd_iommu_perdev_intremap ? @@ -218,13 +245,19 @@ int __init amd_iommu_setup_ioapic_remapping(void) { for ( pin = 0; pin < nr_ioapic_entries[apic]; pin++ ) { + int idx; + rte = __ioapic_read_entry(apic, pin, 1); if ( rte.mask == 1 ) continue; /* get device id of ioapic devices */ - bdf = ioapic_sbdf[IO_APIC_ID(apic)].bdf; - seg = ioapic_sbdf[IO_APIC_ID(apic)].seg; + idx = ioapic_id_to_index(IO_APIC_ID(apic)); + if ( idx < 0 ) + return -EINVAL; + + bdf = ioapic_sbdf[idx].bdf; + seg = ioapic_sbdf[idx].seg; iommu = find_iommu_for_device(seg, bdf); if ( !iommu ) { @@ -250,7 +283,7 @@ int __init amd_iommu_setup_ioapic_remapping(void) spin_unlock_irqrestore(lock, flags); set_rte_index(&rte, offset); - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx[pin] = offset; + ioapic_sbdf[idx].pin_2_idx[pin] = offset; __ioapic_write_entry(apic, pin, 1, rte); if ( iommu->enabled ) @@ -277,6 +310,7 @@ void amd_iommu_ioapic_update_ire( unsigned int pin = (reg - 0x10) / 2; int saved_mask, seg, bdf, rc; struct amd_iommu *iommu; + int idx; if ( !iommu_intremap ) { @@ -284,9 +318,13 @@ void amd_iommu_ioapic_update_ire( return; } + idx = ioapic_id_to_index(IO_APIC_ID(apic)); + if ( idx < 0 ) + return; + /* get device id of ioapic devices */ - bdf = ioapic_sbdf[IO_APIC_ID(apic)].bdf; - seg = ioapic_sbdf[IO_APIC_ID(apic)].seg; + bdf = ioapic_sbdf[idx].bdf; + seg = ioapic_sbdf[idx].seg; iommu = find_iommu_for_device(seg, bdf); if ( !iommu ) { @@ -313,7 +351,7 @@ void amd_iommu_ioapic_update_ire( } if ( new_rte.mask && - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx[pin] >= INTREMAP_ENTRIES ) + ioapic_sbdf[idx].pin_2_idx[pin] >= INTREMAP_ENTRIES ) { ASSERT(saved_mask); __io_apic_write(apic, reg, value); @@ -330,7 +368,7 @@ void amd_iommu_ioapic_update_ire( /* Update interrupt remapping entry */ rc = update_intremap_entry_from_ioapic( bdf, iommu, &new_rte, reg == rte_lo, - &ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx[pin]); + &ioapic_sbdf[idx].pin_2_idx[pin]); __io_apic_write(apic, reg, ((u32 *)&new_rte)[reg != rte_lo]); @@ -357,14 +395,21 @@ void amd_iommu_ioapic_update_ire( unsigned int amd_iommu_read_ioapic_from_ire( unsigned int apic, unsigned int reg) { + int idx; + unsigned int offset; unsigned int val = __io_apic_read(apic, reg); unsigned int pin = (reg - 0x10) / 2; - unsigned int offset = ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx[pin]; + + idx = ioapic_id_to_index(IO_APIC_ID(apic)); + if ( idx < 0 ) + return -EINVAL; + + offset = ioapic_sbdf[idx].pin_2_idx[pin]; if ( !(reg & 1) && offset < INTREMAP_ENTRIES ) { - u16 bdf = ioapic_sbdf[IO_APIC_ID(apic)].bdf; - u16 seg = ioapic_sbdf[IO_APIC_ID(apic)].seg; + u16 bdf = ioapic_sbdf[idx].bdf; + u16 seg = ioapic_sbdf[idx].seg; u16 req_id = get_intremap_requestor_id(seg, bdf); const u32 *entry = get_intremap_entry(seg, req_id, offset); diff --git a/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h b/xen/include/asm-x86/hvm/svm/amd-iommu-proto.h index 6c702e8..d3db8f1 100644 --- 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; } ioapic_sbdf[MAX_IO_APICS]; +extern unsigned int nr_ioapic_sbdf; +int ioapic_id_to_index(unsigned int apic_id); +int get_next_ioapic_bdf_index(void); + extern struct hpet_sbdf { u16 bdf, seg, id; enum {
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 <suravee.suthikulpanit@amd.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com> --- 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(-)