[PATCH] arm64: kvm: Annotate assembly using modern annoations

[PATCH] arm64: kvm: Annotate assembly using modern annoations

[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 <broonie@kernel.org>
---
 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(-)

diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S
index 160be2b4696d..84f32cf5abc7 100644
--- a/arch/arm64/kvm/hyp-init.S
+++ b/arch/arm64/kvm/hyp-init.S
@@ -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
 
diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
index c0094d520dff..3c79a1124af2 100644
--- a/arch/arm64/kvm/hyp.S
+++ b/arch/arm64/kvm/hyp.S
@@ -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)
diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
index 78ff53225691..5b8ff517ff10 100644
--- a/arch/arm64/kvm/hyp/fpsimd.S
+++ b/arch/arm64/kvm/hyp/fpsimd.S
@@ -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)
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index ffa68d5713f1..f7b0cb189b77 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -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
-- 
2.20.1

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations

[Marc Zyngier]

Hi Mark,

On 2020-02-13 15:38, Mark Brown wrote:
> 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 <broonie@kernel.org>
> ---
>  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(-)
> 
> diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S
> index 160be2b4696d..84f32cf5abc7 100644
> --- a/arch/arm64/kvm/hyp-init.S
> +++ b/arch/arm64/kvm/hyp-init.S
> @@ -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
> 
> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S
> index c0094d520dff..3c79a1124af2 100644
> --- a/arch/arm64/kvm/hyp.S
> +++ b/arch/arm64/kvm/hyp.S
> @@ -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?

>  	hvc	#0
>  	ret
> -ENDPROC(__kvm_call_hyp)
> +SYM_FUNC_END(__kvm_call_hyp)
> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
> index 78ff53225691..5b8ff517ff10 100644
> --- a/arch/arm64/kvm/hyp/fpsimd.S
> +++ b/arch/arm64/kvm/hyp/fpsimd.S
> @@ -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)

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

> diff --git a/arch/arm64/kvm/hyp/hyp-entry.S 
> b/arch/arm64/kvm/hyp/hyp-entry.S
> index ffa68d5713f1..f7b0cb189b77 100644
> --- a/arch/arm64/kvm/hyp/hyp-entry.S
> +++ b/arch/arm64/kvm/hyp/hyp-entry.S
> @@ -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)

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.

>  	.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.

> 
>  	.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

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1057 bytes --]

On Thu, Feb 13, 2020 at 09:36:56PM +0000, Marc Zyngier wrote:
> On 2020-02-13 15:38, Mark Brown wrote:

> > -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.

> >  	.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.

> 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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations

[Marc Zyngier]

On 2020-02-14 11:40, Mark Brown wrote:
> On Thu, Feb 13, 2020 at 09:36:56PM +0000, Marc Zyngier wrote:
>> On 2020-02-13 15:38, Mark Brown wrote:
> 
>> > -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.

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

> 
>> >  	.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).

> 
>> 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.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1635 bytes --]

On Fri, Feb 14, 2020 at 02:19:18PM +0000, Marc Zyngier wrote:
> On 2020-02-14 11:40, Mark Brown wrote:
> > On Thu, Feb 13, 2020 at 09:36:56PM +0000, Marc Zyngier wrote:
> > > On 2020-02-13 15:38, Mark Brown wrote:

> > > >  	.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).

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.

> > > 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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 933 bytes --]

On Fri, Feb 14, 2020 at 02:19:18PM +0000, Marc Zyngier wrote:

> > > >  	.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).

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.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm