* [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
@ 2013-06-04 16:38 Andrew Cooper
2013-06-10 12:25 ` Jan Beulich
2013-06-26 9:54 ` Andrew Cooper
0 siblings, 2 replies; 13+ messages in thread
From: Andrew Cooper @ 2013-06-04 16:38 UTC (permalink / raw)
To: xen-devel; +Cc: Jacob Shin, Keir Fraser, Suravee Suthikulpanit, Jan Beulich
XSA-36 changed the default vector map mode from global to per-device. This is
because a global vector map does not prevent one PCI device from impersonating
another and launching a DoS on the system.
However, the per-device vector map logic is broken for devices with multiple
MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
of a guests interrupt remapping tables. The core problem is not trivial to
fix.
In an effort to get AMD systems back to a non-regressed state, introduce a new
type of vector map called per-device-global. This uses per-device vector maps
in the IOMMU, but uses a single used_vector map for the core IRQ logic.
This patch is intended to be removed as soon as the per-device logic is fixed
correctly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
* Do not override command line.
* reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
Changes since v1:
* Correct stupid mistake in commit message, making it confusing to read
diff -r 2d37d2d652a8 -r a017d74f346d xen/drivers/passthrough/amd/pci_amd_iommu.c
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
{
if ( amd_iommu_perdev_intremap )
{
- printk("AMD-Vi: Enabling per-device vector maps\n");
- opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
+ /* Per-device vector map logic is broken for devices with multiple
+ * MSI-X interrupts (and would also be for multiple MSI, if Xen
+ * supported it).
+ *
+ * Until this is fixed, use global vector tables as far as the irq
+ * logic is concerned to avoid the buggy behaviour of per-device
+ * maps in map_domain_pirq(), and use per-device tables as far as
+ * intremap code is concerned to avoid the security issue.
+ */
+ printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
+ "Using per-device-global maps instead until a fix is found\n");
+
+ opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
}
else
{
@@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
else
{
printk("AMD-Vi: Not overriding irq_vector_map setting\n");
+
+ if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
+ {
+ printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
+ "Use irq_vector_map=global to work around.");
+ }
}
if ( !amd_iommu_perdev_intremap )
printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-04 16:38 [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed Andrew Cooper
@ 2013-06-10 12:25 ` Jan Beulich
2013-06-14 8:45 ` Jan Beulich
2013-06-15 1:13 ` Suravee Suthikulanit
2013-06-26 9:54 ` Andrew Cooper
1 sibling, 2 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-10 12:25 UTC (permalink / raw)
To: Jacob Shin, SuraveeSuthikulpanit
Cc: George Dunlap, Andrew Cooper, Keir Fraser, xen-devel
>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> XSA-36 changed the default vector map mode from global to per-device. This is
> because a global vector map does not prevent one PCI device from
> impersonating
> another and launching a DoS on the system.
>
> However, the per-device vector map logic is broken for devices with multiple
> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
> of a guests interrupt remapping tables. The core problem is not trivial to
> fix.
>
> In an effort to get AMD systems back to a non-regressed state, introduce a
> new
> type of vector map called per-device-global. This uses per-device vector maps
> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>
> This patch is intended to be removed as soon as the per-device logic is fixed
> correctly.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Suravee, Jacob,
no opinion on this at all? I've been talked into considering this
acceptable (with a small coding style fixup, and with the question on
the usefulness of the final warning message - imo redundant with the
immediately preceding message that is being left untouched), with
the strict expectation that this would get reverted right away after
4.3, with the multi-vector MSI series being the real fix to this
(presumably allowing to drop the vector map stuff altogether).
Jan
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
> {
> if ( amd_iommu_perdev_intremap )
> {
> - printk("AMD-Vi: Enabling per-device vector maps\n");
> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
> + /* Per-device vector map logic is broken for devices with multiple
> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
> + * supported it).
> + *
> + * Until this is fixed, use global vector tables as far as the irq
> + * logic is concerned to avoid the buggy behaviour of per-device
> + * maps in map_domain_pirq(), and use per-device tables as far as
> + * intremap code is concerned to avoid the security issue.
> + */
> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
> + "Using per-device-global maps instead until a fix is found\n");
> +
> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
> }
> else
> {
> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
> else
> {
> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
> +
> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
> + {
> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
> + "Use irq_vector_map=global to work around.");
> + }
> }
> if ( !amd_iommu_perdev_intremap )
> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-10 12:25 ` Jan Beulich
@ 2013-06-14 8:45 ` Jan Beulich
2013-06-15 1:13 ` Suravee Suthikulanit
1 sibling, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-14 8:45 UTC (permalink / raw)
To: Jacob Shin, SuraveeSuthikulpanit
Cc: George Dunlap, Andrew Cooper, xen-devel, Keir Fraser,
Sherry Hurwitz
>>> On 10.06.13 at 14:25, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> XSA-36 changed the default vector map mode from global to per-device. This is
>> because a global vector map does not prevent one PCI device from
>> impersonating
>> another and launching a DoS on the system.
>>
>> However, the per-device vector map logic is broken for devices with multiple
>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>> of a guests interrupt remapping tables. The core problem is not trivial to
>> fix.
>>
>> In an effort to get AMD systems back to a non-regressed state, introduce a
>> new
>> type of vector map called per-device-global. This uses per-device vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>
>> This patch is intended to be removed as soon as the per-device logic is fixed
>> correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> Suravee, Jacob,
>
> no opinion on this at all? I've been talked into considering this
> acceptable (with a small coding style fixup, and with the question on
> the usefulness of the final warning message - imo redundant with the
> immediately preceding message that is being left untouched), with
> the strict expectation that this would get reverted right away after
> 4.3, with the multi-vector MSI series being the real fix to this
> (presumably allowing to drop the vector map stuff altogether).
as we're running out of time with this for 4.3, I'm inclined to
treat the lack of a response from you as a "don't care, do what
you want" and commit the patch without an ack. As I'm in no
way happy about that, I'd like to give you a last chance to ack or
nack the patch, or comment on it in other ways. Early next week,
if no response from either of you arrived, I'm going to commit it
with a yet to be invented tag stating the situation.
Jan
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
>> {
>> if ( amd_iommu_perdev_intremap )
>> {
>> - printk("AMD-Vi: Enabling per-device vector maps\n");
>> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
>> + /* Per-device vector map logic is broken for devices with
> multiple
>> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
>> + * supported it).
>> + *
>> + * Until this is fixed, use global vector tables as far as the
> irq
>> + * logic is concerned to avoid the buggy behaviour of per-device
>> + * maps in map_domain_pirq(), and use per-device tables as far
> as
>> + * intremap code is concerned to avoid the security issue.
>> + */
>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
> broken. "
>> + "Using per-device-global maps instead until a fix is
> found\n");
>> +
>> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
>> }
>> else
>> {
>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>> else
>> {
>> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>> +
>> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>> + {
>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
> broken. "
>> + "Use irq_vector_map=global to work around.");
>> + }
>> }
>> if ( !amd_iommu_perdev_intremap )
>> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is
> not recommended (see XSA-36)!\n");
>
>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-10 12:25 ` Jan Beulich
2013-06-14 8:45 ` Jan Beulich
@ 2013-06-15 1:13 ` Suravee Suthikulanit
2013-06-17 8:19 ` Jan Beulich
2013-06-17 8:55 ` George Dunlap
1 sibling, 2 replies; 13+ messages in thread
From: Suravee Suthikulanit @ 2013-06-15 1:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir Fraser, George Dunlap, Andrew Cooper, Jacob Shin, xen-devel,
Hurwitz, Sherry
[-- Attachment #1: Type: text/plain, Size: 2218 bytes --]
On 6/10/2013 7:25 AM, Jan Beulich wrote:
>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> XSA-36 changed the default vector map mode from global to per-device. This is
>> because a global vector map does not prevent one PCI device from
>> impersonating
>> another and launching a DoS on the system.
>>
>> However, the per-device vector map logic is broken for devices with multiple
>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>> of a guests interrupt remapping tables. The core problem is not trivial to
>> fix.
>>
>> In an effort to get AMD systems back to a non-regressed state, introduce a
>> new
>> type of vector map called per-device-global. This uses per-device vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>
>> This patch is intended to be removed as soon as the per-device logic is fixed
>> correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Suravee, Jacob,
>
> no opinion on this at all? I've been talked into considering this
> acceptable
Sorry for late reply, and for having missed this conversation previously.
If we have to go with this solution temporary until we have the permanent fix.
I think that is okay with me. Although, would you mind pointing out the affect
of having "per-device" vs. "global" irq vector map? I am not quite familiar
with the differences.
> (with a small coding style fixup, and with the question on
> the usefulness of the final warning message - imo redundant with the
> immediately preceding message that is being left untouched)
I also think the messages are quite confusing. Actually, now that we can have
irq vector map and intremap map with different mode, we should be more explicit
in the message.
Also, the message "Not overriding irq_vector_map setting" is confusing to me.
Would you mind considering the attached patch? Here is the sample output
(XEN) AMD-Vi: IOMMU 0 Enabled.
(XEN) AMD-Vi BUG: per-device vector map logic is broken. Using per-device-global maps instead until a fix is found
(XEN) AMD-Vi: Enabling global irq vector map
(XEN) AMD-Vi: Enabling per-device interrupt remap table.
Thank you,
Suravee
[-- Attachment #2: AMD-intremap-Prevent-use-of-per-device-vector-maps-V4.patch --]
[-- Type: text/plain, Size: 3208 bytes --]
XSA-36 changed the default vector map mode from global to per-device. This is
because a global vector map does not prevent one PCI device from impersonating
another and launching a DoS on the system.
However, the per-device vector map logic is broken for devices with multiple
MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
of a guests interrupt remapping tables. The core problem is not trivial to
fix.
In an effort to get AMD systems back to a non-regressed state, introduce a new
type of vector map called per-device-global. This uses per-device vector maps
in the IOMMU, but uses a single used_vector map for the core IRQ logic.
This patch is intended to be removed as soon as the per-device logic is fixed
correctly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Clean up the message and explicitely list the mode of the irq map and
interrupt remap table.
Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
xen/drivers/passthrough/amd/pci_amd_iommu.c | 33 +++++++++++++++++----------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 60696d7..a11e239 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -223,21 +223,30 @@ int __init amd_iov_detect(void)
{
if ( amd_iommu_perdev_intremap )
{
- printk("AMD-Vi: Enabling per-device vector maps\n");
- opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
- }
- else
- {
- printk("AMD-Vi: Enabling global vector map\n");
- opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
+ /* Per-device vector map logic is broken for devices with multiple
+ * MSI-X interrupts (and would also be for multiple MSI, if Xen
+ * supported it).
+ *
+ * Until this is fixed, use global vector tables as far as the irq
+ * logic is concerned to avoid the buggy behaviour of per-device
+ * maps in map_domain_pirq(), and use per-device tables as far as
+ * intremap code is concerned to avoid the security issue.
+ */
+ printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
+ "Using per-device-global maps instead until a fix is found\n");
}
+ opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
}
- else
- {
- printk("AMD-Vi: Not overriding irq_vector_map setting\n");
- }
+
+ printk("AMD-Vi: Enabling %s irq vector map\n",
+ (opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_PERDEV)? "per-device": "global");
+
+ printk("AMD-Vi: Enabling %s interrupt remap table.\n",
+ (amd_iommu_perdev_intremap)? "per-device": "global");
+
if ( !amd_iommu_perdev_intremap )
- printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
+ printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended. (See XSA-36!)\n");
+
return scan_pci_devices();
}
--
1.7.10.4
[-- 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 related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-15 1:13 ` Suravee Suthikulanit
@ 2013-06-17 8:19 ` Jan Beulich
2013-06-17 8:55 ` George Dunlap
1 sibling, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-17 8:19 UTC (permalink / raw)
To: Suravee Suthikulanit
Cc: Keir Fraser, George Dunlap, Andrew Cooper, Jacob Shin, xen-devel,
Sherry Hurwitz
>>> On 15.06.13 at 03:13, Suravee Suthikulanit <suravee.suthikulpanit@amd.com> wrote:
> On 6/10/2013 7:25 AM, Jan Beulich wrote:
> If we have to go with this solution temporary until we have the permanent
> fix.
> I think that is okay with me. Although, would you mind pointing out the
> affect
> of having "per-device" vs. "global" irq vector map? I am not quite familiar
> with the differences.
This simply controls within what group of entities (apart from the
CPU grouping enforced by the APIC model - logical/cluster/physical -
used) limits the selection of a vector: "global" means the same
vector can't be used for different IRQs at all, whereas per-device
means that the same vector can't be used for multiple IRQs within
the same device (which for MSI-X doesn't currently work, and
hence isn't useful to switch to by default).
>> (with a small coding style fixup, and with the question on
>> the usefulness of the final warning message - imo redundant with the
>> immediately preceding message that is being left untouched)
>
> I also think the messages are quite confusing. Actually, now that we can
> have
> irq vector map and intremap map with different mode, we should be more
> explicit
> in the message.
>
> Also, the message "Not overriding irq_vector_map setting" is confusing to
> me.
>
> Would you mind considering the attached patch? Here is the sample output
Actually I don't see the improvement, yet I do see at least one issue:
> printk("AMD-Vi: Enabling %s irq vector map\n",
> (opt_irq_vector_map == OPT_IRQ_VECTOR_MAP_PERDEV)? "per-device": "global");
neglects that opt_irq_vector_map can have (at this point) three
different values.
I also certainly see no point in adjusting the already existing XSA-36
related message.
I'd also like to wait for Andrew's opinion, but right now I tend
towards going with his original patch.
In any case please remember that this is temporary and the code
this modifies will go away after 4.3 anyway (hence cosmetics
aren't that relevant).
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-15 1:13 ` Suravee Suthikulanit
2013-06-17 8:19 ` Jan Beulich
@ 2013-06-17 8:55 ` George Dunlap
2013-06-17 9:00 ` Jan Beulich
1 sibling, 1 reply; 13+ messages in thread
From: George Dunlap @ 2013-06-17 8:55 UTC (permalink / raw)
To: Suravee Suthikulanit
Cc: Keir Fraser, Andrew Cooper, Jacob Shin, xen-devel, Jan Beulich,
Hurwitz, Sherry
On 15/06/13 02:13, Suravee Suthikulanit wrote:
> On 6/10/2013 7:25 AM, Jan Beulich wrote:
>>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com>
>>>>> wrote:
>>> XSA-36 changed the default vector map mode from global to
>>> per-device. This is
>>> because a global vector map does not prevent one PCI device from
>>> impersonating
>>> another and launching a DoS on the system.
>>>
>>> However, the per-device vector map logic is broken for devices with
>>> multiple
>>> MSI-X vectors, which can either result in a failed ASSERT() or
>>> misprogramming
>>> of a guests interrupt remapping tables. The core problem is not
>>> trivial to
>>> fix.
>>>
>>> In an effort to get AMD systems back to a non-regressed state,
>>> introduce a
>>> new
>>> type of vector map called per-device-global. This uses per-device
>>> vector maps
>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>
>>> This patch is intended to be removed as soon as the per-device logic
>>> is fixed
>>> correctly.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Suravee, Jacob,
>>
>> no opinion on this at all? I've been talked into considering this
>> acceptable
>
> Sorry for late reply, and for having missed this conversation previously.
>
> If we have to go with this solution temporary until we have the
> permanent fix.
> I think that is okay with me. Although, would you mind pointing out
> the affect
> of having "per-device" vs. "global" irq vector map? I am not quite
> familiar
> with the differences.
>
>> (with a small coding style fixup, and with the question on
>> the usefulness of the final warning message - imo redundant with the
>> immediately preceding message that is being left untouched)
>
> I also think the messages are quite confusing. Actually, now that we
> can have
> irq vector map and intremap map with different mode, we should be more
> explicit
> in the message.
>
> Also, the message "Not overriding irq_vector_map setting" is confusing
> to me.
>
> Would you mind considering the attached patch? Here is the sample output
>
> (XEN) AMD-Vi: IOMMU 0 Enabled.
> (XEN) AMD-Vi BUG: per-device vector map logic is broken. Using
> per-device-global maps instead until a fix is found
At the very least it can't say BUG -- that needs to be reserved for
things that actually cause the host to crash (a la BUG_ON()).
-George
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-17 8:55 ` George Dunlap
@ 2013-06-17 9:00 ` Jan Beulich
2013-06-17 10:01 ` Andrew Cooper
0 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2013-06-17 9:00 UTC (permalink / raw)
To: Suravee Suthikulanit, George Dunlap
Cc: AndrewCooper, Keir Fraser, xen-devel, Jacob Shin, Sherry Hurwitz
>>> On 17.06.13 at 10:55, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 15/06/13 02:13, Suravee Suthikulanit wrote:
>> On 6/10/2013 7:25 AM, Jan Beulich wrote:
>>>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>> wrote:
>>>> XSA-36 changed the default vector map mode from global to
>>>> per-device. This is
>>>> because a global vector map does not prevent one PCI device from
>>>> impersonating
>>>> another and launching a DoS on the system.
>>>>
>>>> However, the per-device vector map logic is broken for devices with
>>>> multiple
>>>> MSI-X vectors, which can either result in a failed ASSERT() or
>>>> misprogramming
>>>> of a guests interrupt remapping tables. The core problem is not
>>>> trivial to
>>>> fix.
>>>>
>>>> In an effort to get AMD systems back to a non-regressed state,
>>>> introduce a
>>>> new
>>>> type of vector map called per-device-global. This uses per-device
>>>> vector maps
>>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>>
>>>> This patch is intended to be removed as soon as the per-device logic
>>>> is fixed
>>>> correctly.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Suravee, Jacob,
>>>
>>> no opinion on this at all? I've been talked into considering this
>>> acceptable
>>
>> Sorry for late reply, and for having missed this conversation previously.
>>
>> If we have to go with this solution temporary until we have the
>> permanent fix.
>> I think that is okay with me. Although, would you mind pointing out
>> the affect
>> of having "per-device" vs. "global" irq vector map? I am not quite
>> familiar
>> with the differences.
>>
>>> (with a small coding style fixup, and with the question on
>>> the usefulness of the final warning message - imo redundant with the
>>> immediately preceding message that is being left untouched)
>>
>> I also think the messages are quite confusing. Actually, now that we
>> can have
>> irq vector map and intremap map with different mode, we should be more
>> explicit
>> in the message.
>>
>> Also, the message "Not overriding irq_vector_map setting" is confusing
>> to me.
>>
>> Would you mind considering the attached patch? Here is the sample output
>>
>> (XEN) AMD-Vi: IOMMU 0 Enabled.
>> (XEN) AMD-Vi BUG: per-device vector map logic is broken. Using
>> per-device-global maps instead until a fix is found
>
> At the very least it can't say BUG -- that needs to be reserved for
> things that actually cause the host to crash (a la BUG_ON()).
That was worded this way in Andrew's original version of the patch
too, and I had also already noted that this wording is too strong.
Jan
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-17 9:00 ` Jan Beulich
@ 2013-06-17 10:01 ` Andrew Cooper
0 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2013-06-17 10:01 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir (Xen.org), George Dunlap, Jacob Shin,
xen-devel@lists.xen.org, Suravee Suthikulanit, Sherry Hurwitz
On 17/06/13 10:00, Jan Beulich wrote:
>>>> On 17.06.13 at 10:55, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 15/06/13 02:13, Suravee Suthikulanit wrote:
>>> On 6/10/2013 7:25 AM, Jan Beulich wrote:
>>>>>>> On 04.06.13 at 18:38, Andrew Cooper <andrew.cooper3@citrix.com>
>>>>>>> wrote:
>>>>> XSA-36 changed the default vector map mode from global to
>>>>> per-device. This is
>>>>> because a global vector map does not prevent one PCI device from
>>>>> impersonating
>>>>> another and launching a DoS on the system.
>>>>>
>>>>> However, the per-device vector map logic is broken for devices with
>>>>> multiple
>>>>> MSI-X vectors, which can either result in a failed ASSERT() or
>>>>> misprogramming
>>>>> of a guests interrupt remapping tables. The core problem is not
>>>>> trivial to
>>>>> fix.
>>>>>
>>>>> In an effort to get AMD systems back to a non-regressed state,
>>>>> introduce a
>>>>> new
>>>>> type of vector map called per-device-global. This uses per-device
>>>>> vector maps
>>>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>>>
>>>>> This patch is intended to be removed as soon as the per-device logic
>>>>> is fixed
>>>>> correctly.
>>>>>
>>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> Suravee, Jacob,
>>>>
>>>> no opinion on this at all? I've been talked into considering this
>>>> acceptable
>>> Sorry for late reply, and for having missed this conversation previously.
>>>
>>> If we have to go with this solution temporary until we have the
>>> permanent fix.
>>> I think that is okay with me. Although, would you mind pointing out
>>> the affect
>>> of having "per-device" vs. "global" irq vector map? I am not quite
>>> familiar
>>> with the differences.
>>>
>>>> (with a small coding style fixup, and with the question on
>>>> the usefulness of the final warning message - imo redundant with the
>>>> immediately preceding message that is being left untouched)
>>> I also think the messages are quite confusing. Actually, now that we
>>> can have
>>> irq vector map and intremap map with different mode, we should be more
>>> explicit
>>> in the message.
>>>
>>> Also, the message "Not overriding irq_vector_map setting" is confusing
>>> to me.
>>>
>>> Would you mind considering the attached patch? Here is the sample output
>>>
>>> (XEN) AMD-Vi: IOMMU 0 Enabled.
>>> (XEN) AMD-Vi BUG: per-device vector map logic is broken. Using
>>> per-device-global maps instead until a fix is found
>> At the very least it can't say BUG -- that needs to be reserved for
>> things that actually cause the host to crash (a la BUG_ON()).
> That was worded this way in Andrew's original version of the patch
> too, and I had also already noted that this wording is too strong.
>
> Jan
>
I am not overly attached to the current wording, so feel free to tweak
if you wish.
As for my opinions of the revised patch:
The explicit print of the IOMMU mode is nice, although those in the know
could already work it out given the reference or lackthereof to XSA-36.
The explicit print of the vector map is wrong, and is liable to be
disappearing in 4.4 anyway.
On the face of it, the revised patch seems much more like a general
cleanup of the printing, rather than the temporary bugfix it is indented
to be. I dont have a problem with the cleanup persay, but it should be
part of a separate patch.
~Andrew
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-04 16:38 [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed Andrew Cooper
2013-06-10 12:25 ` Jan Beulich
@ 2013-06-26 9:54 ` Andrew Cooper
2013-06-26 23:28 ` Suravee Suthikulanit
1 sibling, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2013-06-26 9:54 UTC (permalink / raw)
To: xen-devel@lists.xen.org
Cc: Keir (Xen.org), Jacob Shin, Jan Beulich, Suravee Suthikulpanit
On 04/06/13 17:38, Andrew Cooper wrote:
> XSA-36 changed the default vector map mode from global to per-device. This is
> because a global vector map does not prevent one PCI device from impersonating
> another and launching a DoS on the system.
>
> However, the per-device vector map logic is broken for devices with multiple
> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
> of a guests interrupt remapping tables. The core problem is not trivial to
> fix.
>
> In an effort to get AMD systems back to a non-regressed state, introduce a new
> type of vector map called per-device-global. This uses per-device vector maps
> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>
> This patch is intended to be removed as soon as the per-device logic is fixed
> correctly.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Can we get a decision on this? The 4.3 is looming and multi MSI-X PCI
functions are *still* broken on AMD systems, in all stable versions of
Xen, regressed by XSA-36.
>From my understanding of the points so far, we have agreed that this
patch is suitable for 4.3 and previous, with Jan's multi-MSI series
being the correct solution going forwards into 4.4.
The only query at the moment is for the exact wording, which has had no
attention for a week.
~Andrew
>
> ---
> Changes since v2:
> * Do not override command line.
> * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>
> Changes since v1:
> * Correct stupid mistake in commit message, making it confusing to read
>
> diff -r 2d37d2d652a8 -r a017d74f346d xen/drivers/passthrough/amd/pci_amd_iommu.c
> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
> {
> if ( amd_iommu_perdev_intremap )
> {
> - printk("AMD-Vi: Enabling per-device vector maps\n");
> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
> + /* Per-device vector map logic is broken for devices with multiple
> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
> + * supported it).
> + *
> + * Until this is fixed, use global vector tables as far as the irq
> + * logic is concerned to avoid the buggy behaviour of per-device
> + * maps in map_domain_pirq(), and use per-device tables as far as
> + * intremap code is concerned to avoid the security issue.
> + */
> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
> + "Using per-device-global maps instead until a fix is found\n");
> +
> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
> }
> else
> {
> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
> else
> {
> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
> +
> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
> + {
> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
> + "Use irq_vector_map=global to work around.");
> + }
> }
> if ( !amd_iommu_perdev_intremap )
> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-26 9:54 ` Andrew Cooper
@ 2013-06-26 23:28 ` Suravee Suthikulanit
2013-06-27 8:47 ` Jan Beulich
0 siblings, 1 reply; 13+ messages in thread
From: Suravee Suthikulanit @ 2013-06-26 23:28 UTC (permalink / raw)
To: Andrew Cooper
Cc: Keir (Xen.org), Jacob Shin, Jan Beulich, xen-devel@lists.xen.org
On 6/26/2013 4:54 AM, Andrew Cooper wrote:
> On 04/06/13 17:38, Andrew Cooper wrote:
>> XSA-36 changed the default vector map mode from global to per-device. This is
>> because a global vector map does not prevent one PCI device from impersonating
>> another and launching a DoS on the system.
>>
>> However, the per-device vector map logic is broken for devices with multiple
>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>> of a guests interrupt remapping tables. The core problem is not trivial to
>> fix.
>>
>> In an effort to get AMD systems back to a non-regressed state, introduce a new
>> type of vector map called per-device-global. This uses per-device vector maps
>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>
>> This patch is intended to be removed as soon as the per-device logic is fixed
>> correctly.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Can we get a decision on this? The 4.3 is looming and multi MSI-X PCI
> functions are *still* broken on AMD systems, in all stable versions of
> Xen, regressed by XSA-36.
>
> From my understanding of the points so far, we have agreed that this
> patch is suitable for 4.3 and previous, with Jan's multi-MSI series
> being the correct solution going forwards into 4.4.
Since the feedback suggesting that cleaning up is probably not
necessary, the only thing is probably the use of the word "BUG". Could
it be replaced with "Workaround" instead?
Suravee
>
> The only query at the moment is for the exact wording, which has had no
> attention for a week.
>
> ~Andrew
>
>> ---
>> Changes since v2:
>> * Do not override command line.
>> * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>>
>> Changes since v1:
>> * Correct stupid mistake in commit message, making it confusing to read
>>
>> diff -r 2d37d2d652a8 -r a017d74f346d xen/drivers/passthrough/amd/pci_amd_iommu.c
>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
>> {
>> if ( amd_iommu_perdev_intremap )
>> {
>> - printk("AMD-Vi: Enabling per-device vector maps\n");
>> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
>> + /* Per-device vector map logic is broken for devices with multiple
>> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
>> + * supported it).
>> + *
>> + * Until this is fixed, use global vector tables as far as the irq
>> + * logic is concerned to avoid the buggy behaviour of per-device
>> + * maps in map_domain_pirq(), and use per-device tables as far as
>> + * intremap code is concerned to avoid the security issue.
>> + */
>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
>> + "Using per-device-global maps instead until a fix is found\n");
>> +
>> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
>> }
>> else
>> {
>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>> else
>> {
>> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>> +
>> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>> + {
>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is broken. "
>> + "Use irq_vector_map=global to work around.");
>> + }
>> }
>> if ( !amd_iommu_perdev_intremap )
>> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table is not recommended (see XSA-36)!\n");
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-26 23:28 ` Suravee Suthikulanit
@ 2013-06-27 8:47 ` Jan Beulich
2013-06-27 9:13 ` Andrew Cooper
2013-06-27 11:20 ` Suravee Suthikulpanit
0 siblings, 2 replies; 13+ messages in thread
From: Jan Beulich @ 2013-06-27 8:47 UTC (permalink / raw)
To: Suravee Suthikulanit
Cc: Andrew Cooper, Keir (Xen.org), Jacob Shin,
xen-devel@lists.xen.org
>>> On 27.06.13 at 01:28, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
wrote:
> On 6/26/2013 4:54 AM, Andrew Cooper wrote:
>> On 04/06/13 17:38, Andrew Cooper wrote:
>>> XSA-36 changed the default vector map mode from global to per-device. This is
>>> because a global vector map does not prevent one PCI device from
> impersonating
>>> another and launching a DoS on the system.
>>>
>>> However, the per-device vector map logic is broken for devices with multiple
>>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>>> of a guests interrupt remapping tables. The core problem is not trivial to
>>> fix.
>>>
>>> In an effort to get AMD systems back to a non-regressed state, introduce a
> new
>>> type of vector map called per-device-global. This uses per-device vector maps
>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>
>>> This patch is intended to be removed as soon as the per-device logic is fixed
>>> correctly.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Can we get a decision on this? The 4.3 is looming and multi MSI-X PCI
>> functions are *still* broken on AMD systems, in all stable versions of
>> Xen, regressed by XSA-36.
>>
>> From my understanding of the points so far, we have agreed that this
>> patch is suitable for 4.3 and previous, with Jan's multi-MSI series
>> being the correct solution going forwards into 4.4.
> Since the feedback suggesting that cleaning up is probably not
> necessary, the only thing is probably the use of the word "BUG". Could
> it be replaced with "Workaround" instead?
I'd just drop the "BUG:". And I can certainly do so while applying.
So in cases where you want something trivial changed, you could
simply give an ack saying under what conditions that ack applies.
Jan
>> The only query at the moment is for the exact wording, which has had no
>> attention for a week.
>>
>> ~Andrew
>>
>>> ---
>>> Changes since v2:
>>> * Do not override command line.
>>> * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>>>
>>> Changes since v1:
>>> * Correct stupid mistake in commit message, making it confusing to read
>>>
>>> diff -r 2d37d2d652a8 -r a017d74f346d
> xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
>>> {
>>> if ( amd_iommu_perdev_intremap )
>>> {
>>> - printk("AMD-Vi: Enabling per-device vector maps\n");
>>> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
>>> + /* Per-device vector map logic is broken for devices with
> multiple
>>> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
>>> + * supported it).
>>> + *
>>> + * Until this is fixed, use global vector tables as far as the
> irq
>>> + * logic is concerned to avoid the buggy behaviour of per-device
>>> + * maps in map_domain_pirq(), and use per-device tables as far
> as
>>> + * intremap code is concerned to avoid the security issue.
>>> + */
>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
> broken. "
>>> + "Using per-device-global maps instead until a fix is
> found\n");
>>> +
>>> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
>>> }
>>> else
>>> {
>>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>>> else
>>> {
>>> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>>> +
>>> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>>> + {
>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
> broken. "
>>> + "Use irq_vector_map=global to work around.");
>>> + }
>>> }
>>> if ( !amd_iommu_perdev_intremap )
>>> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table
> is not recommended (see XSA-36)!\n");
>>>
>>> _______________________________________________
>>> Xen-devel mailing list
>>> Xen-devel@lists.xen.org
>>> http://lists.xen.org/xen-devel
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-27 8:47 ` Jan Beulich
@ 2013-06-27 9:13 ` Andrew Cooper
2013-06-27 11:20 ` Suravee Suthikulpanit
1 sibling, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2013-06-27 9:13 UTC (permalink / raw)
To: Jan Beulich
Cc: Keir (Xen.org), Jacob Shin, Suravee Suthikulanit,
xen-devel@lists.xen.org
On 27/06/13 09:47, Jan Beulich wrote:
>>>> On 27.06.13 at 01:28, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 6/26/2013 4:54 AM, Andrew Cooper wrote:
>>> On 04/06/13 17:38, Andrew Cooper wrote:
>>>> XSA-36 changed the default vector map mode from global to per-device. This is
>>>> because a global vector map does not prevent one PCI device from
>> impersonating
>>>> another and launching a DoS on the system.
>>>>
>>>> However, the per-device vector map logic is broken for devices with multiple
>>>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>>>> of a guests interrupt remapping tables. The core problem is not trivial to
>>>> fix.
>>>>
>>>> In an effort to get AMD systems back to a non-regressed state, introduce a
>> new
>>>> type of vector map called per-device-global. This uses per-device vector maps
>>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>>
>>>> This patch is intended to be removed as soon as the per-device logic is fixed
>>>> correctly.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Can we get a decision on this? The 4.3 is looming and multi MSI-X PCI
>>> functions are *still* broken on AMD systems, in all stable versions of
>>> Xen, regressed by XSA-36.
>>>
>>> From my understanding of the points so far, we have agreed that this
>>> patch is suitable for 4.3 and previous, with Jan's multi-MSI series
>>> being the correct solution going forwards into 4.4.
>> Since the feedback suggesting that cleaning up is probably not
>> necessary, the only thing is probably the use of the word "BUG". Could
>> it be replaced with "Workaround" instead?
> I'd just drop the "BUG:". And I can certainly do so while applying.
> So in cases where you want something trivial changed, you could
> simply give an ack saying under what conditions that ack applies.
>
> Jan
I am happy with either of the two suggested tweaks to the wording.
~Andrew
>
>>> The only query at the moment is for the exact wording, which has had no
>>> attention for a week.
>>>
>>> ~Andrew
>>>
>>>> ---
>>>> Changes since v2:
>>>> * Do not override command line.
>>>> * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>>>>
>>>> Changes since v1:
>>>> * Correct stupid mistake in commit message, making it confusing to read
>>>>
>>>> diff -r 2d37d2d652a8 -r a017d74f346d
>> xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
>>>> {
>>>> if ( amd_iommu_perdev_intremap )
>>>> {
>>>> - printk("AMD-Vi: Enabling per-device vector maps\n");
>>>> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
>>>> + /* Per-device vector map logic is broken for devices with
>> multiple
>>>> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
>>>> + * supported it).
>>>> + *
>>>> + * Until this is fixed, use global vector tables as far as the
>> irq
>>>> + * logic is concerned to avoid the buggy behaviour of per-device
>>>> + * maps in map_domain_pirq(), and use per-device tables as far
>> as
>>>> + * intremap code is concerned to avoid the security issue.
>>>> + */
>>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
>> broken. "
>>>> + "Using per-device-global maps instead until a fix is
>> found\n");
>>>> +
>>>> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
>>>> }
>>>> else
>>>> {
>>>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>>>> else
>>>> {
>>>> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>>>> +
>>>> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>>>> + {
>>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
>> broken. "
>>>> + "Use irq_vector_map=global to work around.");
>>>> + }
>>>> }
>>>> if ( !amd_iommu_perdev_intremap )
>>>> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table
>> is not recommended (see XSA-36)!\n");
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed
2013-06-27 8:47 ` Jan Beulich
2013-06-27 9:13 ` Andrew Cooper
@ 2013-06-27 11:20 ` Suravee Suthikulpanit
1 sibling, 0 replies; 13+ messages in thread
From: Suravee Suthikulpanit @ 2013-06-27 11:20 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Keir (Xen.org), Jacob Shin,
xen-devel@lists.xen.org
On 6/27/2013 3:47 AM, Jan Beulich wrote:
>>>> On 27.06.13 at 01:28, Suravee Suthikulanit <suravee.suthikulpanit@amd.com>
> wrote:
>> On 6/26/2013 4:54 AM, Andrew Cooper wrote:
>>> On 04/06/13 17:38, Andrew Cooper wrote:
>>>> XSA-36 changed the default vector map mode from global to per-device. This is
>>>> because a global vector map does not prevent one PCI device from
>> impersonating
>>>> another and launching a DoS on the system.
>>>>
>>>> However, the per-device vector map logic is broken for devices with multiple
>>>> MSI-X vectors, which can either result in a failed ASSERT() or misprogramming
>>>> of a guests interrupt remapping tables. The core problem is not trivial to
>>>> fix.
>>>>
>>>> In an effort to get AMD systems back to a non-regressed state, introduce a
>> new
>>>> type of vector map called per-device-global. This uses per-device vector maps
>>>> in the IOMMU, but uses a single used_vector map for the core IRQ logic.
>>>>
>>>> This patch is intended to be removed as soon as the per-device logic is fixed
>>>> correctly.
>>>>
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Can we get a decision on this? The 4.3 is looming and multi MSI-X PCI
>>> functions are *still* broken on AMD systems, in all stable versions of
>>> Xen, regressed by XSA-36.
>>>
>>> From my understanding of the points so far, we have agreed that this
>>> patch is suitable for 4.3 and previous, with Jan's multi-MSI series
>>> being the correct solution going forwards into 4.4.
>> Since the feedback suggesting that cleaning up is probably not
>> necessary, the only thing is probably the use of the word "BUG". Could
>> it be replaced with "Workaround" instead?
> I'd just drop the "BUG:". And I can certainly do so while applying.
> So in cases where you want something trivial changed, you could
> simply give an ack saying under what conditions that ack applies.
>
> Jan
Acked with removing the word "BUG" as suggested by Jan.
Suravee
>
>>> The only query at the moment is for the exact wording, which has had no
>>> attention for a week.
>>>
>>> ~Andrew
>>>
>>>> ---
>>>> Changes since v2:
>>>> * Do not override command line.
>>>> * reuse OPT_IRQ_VECTOR_MAP_GLOBAL.
>>>>
>>>> Changes since v1:
>>>> * Correct stupid mistake in commit message, making it confusing to read
>>>>
>>>> diff -r 2d37d2d652a8 -r a017d74f346d
>> xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
>>>> @@ -223,8 +223,19 @@ int __init amd_iov_detect(void)
>>>> {
>>>> if ( amd_iommu_perdev_intremap )
>>>> {
>>>> - printk("AMD-Vi: Enabling per-device vector maps\n");
>>>> - opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_PERDEV;
>>>> + /* Per-device vector map logic is broken for devices with
>> multiple
>>>> + * MSI-X interrupts (and would also be for multiple MSI, if Xen
>>>> + * supported it).
>>>> + *
>>>> + * Until this is fixed, use global vector tables as far as the
>> irq
>>>> + * logic is concerned to avoid the buggy behaviour of per-device
>>>> + * maps in map_domain_pirq(), and use per-device tables as far
>> as
>>>> + * intremap code is concerned to avoid the security issue.
>>>> + */
>>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
>> broken. "
>>>> + "Using per-device-global maps instead until a fix is
>> found\n");
>>>> +
>>>> + opt_irq_vector_map = OPT_IRQ_VECTOR_MAP_GLOBAL;
>>>> }
>>>> else
>>>> {
>>>> @@ -235,6 +246,12 @@ int __init amd_iov_detect(void)
>>>> else
>>>> {
>>>> printk("AMD-Vi: Not overriding irq_vector_map setting\n");
>>>> +
>>>> + if ( opt_irq_vector_map != OPT_IRQ_VECTOR_MAP_GLOBAL )
>>>> + {
>>>> + printk(XENLOG_WARNING "AMD-Vi BUG: per-device vector map logic is
>> broken. "
>>>> + "Use irq_vector_map=global to work around.");
>>>> + }
>>>> }
>>>> if ( !amd_iommu_perdev_intremap )
>>>> printk(XENLOG_WARNING "AMD-Vi: Using global interrupt remap table
>> is not recommended (see XSA-36)!\n");
>>>> _______________________________________________
>>>> Xen-devel mailing list
>>>> Xen-devel@lists.xen.org
>>>> http://lists.xen.org/xen-devel
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2013-06-27 11:20 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-04 16:38 [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed Andrew Cooper
2013-06-10 12:25 ` Jan Beulich
2013-06-14 8:45 ` Jan Beulich
2013-06-15 1:13 ` Suravee Suthikulanit
2013-06-17 8:19 ` Jan Beulich
2013-06-17 8:55 ` George Dunlap
2013-06-17 9:00 ` Jan Beulich
2013-06-17 10:01 ` Andrew Cooper
2013-06-26 9:54 ` Andrew Cooper
2013-06-26 23:28 ` Suravee Suthikulanit
2013-06-27 8:47 ` Jan Beulich
2013-06-27 9:13 ` Andrew Cooper
2013-06-27 11:20 ` Suravee Suthikulpanit
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).