[v3 0/5] arm64: kvm: reset hyp context for kexec

This patch set addresses KVM issue described in Geoff's kexec patch set[1].
(The subject was changed from "arm64: kexec: fix kvm issue in kexec.")
See "Changes" below.

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 (and #5) 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 at 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 + Arn's patch[2]


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[2] 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.

Changes from v2:
* modified kvm_virt_to_trampoline() macro to fix a page-alignment issue[4]

39 more lines

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 <redacted>
---
 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         |   35 +++++++++++++++++++++++++++++++++++
 arch/arm64/kvm/hyp.S              |   16 +++++++++++++---
 8 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 5560f74..39df694 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c

@@ -897,6 +897,21 @@ static void cpu_init_hyp_mode(void *dummy)
 	__cpu_init_hyp_mode(boot_pgd_ptr, pgd_ptr, hyp_stack_ptr, vector_ptr);
 }
 
+static void kvm_cpu_reset(void *dummy)
+{
+	phys_addr_t boot_pgd_ptr;
+	phys_addr_t phys_idmap_start;
+
+	if (__hyp_get_vectors() == hyp_default_vectors)
+		return;
+
+	boot_pgd_ptr = kvm_mmu_get_boot_httbr();
+	phys_idmap_start = kvm_get_idmap_start();
+	__cpu_reset_hyp_mode(boot_pgd_ptr, phys_idmap_start,
+			     hyp_default_vectors,
+			     kvm_virt_to_trampoline(__kvm_hyp_reset));
+}
+
 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..3631a37 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c

@@ -1490,6 +1490,11 @@ phys_addr_t kvm_get_idmap_vector(void)
 	return hyp_idmap_vector;
 }
 
+phys_addr_t kvm_get_idmap_start(void)
+{
+	return hyp_idmap_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..f1c16e2 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h

@@ -118,6 +118,7 @@ struct kvm_vcpu;
 
 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..6a8da9c 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -199,6 +199,8 @@ 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(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 +225,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(phys_addr_t boot_pgd_ptr,
+					phys_addr_t phys_idmap_start,
+					unsigned long stub_vector_ptr,
+					unsigned long reset_func)
+{
+	kvm_call_reset(boot_pgd_ptr, phys_idmap_start, stub_vector_ptr,
+		       reset_func);
+}
+
 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..facfd6d 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h

@@ -96,6 +96,7 @@ 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);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 

@@ -305,5 +306,11 @@ static inline void __kvm_flush_dcache_pud(pud_t pud)
 void kvm_set_way_flush(struct kvm_vcpu *vcpu);
 void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled);
 
+extern char __hyp_idmap_text_start[];
+#define kvm_virt_to_trampoline(x)	\
+		(TRAMPOLINE_VA		\
+		+ ((unsigned long)(x)	\
+		  - ((unsigned long)__hyp_idmap_text_start & PAGE_MASK)))
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ARM64_KVM_MMU_H__ */

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..d212990 100644
--- a/arch/arm64/kvm/hyp-init.S
+++ b/arch/arm64/kvm/hyp-init.S

@@ -115,6 +115,41 @@ 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
+	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
+
+	/* Install 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..7c3bdee 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,14 @@ el1_sync:					// Guest trapped into EL2
 	cmp	x18, #HVC_GET_VECTORS
 	b.ne	1f
 	mrs	x0, vbar_el2
-	b	2f
+	b	3f
 
-1:	/* Default to HVC_CALL_HYP. */
+	/* jump into trampoline code */
+1:	cmp	x18, #HVC_RESET
+	b.ne	2f
+	br	x3				// no return
 
+2:	/* Default to HVC_CALL_HYP. */
 	push	lr, xzr
 
 	/*

@@ -1196,7 +1205,8 @@ el1_sync:					// Guest trapped into EL2
 	blr	lr
 
 	pop	lr, xzr
-2:	eret
+
+3:	eret
 
 el1_trap:
 	/*

-- 
1.7.9.5

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 <redacted>
---
 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 39df694..f64713e 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>

@@ -1100,6 +1101,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.
  */

