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

Ingo Molnar
2009-08-21T13:51:25Z

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