From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulanit Subject: Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed Date: Fri, 14 Jun 2013 20:13:01 -0500 Message-ID: <51BBBF9D.9060404@amd.com> References: <51B5E1CD02000078000DCA59@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------030107090005090303050103" Return-path: In-Reply-To: <51B5E1CD02000078000DCA59@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Keir Fraser , George Dunlap , Andrew Cooper , Jacob Shin , xen-devel@lists.xen.org, "Hurwitz, Sherry" List-Id: xen-devel@lists.xenproject.org --------------030107090005090303050103 Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit On 6/10/2013 7:25 AM, Jan Beulich wrote: >>>> On 04.06.13 at 18: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 > 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 --------------030107090005090303050103 Content-Type: text/plain; charset="windows-1252"; name="AMD-intremap-Prevent-use-of-per-device-vector-maps-V4.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="AMD-intremap-Prevent-use-of-per-device-vector-maps-V4.patch" Content-Description: AMD-intremap-Prevent-use-of-per-device-vector-maps-V4.patch 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 Clean up the message and explicitely list the mode of the irq map and interrupt remap table. Signed-off-by: Suravee Suthikulpanit --- 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 --------------030107090005090303050103 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------030107090005090303050103--