diff mbox series

[v3,05/11] kvm: arm64: Remove hyp_adr/ldr_this_cpu

Message ID 20200916173439.32265-6-dbrazdil@google.com (mailing list archive)
State New, archived
Headers show
Series Independent per-CPU data section for nVHE | expand

Commit Message

David Brazdil Sept. 16, 2020, 5:34 p.m. UTC 
The hyp_adr/ldr_this_cpu helpers were introduced for use in hyp code
because they always needed to use TPIDR_EL2 for base, while
adr/ldr_this_cpu from kernel proper would select between TPIDR_EL2 and
_EL1 based on VHE/nVHE.

Simplify this now that the hyp mode case can be handled using the
__KVM_VHE/NVHE_HYPERVISOR__ macros.

Acked-by: Andrew Scull <ascull@google.com>
Signed-off-by: David Brazdil <dbrazdil@google.com>
---
 arch/arm64/include/asm/assembler.h | 27 +++++++++++++++++----------
 arch/arm64/include/asm/kvm_asm.h   | 14 +-------------
 arch/arm64/kvm/hyp/hyp-entry.S     |  2 +-
 3 files changed, 19 insertions(+), 24 deletions(-)

Comments

Will Deacon Sept. 18, 2020, 9:05 a.m. UTC | #1 
On Wed, Sep 16, 2020 at 06:34:33PM +0100, David Brazdil wrote:
> The hyp_adr/ldr_this_cpu helpers were introduced for use in hyp code
> because they always needed to use TPIDR_EL2 for base, while
> adr/ldr_this_cpu from kernel proper would select between TPIDR_EL2 and
> _EL1 based on VHE/nVHE.
> 
> Simplify this now that the hyp mode case can be handled using the
> __KVM_VHE/NVHE_HYPERVISOR__ macros.
> 
> Acked-by: Andrew Scull <ascull@google.com>
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> ---
>  arch/arm64/include/asm/assembler.h | 27 +++++++++++++++++----------
>  arch/arm64/include/asm/kvm_asm.h   | 14 +-------------
>  arch/arm64/kvm/hyp/hyp-entry.S     |  2 +-
>  3 files changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 54d181177656..f79231a0f949 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -218,6 +218,21 @@ lr	.req	x30		// link register
>  	str	\src, [\tmp, :lo12:\sym]
>  	.endm
>  
> +	/*
> +	 * @dst: destination register (32 or 64 bit wide)
> +	 */
> +	.macro	this_cpu_offset, dst
> +#if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
> +	mrs	\dst, tpidr_el2
> +#else
> +alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> +	mrs	\dst, tpidr_el1
> +alternative_else
> +	mrs	\dst, tpidr_el2
> +alternative_endif
> +#endif

Cosmetic, but I think it would be cleaner just to define two variants of the
macro here:

#if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
	.macro  this_cpu_offset, dst
	mrs     \dst, tpidr_el2
	.endm
#else
	.macro  this_cpu_offset, dst
alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
	mrs     \dst, tpidr_el1
alternative_else
	mrs     \dst, tpidr_el2
alternative_endif
	.endm
#endif

(and should we have a shorthand __HYPERVISOR define to avoid the NVHE || VHE
logic?)

With that:

Acked-by: Will Deacon <will@kernel.org>

Will
David Brazdil Sept. 21, 2020, 2:53 p.m. UTC | #2 
Hi Will,

> Cosmetic, but I think it would be cleaner just to define two variants of the
> macro here:
> 
> #if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
> 	.macro  this_cpu_offset, dst
> 	mrs     \dst, tpidr_el2
> 	.endm
> #else
> 	.macro  this_cpu_offset, dst
> alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> 	mrs     \dst, tpidr_el1
> alternative_else
> 	mrs     \dst, tpidr_el2
> alternative_endif
> 	.endm
> #endif

Sure.

> (and should we have a shorthand __HYPERVISOR define to avoid the NVHE || VHE
> logic?)

Happy to add this but let's agree on the details.
 * name: just __HYPERVISOR or __KVM_HYPERVISOR__?
 * defined where? I'm wary of defining it in a header file because then sombody
   will forget to add it and 'ifdef HYP' will be skipped. So I'd put this as
   another '-D__HYPERVISOR' in the build rules. Do you agree?

