Skip to contentSkip to conversation

arm64: hibernate: pass HVC_SET_VECTORS args to the resume hvc

PATCH Applied Bradley Morgan 3 participants 10 messages Aug 9 – Sep 9 arm64

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 arm64: hibernate: pass HVC_SET_VECTORS args to the resume hvc

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors")
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
arch/arm64/kernel/hibernate-asm.S +2 -0
index 0e1d9c3c6a93..2baefe7a82d3 100644
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4",  ARM64_WORKAROUND_CLEAN_CACHE
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)
Signature · 1 line
2.47.3
maintainer
#1 lore raw

[+Maz, Pasha and James]

Quoted 21 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors")
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
arch/arm64/kernel/hibernate-asm.S +0 -0
index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4",  ARM64_WORKAROUND_CLEAN_CACHE
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

+el1_sync:
+ msr vbar_el2, x24
+ eret
+ENDPROC(el1_sync)

However, it _also_ set the vectors from C code in swsusp_arch_resume():

+ if (el2_reset_needed()) {
+ phys_addr_t el2_vectors = phys_hibernate_exit; /* base */
+ el2_vectors += hibernate_el2_vectors -
+ __hibernate_exit_text_start; /* offset */
+
+ __hyp_set_vectors(el2_vectors);
+ }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation path.

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)
1. Why the original hibernation code set the vectors twice.
2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.
3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

Will

#2 lore raw
Quoted 63 lines · Will Deacon

[+Maz, Pasha and James]

Quoted 10 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add

trans_pgd_copy_el2_vectors")

Review

Quoted 7 lines
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0

b/arch/arm64/kernel/hibernate-asm.S

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",

Quoted 4 lines

index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S +0 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4", 

ARM64_WORKAROUND_CLEAN_CACHE

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

(quoted diff) +4 -0
+el1_sync:
+       msr     vbar_el2, x24
+       eret
+ENDPROC(el1_sync)

However, it _also_ set the vectors from C code in swsusp_arch_resume():

(quoted diff) +7 -0
+       if (el2_reset_needed()) {
+               phys_addr_t el2_vectors = phys_hibernate_exit;  /* base */
+               el2_vectors += hibernate_el2_vectors -
+                              __hibernate_exit_text_start;     /* offset */
+
+               __hyp_set_vectors(el2_vectors);
+       }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation path.

Quoted 4 lines

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)

Yes, since 788bfdd97434.

Quoted 1 line

1. Why the original hibernation code set the vectors twice.

They do different jobs. The C call parks EL2 on the safe page copy before the restore overwrites the current table. The asm call installs the final __hyp_stub_vectors afterwards.

Quoted 2 lines

2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.

No. After the restore vbar_el2 is only writable from EL2, and the temporary copy cannot stay. swsusp_free() frees it right after resume.

Quoted 3 lines

3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

No, this hvc uses it, and so does kexec.

Quoted 2 lines

Will

Thanks!

maintainer
#3 lore raw
Quoted 64 lines · Bradley Morgan
Quoted 63 lines · Will Deacon

[+Maz, Pasha and James]

Quoted 10 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add

trans_pgd_copy_el2_vectors")

Review

Quoted 7 lines
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0

b/arch/arm64/kernel/hibernate-asm.S

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",

Quoted 4 lines

index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S +0 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4", 

ARM64_WORKAROUND_CLEAN_CACHE

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

(quoted diff) +4 -0
+el1_sync:
+       msr     vbar_el2, x24
+       eret
+ENDPROC(el1_sync)

However, it _also_ set the vectors from C code in swsusp_arch_resume():

(quoted diff) +7 -0
+       if (el2_reset_needed()) {
+               phys_addr_t el2_vectors = phys_hibernate_exit;  /* base */
+               el2_vectors += hibernate_el2_vectors -
+                              __hibernate_exit_text_start;     /* offset */
+
+               __hyp_set_vectors(el2_vectors);
+       }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation path.

Quoted 6 lines
Quoted 4 lines

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)

