[PATCH 0/2] x86: tboot adjustments

[PATCH 0/2] x86: tboot adjustments

[Jan Beulich]

1: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use
2: simplify DMAR table copying

Signed-off-by: Jan Beulich <jbeulich@suse.com>

[PATCH 1/2] x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

[Jan Beulich]

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

In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus
running in VMX non-root mode")'s re-use of that fixmap entry to not
cause undesirable (in crash context) cross-CPU TLB flushes, invalidate
the fixmap entry right after use.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -138,6 +138,7 @@ void __init tboot_probe(void)
                       TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
     tboot_copy_memory((unsigned char *)&sinit_size, sizeof(sinit_size),
                       TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE);
+    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
 }
 
 /* definitions from xen/drivers/passthrough/vtd/iommu.h
@@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
     dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
     tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
     dmar_table = (struct acpi_table_header *)dmar_table_raw;
+    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
+
     rc = dmar_handler(dmar_table);
     xfree(dmar_table_raw);
 




[-- Attachment #2: x86-tboot-invalidate.patch --]
[-- Type: text/plain, Size: 1174 bytes --]

x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus
running in VMX non-root mode")'s re-use of that fixmap entry to not
cause undesirable (in crash context) cross-CPU TLB flushes, invalidate
the fixmap entry right after use.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -138,6 +138,7 @@ void __init tboot_probe(void)
                       TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
     tboot_copy_memory((unsigned char *)&sinit_size, sizeof(sinit_size),
                       TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE);
+    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
 }
 
 /* definitions from xen/drivers/passthrough/vtd/iommu.h
@@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
     dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
     tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
     dmar_table = (struct acpi_table_header *)dmar_table_raw;
+    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
+
     rc = dmar_handler(dmar_table);
     xfree(dmar_table_raw);
 

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

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

[PATCH 2/2] x86/tboot: simplify DMAR table copying

[Jan Beulich]

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

There's no need for more than one variable, no need for casts, and no
point in using the type-safe xmalloc_array() here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -435,13 +435,12 @@ int __init tboot_protect_mem_regions(voi
 
 int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler)
 {
-    struct acpi_table_header *dmar_table;
     int rc;
     uint64_t size;
     uint32_t dmar_table_length;
     unsigned long pa;
     sinit_mle_data_t sinit_mle_data;
-    unsigned char *dmar_table_raw;
+    void *dmar_table;
 
     if ( !tboot_in_measured_env() )
         return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);
@@ -474,13 +473,12 @@ int __init tboot_parse_dmar_table(acpi_t
     tboot_copy_memory((unsigned char *)&dmar_table_length,
                       sizeof(dmar_table_length),
                       pa + sizeof(char) * ACPI_NAME_SIZE);
-    dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
-    tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
-    dmar_table = (struct acpi_table_header *)dmar_table_raw;
+    dmar_table = xmalloc_bytes(dmar_table_length);
+    tboot_copy_memory(dmar_table, dmar_table_length, pa);
     __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
 
     rc = dmar_handler(dmar_table);
-    xfree(dmar_table_raw);
+    xfree(dmar_table);
 
     /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */
     /* but dom0 will read real table, so must zap it there too */




