Re: [PATCH 16/29] arm64: KVM: HYP mode world switch implementation

Christopher Covington
2013-03-13T19:59:20Z

Hi Marc,

I like how you were able to use a common fpsimd_(save|restore) macro, and
wonder if you can't do the same sort of thing for the general purpose
registers and system registers. In the end, both guest and host are EL1
software, and while they may differ in terms of things like VTTBR settings and
physical timer access, my intuition is that which general purpose and system
registers need to be saved and restored on context switches is shared.

On 03/04/2013 10:47 PM, Marc Zyngier wrote:
> The HYP mode world switch in all its glory.
>
> Implements save/restore of host/guest registers, EL2 trapping,
> IPA resolution, and additional services (tlb invalidation).
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/kernel/asm-offsets.c | 33 ++
> arch/arm64/kvm/hyp.S | 756 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 789 insertions(+)
> create mode 100644 arch/arm64/kvm/hyp.S

[...]

> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S

[...]

> +.macro save_host_regs
> + push x19, x20
> + push x21, x22
> + push x23, x24
> + push x25, x26
> + push x27, x28
> + push x29, lr
> +
> + mrs x19, sp_el0
> + mrs x20, sp_el1
> + mrs x21, elr_el1
> + mrs x22, spsr_el1
> + mrs x23, elr_el2
> + mrs x24, spsr_el2
> +
> + push x19, x20
> + push x21, x22
> + push x23, x24
> +.endm

[...]

> +.macro save_guest_regs
> + // x0 is the vcpu address.
> + // x1 is the return code, do not corrupt!
> + // Guest's x0-x3 are on the stack
> +
> + // Compute base to save registers
> + add x2, x0, #REG_OFFSET(4)
> + mrs x3, sp_el0
> + stp x4, x5, [x2], #16
> + stp x6, x7, [x2], #16
> + stp x8, x9, [x2], #16
> + stp x10, x11, [x2], #16
> + stp x12, x13, [x2], #16
> + stp x14, x15, [x2], #16
> + stp x16, x17, [x2], #16
> + stp x18, x19, [x2], #16
> + stp x20, x21, [x2], #16
> + stp x22, x23, [x2], #16
> + stp x24, x25, [x2], #16
> + stp x26, x27, [x2], #16
> + stp x28, x29, [x2], #16
> + stp lr, x3, [x2], #16 // LR, SP_EL0
> +
> + mrs x4, elr_el2 // PC
> + mrs x5, spsr_el2 // CPSR
> + stp x4, x5, [x2], #16
> +
> + pop x6, x7 // x2, x3
> + pop x4, x5 // x0, x1
> +
> + add x2, x0, #REG_OFFSET(0)
> + stp x4, x5, [x2], #16
> + stp x6, x7, [x2], #16
> +
> + // EL1 state
> + mrs x4, sp_el1
> + mrs x5, elr_el1
> + mrs x6, spsr_el1
> + str x4, [x0, #VCPU_SP_EL1]
> + str x5, [x0, #VCPU_ELR_EL1]
> + str x6, [x0, #SPSR_OFFSET(KVM_SPSR_EL1)]
> +.endm

There are two relatively easily reconciled differences in my mind that tend to
obscure the similarity between these pieces of code. The first is the use of
push and pop macros standing in for the underlying stp and ldp instructions
and the second is the order in which the registers are stored. I may be
missing something, but my impression is that the order doesn't really matter,
as long as there is universal agreement on what the order will be.

It seems to me then that the fundamental differences are the base address of
the load and store operations and which registers have already been saved by
other code.

What if the base address for the loads and stores, sp versus x2, was made a
macro argument? If it's not straightforward to make the direction of the guest
and host stores the same then the increment value or its sign could also be
made an argument. Alternatively, you could consider storing the host registers
in a slimmed-down vcpu structure for hosts, rather than on the stack.

I need to study the call graph to better understand the asymmetry in which
registers are already saved off by the time we get here. I wonder if there's
more opportunity to unify code there. Short of that perhaps more ideal
solution one could still share the GPR's 19-29 and system register code, but
have the guest version save off GPR's 4-18 before going down an at least
source-level shared path.

[...]

> +/*
> + * Macros to perform system register save/restore.
> + *
> + * Ordering here is absolutely critical, and must be kept consistent
> + * in dump_sysregs, load_sysregs, {save,restore}_guest_sysregs,
> + * {save,restore}_guest_32bit_state, and in kvm_asm.h.
> + *
> + * In other words, don't touch any of these unless you know what
> + * you are doing.
> + */
> +.macro dump_sysregs
> + mrs x4, mpidr_el1

Maybe this should be taken out of the shared code and put in save_host_sysregs
if it only applies to hosts? Also, is the use of mpidr_el1 here and vmpidr_el2
in load_sysregs intentional? If so it might be nice to add a note about that
to your existing comment.

> + mrs x5, csselr_el1
> + mrs x6, sctlr_el1
> + mrs x7, actlr_el1
> + mrs x8, cpacr_el1
> + mrs x9, ttbr0_el1
> + mrs x10, ttbr1_el1
> + mrs x11, tcr_el1
> + mrs x12, esr_el1
> + mrs x13, afsr0_el1
> + mrs x14, afsr1_el1
> + mrs x15, far_el1
> + mrs x16, mair_el1
> + mrs x17, vbar_el1
> + mrs x18, contextidr_el1
> + mrs x19, tpidr_el0
> + mrs x20, tpidrro_el0
> + mrs x21, tpidr_el1
> + mrs x22, amair_el1
> + mrs x23, cntkctl_el1
> +.endm

[...]

> +.macro save_guest_sysregs
> + dump_sysregs
> + add x2, x0, #SYSREG_OFFSET(CSSELR_EL1) // MIPDR_EL2 not written back

MPIDR_EL1

[...]

Regards,
Christopher

--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
the Linux Foundation