Yes, since 788bfdd97434.

Right, but did you manage to reproduce a crash? You're implying that this hasn't worked for five years, which makes me wonder why we bother to try to maintain this code!

Quoted 5 lines
Quoted 1 line

1. Why the original hibernation code set the vectors twice.

They do different jobs. The C call parks EL2 on the safe page copy before the restore overwrites the current table. The asm call installs the final __hyp_stub_vectors afterwards.

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C code.

Quoted 5 lines
Quoted 2 lines

2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.

No. After the restore vbar_el2 is only writable from EL2, and the temporary copy cannot stay. swsusp_free() frees it right after resume.

Isn't vbar_el2 always only writable from EL2?

Quoted 4 lines
Quoted 3 lines

3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

No, this hvc uses it, and so does kexec.

Where does kexec use it? I could only spot it making use of HVC_SOFT_RESTART.

Will

#4 lore raw
Quoted 68 lines · Will Deacon
Quoted 49 lines · Bradley Morgan
Quoted 48 lines · Will Deacon

[+Maz, Pasha and James]

Quoted 10 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add

trans_pgd_copy_el2_vectors")

Review

Quoted 7 lines
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0

b/arch/arm64/kernel/hibernate-asm.S

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",

Quoted 4 lines

index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S +0 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4", 

ARM64_WORKAROUND_CLEAN_CACHE

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

(quoted diff) +4 -0
+el1_sync:
+       msr     vbar_el2, x24
+       eret
+ENDPROC(el1_sync)
Quoted 4 lines

Review

Quoted 4 lines

However, it _also_ set the vectors from C code in swsusp_arch_resume():

(quoted diff) +2 -0
+       if (el2_reset_needed()) {
+               phys_addr_t el2_vectors = phys_hibernate_exit;  /* base

*/

Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+               el2_vectors += hibernate_el2_vectors -
+                              __hibernate_exit_text_start;     /*

offset */

Quoted 8 lines
Quoted 8 lines
(quoted diff) +3 -0
+
+               __hyp_set_vectors(el2_vectors);
+       }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation

path.

Quoted 1 line
Quoted 1 line
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)

Yes, since 788bfdd97434.

Right, but did you manage to reproduce a crash?

No, I had a read of the code, yk, summer holidays are here for me, so I have this kinda time

You're implying that this

Quoted 2 lines

hasn't worked for five years, which makes me wonder why we bother to try to maintain this code!

the path only runs when is_hyp_nvhe(), hence VHE machines never get there,

Quoted 11 lines
Quoted 6 lines
Quoted 2 lines

1. Why the original hibernation code set the vectors twice.

They do different jobs. The C call parks EL2 on the safe page copy before the restore overwrites the current table. The asm call installs the final __hyp_stub_vectors afterwards.

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C code.

head.S installs __hyp_stub_vectors in vbar_el2 before dropping to EL1. The last set can't come from C code because at that point the image has been restored over the kernel. The safe page copy is the only code still running and it's about to hand control back. The hvc is how it reaches EL2.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.

No. After the restore vbar_el2 is only writable from EL2, and the temporary copy cannot stay. swsusp_free() frees it right after resume.

Isn't vbar_el2 always only writable from EL2?

Yes, that phrasing was sloppy on my end. All I meant was the safe page code runs at EL1, so the hvc is the way it reaches EL2.

Quoted 8 lines
Quoted 4 lines
Quoted 3 lines

3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

No, this hvc uses it, and so does kexec.

Where does kexec use it? I could only spot it making use of HVC_SOFT_RESTART.

The non kexec_file path. machine_kexec.c calls __hyp_set_vectors(kimage->arch.el2_vectors) before jumping to the relocation code when is_hyp_nvhe(). The kexec_file path takes cpu_soft_restart, which is where HVC_SOFT_RESTART comes in. So kexec uses both, one per path........

Well, I also will think about this approach, if you have any suggestions, feel free to show me. Please :)

Quoted 2 lines

Will

Thanks!

Reviewed-by
#5 lore raw
In reply to Will Deacon

