Russell, Dave, Will, could you please check below inline, looking for your opinion.
Marc, response is inline.
On 21 January 2014 01:58, Marc Zyngier marc.zyngier@arm.com wrote:
On 21/01/14 01:18, Christoffer Dall wrote:
On Fri, Dec 20, 2013 at 08:48:41AM -0800, Victor Kamensky wrote:
Before fix kvm interrupt.S and interrupt_head.S used push and pop assembler instruction. It causes problem if <asm/assembler.h> file should be include. In assembler.h "push" is defined as macro so it causes compilation errors like this:
"Before fix kvm..." doesn't read very pleasently, consider using something like "Prior to this commit...."
"causes a problem" or "causes problems"
change "if <asm/assembler.h> file should be include..." to "if <asm/assembler.h> is included, because assember.h defines 'push' as a macro..."
arch/arm/kvm/interrupts.S: Assembler messages: arch/arm/kvm/interrupts.S:51: Error: ARM register expected -- `lsr {r2,r3}'
Solution implemented by this patch replaces all 'push {...}' with 'stdmb sp!, {...}' instruction; and all 'pop {...}' with 'ldmia sp!, {...}'.
Signed-off-by: Victor Kamensky victor.kamensky@linaro.org
arch/arm/kvm/interrupts.S | 38 +++++++++++++++++++------------------- arch/arm/kvm/interrupts_head.S | 34 +++++++++++++++++----------------- 2 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S index ddc1553..df19133 100644 --- a/arch/arm/kvm/interrupts.S +++ b/arch/arm/kvm/interrupts.S @@ -47,7 +47,7 @@ __kvm_hyp_code_start:
- instead, ignoring the ipa value.
*/ ENTRY(__kvm_tlb_flush_vmid_ipa)
- push {r2, r3}
stmdb sp!, {r2, r3}
dsb ishst add r0, r0, #KVM_VTTBR
@@ -62,7 +62,7 @@ ENTRY(__kvm_tlb_flush_vmid_ipa) mcrr p15, 6, r2, r3, c2 @ Back to VMID #0 isb @ Not necessary if followed by eret
- pop {r2, r3}
- ldmia sp!, {r2, r3} bx lr
ENDPROC(__kvm_tlb_flush_vmid_ipa)
@@ -110,7 +110,7 @@ ENTRY(__kvm_vcpu_run) #ifdef CONFIG_VFPv3 @ Set FPEXC_EN so the guest doesn't trap floating point instructions VFPFMRX r2, FPEXC @ VMRS
- push {r2}
- stmdb sp!, {r2} orr r2, r2, #FPEXC_EN VFPFMXR FPEXC, r2 @ VMSR
#endif @@ -175,7 +175,7 @@ __kvm_vcpu_return:
after_vfp_restore: @ Restore FPEXC_EN which we clobbered on entry
- pop {r2}
- ldmia sp!, {r2} VFPFMXR FPEXC, r2
#endif
@@ -260,7 +260,7 @@ ENTRY(kvm_call_hyp)
/* Handle undef, svc, pabt, or dabt by crashing with a user notice */ .macro bad_exception exception_code, panic_str
- push {r0-r2}
- stmdb sp!, {r0-r2} mrrc p15, 6, r0, r1, c2 @ Read VTTBR lsr r1, r1, #16 ands r1, r1, #0xff
@@ -338,7 +338,7 @@ hyp_hvc: * Getting here is either becuase of a trap from a guest or from calling * HVC from the host kernel, which means "switch to Hyp mode". */
- push {r0, r1, r2}
stmdb sp!, {r0, r1, r2}
@ Check syndrome register mrc p15, 4, r1, c5, c2, 0 @ HSR
@@ -361,11 +361,11 @@ hyp_hvc: bne guest_trap @ Guest called HVC
host_switch_to_hyp:
- pop {r0, r1, r2}
- ldmia sp!, {r0, r1, r2}
- push {lr}
- stmdb sp!, {lr} mrs lr, SPSR
- push {lr}
stmdb sp!, {lr}
mov lr, r0 mov r0, r1
@@ -375,9 +375,9 @@ host_switch_to_hyp: THUMB( orr lr, #1) blx lr @ Call the HYP function
- pop {lr}
- ldmia sp!, {lr} msr SPSR_csxf, lr
- pop {lr}
- ldmia sp!, {lr} eret
guest_trap: @@ -418,7 +418,7 @@ guest_trap:
/* Preserve PAR */ mrrc p15, 0, r0, r1, c7 @ PAR
- push {r0, r1}
stmdb sp!, {r0, r1}
/* Resolve IPA using the xFAR */ mcr p15, 0, r2, c7, c8, 0 @ ATS1CPR
@@ -431,7 +431,7 @@ guest_trap: orr r2, r2, r1, lsl #24
/* Restore PAR */
- pop {r0, r1}
- ldmia sp!, {r0, r1} mcrr p15, 0, r0, r1, c7 @ PAR
3: load_vcpu @ Load VCPU pointer to r0 @@ -440,10 +440,10 @@ guest_trap: 1: mov r1, #ARM_EXCEPTION_HVC b __kvm_vcpu_return
-4: pop {r0, r1} @ Failed translation, return to guest +4: ldmia sp!, {r0, r1} @ Failed translation, return to guest mcrr p15, 0, r0, r1, c7 @ PAR clrex
- pop {r0, r1, r2}
- ldmia sp!, {r0, r1, r2} eret
/* @@ -455,7 +455,7 @@ guest_trap: #ifdef CONFIG_VFPv3 switch_to_guest_vfp: load_vcpu @ Load VCPU pointer to r0
- push {r3-r7}
stmdb sp!, {r3-r7}
@ NEON/VFP used. Turn on VFP access. set_hcptr vmexit, (HCPTR_TCP(10) | HCPTR_TCP(11))
@@ -467,15 +467,15 @@ switch_to_guest_vfp: add r7, r0, #VCPU_VFP_GUEST restore_vfp_state r7
- pop {r3-r7}
- pop {r0-r2}
- ldmia sp!, {r3-r7}
- ldmia sp!, {r0-r2} clrex eret
#endif
.alignhyp_irq:
- push {r0, r1, r2}
- stmdb sp!, {r0, r1, r2} mov r1, #ARM_EXCEPTION_IRQ load_vcpu @ Load VCPU pointer to r0 b __kvm_vcpu_return
diff --git a/arch/arm/kvm/interrupts_head.S b/arch/arm/kvm/interrupts_head.S index 6f18695..c371db7 100644 --- a/arch/arm/kvm/interrupts_head.S +++ b/arch/arm/kvm/interrupts_head.S @@ -63,7 +63,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mrs r2, SP_\mode mrs r3, LR_\mode mrs r4, SPSR_\mode
- push {r2, r3, r4}
- stmdb sp!, {r2, r3, r4}
.endm
/* @@ -73,13 +73,13 @@ vcpu .req r0 @ vcpu pointer always in r0 .macro save_host_regs /* Hyp regs. Only ELR_hyp (SPSR_hyp already saved) */ mrs r2, ELR_hyp
- push {r2}
stmdb sp!, {r2}
/* usr regs */
- push {r4-r12} @ r0-r3 are always clobbered
- stmdb sp!, {r4-r12} @ r0-r3 are always clobbered mrs r2, SP_usr mov r3, lr
- push {r2, r3}
stmdb sp!, {r2, r3}
push_host_regs_mode svc push_host_regs_mode abt
@@ -95,11 +95,11 @@ vcpu .req r0 @ vcpu pointer always in r0 mrs r7, SP_fiq mrs r8, LR_fiq mrs r9, SPSR_fiq
- push {r2-r9}
- stmdb sp!, {r2-r9}
.endm
.macro pop_host_regs_mode mode
- pop {r2, r3, r4}
- ldmia sp!, {r2, r3, r4} msr SP_\mode, r2 msr LR_\mode, r3 msr SPSR_\mode, r4
@@ -110,7 +110,7 @@ vcpu .req r0 @ vcpu pointer always in r0
- Clobbers all registers, in all modes, except r0 and r1.
*/ .macro restore_host_regs
- pop {r2-r9}
- ldmia sp!, {r2-r9} msr r8_fiq, r2 msr r9_fiq, r3 msr r10_fiq, r4
@@ -125,12 +125,12 @@ vcpu .req r0 @ vcpu pointer always in r0 pop_host_regs_mode abt pop_host_regs_mode svc
- pop {r2, r3}
- ldmia sp!, {r2, r3} msr SP_usr, r2 mov lr, r3
- pop {r4-r12}
- ldmia sp!, {r4-r12}
- pop {r2}
- ldmia sp!, {r2} msr ELR_hyp, r2
.endm
@@ -218,7 +218,7 @@ vcpu .req r0 @ vcpu pointer always in r0 add r2, vcpu, #VCPU_USR_REG(3) stm r2, {r3-r12} add r2, vcpu, #VCPU_USR_REG(0)
- pop {r3, r4, r5} @ r0, r1, r2
- ldmia sp!, {r3, r4, r5} @ r0, r1, r2 stm r2, {r3, r4, r5} mrs r2, SP_usr mov r3, lr
@@ -258,7 +258,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mrc p15, 2, r12, c0, c0, 0 @ CSSELR
.if \store_to_vcpu == 0
- push {r2-r12} @ Push CP15 registers
- stmdb sp!, {r2-r12} @ Push CP15 registers .else str r2, [vcpu, #CP15_OFFSET(c1_SCTLR)] str r3, [vcpu, #CP15_OFFSET(c1_CPACR)]
@@ -286,7 +286,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mrc p15, 0, r12, c12, c0, 0 @ VBAR
.if \store_to_vcpu == 0
- push {r2-r12} @ Push CP15 registers
- stmdb sp!, {r2-r12} @ Push CP15 registers .else str r2, [vcpu, #CP15_OFFSET(c13_CID)] str r3, [vcpu, #CP15_OFFSET(c13_TID_URW)]
@@ -305,7 +305,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mrrc p15, 0, r4, r5, c7 @ PAR
.if \store_to_vcpu == 0
- push {r2,r4-r5}
- stmdb sp!, {r2,r4-r5} .else str r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)] add r12, vcpu, #CP15_OFFSET(c7_PAR)
@@ -322,7 +322,7 @@ vcpu .req r0 @ vcpu pointer always in r0 */ .macro write_cp15_state read_from_vcpu .if \read_from_vcpu == 0
- pop {r2,r4-r5}
- ldmia sp!, {r2,r4-r5} .else ldr r2, [vcpu, #CP15_OFFSET(c14_CNTKCTL)] add r12, vcpu, #CP15_OFFSET(c7_PAR)
@@ -333,7 +333,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mcrr p15, 0, r4, r5, c7 @ PAR
.if \read_from_vcpu == 0
- pop {r2-r12}
- ldmia sp!, {r2-r12} .else ldr r2, [vcpu, #CP15_OFFSET(c13_CID)] ldr r3, [vcpu, #CP15_OFFSET(c13_TID_URW)]
@@ -361,7 +361,7 @@ vcpu .req r0 @ vcpu pointer always in r0 mcr p15, 0, r12, c12, c0, 0 @ VBAR
.if \read_from_vcpu == 0
- pop {r2-r12}
- ldmia sp!, {r2-r12} .else ldr r2, [vcpu, #CP15_OFFSET(c1_SCTLR)] ldr r3, [vcpu, #CP15_OFFSET(c1_CPACR)]
-- 1.8.1.4
If you fix to address Dave's comments, then the code change otherwise looks good.
How about trying this alternative approach:
It looks like all the users of the push/pop macros are located in arch/arm/lib (mostly checksumming code). Can't we move these macros to a separate include file and leave the code that uses push/pop (as defined by the assembler) alone?
Marc, personally I am OK with such proposal. I was considering something along these lines as one of the options. It works for me both ways. If others agree I am happy to recode it as your suggested. I choose proposed above patch because kvm arm code came after push and pop defines were introduced in asm/assembler.h and used in other places. I am OK either way. I agree that use of push and pop as define names seems a bit unfortunate, but I don't have any historic visibility here
Russell, Dave, Will, do you have any opinion on Marc's proposal to fix this issue?
Thanks, Victor
Thanks,
M.-- Jazz is not dead. It just smells funny...