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