Re: [PATCH 04/15] arm64: kvm: Add build rules for separate nVHE object files

[Marc Zyngier <maz@kernel.org>]

On Thu, 30 Apr 2020 15:48:20 +0100,
David Brazdil <dbrazdil@google.com> wrote:
> 
> Add new folder arch/arm64/kvm/hyp/nvhe and a Makefile for building code that

s/folder/directory/

> runs in EL2 under nVHE KVM.
> 
> Compile each source file into a `.hyp.tmp.o` object first, then prefix all
> its symbols with "__hyp_text_" using `objcopy` and produce a `.hyp.o`.

Madness. ;-)

> Suffixes were chosen so that it would be possible for VHE and nVHE to share
> some source files, but compiled with different CFLAGS. nVHE build rules add
> -D__HYPERVISOR__.
> 
> Macros are added for prefixing a nVHE symbol name when referenced from kernel
> proper.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h | 13 ++++++++++++
>  arch/arm64/kernel/image-vars.h   | 12 +++++++++++
>  arch/arm64/kvm/hyp/Makefile      |  4 ++--
>  arch/arm64/kvm/hyp/nvhe/Makefile | 35 ++++++++++++++++++++++++++++++++
>  scripts/kallsyms.c               |  1 +
>  5 files changed, 63 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm64/kvm/hyp/nvhe/Makefile
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
> index 7c7eeeaab9fa..99ab204519ca 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -42,6 +42,12 @@
>  
>  #include <linux/mm.h>
>  
> +/* Translate the name of @sym to its nVHE equivalent. */
> +#define kvm_nvhe_sym(sym)	__hyp_text_##sym
> +
> +/* Choose between VHE and nVHE variants of a symbol. */
> +#define kvm_hyp_sym(sym)	(has_vhe() ? sym : kvm_nvhe_sym(sym))
> +
>  /* Translate a kernel address of @sym into its equivalent linear mapping */
>  #define kvm_ksym_ref(sym)						\
>  	({								\
> @@ -51,6 +57,13 @@
>  		val;							\
>  	 })
>  
> +/*
> + * Translate a kernel address of @sym into its equivalent linear mapping,
> + * choosing between VHE and nVHE variant of @sym accordingly.
> + */
> +#define kvm_hyp_ref(sym) \
> +	(has_vhe() ? kvm_ksym_ref(sym) : kvm_ksym_ref(kvm_nvhe_sym(sym)))
> +

Things are becoming a bit redundant here: you have a has_vhe() test,
followed by a is_kernel_in_hyp_mode() test in kvm_ksym_ref(). Certainly
this could do with some simplification. Another thing to think about is
whether has_vhe() is the right thing to use as it isn't valid until you
have discovered the capabilities. I think it is fine, but you should
check it.

>  struct kvm;
>  struct kvm_vcpu;
>  
> diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
> index 7f06ad93fc95..13850134fc28 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -51,4 +51,16 @@ __efistub__ctype		= _ctype;
>  
>  #endif
>  
> +#ifdef CONFIG_KVM
> +
> +/*
> + * KVM nVHE code has its own symbol namespace prefixed by __hyp_text_, to
> + * isolate it from the kernel proper. The following symbols are legally
> + * accessed by it, therefore provide aliases to make them linkable.
> + * Do not include symbols which may not be safely accessed under hypervisor
> + * memory mappings.
> + */
> +
> +#endif /* CONFIG_KVM */
> +
>  #endif /* __ARM64_KERNEL_IMAGE_VARS_H */
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index 29e2b2cd2fbc..79bf822a484b 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -6,9 +6,9 @@
>  ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING \
>  		$(DISABLE_STACKLEAK_PLUGIN)
>  
> -obj-$(CONFIG_KVM) += hyp.o
> +obj-$(CONFIG_KVM) += vhe.o nvhe/
>  
> -hyp-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \
> +vhe-y := vgic-v3-sr.o timer-sr.o aarch32.o vgic-v2-cpuif-proxy.o sysreg-sr.o \
>  	 debug-sr.o entry.o switch.o fpsimd.o tlb.o host_hypercall.o hyp-entry.o
>  
>  # KVM code is run at a different exception code with a different map, so
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> new file mode 100644
> index 000000000000..873d3ab0fd68
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -0,0 +1,35 @@
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# Makefile for Kernel-based Virtual Machine module, HYP/nVHE part
> +#
> +
> +asflags-y := -D__HYPERVISOR__
> +ccflags-y := -D__HYPERVISOR__ -fno-stack-protector -DDISABLE_BRANCH_PROFILING \
> +	     $(DISABLE_STACKLEAK_PLUGIN)
> +
> +obj-y :=
> +
> +obj-y := $(patsubst %.o,%.hyp.o,$(obj-y))
> +extra-y := $(patsubst %.hyp.o,%.hyp.tmp.o,$(obj-y))
> +
> +$(obj)/%.hyp.tmp.o: $(src)/%.c FORCE
> +	$(call if_changed_rule,cc_o_c)
> +$(obj)/%.hyp.tmp.o: $(src)/%.S FORCE
> +	$(call if_changed_rule,as_o_S)
> +$(obj)/%.hyp.o: $(obj)/%.hyp.tmp.o FORCE
> +	$(call if_changed,hypcopy)
> +
> +quiet_cmd_hypcopy = HYPCOPY $@
> +      cmd_hypcopy = $(OBJCOPY) --prefix-symbols=__hyp_text_ $< $@
> +
> +# KVM nVHE code is run at a different exception code with a different map, so

s/exception code/exception level/
s/map/memory map/

> +# compiler instrumentation that inserts callbacks or checks into the code may
> +# cause crashes. Just disable it.
> +GCOV_PROFILE	:= n
> +KASAN_SANITIZE	:= n
> +UBSAN_SANITIZE	:= n
> +KCOV_INSTRUMENT	:= n
> +
> +# Skip objtool checking for this directory because nVHE code is compiled with
> +# non-standard build rules.
> +OBJECT_FILES_NON_STANDARD := y
> diff --git a/scripts/kallsyms.c b/scripts/kallsyms.c
> index 3e8dea6e0a95..b5c9dc6de38d 100644
> --- a/scripts/kallsyms.c
> +++ b/scripts/kallsyms.c
> @@ -109,6 +109,7 @@ static bool is_ignored_symbol(const char *name, char type)
>  		".LASANPC",		/* s390 kasan local symbols */
>  		"__crc_",		/* modversions */
>  		"__efistub_",		/* arm64 EFI stub namespace */
> +		"__hyp_text_",		/* arm64 non-VHE KVM namespace */
>  		NULL
>  	};

Could you add a small comment to the commit message as to what this is
for? It certainly piqued my curiosity, and keeping a record of it would
be nice.

>  
> -- 
> 2.26.1
> 
> 

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.