Skip to contentSkip to conversation

arm64: kvm: Annotate assembly using modern annoations

PATCH Superseded Mark Brown 2 participants 6 messages Feb 13, 2020 – Feb 14, 2020

Subsystems (2)
arm64 KVM arm64

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Mark Brown

author
#0 lore raw

PATCH 1/1 arm64: kvm: Annotate assembly using modern annoations

In an effort to clarify and simplify the annotation of assembly functions in the kernel new macros have been introduced. These replace ENTRY and ENDPROC with separate annotations for standard C callable functions, data and code with different calling conventions. Update the annotations in the kvm code to the new macros.

Signed-off-by Mark Brown <[email protected]>
---
 arch/arm64/kvm/hyp-init.S      |  8 ++++----
 arch/arm64/kvm/hyp.S           |  4 ++--
 arch/arm64/kvm/hyp/fpsimd.S    |  8 ++++----
 arch/arm64/kvm/hyp/hyp-entry.S | 27 ++++++++++++++++-----------
 4 files changed, 26 insertions(+), 21 deletions(-)
arch/arm64/kvm/hyp-init.S +4 -4
index 160be2b4696d..84f32cf5abc7 100644
@@ -18,7 +18,7 @@
 
 	.align	11
 
-ENTRY(__kvm_hyp_init)
+SYM_CODE_START(__kvm_hyp_init)
 	ventry	__invalid		// Synchronous EL2t
 	ventry	__invalid		// IRQ EL2t
 	ventry	__invalid		// FIQ EL2t

@@ -117,9 +117,9 @@ CPU_BE(	orr	x4, x4, #SCTLR_ELx_EE)
 
 	/* Hello, World! */
 	eret
-ENDPROC(__kvm_hyp_init)
+SYM_CODE_END(__kvm_hyp_init)
 
-ENTRY(__kvm_handle_stub_hvc)
+SYM_CODE_START(__kvm_handle_stub_hvc)
 	cmp	x0, #HVC_SOFT_RESTART
 	b.ne	1f
 

@@ -158,7 +158,7 @@ reset:
 	ldr	x0, =HVC_STUB_ERR
 	eret
 
-ENDPROC(__kvm_handle_stub_hvc)
+SYM_CODE_END(__kvm_handle_stub_hvc)
 
 	.ltorg
arch/arm64/kvm/hyp.S +2 -2
index c0094d520dff..3c79a1124af2 100644
@@ -28,7 +28,7 @@
  * and is used to implement hyp stubs in the same way as in
  * arch/arm64/kernel/hyp_stub.S.
  */
-ENTRY(__kvm_call_hyp)
+SYM_FUNC_START(__kvm_call_hyp)
 	hvc	#0
 	ret
-ENDPROC(__kvm_call_hyp)
+SYM_FUNC_END(__kvm_call_hyp)
arch/arm64/kvm/hyp/fpsimd.S +4 -4
index 78ff53225691..5b8ff517ff10 100644
@@ -11,12 +11,12 @@
 	.text
 	.pushsection	.hyp.text, "ax"
 
-ENTRY(__fpsimd_save_state)
+SYM_FUNC_START(__fpsimd_save_state)
 	fpsimd_save	x0, 1
 	ret
-ENDPROC(__fpsimd_save_state)
+SYM_FUNC_END(__fpsimd_save_state)
 
-ENTRY(__fpsimd_restore_state)
+SYM_FUNC_START(__fpsimd_restore_state)
 	fpsimd_restore	x0, 1
 	ret
-ENDPROC(__fpsimd_restore_state)
+SYM_FUNC_END(__fpsimd_restore_state)
arch/arm64/kvm/hyp/hyp-entry.S +16 -11
index ffa68d5713f1..f7b0cb189b77 100644
@@ -180,7 +180,7 @@ el2_error:
 	eret
 	sb
 
-ENTRY(__hyp_do_panic)
+SYM_FUNC_START(__hyp_do_panic)
 	mov	lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
 		      PSR_MODE_EL1h)
 	msr	spsr_el2, lr

