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@xxxxxxx> 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@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?
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...
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
