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@xxxxxxxxxx>
>> ---
>> 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
>>
>> .align
>> hyp_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?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
