Re: [Xen-devel] [PATCH] x86: Add a tboot Kconfig option

[Jan Beulich]

>>> On 18.08.16 at 14:06,  wrote:
> On 17/08/16 19:17, Derek Straka wrote:
>> Allows for the conditional inclusion of tboot related functionality via 
>> Kconfig
>>
>> The default configuration for the new CONFIG_TBOOT option is 'y', so the
>> behavior out of the box remains unchanged.  The addition of the option allows
>> advanced users to disable system behaviors associated with tboot at compile
>> time rather than relying on the run-time detection and configuration.

You say "advanced users" here, yet ...

>> +config TBOOT
>> +bool "Xen tboot support"
>> +default y
>> +depends on X86

... there's no dependency on EXPERT here. User selectable options
not depending on EXPERT need extra good reasoning on why they
need to be that way.

>> @@ -127,6 +128,17 @@ int tboot_parse_dmar_table(acpi_table_handler 
>> dmar_handler);
>>  int tboot_s3_resume(void);
>>  void tboot_s3_error(int error);
>>  int tboot_wake_ap(int apicid, unsigned long sipi_vec);
>> +#else
>> +static inline void tboot_probe(void) {}
>> +static inline void tboot_shutdown(uint32_t shutdown_type) {}
>> +static inline int tboot_in_measured_env(void) {return 0;}
>> +static inline int tboot_protect_mem_regions(void) {return 1;}
>> +static inline int tboot_parse_dmar_table(acpi_table_handler dmar_handler) 
>> {return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);}
> 
> Please include spaces immediately inside the braces.

And the last one looks to be an overly long line.

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] x86: Add a tboot Kconfig option

[Andrew Cooper]

On 17/08/16 19:17, Derek Straka wrote:
> Allows for the conditional inclusion of tboot related functionality via 
> Kconfig
>
> The default configuration for the new CONFIG_TBOOT option is 'y', so the
> behavior out of the box remains unchanged.  The addition of the option allows
> advanced users to disable system behaviors associated with tboot at compile
> time rather than relying on the run-time detection and configuration.
>
> Signed-off-by: Derek Straka 

+1 for the principle.  Some suggestions however.

> ---
>  xen/Rules.mk|  2 +-
>  xen/arch/x86/Makefile   |  2 +-
>  xen/common/Kconfig  | 11 +++
>  xen/include/asm-x86/tboot.h | 12 
>  4 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/xen/Rules.mk b/xen/Rules.mk
> index ebe1dc0..12d3184 100644
> --- a/xen/Rules.mk
> +++ b/xen/Rules.mk
> @@ -44,7 +44,7 @@ ALL_OBJS-y   += $(BASEDIR)/common/built_in.o
>  ALL_OBJS-y   += $(BASEDIR)/drivers/built_in.o
>  ALL_OBJS-y   += $(BASEDIR)/xsm/built_in.o
>  ALL_OBJS-y   += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
> -ALL_OBJS-$(CONFIG_X86)   += $(BASEDIR)/crypto/built_in.o
> +ALL_OBJS-$(CONFIG_TBOOT)   += $(BASEDIR)/crypto/built_in.o

TBOOT is currently the only consumer, but there are a few other
suggestions on the horizons.

I think it might be better to have a CONFIG_CRYPTO (default to n) which
is selected by CONFIG_TBOOT.  When future work (e.g. hypervisor side
signature checks on hotpatches?) gets completed, it can also select
CONFIG_CRYPTO.

>  
>  CFLAGS += -nostdinc -fno-builtin -fno-common
>  CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index b18f033..5b9e9da 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -62,7 +62,7 @@ obj-y += trace.o
>  obj-y += traps.o
>  obj-y += usercopy.o
>  obj-y += x86_emulate.o
> -obj-y += tboot.o
> +obj-$(CONFIG_TBOOT) += tboot.o
>  obj-y += hpet.o
>  obj-y += vm_event.o
>  obj-y += xstate.o
> diff --git a/xen/common/Kconfig b/xen/common/Kconfig
> index 51afa24..cb9a92a 100644
> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -218,6 +218,17 @@ config SCHED_DEFAULT
>  
>  endmenu
>  
> +# Enable/Disable tboot support

This comment isn't very helpful.  I would simply omit it.  (In fact, I
am going to submit a patch stripping all such comments from the existing
Kconfig files)

> +config TBOOT
> + bool "Xen tboot support"
> + default y
> + depends on X86
> + ---help---
> +   Allows support for Trusted Boot using the Intel(R) Trusted Execution
> +   Technology (TXT)
> +
> +   If unsure, say Y.

This should live in xen/arch/x86/Kconfig, rather than common/Kconfig

