From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH v3] AMD/intremap: Prevent use of per-device vector maps until irq logic is fixed Date: Thu, 27 Jun 2013 10:13:06 +0100 Message-ID: <51CC0222.90909@citrix.com> References: <51CABA47.6030107@citrix.com> <51CB7937.3000002@amd.com> <51CC185C02000078000E1116@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51CC185C02000078000E1116@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 (Xen.org)" , Jacob Shin , Suravee Suthikulanit , "xen-devel@lists.xen.org" List-Id: xen-devel@lists.xenproject.org On 27/06/13 09:47, Jan Beulich wrote: >>>> On 27.06.13 at 01:28, Suravee Suthikulanit > 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 >>> 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 >