Hi Will,

Quoted 73 lines · Will Deacon
Quoted 63 lines · Bradley Morgan
Quoted 62 lines · Will Deacon

[+Maz, Pasha and James]

Quoted 10 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add

trans_pgd_copy_el2_vectors")

Review

Quoted 7 lines
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0

b/arch/arm64/kernel/hibernate-asm.S

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",

Quoted 4 lines

index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S +0 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4", 

ARM64_WORKAROUND_CLEAN_CACHE

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

(quoted diff) +4 -0
+el1_sync:
+       msr     vbar_el2, x24
+       eret
+ENDPROC(el1_sync)

However, it _also_ set the vectors from C code in swsusp_arch_resume():

(quoted diff) +7 -0
+       if (el2_reset_needed()) {
+               phys_addr_t el2_vectors = phys_hibernate_exit;  /* base */
+               el2_vectors += hibernate_el2_vectors -
+                              __hibernate_exit_text_start;     /* offset */
+
+               __hyp_set_vectors(el2_vectors);
+       }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation path.

Quoted 5 lines
Quoted 4 lines

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)

Yes, since 788bfdd97434.

Right, but did you manage to reproduce a crash? You're implying that this hasn't worked for five years, which makes me wonder why we bother to try to maintain this code!

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

1. Why the original hibernation code set the vectors twice.

They do different jobs. The C call parks EL2 on the safe page copy before the restore overwrites the current table. The asm call installs the final __hyp_stub_vectors afterwards.

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C code.

Here is my understanding of how things work. Assuming nVHE mode, we restore the previously saved image X from the currently running kernel Y. I’ll use the suffixes _X and _Y for addresses belonging to the respective images.

On the resume path, KVM teardown sets VBAR_EL2 to __hyp_stub_vectors_Y.

swsusp_arch_resume() then temporarily (re)sets VBAR_EL2 to trans_pgd_stub_vectors while transitioning to image X:

| if (el2_reset_needed())
| __hyp_set_vectors(el2_vectors);

Currently, swsusp_arch_suspend_exit() attempts to restore VBAR_EL2 to __hyp_stub_vectors_X:

|       cbz     x24, 3f         /* Do we need to re-initialise EL2? */
|       hvc     #0

where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of __hyp_stub_vectors from image X)

However, that request is ignored, so VBAR_EL2 remains pointing to trans_pgd_stub_vectors

We re-enter the kernel X.

During KVM initialisation, we call __hyp_reset_vectors(). This is handled by trans_pgd_stub_vectors, but ignored since HVC_RESET_VECTORS is not recognised. We then call __hyp_set_vectors(), which is also handled by trans_pgd_stub_vectors, but this time VBAR_EL2 get set to __kvm_hyp_init_X.

From that point on, __kvm_hyp_init_X handles subsequent HVC calls, including the later call that redirects VBAR_EL2 to __kvm_hyp_host_vector_X during KVM setup.

The memory containing trans_pgd_stub_vectors is freed later by swsusp_free(). That perhaps explains why we do not see any crash.

With the proposed fix, swsusp_arch_suspend_exit() successfully restore VBAR_EL2 to __hyp_stub_vectors_X just before we re-enter kernel X, and that vector is used for subsequent HVC calls.

__hyp_stub_vectors is similar to trans_pgd_stub_vectors (or another way around?) - it handles both HVC_SET_VECTORS and HVC_SOFT_RESTART, and ignores HVC_RESET_VECTORS.

So the fix makes sense to me. It also restores clear boundary between the temporary resume mechanism and handing control over to the restored image.

FWIW,

Quoted 15 lines
Quoted 4 lines
Quoted 2 lines

2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.

No. After the restore vbar_el2 is only writable from EL2, and the temporary copy cannot stay. swsusp_free() frees it right after resume.

Isn't vbar_el2 always only writable from EL2?

Quoted 4 lines
Quoted 3 lines

3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

No, this hvc uses it, and so does kexec.

Where does kexec use it? I could only spot it making use of HVC_SOFT_RESTART.

Will

#6 lore raw
Quoted 142 lines · Vladimir Murzin

Hi Will,

Quoted 44 lines · Will Deacon
Quoted 43 lines · Bradley Morgan
Quoted 42 lines · Will Deacon

[+Maz, Pasha and James]

Quoted 10 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

Set the arguments up the same way __hyp_set_vectors() does.

Fixes 788bfdd97434 ("arm64: trans_pgd: hibernate: Add

trans_pgd_copy_el2_vectors")

Review

Quoted 7 lines
---
 arch/arm64/kernel/hibernate-asm.S | 2 ++
 1 file changed, 2 insertions(+)
(quoted diff) +0 -0

b/arch/arm64/kernel/hibernate-asm.S

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",

Quoted 4 lines

index 0e1d9c3c6a93..2baefe7a82d3 100644

arch/arm64/kernel/hibernate-asm.S +0 -0
@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4",  "dc civac, x4", 

ARM64_WORKAROUND_CLEAN_CACHE

arch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4", ARM64_WORKAROUND_CLEAN_CACHE · line 96

Quoted 8 lines
arch/arm64/kernel/hibernate-asm.S +2 -0
 	isb
 
 	cbz	x24, 3f		/* Do we need to re-initialise EL2? */
+	mov	x1, x24
+	mov	x0, #HVC_SET_VECTORS
 	hvc	#0
 3:	ret
 SYM_CODE_END(swsusp_arch_suspend_exit)

I'm having a really hard time figuring out what's supposed to be going on here!

The original hibernation code added by James in 82869ac57b5d ("arm64:
kernel: Add support for hibernate/suspend-to-disk") unconditionally set the vectors in the exception handler:

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
(quoted diff) +4 -0
+el1_sync:
+       msr     vbar_el2, x24
+       eret
+ENDPROC(el1_sync)

However, it _also_ set the vectors from C code in

swsusp_arch_resume():

Quoted 3 lines
Quoted 3 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+       if (el2_reset_needed()) {
+               phys_addr_t el2_vectors = phys_hibernate_exit;  /*

base */

Quoted 2 lines
Quoted 2 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+               el2_vectors += hibernate_el2_vectors -
+                              __hibernate_exit_text_start;     /*

offset */

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
(quoted diff) +3 -0
+
+               __hyp_set_vectors(el2_vectors);
+       }

Later, Pasha refactored the assembly so that it could be shared with kexec in 788bfdd97434 ("arm64: trans_pgd: hibernate: Add trans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernation

path.

Quoted 7 lines
Quoted 6 lines
Quoted 5 lines

So I think we need to figure out:

0. Whether this code is actually broken atm (I have a feeling it might happen to work)

Yes, since 788bfdd97434.

Right, but did you manage to reproduce a crash? You're implying that

this

Quoted 10 lines

hasn't worked for five years, which makes me wonder why we bother to try to maintain this code!

Quoted 4 lines
Quoted 1 line

1. Why the original hibernation code set the vectors twice.

They do different jobs. The C call parks EL2 on the safe page copy before the restore overwrites the current table. The asm call installs the final __hyp_stub_vectors afterwards.

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C

code.

Quoted 1 line

Here is my understanding of how things work. Assuming nVHE mode, we restore the previously saved image X from the currently running kernel Y. I’ll use the suffixes _X and _Y for addresses belonging to the respective images.

On the resume path, KVM teardown sets VBAR_EL2 to __hyp_stub_vectors_Y.

swsusp_arch_resume() then temporarily (re)sets VBAR_EL2 to trans_pgd_stub_vectors
while transitioning to image X:

| if (el2_reset_needed())
| __hyp_set_vectors(el2_vectors);

Currently, swsusp_arch_suspend_exit() attempts to restore VBAR_EL2 to __hyp_stub_vectors_X:

|       cbz     x24, 3f         /* Do we need to re-initialise EL2? */
|       hvc     #0

where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of __hyp_stub_vectors from image X)

However, that request is ignored, so VBAR_EL2 remains pointing to trans_pgd_stub_vectors

We re-enter the kernel X.

During KVM initialisation, we call __hyp_reset_vectors(). This is handled by trans_pgd_stub_vectors, but ignored since HVC_RESET_VECTORS is not recognised. We then call __hyp_set_vectors(), which is also handled by trans_pgd_stub_vectors, but this time VBAR_EL2 get set to __kvm_hyp_init_X.

From that point on, __kvm_hyp_init_X handles subsequent HVC calls, including the later call that redirects VBAR_EL2 to __kvm_hyp_host_vector_X during KVM setup.

The memory containing trans_pgd_stub_vectors is freed later by swsusp_free(). That perhaps explains why we do not see any crash.

With the proposed fix, swsusp_arch_suspend_exit() successfully restore VBAR_EL2 to __hyp_stub_vectors_X just before we re-enter kernel X, and that vector is used for subsequent HVC calls.

__hyp_stub_vectors is similar to trans_pgd_stub_vectors (or another way around?) - it handles both HVC_SET_VECTORS and HVC_SOFT_RESTART, and ignores HVC_RESET_VECTORS.

Quoted 8 lines

So the fix makes sense to me. It also restores clear boundary between the temporary resume mechanism and handing control over to the restored image.

FWIW,

Cheers!

Quoted 10 lines
Quoted 10 lines
Quoted 5 lines
Quoted 3 lines

2. Assuming they only need to be set once, whether we can drop the hvc from the swsusp_arch_suspend_exit assembly code entirely.

No. After the restore vbar_el2 is only writable from EL2, and the temporary copy cannot stay. swsusp_free() frees it right after resume.

Isn't vbar_el2 always only writable from EL2?

Quoted 3 lines
Quoted 3 lines

3. Whether we can then drop the HVC_SET_VECTORS handling from this set of vectors.

Quoted 8 lines
Quoted 6 lines
Quoted 1 line

No, this hvc uses it, and so does kexec.

Where does kexec use it? I could only spot it making use of HVC_SOFT_RESTART.

Will

Thanks!

maintainer
#7 lore raw
In reply to Vladimir Murzin

Hi Vladimir,

Quoted 4 lines · Vladimir Murzin
Quoted 3 lines · Will Deacon

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C code.

I finally got back to this...

Quoted 4 lines

Here is my understanding of how things work. Assuming nVHE mode, we restore the previously saved image X from the currently running kernel Y. I’ll use the suffixes _X and _Y for addresses belonging to the respective images.

Nice, thanks for the walkthrough!

Quoted 30 lines

On the resume path, KVM teardown sets VBAR_EL2 to __hyp_stub_vectors_Y.

swsusp_arch_resume() then temporarily (re)sets VBAR_EL2 to trans_pgd_stub_vectors while transitioning to image X:

| if (el2_reset_needed())
| __hyp_set_vectors(el2_vectors);

Currently, swsusp_arch_suspend_exit() attempts to restore VBAR_EL2 to __hyp_stub_vectors_X:

|       cbz     x24, 3f         /* Do we need to re-initialise EL2? */
|       hvc     #0

where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of __hyp_stub_vectors from image X)

However, that request is ignored, so VBAR_EL2 remains pointing to trans_pgd_stub_vectors

We re-enter the kernel X.

During KVM initialisation, we call __hyp_reset_vectors(). This is handled by trans_pgd_stub_vectors, but ignored since HVC_RESET_VECTORS is not recognised. We then call __hyp_set_vectors(), which is also handled by trans_pgd_stub_vectors, but this time VBAR_EL2 get set to __kvm_hyp_init_X.

Quoted 6 lines

From that point on, __kvm_hyp_init_X handles subsequent HVC calls, including the later call that redirects VBAR_EL2 to __kvm_hyp_host_vector_X during KVM setup.

The memory containing trans_pgd_stub_vectors is freed later by swsusp_free(). That perhaps explains why we do not see any crash.

I was trying to figure out if we end up initialising VBAR_EL2 before we "return" from swsusp_arch_suspend() and free the reserved pages. However, it looks like we run _cpu_resume() instead of cpu_resume() so I still can't tell why this hasn't exploded. AFAICT, the active hyp vectors will have been freed (and possibly poisoned) long before KVM initialises.

Speaking off-list to Mark R, he pointed me at some old rework he had for all of this:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/suspend/cleanup

one of the patches there is very similar (identical?) to this one, but his series gets us to a point where we _do_ resume at cpu_resume() and, tbh, the whole thing ends up making a lot more sense.

If somebody could dust off that two-year old (!) stack, that would be great.

Will

Tested-by
#8 lore raw

Hi Will,

Quoted 49 lines · Will Deacon

Hi Vladimir,

Quoted 4 lines · Vladimir Murzin
Quoted 3 lines · Will Deacon

I think I probably need to spend some time understanding how all this is supposed to work. I can't currently tell how we end up with the stub vectors installed to start with nor why we can't do all this from C code.

I finally got back to this...

Quoted 4 lines

Here is my understanding of how things work. Assuming nVHE mode, we restore the previously saved image X from the currently running kernel Y. I’ll use the suffixes _X and _Y for addresses belonging to the respective images.

Nice, thanks for the walkthrough!

Quoted 34 lines

On the resume path, KVM teardown sets VBAR_EL2 to __hyp_stub_vectors_Y.

swsusp_arch_resume() then temporarily (re)sets VBAR_EL2 to trans_pgd_stub_vectors while transitioning to image X:

| if (el2_reset_needed())
| __hyp_set_vectors(el2_vectors);

Currently, swsusp_arch_suspend_exit() attempts to restore VBAR_EL2 to __hyp_stub_vectors_X:

|       cbz     x24, 3f         /* Do we need to re-initialise EL2? */
|       hvc     #0

where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of __hyp_stub_vectors from image X)

