[PATCH] x86/tboot: Use an integer_param for "tboot="

[PATCH] x86/tboot: Use an integer_param for "tboot="

[Andrew Cooper]

rather than using a string param and manually parsing it.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>

---

I also suspect the fixmap entry can be replaced with a vmap(), but without a
tboot setup to hand, I wouldn't be able to test it.
---
 xen/arch/x86/tboot.c |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
index ccde4a0..06a5181 100644
--- a/xen/arch/x86/tboot.c
+++ b/xen/arch/x86/tboot.c
@@ -15,8 +15,8 @@
 #include <crypto/vmac.h>
 
 /* tboot=<physical address of shared page> */
-static char __initdata opt_tboot[20] = "";
-string_param("tboot", opt_tboot);
+static unsigned long __initdata opt_tboot_pa;
+integer_param("tboot", opt_tboot_pa);
 
 /* Global pointer to shared data; NULL means no measured launch. */
 tboot_shared_t *g_tboot_shared;
@@ -93,15 +93,13 @@ static void __init tboot_copy_memory(unsigned char *va, uint32_t size,
 void __init tboot_probe(void)
 {
     tboot_shared_t *tboot_shared;
-    unsigned long p_tboot_shared;
 
     /* Look for valid page-aligned address for shared page. */
-    p_tboot_shared = simple_strtoul(opt_tboot, NULL, 0);
-    if ( (p_tboot_shared == 0) || ((p_tboot_shared & ~PAGE_MASK) != 0) )
+    if ( !opt_tboot_pa || (opt_tboot_pa & ~PAGE_MASK) )
         return;
 
     /* Map and check for tboot UUID. */
-    set_fixmap(FIX_TBOOT_SHARED_BASE, p_tboot_shared);
+    set_fixmap(FIX_TBOOT_SHARED_BASE, opt_tboot_pa);
     tboot_shared = (tboot_shared_t *)fix_to_virt(FIX_TBOOT_SHARED_BASE);
     if ( tboot_shared == NULL )
         return;
@@ -117,7 +115,7 @@ void __init tboot_probe(void)
     }
 
     g_tboot_shared = tboot_shared;
-    printk("TBOOT: found shared page at phys addr %lx:\n", p_tboot_shared);
+    printk("TBOOT: found shared page at phys addr 0x%lx:\n", opt_tboot_pa);
     printk("  version: %d\n", tboot_shared->version);
     printk("  log_addr: %#x\n", tboot_shared->log_addr);
     printk("  shutdown_entry: %#x\n", tboot_shared->shutdown_entry);
-- 
1.7.10.4

Re: [PATCH] x86/tboot: Use an integer_param for "tboot="

[Daniel De Graaf]

On 03/13/2014 12:35 PM, Andrew Cooper wrote:
> rather than using a string param and manually parsing it.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>

While this change looks correct to me, I think you actually need to CC
the TXT maintainers from Intel:

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
F:  xen/arch/x86/tboot.c
F:  xen/include/asm-x86/tboot.h

>
> ---
>
> I also suspect the fixmap entry can be replaced with a vmap(), but without a
> tboot setup to hand, I wouldn't be able to test it.
> ---
>   xen/arch/x86/tboot.c |   12 +++++-------
>   1 file changed, 5 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/tboot.c b/xen/arch/x86/tboot.c
> index ccde4a0..06a5181 100644
> --- a/xen/arch/x86/tboot.c
> +++ b/xen/arch/x86/tboot.c
> @@ -15,8 +15,8 @@
>   #include <crypto/vmac.h>
>
>   /* tboot=<physical address of shared page> */
> -static char __initdata opt_tboot[20] = "";
> -string_param("tboot", opt_tboot);
> +static unsigned long __initdata opt_tboot_pa;
> +integer_param("tboot", opt_tboot_pa);
>
>   /* Global pointer to shared data; NULL means no measured launch. */
>   tboot_shared_t *g_tboot_shared;
> @@ -93,15 +93,13 @@ static void __init tboot_copy_memory(unsigned char *va, uint32_t size,
>   void __init tboot_probe(void)
>   {
>       tboot_shared_t *tboot_shared;
> -    unsigned long p_tboot_shared;
>
>       /* Look for valid page-aligned address for shared page. */
> -    p_tboot_shared = simple_strtoul(opt_tboot, NULL, 0);
> -    if ( (p_tboot_shared == 0) || ((p_tboot_shared & ~PAGE_MASK) != 0) )
> +    if ( !opt_tboot_pa || (opt_tboot_pa & ~PAGE_MASK) )
>           return;
>
>       /* Map and check for tboot UUID. */
> -    set_fixmap(FIX_TBOOT_SHARED_BASE, p_tboot_shared);
> +    set_fixmap(FIX_TBOOT_SHARED_BASE, opt_tboot_pa);
>       tboot_shared = (tboot_shared_t *)fix_to_virt(FIX_TBOOT_SHARED_BASE);
>       if ( tboot_shared == NULL )
>           return;
> @@ -117,7 +115,7 @@ void __init tboot_probe(void)
>       }
>
>       g_tboot_shared = tboot_shared;
> -    printk("TBOOT: found shared page at phys addr %lx:\n", p_tboot_shared);
> +    printk("TBOOT: found shared page at phys addr 0x%lx:\n", opt_tboot_pa);
>       printk("  version: %d\n", tboot_shared->version);
>       printk("  log_addr: %#x\n", tboot_shared->log_addr);
>       printk("  shutdown_entry: %#x\n", tboot_shared->shutdown_entry);
>


-- 
Daniel De Graaf
National Security Agency

Re: [PATCH] x86/tboot: Use an integer_param for "tboot="

[Andrew Cooper]

On 13/03/14 18:20, Daniel De Graaf wrote:
> On 03/13/2014 12:35 PM, Andrew Cooper wrote:
>> rather than using a string param and manually parsing it.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Daniel De Graaf <dgdegra@tycho.nsa.gov>
>
> While this change looks correct to me, I think you actually need to CC
> the TXT maintainers from Intel:

Oh of course - my apologies.  With all the XSM traffic, I just mentally
associated you with tboot itself, and decided to forgo get_maintainers.

~Andrew

Re: [PATCH] x86/tboot: Use an integer_param for "tboot="

[Jan Beulich]

>>> On 13.03.14 at 17:35, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
> @@ -117,7 +115,7 @@ void __init tboot_probe(void)
>      }
>  
>      g_tboot_shared = tboot_shared;
> -    printk("TBOOT: found shared page at phys addr %lx:\n", p_tboot_shared);
> +    printk("TBOOT: found shared page at phys addr 0x%lx:\n", opt_tboot_pa);
>      printk("  version: %d\n", tboot_shared->version);
>      printk("  log_addr: %#x\n", tboot_shared->log_addr);
>      printk("  shutdown_entry: %#x\n", tboot_shared->shutdown_entry);

As can even be seen from the patch context, we standardized on
the shorter "%#lx" a while ago - I fixed this up before committing
(the change was sort of unrelated anyway).

Jan