Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date:   Thu, 18 Jun 2020 18:17:15 +0100
From:   Marc Zyngier <maz@kernel.org>
To:     David Brazdil <dbrazdil@google.com>
Cc:     Will Deacon <will@kernel.org>,
        Catalin Marinas <catalin.marinas@arm.com>,
        James Morse <james.morse@arm.com>,
        Julien Thierry <julien.thierry.kdev@gmail.com>,
        Suzuki K Poulose <suzuki.poulose@arm.com>,
        kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org, android-kvm@google.com,
        kernel-team@android.com
Subject: Re: [PATCH v3 05/15] arm64: kvm: Build hyp-entry.S separately for
 VHE/nVHE
In-Reply-To: <20200618122537.9625-6-dbrazdil@google.com>
References: <20200618122537.9625-1-dbrazdil@google.com>
 <20200618122537.9625-6-dbrazdil@google.com>
User-Agent: Roundcube Webmail/1.4.5
Message-ID: <5029f8fb4a7816e11de7469c09347c79@kernel.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi David,

On 2020-06-18 13:25, David Brazdil wrote:
> This patch is part of a series which builds KVM's non-VHE hyp code 
> separately
> from VHE and the rest of the kernel.

The above comment doesn't really belong here, and us only fit for the 
cover letter.

