Re: [PATCH v2 09/21] arm64: KVM: Implement guest entry

2015-12-01 Thread Marc Zyngier 
On 01/12/15 15:29, Christoffer Dall wrote:
> On Fri, Nov 27, 2015 at 06:50:03PM +, Marc Zyngier wrote:
>> Contrary to the previous patch, the guest entry is fairly different
>> from its assembly counterpart, mostly because it is only concerned
>> with saving/restoring the GP registers, and nothing else.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  arch/arm64/kvm/hyp/Makefile |   1 +
>>  arch/arm64/kvm/hyp/entry.S  | 155 
>> 
>>  arch/arm64/kvm/hyp/hyp.h|   2 +
>>  3 files changed, 158 insertions(+)
>>  create mode 100644 arch/arm64/kvm/hyp/entry.S
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index ec14cac..1e1ff06 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -7,3 +7,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += timer-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
>>  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
>> +obj-$(CONFIG_KVM_ARM_HOST) += entry.o
>> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
>> new file mode 100644
>> index 000..2c4449a
>> --- /dev/null
>> +++ b/arch/arm64/kvm/hyp/entry.S
>> @@ -0,0 +1,155 @@
>> +/*
>> + * Copyright (C) 2015 - ARM Ltd
>> + * Author: Marc Zyngier 
>> + *
>> + * 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 .
>> + */
>> +
>> +#include 
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define CPU_GP_REG_OFFSET(x)(CPU_GP_REGS + x)
>> +#define CPU_XREG_OFFSET(x)  CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
>> +
>> +.text
>> +.pushsection.hyp.text, "ax"
>> +
>> +.macro save_common_regs ctxt
>> +stp x19, x20, [\ctxt, #CPU_XREG_OFFSET(19)]
>> +stp x21, x22, [\ctxt, #CPU_XREG_OFFSET(21)]
>> +stp x23, x24, [\ctxt, #CPU_XREG_OFFSET(23)]
>> +stp x25, x26, [\ctxt, #CPU_XREG_OFFSET(25)]
>> +stp x27, x28, [\ctxt, #CPU_XREG_OFFSET(27)]
>> +stp x29, lr,  [\ctxt, #CPU_XREG_OFFSET(29)]
>> +.endm
>> +
>> +.macro restore_common_regs ctxt
>> +ldp x19, x20, [\ctxt, #CPU_XREG_OFFSET(19)]
>> +ldp x21, x22, [\ctxt, #CPU_XREG_OFFSET(21)]
>> +ldp x23, x24, [\ctxt, #CPU_XREG_OFFSET(23)]
>> +ldp x25, x26, [\ctxt, #CPU_XREG_OFFSET(25)]
>> +ldp x27, x28, [\ctxt, #CPU_XREG_OFFSET(27)]
>> +ldp x29, lr,  [\ctxt, #CPU_XREG_OFFSET(29)]
>> +.endm
>> +
>> +.macro save_host_regs reg
>> +save_common_regs \reg
>> +.endm
>> +
>> +.macro restore_host_regs reg
>> +restore_common_regs \reg
>> +.endm
>> +
>> +.macro save_guest_regs
>> +// x0 is the vcpu address
>> +// x1 is the return code, do not corrupt!
>> +// x2 is the cpu context
> 
> this is confusing because the caller says x2 is free, so are these the
> inputs or invariants preserved in the function, or?
> 
> note that you'll avoid this kind of confusion by inlining this stuff in
> __guest_exit.

Indeed. I might just do that.

>> +// x3 is a tmp register
>> +// Guest's x0-x3 are on the stack
>> +
>> +add x2, x0, #VCPU_CONTEXT
>> +
>> +// Compute base to save registers
> 
> misleading comment?

Of course. Isn't that the very purpose of a comment? I'm confused... ;-)

>> +stp x4, x5,   [x2, #CPU_XREG_OFFSET(4)]
>> +stp x6, x7,   [x2, #CPU_XREG_OFFSET(6)]
>> +stp x8, x9,   [x2, #CPU_XREG_OFFSET(8)]
>> +stp x10, x11, [x2, #CPU_XREG_OFFSET(10)]
>> +stp x12, x13, [x2, #CPU_XREG_OFFSET(12)]
>> +stp x14, x15, [x2, #CPU_XREG_OFFSET(14)]
>> +stp x16, x17, [x2, #CPU_XREG_OFFSET(16)]
>> +str x18,  [x2, #CPU_XREG_OFFSET(18)]
>> +
>> +pop x6, x7  // x2, x3
>> +pop x4, x5  // x0, x1
> 
> hard to review when I haven't seen the code that calls this, but I'll
> assume we store things in register order on the stack.

Indeed. I've basically lifted that code from the previous version, so
some things may have stuck...

>> +
>> +stp x4, x5, [x2, #CPU_XREG_OFFSET(0)]
>> +stp x6, x7, [x2, #CPU_XREG_OFFSET(2)]
>> +
>> +save_common_regs x2
>> +.endm
>> +
>> +.macro restore_guest_regs
>> +// Assume vcpu in x0, clobbers everything else
> 
> nit: clobbers everything (x0 gets nuked too)
> 
>> +
>> +add x2, x0, #VCPU_CONTEXT
>> +
>>

Re: [PATCH v2 09/21] arm64: KVM: Implement guest entry

2015-12-01 Thread Christoffer Dall 
On Fri, Nov 27, 2015 at 06:50:03PM +, Marc Zyngier wrote:
> Contrary to the previous patch, the guest entry is fairly different
> from its assembly counterpart, mostly because it is only concerned
> with saving/restoring the GP registers, and nothing else.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  arch/arm64/kvm/hyp/Makefile |   1 +
>  arch/arm64/kvm/hyp/entry.S  | 155 
> 
>  arch/arm64/kvm/hyp/hyp.h|   2 +
>  3 files changed, 158 insertions(+)
>  create mode 100644 arch/arm64/kvm/hyp/entry.S
> 
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index ec14cac..1e1ff06 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -7,3 +7,4 @@ obj-$(CONFIG_KVM_ARM_HOST) += vgic-v3-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += timer-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += sysreg-sr.o
>  obj-$(CONFIG_KVM_ARM_HOST) += debug-sr.o
> +obj-$(CONFIG_KVM_ARM_HOST) += entry.o
> diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
> new file mode 100644
> index 000..2c4449a
> --- /dev/null
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -0,0 +1,155 @@
> +/*
> + * Copyright (C) 2015 - ARM Ltd
> + * Author: Marc Zyngier 
> + *
> + * 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 .
> + */
> +
> +#include 
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define CPU_GP_REG_OFFSET(x) (CPU_GP_REGS + x)
> +#define CPU_XREG_OFFSET(x)   CPU_GP_REG_OFFSET(CPU_USER_PT_REGS + 8*x)
> +
> + .text
> + .pushsection.hyp.text, "ax"
> +
> +.macro save_common_regs ctxt
> + stp x19, x20, [\ctxt, #CPU_XREG_OFFSET(19)]
> + stp x21, x22, [\ctxt, #CPU_XREG_OFFSET(21)]
> + stp x23, x24, [\ctxt, #CPU_XREG_OFFSET(23)]
> + stp x25, x26, [\ctxt, #CPU_XREG_OFFSET(25)]
> + stp x27, x28, [\ctxt, #CPU_XREG_OFFSET(27)]
> + stp x29, lr,  [\ctxt, #CPU_XREG_OFFSET(29)]
> +.endm
> +
> +.macro restore_common_regs ctxt
> + ldp x19, x20, [\ctxt, #CPU_XREG_OFFSET(19)]
> + ldp x21, x22, [\ctxt, #CPU_XREG_OFFSET(21)]
> + ldp x23, x24, [\ctxt, #CPU_XREG_OFFSET(23)]
> + ldp x25, x26, [\ctxt, #CPU_XREG_OFFSET(25)]
> + ldp x27, x28, [\ctxt, #CPU_XREG_OFFSET(27)]
> + ldp x29, lr,  [\ctxt, #CPU_XREG_OFFSET(29)]
> +.endm
> +
> +.macro save_host_regs reg
> + save_common_regs \reg
> +.endm
> +
> +.macro restore_host_regs reg
> + restore_common_regs \reg
> +.endm
> +
> +.macro save_guest_regs
> + // x0 is the vcpu address
> + // x1 is the return code, do not corrupt!
> + // x2 is the cpu context

this is confusing because the caller says x2 is free, so are these the
inputs or invariants preserved in the function, or?

note that you'll avoid this kind of confusion by inlining this stuff in
__guest_exit.

> + // x3 is a tmp register
> + // Guest's x0-x3 are on the stack
> +
> + add x2, x0, #VCPU_CONTEXT
> +
> + // Compute base to save registers

misleading comment?

> + stp x4, x5,   [x2, #CPU_XREG_OFFSET(4)]
> + stp x6, x7,   [x2, #CPU_XREG_OFFSET(6)]
> + stp x8, x9,   [x2, #CPU_XREG_OFFSET(8)]
> + stp x10, x11, [x2, #CPU_XREG_OFFSET(10)]
> + stp x12, x13, [x2, #CPU_XREG_OFFSET(12)]
> + stp x14, x15, [x2, #CPU_XREG_OFFSET(14)]
> + stp x16, x17, [x2, #CPU_XREG_OFFSET(16)]
> + str x18,  [x2, #CPU_XREG_OFFSET(18)]
> +
> + pop x6, x7  // x2, x3
> + pop x4, x5  // x0, x1

hard to review when I haven't seen the code that calls this, but I'll
assume we store things in register order on the stack.

> +
> + stp x4, x5, [x2, #CPU_XREG_OFFSET(0)]
> + stp x6, x7, [x2, #CPU_XREG_OFFSET(2)]
> +
> + save_common_regs x2
> +.endm
> +
> +.macro restore_guest_regs
> + // Assume vcpu in x0, clobbers everything else

nit: clobbers everything (x0 gets nuked too)

> +
> + add x2, x0, #VCPU_CONTEXT
> +
> + // Prepare x0-x3 for later restore
> + ldp x4, x5, [x2, #CPU_XREG_OFFSET(0)]
> + ldp x6, x7, [x2, #CPU_XREG_OFFSET(2)]
> + pushx4, x5  // Push x0-x3 on the stack
> + pushx6, x7

why do you need x2 and x3 later? can't you just make do with x0 and x1
and move the cpu context pointer to x1 ?

> +
> + // x4-x18
> +