On Mon, Oct 1, 2012 at 9:42 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> The current HYP ABI has the following semantics:
>
> <svc code>
> hvc #0 ; Enter HYP mode
> <hyp code>
> hvc #0 ; Exit HYP mode
> <svc code>
>
> This relies on the implicit assumption that you can map both SVC and
> HYP code at the same virtual address so you can seamlessly carry on
> executing code in sequence after switching mode.
>
> This assumption may not be true for ARMv8, if the kernel is located
> high enough to be out of reach of HTTBR.
>
> This patch changes the ABI so the call into HYP becomes a function
> call:
>
> <svc code>
> ldr r0, =BSYM(my_hyp_fn)
> ldr r1, =my_param
> hvc #0 ; Call my_hyp_fn(my_param) from HYP mode
> <svc code>
>
> This allows the HVC handler to offset the function address passed
> from SVC so it can be called. The ABI only supports up to 3 32bit
> parameters, and can return up to 64bits of data.
>
> >From the host point of view, any function mapped in HYP mode can
> be called using the kvm_call_hyp() helper:
>
> u64 kvm_call_hyp(void *hypfn, ...);
>
> For example, calling __kvm_vcpu_run(vcpu) is replaced by:
>
> kvm_call_hyp(__kvm_vcpu_run, vcpu);
>
> Tested on TC2.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> arch/arm/include/asm/kvm_host.h | 1 +
> arch/arm/kvm/arm.c | 4 +-
> arch/arm/kvm/interrupts.S | 104 +++++++++++++++++++---------------------
> arch/arm/kvm/mmu.c | 20 ++++++--
> 4 files changed, 66 insertions(+), 63 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index a1c87e3..25ab5be 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -173,5 +173,6 @@ unsigned long kvm_arm_num_coproc_regs(struct kvm_vcpu *vcpu);
> struct kvm_one_reg;
> int kvm_arm_coproc_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
> int kvm_arm_coproc_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
> +u64 kvm_call_hyp(void *hypfn, ...);
>
> #endif /* __ARM_KVM_HOST_H__ */
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index d64ce2a..7f2ea3d 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -393,7 +393,7 @@ int kvm_arch_vcpu_in_guest_mode(struct kvm_vcpu *v)
>
> static void reset_vm_context(void *info)
> {
> - __kvm_flush_vm_context();
> + kvm_call_hyp(__kvm_flush_vm_context);
> }
>
> /**
> @@ -683,7 +683,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> /* This means ignore, try again. */
> ret = ARM_EXCEPTION_IRQ;
> } else {
> - ret = __kvm_vcpu_run(vcpu);
> + ret = kvm_call_hyp(__kvm_vcpu_run, vcpu);
> }
>
> vcpu->mode = OUTSIDE_GUEST_MODE;
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index c171961..849ff2a 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -20,6 +20,7 @@
> #include <linux/const.h>
> #include <asm/unified.h>
> #include <asm/page.h>
> +#include <asm/ptrace.h>
> #include <asm/asm-offsets.h>
> #include <asm/kvm_asm.h>
> #include <asm/kvm_arm.h>
> @@ -44,7 +45,6 @@ __kvm_hyp_code_start:
> * hardware assisted version.
> */
> ENTRY(__kvm_tlb_flush_vmid)
> - hvc #0 @ Switch to Hyp mode
> push {r2, r3}
>
> add r0, r0, #KVM_VTTBR
> @@ -59,7 +59,6 @@ ENTRY(__kvm_tlb_flush_vmid)
> mcrr p15, 6, r2, r3, c2 @ Back to VMID #0
>
> pop {r2, r3}
> - hvc #0 @ Back to SVC
now perhaps we should re-instate that isb, since the function may be
called without taking an exception straight after.
> bx lr
> ENDPROC(__kvm_tlb_flush_vmid)
>
> @@ -69,14 +68,11 @@ ENDPROC(__kvm_tlb_flush_vmid)
> * void __kvm_flush_vm_context(void);
> */
> ENTRY(__kvm_flush_vm_context)
> - hvc #0 @ switch to hyp-mode
> -
> mov r0, #0 @ rn parameter for c15 flushes is SBZ
> mcr p15, 4, r0, c8, c7, 4 @ Invalidate Non-secure Non-Hyp TLB
> mcr p15, 0, r0, c7, c5, 0 @ Invalidate instruction caches
> dsb
isb back here as well?
>
> - hvc #0 @ switch back to svc-mode, see hyp_svc
> bx lr
> ENDPROC(__kvm_flush_vm_context)
>
> @@ -89,14 +85,15 @@ ENDPROC(__kvm_flush_vm_context)
> @ Arguments:
> @ r0: pointer to vcpu struct
> ENTRY(__kvm_vcpu_run)
> - hvc #0 @ switch to hyp-mode
> + @ Preserve ELR_hyp to return to the HYP trampoline
> + mrs r1, ELR_hyp
> + push {r1}
why not preserve this in the trampoline? It would make the code
slightly cleaner I believe and does not hurt any critical path.
>
> @ Save the vcpu pointer
> mcr p15, 4, r0, c13, c0, 2 @ HTPIDR
>
> - @ Now we're in Hyp-mode and lr_usr, spsr_hyp are on the stack
> - mrs r2, sp_usr
> - push {r2} @ Push r13_usr
> + @ Now we're in Hyp-mode and ELR_hyp, spsr_hyp are on the stack
> + store_mode_state sp, usr
> push {r4-r12} @ Push r4-r12
>
> store_mode_state sp, svc
> @@ -235,10 +232,10 @@ after_vfp_restore:
> load_mode_state sp, svc
>
> pop {r4-r12} @ Pop r4-r12
> - pop {r2} @ Pop r13_usr
> - msr sp_usr, r2
> + load_mode_state sp, usr @ SP_usr, LR_usr
>
> - hvc #0 @ switch back to svc-mode, see hyp_svc
> + pop {r2}
> + msr ELR_hyp, r2
>
> clrex @ Clear exclusive monitor
> bx lr @ return to IOCTL
> @@ -255,8 +252,6 @@ after_vfp_restore:
> * Returns 64 bit PAR value.
> */
> ENTRY(__kvm_va_to_pa)
> - hvc #0 @ switch to hyp-mode
> -
> push {r4-r12}
>
> @ Fold flag into r1, easier than using stack.
> @@ -280,7 +275,10 @@ ENTRY(__kvm_va_to_pa)
>
> mrrc p15, 0, r0, r1, c7 @ PAR
> pop {r4-r12}
> - hvc #0 @ Back to SVC
> + bx lr
> +
> +ENTRY(kvm_call_hyp)
> + hvc #0
> bx lr
>
>
> @@ -302,20 +300,11 @@ ENTRY(__kvm_va_to_pa)
> * Hyp-ABI: Switching from host kernel to Hyp-mode:
this should be rewritten now to be phrased something like:
Hyp-ABI: Calling functions in Hyp mode, and the text below should be
adapted accordingly.
> * Switching to Hyp mode is done through a simple HVC instructions. The
> * exception vector code will check that the HVC comes from VMID==0 and if
> - * so will store the necessary state on the Hyp stack, which will look like
> - * this (growing downwards, see the hyp_hvc handler):
> - * ...
> - * stack_page + 4: spsr (Host-SVC cpsr)
> - * stack_page : lr_usr
> - * --------------: stack bottom
> - *
> - * Hyp-ABI: Switching from Hyp-mode to host kernel SVC mode:
> - * When returning from Hyp mode to SVC mode, another HVC instruction is
> - * executed from Hyp mode, which is taken in the hyp_svc handler. The
> - * bottom of the Hyp is derived from the Hyp stack pointer (only a single
> - * page aligned stack is used per CPU) and the initial SVC registers are
> - * used to restore the host state.
> - *
> + * so will push the necessary state (SPSR, lr_usr) on the Hyp stack.
> + * - r0 contains a pointer to a HYP function
> + * - r1, r2, and r3 contain arguments to the above function.
> + * - The HYP function will be called with its arguments in r0, r1 and r2.
> + * On HYP function return, we return directly to SVC.
> *
> * Note that the above is used to execute code in Hyp-mode from a host-kernel
> * point of view, and is a different concept from performing a world-switch and
> @@ -351,12 +340,18 @@ ENTRY(__kvm_va_to_pa)
> str r2, [r1, #VCPU_HYP_PC]
> b __kvm_vcpu_return
>
> - @ We were in the host already
> -99: hvc #0 @ switch to SVC mode
> - ldr r0, \panic_str
> + @ We were in the host already. Let's craft a panic-ing return to SVC.
> +99: mrs r2, cpsr
> + bic r2, r2, #MODE_MASK
> + orr r2, r2, #SVC_MODE
> +THUMB( orr r2, r2, #PSR_T_BIT )
> + msr spsr_cxsf, r2
> mrs r1, ELR_hyp
> - b panic
> -
> +THUMB( ldr r2, =BSYM(panic) + 4)
> +ARM( ldr r2, =BSYM(panic) + 8)
forgive my ignorance, why do we need the + 4 or + 8 offsets here?
> + msr ELR_hyp, r2
> + ldr r0, \panic_str
> + eret
> .endm
>
> .text
> @@ -385,17 +380,7 @@ hyp_undef:
>
> .align
> hyp_svc:
> - @ Can only get here if HVC or SVC is called from Hyp, mode which means
> - @ we want to change mode back to SVC mode.
> - push {r12}
> - mov r12, sp
> - bic r12, r12, #0x0ff
> - bic r12, r12, #0xf00
> - ldr lr, [r12, #4]
> - msr SPSR_csxf, lr
> - ldr lr, [r12]
> - pop {r12}
> - eret
> + bad_exception ARM_EXCEPTION_HVC, svc_die_str
>
> .align
> hyp_pabt:
> @@ -434,19 +419,24 @@ hyp_hvc:
> bne guest_trap @ Guest called HVC
>
> host_switch_to_hyp:
> - @ Store lr_usr,spsr (svc cpsr) on bottom of stack
> - mov r1, sp
> - bic r1, r1, #0x0ff
> - bic r1, r1, #0xf00
> - str lr, [r1]
> - mrs lr, spsr
> - str lr, [r1, #4]
> -
> pop {r0, r1, r2}
>
> - @ Return to caller in Hyp mode
> - mrs lr, ELR_hyp
> - mov pc, lr
> + push {lr}
> + mrs lr, SPSR
> + push {lr}
> +
> + mov lr, r0
> + mov r0, r1
> + mov r1, r2
> + mov r2, r3
> +
> +THUMB( orr lr, #1)
> + blx lr @ Call the HYP function
> +
> + pop {lr}
> + msr SPSR_csxf, lr
> + pop {lr}
> + eret
>
> guest_trap:
> load_vcpu r1 @ Load VCPU pointer
> @@ -525,6 +515,8 @@ pabt_die_str:
> .ascii "unexpected prefetch abort in Hyp mode at: %#08x"
> dabt_die_str:
> .ascii "unexpected data abort in Hyp mode at: %#08x"
> +svc_die_str:
> + .ascii "unexpected HVC/SVC trap in Hyp mode at: %#08x"
>
> /*
> * The below lines makes sure the HYP mode code fits in a single page (the
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index a35a8a9..8f6761c 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -36,6 +36,11 @@
> static DEFINE_MUTEX(kvm_hyp_pgd_mutex);
> static pgd_t *hyp_pgd;
>
> +static void kvm_tlb_flush_vmid(struct kvm *kvm)
> +{
> + kvm_call_hyp(__kvm_tlb_flush_vmid, kvm);
> +}
> +
> static int mmu_topup_memory_cache(struct kvm_mmu_memory_cache *cache,
> int min, int max)
> {
> @@ -393,7 +398,7 @@ static void stage2_clear_pte(struct kvm *kvm, phys_addr_t addr)
> page = virt_to_page(pte);
> put_page(page);
> if (page_count(page) != 1) {
> - __kvm_tlb_flush_vmid(kvm);
> + kvm_tlb_flush_vmid(kvm);
> return;
> }
>
> @@ -404,7 +409,7 @@ static void stage2_clear_pte(struct kvm *kvm, phys_addr_t addr)
> page = virt_to_page(pmd);
> put_page(page);
> if (page_count(page) != 1) {
> - __kvm_tlb_flush_vmid(kvm);
> + kvm_tlb_flush_vmid(kvm);
> return;
> }
>
> @@ -413,7 +418,7 @@ static void stage2_clear_pte(struct kvm *kvm, phys_addr_t addr)
>
> page = virt_to_page(pud);
> put_page(page);
> - __kvm_tlb_flush_vmid(kvm);
> + kvm_tlb_flush_vmid(kvm);
> }
>
> static void stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
> @@ -453,7 +458,7 @@ static void stage2_set_pte(struct kvm *kvm, struct kvm_mmu_memory_cache *cache,
> old_pte = *pte;
> set_pte_ext(pte, *new_pte, 0);
> if (pte_present(old_pte))
> - __kvm_tlb_flush_vmid(kvm);
> + kvm_tlb_flush_vmid(kvm);
> else
> get_page(virt_to_page(pte));
> }
> @@ -611,6 +616,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> return 0;
> }
>
> +static u64 kvm_va_to_pa(struct kvm_vcpu *vcpu, u32 va, bool priv)
> +{
> + return kvm_call_hyp(__kvm_va_to_pa, vcpu, va, priv);
> +}
> +
> /**
> * copy_from_guest_va - copy memory from guest (very slow!)
> * @vcpu: vcpu pointer
> @@ -631,7 +641,7 @@ static bool copy_from_guest_va(struct kvm_vcpu *vcpu,
> int err;
>
> BUG_ON((gva & PAGE_MASK) != ((gva + len) & PAGE_MASK));
> - par = __kvm_va_to_pa(vcpu, gva & PAGE_MASK, priv);
> + par = kvm_va_to_pa(vcpu, gva & PAGE_MASK, priv);
> if (par & 1) {
> kvm_err("IO abort from invalid instruction address"
> " %#lx!\n", gva);
> --
> 1.7.12
>
>
this is definitely much nicer,
thanks.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
