* [RFC 0/4] arm64: kexec: fix kvm issue in kexec
@ 2015-03-23 11:53 AKASHI Takahiro
2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: AKASHI Takahiro @ 2015-03-23 11:53 UTC (permalink / raw)
To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland
Cc: linux-arm-kernel, linaro-kernel, geoff, kexec, linux-kernel,
AKASHI Takahiro, broonie, david.griego, christoffer.dall,
freddy77
This patch set addresses KVM issue described in Geoff's kexec patch set.
http://lists.infradead.org/pipermail/kexec/2015-March/013432.html
The basic approach here is to define a kvm tear-down function and add
a reboot hook to gracefully shutdown the 1st kernel. This way, kvm gets
free from kexec-specific cleanup and yet we allows future enhancement,
like cpu hotplug & building kvm as a module, based on tear-down function.
In this sense, patch #1 & #2 actually fix the problem, and #3 & #4 are
rather informative.
I confirmed that 1st kernel successfully shut down and 2nd kernel started
with the following messages:
kvm [1]: Using HYP init bounce page @8fa52f000
kvm [1]: interrupt-controller@2c02f000 IRQ6
kvm [1]: timer IRQ3
kvm [1]: Hyp mode initialized successfully
test target: Base fast model
version: kernel v4.0-rc4 + Geoff's kexec v8
I still have some concerns about the following points. Please let me know
if you have any comments:
1) Call kvm_cpu_reset() on non-boot cpus in reboot notifier
We don't have to do so in kexec-specific case. But the current code runs
the function on each cpu for safety since we use a general reboot hook.
2) Flush D$ in kvm_cpu_reset()
Currently doesn't do so because all the cpus are just going to shut down,
and we actually flush D$ on boot cpu in Geoff's cpu_reset().
3) Compatibility with arm implementation
Frediano is no longer working on this issue on arm as he left his
company. But my approach here is based on a generic interface and can be
applied to arm in a similar way.
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-February/322231.html
AKASHI Takahiro (4):
arm64: kvm: add a cpu tear-down function
arm64: kexec: fix kvm issue
arm64: kvm: add cpu reset hook for cpu hotplug
arm64: kvm: add cpu reset at module exit
arch/arm/kvm/arm.c | 54 +++++++++++++++++++++++++++++++++++++
arch/arm/kvm/mmu.c | 16 +++++++++++
arch/arm64/include/asm/kvm_asm.h | 3 +++
arch/arm64/include/asm/kvm_host.h | 11 +++++++-
arch/arm64/include/asm/kvm_mmu.h | 3 +++
arch/arm64/include/asm/virt.h | 11 ++++++++
arch/arm64/kvm/Kconfig | 1 -
arch/arm64/kvm/hyp-init.S | 41 ++++++++++++++++++++++++++++
arch/arm64/kvm/hyp.S | 29 +++++++++++++++++---
9 files changed, 163 insertions(+), 6 deletions(-)
--
1.7.9.5
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
^ permalink raw reply [flat|nested] 14+ messages in thread* [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-23 11:53 [RFC 0/4] arm64: kexec: fix kvm issue in kexec AKASHI Takahiro @ 2015-03-23 11:53 ` AKASHI Takahiro 2015-03-23 16:46 ` Geoff Levand 2015-03-24 10:00 ` Marc Zyngier 2015-03-23 11:53 ` [RFC 2/4] arm64: kexec: fix kvm issue AKASHI Takahiro ` (2 subsequent siblings) 3 siblings, 2 replies; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-23 11:53 UTC (permalink / raw) To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland Cc: linux-arm-kernel, linaro-kernel, geoff, kexec, linux-kernel, AKASHI Takahiro, broonie, david.griego, christoffer.dall, freddy77 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@linaro.org> --- arch/arm/kvm/arm.c | 18 ++++++++++++++++ arch/arm/kvm/mmu.c | 16 +++++++++++++++ arch/arm64/include/asm/kvm_asm.h | 3 +++ arch/arm64/include/asm/kvm_host.h | 10 +++++++++ arch/arm64/include/asm/kvm_mmu.h | 3 +++ arch/arm64/include/asm/virt.h | 11 ++++++++++ arch/arm64/kvm/hyp-init.S | 41 +++++++++++++++++++++++++++++++++++++ arch/arm64/kvm/hyp.S | 29 ++++++++++++++++++++++---- 8 files changed, 127 insertions(+), 4 deletions(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 5560f74..35c8bc0 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -897,6 +897,24 @@ static void cpu_init_hyp_mode(void *dummy) __cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr); } +void kvm_cpu_reset(void *dummy) +{ + phys_addr_t boot_pgd_ptr; + phys_addr_t phys_idmap_start; + unsigned long reset_func; + unsigned long vector_ptr; + + if (__hyp_get_vectors() == hyp_default_vectors) + return; + + reset_func = kvm_reset_func_entry(); + boot_pgd_ptr = kvm_mmu_get_boot_httbr(); + phys_idmap_start = kvm_get_idmap_start(); + vector_ptr = kvm_get_stub_vectors(); + __cpu_reset_hyp_mode(reset_func, + boot_pgd_ptr, phys_idmap_start, vector_ptr); +} + static int hyp_init_cpu_notify(struct notifier_block *self, unsigned long action, void *cpu) { diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c index 3e6859b..428f41c 100644 --- a/arch/arm/kvm/mmu.c +++ b/arch/arm/kvm/mmu.c @@ -1490,6 +1490,22 @@ phys_addr_t kvm_get_idmap_vector(void) return hyp_idmap_vector; } +phys_addr_t kvm_get_idmap_start(void) +{ + return hyp_idmap_start; +} + +phys_addr_t kvm_get_stub_vectors(void) +{ + return virt_to_phys(__hyp_stub_vectors); +} + +unsigned long kvm_reset_func_entry(void) +{ + /* VA of __kvm_hyp_reset in trampline code */ + return TRAMPOLINE_VA + (__kvm_hyp_reset - __hyp_idmap_text_start); +} + int kvm_mmu_init(void) { int err; diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h index 4f7310f..97ee2fc 100644 --- a/arch/arm64/include/asm/kvm_asm.h +++ b/arch/arm64/include/asm/kvm_asm.h @@ -116,8 +116,11 @@ struct kvm; struct kvm_vcpu; +extern char __hyp_stub_vectors[]; + extern char __kvm_hyp_init[]; extern char __kvm_hyp_init_end[]; +extern char __kvm_hyp_reset[]; extern char __kvm_hyp_vector[]; diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 8ac3c70..97f88fe 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -199,6 +199,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void); struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void); u64 kvm_call_hyp(void *hypfn, ...); +void kvm_call_reset(unsigned long reset_func, ...); void force_vm_exit(const cpumask_t *mask); void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); @@ -223,6 +224,15 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr, hyp_stack_ptr, vector_ptr); } +static inline void __cpu_reset_hyp_mode(unsigned long reset_func, + phys_addr_t boot_pgd_ptr, + phys_addr_t phys_idmap_start, + unsigned long stub_vector_ptr) +{ + kvm_call_reset(reset_func, boot_pgd_ptr, + phys_idmap_start, stub_vector_ptr); +} + struct vgic_sr_vectors { void *save_vgic; void *restore_vgic; diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h index 6458b53..36be582 100644 --- a/arch/arm64/include/asm/kvm_mmu.h +++ b/arch/arm64/include/asm/kvm_mmu.h @@ -96,6 +96,9 @@ void kvm_mmu_free_memory_caches(struct kvm_vcpu *vcpu); phys_addr_t kvm_mmu_get_httbr(void); phys_addr_t kvm_mmu_get_boot_httbr(void); phys_addr_t kvm_get_idmap_vector(void); +phys_addr_t kvm_get_idmap_start(void); +phys_addr_t kvm_get_stub_vectors(void); +unsigned long kvm_reset_func_entry(void); int kvm_mmu_init(void); void kvm_clear_hyp_idmap(void); diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h index 3070096..7fcd087 100644 --- a/arch/arm64/include/asm/virt.h +++ b/arch/arm64/include/asm/virt.h @@ -61,6 +61,17 @@ #define BOOT_CPU_MODE_EL1 (0xe11) #define BOOT_CPU_MODE_EL2 (0xe12) +/* + * HVC_RESET - Reset cpu in EL2 to initial state. + * + * @x0: entry address in trampoline code in va + * @x1: identical mapping page table in pa + * @x2: start address of identical mapping in pa + * @x3: initial stub vector in pa + */ + +#define HVC_RESET 5 + #ifndef __ASSEMBLY__ /* diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S index c319116..ca7e6bf 100644 --- a/arch/arm64/kvm/hyp-init.S +++ b/arch/arm64/kvm/hyp-init.S @@ -115,6 +115,47 @@ 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 */ + /* Invalidate the old TLBs */ + tlbi alle2 + dsb sy + + /* Switch back to boot page tables */ + msr ttbr0_el2, x0 + isb + + /* Branch into PA space */ + adr x0, 1f + bfi x1, x0, #0, #PAGE_SHIFT + br x1 + + /* We're now in idmap */ +1: /* Invalidate the old TLBs again */ + tlbi alle2 + dsb sy + + /* Disable MMU */ + mrs x0, sctlr_el2 + and x1, x0, #SCTLR_EL2_EE + orr x0, x0, x1 // preserve endianness of EL2 + ldr x1, =SCTLR_EL2_FLAGS + eor x1, x1, xzr + bic x0, x0, x1 // Clear SCTL_M and etc + msr sctlr_el2, x0 + isb + + /* Switch back to stub vectors */ + msr vbar_el2, x2 + + eret +ENDPROC(__kvm_hyp_reset) + .ltorg .popsection diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S index fd085ec..aee75f9 100644 --- a/arch/arm64/kvm/hyp.S +++ b/arch/arm64/kvm/hyp.S @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) ret ENDPROC(kvm_call_hyp) +ENTRY(kvm_call_reset) + hvc #HVC_RESET + ret +ENDPROC(kvm_call_reset) + .macro invalid_vector label, target .align 2 \label: @@ -1179,10 +1184,10 @@ el1_sync: // Guest trapped into EL2 cmp x18, #HVC_GET_VECTORS b.ne 1f mrs x0, vbar_el2 - b 2f - -1: /* Default to HVC_CALL_HYP. */ + b 3f +1: cmp x18, #HVC_CALL_HYP + b.ne 2f push lr, xzr /* @@ -1196,7 +1201,23 @@ el1_sync: // Guest trapped into EL2 blr lr pop lr, xzr -2: eret + b 3f + + /* + * shuffle the parameters and jump into trampline code. + */ +2: cmp x18, #HVC_RESET + b.ne 3f + + mov x18, x0 + mov x0, x1 + mov x1, x2 + mov x2, x3 + mov x3, x4 + br x18 + /* not reach here */ + +3: eret el1_trap: /* -- 1.7.9.5 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro @ 2015-03-23 16:46 ` Geoff Levand 2015-03-24 7:48 ` AKASHI Takahiro 2015-03-24 10:00 ` Marc Zyngier 1 sibling, 1 reply; 14+ messages in thread From: Geoff Levand @ 2015-03-23 16:46 UTC (permalink / raw) To: AKASHI Takahiro Cc: mark.rutland, linaro-kernel, linux-arm-kernel, marc.zyngier, catalin.marinas, will.deacon, linux-kernel, broonie, david.griego, kexec, christoffer.dall, freddy77 Hi Takahiro, On Mon, 2015-03-23 at 20:53 +0900, AKASHI Takahiro wrote: > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index 3e6859b..428f41c 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c ... > +phys_addr_t kvm_get_stub_vectors(void) > +{ > + return virt_to_phys(__hyp_stub_vectors); > +} The stub vectors are not part of KVM, but part of kernel, so to me a routine get_hyp_stub_vectors() with a prototype in asm/virt.h, then a definition in maybe kernel/process.c, or a new file kernel/virt.c makes more sense. > +unsigned long kvm_reset_func_entry(void) > +{ > + /* VA of __kvm_hyp_reset in trampline code */ > + return TRAMPOLINE_VA + (__kvm_hyp_reset - __hyp_idmap_text_start); > +} > + > int kvm_mmu_init(void) > { > int err; > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index 4f7310f..97ee2fc 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -116,8 +116,11 @@ > struct kvm; > struct kvm_vcpu; > > +extern char __hyp_stub_vectors[]; I think this should at least be in asm/virt.h, or better, have a get_hyp_stub_vectors(). > extern char __kvm_hyp_init[]; > extern char __kvm_hyp_init_end[]; > +extern char __kvm_hyp_reset[]; > > extern char __kvm_hyp_vector[]; > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8ac3c70..97f88fe 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -199,6 +199,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void); > struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void); > > u64 kvm_call_hyp(void *hypfn, ...); > +void kvm_call_reset(unsigned long reset_func, ...); kvm_call_reset() takes a fixed number of args, so we shouldn't have it as a variadic here. I think a variadic routine complicates things for my kvm_call_reset() suggestion below. > void force_vm_exit(const cpumask_t *mask); > void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); > > @@ -223,6 +224,15 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr, > hyp_stack_ptr, vector_ptr); > } > > +static inline void __cpu_reset_hyp_mode(unsigned long reset_func, > + phys_addr_t boot_pgd_ptr, > + phys_addr_t phys_idmap_start, > + unsigned long stub_vector_ptr) > + kvm_call_reset(reset_func, boot_pgd_ptr, > + phys_idmap_start, stub_vector_ptr); Why not switch the order of the args here to: kvm_call_reset(boot_pgd_ptr, phys_idmap_start, stub_vector_ptr, reset_func) This will eliminate the register shifting in the HVC_RESET hcall vector which becomes just 'br x3'. > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index fd085ec..aee75f9 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) > ret > ENDPROC(kvm_call_hyp) > > +ENTRY(kvm_call_reset) > + hvc #HVC_RESET > + ret > +ENDPROC(kvm_call_reset) > + > .macro invalid_vector label, target > .align 2 > \label: > @@ -1179,10 +1184,10 @@ el1_sync: // Guest trapped into EL2 > cmp x18, #HVC_GET_VECTORS > b.ne 1f > mrs x0, vbar_el2 > - b 2f > - > -1: /* Default to HVC_CALL_HYP. */ It seems you are deleting this comment and also removing the logic that makes HVC_CALL_HYP the default. > + b 3f > > +1: cmp x18, #HVC_CALL_HYP > + b.ne 2f > push lr, xzr > > /* > @@ -1196,7 +1201,23 @@ el1_sync: // Guest trapped into EL2 > blr lr > > pop lr, xzr > -2: eret > + b 3f > + > + /* > + * shuffle the parameters and jump into trampline code. > + */ > +2: cmp x18, #HVC_RESET > + b.ne 3f > + > + mov x18, x0 > + mov x0, x1 > + mov x1, x2 > + mov x2, x3 > + mov x3, x4 > + br x18 > + /* not reach here */ > + > +3: eret We don't need to change labels for each new hcall, I think just this is OK: cmp x18, #HVC_GET_VECTORS b.ne 1f mrs x0, vbar_el2 b 2f +1: cmp x18, #HVC_RESET + b.ne 1f + br x3 // No return 1: /* Default to HVC_CALL_HYP. */ ... -Geoff _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-23 16:46 ` Geoff Levand @ 2015-03-24 7:48 ` AKASHI Takahiro 0 siblings, 0 replies; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-24 7:48 UTC (permalink / raw) To: Geoff Levand Cc: mark.rutland, linaro-kernel, linux-arm-kernel, marc.zyngier, catalin.marinas, will.deacon, linux-kernel, broonie, david.griego, kexec, christoffer.dall, freddy77 Geoff, On 03/24/2015 01:46 AM, Geoff Levand wrote: > Hi Takahiro, > > On Mon, 2015-03-23 at 20:53 +0900, AKASHI Takahiro wrote: >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index 3e6859b..428f41c 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c > ... >> +phys_addr_t kvm_get_stub_vectors(void) >> +{ >> + return virt_to_phys(__hyp_stub_vectors); >> +} > > The stub vectors are not part of KVM, but part of kernel, > so to me a routine get_hyp_stub_vectors() with a prototype > in asm/virt.h, then a definition in maybe > kernel/process.c, or a new file kernel/virt.c makes more > sense. Right. Will rename the function to get_hyp_stub_vectors() and put it in asm/virt.h. >> +unsigned long kvm_reset_func_entry(void) >> +{ >> + /* VA of __kvm_hyp_reset in trampline code */ >> + return TRAMPOLINE_VA + (__kvm_hyp_reset - __hyp_idmap_text_start); >> +} >> + >> int kvm_mmu_init(void) >> { >> int err; >> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h >> index 4f7310f..97ee2fc 100644 >> --- a/arch/arm64/include/asm/kvm_asm.h >> +++ b/arch/arm64/include/asm/kvm_asm.h >> @@ -116,8 +116,11 @@ >> struct kvm; >> struct kvm_vcpu; >> >> +extern char __hyp_stub_vectors[]; > > I think this should at least be in asm/virt.h, or better, > have a get_hyp_stub_vectors(). See above. >> extern char __kvm_hyp_init[]; >> extern char __kvm_hyp_init_end[]; >> +extern char __kvm_hyp_reset[]; >> >> extern char __kvm_hyp_vector[]; >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 8ac3c70..97f88fe 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -199,6 +199,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void); >> struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void); >> >> u64 kvm_call_hyp(void *hypfn, ...); >> +void kvm_call_reset(unsigned long reset_func, ...); > > kvm_call_reset() takes a fixed number of args, so we shouldn't > have it as a variadic here. I think a variadic routine > complicates things for my kvm_call_reset() suggestion below. > >> void force_vm_exit(const cpumask_t *mask); >> void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); >> >> @@ -223,6 +224,15 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr, >> hyp_stack_ptr, vector_ptr); >> } >> >> +static inline void __cpu_reset_hyp_mode(unsigned long reset_func, >> + phys_addr_t boot_pgd_ptr, >> + phys_addr_t phys_idmap_start, >> + unsigned long stub_vector_ptr) > >> + kvm_call_reset(reset_func, boot_pgd_ptr, >> + phys_idmap_start, stub_vector_ptr); > > Why not switch the order of the args here to: > > kvm_call_reset(boot_pgd_ptr, phys_idmap_start, stub_vector_ptr, reset_func) > > This will eliminate the register shifting in the HVC_RESET > hcall vector which becomes just 'br x3'. Looks nice. FYI, initially I wanted to implement kvm_cpu_reset() using kvm_call_hyp() and so both have similar code. > >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index fd085ec..aee75f9 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) >> ret >> ENDPROC(kvm_call_hyp) >> >> +ENTRY(kvm_call_reset) >> + hvc #HVC_RESET >> + ret >> +ENDPROC(kvm_call_reset) >> + >> .macro invalid_vector label, target >> .align 2 >> \label: >> @@ -1179,10 +1184,10 @@ el1_sync: // Guest trapped into EL2 >> cmp x18, #HVC_GET_VECTORS >> b.ne 1f >> mrs x0, vbar_el2 >> - b 2f >> - >> -1: /* Default to HVC_CALL_HYP. */ > > It seems you are deleting this comment and also > removing the logic that makes HVC_CALL_HYP the default. Yeah, I didn't think of that. But IIUC, we don't have to handle it as default case because all interfaces come from kvm_call_hyp() once KVM is initialized. >> + b 3f >> >> +1: cmp x18, #HVC_CALL_HYP >> + b.ne 2f >> push lr, xzr >> >> /* >> @@ -1196,7 +1201,23 @@ el1_sync: // Guest trapped into EL2 >> blr lr >> >> pop lr, xzr >> -2: eret >> + b 3f >> + >> + /* >> + * shuffle the parameters and jump into trampline code. >> + */ >> +2: cmp x18, #HVC_RESET >> + b.ne 3f >> + >> + mov x18, x0 >> + mov x0, x1 >> + mov x1, x2 >> + mov x2, x3 >> + mov x3, x4 >> + br x18 >> + /* not reach here */ >> + >> +3: eret > > We don't need to change labels for each new hcall, I > think just this is OK: > > cmp x18, #HVC_GET_VECTORS > b.ne 1f > mrs x0, vbar_el2 > b 2f > > +1: cmp x18, #HVC_RESET > + b.ne 1f > + br x3 // No return > > 1: /* Default to HVC_CALL_HYP. */ > ... Will fix it. Thanks, -Takahiro AKASHI > -Geoff > > > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro 2015-03-23 16:46 ` Geoff Levand @ 2015-03-24 10:00 ` Marc Zyngier 2015-03-25 8:06 ` AKASHI Takahiro 1 sibling, 1 reply; 14+ messages in thread From: Marc Zyngier @ 2015-03-24 10:00 UTC (permalink / raw) To: AKASHI Takahiro, Catalin Marinas, Will Deacon, Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linaro-kernel@lists.linaro.org, geoff@infradead.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, broonie@kernel.org, david.griego@linaro.org, christoffer.dall@linaro.org, freddy77@gmail.com Hi Takahiro, On 23/03/15 11:53, 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. Thanks for having a look at this. > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> > --- > arch/arm/kvm/arm.c | 18 ++++++++++++++++ > arch/arm/kvm/mmu.c | 16 +++++++++++++++ > arch/arm64/include/asm/kvm_asm.h | 3 +++ > arch/arm64/include/asm/kvm_host.h | 10 +++++++++ > arch/arm64/include/asm/kvm_mmu.h | 3 +++ > arch/arm64/include/asm/virt.h | 11 ++++++++++ > arch/arm64/kvm/hyp-init.S | 41 +++++++++++++++++++++++++++++++++++++ > arch/arm64/kvm/hyp.S | 29 ++++++++++++++++++++++---- > 8 files changed, 127 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index 5560f74..35c8bc0 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -897,6 +897,24 @@ static void cpu_init_hyp_mode(void *dummy) > __cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr); > } > > +void kvm_cpu_reset(void *dummy) Given that the notifier introduced in patch #2 lives in the same file, this could be made static, couldn't it? > +{ > + phys_addr_t boot_pgd_ptr; > + phys_addr_t phys_idmap_start; > + unsigned long reset_func; > + unsigned long vector_ptr; > + > + if (__hyp_get_vectors() == hyp_default_vectors) > + return; > + > + reset_func = kvm_reset_func_entry(); > + boot_pgd_ptr = kvm_mmu_get_boot_httbr(); > + phys_idmap_start = kvm_get_idmap_start(); > + vector_ptr = kvm_get_stub_vectors(); Isn't that hyp_default_vectors already? > + __cpu_reset_hyp_mode(reset_func, > + boot_pgd_ptr, phys_idmap_start, vector_ptr); > +} > + > static int hyp_init_cpu_notify(struct notifier_block *self, > unsigned long action, void *cpu) > { > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c > index 3e6859b..428f41c 100644 > --- a/arch/arm/kvm/mmu.c > +++ b/arch/arm/kvm/mmu.c > @@ -1490,6 +1490,22 @@ phys_addr_t kvm_get_idmap_vector(void) > return hyp_idmap_vector; > } > > +phys_addr_t kvm_get_idmap_start(void) > +{ > + return hyp_idmap_start; > +} > + > +phys_addr_t kvm_get_stub_vectors(void) > +{ > + return virt_to_phys(__hyp_stub_vectors); > +} As Geoff already mentioned, this doesn't belong in KVM. But I don't think you need it, as explained above. > + > +unsigned long kvm_reset_func_entry(void) > +{ > + /* VA of __kvm_hyp_reset in trampline code */ > + return TRAMPOLINE_VA + (__kvm_hyp_reset - __hyp_idmap_text_start); > +} If you need to compute addresses in the trampoline page, it would be better to have a generic macro that takes a kernel VA, and turns it into a trampoline VA. > + > int kvm_mmu_init(void) > { > int err; > diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h > index 4f7310f..97ee2fc 100644 > --- a/arch/arm64/include/asm/kvm_asm.h > +++ b/arch/arm64/include/asm/kvm_asm.h > @@ -116,8 +116,11 @@ > struct kvm; > struct kvm_vcpu; > > +extern char __hyp_stub_vectors[]; Not a KVM thing (for the same reason). > + > extern char __kvm_hyp_init[]; > extern char __kvm_hyp_init_end[]; > +extern char __kvm_hyp_reset[]; > > extern char __kvm_hyp_vector[]; > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8ac3c70..97f88fe 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -199,6 +199,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void); > struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void); > > u64 kvm_call_hyp(void *hypfn, ...); > +void kvm_call_reset(unsigned long reset_func, ...); You seem to have a single call signature for this function, and have defined all the parameters below. You can drop the variadic aspect. > void force_vm_exit(const cpumask_t *mask); > void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); > > @@ -223,6 +224,15 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr, > hyp_stack_ptr, vector_ptr); > } > > +static inline void __cpu_reset_hyp_mode(unsigned long reset_func, > + phys_addr_t boot_pgd_ptr, > + phys_addr_t phys_idmap_start, > + unsigned long stub_vector_ptr) > +{ > + kvm_call_reset(reset_func, boot_pgd_ptr, > + phys_idmap_start, stub_vector_ptr); > +} > + > struct vgic_sr_vectors { > void *save_vgic; > void *restore_vgic; > diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h > index 6458b53..36be582 100644 > --- a/arch/arm64/include/asm/kvm_mmu.h > +++ b/arch/arm64/include/asm/kvm_mmu.h > @@ -96,6 +96,9 @@ void kvm_mmu_free_memory_caches(struct kvm_vcpu *vcpu); > phys_addr_t kvm_mmu_get_httbr(void); > phys_addr_t kvm_mmu_get_boot_httbr(void); > phys_addr_t kvm_get_idmap_vector(void); > +phys_addr_t kvm_get_idmap_start(void); > +phys_addr_t kvm_get_stub_vectors(void); > +unsigned long kvm_reset_func_entry(void); > int kvm_mmu_init(void); > void kvm_clear_hyp_idmap(void); > > diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h > index 3070096..7fcd087 100644 > --- a/arch/arm64/include/asm/virt.h > +++ b/arch/arm64/include/asm/virt.h > @@ -61,6 +61,17 @@ > #define BOOT_CPU_MODE_EL1 (0xe11) > #define BOOT_CPU_MODE_EL2 (0xe12) > > +/* > + * HVC_RESET - Reset cpu in EL2 to initial state. > + * > + * @x0: entry address in trampoline code in va > + * @x1: identical mapping page table in pa > + * @x2: start address of identical mapping in pa > + * @x3: initial stub vector in pa > + */ > + > +#define HVC_RESET 5 > + > #ifndef __ASSEMBLY__ > > /* > diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S > index c319116..ca7e6bf 100644 > --- a/arch/arm64/kvm/hyp-init.S > +++ b/arch/arm64/kvm/hyp-init.S > @@ -115,6 +115,47 @@ 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 */ > + /* Invalidate the old TLBs */ > + tlbi alle2 > + dsb sy Invalidating the TLBs before switching TTBR0_EL2 is unlikely to have the effect you want. > + /* Switch back to boot page tables */ > + msr ttbr0_el2, x0 > + isb This is the place where you want TLBI to occur. > + /* Branch into PA space */ > + adr x0, 1f > + bfi x1, x0, #0, #PAGE_SHIFT > + br x1 > + > + /* We're now in idmap */ > +1: /* Invalidate the old TLBs again */ > + tlbi alle2 > + dsb sy See? This is the only TLBI that actually makes sense. Now, given that you are actually disabling the MMU, I'm not sure these TBLIs make much sense. > + /* Disable MMU */ > + mrs x0, sctlr_el2 > + and x1, x0, #SCTLR_EL2_EE > + orr x0, x0, x1 // preserve endianness of EL2 > + ldr x1, =SCTLR_EL2_FLAGS > + eor x1, x1, xzr > + bic x0, x0, x1 // Clear SCTL_M and etc > + msr sctlr_el2, x0 > + isb > + > + /* Switch back to stub vectors */ > + msr vbar_el2, x2 > + > + eret > +ENDPROC(__kvm_hyp_reset) > + > .ltorg > > .popsection > diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S > index fd085ec..aee75f9 100644 > --- a/arch/arm64/kvm/hyp.S > +++ b/arch/arm64/kvm/hyp.S > @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) > ret > ENDPROC(kvm_call_hyp) > > +ENTRY(kvm_call_reset) > + hvc #HVC_RESET > + ret > +ENDPROC(kvm_call_reset) > + > .macro invalid_vector label, target > .align 2 > \label: > @@ -1179,10 +1184,10 @@ el1_sync: // Guest trapped into EL2 > cmp x18, #HVC_GET_VECTORS > b.ne 1f > mrs x0, vbar_el2 > - b 2f > - > -1: /* Default to HVC_CALL_HYP. */ > + b 3f > > +1: cmp x18, #HVC_CALL_HYP > + b.ne 2f > push lr, xzr > > /* > @@ -1196,7 +1201,23 @@ el1_sync: // Guest trapped into EL2 > blr lr > > pop lr, xzr > -2: eret > + b 3f > + > + /* > + * shuffle the parameters and jump into trampline code. > + */ > +2: cmp x18, #HVC_RESET > + b.ne 3f > + > + mov x18, x0 > + mov x0, x1 > + mov x1, x2 > + mov x2, x3 > + mov x3, x4 > + br x18 I'd rather move the shuffling of the registers before the HVC call. > + /* not reach here */ > + > +3: eret > > el1_trap: > /* > Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-24 10:00 ` Marc Zyngier @ 2015-03-25 8:06 ` AKASHI Takahiro 2015-03-25 9:48 ` Marc Zyngier 0 siblings, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-25 8:06 UTC (permalink / raw) To: Marc Zyngier, Catalin Marinas, Will Deacon, Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linaro-kernel@lists.linaro.org, geoff@infradead.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, broonie@kernel.org, david.griego@linaro.org, christoffer.dall@linaro.org, freddy77@gmail.com Marc, On 03/24/2015 07:00 PM, Marc Zyngier wrote: > Hi Takahiro, > > On 23/03/15 11:53, 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. > > Thanks for having a look at this. > >> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> >> --- >> arch/arm/kvm/arm.c | 18 ++++++++++++++++ >> arch/arm/kvm/mmu.c | 16 +++++++++++++++ >> arch/arm64/include/asm/kvm_asm.h | 3 +++ >> arch/arm64/include/asm/kvm_host.h | 10 +++++++++ >> arch/arm64/include/asm/kvm_mmu.h | 3 +++ >> arch/arm64/include/asm/virt.h | 11 ++++++++++ >> arch/arm64/kvm/hyp-init.S | 41 +++++++++++++++++++++++++++++++++++++ >> arch/arm64/kvm/hyp.S | 29 ++++++++++++++++++++++---- >> 8 files changed, 127 insertions(+), 4 deletions(-) >> >> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c >> index 5560f74..35c8bc0 100644 >> --- a/arch/arm/kvm/arm.c >> +++ b/arch/arm/kvm/arm.c >> @@ -897,6 +897,24 @@ static void cpu_init_hyp_mode(void *dummy) >> __cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr); >> } >> >> +void kvm_cpu_reset(void *dummy) > > Given that the notifier introduced in patch #2 lives in the same file, > this could be made static, couldn't it? Yes. Will fix it. >> +{ >> + phys_addr_t boot_pgd_ptr; >> + phys_addr_t phys_idmap_start; >> + unsigned long reset_func; >> + unsigned long vector_ptr; >> + >> + if (__hyp_get_vectors() == hyp_default_vectors) >> + return; >> + >> + reset_func = kvm_reset_func_entry(); >> + boot_pgd_ptr = kvm_mmu_get_boot_httbr(); >> + phys_idmap_start = kvm_get_idmap_start(); >> + vector_ptr = kvm_get_stub_vectors(); > > Isn't that hyp_default_vectors already? Yeah, I already use it in kvm_cpu_reset(). Will fix it. >> + __cpu_reset_hyp_mode(reset_func, >> + boot_pgd_ptr, phys_idmap_start, vector_ptr); >> +} >> + >> static int hyp_init_cpu_notify(struct notifier_block *self, >> unsigned long action, void *cpu) >> { >> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c >> index 3e6859b..428f41c 100644 >> --- a/arch/arm/kvm/mmu.c >> +++ b/arch/arm/kvm/mmu.c >> @@ -1490,6 +1490,22 @@ phys_addr_t kvm_get_idmap_vector(void) >> return hyp_idmap_vector; >> } >> >> +phys_addr_t kvm_get_idmap_start(void) >> +{ >> + return hyp_idmap_start; >> +} >> + >> +phys_addr_t kvm_get_stub_vectors(void) >> +{ >> + return virt_to_phys(__hyp_stub_vectors); >> +} > > As Geoff already mentioned, this doesn't belong in KVM. But I don't > think you need it, as explained above. Will remove it. >> + >> +unsigned long kvm_reset_func_entry(void) >> +{ >> + /* VA of __kvm_hyp_reset in trampline code */ >> + return TRAMPOLINE_VA + (__kvm_hyp_reset - __hyp_idmap_text_start); >> +} > > If you need to compute addresses in the trampoline page, it would be > better to have a generic macro that takes a kernel VA, and turns it into > a trampoline VA. This is the only place to calculate the address. But will fix it. >> + >> int kvm_mmu_init(void) >> { >> int err; >> diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h >> index 4f7310f..97ee2fc 100644 >> --- a/arch/arm64/include/asm/kvm_asm.h >> +++ b/arch/arm64/include/asm/kvm_asm.h >> @@ -116,8 +116,11 @@ >> struct kvm; >> struct kvm_vcpu; >> >> +extern char __hyp_stub_vectors[]; > > Not a KVM thing (for the same reason). Will remove it. >> + >> extern char __kvm_hyp_init[]; >> extern char __kvm_hyp_init_end[]; >> +extern char __kvm_hyp_reset[]; >> >> extern char __kvm_hyp_vector[]; >> >> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >> index 8ac3c70..97f88fe 100644 >> --- a/arch/arm64/include/asm/kvm_host.h >> +++ b/arch/arm64/include/asm/kvm_host.h >> @@ -199,6 +199,7 @@ struct kvm_vcpu *kvm_arm_get_running_vcpu(void); >> struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void); >> >> u64 kvm_call_hyp(void *hypfn, ...); >> +void kvm_call_reset(unsigned long reset_func, ...); > > You seem to have a single call signature for this function, and have > defined all the parameters below. You can drop the variadic aspect. As Geoff suggested, the arguments of __cpu_reset_hyp_mode() and kvm_call_reset() will be shuffled as in this order: phys_addr_t boot_pgd_ptr, phys_addr_t phys_idmap_start, unsigned long stub_vector_ptr unsigned long reset_func. >> void force_vm_exit(const cpumask_t *mask); >> void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot); >> >> @@ -223,6 +224,15 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr, >> hyp_stack_ptr, vector_ptr); >> } >> >> +static inline void __cpu_reset_hyp_mode(unsigned long reset_func, >> + phys_addr_t boot_pgd_ptr, >> + phys_addr_t phys_idmap_start, >> + unsigned long stub_vector_ptr) >> +{ >> + kvm_call_reset(reset_func, boot_pgd_ptr, >> + phys_idmap_start, stub_vector_ptr); >> +} >> + >> struct vgic_sr_vectors { >> void *save_vgic; >> void *restore_vgic; >> diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h >> index 6458b53..36be582 100644 >> --- a/arch/arm64/include/asm/kvm_mmu.h >> +++ b/arch/arm64/include/asm/kvm_mmu.h >> @@ -96,6 +96,9 @@ void kvm_mmu_free_memory_caches(struct kvm_vcpu *vcpu); >> phys_addr_t kvm_mmu_get_httbr(void); >> phys_addr_t kvm_mmu_get_boot_httbr(void); >> phys_addr_t kvm_get_idmap_vector(void); >> +phys_addr_t kvm_get_idmap_start(void); >> +phys_addr_t kvm_get_stub_vectors(void); >> +unsigned long kvm_reset_func_entry(void); >> int kvm_mmu_init(void); >> void kvm_clear_hyp_idmap(void); >> >> diff --git a/arch/arm64/include/asm/virt.h b/arch/arm64/include/asm/virt.h >> index 3070096..7fcd087 100644 >> --- a/arch/arm64/include/asm/virt.h >> +++ b/arch/arm64/include/asm/virt.h >> @@ -61,6 +61,17 @@ >> #define BOOT_CPU_MODE_EL1 (0xe11) >> #define BOOT_CPU_MODE_EL2 (0xe12) >> >> +/* >> + * HVC_RESET - Reset cpu in EL2 to initial state. >> + * >> + * @x0: entry address in trampoline code in va >> + * @x1: identical mapping page table in pa >> + * @x2: start address of identical mapping in pa >> + * @x3: initial stub vector in pa >> + */ >> + >> +#define HVC_RESET 5 >> + >> #ifndef __ASSEMBLY__ >> >> /* >> diff --git a/arch/arm64/kvm/hyp-init.S b/arch/arm64/kvm/hyp-init.S >> index c319116..ca7e6bf 100644 >> --- a/arch/arm64/kvm/hyp-init.S >> +++ b/arch/arm64/kvm/hyp-init.S >> @@ -115,6 +115,47 @@ 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 */ >> + /* Invalidate the old TLBs */ >> + tlbi alle2 >> + dsb sy > > Invalidating the TLBs before switching TTBR0_EL2 is unlikely to have the > effect you want. > >> + /* Switch back to boot page tables */ >> + msr ttbr0_el2, x0 >> + isb > > This is the place where you want TLBI to occur. Will remove tlbi above and put it here. >> + /* Branch into PA space */ >> + adr x0, 1f >> + bfi x1, x0, #0, #PAGE_SHIFT >> + br x1 >> + >> + /* We're now in idmap */ >> +1: /* Invalidate the old TLBs again */ >> + tlbi alle2 >> + dsb sy > > See? This is the only TLBI that actually makes sense. Now, given that > you are actually disabling the MMU, I'm not sure these TBLIs make much > sense. Probably you're right, but otherwise, I guess, bogus TLB might remain and be used when MMU get enabled again. (MMU setting would be the same across disabling/enabling hyp mode though.) >> + /* Disable MMU */ >> + mrs x0, sctlr_el2 >> + and x1, x0, #SCTLR_EL2_EE >> + orr x0, x0, x1 // preserve endianness of EL2 >> + ldr x1, =SCTLR_EL2_FLAGS >> + eor x1, x1, xzr >> + bic x0, x0, x1 // Clear SCTL_M and etc >> + msr sctlr_el2, x0 >> + isb >> + >> + /* Switch back to stub vectors */ >> + msr vbar_el2, x2 >> + >> + eret >> +ENDPROC(__kvm_hyp_reset) >> + >> .ltorg >> >> .popsection >> diff --git a/arch/arm64/kvm/hyp.S b/arch/arm64/kvm/hyp.S >> index fd085ec..aee75f9 100644 >> --- a/arch/arm64/kvm/hyp.S >> +++ b/arch/arm64/kvm/hyp.S >> @@ -1136,6 +1136,11 @@ ENTRY(kvm_call_hyp) >> ret >> ENDPROC(kvm_call_hyp) >> >> +ENTRY(kvm_call_reset) >> + hvc #HVC_RESET >> + ret >> +ENDPROC(kvm_call_reset) >> + >> .macro invalid_vector label, target >> .align 2 >> \label: >> @@ -1179,10 +1184,10 @@ el1_sync: // Guest trapped into EL2 >> cmp x18, #HVC_GET_VECTORS >> b.ne 1f >> mrs x0, vbar_el2 >> - b 2f >> - >> -1: /* Default to HVC_CALL_HYP. */ >> + b 3f >> >> +1: cmp x18, #HVC_CALL_HYP >> + b.ne 2f >> push lr, xzr >> >> /* >> @@ -1196,7 +1201,23 @@ el1_sync: // Guest trapped into EL2 >> blr lr >> >> pop lr, xzr >> -2: eret >> + b 3f >> + >> + /* >> + * shuffle the parameters and jump into trampline code. >> + */ >> +2: cmp x18, #HVC_RESET >> + b.ne 3f >> + >> + mov x18, x0 >> + mov x0, x1 >> + mov x1, x2 >> + mov x2, x3 >> + mov x3, x4 >> + br x18 > > I'd rather move the shuffling of the registers before the HVC call. See above. Thanks, -Takahiro AKASHI >> + /* not reach here */ >> + >> +3: eret >> >> el1_trap: >> /* >> > > Thanks, > > M. > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 1/4] arm64: kvm: add a cpu tear-down function 2015-03-25 8:06 ` AKASHI Takahiro @ 2015-03-25 9:48 ` Marc Zyngier 0 siblings, 0 replies; 14+ messages in thread From: Marc Zyngier @ 2015-03-25 9:48 UTC (permalink / raw) To: AKASHI Takahiro, Catalin Marinas, Will Deacon, Mark Rutland Cc: linux-arm-kernel@lists.infradead.org, linaro-kernel@lists.linaro.org, geoff@infradead.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, broonie@kernel.org, david.griego@linaro.org, christoffer.dall@linaro.org, freddy77@gmail.com Hi Takahiro, On 25/03/15 08:06, AKASHI Takahiro wrote: >>> + /* Switch back to boot page tables */ >>> + msr ttbr0_el2, x0 >>> + isb >> >> This is the place where you want TLBI to occur. > > Will remove tlbi above and put it here. There is only need for one TLBI, if at all. >>> + /* Branch into PA space */ >>> + adr x0, 1f >>> + bfi x1, x0, #0, #PAGE_SHIFT >>> + br x1 >>> + >>> + /* We're now in idmap */ >>> +1: /* Invalidate the old TLBs again */ >>> + tlbi alle2 >>> + dsb sy >> >> See? This is the only TLBI that actually makes sense. Now, given that >> you are actually disabling the MMU, I'm not sure these TBLIs make much >> sense. > > Probably you're right, but > otherwise, I guess, bogus TLB might remain and be used when MMU get enabled again. > (MMU setting would be the same across disabling/enabling hyp mode though.) Anyone enabling the MMU must invalidate the TLB before doing so (we've been caught by that before). Invalidation on the way out doesn't hurt, but it also give a false sense of security. I'll leave it up to you. Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC 2/4] arm64: kexec: fix kvm issue 2015-03-23 11:53 [RFC 0/4] arm64: kexec: fix kvm issue in kexec AKASHI Takahiro 2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro @ 2015-03-23 11:53 ` AKASHI Takahiro 2015-03-23 15:56 ` Geoff Levand 2015-03-23 11:53 ` [RFC 3/4] arm64: kvm: add cpu reset hook for cpu hotplug AKASHI Takahiro 2015-03-23 11:53 ` [RFC 4/4] arm64: kvm: add cpu reset at module exit AKASHI Takahiro 3 siblings, 1 reply; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-23 11:53 UTC (permalink / raw) To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland Cc: linux-arm-kernel, linaro-kernel, geoff, kexec, linux-kernel, AKASHI Takahiro, broonie, david.griego, christoffer.dall, freddy77 The current kvm implementation keeps EL2 vector table installed even when the system is shut down. This prevents kexec from putting the system with kvm back into EL2 when starting a new kernel. This patch resolves this issue by calling a cpu tear-down function via reboot notifier, kvm_reboot_notify(), which is invoked by kernel_restart_prepare() in kernel_kexec(). While kvm has a generic hook, kvm_reboot(), we can't use it here because a cpu teardown function will not be invoked, under current implementation, if no guest vm has been created by kvm_create_vm(). Please note that kvm_usage_count is zero in this case. We'd better, in the future, implement cpu hotplug support and put the arch-specific initialization into kvm_arch_hardware_enable/disable(). This way, we would be able to revert this patch. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm/kvm/arm.c | 21 +++++++++++++++++++++ arch/arm64/kvm/Kconfig | 1 - 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 35c8bc0..b879cf6 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -25,6 +25,7 @@ #include <linux/vmalloc.h> #include <linux/fs.h> #include <linux/mman.h> +#include <linux/reboot.h> #include <linux/sched.h> #include <linux/kvm.h> #include <trace/events/kvm.h> @@ -1103,6 +1104,23 @@ struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr) return NULL; } +static int kvm_reboot_notify(struct notifier_block *nb, + unsigned long val, void *v) +{ + /* + * Reset each CPU in EL2 to initial state. + */ + on_each_cpu(kvm_cpu_reset, NULL, 1); + + return NOTIFY_DONE; +} + +static struct notifier_block kvm_reboot_nb = { + .notifier_call = kvm_reboot_notify, + .next = NULL, + .priority = 0, /* FIXME */ +}; + /** * Initialize Hyp-mode and memory mappings on all CPUs. */ @@ -1141,6 +1159,9 @@ int kvm_arch_init(void *opaque) hyp_cpu_pm_init(); kvm_coproc_table_init(); + + register_reboot_notifier(&kvm_reboot_nb); + return 0; out_err: cpu_notifier_register_done(); diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index 30ae7a7..f5590c8 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -18,7 +18,6 @@ if VIRTUALIZATION config KVM bool "Kernel-based Virtual Machine (KVM) support" - depends on !KEXEC select MMU_NOTIFIER select PREEMPT_NOTIFIERS select ANON_INODES -- 1.7.9.5 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC 2/4] arm64: kexec: fix kvm issue 2015-03-23 11:53 ` [RFC 2/4] arm64: kexec: fix kvm issue AKASHI Takahiro @ 2015-03-23 15:56 ` Geoff Levand 2015-03-24 7:52 ` AKASHI Takahiro 2015-03-24 8:46 ` Marc Zyngier 0 siblings, 2 replies; 14+ messages in thread From: Geoff Levand @ 2015-03-23 15:56 UTC (permalink / raw) To: AKASHI Takahiro Cc: mark.rutland, linaro-kernel, linux-arm-kernel, marc.zyngier, catalin.marinas, will.deacon, linux-kernel, broonie, david.griego, kexec, christoffer.dall, freddy77 On Mon, 2015-03-23 at 20:53 +0900, AKASHI Takahiro wrote: > The current kvm implementation keeps EL2 vector table installed even > when the system is shut down. This prevents kexec from putting the system > with kvm back into EL2 when starting a new kernel. This is a kvm bug fix, so I think the subject should be something like 'arm64/kvm: Fix shutdown issue'. -Geoff _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 2/4] arm64: kexec: fix kvm issue 2015-03-23 15:56 ` Geoff Levand @ 2015-03-24 7:52 ` AKASHI Takahiro 2015-03-24 8:46 ` Marc Zyngier 1 sibling, 0 replies; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-24 7:52 UTC (permalink / raw) To: Geoff Levand Cc: mark.rutland, linaro-kernel, linux-arm-kernel, marc.zyngier, catalin.marinas, will.deacon, linux-kernel, broonie, david.griego, kexec, christoffer.dall, freddy77 On 03/24/2015 12:56 AM, Geoff Levand wrote: > On Mon, 2015-03-23 at 20:53 +0900, AKASHI Takahiro wrote: >> The current kvm implementation keeps EL2 vector table installed even >> when the system is shut down. This prevents kexec from putting the system >> with kvm back into EL2 when starting a new kernel. > > This is a kvm bug fix, so I think the subject should > be something like 'arm64/kvm: Fix shutdown issue'. Yeah, "arm64: kvm: fix shutdown issue in kexec" -Takahiro AKASHI > -Geoff > > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 2/4] arm64: kexec: fix kvm issue 2015-03-23 15:56 ` Geoff Levand 2015-03-24 7:52 ` AKASHI Takahiro @ 2015-03-24 8:46 ` Marc Zyngier 2015-03-24 16:56 ` Geoff Levand 1 sibling, 1 reply; 14+ messages in thread From: Marc Zyngier @ 2015-03-24 8:46 UTC (permalink / raw) To: Geoff Levand, AKASHI Takahiro Cc: Mark Rutland, linaro-kernel@lists.linaro.org, linux-arm-kernel@lists.infradead.org, Catalin Marinas, Will Deacon, linux-kernel@vger.kernel.org, broonie@kernel.org, david.griego@linaro.org, kexec@lists.infradead.org, christoffer.dall@linaro.org, freddy77@gmail.com On 23/03/15 15:56, Geoff Levand wrote: > On Mon, 2015-03-23 at 20:53 +0900, AKASHI Takahiro wrote: >> The current kvm implementation keeps EL2 vector table installed even >> when the system is shut down. This prevents kexec from putting the system >> with kvm back into EL2 when starting a new kernel. > > This is a kvm bug fix, so I think the subject should > be something like 'arm64/kvm: Fix shutdown issue'. Not quite. On its own, this doesn't fix anything in KVM. It simply plugs a deficiency in the arm64 kexec implementation. If you want to be completely true to the content of the patch, it should read: "arm64: KVM: Allow EL2 context to be reset on shutdown" Can we now drop the blame game and get back to the actual code? Thanks, M. -- Jazz is not dead. It just smells funny... _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC 2/4] arm64: kexec: fix kvm issue 2015-03-24 8:46 ` Marc Zyngier @ 2015-03-24 16:56 ` Geoff Levand 0 siblings, 0 replies; 14+ messages in thread From: Geoff Levand @ 2015-03-24 16:56 UTC (permalink / raw) To: Marc Zyngier Cc: Mark Rutland, linaro-kernel@lists.linaro.org, linux-arm-kernel@lists.infradead.org, Catalin Marinas, Will Deacon, linux-kernel@vger.kernel.org, AKASHI Takahiro, broonie@kernel.org, david.griego@linaro.org, kexec@lists.infradead.org, christoffer.dall@linaro.org, freddy77@gmail.com Hi Marc, On Tue, 2015-03-24 at 08:46 +0000, Marc Zyngier wrote: > "arm64: KVM: Allow EL2 context to be reset on shutdown" Sure, this sounds good. -Geoff _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC 3/4] arm64: kvm: add cpu reset hook for cpu hotplug 2015-03-23 11:53 [RFC 0/4] arm64: kexec: fix kvm issue in kexec AKASHI Takahiro 2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro 2015-03-23 11:53 ` [RFC 2/4] arm64: kexec: fix kvm issue AKASHI Takahiro @ 2015-03-23 11:53 ` AKASHI Takahiro 2015-03-23 11:53 ` [RFC 4/4] arm64: kvm: add cpu reset at module exit AKASHI Takahiro 3 siblings, 0 replies; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-23 11:53 UTC (permalink / raw) To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland Cc: linux-arm-kernel, linaro-kernel, geoff, kexec, linux-kernel, AKASHI Takahiro, broonie, david.griego, christoffer.dall, freddy77 This patch doesn't enable cpu hotplug under kvm, but is a prerequiste when the feature is implemented. Once kvm_arch_hardware_enable/disable() is properly implemented, arm64-specific cpu notifier hook, hyp_init_cpu_notify(), will be able to be removed and replaced by generic kvm_cpu_hotplug(). Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm/kvm/arm.c | 9 +++++++++ arch/arm64/include/asm/kvm_host.h | 1 - 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index b879cf6..08804d3 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -925,6 +925,10 @@ static int hyp_init_cpu_notify(struct notifier_block *self, if (__hyp_get_vectors() == hyp_default_vectors) cpu_init_hyp_mode(NULL); break; + case CPU_DYING: + case CPU_DYING_FROZEN: + kvm_cpu_reset(NULL); + break; } return NOTIFY_OK; @@ -1168,6 +1172,11 @@ out_err: return err; } +void kvm_arch_hardware_disable(void) +{ + kvm_cpu_reset(NULL); +} + /* NOP: Compiling as a module not supported */ void kvm_arch_exit(void) { diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 97f88fe..a97b2fe 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -261,7 +261,6 @@ static inline void vgic_arch_setup(const struct vgic_params *vgic) } } -static inline void kvm_arch_hardware_disable(void) {} static inline void kvm_arch_hardware_unsetup(void) {} static inline void kvm_arch_sync_events(struct kvm *kvm) {} static inline void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu) {} -- 1.7.9.5 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC 4/4] arm64: kvm: add cpu reset at module exit 2015-03-23 11:53 [RFC 0/4] arm64: kexec: fix kvm issue in kexec AKASHI Takahiro ` (2 preceding siblings ...) 2015-03-23 11:53 ` [RFC 3/4] arm64: kvm: add cpu reset hook for cpu hotplug AKASHI Takahiro @ 2015-03-23 11:53 ` AKASHI Takahiro 3 siblings, 0 replies; 14+ messages in thread From: AKASHI Takahiro @ 2015-03-23 11:53 UTC (permalink / raw) To: catalin.marinas, will.deacon, marc.zyngier, mark.rutland Cc: linux-arm-kernel, linaro-kernel, geoff, kexec, linux-kernel, AKASHI Takahiro, broonie, david.griego, christoffer.dall, freddy77 This patch doesn't enable kvm to be built as a module, but is a prerequisite when kvm is transformed to be module-capable. Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org> --- arch/arm/kvm/arm.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index 08804d3..b85dad6 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -1181,6 +1181,12 @@ void kvm_arch_hardware_disable(void) void kvm_arch_exit(void) { kvm_perf_teardown(); + + unregister_reboot_notifier(&kvm_reboot_nb); + /* + * Reset each CPU in EL2 to initial state. + */ + on_each_cpu(kvm_cpu_reset, NULL, 1); } static int arm_init(void) -- 1.7.9.5 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2015-03-25 9:48 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-03-23 11:53 [RFC 0/4] arm64: kexec: fix kvm issue in kexec AKASHI Takahiro 2015-03-23 11:53 ` [RFC 1/4] arm64: kvm: add a cpu tear-down function AKASHI Takahiro 2015-03-23 16:46 ` Geoff Levand 2015-03-24 7:48 ` AKASHI Takahiro 2015-03-24 10:00 ` Marc Zyngier 2015-03-25 8:06 ` AKASHI Takahiro 2015-03-25 9:48 ` Marc Zyngier 2015-03-23 11:53 ` [RFC 2/4] arm64: kexec: fix kvm issue AKASHI Takahiro 2015-03-23 15:56 ` Geoff Levand 2015-03-24 7:52 ` AKASHI Takahiro 2015-03-24 8:46 ` Marc Zyngier 2015-03-24 16:56 ` Geoff Levand 2015-03-23 11:53 ` [RFC 3/4] arm64: kvm: add cpu reset hook for cpu hotplug AKASHI Takahiro 2015-03-23 11:53 ` [RFC 4/4] arm64: kvm: add cpu reset at module exit AKASHI Takahiro
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).