Re: [PATCH] txt: fix the build errors on non-X86 platforms

[Ingo Molnar <mingo@elte.hu>]

* Shane Wang <shane.wang@intel.com> wrote:

> This patch moves tboot.h from asm to linux to fix the build errors 
> on non-X86 platforms.
>
> Signed-off-by: Shane Wang <shane.wang@intel.com>
>
>
> diff -r 04f249f00303 arch/x86/include/asm/tboot.h

[ small nit: please always generate diffstat information for your 
  patches as well, so that one can see which files are changed and 
  by how much. ]

This patch looks better, but i have to question why tboot modifies 
generic code at all.

i've attached those generic-code changes below. The init/main.c one 
could sure be done in x86 arch init code, or via an initcall, right? 

Regarding kernel/cpu.c. Tthis code in tboot_wait_for_aps() looks 
suspicious:

int tboot_wait_for_aps(int num_aps)
{
        unsigned long timeout;

        if (!tboot_enabled())
                return 0;

        timeout = jiffies + AP_WAIT_TIMEOUT*HZ;
        while (atomic_read((atomic_t *)&tboot->num_in_wfs) != num_aps &&
               time_before(jiffies, timeout))
                cpu_relax();

        return time_before(jiffies, timeout) ? 0 : 1;
}

the return code looks a bit racy - what if an AP came back just in 
the final moment. It should return whether num_in_wfs == num_aps.

But more importantly, why does this have to be done in generic code 
in kernel/smp.c? Why doesnt the x86 arch level bit of _cpu_down() 
check whether the CPU goes down. (or, if there's no proper 
signalling for that one in the tboot protocol - the _cpu_down() code 
in x86 could call tboot_wait_for_aps() if num_online_cpus() == 1 - 
no need to change generic code here.

Also, as i pointed it out in prior review, this depends on line:

+config INTEL_TXT
+       bool "Enable Intel(R) Trusted Execution Technology (Intel(R) TXT)"
+       depends on EXPERIMENTAL && X86 && DMAR && ACPI

should be turned into the standard:

	depends on HAVE_INTEL_TXT 

line, where arch/x86/Kconfig selects HAVE_INTEL_TXT. As we do it for 
other options as well.

	Ingo

---------------->

 init/main.c  |    3 +++
 kernel/cpu.c |    7 ++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/init/main.c b/init/main.c
index 2d9d6bd..f8e9124 100644
--- a/init/main.c
+++ b/init/main.c
@@ -73,6 +73,7 @@
 #include <asm/io.h>
 #include <asm/bugs.h>
 #include <asm/setup.h>
+#include <asm/tboot.h>
 #include <asm/sections.h>
 #include <asm/cacheflush.h>
 
@@ -715,6 +716,8 @@ asmlinkage void __init start_kernel(void)
 
 	ftrace_init();
 
+	tboot_create_trampoline();
+
 	/* Do the rest non-__init'ed, we're now alive */
 	rest_init();
 }
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 8ce1004..ff071e0 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -14,6 +14,7 @@
 #include <linux/kthread.h>
 #include <linux/stop_machine.h>
 #include <linux/mutex.h>
+#include <asm/tboot.h>
 
 #ifdef CONFIG_SMP
 /* Serializes the updates to cpu_online_mask, cpu_present_mask */
@@ -376,7 +377,7 @@ static cpumask_var_t frozen_cpus;
 
 int disable_nonboot_cpus(void)
 {
-	int cpu, first_cpu, error;
+	int cpu, first_cpu, error, num_cpus = 0;
 
 	error = stop_machine_create();
 	if (error)
@@ -391,6 +392,7 @@ int disable_nonboot_cpus(void)
 	for_each_online_cpu(cpu) {
 		if (cpu == first_cpu)
 			continue;
+		num_cpus++;
 		error = _cpu_down(cpu, 1);
 		if (!error) {
 			cpumask_set_cpu(cpu, frozen_cpus);
@@ -401,6 +403,9 @@ int disable_nonboot_cpus(void)
 			break;
 		}
 	}
+	/* ensure all CPUs have gone into wait-for-SIPI */
+	error |= tboot_wait_for_aps(num_cpus);
+
 	if (!error) {
 		BUG_ON(num_online_cpus() > 1);
 		/* Make sure the CPUs won't be enabled by someone else */