Hi Takahiro, On 27/05/15 06:21, AKASHI Takahiro wrote: > Marc, > > Thank you for your reviews: > > On 05/26/2015 06:26 PM, Marc Zyngier wrote: >> Hi Takahiro, >> >> On 08/05/15 02:18, AKASHI Takahiro wrote: >>> Cpu must be put back into its initial state, at least, in the >>> following cases in order to shutdown the system and/or re-initialize cpus >>> later on: >>> 1) kexec/kdump >>> 2) cpu hotplug (offline) >>> 3) removing kvm as a module >>> >>> To address those issues in later patches, this patch adds a tear-down >>> function, kvm_cpu_reset(), that disables D-cache & MMU and restore a vector >>> table to the initial stub at EL2. >>> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org> >>> --- >>> arch/arm/kvm/arm.c | 15 +++++++++++++++ >>> arch/arm/kvm/mmu.c | 5 +++++ >>> arch/arm64/include/asm/kvm_asm.h | 1 + >>> arch/arm64/include/asm/kvm_host.h | 11 +++++++++++ >>> arch/arm64/include/asm/kvm_mmu.h | 7 +++++++ >>> arch/arm64/include/asm/virt.h | 11 +++++++++++ >>> arch/arm64/kvm/hyp-init.S | 32 ++++++++++++++++++++++++++++++++ >>> arch/arm64/kvm/hyp.S | 16 +++++++++++++--- >>> 8 files changed, 95 insertions(+), 3 deletions(-) >>> [...] >>> diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S >>> index c319116..2614cfc 100644 >>> --- a/arch/arm64/kvm/hyp-init.S >>> +++ b/arch/arm64/kvm/hyp-init.S >>> @@ -115,6 +115,38 @@ target: /* We're now in the trampoline code, switch page tables */ >>> eret >>> ENDPROC(__kvm_hyp_init) >>> >>> + /* >>> + * x0: HYP boot pgd >>> + * x1: HYP phys_idmap_start >>> + * x2: HYP stub vectors >>> + */ >>> +ENTRY(__kvm_hyp_reset) >>> + /* We're in trampoline code in VA, switch back to boot page tables */ >>> + msr ttbr0_el2, x0 >>> + isb >>> + >>> + /* Invalidate the old TLBs */ >>> + tlbi alle2 >>> + dsb sy >>> + >>> + /* Branch into PA space */ >>> + adr x0, 1f >>> + bfi x1, x0, #0, #PAGE_SHIFT >>> + br x1 >>> + >>> + /* We're now in idmap, disable MMU */ >>> +1: mrs x0, sctlr_el2 >>> + ldr x1, =SCTLR_EL2_FLAGS >>> + bic x0, x0, x1 // Clear SCTL_M and etc >>> + msr sctlr_el2, x0 >>> + isb >>> + >>> + /* Install stub vectors */ >>> + msr vbar_el2, x2 >> >> Instead of using a parameter, can't you just do >> >> adr x2, __hyp_stub_vectors >> msr vbar_el2, x2 >> >> ? I can't imagine a case where we don't want this behaviour, and this >> would slightly simplify the calling convention. > > Yeah, but it will also enforces kvm to be statically linked to the kernel. > (not module-capable) Is that OK? I don't think this enforces anything to be statically linked. You don't have to refer to the symbol from C code for module linking to work. You may have to use a combination of ADRP + ADD :lo12: to make sure you can reach the symbol (ADR is limited to +/- 1MB). [...] >>> -1: /* Default to HVC_CALL_HYP. */ >>> + /* jump into trampoline code */ >>> +1: cmp x18, #HVC_RESET >>> + b.ne 2f >>> + br x3 // no return >> >> Same here. If we're always jumping to the trampoline code, why do we >> have to pass it as a parameter? > > We need here a physical address of reset function. It seems easy to call > kvm_virt_to_trampoline() in C code. But, yes, we can also do it in asm. I think that would be a lot clearer as it will be self-contained. Thanks, M. -- Jazz is not dead. It just smells funny...
