Re: [PATCH v2 14/28] ARM: KVM: Add guest entry code

[Marc Zyngier <marc.zyngier@arm.com>]

On 09/02/16 18:44, Christoffer Dall wrote:
> On Thu, Feb 04, 2016 at 11:00:31AM +0000, Marc Zyngier wrote:
>> Add the very minimal piece of code that is now required to jump
>> into the guest (and return from it). This code is only concerned
>> with save/restoring the USR registers (r0-r12+lr for the guest,
>> r4-r12+lr for the host), as everything else is dealt with in C
>> (VFP is another matter though).
>>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm/kvm/hyp/Makefile |  1 +
>>  arch/arm/kvm/hyp/entry.S  | 70 +++++++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm/kvm/hyp/hyp.h    |  2 ++
>>  3 files changed, 73 insertions(+)
>>  create mode 100644 arch/arm/kvm/hyp/entry.S
>>
>> diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
>> index 173bd1d..c779690 100644
>> --- a/arch/arm/kvm/hyp/Makefile
>> +++ b/arch/arm/kvm/hyp/Makefile
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += timer-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += vgic-v2-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += vfp.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += banked-sr.o
>> +obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>> diff --git a/arch/arm/kvm/hyp/entry.S b/arch/arm/kvm/hyp/entry.S
>> new file mode 100644
>> index 0000000..32f79b0
>> --- /dev/null
>> +++ b/arch/arm/kvm/hyp/entry.S
>> @@ -0,0 +1,70 @@
>> +/*
>> + * Copyright (C) 2016 - ARM Ltd
>> + * Author: Marc Zyngier <marc.zyngier@arm.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +*/
>> +
>> +#include <linux/linkage.h>
>> +#include <asm/asm-offsets.h>
>> +#include <asm/kvm_arm.h>
>> +
>> +	.arch_extension     virt
>> +
>> +	.text
>> +	.pushsection	.hyp.text, "ax"
>> +
>> +#define USR_REGS_OFFSET		(CPU_CTXT_GP_REGS + GP_REGS_USR)
>> +
>> +/* int __guest_enter(struct kvm_vcpu *vcpu, struct kvm_cpu_context *host) */
>> +ENTRY(__guest_enter)
>> +	@ Save host registers
>> +	add	r1, r1, #(USR_REGS_OFFSET + S_R4)
>> +	stm	r1!, {r4-r12}
>> +	str	lr, [r1, #4]	@ Skip SP_usr (already saved)
>> +
>> +	@ Restore guest registers
>> +	add	r0, r0,  #(VCPU_GUEST_CTXT + USR_REGS_OFFSET + S_R0)
> 
> this really relies on offsetof(struct pt_regs, ARM_r0) == 0, which I
> guess will likely never change, but given there's both a kernel and uapi
> version of struct pt_regs, are we sure about this?

If they did diverge, a lot of things would just break. arm64 does have
different types between user and kernel, but the userspace version is
guaranteed to be a strict prefix of the kernel one. I believe arm would
have to enforce the same thing if it changed.

> 
>> +	ldr	lr, [r0, #S_LR]
>> +	ldm	r0, {r0-r12}
>> +
>> +	clrex
>> +	eret
>> +ENDPROC(__guest_enter)
>> +
>> +ENTRY(__guest_exit)
>> +	/*
>> +	 * return convention:
>> +	 * guest r0, r1, r2 saved on the stack
>> +	 * r0: vcpu pointer
>> +	 * r1: exception code
>> +	 */
>> +
>> +	add	r2, r0, #(VCPU_GUEST_CTXT + USR_REGS_OFFSET + S_R3)
>> +	stm	r2!, {r3-r12}
>> +	str	lr, [r2, #4]
>> +	add	r2, r0, #(VCPU_GUEST_CTXT + USR_REGS_OFFSET + S_R0)
>> +	pop	{r3, r4, r5}		@ r0, r1, r2
>> +	stm	r2, {r3-r5}
>> +
>> +	ldr	r0, [r0, #VCPU_HOST_CTXT]
>> +	add	r0, r0, #(USR_REGS_OFFSET + S_R4)
>> +	ldm	r0!, {r4-r12}
>> +	ldr	lr, [r0, #4]
>> +
>> +	mov	r0, r1
>> +	bx	lr
>> +ENDPROC(__guest_exit)
>> +
>> +	.popsection
>> +
>> diff --git a/arch/arm/kvm/hyp/hyp.h b/arch/arm/kvm/hyp/hyp.h
>> index 278eb1f..b3f6ed2 100644
>> --- a/arch/arm/kvm/hyp/hyp.h
>> +++ b/arch/arm/kvm/hyp/hyp.h
>> @@ -110,4 +110,6 @@ static inline bool __vfp_enabled(void)
>>  void __hyp_text __banked_save_state(struct kvm_cpu_context *ctxt);
>>  void __hyp_text __banked_restore_state(struct kvm_cpu_context *ctxt);
>>  
>> +int asmlinkage __guest_enter(struct kvm_vcpu *vcpu,
>> +			     struct kvm_cpu_context *host);
>>  #endif /* __ARM_KVM_HYP_H__ */
>> -- 
>> 2.1.4
>>
> 
> Otherwise:
> Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
> 

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...