Messages in this thread Patch in this message | | | Date | Fri, 21 Aug 2009 15:50:50 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] txt: fix the build errors on non-X86 platforms |
| |
* 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 */
| |