> 
> hyp-entry.S contains implementation of KVM hyp vectors. This code is 
> mostly
> shared between VHE/nVHE, therefore compile it under both VHE and nVHE 
> build
> rules. nVHE-specific host HVC handler is hidden behind 
> __KVM_NVHE_HYPERVISOR__.
> 
> Adjust code which selects which KVM hyp vecs to install to choose the 
> correct
> VHE/nVHE symbol.
> 
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/include/asm/kvm_asm.h |  7 ++++++-
>  arch/arm64/include/asm/kvm_mmu.h | 16 ++++++++++------
>  arch/arm64/include/asm/mmu.h     |  7 -------
>  arch/arm64/kernel/cpu_errata.c   |  4 +++-
>  arch/arm64/kernel/image-vars.h   | 12 ++++++++++++
>  arch/arm64/kvm/hyp/hyp-entry.S   |  2 ++
>  arch/arm64/kvm/hyp/nvhe/Makefile |  2 +-
>  arch/arm64/kvm/va_layout.c       |  2 +-
>  8 files changed, 35 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_asm.h 
> b/arch/arm64/include/asm/kvm_asm.h
> index 6a682d66a640..2baa69324cc9 100644
> --- a/arch/arm64/include/asm/kvm_asm.h
> +++ b/arch/arm64/include/asm/kvm_asm.h
> @@ -76,7 +76,12 @@ struct kvm_vcpu;
>  extern char __kvm_hyp_init[];
>  extern char __kvm_hyp_init_end[];
> 
> -extern char __kvm_hyp_vector[];
> +DECLARE_KVM_HYP_SYM(__kvm_hyp_vector);
> +
> +#ifdef CONFIG_KVM_INDIRECT_VECTORS
> +DECLARE_KVM_HYP_SYM(__bp_harden_hyp_vecs);
> +extern atomic_t arm64_el2_vector_last_slot;
> +#endif
> 
>  extern void __kvm_flush_vm_context(void);
>  extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t 
> ipa);
> diff --git a/arch/arm64/include/asm/kvm_mmu.h 
> b/arch/arm64/include/asm/kvm_mmu.h
> index b12bfc1f051a..5bfc7ee61997 100644
> --- a/arch/arm64/include/asm/kvm_mmu.h
> +++ b/arch/arm64/include/asm/kvm_mmu.h
> @@ -461,11 +461,15 @@ extern int __kvm_harden_el2_vector_slot;
>  static inline void *kvm_get_hyp_vector(void)
>  {
>  	struct bp_hardening_data *data = arm64_get_bp_hardening_data();
> -	void *vect = kern_hyp_va(kvm_ksym_ref(__kvm_hyp_vector));
>  	int slot = -1;
> +	void *vect = kern_hyp_va(has_vhe()
> +		? kvm_ksym_ref(__kvm_hyp_vector)
> +		: kvm_ksym_ref_nvhe(__kvm_hyp_vector));

If you are introducing has_vhe() at this stage, then you might as well 
not apply kernel_hyp_va() to the address. This also make the declaration 
block look a bit ugly (yes, I'm a bit of a maniac). I'd rather see 
something like:

diff --git a/arch/arm64/include/asm/kvm_mmu.h 
b/arch/arm64/include/asm/kvm_mmu.h
index 5bfc7ee61997..e915c47744bc 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -457,19 +457,25 @@ static inline int kvm_write_guest_lock(struct kvm 
*kvm, gpa_t gpa,
  extern void *__kvm_bp_vect_base;
  extern int __kvm_harden_el2_vector_slot;

+#define get_hyp_vector_address(v)				\
+({								\
+	void *__v;						\
+	if (has_vhe())						\
+		__v = kvm_ksym_ref(v);				\
+	else							\
+		__v = kern_hyp_va(kvm_ksym_ref_nvhe(v));	\
+	__v;							\
+})
+
  /*  This is called on both VHE and !VHE systems */
  static inline void *kvm_get_hyp_vector(void)
  {
  	struct bp_hardening_data *data = arm64_get_bp_hardening_data();
+	void *vect = get_hyp_vector_address(__kvm_hyp_vector);
  	int slot = -1;
-	void *vect = kern_hyp_va(has_vhe()
-		? kvm_ksym_ref(__kvm_hyp_vector)
-		: kvm_ksym_ref_nvhe(__kvm_hyp_vector));

  	if (cpus_have_const_cap(ARM64_HARDEN_BRANCH_PREDICTOR) && data->fn) {
-		vect = kern_hyp_va(has_vhe()
-			? kvm_ksym_ref(__bp_harden_hyp_vecs)
-			: kvm_ksym_ref_nvhe(__bp_harden_hyp_vecs));
+		vect = get_hyp_vector_address(__bp_harden_hyp_vecs);
  		slot = data->hyp_vectors_slot;
  	}

> 
>  	if (cpus_have_const_cap(ARM64_HARDEN_BRANCH_PREDICTOR) && data->fn) {
> -		vect = kern_hyp_va(kvm_ksym_ref(__bp_harden_hyp_vecs));
> +		vect = kern_hyp_va(has_vhe()
> +			? kvm_ksym_ref(__bp_harden_hyp_vecs)
> +			: kvm_ksym_ref_nvhe(__bp_harden_hyp_vecs));
>  		slot = data->hyp_vectors_slot;
>  	}
> 
> @@ -494,12 +498,11 @@ static inline int kvm_map_vectors(void)
>  	 *  HBP +  HEL2 -> use hardened vertors and use exec mapping
>  	 */
>  	if (cpus_have_const_cap(ARM64_HARDEN_BRANCH_PREDICTOR)) {
> -		__kvm_bp_vect_base = kvm_ksym_ref(__bp_harden_hyp_vecs);
> -		__kvm_bp_vect_base = kern_hyp_va(__kvm_bp_vect_base);
> +		__kvm_bp_vect_base = 
> kern_hyp_va(kvm_ksym_ref_nvhe(__bp_harden_hyp_vecs));
>  	}
> 
>  	if (cpus_have_const_cap(ARM64_HARDEN_EL2_VECTORS)) {
> -		phys_addr_t vect_pa = __pa_symbol(__bp_harden_hyp_vecs);
> +		phys_addr_t vect_pa = 
> __pa_symbol(kvm_nvhe_sym(__bp_harden_hyp_vecs));
>  		unsigned long size = __BP_HARDEN_HYP_VECS_SZ;
> 
>  		/*
> @@ -518,7 +521,8 @@ static inline int kvm_map_vectors(void)
>  #else
>  static inline void *kvm_get_hyp_vector(void)
>  {
> -	return kern_hyp_va(kvm_ksym_ref(__kvm_hyp_vector));
> +	return kern_hyp_va(has_vhe() ? kvm_ksym_ref(__kvm_hyp_vector)
> +				     : kvm_ksym_ref_nvhe(__kvm_hyp_vector));
>  }
> 
>  static inline int kvm_map_vectors(void)
> diff --git a/arch/arm64/include/asm/mmu.h 
> b/arch/arm64/include/asm/mmu.h
> index 68140fdd89d6..4d913f6dd366 100644
> --- a/arch/arm64/include/asm/mmu.h
> +++ b/arch/arm64/include/asm/mmu.h
> @@ -42,13 +42,6 @@ struct bp_hardening_data {
>  	bp_hardening_cb_t	fn;
>  };
> 
> -#if (defined(CONFIG_HARDEN_BRANCH_PREDICTOR) ||	\
> -     defined(CONFIG_HARDEN_EL2_VECTORS))
> -
> -extern char __bp_harden_hyp_vecs[];
> -extern atomic_t arm64_el2_vector_last_slot;
> -#endif  /* CONFIG_HARDEN_BRANCH_PREDICTOR || CONFIG_HARDEN_EL2_VECTORS 
> */
> -
>  #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
>  DECLARE_PER_CPU_READ_MOSTLY(struct bp_hardening_data, 
> bp_hardening_data);
> 
> diff --git a/arch/arm64/kernel/cpu_errata.c 
> b/arch/arm64/kernel/cpu_errata.c
> index ad06d6802d2e..318b76a62c56 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -117,7 +117,9 @@ DEFINE_PER_CPU_READ_MOSTLY(struct
> bp_hardening_data, bp_hardening_data);
>  static void __copy_hyp_vect_bpi(int slot, const char *hyp_vecs_start,
>  				const char *hyp_vecs_end)
>  {
> -	void *dst = lm_alias(__bp_harden_hyp_vecs + slot * SZ_2K);
> +	char *vec = has_vhe() ? __bp_harden_hyp_vecs
> +			      : kvm_nvhe_sym(__bp_harden_hyp_vecs);

If we get this construct often, then something that abstracts
the uggliness of the symbol duality would be nice...

> +	void *dst = lm_alias(vec + slot * SZ_2K);
>  	int i;
> 
>  	for (i = 0; i < SZ_2K; i += 0x80)
> diff --git a/arch/arm64/kernel/image-vars.h 
> b/arch/arm64/kernel/image-vars.h
> index 89affa38b143..dc7ee85531f5 100644
> --- a/arch/arm64/kernel/image-vars.h
> +++ b/arch/arm64/kernel/image-vars.h
> @@ -61,9 +61,11 @@ __efistub__ctype		= _ctype;
>   * memory mappings.
>   */
> 
> +__kvm_nvhe___guest_exit = __guest_exit;
>  __kvm_nvhe___kvm_enable_ssbs = __kvm_enable_ssbs;
>  __kvm_nvhe___kvm_flush_vm_context = __kvm_flush_vm_context;
>  __kvm_nvhe___kvm_get_mdcr_el2 = __kvm_get_mdcr_el2;
> +__kvm_nvhe___kvm_handle_stub_hvc = __kvm_handle_stub_hvc;
>  __kvm_nvhe___kvm_timer_set_cntvoff = __kvm_timer_set_cntvoff;
>  __kvm_nvhe___kvm_tlb_flush_local_vmid = __kvm_tlb_flush_local_vmid;
>  __kvm_nvhe___kvm_tlb_flush_vmid = __kvm_tlb_flush_vmid;
> @@ -75,6 +77,16 @@ __kvm_nvhe___vgic_v3_read_vmcr = 
> __vgic_v3_read_vmcr;
>  __kvm_nvhe___vgic_v3_restore_aprs = __vgic_v3_restore_aprs;
>  __kvm_nvhe___vgic_v3_save_aprs = __vgic_v3_save_aprs;
>  __kvm_nvhe___vgic_v3_write_vmcr = __vgic_v3_write_vmcr;
> +__kvm_nvhe_abort_guest_exit_end = abort_guest_exit_end;
> +__kvm_nvhe_abort_guest_exit_start = abort_guest_exit_start;
> +__kvm_nvhe_arm64_enable_wa2_handling = arm64_enable_wa2_handling;
> +__kvm_nvhe_arm64_ssbd_callback_required = 
> arm64_ssbd_callback_required;
> +__kvm_nvhe_hyp_panic = hyp_panic;
> +__kvm_nvhe_kimage_voffset = kimage_voffset;
> +__kvm_nvhe_kvm_host_data = kvm_host_data;
> +__kvm_nvhe_kvm_patch_vector_branch = kvm_patch_vector_branch;
> +__kvm_nvhe_kvm_update_va_mask = kvm_update_va_mask;
> +__kvm_nvhe_panic = panic;
> 
>  #endif /* CONFIG_KVM */
> 
> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S 
> b/arch/arm64/kvm/hyp/hyp-entry.S
> index d362fad97cc8..7e3c72fa634f 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -40,6 +40,7 @@ el1_sync:				// Guest trapped into EL2
>  	ccmp	x0, #ESR_ELx_EC_HVC32, #4, ne
>  	b.ne	el1_trap
> 
> +#ifdef __KVM_NVHE_HYPERVISOR__
>  	mrs	x1, vttbr_el2		// If vttbr is valid, the guest
>  	cbnz	x1, el1_hvc_guest	// called HVC
> 
> @@ -74,6 +75,7 @@ el1_sync:				// Guest trapped into EL2
> 
>  	eret
>  	sb
> +#endif /* __KVM_NVHE_HYPERVISOR__ */
> 
>  el1_hvc_guest:
>  	/*
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile 
> b/arch/arm64/kvm/hyp/nvhe/Makefile
> index 7d64235dba62..c68801e24950 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -7,7 +7,7 @@ asflags-y := -D__KVM_NVHE_HYPERVISOR__
>  ccflags-y := -D__KVM_NVHE_HYPERVISOR__ -fno-stack-protector \
>  	     -DDISABLE_BRANCH_PROFILING $(DISABLE_STACKLEAK_PLUGIN)
> 
> -obj-y :=
> +obj-y := ../hyp-entry.o
> 
>  obj-y := $(patsubst %.o,%.hyp.o,$(obj-y))
>  extra-y := $(patsubst %.hyp.o,%.hyp.tmp.o,$(obj-y))
> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> index a4f48c1ac28c..157d106235f7 100644
> --- a/arch/arm64/kvm/va_layout.c
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -150,7 +150,7 @@ void kvm_patch_vector_branch(struct alt_instr *alt,
>  	/*
>  	 * Compute HYP VA by using the same computation as kern_hyp_va()
>  	 */
> -	addr = (uintptr_t)kvm_ksym_ref(__kvm_hyp_vector);
> +	addr = (uintptr_t)kvm_ksym_ref_nvhe(__kvm_hyp_vector);
>  	addr &= va_mask;
>  	addr |= tag_val << tag_lsb;

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...