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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <derek@xxxxxxxxxxx>

+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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel