[PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Kay, Allen M]

[-- Attachment #1: Type: text/plain, Size: 261 bytes --]

Check flags field in ACPI DMAR structure before enabling interrupt remapping or x2apic.  This allows platform vendors to disable interrupt remapping or x2apic features if on board BIOS does not support them.

Signed-off-by: Allen Kay <allen.m.kay@intel.com>

[-- Attachment #2: dmar0329.patch --]
[-- Type: application/octet-stream, Size: 5587 bytes --]

diff -r 9549c04a8384 xen/arch/x86/apic.c
--- a/xen/arch/x86/apic.c	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/arch/x86/apic.c	Tue Mar 29 10:45:37 2011 -0700
@@ -516,7 +516,7 @@ static void resume_x2apic(void)
     mask_8259A();
     mask_IO_APIC_setup(ioapic_entries);
 
-    iommu_enable_IR();
+    iommu_enable_x2apic_IR();
     __enable_x2apic();
 
     restore_IO_APIC_setup(ioapic_entries);
@@ -735,7 +735,7 @@ int lapic_suspend(void)
 
     local_irq_save(flags);
     disable_local_APIC();
-    iommu_disable_IR();
+    iommu_disable_x2apic_IR();
     local_irq_restore(flags);
     return 0;
 }
@@ -958,7 +958,7 @@ void __init x2apic_bsp_setup(void)
     mask_8259A();
     mask_IO_APIC_setup(ioapic_entries);
 
-    if ( iommu_enable_IR() )
+    if ( iommu_enable_x2apic_IR() )
     {
         if ( x2apic_enabled )
             panic("Interrupt remapping could not be enabled while "
diff -r 9549c04a8384 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/drivers/passthrough/vtd/dmar.c	Tue Mar 29 10:45:37 2011 -0700
@@ -47,6 +47,7 @@ static LIST_HEAD_READ_MOSTLY(acpi_rhsa_u
 static LIST_HEAD_READ_MOSTLY(acpi_rhsa_units);
 
 static struct acpi_table_header *__read_mostly dmar_table;
+static int __read_mostly dmar_flags;
 static u64 __read_mostly igd_drhd_address;
 
 static void __init dmar_scope_add_buses(struct dmar_scope *scope, u16 sec_bus,
@@ -761,7 +762,11 @@ out:
 
 int __init acpi_dmar_init(void)
 {
+    struct acpi_table_dmar *dmar;
+
     acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
+    dmar = (struct acpi_table_dmar *) dmar_table;
+    dmar_flags = dmar->flags;
 
     return parse_dmar_table(acpi_parse_dmar);
 }
@@ -781,3 +786,22 @@ void acpi_dmar_zap(void)
     dmar_table->signature[0] = 'X';
     dmar_table->checksum -= 'X'-'D';
 }
+
+int __init platform_supports_intremap(void)
+{
+    unsigned int flags = 0;
+
+    flags = DMAR_INTR_REMAP;
+    return ((dmar_flags & flags) == DMAR_INTR_REMAP);
+}
+
+int __init platform_supports_x2apic(void)
+{
+    unsigned int flags = 0;
+
+    if (!cpu_has_x2apic)
+        return 0;
+
+    flags = DMAR_INTR_REMAP | DMAR_X2APIC_OPT_OUT;
+    return ((dmar_flags & flags) == DMAR_INTR_REMAP);
+}
diff -r 9549c04a8384 xen/drivers/passthrough/vtd/extern.h
--- a/xen/drivers/passthrough/vtd/extern.h	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/drivers/passthrough/vtd/extern.h	Tue Mar 29 10:45:37 2011 -0700
@@ -119,5 +119,7 @@ void vtd_ops_postamble_quirk(struct iomm
 void vtd_ops_postamble_quirk(struct iommu* iommu);
 void me_wifi_quirk(struct domain *domain, u8 bus, u8 devfn, int map);
 void pci_vtd_quirk(struct pci_dev *pdev);
+int __init platform_supports_intremap(void);
+int __init platform_supports_x2apic(void);
 
 #endif // _VTD_EXTERN_H_
diff -r 9549c04a8384 xen/drivers/passthrough/vtd/intremap.c
--- a/xen/drivers/passthrough/vtd/intremap.c	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/drivers/passthrough/vtd/intremap.c	Tue Mar 29 10:45:37 2011 -0700
@@ -735,6 +735,13 @@ int enable_intremap(struct iommu *iommu,
 
     ASSERT(ecap_intr_remap(iommu->ecap) && iommu_intremap);
 
+    if ( !platform_supports_intremap() )
+    {
+        dprintk(XENLOG_ERR VTDPREFIX,
+                "Platform firmware does not support interrupt remapping\n");
+        return -EINVAL;
+    }
+
     ir_ctrl = iommu_ir_ctrl(iommu);
     sts = dmar_readl(iommu->reg, DMAR_GSTS_REG);
 
@@ -821,15 +828,18 @@ out:
 }
 
 /*
- * This function is used to enable Interrutp remapping when
+ * This function is used to enable Interrupt remapping when
  * enable x2apic
  */
-int iommu_enable_IR(void)
+int iommu_enable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
     struct iommu *iommu;
 
     if ( !iommu_supports_eim() )
+        return -1;
+
+    if ( !platform_supports_x2apic() )
         return -1;
 
     for_each_drhd_unit ( drhd )
@@ -881,7 +891,7 @@ int iommu_enable_IR(void)
  * This function is used to disable Interrutp remapping when
  * suspend local apic
  */
-void iommu_disable_IR(void)
+void iommu_disable_x2apic_IR(void)
 {
     struct acpi_drhd_unit *drhd;
 
diff -r 9549c04a8384 xen/drivers/passthrough/vtd/iommu.c
--- a/xen/drivers/passthrough/vtd/iommu.c	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/drivers/passthrough/vtd/iommu.c	Tue Mar 29 10:45:37 2011 -0700
@@ -2014,7 +2014,7 @@ static int init_vtd_hw(void)
             if ( enable_intremap(iommu, 0) != 0 )
             {
                 dprintk(XENLOG_WARNING VTDPREFIX,
-                        "Failed to enable Interrupt Remapping!\n");
+                        "Interrupt Remapping not enabled\n");
                 break;
             }
         }
diff -r 9549c04a8384 xen/drivers/passthrough/vtd/iommu.h
--- a/xen/drivers/passthrough/vtd/iommu.h	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/drivers/passthrough/vtd/iommu.h	Tue Mar 29 10:45:37 2011 -0700
@@ -21,6 +21,10 @@
 #define _INTEL_IOMMU_H_
 
 #include <xen/types.h>
+
+/* DMAR Flags bits */
+#define DMAR_INTR_REMAP 0x1
+#define DMAR_X2APIC_OPT_OUT 0x2
 
 /*
  * Intel IOMMU register specification per version 1.0 public spec.
diff -r 9549c04a8384 xen/include/xen/iommu.h
--- a/xen/include/xen/iommu.h	Mon Mar 28 13:44:08 2011 +0100
+++ b/xen/include/xen/iommu.h	Tue Mar 29 10:45:37 2011 -0700
@@ -63,8 +63,8 @@ struct iommu {
 
 int iommu_setup(void);
 int iommu_supports_eim(void);
-int iommu_enable_IR(void);
-void iommu_disable_IR(void);
+int iommu_enable_x2apic_IR(void);
+void iommu_disable_x2apic_IR(void);
 
 int iommu_add_device(struct pci_dev *pdev);
 int iommu_remove_device(struct pci_dev *pdev);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Jan Beulich]

>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Check flags field in ACPI DMAR structure before enabling interrupt remapping 
> or x2apic.  This allows platform vendors to disable interrupt remapping or 
> x2apic features if on board BIOS does not support them.
> 
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>

Taking a second look (while trying to merge this into our SLE11
code base), I think this

>@@ -761,7 +762,11 @@ out:
> 
> int __init acpi_dmar_init(void)
> {
>+    struct acpi_table_dmar *dmar;
>+
>     acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
>+    dmar = (struct acpi_table_dmar *) dmar_table;
>+    dmar_flags = dmar->flags;
> 
>     return parse_dmar_table(acpi_parse_dmar);
> }

is wrong from a TXT perspective - the table obtained here must
only be used for the signature zapping activity, nothing else. All
decisions must be based upon what gets passed to
acpi_parse_dmar(). (This would at once reduce the size of
the change, since a cast struct acpi_table_dmar * is already
available there.)

Jan

RE: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Kay, Allen M]

Not clear about the functional difference but I have moved the code inside of acpi_parse_dmar() in the latest patch.

Allen

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@novell.com] 
Sent: Tuesday, April 05, 2011 3:42 AM
To: Kay, Allen M
Cc: Cihula, Joseph; xen-devel@lists.xensource.com; Keir Fraser
Subject: Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Check flags field in ACPI DMAR structure before enabling interrupt remapping 
> or x2apic.  This allows platform vendors to disable interrupt remapping or 
> x2apic features if on board BIOS does not support them.
> 
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>

Taking a second look (while trying to merge this into our SLE11
code base), I think this

>@@ -761,7 +762,11 @@ out:
> 
> int __init acpi_dmar_init(void)
> {
>+    struct acpi_table_dmar *dmar;
>+
>     acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
>+    dmar = (struct acpi_table_dmar *) dmar_table;
>+    dmar_flags = dmar->flags;
> 
>     return parse_dmar_table(acpi_parse_dmar);
> }

is wrong from a TXT perspective - the table obtained here must
only be used for the signature zapping activity, nothing else. All
decisions must be based upon what gets passed to
acpi_parse_dmar(). (This would at once reduce the size of
the change, since a cast struct acpi_table_dmar * is already
available there.)

Jan

RE: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Jan Beulich]

>>> On 05.04.11 at 20:13, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Not clear about the functional difference but I have moved the code inside of 
> acpi_parse_dmar() in the latest patch.

The difference is that the copy obtained from tboot code is trusted,
while the raw one (obtained directly from ACPI tables) isn't.

Jan

RE: Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Wei, Gang]

Jan Beulich wrote on 2011-04-05:
>>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>> Check flags field in ACPI DMAR structure before enabling interrupt
>> remapping or x2apic.  This allows platform vendors to disable interrupt
>> remapping or x2apic features if on board BIOS does not support them.
>> 
>> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
> 
> Taking a second look (while trying to merge this into our SLE11 code
> base), I think this
> 
>> @@ -761,7 +762,11 @@ out:
>> 
>> int __init acpi_dmar_init(void)
>> {
>> +    struct acpi_table_dmar *dmar;
>> +
>>     acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
>> +    dmar = (struct acpi_table_dmar *) dmar_table;
>> +    dmar_flags = dmar->flags;
>> 
>>     return parse_dmar_table(acpi_parse_dmar);
>> }
> 
> is wrong from a TXT perspective - the table obtained here must only be
> used for the signature zapping activity, nothing else. All decisions
> must be based upon what gets passed to acpi_parse_dmar(). (This would
> at once reduce the size of the change, since a cast struct
> acpi_table_dmar * is already available there.)
> 
> Jan

You are right. Take TXT into consideration, we should check the dmar table passed to acpi_parse_dmar(), and should not check the dmar_table directly.

Jimmy

Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Jan Beulich]

>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Check flags field in ACPI DMAR structure before enabling interrupt remapping 
> or x2apic.  This allows platform vendors to disable interrupt remapping or 
> x2apic features if on board BIOS does not support them.
> 
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>

Marking platform_supports_intremap() and platform_supports_x2apic()
__init isn't correct - both get called from the resume path.

Jan

RE: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Kay, Allen M]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

Removed __init modifier for platform_supports_intremap() and platform_supports_x2apic().

Signed-off-by: Allen Kay <allen.m.kay@intel.com>

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@novell.com] 
Sent: Tuesday, April 05, 2011 3:53 AM
To: Kay, Allen M
Cc: xen-devel@lists.xensource.com; Keir Fraser
Subject: Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Check flags field in ACPI DMAR structure before enabling interrupt remapping 
> or x2apic.  This allows platform vendors to disable interrupt remapping or 
> x2apic features if on board BIOS does not support them.
> 
> Signed-off-by: Allen Kay <allen.m.kay@intel.com>

Marking platform_supports_intremap() and platform_supports_x2apic()
__init isn't correct - both get called from the resume path.

Jan


[-- Attachment #2: dmar0405.patch --]
[-- Type: application/octet-stream, Size: 1121 bytes --]

diff -r fbfee2a01a91 xen/drivers/passthrough/vtd/dmar.c
--- a/xen/drivers/passthrough/vtd/dmar.c	Tue Apr 05 13:05:05 2011 +0100
+++ b/xen/drivers/passthrough/vtd/dmar.c	Tue Apr 05 18:02:56 2011 -0700
@@ -675,6 +675,7 @@ static int __init acpi_parse_dmar(struct
     int ret = 0;
 
     dmar = (struct acpi_table_dmar *)table;
+    dmar_flags = dmar->flags;
 
     if ( !iommu_enabled )
     {
@@ -762,12 +763,7 @@ out:
 
 int __init acpi_dmar_init(void)
 {
-    struct acpi_table_dmar *dmar;
-
     acpi_get_table(ACPI_SIG_DMAR, 0, &dmar_table);
-    dmar = (struct acpi_table_dmar *) dmar_table;
-    dmar_flags = dmar->flags;
-
     return parse_dmar_table(acpi_parse_dmar);
 }
 
@@ -787,7 +783,7 @@ void acpi_dmar_zap(void)
     dmar_table->checksum -= 'X'-'D';
 }
 
-int __init platform_supports_intremap(void)
+int platform_supports_intremap(void)
 {
     unsigned int flags = 0;
 
@@ -795,7 +791,7 @@ int __init platform_supports_intremap(vo
     return ((dmar_flags & flags) == DMAR_INTR_REMAP);
 }
 
-int __init platform_supports_x2apic(void)
+int platform_supports_x2apic(void)
 {
     unsigned int flags = 0;
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

RE: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Jan Beulich]

>>> On 05.04.11 at 20:10, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
> Removed __init modifier for platform_supports_intremap() and 
> platform_supports_x2apic().

Sorry, but removing the __init from just the definitions isn't sufficient
(normally the declarations shouldn't have these added, but as you
did they'll need to be removed there too).

Jan

> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
> 
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@novell.com] 
> Sent: Tuesday, April 05, 2011 3:53 AM
> To: Kay, Allen M
> Cc: xen-devel@lists.xensource.com; Keir Fraser
> Subject: Re: [PATCH][VTD] check BIOS settings before enabling interrupt 
> remapping or x2apic
> 
>>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>> Check flags field in ACPI DMAR structure before enabling interrupt remapping 
> 
>> or x2apic.  This allows platform vendors to disable interrupt remapping or 
>> x2apic features if on board BIOS does not support them.
>> 
>> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
> 
> Marking platform_supports_intremap() and platform_supports_x2apic()
> __init isn't correct - both get called from the resume path.
> 
> Jan

Re: [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

[Keir Fraser]

On 06/04/2011 07:39, "Jan Beulich" <JBeulich@novell.com> wrote:

>>>> On 05.04.11 at 20:10, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>> Removed __init modifier for platform_supports_intremap() and
>> platform_supports_x2apic().
> 
> Sorry, but removing the __init from just the definitions isn't sufficient
> (normally the declarations shouldn't have these added, but as you
> did they'll need to be removed there too).

I'll fix this and apply the patch.

 Thanks,
 Keir

> Jan
> 
>> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
>> 
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@novell.com]
>> Sent: Tuesday, April 05, 2011 3:53 AM
>> To: Kay, Allen M
>> Cc: xen-devel@lists.xensource.com; Keir Fraser
>> Subject: Re: [PATCH][VTD] check BIOS settings before enabling interrupt
>> remapping or x2apic
>> 
>>>>> On 29.03.11 at 19:58, "Kay, Allen M" <allen.m.kay@intel.com> wrote:
>>> Check flags field in ACPI DMAR structure before enabling interrupt remapping
>> 
>>> or x2apic.  This allows platform vendors to disable interrupt remapping or
>>> x2apic features if on board BIOS does not support them.
>>> 
>>> Signed-off-by: Allen Kay <allen.m.kay@intel.com>
>> 
>> Marking platform_supports_intremap() and platform_supports_x2apic()
>> __init isn't correct - both get called from the resume path.
>> 
>> Jan
> 
> 
>