David
Will Deacon Sept. 21, 2020, 5:15 p.m. UTC | #3 
On Mon, Sep 21, 2020 at 03:53:20PM +0100, David Brazdil wrote:
> > Cosmetic, but I think it would be cleaner just to define two variants of the
> > macro here:
> > 
> > #if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
> > 	.macro  this_cpu_offset, dst
> > 	mrs     \dst, tpidr_el2
> > 	.endm
> > #else
> > 	.macro  this_cpu_offset, dst
> > alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
> > 	mrs     \dst, tpidr_el1
> > alternative_else
> > 	mrs     \dst, tpidr_el2
> > alternative_endif
> > 	.endm
> > #endif
> 
> Sure.

Ta.

> > (and should we have a shorthand __HYPERVISOR define to avoid the NVHE || VHE
> > logic?)
> 
> Happy to add this but let's agree on the details.
>  * name: just __HYPERVISOR or __KVM_HYPERVISOR__?

Hey, I leave the hard decisions to others!

>  * defined where? I'm wary of defining it in a header file because then sombody
>    will forget to add it and 'ifdef HYP' will be skipped. So I'd put this as
>    another '-D__HYPERVISOR' in the build rules. Do you agree?

Hmm, that's a good point. Leave that part as-is for now then and we can
tackle it separately if it comes up again.

Will
diff mbox series

Patch

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 54d181177656..f79231a0f949 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -218,6 +218,21 @@  lr	.req	x30		// link register
 	str	\src, [\tmp, :lo12:\sym]
 	.endm
 
+	/*
+	 * @dst: destination register (32 or 64 bit wide)
+	 */
+	.macro	this_cpu_offset, dst
+#if defined(__KVM_NVHE_HYPERVISOR__) || defined(__KVM_VHE_HYPERVISOR__)
+	mrs	\dst, tpidr_el2
+#else
+alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
+	mrs	\dst, tpidr_el1
+alternative_else
+	mrs	\dst, tpidr_el2
+alternative_endif
+#endif
+	.endm
+
 	/*
 	 * @dst: Result of per_cpu(sym, smp_processor_id()) (can be SP)
 	 * @sym: The name of the per-cpu variable
@@ -226,11 +241,7 @@  lr	.req	x30		// link register
 	.macro adr_this_cpu, dst, sym, tmp
 	adrp	\tmp, \sym
 	add	\dst, \tmp, #:lo12:\sym
-alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
-	mrs	\tmp, tpidr_el1
-alternative_else
-	mrs	\tmp, tpidr_el2
-alternative_endif
+	this_cpu_offset \tmp
 	add	\dst, \dst, \tmp
 	.endm
 
@@ -241,11 +252,7 @@  alternative_endif
 	 */
 	.macro ldr_this_cpu dst, sym, tmp
 	adr_l	\dst, \sym
-alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
-	mrs	\tmp, tpidr_el1
-alternative_else
-	mrs	\tmp, tpidr_el2
-alternative_endif
+	this_cpu_offset \tmp
 	ldr	\dst, [\dst, \tmp]
 	.endm
 
diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index c196eec25498..cf9456663289 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -173,20 +173,8 @@  extern char __smccc_workaround_1_smc[__SMCCC_WORKAROUND_1_SMC_SZ];
 
 #else /* __ASSEMBLY__ */
 
-.macro hyp_adr_this_cpu reg, sym, tmp
-	adr_l	\reg, \sym
-	mrs	\tmp, tpidr_el2
-	add	\reg, \reg, \tmp
-.endm
-
-.macro hyp_ldr_this_cpu reg, sym, tmp
-	adr_l	\reg, \sym
-	mrs	\tmp, tpidr_el2
-	ldr	\reg,  [\reg, \tmp]
-.endm
-
 .macro get_host_ctxt reg, tmp
-	hyp_adr_this_cpu \reg, kvm_host_data, \tmp
+	adr_this_cpu \reg, kvm_host_data, \tmp
 	add	\reg, \reg, #HOST_DATA_CONTEXT
 .endm
 
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 46b4dab933d0..fba91c2ab410 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -132,7 +132,7 @@  alternative_cb_end
 	str	x0, [x2, #VCPU_WORKAROUND_FLAGS]
 
 	/* Check that we actually need to perform the call */
-	hyp_ldr_this_cpu x0, arm64_ssbd_callback_required, x2
+	ldr_this_cpu x0, arm64_ssbd_callback_required, x2
 	cbz	x0, wa2_end
 
 	mov	w0, #ARM_SMCCC_ARCH_WORKAROUND_2