@@ -188,18 +188,19 @@ ENTRY(__hyp_do_panic)
 	msr	elr_el2, lr
 	eret
 	sb
-ENDPROC(__hyp_do_panic)
+SYM_FUNC_END(__hyp_do_panic)
 
-ENTRY(__hyp_panic)
+SYM_CODE_START(__hyp_panic)
 	get_host_ctxt x0, x1
 	b	hyp_panic
-ENDPROC(__hyp_panic)
+SYM_CODE_END(__hyp_panic)
 
 .macro invalid_vector	label, target = __hyp_panic
 	.align	2
+SYM_CODE_START(\label)
 \label:
 	b \target
-ENDPROC(\label)
+SYM_CODE_END(\label)
 .endm
 
 	/* None of these should ever happen */

@@ -246,7 +247,7 @@ check_preamble_length 661b, 662b
 check_preamble_length 661b, 662b
 .endm
 
-ENTRY(__kvm_hyp_vector)
+SYM_CODE_START(__kvm_hyp_vector)
 	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
 	invalid_vect	el2t_irq_invalid	// IRQ EL2t
 	invalid_vect	el2t_fiq_invalid	// FIQ EL2t

@@ -266,7 +267,7 @@ ENTRY(__kvm_hyp_vector)
 	valid_vect	el1_irq			// IRQ 32-bit EL1
 	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
 	valid_vect	el1_error		// Error 32-bit EL1
-ENDPROC(__kvm_hyp_vector)
+SYM_CODE_END(__kvm_hyp_vector)
 
 #ifdef CONFIG_KVM_INDIRECT_VECTORS
 .macro hyp_ventry

@@ -311,15 +312,18 @@ alternative_cb_end
 .endm
 
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)
 	.rept BP_HARDEN_EL2_SLOTS
 	generate_vectors
 	.endr
-ENTRY(__bp_harden_hyp_vecs_end)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_end, SYM_L_GLOBAL)
+SYM_CODE_END(__bp_harden_hyp_vecs)
 
 	.popsection
 
