Kexec and KVM not working gracefully together

[takahiro.akashi@linaro.org (AKASHI Takahiro)]

Hi Frediano
[add Geoff to cc]

I still have to compare my code with yours, but please note that
some files in arch/arm/kvm are shared with arm64, especially
arm.c, mmu.c and related headers.

So can you please
- submit your new patch without including old e-mail threads.
   (don't forget RFC or PATCH.)
- break it up into two pieces, one for common, the other for arm

and a few more comments below:

On 02/06/2015 01:56 AM, Frediano Ziglio wrote:

[snip]

> New version. Changes:
> - removed saved value and test again
> - remove vm flush, useless
> - correct compile check on header
> - try KVM enabled and not, compile link and tests
> - rewrite comments, add more where necessary
> - remove code to free and allocate again boot PGD and related, keep in
> memory if KEXEC is enabled
>
> Still not trying SMP. Still to be considered RFC. I tried different
> compile options (KEXEC and KVM turned on/off). I realized that I
> should test if kernel is started with SVC mode code still works
> correctly. ARM_VIRT_EXT is always turned on for V7 CPU.
>
>
> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
> index 3a67bec..985f0a3 100644
> --- a/arch/arm/include/asm/kvm_asm.h
> +++ b/arch/arm/include/asm/kvm_asm.h
> @@ -97,7 +97,19 @@ extern char __kvm_hyp_code_end[];
>   extern void __kvm_flush_vm_context(void);
>   extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
>
> +extern void __kvm_set_vectors(unsigned long phys_vector_base);
> +
>   extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
> +
> +#ifndef CONFIG_ARM_VIRT_EXT

Arm64 doesn't have CONFIG_ARM_VIRT_EXT. Why don't use CONFIG_KVM?
I'd rather put this conditional into the place where the function is
actually called for flexible implementation. (See below)

> +static void kvm_cpu_reset(void (*phys_reset)(void *), void *addr)
> +{
> +    phys_reset(addr);
> +}
> +#else
> +extern void kvm_cpu_reset(void (*phys_reset)(void *), void *addr);
> +#endif
> +
>   #endif
>
>   #endif /* __ARM_KVM_ASM_H__ */
> diff --git a/arch/arm/kernel/hyp-stub.S b/arch/arm/kernel/hyp-stub.S
> index 2a55373..30339ad 100644
> --- a/arch/arm/kernel/hyp-stub.S
> +++ b/arch/arm/kernel/hyp-stub.S
> @@ -164,13 +164,63 @@ ARM_BE8(orr    r7, r7, #(1 << 25))     @ HSCTLR.EE
>       bx    lr            @ The boot CPU mode is left in r4.
>   ENDPROC(__hyp_stub_install_secondary)
>
> +#undef CPU_RESET
> +#if defined(CONFIG_ARM_VIRT_EXT) && !defined(CONFIG_KVM) && !defined(ZIMAGE)
> +#  define CPU_RESET 1
> +#endif
> +
>   __hyp_stub_do_trap:
> +#ifdef CPU_RESET
> +    cmp    r0, #-2
> +    bne    1f
> +
> +    mrc    p15, 4, r0, c1, c0, 0    @ HSCR
> +    ldr    r2, =0x40003807
> +    bic    r0, r0, r2
> +    mcr    p15, 4, r0, c1, c0, 0    @ HSCR
> +    isb
> +
> +    @ Disable MMU, caches and Thumb in SCTLR
> +    mrc    p15, 0, r0, c1, c0, 0    @ SCTLR
> +    bic    r0, r0, r2
> +    mcr    p15, 0, r0, c1, c0, 0
> +
> +    bx    r1
> +    .ltorg
> +1:
> +#endif
>       cmp    r0, #-1
>       mrceq    p15, 4, r0, c12, c0, 0    @ get HVBAR
>       mcrne    p15, 4, r0, c12, c0, 0    @ set HVBAR
>       __ERET
>   ENDPROC(__hyp_stub_do_trap)
>
> +#ifdef CPU_RESET
> +/*
> + * r0 cpu_reset function
> + * r1 address to jump to
> + */
> +ENTRY(kvm_cpu_reset)
> +    mov    r2, r0
> +
> +    @ __boot_cpu_mode should be HYP_MODE
> +    adr    r0, .L__boot_cpu_mode_offset
> +    ldr    r3, [r0]
> +    ldr    r0, [r0, r3]
> +    cmp    r0, #HYP_MODE
> +    beq    reset_to_hyp
> +
> +    @ Call SVC mode cpu_reset
> +    mov    r0, r1
> +THUMB(    orr    r2, r2, 1    )
> +    bx    r2
> +
> +reset_to_hyp:
> +    mov    r0, #-2
> +    b    __hyp_set_vectors
> +ENDPROC(kvm_cpu_reset)
> +#endif
> +
>   /*
>    * __hyp_set_vectors: Call this after boot to set the initial hypervisor
>    * vectors as part of hypervisor installation.  On an SMP system, this should
> diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
> index fdfa3a7..c018719 100644
> --- a/arch/arm/kernel/process.c
> +++ b/arch/arm/kernel/process.c
> @@ -41,6 +41,7 @@
>   #include <asm/system_misc.h>
>   #include <asm/mach/time.h>
>   #include <asm/tls.h>
> +#include <asm/kvm_asm.h>
>
>   #ifdef CONFIG_CC_STACKPROTECTOR
>   #include <linux/stackprotector.h>
> @@ -60,7 +61,7 @@ static const char *isa_modes[] __maybe_unused = {
>   };
>
>   extern void call_with_stack(void (*fn)(void *), void *arg, void *sp);
> -typedef void (*phys_reset_t)(unsigned long);
> +typedef void (*phys_reset_t)(void *);
>
>   /*
>    * A temporary stack to use for CPU reset. This is static so that we
> @@ -89,7 +90,8 @@ static void __soft_restart(void *addr)
>
>       /* Switch to the identity mapping. */
>       phys_reset = (phys_reset_t)(unsigned long)virt_to_phys(cpu_reset);
> -    phys_reset((unsigned long)addr);
> +
> +    kvm_cpu_reset(phys_reset, addr);

How about this?
#ifdef CONFIG_KVM
	kvm_cpu_reset(...);
#endif
	phys_reset(addr);

It will clearify the purpose of kvm_cpu_reset().
The name of kvm_cpu_reset() might better be cpu_*de*init_hyp_mode or similar
given that the function would be called in hyp_init_cpu_notify()
once kvm supports cpu hotplug in the future.

>       /* Should never get here. */
>       BUG();
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 0b0d58a..e4d4a2b 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -1009,7 +1009,7 @@ static int init_hyp_mode(void)
>       if (err)
>           goto out_free_mappings;
>
> -#ifndef CONFIG_HOTPLUG_CPU
> +#if !defined(CONFIG_HOTPLUG_CPU) && !defined(CONFIG_KEXEC)
>       free_boot_hyp_pgd();
>   #endif
>
> @@ -1079,6 +1079,53 @@ out_err:
>       return err;
>   }
>
> +void kvm_cpu_reset(void (*phys_reset)(void *), void *addr)

I'm not sure, but it might be better to put this code into arm/kernel/init.S
as it is a counterpart of _do_hyp_init from cpu_init_hyp_mode().

> +{
> +    unsigned long vector;
> +
> +    if (!is_hyp_mode_available()) {
> +        phys_reset(addr);
> +        return;
> +    }
> +
> +    vector = (unsigned long) kvm_get_idmap_vector();
> +
> +    /*
> +     * Set vectors so we call initialization routines.
> +     * trampoline is mapped at TRAMPOLINE_VA but not at its physical
> +     * address so we don't have an identity map to be able to
> +     * disable MMU. We need to set exception vector at trampoline
> +     * at TRAMPOLINE_VA address which is mapped.
> +     */
> +    kvm_call_hyp(__kvm_set_vectors,(unsigned long) (TRAMPOLINE_VA +
> (vector & (PAGE_SIZE-1))));
> +
> +    /*
> +     * Set HVBAR to physical address, page table to identity to do the switch
> +     */
> +    kvm_call_hyp((void*) 4, (unsigned long) vector, kvm_mmu_get_boot_httbr());
> +
> +    /*
> +     * Flush to make sure code after the cache are disabled see updated values
> +     */
> +    flush_cache_all();
> +
> +    /*
> +     * Turn off caching on Hypervisor mode
> +     */
> +    kvm_call_hyp((void*) 1);
> +
> +    /*
> +     * Flush to make sude code don't see old cached values after cache is
> +     * enabled
> +     */
> +    flush_cache_all();
> +
> +    /*
> +     * Restore execution
> +     */
> +    kvm_call_hyp((void*) 3, addr);
> +}
> +

If you like kvm_call_hyp-like sequences, please specify what each kvm_call_hyp() should do.
(we can do all in one kvm_call_hyp() though.)

Thanks,
-Takahiro AKASHI

>   /* NOP: Compiling as a module not supported */
>   void kvm_arch_exit(void)
>   {
> diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
> index 3988e72..c2f1d4d 100644
> --- a/arch/arm/kvm/init.S
> +++ b/arch/arm/kvm/init.S
> @@ -68,6 +68,12 @@ __kvm_hyp_init:
>       W(b)    .
>
>   __do_hyp_init:
> +    @ check for special values, odd numbers can't be a stack pointer
> +    cmp    r0, #1
> +    beq    cpu_proc_fin
> +    cmp    r0, #3
> +    beq    restore
> +
>       cmp    r0, #0            @ We have a SP?
>       bne    phase2            @ Yes, second stage init
>
> @@ -151,6 +157,33 @@ target:    @ We're now in the trampoline code,
> switch page tables
>
>       eret
>
> +cpu_proc_fin:
> +    mrc    p15, 4, r0, c1, c0, 0        @ ctrl register
> +    bic    r0, r0, #0x1000            @ ...i............
> +    bic    r0, r0, #0x0006            @ .............ca.
> +    mcr    p15, 4, r0, c1, c0, 0        @ disable caches
> +    eret
> +
> +restore:
> +    @ Disable MMU, caches and Thumb in HSCTLR
> +    mrc    p15, 4, r0, c1, c0, 0    @ HSCR
> +    ldr    r2, =0x40003807
> +    bic    r0, r0, r2
> +    mcr    p15, 4, r0, c1, c0, 0    @ HSCR
> +    isb
> +
> +    @ Invalidate old TLBs
> +    mcr    p15, 4, r0, c8, c7, 0    @ TLBIALLH
> +    isb
> +    dsb    ish
> +
> +    @ Disable MMU, caches and Thumb in SCTLR
> +    mrc    p15, 0, r0, c1, c0, 0   @ SCTLR
> +    bic    r0, r0, r2
> +    mcr    p15, 0, r0, c1, c0, 0
> +
> +    bx    r1
> +
>       .ltorg
>
>       .globl __kvm_hyp_init_end
> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
> index 01dcb0e..449e7dd 100644
> --- a/arch/arm/kvm/interrupts.S
> +++ b/arch/arm/kvm/interrupts.S
> @@ -87,6 +87,18 @@ ENDPROC(__kvm_flush_vm_context)
>
>
>   /********************************************************************
> + * Set HVBAR
> + *
> + * void __kvm_set_vectors(unsigned long phys_vector_base);
> + */
> +ENTRY(__kvm_set_vectors)
> +    mcr    p15, 4, r0, c12, c0, 0    @ set HVBAR
> +    dsb    ish
> +    isb
> +    bx    lr
> +ENDPROC(__kvm_set_vectors)
> +
> +/********************************************************************
>    *  Hypervisor world-switch code
>    *
>    *
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 1366625..f853858 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -369,6 +369,11 @@ void free_boot_hyp_pgd(void)
>       free_page((unsigned long)init_bounce_page);
>       init_bounce_page = NULL;
>
> +    /* avoid to reuse possibly invalid values if bounce page is freed */
> +    hyp_idmap_start = 0;
> +    hyp_idmap_end = 0;
> +    hyp_idmap_vector = 0;
> +
>       mutex_unlock(&kvm_hyp_pgd_mutex);
>   }
>
>
> Frediano
>