[-- Attachment #2: x86-tboot-dmar-table.patch --]
[-- Type: text/plain, Size: 1591 bytes --]

x86/tboot: simplify DMAR table copying

There's no need for more than one variable, no need for casts, and no
point in using the type-safe xmalloc_array() here.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -435,13 +435,12 @@ int __init tboot_protect_mem_regions(voi
 
 int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler)
 {
-    struct acpi_table_header *dmar_table;
     int rc;
     uint64_t size;
     uint32_t dmar_table_length;
     unsigned long pa;
     sinit_mle_data_t sinit_mle_data;
-    unsigned char *dmar_table_raw;
+    void *dmar_table;
 
     if ( !tboot_in_measured_env() )
         return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);
@@ -474,13 +473,12 @@ int __init tboot_parse_dmar_table(acpi_t
     tboot_copy_memory((unsigned char *)&dmar_table_length,
                       sizeof(dmar_table_length),
                       pa + sizeof(char) * ACPI_NAME_SIZE);
-    dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
-    tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
-    dmar_table = (struct acpi_table_header *)dmar_table_raw;
+    dmar_table = xmalloc_bytes(dmar_table_length);
+    tboot_copy_memory(dmar_table, dmar_table_length, pa);
     __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
 
     rc = dmar_handler(dmar_table);
-    xfree(dmar_table_raw);
+    xfree(dmar_table);
 
     /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */
     /* but dom0 will read real table, so must zap it there too */

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

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

Re: [PATCH 1/2] x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

[Andrew Cooper]

On 18/02/15 09:03, Jan Beulich wrote:
> In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus
> running in VMX non-root mode")'s re-use of that fixmap entry to not
> cause undesirable (in crash context) cross-CPU TLB flushes, invalidate
> the fixmap entry right after use.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -138,6 +138,7 @@ void __init tboot_probe(void)
>                        TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
>      tboot_copy_memory((unsigned char *)&sinit_size, sizeof(sinit_size),
>                        TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_SIZE);
> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>  }
>  
>  /* definitions from xen/drivers/passthrough/vtd/iommu.h
> @@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
>      dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
>      tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
>      dmar_table = (struct acpi_table_header *)dmar_table_raw;
> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
> +
>      rc = dmar_handler(dmar_table);
>      xfree(dmar_table_raw);
>  
>
>
>

Re: [PATCH 2/2] x86/tboot: simplify DMAR table copying

[Andrew Cooper]

On 18/02/15 09:03, Jan Beulich wrote:
> There's no need for more than one variable, no need for casts, and no
> point in using the type-safe xmalloc_array() here.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -435,13 +435,12 @@ int __init tboot_protect_mem_regions(voi
>  
>  int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler)
>  {
> -    struct acpi_table_header *dmar_table;
>      int rc;
>      uint64_t size;
>      uint32_t dmar_table_length;
>      unsigned long pa;
>      sinit_mle_data_t sinit_mle_data;
> -    unsigned char *dmar_table_raw;
> +    void *dmar_table;
>  
>      if ( !tboot_in_measured_env() )
>          return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);
> @@ -474,13 +473,12 @@ int __init tboot_parse_dmar_table(acpi_t
>      tboot_copy_memory((unsigned char *)&dmar_table_length,
>                        sizeof(dmar_table_length),
>                        pa + sizeof(char) * ACPI_NAME_SIZE);
> -    dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
> -    tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
> -    dmar_table = (struct acpi_table_header *)dmar_table_raw;
> +    dmar_table = xmalloc_bytes(dmar_table_length);
> +    tboot_copy_memory(dmar_table, dmar_table_length, pa);
>      __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>  
>      rc = dmar_handler(dmar_table);
> -    xfree(dmar_table_raw);
> +    xfree(dmar_table);
>  
>      /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */
>      /* but dom0 will read real table, so must zap it there too */
>
>
>

Ping: [PATCH 0/2] x86: tboot adjustments

[Jan Beulich]

I'm intending to commit the first of these right now (i.e. without an
ack), as we need the bug fixed (and want the fix backported for
the pending stable tree releases). I'll holding off on the 2nd one (as
being cleanup) for another week or so, in the hope that at least one
of you will respond one way or another.

Jan

>>> On 18.02.15 at 09:56, <JBeulich@suse.com> wrote:
> 1: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use
> 2: simplify DMAR table copying
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: Ping: [PATCH 0/2] x86: tboot adjustments

[Jan Beulich]

(re-adding xen-devel to Cc)

>>> On 04.03.15 at 08:59, <joseph.cihula@intel.com> wrote:
> FYI, I no longer work on tboot (haven't for 4+ years).  Jimmy is still Mr. 
> tboot, however ;-)

Thanks for letting us know. I'll send a patch to update
./MAINTAINERS then.

Jan

[PATCH] drop Joseph Cihula from TXT section

[Jan Beulich]

... because of him indicating that he hasn't been working on it for the
past 4+ years.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -180,7 +180,6 @@
 F:	tools/debugger/kdd/
 
 INTEL(R) TRUSTED EXECUTION TECHNOLOGY (TXT)
-M:	Joseph Cihula <joseph.cihula@intel.com>
 M:	Gang Wei <gang.wei@intel.com>
 M:	Shane Wang <shane.wang@intel.com>
 S:	Supported

Re: [PATCH 1/2] x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

[Wei, Gang]

On Feb 18, 2015 17:03, Jan Beulich wrote:
> In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus
> running in VMX non-root mode")'s re-use of that fixmap entry to not
> cause undesirable (in crash context) cross-CPU TLB flushes, invalidate
> the fixmap entry right after use.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -138,6 +138,7 @@ void __init tboot_probe(void)
>                        TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
>                        tboot_copy_memory((unsigned char *)&sinit_size,
>                        sizeof(sinit_size), TXT_PUB_CONFIG_REGS_BASE +
>                        TXTCR_SINIT_SIZE);
> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>  }
>  
>  /* definitions from xen/drivers/passthrough/vtd/iommu.h
> @@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
>      dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
>      tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
>      dmar_table = (struct acpi_table_header *)dmar_table_raw;
> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
> +
>      rc = dmar_handler(dmar_table);
>      xfree(dmar_table_raw);
> 
>

It might be better to move the fixmap invalidations into tboot_copy_memory like below.

diff -r 37706d651593 xen/arch/x86/tboot.c
--- a/xen/arch/x86/tboot.c      Wed Mar 04 10:03:48 2015 +0100
+++ b/xen/arch/x86/tboot.c      Thu Mar 05 12:40:25 2015 +0800
@@ -88,6 +88,8 @@ static void __init tboot_copy_memory(uns
         }
         va[i] = map_addr[pa + i - (map_base << PAGE_SHIFT)];
     }
+
+    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
 }

 void __init tboot_probe(void)

Thanks
Jimmy

Re: [PATCH 2/2] x86/tboot: simplify DMAR table copying

[Wei, Gang]

On Feb 18, 2015 17:03, Jan Beulich wrote:
> There's no need for more than one variable, no need for casts, and no 
> point in using the type-safe xmalloc_array() here.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -435,13 +435,12 @@ int __init tboot_protect_mem_regions(voi
> 
>  int __init tboot_parse_dmar_table(acpi_table_handler dmar_handler)  {
> -    struct acpi_table_header *dmar_table;
>      int rc;
>      uint64_t size;
>      uint32_t dmar_table_length;
>      unsigned long pa;
>      sinit_mle_data_t sinit_mle_data;
> -    unsigned char *dmar_table_raw;
> +    void *dmar_table;
> 
>      if ( !tboot_in_measured_env() )
>          return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler); @@ -
> 474,13 +473,12 @@ int __init tboot_parse_dmar_table(acpi_t
>      tboot_copy_memory((unsigned char *)&dmar_table_length,
>                        sizeof(dmar_table_length),
>                        pa + sizeof(char) * ACPI_NAME_SIZE);
> -    dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
> -    tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
> -    dmar_table = (struct acpi_table_header *)dmar_table_raw;
> +    dmar_table = xmalloc_bytes(dmar_table_length);
> +    tboot_copy_memory(dmar_table, dmar_table_length, pa);
>      __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>      
>      rc = dmar_handler(dmar_table);
> -    xfree(dmar_table_raw);
> +    xfree(dmar_table);
> 
>      /* acpi_parse_dmar() zaps APCI DMAR signature in TXT heap table */
>      /* but dom0 will read real table, so must zap it there too */
>

