Re: [PATCH] arm64: hibernate: pass HVC_SET_VECTORS args to the resume hvc

Hi Will,

On 8/11/26 15:37, Will Deacon wrote:
> On Tue, Aug 11, 2026 at 01:50:14PM +0100, Bradley Morgan wrote:
>> On 11 August 2026 11:18:27 BST, Will Deacon <[email protected]> wrote:
>>> [+Maz, Pasha and James]
>>>
>>> On Sun, Aug 09, 2026 at 09:36:15PM +0000, Bradley Morgan wrote:
>>>> 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")
>>>> Cc: [email protected]
>>>> Signed-off-by: Bradley Morgan <[email protected]>
>>>> ---
>>>>  arch/arm64/kernel/hibernate-asm.S | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kernel/hibernate-asm.S
>>> b/arch/arm64/kernel/hibernate-asm.S
>>>> index 0e1d9c3c6a93..2baefe7a82d3 100644
>>>> --- a/arch/arm64/kernel/hibernate-asm.S
>>>> +++ b/arch/arm64/kernel/hibernate-asm.S
>>>> @@ -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)
>> 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!
> 
>>> 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,

Reviewed-by: Vladimir Murzin <[email protected]>


>>> 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?
> 
>>> 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
>