@@ -1138,6 +1156,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

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 <redacted>
---
 arch/arm/include/asm/kvm_host.h   |    1 -
 arch/arm/kvm/arm.c                |    9 +++++++++
 arch/arm64/include/asm/kvm_host.h |    1 -
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index 41008cd..ca97764 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h

@@ -237,7 +237,6 @@ void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 
-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) {}

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f64713e..4892974 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c

@@ -922,6 +922,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;

@@ -1165,6 +1169,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 6a8da9c..831e6a4 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h

@@ -262,7 +262,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

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 <redacted>
---
 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 4892974..85c142b 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c

@@ -1178,6 +1178,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

Signed-off-by: AKASHI Takahiro <redacted>
---
 arch/arm/include/asm/kvm_asm.h  |    1 +
 arch/arm/include/asm/kvm_host.h |   12 ++++++++++++
 arch/arm/include/asm/kvm_mmu.h  |    5 +++++
 arch/arm/kvm/init.S             |    6 ++++++
 4 files changed, 24 insertions(+)

diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
index 25410b2..462babf 100644
--- a/arch/arm/include/asm/kvm_asm.h
+++ b/arch/arm/include/asm/kvm_asm.h

@@ -85,6 +85,7 @@ struct kvm_vcpu;
 
 extern char __kvm_hyp_init[];
 extern char __kvm_hyp_init_end[];
+extern char __kvm_hyp_reset[];
 
 extern char __kvm_hyp_exit[];
 extern char __kvm_hyp_exit_end[];

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index ca97764..6d38134 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h

@@ -220,6 +220,18 @@ static inline void __cpu_init_hyp_mode(phys_addr_t boot_pgd_ptr,
 	kvm_call_hyp((void*)hyp_stack_ptr, vector_ptr, pgd_ptr);
 }
 
+static inline void __cpu_reset_hyp_mode(phys_addr_t boot_pgd_ptr,
+					phys_addr_t phys_idmap_start,
+					unsigned long stub_vector_ptr,
+					unsigned long reset_func)
+{
+	/*
+	 * TODO
+	 * kvm_call_reset(boot_pgd_ptr, phys_idmap_start, stub_vector_ptr,
+	 *		  reset_func);
+	 */
+}
+
 static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 {
 	return 0;

diff --git a/arch/arm/include/asm/kvm_mmu.h b/arch/arm/include/asm/kvm_mmu.h
index bf0fe99..dc9543b 100644
--- a/arch/arm/include/asm/kvm_mmu.h
+++ b/arch/arm/include/asm/kvm_mmu.h

@@ -66,6 +66,7 @@ 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);
 int kvm_mmu_init(void);
 void kvm_clear_hyp_idmap(void);
 

@@ -270,6 +271,10 @@ static inline void __kvm_flush_dcache_pud(pud_t pud)
 void kvm_set_way_flush(struct kvm_vcpu *vcpu);
 void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled);
 
+extern char __hyp_idmap_text_start[];
+#define kvm_virt_to_trampoline(x)       \
+			(TRAMPOLINE_VA + ((x) - __hyp_idmap_text_start))
+
 #endif	/* !__ASSEMBLY__ */
 
 #endif /* __ARM_KVM_MMU_H__ */

diff --git a/arch/arm/kvm/init.S b/arch/arm/kvm/init.S
index 3988e72..9178c9a 100644
--- a/arch/arm/kvm/init.S
+++ b/arch/arm/kvm/init.S

@@ -156,4 +156,10 @@ target:	@ We're now in the trampoline code, switch page tables
 	.globl __kvm_hyp_init_end
 __kvm_hyp_init_end:
 
+	.globl __kvm_hyp_reset
+__kvm_hyp_reset:
+	@ TODO
+
+	eret
+
 	.popsection

-- 
1.7.9.5

This can instead be:

