Re: [PATCH] intel_txt: fix the build errors of intel_txt patch on non-X86 platforms

[Ingo Molnar <mingo@elte.hu>]

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

> Hi
>
> Forget the previous patch. I misundertood Andi's comments. It should be 
> this one. Please comment.
>
> Thanks.
> Shane
>
>
> ---
>  arch/x86/Kconfig              |    4 +++
>  drivers/acpi/acpica/hwsleep.c |    2 -
>  drivers/pci/dmar.c            |    2 -
>  drivers/pci/intel-iommu.c     |    2 -
>  include/linux/tboot.h         |   35 ++++++++++++++++++++++++++++++++
>  init/main.c                   |    2 -
>  kernel/cpu.c                  |    2 -
>  security/Kconfig              |    2 -
>  8 files changed, 45 insertions(+), 6 deletions(-)
>
> Signed-off-by: Shane Wang <shane.wang@intel.com>
>
>
> diff -r e5406357eaf2 arch/x86/Kconfig
> --- a/arch/x86/Kconfig	Thu Aug 20 21:10:50 2009 -0700
> +++ b/arch/x86/Kconfig	Thu Aug 20 21:15:32 2009 -0700
> @@ -179,6 +179,10 @@ config ARCH_SUPPORTS_OPTIMIZED_INLINING
>
>  config ARCH_SUPPORTS_DEBUG_PAGEALLOC
>  	def_bool y
> +
> +config ARCH_HAS_INTEL_TXT
> +	def_bool y
> +	depends on EXPERIMENTAL && DMAR && ACPI
>
>  # Use the generic interrupt handling code in kernel/irq/:
>  config GENERIC_HARDIRQS
> diff -r e5406357eaf2 drivers/acpi/acpica/hwsleep.c
> --- a/drivers/acpi/acpica/hwsleep.c	Thu Aug 20 21:10:50 2009 -0700
> +++ b/drivers/acpi/acpica/hwsleep.c	Thu Aug 20 21:15:32 2009 -0700
> @@ -45,7 +45,7 @@
>  #include <acpi/acpi.h>
>  #include "accommon.h"
>  #include "actables.h"
> -#include <asm/tboot.h>
> +#include <linux/tboot.h>
>
>  #define _COMPONENT          ACPI_HARDWARE
>  ACPI_MODULE_NAME("hwsleep")
> diff -r e5406357eaf2 drivers/pci/dmar.c
> --- a/drivers/pci/dmar.c	Thu Aug 20 21:10:50 2009 -0700
> +++ b/drivers/pci/dmar.c	Thu Aug 20 21:15:32 2009 -0700
> @@ -33,7 +33,7 @@
>  #include <linux/timer.h>
>  #include <linux/irq.h>
>  #include <linux/interrupt.h>
> -#include <asm/tboot.h>
> +#include <linux/tboot.h>
>
>  #undef PREFIX
>  #define PREFIX "DMAR:"
> diff -r e5406357eaf2 drivers/pci/intel-iommu.c
> --- a/drivers/pci/intel-iommu.c	Thu Aug 20 21:10:50 2009 -0700
> +++ b/drivers/pci/intel-iommu.c	Thu Aug 20 21:15:32 2009 -0700
> @@ -37,8 +37,8 @@
>  #include <linux/iommu.h>
>  #include <linux/intel-iommu.h>
>  #include <linux/sysdev.h>
> +#include <linux/tboot.h>
>  #include <asm/cacheflush.h>
> -#include <asm/tboot.h>
>  #include <asm/iommu.h>
>  #include "pci.h"
>
> diff -r e5406357eaf2 include/linux/tboot.h
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/include/linux/tboot.h	Thu Aug 20 21:15:32 2009 -0700
> @@ -0,0 +1,35 @@
> +/*
> + * Copyright (c) 2006-2009, Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +#ifndef _LINUX_TBOOT_H
> +#define _LINUX_TBOOT_H
> +
> +#ifdef CONFIG_ARCH_HAS_INTEL_TXT
> +#include <asm/tboot.h>
> +#else
> +
> +#define tboot_sleep(sleep_state, pm1a_control, pm1b_control)	\
> +					do { } while (0)
> +#define tboot_get_dmar_table(dmar_tbl)	(dmar_tbl)
> +#define tboot_force_iommu()		0
> +#define tboot_create_trampoline()	do { } while (0)
> +#define tboot_wait_for_aps(num_aps)	0
> +
> +#endif /* !CONFIG_ARCH_HAS_INTEL_TXT */

