Xen devel - Mar 2011 - [PATCH][VTD] check BIOS settings before enabling interrupt remapping or x2apic

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>


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

>>> 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


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

>>> 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


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

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



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

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


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

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

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

>>> 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



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

>>> 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


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

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
> 
> 
> 


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