* [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
@ 2014-02-07 9:21 Jan Beulich
2014-02-07 10:41 ` Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jan Beulich @ 2014-02-07 9:21 UTC (permalink / raw)
To: xen-devel; +Cc: suravee.suthikulpanit
[-- Attachment #1: Type: text/plain, Size: 1949 bytes --]
... but interrupt remapping is requested (with per-device remapping
tables). Without it, the timer interrupt is usually not working.
Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
Roedel <joerg.roedel@amd.com>.
Reported-by: Eric Houby <ehouby@yahoo.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Eric Houby <ehouby@yahoo.com>
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
const struct acpi_ivrs_header *ivrs_block;
unsigned long length;
unsigned int apic;
+ bool_t sb_ioapic = !iommu_intremap;
int error = 0;
BUG_ON(!table);
@@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
/* Each IO-APIC must have been mentioned in the table. */
for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
{
- if ( !nr_ioapic_entries[apic] ||
- ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
+ if ( !nr_ioapic_entries[apic] )
+ continue;
+
+ if ( !ioapic_sbdf[IO_APIC_ID(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) )
+ sb_ioapic = 1;
+
+ if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
continue;
if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
@@ -1041,6 +1049,14 @@ static int __init parse_ivrs_table(struc
}
}
+ if ( !error && !sb_ioapic )
+ {
+ if ( amd_iommu_perdev_intremap )
+ error = -ENXIO;
+ printk("%sNo southbridge IO-APIC found in IVRS table\n",
+ amd_iommu_perdev_intremap ? XENLOG_ERR : XENLOG_WARNING);
+ }
+
return error;
}
[-- Attachment #2: AMD-IOMMU-require-SB-IOAPIC.patch --]
[-- Type: text/plain, Size: 1997 bytes --]
AMD IOMMU: fail if there is no southbridge IO-APIC
... but interrupt remapping is requested (with per-device remapping
tables). Without it, the timer interrupt is usually not working.
Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
Roedel <joerg.roedel@amd.com>.
Reported-by: Eric Houby <ehouby@yahoo.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Eric Houby <ehouby@yahoo.com>
--- a/xen/drivers/passthrough/amd/iommu_acpi.c
+++ b/xen/drivers/passthrough/amd/iommu_acpi.c
@@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
const struct acpi_ivrs_header *ivrs_block;
unsigned long length;
unsigned int apic;
+ bool_t sb_ioapic = !iommu_intremap;
int error = 0;
BUG_ON(!table);
@@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
/* Each IO-APIC must have been mentioned in the table. */
for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
{
- if ( !nr_ioapic_entries[apic] ||
- ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
+ if ( !nr_ioapic_entries[apic] )
+ continue;
+
+ if ( !ioapic_sbdf[IO_APIC_ID(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) )
+ sb_ioapic = 1;
+
+ if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
continue;
if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
@@ -1041,6 +1049,14 @@ static int __init parse_ivrs_table(struc
}
}
+ if ( !error && !sb_ioapic )
+ {
+ if ( amd_iommu_perdev_intremap )
+ error = -ENXIO;
+ printk("%sNo southbridge IO-APIC found in IVRS table\n",
+ amd_iommu_perdev_intremap ? XENLOG_ERR : XENLOG_WARNING);
+ }
+
return error;
}
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 9:21 [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC Jan Beulich
@ 2014-02-07 10:41 ` Andrew Cooper
2014-02-07 15:12 ` Boris Ostrovsky
2014-02-10 5:33 ` Suravee Suthikulpanit
2 siblings, 0 replies; 10+ messages in thread
From: Andrew Cooper @ 2014-02-07 10:41 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, suravee.suthikulpanit
[-- Attachment #1.1: Type: text/plain, Size: 2234 bytes --]
On 07/02/14 09:21, Jan Beulich wrote:
> ... but interrupt remapping is requested (with per-device remapping
> tables). Without it, the timer interrupt is usually not working.
>
> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
> Roedel <joerg.roedel@amd.com>.
>
> Reported-by: Eric Houby <ehouby@yahoo.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Eric Houby <ehouby@yahoo.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
> const struct acpi_ivrs_header *ivrs_block;
> unsigned long length;
> unsigned int apic;
> + bool_t sb_ioapic = !iommu_intremap;
> int error = 0;
>
> BUG_ON(!table);
> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
> /* Each IO-APIC must have been mentioned in the table. */
> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
> {
> - if ( !nr_ioapic_entries[apic] ||
> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> + if ( !nr_ioapic_entries[apic] )
> + continue;
> +
> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
> + sb_ioapic = 1;
> +
> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> continue;
>
> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
> @@ -1041,6 +1049,14 @@ static int __init parse_ivrs_table(struc
> }
> }
>
> + if ( !error && !sb_ioapic )
> + {
> + if ( amd_iommu_perdev_intremap )
> + error = -ENXIO;
> + printk("%sNo southbridge IO-APIC found in IVRS table\n",
> + amd_iommu_perdev_intremap ? XENLOG_ERR : XENLOG_WARNING);
> + }
> +
> return error;
> }
>
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 3312 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 9:21 [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC Jan Beulich
2014-02-07 10:41 ` Andrew Cooper
@ 2014-02-07 15:12 ` Boris Ostrovsky
2014-02-07 15:23 ` Jan Beulich
2014-02-10 5:33 ` Suravee Suthikulpanit
2 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2014-02-07 15:12 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, suravee.suthikulpanit
[-- Attachment #1.1: Type: text/plain, Size: 2525 bytes --]
On 02/07/2014 04:21 AM, Jan Beulich wrote:
> ... but interrupt remapping is requested (with per-device remapping
> tables). Without it, the timer interrupt is usually not working.
>
> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
> Roedel <joerg.roedel@amd.com>.
>
> Reported-by: Eric Houby <ehouby@yahoo.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Eric Houby <ehouby@yahoo.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
> const struct acpi_ivrs_header *ivrs_block;
> unsigned long length;
> unsigned int apic;
> + bool_t sb_ioapic = !iommu_intremap;
> int error = 0;
>
> BUG_ON(!table);
> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
> /* Each IO-APIC must have been mentioned in the table. */
> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
> {
> - if ( !nr_ioapic_entries[apic] ||
> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> + if ( !nr_ioapic_entries[apic] )
> + continue;
> +
> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
> + sb_ioapic = 1;
> +
> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> continue;
>
> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
I don't know whether 0:14:0 is set in stone, I don't remember seeing
anywhere that this is architectural.
In the (unlikely) event that it is moved somewhere else will the user be
able to overwrite where it is? Do you
think that sb_ioapic may need to be set to true if appropriate bit is
set in ioapic_cmdline?
-boris
> @@ -1041,6 +1049,14 @@ static int __init parse_ivrs_table(struc
> }
> }
>
> + if ( !error && !sb_ioapic )
> + {
> + if ( amd_iommu_perdev_intremap )
> + error = -ENXIO;
> + printk("%sNo southbridge IO-APIC found in IVRS table\n",
> + amd_iommu_perdev_intremap ? XENLOG_ERR : XENLOG_WARNING);
> + }
> +
> return error;
> }
>
>
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
[-- Attachment #1.2: Type: text/html, Size: 3566 bytes --]
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 15:12 ` Boris Ostrovsky
@ 2014-02-07 15:23 ` Jan Beulich
2014-02-07 15:38 ` Boris Ostrovsky
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-02-07 15:23 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, suravee.suthikulpanit
>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>> ... but interrupt remapping is requested (with per-device remapping
>> tables). Without it, the timer interrupt is usually not working.
>>
>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>> Roedel <joerg.roedel@amd.com>.
>>
>> Reported-by: Eric Houby <ehouby@yahoo.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>
>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>> const struct acpi_ivrs_header *ivrs_block;
>> unsigned long length;
>> unsigned int apic;
>> + bool_t sb_ioapic = !iommu_intremap;
>> int error = 0;
>>
>> BUG_ON(!table);
>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>> /* Each IO-APIC must have been mentioned in the table. */
>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>> {
>> - if ( !nr_ioapic_entries[apic] ||
>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>> + if ( !nr_ioapic_entries[apic] )
>> + continue;
>> +
>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>> + sb_ioapic = 1;
>> +
>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>> continue;
>>
>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>
> I don't know whether 0:14:0 is set in stone, I don't remember seeing
> anywhere that this is architectural.
>
> In the (unlikely) event that it is moved somewhere else will the user be
> able to overwrite where it is? Do you
> think that sb_ioapic may need to be set to true if appropriate bit is
> set in ioapic_cmdline?
These are question you'd need to ask to Jörg, the author of the
original Linux side patch. I took as a precondition here that he
knew what he was doing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 15:23 ` Jan Beulich
@ 2014-02-07 15:38 ` Boris Ostrovsky
2014-02-07 15:50 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2014-02-07 15:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, suravee.suthikulpanit
On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>> ... but interrupt remapping is requested (with per-device remapping
>>> tables). Without it, the timer interrupt is usually not working.
>>>
>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>> Roedel <joerg.roedel@amd.com>.
>>>
>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>
>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>> const struct acpi_ivrs_header *ivrs_block;
>>> unsigned long length;
>>> unsigned int apic;
>>> + bool_t sb_ioapic = !iommu_intremap;
>>> int error = 0;
>>>
>>> BUG_ON(!table);
>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>> /* Each IO-APIC must have been mentioned in the table. */
>>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>>> {
>>> - if ( !nr_ioapic_entries[apic] ||
>>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>> + if ( !nr_ioapic_entries[apic] )
>>> + continue;
>>> +
>>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>>> + sb_ioapic = 1;
>>> +
>>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>> continue;
>>>
>>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>> anywhere that this is architectural.
>>
>> In the (unlikely) event that it is moved somewhere else will the user be
>> able to overwrite where it is? Do you
>> think that sb_ioapic may need to be set to true if appropriate bit is
>> set in ioapic_cmdline?
> These are question you'd need to ask to Jörg, the author of the
> original Linux side patch. I took as a precondition here that he
> knew what he was doing.
Xen already has a way to override IVRS' view of IOAPICs with
ioapic_cmdline, something that Linux doesn't. Presumably if the user
sets ivrs_ioapic[] option on boot line then he knows what he is doing
(at least one would hope so).
My concern is that this patch would prevent the user from specifying
where the IOAPIC is. Will this boot option be useful at all now? When we
specify anything but 0:14:0 it will be pretty much ignored, won't it?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 15:38 ` Boris Ostrovsky
@ 2014-02-07 15:50 ` Jan Beulich
2014-02-07 16:03 ` Boris Ostrovsky
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-02-07 15:50 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, suravee.suthikulpanit
>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>>> ... but interrupt remapping is requested (with per-device remapping
>>>> tables). Without it, the timer interrupt is usually not working.
>>>>
>>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>>> Roedel <joerg.roedel@amd.com>.
>>>>
>>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>>
>>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>>> const struct acpi_ivrs_header *ivrs_block;
>>>> unsigned long length;
>>>> unsigned int apic;
>>>> + bool_t sb_ioapic = !iommu_intremap;
>>>> int error = 0;
>>>>
>>>> BUG_ON(!table);
>>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>>> /* Each IO-APIC must have been mentioned in the table. */
>>>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>>>> {
>>>> - if ( !nr_ioapic_entries[apic] ||
>>>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>> + if ( !nr_ioapic_entries[apic] )
>>>> + continue;
>>>> +
>>>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>>>> + sb_ioapic = 1;
>>>> +
>>>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>> continue;
>>>>
>>>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>>> anywhere that this is architectural.
>>>
>>> In the (unlikely) event that it is moved somewhere else will the user be
>>> able to overwrite where it is? Do you
>>> think that sb_ioapic may need to be set to true if appropriate bit is
>>> set in ioapic_cmdline?
>> These are question you'd need to ask to Jörg, the author of the
>> original Linux side patch. I took as a precondition here that he
>> knew what he was doing.
>
> Xen already has a way to override IVRS' view of IOAPICs with
> ioapic_cmdline, something that Linux doesn't. Presumably if the user
> sets ivrs_ioapic[] option on boot line then he knows what he is doing
> (at least one would hope so).
I think the logic we have is sufficiently similar to Linux'es.
> My concern is that this patch would prevent the user from specifying
> where the IOAPIC is. Will this boot option be useful at all now? When we
> specify anything but 0:14:0 it will be pretty much ignored, won't it?
But the purpose here isn't to override how the hardware is
structured, but to overcome firmware vendors not getting their
ACPI tables correct.
Furthermore, what is being specified here can very well be
different from 00:14.0 - consider the northbridge IO-APIC and
eventual further ones.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 15:50 ` Jan Beulich
@ 2014-02-07 16:03 ` Boris Ostrovsky
2014-02-07 16:12 ` Jan Beulich
0 siblings, 1 reply; 10+ messages in thread
From: Boris Ostrovsky @ 2014-02-07 16:03 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel, suravee.suthikulpanit
On 02/07/2014 10:50 AM, Jan Beulich wrote:
>>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>>>> ... but interrupt remapping is requested (with per-device remapping
>>>>> tables). Without it, the timer interrupt is usually not working.
>>>>>
>>>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>>>> Roedel <joerg.roedel@amd.com>.
>>>>>
>>>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>>>
>>>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>>>> const struct acpi_ivrs_header *ivrs_block;
>>>>> unsigned long length;
>>>>> unsigned int apic;
>>>>> + bool_t sb_ioapic = !iommu_intremap;
>>>>> int error = 0;
>>>>>
>>>>> BUG_ON(!table);
>>>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>>>> /* Each IO-APIC must have been mentioned in the table. */
>>>>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>>>>> {
>>>>> - if ( !nr_ioapic_entries[apic] ||
>>>>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>> + if ( !nr_ioapic_entries[apic] )
>>>>> + continue;
>>>>> +
>>>>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>>>>> + sb_ioapic = 1;
>>>>> +
>>>>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>> continue;
>>>>>
>>>>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>>>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>>>> anywhere that this is architectural.
>>>>
>>>> In the (unlikely) event that it is moved somewhere else will the user be
>>>> able to overwrite where it is? Do you
>>>> think that sb_ioapic may need to be set to true if appropriate bit is
>>>> set in ioapic_cmdline?
>>> These are question you'd need to ask to Jörg, the author of the
>>> original Linux side patch. I took as a precondition here that he
>>> knew what he was doing.
>> Xen already has a way to override IVRS' view of IOAPICs with
>> ioapic_cmdline, something that Linux doesn't. Presumably if the user
>> sets ivrs_ioapic[] option on boot line then he knows what he is doing
>> (at least one would hope so).
> I think the logic we have is sufficiently similar to Linux'es.
>
>> My concern is that this patch would prevent the user from specifying
>> where the IOAPIC is. Will this boot option be useful at all now? When we
>> specify anything but 0:14:0 it will be pretty much ignored, won't it?
> But the purpose here isn't to override how the hardware is
> structured, but to overcome firmware vendors not getting their
> ACPI tables correct.
>
> Furthermore, what is being specified here can very well be
> different from 00:14.0 - consider the northbridge IO-APIC and
> eventual further ones.
This is exactly what I am asking: Suppose we have IOAPIC in the NB (I
think it's something like 0:02.0) *and* IVRS is broken. Currently we can
say 'ivrs_ioapic[0]=0:02.0' and we are good to go (right?). Will we
still be able to do this?
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 16:03 ` Boris Ostrovsky
@ 2014-02-07 16:12 ` Jan Beulich
2014-02-07 17:14 ` Suravee Suthikulpanit
0 siblings, 1 reply; 10+ messages in thread
From: Jan Beulich @ 2014-02-07 16:12 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: xen-devel, suravee.suthikulpanit
>>> On 07.02.14 at 17:03, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
> On 02/07/2014 10:50 AM, Jan Beulich wrote:
>>>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>>>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>>>>> ... but interrupt remapping is requested (with per-device remapping
>>>>>> tables). Without it, the timer interrupt is usually not working.
>>>>>>
>>>>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>>>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>>>>> Roedel <joerg.roedel@amd.com>.
>>>>>>
>>>>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>>>>
>>>>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>>>>> const struct acpi_ivrs_header *ivrs_block;
>>>>>> unsigned long length;
>>>>>> unsigned int apic;
>>>>>> + bool_t sb_ioapic = !iommu_intremap;
>>>>>> int error = 0;
>>>>>>
>>>>>> BUG_ON(!table);
>>>>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>>>>> /* Each IO-APIC must have been mentioned in the table. */
>>>>>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>>>>>> {
>>>>>> - if ( !nr_ioapic_entries[apic] ||
>>>>>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>>> + if ( !nr_ioapic_entries[apic] )
>>>>>> + continue;
>>>>>> +
>>>>>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>>>>>> + sb_ioapic = 1;
>>>>>> +
>>>>>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>>> continue;
>>>>>>
>>>>>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>>>>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>>>>> anywhere that this is architectural.
>>>>>
>>>>> In the (unlikely) event that it is moved somewhere else will the user be
>>>>> able to overwrite where it is? Do you
>>>>> think that sb_ioapic may need to be set to true if appropriate bit is
>>>>> set in ioapic_cmdline?
>>>> These are question you'd need to ask to Jörg, the author of the
>>>> original Linux side patch. I took as a precondition here that he
>>>> knew what he was doing.
>>> Xen already has a way to override IVRS' view of IOAPICs with
>>> ioapic_cmdline, something that Linux doesn't. Presumably if the user
>>> sets ivrs_ioapic[] option on boot line then he knows what he is doing
>>> (at least one would hope so).
>> I think the logic we have is sufficiently similar to Linux'es.
>>
>>> My concern is that this patch would prevent the user from specifying
>>> where the IOAPIC is. Will this boot option be useful at all now? When we
>>> specify anything but 0:14:0 it will be pretty much ignored, won't it?
>> But the purpose here isn't to override how the hardware is
>> structured, but to overcome firmware vendors not getting their
>> ACPI tables correct.
>>
>> Furthermore, what is being specified here can very well be
>> different from 00:14.0 - consider the northbridge IO-APIC and
>> eventual further ones.
>
> This is exactly what I am asking: Suppose we have IOAPIC in the NB (I
> think it's something like 0:02.0) *and* IVRS is broken. Currently we can
> say 'ivrs_ioapic[0]=0:02.0' and we are good to go (right?).
No - there _has_ to be an IO-APIC in the SB.
> Will we still be able to do this?
It shouldn't have worked before either, unless I'm not really
understanding the purpose of the check in Linux.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 16:12 ` Jan Beulich
@ 2014-02-07 17:14 ` Suravee Suthikulpanit
0 siblings, 0 replies; 10+ messages in thread
From: Suravee Suthikulpanit @ 2014-02-07 17:14 UTC (permalink / raw)
To: Jan Beulich, Boris Ostrovsky; +Cc: xen-devel
On 2/7/2014 10:12 AM, Jan Beulich wrote:
>>>> On 07.02.14 at 17:03, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>> On 02/07/2014 10:50 AM, Jan Beulich wrote:
>>>>>> On 07.02.14 at 16:38, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>>> On 02/07/2014 10:23 AM, Jan Beulich wrote:
>>>>>>>> On 07.02.14 at 16:12, Boris Ostrovsky <boris.ostrovsky@oracle.com> wrote:
>>>>>> On 02/07/2014 04:21 AM, Jan Beulich wrote:
>>>>>>> ... but interrupt remapping is requested (with per-device remapping
>>>>>>> tables). Without it, the timer interrupt is usually not working.
>>>>>>>
>>>>>>> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
>>>>>>> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
>>>>>>> Roedel <joerg.roedel@amd.com>.
>>>>>>>
>>>>>>> Reported-by: Eric Houby <ehouby@yahoo.com>
>>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>>>> Tested-by: Eric Houby <ehouby@yahoo.com>
>>>>>>>
>>>>>>> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>>>> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
>>>>>>> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
>>>>>>> const struct acpi_ivrs_header *ivrs_block;
>>>>>>> unsigned long length;
>>>>>>> unsigned int apic;
>>>>>>> + bool_t sb_ioapic = !iommu_intremap;
>>>>>>> int error = 0;
>>>>>>>
>>>>>>> BUG_ON(!table);
>>>>>>> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
>>>>>>> /* Each IO-APIC must have been mentioned in the table. */
>>>>>>> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
>>>>>>> {
>>>>>>> - if ( !nr_ioapic_entries[apic] ||
>>>>>>> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>>>> + if ( !nr_ioapic_entries[apic] )
>>>>>>> + continue;
>>>>>>> +
>>>>>>> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
>>>>>>> + sb_ioapic = 1;
>>>>>>> +
>>>>>>> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
>>>>>>> continue;
>>>>>>>
>>>>>>> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
>>>>>> I don't know whether 0:14:0 is set in stone, I don't remember seeing
>>>>>> anywhere that this is architectural.
>>>>>>
[Suravee] From what I have seen on all of our system and checking with
design engineer. IOAPIC in the SB on AMD system always have "0:14.0"
and should always be enabled.
>>>>>> In the (unlikely) event that it is moved somewhere else will the user be
>>>>>> able to overwrite where it is? Do you
>>>>>> think that sb_ioapic may need to be set to true if appropriate bit is
>>>>>> set in ioapic_cmdline?
>>>>> These are question you'd need to ask to Jörg, the author of the
>>>>> original Linux side patch. I took as a precondition here that he
>>>>> knew what he was doing.
>>>> Xen already has a way to override IVRS' view of IOAPICs with
>>>> ioapic_cmdline, something that Linux doesn't. Presumably if the user
>>>> sets ivrs_ioapic[] option on boot line then he knows what he is doing
>>>> (at least one would hope so).
>>> I think the logic we have is sufficiently similar to Linux'es.
>>>
>>>> My concern is that this patch would prevent the user from specifying
>>>> where the IOAPIC is. Will this boot option be useful at all now? When we
>>>> specify anything but 0:14:0 it will be pretty much ignored, won't it?
>>> But the purpose here isn't to override how the hardware is
>>> structured, but to overcome firmware vendors not getting their
>>> ACPI tables correct.
[Suravee] The ivrs_ioapic[] would work for the case where users want to
fix the IVHD when it incorrectly lists the handle ID for IOAPIC, or
missing it. But the IOAPIC needs to also be cross checked with the
IOAPIC entry listed in the APIC table.
In this case, the IOAPIC is also missing in the APIC table, and I don't
think ivrs_ioapic[] would be handling this case anyways.
Suravee
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC
2014-02-07 9:21 [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC Jan Beulich
2014-02-07 10:41 ` Andrew Cooper
2014-02-07 15:12 ` Boris Ostrovsky
@ 2014-02-10 5:33 ` Suravee Suthikulpanit
2 siblings, 0 replies; 10+ messages in thread
From: Suravee Suthikulpanit @ 2014-02-10 5:33 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Acked-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
On 2/7/2014 3:21 AM, Jan Beulich wrote:
> ... but interrupt remapping is requested (with per-device remapping
> tables). Without it, the timer interrupt is usually not working.
>
> Inspired by Linux'es "iommu/amd: Work around wrong IOAPIC device-id in
> IVRS table" (commit c2ff5cf5294bcbd7fa50f7d860e90a66db7e5059) by Joerg
> Roedel <joerg.roedel@amd.com>.
>
> Reported-by: Eric Houby <ehouby@yahoo.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> Tested-by: Eric Houby <ehouby@yahoo.com>
>
> --- a/xen/drivers/passthrough/amd/iommu_acpi.c
> +++ b/xen/drivers/passthrough/amd/iommu_acpi.c
> @@ -984,6 +984,7 @@ static int __init parse_ivrs_table(struc
> const struct acpi_ivrs_header *ivrs_block;
> unsigned long length;
> unsigned int apic;
> + bool_t sb_ioapic = !iommu_intremap;
> int error = 0;
>
> BUG_ON(!table);
> @@ -1017,8 +1018,15 @@ static int __init parse_ivrs_table(struc
> /* Each IO-APIC must have been mentioned in the table. */
> for ( apic = 0; !error && iommu_intremap && apic < nr_ioapics; ++apic )
> {
> - if ( !nr_ioapic_entries[apic] ||
> - ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> + if ( !nr_ioapic_entries[apic] )
> + continue;
> +
> + if ( !ioapic_sbdf[IO_APIC_ID(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) )
> + sb_ioapic = 1;
> +
> + if ( ioapic_sbdf[IO_APIC_ID(apic)].pin_2_idx )
> continue;
>
> if ( !test_bit(IO_APIC_ID(apic), ioapic_cmdline) )
> @@ -1041,6 +1049,14 @@ static int __init parse_ivrs_table(struc
> }
> }
>
> + if ( !error && !sb_ioapic )
> + {
> + if ( amd_iommu_perdev_intremap )
> + error = -ENXIO;
> + printk("%sNo southbridge IO-APIC found in IVRS table\n",
> + amd_iommu_perdev_intremap ? XENLOG_ERR : XENLOG_WARNING);
> + }
> +
> return error;
> }
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-02-10 5:33 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-07 9:21 [PATCH] AMD IOMMU: fail if there is no southbridge IO-APIC Jan Beulich
2014-02-07 10:41 ` Andrew Cooper
2014-02-07 15:12 ` Boris Ostrovsky
2014-02-07 15:23 ` Jan Beulich
2014-02-07 15:38 ` Boris Ostrovsky
2014-02-07 15:50 ` Jan Beulich
2014-02-07 16:03 ` Boris Ostrovsky
2014-02-07 16:12 ` Jan Beulich
2014-02-07 17:14 ` Suravee Suthikulpanit
2014-02-10 5:33 ` Suravee Suthikulpanit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.