However, that request is ignored, so VBAR_EL2 remains pointing to trans_pgd_stub_vectors

We re-enter the kernel X.

During KVM initialisation, we call __hyp_reset_vectors(). This is handled by trans_pgd_stub_vectors, but ignored since HVC_RESET_VECTORS is not recognised. We then call __hyp_set_vectors(), which is also handled by trans_pgd_stub_vectors, but this time VBAR_EL2 get set to __kvm_hyp_init_X.

From that point on, __kvm_hyp_init_X handles subsequent HVC calls, including the later call that redirects VBAR_EL2 to __kvm_hyp_host_vector_X during KVM setup.

Quoted 9 lines
Quoted 2 lines

The memory containing trans_pgd_stub_vectors is freed later by swsusp_free(). That perhaps explains why we do not see any crash.

I was trying to figure out if we end up initialising VBAR_EL2 before we "return" from swsusp_arch_suspend() and free the reserved pages. However, it looks like we run _cpu_resume() instead of cpu_resume() so I still can't tell why this hasn't exploded. AFAICT, the active hyp vectors will have been freed (and possibly poisoned) long before KVM initialises.

My apologies. I managed to confuse myself (but hopefully not you) with the order of operations. Indeed, swsups_free() happens *before* KVM initialisation which eventually happens later via syscore_resume().

I can now reproduce the problem with CONFIG_PAGE_POISONING=y and page_poison=on

# swapon /dev/vdb
[   55.330849] Adding 2097148k swap on /dev/vdb.  Priority:-1 extents:1 across:2097148k 
# echo test_resume > /sys/power/disk
# echo disk > /sys/power/state
[   62.534444] PM: hibernation: hibernation entry
[   62.578411] Filesystems sync: 0.021 seconds
[   62.579550] Freezing user space processes
[   62.585909] Freezing user space processes completed (elapsed 0.005 seconds)
[   62.586835] OOM killer disabled.
[   62.588332] Freezing remaining freezable tasks
[   62.591318] Freezing remaining freezable tasks completed (elapsed 0.002 seconds)
[   62.593983] PM: hibernation: Preallocating image memory
[   62.972326] PM: hibernation: Allocated 25745 pages for snapshot
[   62.973119] PM: hibernation: Allocated 102980 kbytes in 0.37 seconds (278.32 MB/s)
[   63.011607] Disabling non-boot CPUs ...
[   63.021633] virtio_blk virtio0: 1/0/0 default/read/poll queues
[   63.025486] virtio_blk virtio3: 1/0/0 default/read/poll queues
[   63.035080] hibernate: Hibernating on CPU 0 [mpidr:0x0]
[   63.039859] PM: Using 1 thread(s) for lzo compression
[   63.040459] PM: Compressing and saving image data (23314 pages)...
[   63.041558] PM: Image saving progress:   0%
[   63.247947] PM: Image saving progress:  10%
[   63.455279] PM: Image saving progress:  20%
[   63.619264] PM: Image saving progress:  30%
[   63.785097] PM: Image saving progress:  40%
[   63.924669] PM: Image saving progress:  50%
[   64.018580] PM: Image saving progress:  60%
[   64.116893] PM: Image saving progress:  70%
[   64.286410] PM: Image saving progress:  80%
[   64.490785] PM: Image saving progress:  90%
[   64.618506] PM: Image saving progress: 100%
[   64.621388] PM: hibernation: Wrote 93256 kbytes in 1.58 seconds (59.02 MB/s)
[   64.622108] PM: Image size after compression: 32298 kbytes
[   64.622702] PM: Image saving done
[   64.624592] PM: S|
[   64.777608] PM: Using 1 thread(s) for lzo decompression
[   64.778448] PM: Loading and decompressing image data (23314 pages)...
[   64.779237] hibernate: Hibernated on CPU 0 [mpidr:0x0]
[   64.877435] PM: Image loading progress:   0%
[   65.172066] PM: Image loading progress:  10%
[   65.273214] PM: Image loading progress:  20%
[   65.345142] PM: Image loading progress:  30%
[   65.454007] PM: Image loading progress:  40%
[   65.521889] PM: Image loading progress:  50%
[   65.595804] PM: Image loading progress:  60%
[   65.659942] PM: Image loading progress:  70%
[   65.720256] PM: Image loading progress:  80%
[   65.788729] PM: Image loading progress:  90%
[   65.851162] PM: Image loading progress: 100%
[   65.852057] PM: Image loading done
[   65.853050] PM: hibernation: Read 93256 kbytes in 1.07 seconds (87.15 MB/s)
[   65.900987] Disabling non-boot CPUs ...
[   63.012805] hibernate: Restored 0 MTE pages
[   63.012805] PM: hibernation: free pages cleared after restore