> +
>  # Enable/Disable live patching support
>  config LIVEPATCH
>   bool "Live patching support (TECH PREVIEW)"
> diff --git a/xen/include/asm-x86/tboot.h b/xen/include/asm-x86/tboot.h
> index d242862..977e509 100644
> --- a/xen/include/asm-x86/tboot.h
> +++ b/xen/include/asm-x86/tboot.h
> @@ -119,6 +119,7 @@ typedef struct __packed {
>  
>  extern tboot_shared_t *g_tboot_shared;
>  
> +#ifdef CONFIG_TBOOT
>  void tboot_probe(void);
>  void tboot_shutdown(uint32_t shutdown_type);
>  int tboot_in_measured_env(void);
> @@ -127,6 +128,17 @@ int tboot_parse_dmar_table(acpi_table_handler 
> dmar_handler);
>  int tboot_s3_resume(void);
>  void tboot_s3_error(int error);
>  int tboot_wake_ap(int apicid, unsigned long sipi_vec);
> +#else
> +static inline void tboot_probe(void) {}
> +static inline void tboot_shutdown(uint32_t shutdown_type) {}
> +static inline int tboot_in_measured_env(void) {return 0;}
> +static inline int tboot_protect_mem_regions(void) {return 1;}
> +static inline int tboot_parse_dmar_table(acpi_table_handler dmar_handler) 
> {return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);}

Please include spaces immediately inside the braces.

~Andrew

> +static inline int tboot_s3_resume(void) { return 0; }
> +
> +static inline void tboot_s3_error(int error) {}
> +static inline int tboot_wake_ap(int apicid, unsigned long sipi_vec) {return 
> 1;}
> +#endif /* CONFIG_TBOOT */
>  
>  #endif /* __TBOOT_H__ */
>  


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[Xen-devel] [PATCH] x86: Add a tboot Kconfig option

[Derek Straka]

Allows for the conditional inclusion of tboot related functionality via Kconfig

The default configuration for the new CONFIG_TBOOT option is 'y', so the
behavior out of the box remains unchanged.  The addition of the option allows
advanced users to disable system behaviors associated with tboot at compile
time rather than relying on the run-time detection and configuration.

Signed-off-by: Derek Straka 
---
 xen/Rules.mk|  2 +-
 xen/arch/x86/Makefile   |  2 +-
 xen/common/Kconfig  | 11 +++
 xen/include/asm-x86/tboot.h | 12 
 4 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index ebe1dc0..12d3184 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -44,7 +44,7 @@ ALL_OBJS-y   += $(BASEDIR)/common/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/drivers/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/xsm/built_in.o
 ALL_OBJS-y   += $(BASEDIR)/arch/$(TARGET_ARCH)/built_in.o
-ALL_OBJS-$(CONFIG_X86)   += $(BASEDIR)/crypto/built_in.o
+ALL_OBJS-$(CONFIG_TBOOT)   += $(BASEDIR)/crypto/built_in.o
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index b18f033..5b9e9da 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -62,7 +62,7 @@ obj-y += trace.o
 obj-y += traps.o
 obj-y += usercopy.o
 obj-y += x86_emulate.o
-obj-y += tboot.o
+obj-$(CONFIG_TBOOT) += tboot.o
 obj-y += hpet.o
 obj-y += vm_event.o
 obj-y += xstate.o
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 51afa24..cb9a92a 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -218,6 +218,17 @@ config SCHED_DEFAULT
 
 endmenu
 
+# Enable/Disable tboot support
+config TBOOT
+   bool "Xen tboot support"
+   default y
+   depends on X86
+   ---help---
+ Allows support for Trusted Boot using the Intel(R) Trusted Execution
+ Technology (TXT)
+
+ If unsure, say Y.
+
 # Enable/Disable live patching support
 config LIVEPATCH
bool "Live patching support (TECH PREVIEW)"
diff --git a/xen/include/asm-x86/tboot.h b/xen/include/asm-x86/tboot.h
index d242862..977e509 100644
--- a/xen/include/asm-x86/tboot.h
+++ b/xen/include/asm-x86/tboot.h
@@ -119,6 +119,7 @@ typedef struct __packed {
 
 extern tboot_shared_t *g_tboot_shared;
 
+#ifdef CONFIG_TBOOT
 void tboot_probe(void);
 void tboot_shutdown(uint32_t shutdown_type);
 int tboot_in_measured_env(void);
@@ -127,6 +128,17 @@ int tboot_parse_dmar_table(acpi_table_handler 
dmar_handler);
 int tboot_s3_resume(void);
 void tboot_s3_error(int error);
 int tboot_wake_ap(int apicid, unsigned long sipi_vec);
+#else
+static inline void tboot_probe(void) {}
+static inline void tboot_shutdown(uint32_t shutdown_type) {}
+static inline int tboot_in_measured_env(void) {return 0;}
+static inline int tboot_protect_mem_regions(void) {return 1;}
+static inline int tboot_parse_dmar_table(acpi_table_handler dmar_handler) 
{return acpi_table_parse(ACPI_SIG_DMAR, dmar_handler);}
+static inline int tboot_s3_resume(void) { return 0; }
+
+static inline void tboot_s3_error(int error) {}
+static inline int tboot_wake_ap(int apicid, unsigned long sipi_vec) {return 1;}
+#endif /* CONFIG_TBOOT */
 
 #endif /* __TBOOT_H__ */
 
-- 
1.9.1


___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel