arm64: hibernate: pass HVC_SET_VECTORS args to the resume hvc
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.
--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)
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)
Signature · 1 line
2.47.3
Will Deacon
maintainer[+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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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")Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)arch/arm64/kernel/hibernate-asm.S +0
-0
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
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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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
Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm64/kernel/hibernate-asm.Sarch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",
Quoted 4 lines
index
0e1d9c3c6a93..2baefe7a82d3100644arch/arm64/kernel/hibernate-asm.S +0
-0@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4", "dc civac, x4",
ARM64_WORKAROUND_CLEAN_CACHEarch/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
-0isb 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: Addtrans_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_exitassembly 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_VECTORShandling from this set of vectors.
No, this hvc uses it, and so does kexec.
Quoted 2 lines
Will
Thanks!
Will Deacon
maintainerQuoted 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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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
Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm64/kernel/hibernate-asm.Sarch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",
Quoted 4 lines
index
0e1d9c3c6a93..2baefe7a82d3100644arch/arm64/kernel/hibernate-asm.S +0
-0@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4", "dc civac, x4",
ARM64_WORKAROUND_CLEAN_CACHEarch/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
-0isb 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: Addtrans_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_vectorsafterwards.
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_exitassembly code entirely.No. After the restore
vbar_el2is 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_VECTORShandling 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
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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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
Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm64/kernel/hibernate-asm.Sarch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",
Quoted 4 lines
index
0e1d9c3c6a93..2baefe7a82d3100644arch/arm64/kernel/hibernate-asm.S +0
-0@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4", "dc civac, x4",
ARM64_WORKAROUND_CLEAN_CACHEarch/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
-0isb 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: Addtrans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernationpath.
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_vectorsafterwards.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_exitassembly code entirely.No. After the restore
vbar_el2is only writable from EL2, and the temporary copy cannot stay.swsusp_free()frees it right after resume.Isn't
vbar_el2always 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_VECTORShandling 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!
Vladimir Murzin
Reviewed-byHi 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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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
Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm64/kernel/hibernate-asm.Sarch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",
Quoted 4 lines
index
0e1d9c3c6a93..2baefe7a82d3100644arch/arm64/kernel/hibernate-asm.S +0
-0@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4", "dc civac, x4",
ARM64_WORKAROUND_CLEAN_CACHEarch/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
-0isb 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: Addtrans_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_vectorsafterwards.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_exitassembly code entirely.No. After the restore
vbar_el2is only writable from EL2, and the temporary copy cannot stay.swsusp_free()frees it right after resume.Isn't
vbar_el2always only writable from EL2?Quoted 4 lines
Quoted 3 lines
3. Whether we can then drop the
HVC_SET_VECTORShandling 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
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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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
Signed-off-by Bradley Morgan <[email protected]>--- arch/arm64/kernel/hibernate-asm.S | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm64/kernel/hibernate-asm.Sarch/arm64/kernel/hibernate-asm.S alternative_insn "dc cvau, x4", "dc civac, x4",
Quoted 4 lines
index
0e1d9c3c6a93..2baefe7a82d3100644arch/arm64/kernel/hibernate-asm.S +0
-0@@ -89,6 +89,8 @@ alternative_insn "dc cvau, x4", "dc civac, x4",
ARM64_WORKAROUND_CLEAN_CACHEarch/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
-0isb 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: Addtrans_pgd_copy_el2_vectors"), however this added arguments to the exception handler without updating the hypercall on the hibernationpath.
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_vectorsafterwards.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_EL2to__hyp_stub_vectors_Y.
swsusp_arch_resume()then temporarily (re)setsVBAR_EL2totrans_pgd_stub_vectors
while transitioning to image X:| if (
el2_reset_needed())
|__hyp_set_vectors(el2_vectors);Currently,
swsusp_arch_suspend_exit()attempts to restoreVBAR_EL2to__hyp_stub_vectors_X:| cbz x24, 3f /* Do we need to re-initialise EL2? */ | hvc #0where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of
__hyp_stub_vectorsfrom image X)However, that request is ignored, so
VBAR_EL2remains pointing totrans_pgd_stub_vectorsWe re-enter the kernel X.
During KVM initialisation, we call
__hyp_reset_vectors(). This is handled bytrans_pgd_stub_vectors, but ignored sinceHVC_RESET_VECTORSis not recognised. We then call__hyp_set_vectors(), which is also handled bytrans_pgd_stub_vectors, but this timeVBAR_EL2get set to__kvm_hyp_init_X.From that point on,
__kvm_hyp_init_Xhandles subsequent HVC calls, including the later call that redirectsVBAR_EL2to__kvm_hyp_host_vector_Xduring KVM setup.The memory containing
trans_pgd_stub_vectorsis freed later byswsusp_free(). That perhaps explains why we do not see any crash.With the proposed fix,
swsusp_arch_suspend_exit()successfully restoreVBAR_EL2to__hyp_stub_vectors_Xjust before we re-enter kernel X, and that vector is used for subsequent HVC calls.
__hyp_stub_vectorsis similar totrans_pgd_stub_vectors(or another way around?) - it handles bothHVC_SET_VECTORSandHVC_SOFT_RESTART, and ignoresHVC_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,
Reviewed-by Vladimir Murzin <[email protected]>
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_exitassembly code entirely.No. After the restore
vbar_el2is only writable from EL2, and the temporary copy cannot stay.swsusp_free()frees it right after resume.Isn't
vbar_el2always only writable from EL2?Quoted 3 lines
Quoted 3 lines
3. Whether we can then drop the
HVC_SET_VECTORShandling 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!
Will Deacon
maintainerHi 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_EL2to__hyp_stub_vectors_Y.
swsusp_arch_resume()then temporarily (re)setsVBAR_EL2totrans_pgd_stub_vectorswhile transitioning to image X:| if (
el2_reset_needed())
|__hyp_set_vectors(el2_vectors);Currently,
swsusp_arch_suspend_exit()attempts to restoreVBAR_EL2to__hyp_stub_vectors_X:| cbz x24, 3f /* Do we need to re-initialise EL2? */ | hvc #0where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of
__hyp_stub_vectorsfrom image X)However, that request is ignored, so
VBAR_EL2remains pointing totrans_pgd_stub_vectorsWe re-enter the kernel X.
During KVM initialisation, we call
__hyp_reset_vectors(). This is handled bytrans_pgd_stub_vectors, but ignored sinceHVC_RESET_VECTORSis not recognised. We then call__hyp_set_vectors(), which is also handled bytrans_pgd_stub_vectors, but this timeVBAR_EL2get set to__kvm_hyp_init_X.
Quoted 6 lines
From that point on,
__kvm_hyp_init_Xhandles subsequent HVC calls, including the later call that redirectsVBAR_EL2to__kvm_hyp_host_vector_Xduring KVM setup.The memory containing
trans_pgd_stub_vectorsis freed later byswsusp_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
Vladimir Murzin
Tested-byHi 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_EL2to__hyp_stub_vectors_Y.
swsusp_arch_resume()then temporarily (re)setsVBAR_EL2totrans_pgd_stub_vectorswhile transitioning to image X:| if (
el2_reset_needed())
|__hyp_set_vectors(el2_vectors);Currently,
swsusp_arch_suspend_exit()attempts to restoreVBAR_EL2to__hyp_stub_vectors_X:| cbz x24, 3f /* Do we need to re-initialise EL2? */ | hvc #0where x24 is resume_hdr.__hyp_stub_vectors (in other words snapshot of
__hyp_stub_vectorsfrom image X)However, that request is ignored, so
VBAR_EL2remains pointing totrans_pgd_stub_vectorsWe re-enter the kernel X.
During KVM initialisation, we call
__hyp_reset_vectors(). This is handled bytrans_pgd_stub_vectors, but ignored sinceHVC_RESET_VECTORSis not recognised. We then call__hyp_set_vectors(), which is also handled bytrans_pgd_stub_vectors, but this timeVBAR_EL2get set to__kvm_hyp_init_X.From that point on,
__kvm_hyp_init_Xhandles subsequent HVC calls, including the later call that redirectsVBAR_EL2to__kvm_hyp_host_vector_Xduring KVM setup.
Quoted 9 lines
Quoted 2 lines
The memory containing
trans_pgd_stub_vectorsis freed later byswsusp_free(). That perhaps explains why we do not see any crash.I was trying to figure out if we end up initialising
VBAR_EL2before we "return" fromswsusp_arch_suspend()and free the reserved pages. However, it looks like we run_cpu_resume()instead ofcpu_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
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 toHVC_SET_VECTORSand x1 is not set to the vector address, so the stub dispatch falls through and returns without writingvbar_el2. EL2 is left pointing at thetrans_pgdcopy of the vectors, a page thatswsusp_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