1:	mrs	x0, sctlr_el2
	ldr	x1, =SCTLR_EL2_FLAGS
	bic	x0, x0, x1
	msr	sctlr_el2, x0
	isb

As SCTLR_EL2_FLAGS doesn't contain the EE bit.

Mark.

On Thu, Apr 02, 2015 at 06:40:13AM +0100, 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 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.

Why can't we use kvm_arch_hardware_enable/disable() currently?

It would be helpful for the comment to explain why this is wrong, and
what needs fixing.

Mark.

On 04/08/2015 10:02 PM, Mark Rutland wrote:

quoted

+	/* We're now in idmap, disable MMU */
+1:	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

This can instead be:

1:	mrs	x0, sctlr_el2
	ldr	x1, =SCTLR_EL2_FLAGS
	bic	x0, x0, x1
	msr	sctlr_el2, x0
	isb

As SCTLR_EL2_FLAGS doesn't contain the EE bit.

Oops. I will fix it.

Thanks,
-Takahiro AKASHI

Mark.

Mark,

On 04/08/2015 10:05 PM, Mark Rutland wrote:

On Thu, Apr 02, 2015 at 06:40:13AM +0100, AKASHI Takahiro wrote:

quoted

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.

Why can't we use kvm_arch_hardware_enable/disable() currently?

IIUC, kvm will call kvm_arch_hardware_enable() iff a new guest is being
created *and* cpus have not been initialized yet. kvm_usage_count==0
indicates this. Similarly, kvm will call kvm_arch_hardware_disable() whenever
a guest is being terminated (i.e. kvm_usage_count != 0).
Therefore if kvm_arch_hardware_enable/disable() also handle EL2 vector table
initialization, we don't have to have any particular operations, as my patch
does, for kexec case.
(a long-term solution)

Since arm64 doesn't implement kvm_arch_hardware_enable() (I don't know why),
I'm trying to fix the problem by adding a minimum tear-down function, kvm_cpu_reset,
and invoking it via a reboot hook.
(an interim fix)

This scheme of a interim fix and a long-term solution, I heard, has been agreed
by Marc and Geoff in LCU14. I just followed it.

Is this clear?

quoted

Signed-off-by: AKASHI Takahiro <redacted>
---
  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 39df694..f64713e 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>

@@ -1100,6 +1101,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 */

It would be helpful for the comment to explain why this is wrong, and

1 more quoted lines

Thank for reminding me of this.

*priority* enforces a calling order of registered hook functions.
If some hook returns NOTIFY_STOP_MASK, subsequent hooks won't be called.
(Nevertheless, reboot sequence will go ahead. See kernel_restart_prepare()/
notifier_call_chain().)

So we should make sure that kvm_reboot_notify() be called
1) after any hook functions which may depend on kvm, and
2) before any hook functions which kvm may depend on, and
3) before any hook functions that may return NOTIFY_STOP_MASK

But how can we guarantee this and determine a priority of kvm_reboot_notify()?
Looking into all the occurrences of register_reboot_notifier(),
1) => nothing
2) => virt/kvm/kvm_main.c (priority: 0)
3) => drivers/cpufreq/s32416-cpufreq.c (priority: 0)
       drivers/cpufreq/s5pv210-cpufreq.c (priority: 0)

So a priority higher than zero might be safe and better, but exactly what?
Some hooks use "INT_MAX."

Thanks,
-Takahiro AKASHI

Mark.

On Thu, Apr 09, 2015 at 05:53:33AM +0100, AKASHI Takahiro wrote:

Mark,

On 04/08/2015 10:05 PM, Mark Rutland wrote:

quoted

On Thu, Apr 02, 2015 at 06:40:13AM +0100, AKASHI Takahiro wrote:

quoted

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.

Why can't we use kvm_arch_hardware_enable/disable() currently?

IIUC, kvm will call kvm_arch_hardware_enable() iff a new guest is being
created *and* cpus have not been initialized yet. kvm_usage_count==0
indicates this. Similarly, kvm will call kvm_arch_hardware_disable() whenever
a guest is being terminated (i.e. kvm_usage_count != 0).
Therefore if kvm_arch_hardware_enable/disable() also handle EL2 vector table
initialization, we don't have to have any particular operations, as my patch
does, for kexec case.
(a long-term solution)

Since arm64 doesn't implement kvm_arch_hardware_enable() (I don't know why),
I'm trying to fix the problem by adding a minimum tear-down function, kvm_cpu_reset,
and invoking it via a reboot hook.
(an interim fix)

What I don't understand is why we can't move the init and tear-down
functions into kvm_arch_hardware_enable/disable(). They seem to be for
precisely what you are implementing, with the only difference being the
time that they are called.

Either I'm missing something, or we can simply implement the existing
hooks. I assume I'm missing something.

quoted

quoted

+static struct notifier_block kvm_reboot_nb = {
+	.notifier_call		= kvm_reboot_notify,
+	.next			= NULL,
+	.priority		= 0, /* FIXME */

It would be helpful for the comment to explain why this is wrong, and
what needs fixing.

Thank for reminding me of this.

*priority* enforces a calling order of registered hook functions.
If some hook returns NOTIFY_STOP_MASK, subsequent hooks won't be called.
(Nevertheless, reboot sequence will go ahead. See kernel_restart_prepare()/
notifier_call_chain().)

So we should make sure that kvm_reboot_notify() be called
1) after any hook functions which may depend on kvm, and

Which hooks depend on KVM?

2) before any hook functions which kvm may depend on, and

Which other hooks does KVM depend on?

3) before any hook functions that may return NOTIFY_STOP_MASK

I think this would be solved by using kvm_arch_hardware_enable/disable.
As far as I can tell, the VMs would be destroyed earlier (and hence KVM
disabled) before we got to the final teardown.

Thanks,
Mark.

Mark
Cc: Marc, Geoff

On 04/10/2015 12:02 AM, Mark Rutland wrote:

On Thu, Apr 09, 2015 at 05:53:33AM +0100, AKASHI Takahiro wrote:

quoted

Mark,

On 04/08/2015 10:05 PM, Mark Rutland wrote:

quoted

On Thu, Apr 02, 2015 at 06:40:13AM +0100, AKASHI Takahiro wrote:

quoted

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.

Why can't we use kvm_arch_hardware_enable/disable() currently?

IIUC, kvm will call kvm_arch_hardware_enable() iff a new guest is being
created *and* cpus have not been initialized yet. kvm_usage_count==0
indicates this. Similarly, kvm will call kvm_arch_hardware_disable() whenever
a guest is being terminated (i.e. kvm_usage_count != 0).
Therefore if kvm_arch_hardware_enable/disable() also handle EL2 vector table
initialization, we don't have to have any particular operations, as my patch
does, for kexec case.
(a long-term solution)

Since arm64 doesn't implement kvm_arch_hardware_enable() (I don't know why),
I'm trying to fix the problem by adding a minimum tear-down function, kvm_cpu_reset,
and invoking it via a reboot hook.
(an interim fix)

What I don't understand is why we can't move the init and tear-down
functions into kvm_arch_hardware_enable/disable(). They seem to be for
precisely what you are implementing, with the only difference being the

1 more quoted lines

I don't know, neither. I just followed the discussions between Marc and Geoff,
and their conclusion. I guessed that *refactoring* might be more complicated than
expected.

FYI, I gave a quick try to kvm_arch_hardware_enable() approach by removing
cpu_init_hyp_mode() from init_hyp_mode() and putting it into kvm_arch_hardware_enable(),
and it seems to work, at least, in my environment:
    boot => start a kvm guest => kexec reboot => start a kvm guest

Either I'm missing something, or we can simply implement the existing
hooks. I assume I'm missing something.

Marc, Geoff, any comments?

quoted

quoted

quoted