Ack.

Jimmy

Re: [PATCH 1/2] x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

[Jan Beulich]

>>> On 05.03.15 at 05:45, <gang.wei@intel.com> wrote:
> On Feb 18, 2015 17:03, Jan Beulich wrote:
>> In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus
>> running in VMX non-root mode")'s re-use of that fixmap entry to not
>> cause undesirable (in crash context) cross-CPU TLB flushes, invalidate
>> the fixmap entry right after use.
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/arch/x86/tboot.c
>> +++ b/xen/arch/x86/tboot.c
>> @@ -138,6 +138,7 @@ void __init tboot_probe(void)
>>                        TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
>>                        tboot_copy_memory((unsigned char *)&sinit_size,
>>                        sizeof(sinit_size), TXT_PUB_CONFIG_REGS_BASE +
>>                        TXTCR_SINIT_SIZE);
>> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>>  }
>>  
>>  /* definitions from xen/drivers/passthrough/vtd/iommu.h
>> @@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
>>      dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
>>      tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
>>      dmar_table = (struct acpi_table_header *)dmar_table_raw;
>> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>> +
>>      rc = dmar_handler(dmar_table);
>>      xfree(dmar_table_raw);
>> 
>>
> 
> It might be better to move the fixmap invalidations into tboot_copy_memory 
> like below.

I considered this, but dropped the idea due to resulting in several
redundant invalidations.

Jan

Re: [PATCH 1/2] x86/tboot: invalidate FIX_TBOOT_MAP_ADDRESS mapping after use

[Wei, Gang]

On Mar 5, 2015 15:14, Jan Beulich wrote:
>>>> On 05.03.15 at 05:45, <gang.wei@intel.com> wrote:
>> On Feb 18, 2015 17:03, Jan Beulich wrote:
>>> In order for commit cbeeaa7d ("x86/nmi: fix shootdown of pcpus running
>>> in VMX non-root mode")'s re-use of that fixmap entry to not cause
>>> undesirable (in crash context) cross-CPU TLB flushes, invalidate the
>>> fixmap entry right after use.
>>> 
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> 
>>> --- a/xen/arch/x86/tboot.c
>>> +++ b/xen/arch/x86/tboot.c
>>> @@ -138,6 +138,7 @@ void __init tboot_probe(void)
>>>                        TXT_PUB_CONFIG_REGS_BASE + TXTCR_SINIT_BASE);
>>>                        tboot_copy_memory((unsigned char *)&sinit_size,
>>>                        sizeof(sinit_size), TXT_PUB_CONFIG_REGS_BASE +
>>>                        TXTCR_SINIT_SIZE);
>>> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>>>  }
>>>  
>>>  /* definitions from xen/drivers/passthrough/vtd/iommu.h
>>> @@ -476,6 +477,8 @@ int __init tboot_parse_dmar_table(acpi_t
>>>      dmar_table_raw = xmalloc_array(unsigned char, dmar_table_length);
>>>      tboot_copy_memory(dmar_table_raw, dmar_table_length, pa);
>>>      dmar_table = (struct acpi_table_header *)dmar_table_raw;
>>> +    __set_fixmap(FIX_TBOOT_MAP_ADDRESS, 0, 0);
>>> +
>>>      rc = dmar_handler(dmar_table);
>>>      xfree(dmar_table_raw);
>>> 
>> 
>> It might be better to move the fixmap invalidations into
>> tboot_copy_memory like below.
> 
> I considered this, but dropped the idea due to resulting in several
> redundant invalidations.

>From performance perspective, I am ok with your final changes.

Jimmy