Re: [PATCH] arm64: kvm: Annotate assembly using modern annoations
On Fri, Feb 14, 2020 at 02:19:18PM +, 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. signature.asc Description: PGP signature ___ 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
On Fri, Feb 14, 2020 at 02:19:18PM +, Marc Zyngier wrote: > On 2020-02-14 11:40, Mark Brown wrote: > > On Thu, Feb 13, 2020 at 09:36:56PM +, 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. signature.asc Description: PGP signature ___ 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
On 2020-02-14 11:40, Mark Brown wrote: On Thu, Feb 13, 2020 at 09:36:56PM +, 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
On Thu, Feb 13, 2020 at 09:36:56PM +, 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. signature.asc Description: PGP signature ___ 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
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 --- 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.ne1f @@ -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_vectel2t_sync_invalid // Synchronous EL2t invalid_vectel2t_irq_invalid// IRQ EL2t invalid_vectel2t_fiq_invalid// FIQ EL2t @@ -266,7 +267,7 @@ ENTRY(__kvm_hyp_vector) valid_vect el1_irq // IRQ 32-bit EL1 invalid_vectel1_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_