-ENTRY(__smccc_workaround_1_smc_start)
+SYM_CODE_START(__smccc_workaround_1_smc)
+SYM_INNER_LABEL(__smccc_workaround_1_smc_start, SYM_L_GLOBAL)
 	esb
 	sub	sp, sp, #(8 * 4)
 	stp	x2, x3, [sp, #(8 * 0)]

@@ -329,5 +333,6 @@ ENTRY(__smccc_workaround_1_smc_start)
 	ldp	x2, x3, [sp, #(8 * 0)]
 	ldp	x0, x1, [sp, #(8 * 2)]
 	add	sp, sp, #(8 * 4)
-ENTRY(__smccc_workaround_1_smc_end)
+SYM_INNER_LABEL(__smccc_workaround_1_smc_end, SYM_L_GLOBAL)
+SYM_CODE_END(__smccc_workaround_1_smc)
 #endif
Signature · 7 lines
2.20.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
maintainer
#1 lore raw

Hi Mark,

Quoted 53 lines · Mark Brown

In an effort to clarify and simplify the annotation of assembly functions in the kernel new macros have been introduced. These replace ENTRY and ENDPROC with separate annotations for standard C callable functions, data and code with different calling conventions. Update the annotations in the kvm code to the new macros.

Signed-off-by Mark Brown <[email protected]>
---
 arch/arm64/kvm/hyp-init.S      |  8 ++++----
 arch/arm64/kvm/hyp.S           |  4 ++--
 arch/arm64/kvm/hyp/fpsimd.S    |  8 ++++----
 arch/arm64/kvm/hyp/hyp-entry.S | 27 ++++++++++++++++-----------
 4 files changed, 26 insertions(+), 21 deletions(-)
arch/arm64/kvm/hyp-init.S +4 -4
index 160be2b4696d..84f32cf5abc7 100644
@@ -18,7 +18,7 @@
 
 	.align	11
 
-ENTRY(__kvm_hyp_init)
+SYM_CODE_START(__kvm_hyp_init)
 	ventry	__invalid		// Synchronous EL2t
 	ventry	__invalid		// IRQ EL2t
 	ventry	__invalid		// FIQ EL2t

@@ -117,9 +117,9 @@ CPU_BE(	orr	x4, x4, #SCTLR_ELx_EE)
 
 	/* Hello, World! */
 	eret
-ENDPROC(__kvm_hyp_init)
+SYM_CODE_END(__kvm_hyp_init)
 
-ENTRY(__kvm_handle_stub_hvc)
+SYM_CODE_START(__kvm_handle_stub_hvc)
 	cmp	x0, #HVC_SOFT_RESTART
 	b.ne	1f
 

@@ -158,7 +158,7 @@ reset:
 	ldr	x0, =HVC_STUB_ERR
 	eret
 
-ENDPROC(__kvm_handle_stub_hvc)
+SYM_CODE_END(__kvm_handle_stub_hvc)
 
 	.ltorg
arch/arm64/kvm/hyp.S +0 -0
index c0094d520dff..3c79a1124af2 100644

arch/arm64/kvm/hyp.S line 31

Quoted 5 lines
arch/arm64/kvm/hyp.S +1 -1
@@ -28,7 +28,7 @@
  * and is used to implement hyp stubs in the same way as in
  * arch/arm64/kernel/hyp_stub.S.
  */
-ENTRY(__kvm_call_hyp)
+SYM_FUNC_START(__kvm_call_hyp)

I'm not convinced by this particular change. _kvm_call_hyp is called directly from C, and behaves almost like a normal function. What's the issue here?

Quoted 15 lines
arch/arm64/kvm/hyp.S +1 -1
 	hvc	#0
 	ret
-ENDPROC(__kvm_call_hyp)
+SYM_FUNC_END(__kvm_call_hyp)
arch/arm64/kvm/hyp/fpsimd.S +1 -1
index 78ff53225691..5b8ff517ff10 100644
@@ -11,12 +11,12 @@
 	.text
 	.pushsection	.hyp.text, "ax"
 
-ENTRY(__fpsimd_save_state)
+SYM_FUNC_START(__fpsimd_save_state)
 	fpsimd_save	x0, 1

arch/arm64/kvm/hyp/fpsimd.S line 22

Quoted 10 lines
arch/arm64/kvm/hyp/fpsimd.S +3 -3
 	ret
-ENDPROC(__fpsimd_save_state)
+SYM_FUNC_END(__fpsimd_save_state)
 
-ENTRY(__fpsimd_restore_state)
+SYM_FUNC_START(__fpsimd_restore_state)
 	fpsimd_restore	x0, 1
 	ret
-ENDPROC(__fpsimd_restore_state)
+SYM_FUNC_END(__fpsimd_restore_state)

Same for these. The only reason they are not written inline assemply in a normal C function is that we have these fpsimd_* macros.

Quoted 56 lines
arch/arm64/kvm/hyp/hyp-entry.S +8 -7
index ffa68d5713f1..f7b0cb189b77 100644
@@ -180,7 +180,7 @@ el2_error:
 	eret
 	sb
 
-ENTRY(__hyp_do_panic)
+SYM_FUNC_START(__hyp_do_panic)
 	mov	lr, #(PSR_F_BIT | PSR_I_BIT | PSR_A_BIT | PSR_D_BIT |\
 		      PSR_MODE_EL1h)
 	msr	spsr_el2, lr

@@ -188,18 +188,19 @@ ENTRY(__hyp_do_panic)
 	msr	elr_el2, lr
 	eret
 	sb
-ENDPROC(__hyp_do_panic)
+SYM_FUNC_END(__hyp_do_panic)
 
-ENTRY(__hyp_panic)
+SYM_CODE_START(__hyp_panic)
 	get_host_ctxt x0, x1
 	b	hyp_panic
-ENDPROC(__hyp_panic)
+SYM_CODE_END(__hyp_panic)
 
 .macro invalid_vector	label, target = __hyp_panic
 	.align	2
+SYM_CODE_START(\label)
 \label:
 	b \target
-ENDPROC(\label)
+SYM_CODE_END(\label)
 .endm
 
 	/* None of these should ever happen */

@@ -246,7 +247,7 @@ check_preamble_length 661b, 662b
 check_preamble_length 661b, 662b
 .endm
 
-ENTRY(__kvm_hyp_vector)
+SYM_CODE_START(__kvm_hyp_vector)
 	invalid_vect	el2t_sync_invalid	// Synchronous EL2t
 	invalid_vect	el2t_irq_invalid	// IRQ EL2t
 	invalid_vect	el2t_fiq_invalid	// FIQ EL2t

@@ -266,7 +267,7 @@ ENTRY(__kvm_hyp_vector)
 	valid_vect	el1_irq			// IRQ 32-bit EL1
 	invalid_vect	el1_fiq_invalid		// FIQ 32-bit EL1
 	valid_vect	el1_error		// Error 32-bit EL1
-ENDPROC(__kvm_hyp_vector)
+SYM_CODE_END(__kvm_hyp_vector)
 
 #ifdef CONFIG_KVM_INDIRECT_VECTORS
 .macro hyp_ventry

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 316

Quoted 6 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
@@ -311,15 +312,18 @@ alternative_cb_end
 .endm
 
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)

Why isn't SYM_CODE_START_NOALIGN enough? And what is the rational for the _NOALIGN, btw? I'd expect an alignment of 2kB to be more than enough.

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 321

Quoted 6 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.rept BP_HARDEN_EL2_SLOTS
 	generate_vectors
 	.endr
-ENTRY(__bp_harden_hyp_vecs_end)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_end, SYM_L_GLOBAL)
+SYM_CODE_END(__bp_harden_hyp_vecs)

Same here.

Quoted 10 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.popsection
 
-ENTRY(__smccc_workaround_1_smc_start)
+SYM_CODE_START(__smccc_workaround_1_smc)
+SYM_INNER_LABEL(__smccc_workaround_1_smc_start, SYM_L_GLOBAL)
 	esb
 	sub	sp, sp, #(8 * 4)
 	stp	x2, x3, [sp, #(8 * 0)]

arch/arm64/kvm/hyp/hyp-entry.S ENTRY(__smccc_workaround_1_smc_start) · line 338

Quoted 7 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
@@ -329,5 +333,6 @@ ENTRY(__smccc_workaround_1_smc_start)
 	ldp	x2, x3, [sp, #(8 * 0)]
 	ldp	x0, x1, [sp, #(8 * 2)]
 	add	sp, sp, #(8 * 4)
-ENTRY(__smccc_workaround_1_smc_end)
+SYM_INNER_LABEL(__smccc_workaround_1_smc_end, SYM_L_GLOBAL)
+SYM_CODE_END(__smccc_workaround_1_smc)
 #endif

Thanks,

        M.
Signature · 5 lines
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
author
#2 lore raw
Quoted 8 lines · Marc Zyngier

arch/arm64/kvm/hyp.S line 31

Quoted 2 lines · Mark Brown
arch/arm64/kvm/hyp.S +1 -1
-ENTRY(__kvm_call_hyp)
+SYM_FUNC_START(__kvm_call_hyp)

I'm not convinced by this particular change. _kvm_call_hyp is called directly from
C, and behaves almost like a normal function. What's the issue here?

I'm not sure I understand your comment here - this is annotated as SYM_FUNC_ which is the annotation for things that look like normal C functions.

Quoted 6 lines

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 316

Quoted 4 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)

Why isn't SYM_CODE_START_NOALIGN enough? And what is the rational for

The _start and _end labels that were there before are explicitly referenced by code, removing them would break the build.

Quoted 1 line

the _NOALIGN, btw? I'd expect an alignment of 2kB to be more than enough.

So that the explicit .align above takes effect rather than anything the macro decides to do, I'm trying to err on the side of caution here.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

maintainer
#3 lore raw
Quoted 10 lines · Mark Brown
Quoted 8 lines · Marc Zyngier

arch/arm64/kvm/hyp.S line 31

Quoted 2 lines · Mark Brown
arch/arm64/kvm/hyp.S +1 -1
-ENTRY(__kvm_call_hyp)
+SYM_FUNC_START(__kvm_call_hyp)

I'm not convinced by this particular change. _kvm_call_hyp is called directly from
C, and behaves almost like a normal function. What's the issue here?

Quoted 3 lines

I'm not sure I understand your comment here - this is annotated as SYM_FUNC_ which is the annotation for things that look like normal C functions.

You're right, apologies. I got confused between _FUNC_ and _CODE_.

Quoted 10 lines
Quoted 7 lines

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 316

Quoted 5 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)

Why isn't SYM_CODE_START_NOALIGN enough? And what is the rational for

The _start and _end labels that were there before are explicitly referenced by code, removing them would break the build.

But if we're going to clean things up, I'd rather we actually do that. The only time __bp_harden_hyp_vecs_end is used is when computing the size of the vectors, and that'd better be BP_HARDEN_EL2_SLOTS * 2kB (which can be statically asserted at compile time).

Quoted 5 lines
Quoted 2 lines

the _NOALIGN, btw? I'd expect an alignment of 2kB to be more than enough.

So that the explicit .align above takes effect rather than anything the macro decides to do, I'm trying to err on the side of caution here.

I don't think we need this. The macros should do the right thing, and be fixed if they don't.

        M.
Signature · 5 lines
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
author
#4 lore raw
Quoted 11 lines · Marc Zyngier
Quoted 9 lines · Mark Brown
Quoted 8 lines · Marc Zyngier

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 316

Quoted 4 lines · Mark Brown
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)

Why isn't SYM_CODE_START_NOALIGN enough? And what is the rational for

Quoted 7 lines
Quoted 2 lines

The _start and _end labels that were there before are explicitly referenced by code, removing them would break the build.

But if we're going to clean things up, I'd rather we actually do that. The only time __bp_harden_hyp_vecs_end is used is when computing the size of the vectors, and that'd better be BP_HARDEN_EL2_SLOTS * 2kB (which can be statically asserted at compile time).

OK, I'll drop this bit of the patch and send a separate change for this since it's more than a direct substitution.

For reference the underlying goal here is to be able to use SYM_FUNC_START to add BTI annotations to assembly functions for in-kernel BTI, the cleanup is just a byproduct.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

the _NOALIGN, btw? I'd expect an alignment of 2kB to be more than enough.

So that the explicit .align above takes effect rather than anything the macro decides to do, I'm trying to err on the side of caution here.

I don't think we need this. The macros should do the right thing, and be fixed if they don't.

OK, I'll just use a regular annotation - the explicit .align isn't documented so I don't really know what it's supposed to be doing.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

author
#5 lore raw
In reply to Marc Zyngier
Quoted 7 lines · Marc Zyngier
Quoted 6 lines
Quoted 6 lines

arch/arm64/kvm/hyp/hyp-entry.S alternative_cb_end · line 316

Quoted 4 lines
arch/arm64/kvm/hyp/hyp-entry.S +2 -1
 	.align	11
-ENTRY(__bp_harden_hyp_vecs_start)
+SYM_CODE_START_NOALIGN(__bp_harden_hyp_vecs)
+SYM_INNER_LABEL(__bp_harden_hyp_vecs_start, SYM_L_GLOBAL)

Why isn't SYM_CODE_START_NOALIGN enough? And what is the rational for

Quoted 7 lines
Quoted 2 lines

The _start and _end labels that were there before are explicitly referenced by code, removing them would break the build.

But if we're going to clean things up, I'd rather we actually do that. The only time __bp_harden_hyp_vecs_end is used is when computing the size of the vectors, and that'd better be BP_HARDEN_EL2_SLOTS * 2kB (which can be statically asserted at compile time).

I just realized that the same structure is also being used for __smccc_workaround_1_smc which doesn't have such an obviously fixed size - am I missing something with that one? I'll take a look at it anyway.

_______________________________________________
kvmarm mailing list
[email protected]
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm