arm64: kernel: Add support for hibernate/suspend-to-disk

When the kernel is running at EL2, it doesn't need init_hyp_mode() to configure page tables for HYP. This function also registers the CPU hotplug and lower power notifiers that cause HYP to be re-initialised after the CPU has been reset.

To avoid losing the register state that controls stage2 translation, move the registering of these notifiers into init_subsystems(), and add a is_kernel_in_hyp_mode() path to each callback.

Fixes 1e947bad0b6 ("arm64: KVM: Skip HYP setup when already running in HYP")

Signed-off-by James Morse <[email protected]>

---
 arch/arm/kvm/arm.c | 52 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 33 insertions(+), 19 deletions(-)

arch/arm/kvm/arm.c +33 -19

index 6accd66d26f0..b5384311dec4 100644
@@ -1061,15 +1061,27 @@ static void cpu_init_hyp_mode(void *dummy)
 	kvm_arm_init_debug();
 }
 
+static void cpu_hyp_reinit(void)
+{
+	if (is_kernel_in_hyp_mode()) {
+		/*
+		 * cpu_init_stage2() is safe to call even if the PM
+		 * event was cancelled before the CPU was reset.
+		 */
+		cpu_init_stage2(NULL);
+	} else {
+		if (__hyp_get_vectors() == hyp_default_vectors)
+			cpu_init_hyp_mode(NULL);
+	}
+}
+
 static int hyp_init_cpu_notify(struct notifier_block *self,
 			       unsigned long action, void *cpu)
 {
 	switch (action) {
 	case CPU_STARTING:
 	case CPU_STARTING_FROZEN:
-		if (__hyp_get_vectors() == hyp_default_vectors)
-			cpu_init_hyp_mode(NULL);
-		break;
+		cpu_hyp_reinit();
 	}
 
 	return NOTIFY_OK;

@@ -1084,9 +1096,8 @@ static int hyp_init_cpu_pm_notifier(struct notifier_block *self,
 				    unsigned long cmd,
 				    void *v)
 {
-	if (cmd == CPU_PM_EXIT &&
-	    __hyp_get_vectors() == hyp_default_vectors) {
-		cpu_init_hyp_mode(NULL);
+	if (cmd == CPU_PM_EXIT) {
+		cpu_hyp_reinit();
 		return NOTIFY_OK;
 	}
 

@@ -1128,6 +1139,22 @@ static int init_subsystems(void)
 	int err;
 
 	/*
+	 * Register CPU Hotplug notifier
+	 */
+	cpu_notifier_register_begin();
+	err = __register_cpu_notifier(&hyp_init_cpu_nb);
+	cpu_notifier_register_done();
+	if (err) {
+		kvm_err("Cannot register KVM init CPU notifier (%d)\n", err);
+		return err;
+	}
+
+	/*
+	 * Register CPU lower-power notifier
+	 */
+	hyp_cpu_pm_init();
+
+	/*
 	 * Init HYP view of VGIC
 	 */
 	err = kvm_vgic_hyp_init();

@@ -1270,19 +1297,6 @@ static int init_hyp_mode(void)
 	free_boot_hyp_pgd();
 #endif
 
-	cpu_notifier_register_begin();
-
-	err = __register_cpu_notifier(&hyp_init_cpu_nb);
-
-	cpu_notifier_register_done();
-
-	if (err) {
-		kvm_err("Cannot register HYP init CPU notifier (%d)\n", err);
-		goto out_err;
-	}
-
-	hyp_cpu_pm_init();
-
 	/* set size of VMID supported by CPU */
 	kvm_vmid_bits = kvm_get_vmid_bits();
 	kvm_info("%d-bit VMID\n", kvm_vmid_bits);

Signature · 1 line

2.8.0.rc3

Quoted 9 lines · James Morse

When the kernel is running at EL2, it doesn't need init_hyp_mode() to configure page tables for HYP. This function also registers the CPU hotplug and lower power notifiers that cause HYP to be re-initialised after the CPU has been reset.

To avoid losing the register state that controls stage2 translation, move the registering of these notifiers into init_subsystems(), and add a is_kernel_in_hyp_mode() path to each callback.

Quoted 2 lines

Fixes 1e947bad0b6 ("arm64: KVM: Skip HYP setup when already running in HYP")

Signed-off-by James Morse <[email protected]>

To the best of my knowledge of the hyp code:

Acked-by Catalin Marinas <[email protected]>

Since it has a Fixes tag, does this need to be picked for 4.6 or a Cc:
stable if it goes in later?

Hi Catalin,

Quoted 10 lines · Catalin Marinas

Quoted 9 lines · James Morse

When the kernel is running at EL2, it doesn't need init_hyp_mode() to configure page tables for HYP. This function also registers the CPU hotplug and lower power notifiers that cause HYP to be re-initialised after the CPU has been reset.

To avoid losing the register state that controls stage2 translation, move the registering of these notifiers into init_subsystems(), and add a is_kernel_in_hyp_mode() path to each callback.

Quoted 6 lines

Quoted 2 lines

Fixes 1e947bad0b6 ("arm64: KVM: Skip HYP setup when already running in HYP")

Signed-off-by James Morse <[email protected]>

To the best of my knowledge of the hyp code:

Acked-by Catalin Marinas <[email protected]>

Thanks,

Quoted 2 lines

Since it has a Fixes tag, does this need to be picked for 4.6 or a Cc:
stable if it goes in later?

This patch was sent as a separate fix for rc1, its here too so that this series can be applied directly from the list without conflicts after rc2.

From: Geoff Levand <[email protected]>

To allow the assembler macros defined in arch/arm64/mm/proc-macros.S to be used outside the mm code move the contents of proc-macros.S to asm/assembler.h. Also, delete proc-macros.S, and fix up all references to proc-macros.S.

Signed-off-by Geoff Levand <[email protected]>

Acked-by Pavel Machek <[email protected]>

[rebased, included dcache_by_line_op]

Signed-off-by James Morse <[email protected]>

From: Geoff Levand <[email protected]>

We currently have macros defining flags for the arm64 sctlr registers in both kvm_arm.h and sysreg.h. To clean things up and simplify move the definitions of the SCTLR_EL2 flags from kvm_arm.h to sysreg.h, rename any SCTLR_EL1 or SCTLR_EL2 flags that are common to both registers to be SCTLR_ELx, with 'x' indicating a common flag, and fixup all files to include the proper header or to use the new macro names.

Signed-off-by Geoff Levand <[email protected]>

[Restored pgtable-hwdef.h include]

Signed-off-by James Morse <[email protected]>

The hyp-stub could make use of the do_el2_call(), move it to a header file.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/include/asm/virt.h  | 18 +++++++++++++++++-
 arch/arm64/kvm/hyp/hyp-entry.S | 17 +----------------
 2 files changed, 18 insertions(+), 17 deletions(-)

arch/arm64/include/asm/virt.h +7 -1

index 9f22dd607958..b8fdddeca71b 100644
@@ -21,7 +21,23 @@
 #define BOOT_CPU_MODE_EL1	(0xe11)
 #define BOOT_CPU_MODE_EL2	(0xe12)
 
-#ifndef __ASSEMBLY__
+#ifdef __ASSEMBLY__
+.macro do_el2_call
+	/*
+	 * Shuffle the parameters before calling the function
+	 * pointed to in x0. Assumes parameters in x[1,2,3].
+	 */
+	sub	sp, sp, #16

Quoted 23 lines · James Morse

arch/arm64/include/asm/virt.h +14 -1

index 9f22dd607958..b8fdddeca71b 100644
@@ -21,7 +21,23 @@
 #define BOOT_CPU_MODE_EL1	(0xe11)
 #define BOOT_CPU_MODE_EL2	(0xe12)
 
-#ifndef __ASSEMBLY__
+#ifdef __ASSEMBLY__
+.macro do_el2_call
+	/*
+	 * Shuffle the parameters before calling the function
+	 * pointed to in x0. Assumes parameters in x[1,2,3].
+	 */
+	sub	sp, sp, #16
+	str	lr, [sp]
+	mov	lr, x0
+	mov	x0, x1
+	mov	x1, x2
+	mov	x2, x3
+	blr	lr
+	ldr	lr, [sp]

arch/arm64/include/asm/virt.h line 40

Quoted 3 lines

arch/arm64/include/asm/virt.h +3 -0

+	add	sp, sp, #16
+.endm
+#else

So while I'm not opposed to this macro being reused, the name is slightly misleading out of the original context. This macro doesn't take you to EL2, but implements a very peculiar calling convention that only the HYP code is expecting.

Could we have a disclaimer saying something along the lines of "Don't you dare using this macro!"?

Hi Marc,

Quoted 24 lines · Marc Zyngier

Quoted 23 lines · James Morse

arch/arm64/include/asm/virt.h +14 -1

index 9f22dd607958..b8fdddeca71b 100644
@@ -21,7 +21,23 @@
 #define BOOT_CPU_MODE_EL1	(0xe11)
 #define BOOT_CPU_MODE_EL2	(0xe12)
 
-#ifndef __ASSEMBLY__
+#ifdef __ASSEMBLY__
+.macro do_el2_call
+	/*
+	 * Shuffle the parameters before calling the function
+	 * pointed to in x0. Assumes parameters in x[1,2,3].
+	 */
+	sub	sp, sp, #16
+	str	lr, [sp]
+	mov	lr, x0
+	mov	x0, x1
+	mov	x1, x2
+	mov	x2, x3
+	blr	lr
+	ldr	lr, [sp]

Quoted 8 lines

arch/arm64/include/asm/virt.h line 40

Quoted 3 lines

arch/arm64/include/asm/virt.h +3 -0

+	add	sp, sp, #16
+.endm
+#else

So while I'm not opposed to this macro being reused, the name is slightly misleading out of the original context. This macro doesn't take you to EL2, but implements a very peculiar calling convention that only the HYP code is expecting.

Ah, I hadn't thought of it like that!

Quoted 2 lines

Could we have a disclaimer saying something along the lines of "Don't you dare using this macro!"?

Or rename it? Something like 'unpack_el2_call'.

Today the 'hvc' calling kvm or the hyp-stub is expected to preserve all registers. Kvm saves/restores the registers it needs on the EL2 stack using do_el2_call. The hyp-stub has no stack, later patches need at least one register they can use.

Allow theses calls to clobber the link register, and add code to save/restore this register at the call sites.

Signed-off-by James Morse <[email protected]>

From: Geoff Levand <[email protected]>

The existing arm64 hcall implementations are limited in that they only allow for two distinct hcalls; with the x0 register either zero or not zero. Also, the API of the hyp-stub exception vector routines and the KVM exception vector routines differ; hyp-stub uses a non-zero value in x0 to implement __hyp_set_vectors, whereas KVM uses it to implement kvm_call_hyp.

To allow for additional hcalls to be defined and to make the arm64 hcall API more consistent across exception vector routines, change the hcall implementations to reserve all x0 values below PAGE_SIZE for hcalls such as {s,g}et_vectors().

Define two new preprocessor macros HVC_GET_VECTORS, and HVC_SET_VECTORS to be used as hcall type specifiers and convert the existing __hyp_get_vectors() and __hyp_set_vectors() routines to use these new macros when executing an HVC call. Also, change the corresponding hyp-stub and KVM el1_sync exception vector routines to use these new macros.

Signed-off-by Geoff Levand <[email protected]>

[Merged two hcall patches, moved immediate value from esr to x0, use lr
as a scratch register]

Signed-off-by James Morse <[email protected]>

From: AKASHI Takahiro <[email protected]>

The current kvm implementation on arm64 does cpu-specific initialization at system boot, and has no way to gracefully shutdown a core in terms of kvm. This prevents kexec from rebooting the system at EL2.

This patch adds a cpu tear-down function and also puts an existing cpu-init code into a separate function, kvm_arch_hardware_disable() and kvm_arch_hardware_enable() respectively.
We don't need the arm64 specific cpu hotplug hook any more.

Since this patch modifies common code between arm and arm64, one stub definition, __cpu_reset_hyp_mode(), is added on arm side to avoid compilation errors.

Signed-off-by AKASHI Takahiro <[email protected]>

[Rebase, added separate VHE init/exit path, changed resets use of
kvm_call_hyp() to the __version, en/disabled hardware in init_subsystems(), added icache maintenance to __kvm_hyp_reset() and removed lr restore]

Signed-off-by James Morse <[email protected]>

---
 arch/arm/include/asm/kvm_host.h   |  10 ++-
 arch/arm/include/asm/kvm_mmu.h    |   1 +
 arch/arm/kvm/arm.c                | 128 +++++++++++++++++++++++---------------
 arch/arm/kvm/mmu.c                |   5 ++
 arch/arm64/include/asm/kvm_asm.h  |   1 +
 arch/arm64/include/asm/kvm_host.h |  13 +++-
 arch/arm64/include/asm/kvm_mmu.h  |   1 +
 arch/arm64/kvm/hyp-init.S         |  38 +++++++++++
 arch/arm64/kvm/reset.c            |  14 +++++
 9 files changed, 158 insertions(+), 53 deletions(-)

arch/arm/include/asm/kvm_host.h +9 -1

index 385070180c25..738d5eee91de 100644
@@ -265,6 +265,15 @@ static inline void __cpu_init_stage2(void)
 	kvm_call_hyp(__init_stage2_translation);
 }
 
+static inline void __cpu_reset_hyp_mode(phys_addr_t boot_pgd_ptr,
+					phys_addr_t phys_idmap_start)
+{
+	/*
+	 * TODO
+	 * kvm_call_reset(boot_pgd_ptr, phys_idmap_start);
+	 */
+}
+
 static inline int kvm_arch_dev_ioctl_check_extension(long ext)
 {
 	return 0;

@@ -277,7 +286,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) {}

arch/arm/include/asm/kvm_mmu.h +1 -0

index da44be9db4fa..f17a8d41822c 100644
@@ -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);
 

arch/arm/kvm/arm.c +2 -6

index b5384311dec4..962904a443be 100644
@@ -16,7 +16,6 @@
  * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-#include <linux/cpu.h>
 #include <linux/cpu_pm.h>
 #include <linux/errno.h>
 #include <linux/err.h>

@@ -66,6 +65,8 @@ static DEFINE_SPINLOCK(kvm_vmid_lock);
 
 static bool vgic_present;
 
+static DEFINE_PER_CPU(unsigned char, kvm_arm_hardware_enabled);
+
 static void kvm_arm_set_running_vcpu(struct kvm_vcpu *vcpu)
 {
 	BUG_ON(preemptible());

@@ -90,11 +91,6 @@ struct kvm_vcpu * __percpu *kvm_get_running_vcpus(void)
 	return &kvm_arm_running_vcpu;
 }
 
-int kvm_arch_hardware_enable(void)
-{
-	return 0;
-}
-
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu)
 {
 	return kvm_vcpu_exiting_guest_mode(vcpu) == IN_GUEST_MODE;

Quoted 11 lines · James Morse

arch/arm/kvm/arm.c +2 -1

index b5384311dec4..962904a443be 100644
@@ -591,7 +587,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		/*
 		 * Re-check atomic conditions
 		 */
-		if (signal_pending(current)) {
+		if (unlikely(!__this_cpu_read(kvm_arm_hardware_enabled))) {
+			/* cpu has been torn down */

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 595

Quoted 4 lines

arch/arm/kvm/arm.c +4 -0

+			ret = 0;
+			run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+			run->fail_entry.hardware_entry_failure_reason
+					= (u64)-ENOEXEC;

This hunk makes me feel a bit uneasy. Having to check something that critical on the entry path is at least a bit weird. If we've reset EL2 already, it means that we must have forced an exit on the guest to do so.

So why do we hand the control back to KVM (or anything else) once we've nuked a CPU? I'd expect it to be put on some back-burner, never to return in this lifetime...

on_each_cpu(hardware_disable_nolock, NULL, 1);

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Hi Marc, Takahiro,

Quoted 12 lines · Marc Zyngier

Quoted 11 lines · James Morse

arch/arm/kvm/arm.c +2 -1

index b5384311dec4..962904a443be 100644
@@ -591,7 +587,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		/*
 		 * Re-check atomic conditions
 		 */
-		if (signal_pending(current)) {
+		if (unlikely(!__this_cpu_read(kvm_arm_hardware_enabled))) {
+			/* cpu has been torn down */

Quoted 8 lines

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 595

Quoted 4 lines

arch/arm/kvm/arm.c +4 -0

+			ret = 0;
+			run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+			run->fail_entry.hardware_entry_failure_reason
+					= (u64)-ENOEXEC;

This hunk makes me feel a bit uneasy. Having to check something that critical on the entry path is at least a bit weird. If we've reset EL2 already, it means that we must have forced an exit on the guest to do so.

(To save anyone else digging: the story comes from v12 of the kexec copy of this patch [0])

Quoted 3 lines

So why do we hand the control back to KVM (or anything else) once we've nuked a CPU? I'd expect it to be put on some back-burner, never to return in this lifetime...

This looks like the normal reboot code being called in the middle of a running system. Kexec calls kernel_restart_prepare(), which kicks each reboot notifier, one of which is kvm_reboot(), which calls:

Quoted 1 line

on_each_cpu(hardware_disable_nolock, NULL, 1);

We have to give the CPU back as there may be other reboot notifiers, and kexec hasn't yet shuffled onto the boot cpu.

How about moving this check into handle_exit()[1]?
Currently this sees ARM_EXCEPTION_IRQ, and tries to re-enter the guest, we can test for kvm_rebooting, which is set by kvm's reboot notifier .... but this would still break if another vcpu wakes from cond_resched() and sprints towards __kvm_vcpu_run(). Can we move cond_resched() to immediately before handle_exit()?

I can't see a reason why this doesn't happen on the normal reboot path, presumably we rely on user space to kill any running guests.

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Takahiro, any ideas/wisdom on this?

Thanks,

James

[0] http://lists.infradead.org/pipermail/kexec/2015-December/014953.html
[1] Untested(!) alternative.
====================================================

arch/arm/kvm/arm.c +0 -5

index 0e63047a9530..dfa3cc42ec89 100644
@@ -562,11 +562,6 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct k
 vm_run *run)
        ret = 1;
        run->exit_reason = KVM_EXIT_UNKNOWN;
        while (ret > 0) {
-               /*
-                * Check conditions before entering the guest
-                */
-               cond_resched();
-
                update_vttbr(vcpu->kvm);
 

                if (vcpu->arch.power_off || vcpu->arch.pause)

arch/arm/kvm/arm.c +2 -0

@@ -662,6 +657,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kv
 m_run *run)
 
                preempt_enable();
 
+               cond_resched();
+
                ret = handle_exit(vcpu, run, ret);
        }

arch/arm64/kvm/handle_exit.c +6 -0

index eba89e42f0ed..cc562d9ff905 100644
@@ -170,6 +170,12 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 {
        exit_handle_fn exit_handler;
 
+       if (kvm_rebooting) {
+               run->exit_reason = KVM_EXIT_SYSTEM_EVENT;
+               run->fail_entry.hardware_entry_failure_reason = (u64)-ENOEXEC;
+               return 0;
+       }
+
        switch (exception_index) {
        case ARM_EXCEPTION_IRQ:
                return 1;

====================================================

Quoted 33 lines · James Morse

Quoted 12 lines · Marc Zyngier

Quoted 11 lines · James Morse

arch/arm/kvm/arm.c +2 -1

index b5384311dec4..962904a443be 100644
@@ -591,7 +587,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
 		/*
 		 * Re-check atomic conditions
 		 */
-		if (signal_pending(current)) {
+		if (unlikely(!__this_cpu_read(kvm_arm_hardware_enabled))) {
+			/* cpu has been torn down */

Quoted 8 lines

arch/arm/kvm/arm.c int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) · line 595

Quoted 4 lines

arch/arm/kvm/arm.c +4 -0

+			ret = 0;
+			run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+			run->fail_entry.hardware_entry_failure_reason
+					= (u64)-ENOEXEC;

This hunk makes me feel a bit uneasy. Having to check something that critical on the entry path is at least a bit weird. If we've reset EL2 already, it means that we must have forced an exit on the guest to do so.

(To save anyone else digging: the story comes from v12 of the kexec copy of this patch [0])

Quoted 3 lines

So why do we hand the control back to KVM (or anything else) once we've nuked a CPU? I'd expect it to be put on some back-burner, never to return in this lifetime...

This looks like the normal reboot code being called in the middle of a running system. Kexec calls kernel_restart_prepare(), which kicks each reboot notifier, one of which is kvm_reboot(), which calls:

Quoted 4 lines

Quoted 1 line

on_each_cpu(hardware_disable_nolock, NULL, 1);

We have to give the CPU back as there may be other reboot notifiers, and kexec hasn't yet shuffled onto the boot cpu.

Right, and this kvm reboot notifier can be executed via IPI at any time while interrupted is enabled, and so the check must be done between local_irq_disable() and local_irq_enable().

Quoted 5 lines

How about moving this check into handle_exit()[1]?
Currently this sees ARM_EXCEPTION_IRQ, and tries to re-enter the guest, we can test for kvm_rebooting, which is set by kvm's reboot notifier .... but this would still break if another vcpu wakes from cond_resched() and sprints towards __kvm_vcpu_run(). Can we move cond_resched() to immediately before handle_exit()?

I don't think that it would break as reboot process is *one-directional*, and any cpu should be torn down sooner or later.

I'm not quite sure about Marc's point, but if he has concern on overhead of checking per-cpu kvm_arm_hardware_enabled, we may, instead, check on kvm_rebooting.

(And this check won't make sense for VHE-enabled platform.)

Hi Marc, Takahiro,

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Quoted 6 lines · James Morse

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

I very much like that approach, to be honest. Tearing down a CPU is something exceptional, so let's make it an actual exception.

It is now pretty easy to discriminate between KVM functions and stub functions thanks to your earlier patch, so if we end up calling the hyp-stub because we've torn down KVM's EL2, let's just return an appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1.

Hi Marc,

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · James Morse

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Quoted 7 lines

I very much like that approach, to be honest. Tearing down a CPU is something exceptional, so let's make it an actual exception.

It is now pretty easy to discriminate between KVM functions and stub functions thanks to your earlier patch, so if we end up calling the hyp-stub because we've torn down KVM's EL2, let's just return an appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1.

Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and hand over to purgatory, but we could change that to a new 'special' builtin call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so there is no reason the calls have to be same.

Given hibernate doesn't hit this issue, I will drop this hunk from this version of the patch, and repost hibernate incorporating the feedback so far. I will provide a patch for kexec to do the above.

Hi Marc,

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · James Morse

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Quoted 7 lines

I very much like that approach, to be honest. Tearing down a CPU is something exceptional, so let's make it an actual exception.

It is now pretty easy to discriminate between KVM functions and stub functions thanks to your earlier patch, so if we end up calling the hyp-stub because we've torn down KVM's EL2, let's just return an appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1.

Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and hand over to purgatory, but we could change that to a new 'special' builtin call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so there is no reason the calls have to be same.

Given hibernate doesn't hit this issue, I will drop this hunk from this version of the patch, and repost hibernate incorporating the feedback so far. I will provide a patch for kexec to do the above.

Thanks,

James

From d70cf5d3202d819bd7f8c9072c61da783bf07b40 Mon Sep 17 00:00:00 2001

Quoted 22 lines · James Morse

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · James Morse

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Quoted 7 lines

I very much like that approach, to be honest. Tearing down a CPU is something exceptional, so let's make it an actual exception.

It is now pretty easy to discriminate between KVM functions and stub functions thanks to your earlier patch, so if we end up calling the hyp-stub because we've torn down KVM's EL2, let's just return an appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1.

Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and hand over to purgatory, but we could change that to a new 'special' builtin call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so there is no reason the calls have to be same.

Quoted 3 lines

Given hibernate doesn't hit this issue, I will drop this hunk from this version of the patch, and repost hibernate incorporating the feedback so far. I will provide a patch for kexec to do the above.

Thanks, but you don' have to.

If the fix below is acceptable, we will merge it to our next kexec/kdump patch series.

Hi,

Quoted 23 lines · AKASHI Takahiro

Quoted 22 lines · James Morse

Quoted 8 lines · Marc Zyngier

Quoted 6 lines · James Morse

It looks like x86 uses the extable to work around this, their vmx_vcpu_run() has:

Quoted 1 line

		__ex(ASM_VMX_VMLAUNCH) "\n\t"

Where __ex ends up calling ____kvm_handle_fault_on_reboot(), with a nearby comment:

Quoted 3 lines

* Hardware virtualization extension instructions may fault if a
* reboot turns off virtualization while processes are running.
* Trap the fault and ignore the instruction if that happens.

Quoted 7 lines

I very much like that approach, to be honest. Tearing down a CPU is something exceptional, so let's make it an actual exception.

It is now pretty easy to discriminate between KVM functions and stub functions thanks to your earlier patch, so if we end up calling the hyp-stub because we've torn down KVM's EL2, let's just return an appropriate error code (ARM_EXCEPTION_HYP_GONE), and handle it at EL1.

Okay. kexec uses kvm_call_hyp() against the hyp-stub to do the kernel-copy and hand over to purgatory, but we could change that to a new 'special' builtin call, something like HVC_KEXEC_CALL_HYP. It never calls it with kvm loaded, so there is no reason the calls have to be same.

Quoted 5 lines

Quoted 3 lines

Given hibernate doesn't hit this issue, I will drop this hunk from this version of the patch, and repost hibernate incorporating the feedback so far. I will provide a patch for kexec to do the above.

Thanks, but you don' have to.

I was wrong with the 'hibernate doesn't hit this issue', with this patch we re-install the hyp-stub during system reboot, and race with the scheduler. ('reboot -f' while running a guest).

Quoted 2 lines

If the fix below is acceptable, we will merge it to our next kexec/kdump patch series.

I'm testing something that looks very similar at the moment.

Quoted 7 lines

arch/arm64/kvm/handle_exit.c +0 -0

index eba89e4..31b5224 100644
@@ -186,6 +186,10 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		exit_handler = kvm_get_exit_handler(vcpu);
 

arch/arm64/kvm/handle_exit.c int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run, · line 192

Quoted 5 lines

arch/arm64/kvm/handle_exit.c +4 -0

 		return exit_handler(vcpu, run);
+	case ARM_EXCEPTION_HYP_GONE:
+		/* due to kexec reboot */
+		run->exit_reason = KVM_EXIT_SHUTDOWN;
+		return 0;

Is it fair to throw this back out to user space? While the hypervisor doesn't have long to live, it may not be expecting this exit_reason. I couldn't see a value for 'suprise cpu removal', and it looks like the x86 code causes the vcpu to spin round enter guest...

+/*
+ * Memory to save the cpu state is allocated on the stack by
+ * __cpu_suspend_enter()'s caller, and populated by __cpu_suspend_enter().
+ * This data must survive until cpu_resume() is called.
+ *
+ * This struct desribes the size and the layout of the saved cpu state.

arch/arm64/kernel/sleep.S +2 -19

@@ -93,34 +86,11 @@ ENTRY(__cpu_suspend_enter)
 	ldp	w5, w6, [x9, #(MPIDR_HASH_SHIFTS + 8)]
 	compute_mpidr_hash x8, x3, x4, x5, x6, x7, x10
 	add	x1, x1, x8, lsl #3
+
+	push	x29, lr
 	bl	__cpu_suspend_save
-	/*
-	 * Grab suspend finisher in x20 and its argument in x19
-	 */
-	mov	x0, x19
-	mov	x1, x20
-	/*
-	 * We are ready for power down, fire off the suspend finisher
-	 * in x1, with argument in x0
-	 */
-	blr	x1
-        /*
-	 * Never gets here, unless suspend finisher fails.
-	 * Successful cpu_suspend should return from cpu_resume, returning
-	 * through this code path is considered an error
-	 * If the return value is set to 0 force x0 = -EOPNOTSUPP
-	 * to make sure a proper error condition is propagated
-	 */
-	cmp	x0, #0
-	mov	x3, #-EOPNOTSUPP

arch/arm64/kernel/sleep.S +11 -7

+
+ENTRY(_cpu_resume)
 	mrs	x1, mpidr_el1
 	adrp	x8, mpidr_hash
 	add x8, x8, #:lo12:mpidr_hash // x8 = struct mpidr_hash phys address

@@ -134,29 +116,32 @@ ENTRY(cpu_resume)
 	ldp	w5, w6, [x8, #(MPIDR_HASH_SHIFTS + 8)]
 	compute_mpidr_hash x7, x3, x4, x5, x6, x1, x2
         /* x7 contains hash index, let's use it to grab context pointer */
-	ldr_l	x0, sleep_save_sp + SLEEP_SAVE_SP_PHYS
+	ldr_l	x0, sleep_save_stash
 	ldr	x0, [x0, x7, lsl #3]
 	add	x29, x0, #SLEEP_STACK_DATA_CALLEE_REGS
 	add	x0, x0, #SLEEP_STACK_DATA_SYSTEM_REGS
 	/* load sp from context */
 	ldr	x2, [x0, #CPU_CTX_SP]
-	/* load physical address of identity map page table in x1 */
-	adrp	x1, idmap_pg_dir
 	mov	sp, x2
 	/* save thread_info */
 	and	x2, x2, #~(THREAD_SIZE - 1)
 	msr	sp_el0, x2
 	/*
-	 * cpu_do_resume expects x0 to contain context physical address
-	 * pointer and x1 to contain physical address of 1:1 page tables
+	 * cpu_do_resume expects x0 to contain context address pointer
 	 */
-	bl	cpu_do_resume		// PC relative jump, MMU off
-	/* Can't access these by physical address once the MMU is on */
+	bl	cpu_do_resume
+
+#ifdef CONFIG_KASAN
+	mov	x0, sp
+	bl	kasan_unpoison_remaining_stack
+#endif
+

page.h uses '_AC' in the definition of PAGE_SIZE, but doesn't include linux/const.h where this is defined. This produces build warnings when only asm/page.h is included by asm code.

Signed-off-by James Morse <[email protected]>

Acked-by Mark Rutland <[email protected]>

KERNEL_START and KERNEL_END are useful outside head.S, move them to a header file.

Signed-off-by James Morse <[email protected]>

From: Geoff Levand <[email protected]>

Kexec and hibernate need to copy pages of memory, but may not have all of the kernel mapped, and are unable to call copy_page().

Add a simplistic copy_page() macro, that can be inlined in these situations. lib/copy_page.S provides a bigger better version, but uses more registers.

Signed-off-by Geoff Levand <[email protected]>

[Changed asm label to 9998, added commit message]

Signed-off-by James Morse <[email protected]>

Quoted 9 lines · James Morse

From: Geoff Levand <[email protected]>

Kexec and hibernate need to copy pages of memory, but may not have all of the kernel mapped, and are unable to call copy_page().

Add a simplistic copy_page() macro, that can be inlined in these situations. lib/copy_page.S provides a bigger better version, but uses more registers.

Quoted 3 lines

Signed-off-by Geoff Levand <[email protected]>

[Changed asm label to 9998, added commit message]

Signed-off-by James Morse <[email protected]>

Good enough for the brief usage:

Acked-by Catalin Marinas <[email protected]>

Would there be another user of this macro in the future (kexec)?

el2_setup() doesn't just configure el2, it configures el1 too. This means we can't use it to re-configure el2 after resume from hibernate, as we will be returned to el1 with the MMU turned off.

Split the sctlr_el1 setting code up, so that el2_setup() accepts an initial value as an argument. This value will be ignored if el2_setup() is called at el1: the running value will be preserved with endian correction.

Hibernate can now call el2_setup() to re-configure el2, passing the current sctlr_el1 as an argument.

arch/arm64/include/asm/assembler.h +0 -0

index a04fd7a9c102..627d66efec68 100644

arch/arm64/kernel/head.S +5 -0

index 1217262b5210..097986152203 100644
@@ -208,6 +208,7 @@ section_table:
 
 ENTRY(stext)
 	bl	preserve_boot_args
+	init_sctlr_el1	x0
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
 	mov	x23, xzr			// KASLR offset, defaults to 0
 	adrp	x24, __PHYS_OFFSET

@@ -514,8 +515,12 @@ ENTRY(kimage_vaddr)
  *
  * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 in x20 if
  * booted in EL1 or EL2 respectively.
+ *
+ * If booted in EL2, SCTLR_EL1 will be initialised with the value in x0
+ * (otherwise the existing value will be preserved, with endian correction).
  */
 ENTRY(el2_setup)
+	mov	x1, x0				// preserve passed-in sctlr_el1
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
 	b.ne	1f

Quoted 9 lines · James Morse

el2_setup() doesn't just configure el2, it configures el1 too. This means we can't use it to re-configure el2 after resume from hibernate, as we will be returned to el1 with the MMU turned off.

Split the sctlr_el1 setting code up, so that el2_setup() accepts an initial value as an argument. This value will be ignored if el2_setup() is called at el1: the running value will be preserved with endian correction.

Quoted 2 lines

Hibernate can now call el2_setup() to re-configure el2, passing the current sctlr_el1 as an argument.

I don't fully understand the description. The first paragraph says we can't use el2_setup to re-configure el2 after resume from hibernate while the last paragraph says that we can.

Quoted 7 lines

arch/arm64/include/asm/assembler.h +0 -0

index a04fd7a9c102..627d66efec68 100644
@@ -311,6 +311,16 @@ lr	.req	x30		// link register
 	.endm
 

arch/arm64/include/asm/assembler.h lr .req x30 // link register · line 320

Quoted 8 lines

arch/arm64/include/asm/assembler.h +7 -0

 /*
+ * Generate the initial sctlr_el1 value for el2_setup to set if we boot at EL2.
+ */
+	.macro init_sctlr_el1 reg
+		mov	\reg, #0x0800		// Set/clear RES{1,0} bits
+CPU_BE(		movk	\reg, #0x33d0, lsl #16)	// Set EE and E0E on BE systems
+CPU_LE(		movk	\reg, #0x30d0, lsl #16)	// Clear EE and E0E on LE systems
+	.endm

I can see, at least in this patch, that all places where this macro is invoked are immediately followed by a call to el2_setup. Can we not just place this at the beginning of el2_setup?

Quoted 31 lines

arch/arm64/kernel/head.S +6 -1

index 1217262b5210..097986152203 100644
@@ -208,6 +208,7 @@ section_table:
 
 ENTRY(stext)
 	bl	preserve_boot_args
+	init_sctlr_el1	x0
 	bl	el2_setup			// Drop to EL1, w20=cpu_boot_mode
 	mov	x23, xzr			// KASLR offset, defaults to 0
 	adrp	x24, __PHYS_OFFSET

@@ -514,8 +515,12 @@ ENTRY(kimage_vaddr)
  *
  * Returns either BOOT_CPU_MODE_EL1 or BOOT_CPU_MODE_EL2 in x20 if
  * booted in EL1 or EL2 respectively.
+ *
+ * If booted in EL2, SCTLR_EL1 will be initialised with the value in x0
+ * (otherwise the existing value will be preserved, with endian correction).
  */
 ENTRY(el2_setup)
+	mov	x1, x0				// preserve passed-in sctlr_el1
 	mrs	x0, CurrentEL
 	cmp	x0, #CurrentEL_EL2
 	b.ne	1f

@@ -524,7 +529,7 @@ CPU_BE(	orr	x0, x0, #(1 << 25)	)	// Set the EE bit for EL2
 CPU_LE(	bic	x0, x0, #(1 << 25)	)	// Clear the EE bit for EL2
 	msr	sctlr_el2, x0
 	b	2f
-1:	mrs	x0, sctlr_el1
+1:	mrs	x0, sctlr_el1			// ignore passed-in sctlr_el1

arch/arm64/kernel/head.S CPU_BE( orr x0, x0, #(1 << 25) ) // Set the EE bit for EL2 · line 535

Quoted 3 lines

arch/arm64/kernel/head.S +0 -0

 CPU_BE(	orr	x0, x0, #(3 << 24)	)	// Set the EE and E0E bits for EL1
 CPU_LE(	bic	x0, x0, #(3 << 24)	)	// Clear the EE and E0E bits for EL1
 	msr	sctlr_el1, x0

BTW, I wonder whether we need this read-modify-write sequence at all rather than always setting a pre-set sane value (as per the init_sctlr_el1 macro). This way we can always do this at the beginning of el2_setup independent of whether we run in EL1 or EL2.

arch/arm64/kernel/head.S set_hcr: · line 589

Quoted 9 lines

arch/arm64/kernel/head.S +4 -0

@@ -578,6 +583,10 @@ set_hcr:
 
 3:
 #endif
+	/* use sctlr_el1 value we were provided with */
+CPU_BE(	orr	x1, x1, #(3 << 24)	)	// Set the EE and E0E bits for EL1
+CPU_LE(	bic	x1, x1, #(3 << 24)	)	// Clear the EE and E0E bits for EL1
+	msr	sctlr_el1, x1

I don't think we need this here, the value passed already has the EE/E0E bits set accordingly by the init_sctlr_el1 macro.

Hi Catalin,

Quoted 10 lines · Catalin Marinas

Quoted 9 lines · James Morse

el2_setup() doesn't just configure el2, it configures el1 too. This means we can't use it to re-configure el2 after resume from hibernate, as we will be returned to el1 with the MMU turned off.

Split the sctlr_el1 setting code up, so that el2_setup() accepts an initial value as an argument. This value will be ignored if el2_setup() is called at el1: the running value will be preserved with endian correction.

Quoted 6 lines

Quoted 2 lines

Hibernate can now call el2_setup() to re-configure el2, passing the current sctlr_el1 as an argument.

I don't fully understand the description. The first paragraph says we can't use el2_setup to re-configure el2 after resume from hibernate while the last paragraph says that we can.

Then I need to rephrase the middle paragraph. Is this any clearer:
This happens because el2_setup() generates a sctlr_el1 value needed for early boot, (mmu off etc). Change el2_setup() to accept this as an argument, so that hibernate can pass the current sctlr_el1 (with the mmu on), to el2_setup(), and have it re-configure el2 without changing el1.

The motivation for this patch was resuming with a different kernel version, which may have left el2 in an incompatible state. Running the original el2_setup() was the only guaranteed way to know everything was set correctly. If we definitely don't want to support this, then this patch can go, and the minimum el2-reset code can be put into hibernate-asm.S.

Some architectures require code written to memory as if it were data to be 'cleaned' from any data caches before the processor can fetch them as new instructions.

During resume from hibernate, the snapshot code copies some pages directly, meaning these architectures do not get a chance to perform their cache maintenance. Modify the read and decompress code to call flush_icache_range() on all pages that are restored, so that the restored in-place pages are guaranteed to be executable on these architectures.

Signed-off-by James Morse <[email protected]>

Acked-by Pavel Machek <[email protected]>

Acked-by Rafael J. Wysocki <[email protected]>

arch/arm64/kernel/hibernate-asm.S new file +0 -0

arch/arm64/kernel/hibernate-asm.S new file +32 -0

+ENTRY(swsusp_arch_suspend_exit)
+	/* Temporary page tables are a copy, so no need for a trampoline here */
+	msr	ttbr1_el1, x0
+	isb
+	tlbi	vmalle1is	/* invalidate intermediate caching entries */
+	dsb	ish
+
+	mov	x21, x1
+	mov	x30, x2		/* el2_setup() will eret to the lr directly */
+	mov	x24, x4
+	mov	x25, x5
+
+	/* walk the restore_pblist and use copy_page() to over-write memory */
+	mov	x19, x3
+
+1:	ldr	x10, [x19, #HIBERN_PBE_ORIG]
+	mov	x0, x10
+	ldr	x1, [x19, #HIBERN_PBE_ADDR]
+
+	copy_page	x0, x1, x2, x3, x4, x5, x6, x7, x8, x9
+
+	add	x1, x10, #PAGE_SIZE
+	/* Clean the copied page to PoU - based on flush_icache_range() */
+	dcache_line_size x2, x3
+	sub	x3, x2, #1
+	bic	x4, x10, x3
+2:	dc	cvau, x4	/* clean D line / unified line */
+	add	x4, x4, x2
+	cmp	x4, x1
+	b.lo	2b
+
+	ldr	x19, [x19, #HIBERN_PBE_NEXT]

arch/arm64/kernel/hibernate-asm.S new file +31 -0

+
+
+	cbz	x24, 4f		/* Did we boot at el1? */
+	/* Clean el2_setup's page to PoC */
+	mov	x0, x24
+	/*
+	 * We don't know if el2_setup() overlaps a page boundary, clean two
+	 * pages, just in case.
+	 */
+	add	x1, x0, #2*PAGE_SIZE
+	dcache_line_size x2, x3
+	sub	x3, x2, #1
+	bic	x4, x0, x3
+3:	dc	cvac, x4
+	add	x4, x4, x2
+	cmp	x4, x1
+	b.lo	3b
+
+	/* reconfigure el2 */
+	mrs	x0, sctlr_el1
+	hvc	#0
+
+	/*
+	 * el2_setup() will eret to the location in x30, so we
+	 * only get here if we booted at el1.
+	 */
+
+4:	ret
+
+	.ltorg
+ENDPROC(swsusp_arch_suspend_exit)

arch/arm64/kernel/hibernate.c new file +0 -0

@@ -0,0 +1,503 @@

arch/arm64/kernel/hibernate.c new file +24 -0

+/*
+ * Copies length bytes, starting at src_start into an new page,
+ * perform cache maintentance, then map it (nearly) at the bottom of memory
+ * as executable.
+ *
+ * This is used by hibernate to copy the code it needs to execute when
+ * overwriting the kernel text. This function generates a new set of page
+ * tables, which it loads into ttbr0.
+ *
+ * Length is provided as we probably only want 4K of data, even on a 64K
+ * page system. We don't use the very bottom page, so that dereferencing
+ * NULL continues to have the expected behaviour.
+ */
+static int create_safe_exec_page(void *src_start, size_t length,
+				 void **dst_addr, phys_addr_t *phys_dst_addr,
+				 unsigned long (*allocator)(gfp_t mask),
+				 gfp_t mask)
+{
+	int rc = 0;
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *pte;
+	unsigned long dst = allocator(mask);

arch/arm64/kernel/hibernate.c new file +12 -0

+
+out:
+	return rc;
+}
+
+
+int swsusp_arch_suspend(void)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct sleep_stack_data state;
+

arch/arm64/kernel/hibernate.c new file +63 -0

+static int copy_pud(pgd_t *dst_pgd, pgd_t *src_pgd, unsigned long start,
+		    unsigned long end)
+{
+	int rc = 0;
+	pmd_t *dst_pmd;
+	unsigned long next;
+	unsigned long addr = start;
+	pud_t *src_pud = pud_offset(src_pgd, start);
+	pud_t *dst_pud = pud_offset(dst_pgd, start);
+
+	do {
+		next = pud_addr_end(addr, end);
+		if (!pud_val(*src_pud))
+			continue;
+
+		if (pud_table(*(src_pud))) {
+			if (PTRS_PER_PMD != 1) {
+				dst_pmd = (pmd_t *)get_safe_page(GFP_ATOMIC);
+				if (!dst_pmd) {
+					rc = -ENOMEM;
+					break;
+				}
+
+				set_pud(dst_pud, __pud(virt_to_phys(dst_pmd)
+						       | PUD_TYPE_TABLE));
+			}
+
+			rc = copy_pmd(dst_pud, src_pud, addr, next);
+			if (rc)
+				break;
+		} else {
+			set_pud(dst_pud,
+				__pud(pud_val(*src_pud) & ~PMD_SECT_RDONLY));
+		}
+	} while (dst_pud++, src_pud++, addr = next, addr != end);
+
+	return rc;
+}
+
+static int copy_page_tables(pgd_t *dst_pgd, unsigned long start,
+			    unsigned long end)
+{
+	int rc = 0;
+	pud_t *dst_pud;
+	unsigned long next;
+	unsigned long addr = start;
+	pgd_t *src_pgd = pgd_offset_k(start);
+
+	dst_pgd += pgd_index(start);
+
+	do {
+		next = pgd_addr_end(addr, end);
+		if (!pgd_val(*src_pgd))
+			continue;
+
+		if (PTRS_PER_PUD != 1) {
+			dst_pud = (pud_t *)get_safe_page(GFP_ATOMIC);
+			if (!dst_pud) {
+				rc = -ENOMEM;
+				break;
+			}
+
+			set_pgd(dst_pgd, __pgd(virt_to_phys(dst_pud)

arch/arm64/kernel/hibernate.c new file +16 -0

+
+/*
+ * Setup then Resume from the hibernate image using swsusp_arch_suspend_exit().
+ *
+ * Memory allocated by get_safe_page() will be dealt with by the hibernate code,
+ * we don't need to free it here.
+ */
+int swsusp_arch_resume(void)
+{
+	int rc = 0;
+	size_t exit_size;
+	pgd_t *tmp_pg_dir;
+	void *lm_restore_pblist;
+	phys_addr_t phys_hibernate_exit;
+	void __noreturn (*hibernate_exit)(phys_addr_t, phys_addr_t, void *,
+					  void *, unsigned long, phys_addr_t);

Quoted 20 lines · James Morse

arch/arm64/kernel/hibernate-asm.S new file +17 -0

 
+	copy_page	x0, x1, x2, x3, x4, x5, x6, x7, x8, x9
+
+	add	x1, x10, #PAGE_SIZE
+	/* Clean the copied page to PoU - based on flush_icache_range() */
+	dcache_line_size x2, x3
+	sub	x3, x2, #1
+	bic	x4, x10, x3
+2:	dc	cvau, x4	/* clean D line / unified line */
+	add	x4, x4, x2
+	cmp	x4, x1
+	b.lo	2b
+
+	ldr	x19, [x19, #HIBERN_PBE_NEXT]
+	cbnz	x19, 1b
+
+
+	/* switch to the restored kernels page tables, to reconfigure el2 */

arch/arm64/kernel/hibernate-asm.S line 81

Quoted 6 lines

arch/arm64/kernel/hibernate-asm.S new file +6 -0

+	msr	ttbr1_el1, x21  /* physical address of swapper page tables */
+	isb
+	tlbi	vmalle1is	/* invalidate intermediate caching entries */
+	ic	ialluis
+	dsb	ish		/* also waits for PoU cleaning to finish */
+	isb

The waiting for PoU cleaning needs to happen before the IC instruction.

arch/arm64/kernel/hibernate-asm.S

Quoted 6 lines

arch/arm64/kernel/hibernate-asm.S new file +6 -0

+
+	/* Load our new page tables */
+	asm volatile("msr	ttbr0_el1, %0;"
+		     "isb;"
+		     "tlbi	vmalle1is;"
+		     "dsb	ish" : : "r"(virt_to_phys(pgd)));

Do we expect anything to have used ttbr0_el1 at this point?

Quoted 11 lines

arch/arm64/kernel/hibernate-asm.S new file +11 -0

+int swsusp_arch_suspend(void)
+{
+	int ret = 0;
+	unsigned long flags;
+	struct sleep_stack_data state;
+
+	local_dbg_save(flags);
+
+	if (__cpu_suspend_enter(&state)) {
+		ret = swsusp_save();
+	} else {

arch/arm64/kernel/hibernate-asm.S

Quoted 5 lines

arch/arm64/kernel/hibernate-asm.S new file +5 -0

+		void *lm_kernel_start;
+
+		/* Clean kernel to PoC for secondary core startup */
+		lm_kernel_start = LMADDR(KERNEL_START);
+		__flush_dcache_area(lm_kernel_start, KERNEL_END - KERNEL_START);

We don't need to use LMADDR here. The KERNEL_START is already mapped at the caches are PIPT (-like), so flushing any of the aliases would do.

But I'm not sure we even need to flush the whole kernel. The secondary cores would only execute certain areas before they enable the MMU, at which point they have visibility over the whole cache. Is this needed for secondary core startup on resume from hibernate?

PM: Image mismatch: memory size

Resuming using a different kernel version is fragile, while there are sufficient details in the hibernate arch-header to perform the restore, changes in the boot process can have a long-lasting impact on the system. In particular, if the EFI stub causes more runtime services memory to be allocated,

the amount of memory left for linux is reduced. If we are lucky, this will cause restore to fail with the message:

Quoted 1 line

PM: Image mismatch: memory size

If we are unlucky, the system will explode sometime later when an EFI runtime services call is made.

Prevent resuming with a different kernel build, by storing UTS_VERSION in the hibernate header. This also ensures the page size and va bits configuration remain the same.

Signed-off-by James Morse <[email protected]>

---
 arch/arm64/kernel/hibernate.c | 35 +++++------------------------------
 1 file changed, 5 insertions(+), 30 deletions(-)

arch/arm64/kernel/hibernate.c +5 -31

index 279c556ee24b..486315249f2a 100644
@@ -20,6 +20,7 @@
 #include <linux/pm.h>
 #include <linux/sched.h>
 #include <linux/suspend.h>
+#include <linux/utsname.h>
 #include <linux/version.h>
 
 #include <asm/barrier.h>

@@ -42,8 +43,6 @@
 #define pud_index(x)   0
 #endif
 
-#define TCR_IPS_BITS (0x7UL<<32)
-
 /*
  * Hibernate core relies on this value being 0 on resume, and marks it
  * __nosavedata assuming it will keep the resume kernel's '0' value. This

@@ -54,17 +53,6 @@
  */
 extern int in_suspend;
 
-/*
- * This value is written to the hibernate arch header, and prevents resuming
- * from a hibernate image produced by an incompatible kernel. If you change
- * a value that isn't saved/restored by hibernate, you should change this value.
- *
- * For example, if the mair_el1 values used by the kernel are changed, you
- * should prevent resuming from a kernel with incompatible attributes, as these
- * aren't saved/restored.
- */
-#define HIBERNATE_VERSION      KERNEL_VERSION(4, 6, 0)
-
 /* Find a symbols alias in the linear map */
 #define LMADDR(x)      phys_to_virt(virt_to_phys(x))
 

@@ -81,8 +69,7 @@ extern char hibernate_el2_vectors[];
 extern char el2_setup[];
 
 struct arch_hibernate_hdr_invariants {
-       unsigned long   version;
-       unsigned long   tcr_el1;        /* page_size, va bit etc */
+       char            uts_version[__NEW_UTS_LEN + 1];
 };
 
 /* These values need to be know across a hibernate/restore. */

@@ -104,11 +91,8 @@ static struct arch_hibernate_hdr {
 
 static inline void arch_hdr_invariants(struct arch_hibernate_hdr_invariants *i)
 {
-       i->version = HIBERNATE_VERSION;
-       asm volatile("mrs       %0, tcr_el1" : "=r"(i->tcr_el1));
-
-       /* IPS bits vary on big/little systems, mask them out */
-       i->tcr_el1 &= ~TCR_IPS_BITS;
+       memset(i, 0, sizeof(*i));
+       memcpy(i->uts_version, init_utsname()->version, sizeof(i->uts_version));
 }
 
 int pfn_is_nosave(unsigned long pfn)

@@ -155,18 +139,9 @@ int arch_hibernation_header_restore(void *addr)
        struct arch_hibernate_hdr_invariants invariants;
        struct arch_hibernate_hdr *hdr = addr;
 
-       /*
-        * If this header is ancient, it may be smaller than we expect.
-        * Test the version first.
-        */
-       if (hdr->invariants.version != HIBERNATE_VERSION) {
-               pr_crit("Hibernate image not compatible with this kernel version!\n");
-               return -EINVAL;
-       }
-
        arch_hdr_invariants(&invariants);
        if (memcmp(&hdr->invariants, &invariants, sizeof(invariants))) {
-               pr_crit("Hibernate image not compatible with this kernel configuration!\n");
+               pr_crit("Hibernate image not generated by this kernel!\n");
                return -EINVAL;
        }
 
--

2.8.0.rc3

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 5 lines · James Morse

Resuming using a different kernel version is fragile, while there are sufficient details in the hibernate arch-header to perform the restore, changes in the boot process can have a long-lasting impact on the system. In particular, if the EFI stub causes more runtime services memory to be allocated,

Actually, the UEFI stub does not allocate any runtime service code or data memory, that is all done by the firmware itself before even invoking the kernel or the stub. The allocations performed by the stub are for the command line, the UEFI memory map, the kernel image and optionally an initrd, none if which have any significance to the firmware itself, so allocating runtime services memory for them does not make a lot of sense.