If then these should be dummy inlines - but in any case, do you 
expect tboot to be used by non-x86 too?

> +#endif /* _LINUX_TBOOT_H */
> diff -r e5406357eaf2 init/main.c
> --- a/init/main.c	Thu Aug 20 21:10:50 2009 -0700
> +++ b/init/main.c	Thu Aug 20 21:15:32 2009 -0700
> @@ -68,12 +68,12 @@
>  #include <linux/async.h>
>  #include <linux/kmemcheck.h>
>  #include <linux/kmemtrace.h>
> +#include <linux/tboot.h>
>  #include <trace/boot.h>
>
>  #include <asm/io.h>
>  #include <asm/bugs.h>
>  #include <asm/setup.h>
> -#include <asm/tboot.h>
>  #include <asm/sections.h>
>  #include <asm/cacheflush.h>
>
> diff -r e5406357eaf2 kernel/cpu.c
> --- a/kernel/cpu.c	Thu Aug 20 21:10:50 2009 -0700
> +++ b/kernel/cpu.c	Thu Aug 20 21:15:32 2009 -0700
> @@ -14,7 +14,7 @@
>  #include <linux/kthread.h>
>  #include <linux/stop_machine.h>
>  #include <linux/mutex.h>
> -#include <asm/tboot.h>
> +#include <linux/tboot.h>
>
>  #ifdef CONFIG_SMP
>  /* Serializes the updates to cpu_online_mask, cpu_present_mask */
> diff -r e5406357eaf2 security/Kconfig
> --- a/security/Kconfig	Thu Aug 20 21:10:50 2009 -0700
> +++ b/security/Kconfig	Thu Aug 20 21:15:32 2009 -0700
> @@ -131,7 +131,7 @@ config LSM_MMAP_MIN_ADDR
>
>  config INTEL_TXT
>  	bool "Enable Intel(R) Trusted Execution Technology (Intel(R) TXT)"
> -	depends on EXPERIMENTAL && X86 && DMAR && ACPI
> +	depends on ARCH_HAS_INTEL_TXT

Nit: what i suggested was HAVE_INTEL_TXT - that is the standard 
kbuild solution these days. (ARCH_HAS_* is legacy-ish)

>> 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?
>
> As long as the page table is set up and the memory is initialized, 
> since the tboot code is only to set up 1:1 mapping page table for 
> later use. Do you mean setup_arch() in setup.c? I will try.

yes, something like that. (i have not checked the init dependencies, 
so other places might be more suitable - but setup_arch() definitely 
sounds good. You obviously want to unlock your hardware ASAP.)

>> 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.
>
> Yes;-) "return num_in_wfs == num_aps ? 1 : 0" should be better, right?

better yes - cleaner, not ;-)

jiffies loops are dubious anyway - what if this code ever gets 
called from an irqs-off section? If you want to break out then 
please use mdelay based loops and print a warning message if the 
code has to break out after the timeout and the APs have not 
hand-shaken with us properly.

>> 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.
>>
> Which c file do you mentioned about _cpu_down()? I only can find 
> _cpu_down() in kernel/cpu.c.

_cpu_down() is a thin wrapper and there's architecture code that 
does the real thing: __cpu_disable() / smp_ops.cpu_disable() on x86. 

Plus there's a CPU notifier chain action as well you can register to 
via the kernel/cpu.c APIs, and provide a CPU_DYING handler.

(I'd argue for the latter in this case, it's more generic and can 
stay local to the tboot code.)

	Ingo