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

2016-12-04 Thread Jan Beulich 
>>> 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

2016-12-04 Thread Suravee Suthikulpanit 

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

2016-12-01 Thread Jan Beulich 
>>> 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

2016-12-01 Thread Suravee Suthikulpanit 

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

2016-12-01 Thread Jan Beulich 
>>> 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, , 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

2016-12-01 Thread Suravee Suthikulpanit 
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, , 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, , , , );
 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
 {
+