+static struct notifier_block kvm_reboot_nb = {
+	.notifier_call		= kvm_reboot_notify,
+	.next			= NULL,
+	.priority		= 0, /* FIXME */

It would be helpful for the comment to explain why this is wrong, and
what needs fixing.

Thank for reminding me of this.

*priority* enforces a calling order of registered hook functions.
If some hook returns NOTIFY_STOP_MASK, subsequent hooks won't be called.
(Nevertheless, reboot sequence will go ahead. See kernel_restart_prepare()/
notifier_call_chain().)

So we should make sure that kvm_reboot_notify() be called
1) after any hook functions which may depend on kvm, and

Which hooks depend on KVM?

I think I answered this question below:
 >> But how can we guarantee this and determine a priority of kvm_reboot_notify()?
 >> Looking into all the occurrences of register_reboot_notifier(),
 >> 1) => nothing
 >> 2) => virt/kvm/kvm_main.c (priority: 0)
 >> 3) => drivers/cpufreq/s32416-cpufreq.c (priority: 0)
 >>        drivers/cpufreq/s5pv210-cpufreq.c (priority: 0)
 >>
 >> So a priority higher than zero might be safe and better, but exactly what?
 >> Some hooks use "INT_MAX."

Thanks,
-Takahiro AKASHI

quoted

2) before any hook functions which kvm may depend on, and

Which other hooks does KVM depend on?

quoted

3) before any hook functions that may return NOTIFY_STOP_MASK

I think this would be solved by using kvm_arch_hardware_enable/disable.
As far as I can tell, the VMs would be destroyed earlier (and hence KVM
disabled) before we got to the final teardown.

Thanks,
Mark.

quoted

What I don't understand is why we can't move the init and tear-down
functions into kvm_arch_hardware_enable/disable(). They seem to be for
precisely what you are implementing, with the only difference being the
time that they are called.

I don't know, neither. I just followed the discussions between Marc and Geoff,
and their conclusion. I guessed that *refactoring* might be more complicated than
expected.

FYI, I gave a quick try to kvm_arch_hardware_enable() approach by removing
cpu_init_hyp_mode() from init_hyp_mode() and putting it into kvm_arch_hardware_enable(),
and it seems to work, at least, in my environment:
    boot => start a kvm guest => kexec reboot => start a kvm guest

That sounds pretty convincing to me, assuming you wired the teardown
intp kvm_arch_hardware_disable() ?

quoted

Either I'm missing something, or we can simply implement the existing
hooks. I assume I'm missing something.

Marc, Geoff, any comments?

quoted

quoted

quoted

quoted

+static struct notifier_block kvm_reboot_nb = {
+	.notifier_call		= kvm_reboot_notify,
+	.next			= NULL,
+	.priority		= 0, /* FIXME */

It would be helpful for the comment to explain why this is wrong, and
what needs fixing.

Thank for reminding me of this.

*priority* enforces a calling order of registered hook functions.
If some hook returns NOTIFY_STOP_MASK, subsequent hooks won't be called.
(Nevertheless, reboot sequence will go ahead. See kernel_restart_prepare()/
notifier_call_chain().)

So we should make sure that kvm_reboot_notify() be called
1) after any hook functions which may depend on kvm, and

Which hooks depend on KVM?

I think I answered this question below:
 >> But how can we guarantee this and determine a priority of kvm_reboot_notify()?
 >> Looking into all the occurrences of register_reboot_notifier(),
 >> 1) => nothing
 >> 2) => virt/kvm/kvm_main.c (priority: 0)
 >> 3) => drivers/cpufreq/s32416-cpufreq.c (priority: 0)
 >>        drivers/cpufreq/s5pv210-cpufreq.c (priority: 0)
 >>
 >> So a priority higher than zero might be safe and better, but exactly what?
 >> Some hooks use "INT_MAX."

I can't see anything listed which has a dependency on KVM.

The KVM notifier would be superseded by
kvm_arch_hardware_{disable,enable}, and the cpufreq instances don't seem
to have any relationship to KVM.

Other architectures use kvm_arch_hardware_{enable,disable}(), so I
imagine the core KVM code has no problem with the ordering of these.

Mark.