Xen devel - Jan 2013 - [PATCH] VTD/Intremap: Disable Intremap on Chipset 5500/5520/X58 due to errata

http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-chipset-ioh-specification-update.html

Stepping B-3 has two errata (#47 and #53) related to Interrupt
remapping, to which the workaround is for the BIOS to completely disable
interrupt remapping.  These errata are fixed in stepping C-2.

Unfortunately this chipset is very common and many BIOSes are not
disabling remapping.  We can detect this in Xen and prevent turning on
remapping in the first place.  However, this will turn VT-d off on many
systems by default.

Users who still wish to use VT-d can use iommu=force if they are happy
exposing the associated security risk.

Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

diff -r 35a0556a7f76 -r ee475f0e6aeb xen/drivers/passthrough/vtd/quirks.c
--- a/xen/drivers/passthrough/vtd/quirks.c
+++ b/xen/drivers/passthrough/vtd/quirks.c
@@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
     }
 }
 
+/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
+ * Fixed in stepping C-2. */
+void __init tylersburg_intremap_quirk(void)
+{
+    uint32_t bus, device;
+    uint8_t rev;
+
+    for ( bus = 0; bus < 0x100; bus++ )
+    {
+        /* Match on System Management Registers on Device 20 Function 0 */
+        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
+        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
+
+        if ( rev == 0x13 && device == 0x342e8086 )
+        {
+            dprintk(XENLOG_INFO VTDPREFIX,
+                    "Disabling Interrupt Remapping due to Intel
5500/5520/X58 Chipset errata #47, #53\n");
+            iommu_intremap = 0;
+            break;
+        }
+    }
+}
+
 /* initialize platform identification flags */
 void __init platform_quirks_init(void)
 {
@@ -264,6 +287,9 @@ void __init platform_quirks_init(void)
 
     /* ioremap IGD MMIO+0x2000 page */
     map_igd_reg();
+
+    /* Tylersburg interrupt remap quirk */
+    tylersburg_intremap_quirk();
 }
 
 /*

Thanks!
Acked-by: Xiantao Zhang <xiantao.zhang@intel.com>
Xiantao
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Malcolm Crossley
> Sent: Wednesday, January 16, 2013 7:27 AM
> To: xen-devel@lists.xensource.com
> Cc: andrew.cooper3@citrix.com; Zhang, Xiantao
> Subject: [Xen-devel] [PATCH] VTD/Intremap: Disable Intremap on Chipset
> 5500/5520/X58 due to errata
> 
> http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-
> chipset-ioh-specification-update.html
> 
> Stepping B-3 has two errata (#47 and #53) related to Interrupt
> remapping, to which the workaround is for the BIOS to completely disable
> interrupt remapping.  These errata are fixed in stepping C-2.
> 
> Unfortunately this chipset is very common and many BIOSes are not
> disabling remapping.  We can detect this in Xen and prevent turning on
> remapping in the first place.  However, this will turn VT-d off on many
> systems by default.
> 
> Users who still wish to use VT-d can use iommu=force if they are happy
> exposing the associated security risk.
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 35a0556a7f76 -r ee475f0e6aeb xen/drivers/passthrough/vtd/quirks.c
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
>      }
>  }
> 
> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
> + * Fixed in stepping C-2. */
> +void __init tylersburg_intremap_quirk(void)
> +{
> +    uint32_t bus, device;
> +    uint8_t rev;
> +
> +    for ( bus = 0; bus < 0x100; bus++ )
> +    {
> +        /* Match on System Management Registers on Device 20 Function 0 */
> +        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
> +
> +        if ( rev == 0x13 && device == 0x342e8086 )
> +        {
> +            dprintk(XENLOG_INFO VTDPREFIX,
> +                    "Disabling Interrupt Remapping due to Intel
5500/5520/X58
> Chipset errata #47, #53\n");
> +            iommu_intremap = 0;
> +            break;
> +        }
> +    }
> +}
> +
>  /* initialize platform identification flags */
>  void __init platform_quirks_init(void)
>  {
> @@ -264,6 +287,9 @@ void __init platform_quirks_init(void)
> 
>      /* ioremap IGD MMIO+0x2000 page */
>      map_igd_reg();
> +
> +    /* Tylersburg interrupt remap quirk */
> +    tylersburg_intremap_quirk();
>  }
> 
>  /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>
http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-chipset-ioh-specific
> ation-update.html
> 
> Stepping B-3 has two errata (#47 and #53) related to Interrupt
> remapping, to which the workaround is for the BIOS to completely disable
> interrupt remapping.  These errata are fixed in stepping C-2.
> 
> Unfortunately this chipset is very common and many BIOSes are not
> disabling remapping.  We can detect this in Xen and prevent turning on
> remapping in the first place.  However, this will turn VT-d off on many
> systems by default.
> 
> Users who still wish to use VT-d can use iommu=force if they are happy
> exposing the associated security risk.
> 
> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> diff -r 35a0556a7f76 -r ee475f0e6aeb xen/drivers/passthrough/vtd/quirks.c
> --- a/xen/drivers/passthrough/vtd/quirks.c
> +++ b/xen/drivers/passthrough/vtd/quirks.c
> @@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
>      }
>  }
>  
> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
> + * Fixed in stepping C-2. */
> +void __init tylersburg_intremap_quirk(void)
> +{
> +    uint32_t bus, device;
> +    uint8_t rev;
> +
> +    for ( bus = 0; bus < 0x100; bus++ )
> +    {
> +        /* Match on System Management Registers on Device 20 Function 0 */
> +        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
> +
> +        if ( rev == 0x13 && device == 0x342e8086 )
> +        {
> +            dprintk(XENLOG_INFO VTDPREFIX,
> +                    "Disabling Interrupt Remapping due to Intel
5500/5520/X58 Chipset errata #47, #53\n");
> +            iommu_intremap = 0;
Unless it is guaranteed that no system with this chipset can have
x2APIC, I don''t think this is right. For one, this happens way too
late (namely after x2apic_bsp_setup()). And second, if you move
this earlier, such systems with x2APIC pre-enabled won''t boot
anymore.

Jan
> +            break;
> +        }
> +    }
> +}
> +
>  /* initialize platform identification flags */
>  void __init platform_quirks_init(void)
>  {
> @@ -264,6 +287,9 @@ void __init platform_quirks_init(void)
>  
>      /* ioremap IGD MMIO+0x2000 page */
>      map_igd_reg();
> +
> +    /* Tylersburg interrupt remap quirk */
> +    tylersburg_intremap_quirk();
>  }
>  
>  /*
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel

>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
> Unfortunately this chipset is very common and many BIOSes are not
> disabling remapping.  We can detect this in Xen and prevent turning on
> remapping in the first place.  However, this will turn VT-d off on many
> systems by default.
Why would the IOMMU be turned off in that case as a whole?
Interrupt remapping is not a prerequisite iirc.
> Users who still wish to use VT-d can use iommu=force if they are happy
> exposing the associated security risk.
"iommu=force" doesn''t enable interrupt remapping if it was
forcibly turned off.

Jan

On 16/01/13 14:14, Jan Beulich wrote:
>>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>> Unfortunately this chipset is very common and many BIOSes are not
>> disabling remapping.  We can detect this in Xen and prevent turning on
>> remapping in the first place.  However, this will turn VT-d off on many
>> systems by default.
> Why would the IOMMU be turned off in that case as a whole?
> Interrupt remapping is not a prerequisite iirc.
You are correct. I read the description of XSA-3 but didn''t the check 
the current xen-unstable code correctly.
>> Users who still wish to use VT-d can use iommu=force if they are happy
>> exposing the associated security risk.
> "iommu=force" doesn''t enable interrupt remapping if it
was
> forcibly turned off.
This was based upon the faulty code reading above and therefore the 
comment is wrong . I will change the patch description.
> Jan
>

On 16/01/13 13:57, Jan Beulich wrote:
>>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>>
http://www.intel.com/content/www/us/en/chipsets/5520-and-5500-chipset-ioh-specific
>> ation-update.html
>>
>> Stepping B-3 has two errata (#47 and #53) related to Interrupt
>> remapping, to which the workaround is for the BIOS to completely
disable
>> interrupt remapping.  These errata are fixed in stepping C-2.
>>
>> Unfortunately this chipset is very common and many BIOSes are not
>> disabling remapping.  We can detect this in Xen and prevent turning on
>> remapping in the first place.  However, this will turn VT-d off on many
>> systems by default.
>>
>> Users who still wish to use VT-d can use iommu=force if they are happy
>> exposing the associated security risk.
>>
>> Signed-off-by: Malcolm Crossley <malcolm.crossley@citrix.com>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>
>> diff -r 35a0556a7f76 -r ee475f0e6aeb
xen/drivers/passthrough/vtd/quirks.c
>> --- a/xen/drivers/passthrough/vtd/quirks.c
>> +++ b/xen/drivers/passthrough/vtd/quirks.c
>> @@ -244,6 +244,29 @@ void vtd_ops_postamble_quirk(struct iomm
>>       }
>>   }
>>   
>> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping B-3.
>> + * Fixed in stepping C-2. */
>> +void __init tylersburg_intremap_quirk(void)
>> +{
>> +    uint32_t bus, device;
>> +    uint8_t rev;
>> +
>> +    for ( bus = 0; bus < 0x100; bus++ )
>> +    {
>> +        /* Match on System Management Registers on Device 20 Function
0 */
>> +        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
>> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
>> +
>> +        if ( rev == 0x13 && device == 0x342e8086 )
>> +        {
>> +            dprintk(XENLOG_INFO VTDPREFIX,
>> +                    "Disabling Interrupt Remapping due to Intel
5500/5520/X58 Chipset errata #47, #53\n");
>> +            iommu_intremap = 0;
> Unless it is guaranteed that no system with this chipset can have
> x2APIC, I don''t think this is right. For one, this happens way too
> late (namely after x2apic_bsp_setup()). And second, if you move
> this earlier, such systems with x2APIC pre-enabled won''t boot
> anymore.
After some digging, I discovered that the errata affects chipsets 
(5520,5500,X58) which don''t have IOMMU EIM( Extended Interrupt Mode) 
support. EIM is required to support x2APIC mode and so this means the 
chipset can''t support x2APIC mode and so we should never see a system 
with x2APIC enabled.

For reference, the chipset of this processor generation which does 
support EIM is the 7500 and it does not suffer from this errata.

Currently Xen is relying on the ACPI_DMAR_X2APIC_OPT_OUT bit in the DMAR 
table to detect x2apic support in the IOMMU.  In theory it would be 
better to read the EIM bit in the IOMMU device itself instead of relying 
on the BIOS but this may be difficult to do at that early stage of 
initialisation of Xen.

Malcolm
>
>> +            break;
>> +        }
>> +    }
>> +}
>> +
>>   /* initialize platform identification flags */
>>   void __init platform_quirks_init(void)
>>   {
>> @@ -264,6 +287,9 @@ void __init platform_quirks_init(void)
>>   
>>       /* ioremap IGD MMIO+0x2000 page */
>>       map_igd_reg();
>> +
>> +    /* Tylersburg interrupt remap quirk */
>> +    tylersburg_intremap_quirk();
>>   }
>>   
>>   /*
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>
>

>>> On 16.01.13 at 22:09, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
> On 16/01/13 13:57, Jan Beulich wrote:
>>>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>>> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for stepping
B-3.
>>> + * Fixed in stepping C-2. */
>>> +void __init tylersburg_intremap_quirk(void)
>>> +{
>>> +    uint32_t bus, device;
>>> +    uint8_t rev;
>>> +
>>> +    for ( bus = 0; bus < 0x100; bus++ )
>>> +    {
>>> +        /* Match on System Management Registers on Device 20
Function 0 */
>>> +        device = pci_conf_read32(0, bus, 20, 0, PCI_VENDOR_ID);
>>> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
>>> +
>>> +        if ( rev == 0x13 && device == 0x342e8086 )
>>> +        {
>>> +            dprintk(XENLOG_INFO VTDPREFIX,
>>> +                    "Disabling Interrupt Remapping due to
Intel 5500/5520/X58 Chipset errata #47, #53\n");
>>> +            iommu_intremap = 0;
>> Unless it is guaranteed that no system with this chipset can have
>> x2APIC, I don''t think this is right. For one, this happens way
too
>> late (namely after x2apic_bsp_setup()). And second, if you move
>> this earlier, such systems with x2APIC pre-enabled won''t boot
>> anymore.
> After some digging, I discovered that the errata affects chipsets 
> (5520,5500,X58) which don''t have IOMMU EIM( Extended Interrupt
Mode)
> support. EIM is required to support x2APIC mode and so this means the 
> chipset can''t support x2APIC mode and so we should never see a
system
> with x2APIC enabled.
Xiantao,

while I realize that you already ack-ed that patch, I''d like this to
be confirmed, as well as the fact that there indeed is on better
workaround known than disabling interrupt remapping.

Malcolm,

I still don''t feel well with this making a small security hole bigger
(the erratum to me mainly means that with lost interrupts we
might observe systems hangs in the worst case, or maybe
data corruption if recovery code in the OSes doesn''t work well).
Whereas with interrupt remapping disabled passthrough
becomes inherently insecure (and the host vulnerable to guest
attacks).

So the question really is whether, rather than disabling interrupt
remapping, disabling the IOMMU as a whole wouldn''t be the
better (read: more secure) workaround.

Jan

On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
> So the question really is whether, rather than disabling interrupt
> remapping, disabling the IOMMU as a whole wouldn''t be the
> better (read: more secure) workaround. 
That, plus providing a command line "I accept the risks" option, is
the
approach we''ve taken in the past I think.

Ian.

>>> On 17.01.13 at 10:01, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
>> So the question really is whether, rather than disabling interrupt
>> remapping, disabling the IOMMU as a whole wouldn''t be the
>> better (read: more secure) workaround. 
> 
> That, plus providing a command line "I accept the risks" option,
is the
> approach we''ve taken in the past I think.
An option for this we already have - "iommu=force".

Jan

On Thu, 2013-01-17 at 09:09 +0000, Jan Beulich wrote:
> >>> On 17.01.13 at 10:01, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
> >> So the question really is whether, rather than disabling interrupt
> >> remapping, disabling the IOMMU as a whole wouldn''t be the
> >> better (read: more secure) workaround. 
> > 
> > That, plus providing a command line "I accept the risks"
option, is the
> > approach we''ve taken in the past I think.
> 
> An option for this we already have - "iommu=force".
I thought that but isn''t it sort of the other way found? iommu=force
will fail to boot if the iommu cannot be enabled, including intremap
support. But perhaps iommu=no-intremap does what we want already?

Ian.

>>> On 17.01.13 at 10:15, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2013-01-17 at 09:09 +0000, Jan Beulich wrote:
>> >>> On 17.01.13 at 10:01, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
>> >> So the question really is whether, rather than disabling
interrupt
>> >> remapping, disabling the IOMMU as a whole wouldn''t be
the
>> >> better (read: more secure) workaround. 
>> > 
>> > That, plus providing a command line "I accept the risks"
option, is the
>> > approach we''ve taken in the past I think.
>> 
>> An option for this we already have - "iommu=force".
> 
> I thought that but isn''t it sort of the other way found?
iommu=force
> will fail to boot if the iommu cannot be enabled, including intremap
> support.
Of course using "iommu=force" when enabling the IOMMU fails
is a bad idea - that will indeed panic. But we''re talking about the
situation where enabling the IOMMU so far worked fine, and the
option is only to be used as an override to us disabling it as
workaround.
> But perhaps iommu=no-intremap does what we want already?
Not really - we want to do the disabling automatically (turning
off both iommu and interrupt remapping), so that to override
this one would need "iommu=force" (enabling the IOMMU, but
not interrupt remapping) or "iommu=force,intremap" (enabling
both - that case needs some code adjustment to work as
described).

Jan

On Thu, 2013-01-17 at 09:33 +0000, Jan Beulich wrote:
> >>> On 17.01.13 at 10:15, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> > On Thu, 2013-01-17 at 09:09 +0000, Jan Beulich wrote:
> >> >>> On 17.01.13 at 10:01, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
> >> > On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
> >> >> So the question really is whether, rather than disabling
interrupt
> >> >> remapping, disabling the IOMMU as a whole
wouldn''t be the
> >> >> better (read: more secure) workaround. 
> >> > 
> >> > That, plus providing a command line "I accept the
risks" option, is the
> >> > approach we''ve taken in the past I think.
> >> 
> >> An option for this we already have - "iommu=force".
> > 
> > I thought that but isn''t it sort of the other way found?
iommu=force
> > will fail to boot if the iommu cannot be enabled, including intremap
> > support.
> 
> Of course using "iommu=force" when enabling the IOMMU fails
> is a bad idea - that will indeed panic. But we''re talking about
the
> situation where enabling the IOMMU so far worked fine, and the
> option is only to be used as an override to us disabling it as
> workaround.
I wasn''t aware that =force had dual meaning like that. It seems
dangerous to me, if I build a (security) product which wants to ensure
that an iommu is always available and therefore add "iommu=force" to
the
command line this will have the side effect of forcibly enabling the
iommu in an unsafe mode on certain systems.

I hope I''ve just misunderstood how this works!
> > But perhaps iommu=no-intremap does what we want already?
> 
> Not really - we want to do the disabling automatically (turning
> off both iommu and interrupt remapping), so that to override
> this one would need "iommu=force" (enabling the IOMMU, but
> not interrupt remapping) or "iommu=force,intremap" (enabling
> both - that case needs some code adjustment to work as
> described).
I agree with the premise here, even if I''m confused by the exact names
of the options needed to override.

Ian.

>>> On 17.01.13 at 10:51, Ian Campbell <Ian.Campbell@citrix.com>
wrote:
> On Thu, 2013-01-17 at 09:33 +0000, Jan Beulich wrote:
>> >>> On 17.01.13 at 10:15, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> > On Thu, 2013-01-17 at 09:09 +0000, Jan Beulich wrote:
>> >> >>> On 17.01.13 at 10:01, Ian Campbell
<Ian.Campbell@citrix.com> wrote:
>> >> > On Thu, 2013-01-17 at 08:49 +0000, Jan Beulich wrote:
>> >> >> So the question really is whether, rather than
disabling interrupt
>> >> >> remapping, disabling the IOMMU as a whole
wouldn''t be the
>> >> >> better (read: more secure) workaround. 
>> >> > 
>> >> > That, plus providing a command line "I accept the
risks" option, is the
>> >> > approach we''ve taken in the past I think.
>> >> 
>> >> An option for this we already have - "iommu=force".
>> > 
>> > I thought that but isn''t it sort of the other way found?
iommu=force
>> > will fail to boot if the iommu cannot be enabled, including
intremap
>> > support.
>> 
>> Of course using "iommu=force" when enabling the IOMMU fails
>> is a bad idea - that will indeed panic. But we''re talking
about the
>> situation where enabling the IOMMU so far worked fine, and the
>> option is only to be used as an override to us disabling it as
>> workaround.
> 
> I wasn''t aware that =force had dual meaning like that.
It''s not that way currently, just how I envisioned Malcolm''s
patch to be changed (don''t set iommu [and iommu_intremap]
to 0 when iommu_force).
> It seems
> dangerous to me, if I build a (security) product which wants to ensure
> that an iommu is always available and therefore add "iommu=force"
to the
> command line this will have the side effect of forcibly enabling the
> iommu in an unsafe mode on certain systems.
In that case we need a new sub-option instead.

Malcolm - are you going to follow up on this?

Jan

On 17/01/13 08:49, Jan Beulich wrote:
>>>> On 16.01.13 at 22:09, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>> On 16/01/13 13:57, Jan Beulich wrote:
>>>>>> On 16.01.13 at 00:27, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
>>>> +/* 5500/5520/X58 Chipset Interrupt remapping errata, for
stepping B-3.
>>>> + * Fixed in stepping C-2. */
>>>> +void __init tylersburg_intremap_quirk(void)
>>>> +{
>>>> +    uint32_t bus, device;
>>>> +    uint8_t rev;
>>>> +
>>>> +    for ( bus = 0; bus < 0x100; bus++ )
>>>> +    {
>>>> +        /* Match on System Management Registers on Device 20
Function 0 */
>>>> +        device = pci_conf_read32(0, bus, 20, 0,
PCI_VENDOR_ID);
>>>> +        rev = pci_conf_read8(0, bus, 20, 0, PCI_REVISION_ID);
>>>> +
>>>> +        if ( rev == 0x13 && device == 0x342e8086 )
>>>> +        {
>>>> +            dprintk(XENLOG_INFO VTDPREFIX,
>>>> +                    "Disabling Interrupt Remapping due to
Intel 5500/5520/X58 Chipset errata #47, #53\n");
>>>> +            iommu_intremap = 0;
>>> Unless it is guaranteed that no system with this chipset can have
>>> x2APIC, I don''t think this is right. For one, this happens
way too
>>> late (namely after x2apic_bsp_setup()). And second, if you move
>>> this earlier, such systems with x2APIC pre-enabled won''t
boot
>>> anymore.
>> After some digging, I discovered that the errata affects chipsets
>> (5520,5500,X58) which don''t have IOMMU EIM( Extended Interrupt
Mode)
>> support. EIM is required to support x2APIC mode and so this means the
>> chipset can''t support x2APIC mode and so we should never see a
system
>> with x2APIC enabled.
> Malcolm,
>
> I still don''t feel well with this making a small security hole
bigger
> (the erratum to me mainly means that with lost interrupts we
> might observe systems hangs in the worst case, or maybe
> data corruption if recovery code in the OSes doesn''t work well).
> Whereas with interrupt remapping disabled passthrough
> becomes inherently insecure (and the host vulnerable to guest
> attacks).
>
> So the question really is whether, rather than disabling interrupt
> remapping, disabling the IOMMU as a whole wouldn''t be the
> better (read: more secure) workaround.
The patch is just achieving the same effect in Xen as the BIOS 
workaround recommended in the Intel errata document.

I also don''t like the making the security hole bigger but I think we
are
making an Xen IOMMU support policy change if we disable the IOMMU 
support when interrupt remapping is not supported.

Additionally, I believe that if we make this policy change then we will 
disable Xen IOMMU support for the Intel 3100/3200 chipset series.



>

>>> On 17.01.13 at 11:02, Malcolm Crossley
<malcolm.crossley@citrix.com> wrote:
> On 17/01/13 08:49, Jan Beulich wrote:
>> I still don''t feel well with this making a small security hole
bigger
>> (the erratum to me mainly means that with lost interrupts we
>> might observe systems hangs in the worst case, or maybe
>> data corruption if recovery code in the OSes doesn''t work
well).
>> Whereas with interrupt remapping disabled passthrough
>> becomes inherently insecure (and the host vulnerable to guest
>> attacks).
>>
>> So the question really is whether, rather than disabling interrupt
>> remapping, disabling the IOMMU as a whole wouldn''t be the
>> better (read: more secure) workaround.
>
> The patch is just achieving the same effect in Xen as the BIOS 
> workaround recommended in the Intel errata document.
But there''s a difference nevertheless. If firmware disables
something, it''s identical to hardware not having the capability.
Whereas if software disables functionality and opens/widens
a security hole by doing so, that''s a flaw that would
subsequently require an advisory+fix on its own.
> I also don''t like the making the security hole bigger but I think
we are
> making an Xen IOMMU support policy change if we disable the IOMMU 
> support when interrupt remapping is not supported.
Why do you think so? The behavior we currently have causes the
system to not boot if "iommu=force", including the case where
interrupt remapping didn''t get enabled but was intended to be (i.e.
no "iommu=no-intremap"). And without "iommu=force" we make
it
impossible to assign devices to HVM guests (i.e. preventing security
issues). Between functionality and security, I think security has to
win by default (but an override ought to be possible).
> Additionally, I believe that if we make this policy change then we will 
> disable Xen IOMMU support for the Intel 3100/3200 chipset series.
Because of what?

Jan