... hang here ...

GDB shows that we are executing code that looks like it came from the poisoned page.

Program received signal SIGINT, Interrupt.
0x000000004d158000 in ?? ()
1: x/i $pc
=> 0x4d158000: orn x10, x21, x10, asr #42

With the patch applied I do not see hang anymore so you can add

Quoted 14 lines

Speaking off-list to Mark R, he pointed me at some old rework he had for all of this:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/log/?h=arm64/suspend/cleanup

one of the patches there is very similar (identical?) to this one, but his series gets us to a point where we _do_ resume at cpu_resume() and, tbh, the whole thing ends up making a lot more sense.

If somebody could dust off that two-year old (!) stack, that would be great.

Will

maintainer Applied
#9 lore raw
In reply to Bradley Morgan
Quoted 8 lines · Bradley Morgan

swsusp_arch_suspend_exit() reinstalls the restored kernel's hyp stub vectors with an hvc, but never passes the arguments. x0 is not set to HVC_SET_VECTORS and x1 is not set to the vector address, so the stub dispatch falls through and returns without writing vbar_el2. EL2 is left pointing at the trans_pgd copy of the vectors, a page that swsusp_free() releases right after resume.

[...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: hibernate: pass HVC_SET_VECTORS args to the resume hvc

      https://git.kernel.org/arm64/c/955d86e5f3b9

Cheers,

Signature · 5 lines
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev