Re: [PATCH v9 10/13] xen/arm64: Save/restore CPU context across SYSTEM_SUSPEND

[Oleksandr Tyshchenko <olekstysh@xxxxxxxxx>]

On 5/14/26 20:20, Luca Fancellu wrote:
> Hi Mykola,
Hello Mykola and Luca

Mykola, I have no further comments on this patch, but I think Luca has 
raised a valid point. Once that is resolved (or clarified why no change 
is needed), feel free to add my:
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@xxxxxxxx>

see below

> > +#ifdef CONFIG_SYSTEM_SUSPEND
> > +/*
> > + * int prepare_resume_ctx(void)
> > + *
> > + * CPU context saved here will be restored on resume in hyp_resume function.
> > + * prepare_resume_ctx shall return a non-zero value. Upon restoring context
> > + * hyp_resume shall return value zero instead. From C code that invokes
> > + * prepare_resume_ctx, the return value is interpreted to determine whether
> > + * the context is saved (prepare_resume_ctx) or restored (hyp_resume).
> > + */
> > +FUNC(prepare_resume_ctx)
> > +        ldr   x0, =resume_cpu_context
> > +
> > +        /* Store callee-saved registers */
> > +        stp   x19, x20, [x0, #RESUME_CTX_X19]
> > +        stp   x21, x22, [x0, #RESUME_CTX_X21]
> > +        stp   x23, x24, [x0, #RESUME_CTX_X23]
> > +        stp   x25, x26, [x0, #RESUME_CTX_X25]
> > +        stp   x27, x28, [x0, #RESUME_CTX_X27]
> > +        stp   x29, lr, [x0, #RESUME_CTX_X29]
> > +
> > +        /* Store stack-pointer */
> > +        mov   x2, sp
> > +        str   x2, [x0, #RESUME_CTX_SP]
> > +
> > +        /* Store system control registers */
> > +        mrs   x2, VBAR_EL2
> > +        str   x2, [x0, #RESUME_CTX_VBAR_EL2]
> > +        mrs   x2, VTCR_EL2
> > +        str   x2, [x0, #RESUME_CTX_VTCR_EL2]
> > +        mrs   x2, VTTBR_EL2
> > +        str   x2, [x0, #RESUME_CTX_VTTBR_EL2]
> > +        mrs   x2, TPIDR_EL2
> > +        str   x2, [x0, #RESUME_CTX_TPIDR_EL2]
> > +        mrs   x2, MDCR_EL2
> > +        str   x2, [x0, #RESUME_CTX_MDCR_EL2]
> > +        mrs   x2, HSTR_EL2
> > +        str   x2, [x0, #RESUME_CTX_HSTR_EL2]
> > +        mrs   x2, CPTR_EL2
> > +        str   x2, [x0, #RESUME_CTX_CPTR_EL2]
> > +        mrs   x2, HCR_EL2
> > +        str   x2, [x0, #RESUME_CTX_HCR_EL2]
> Do you think we should save also CNTHCTL_EL2? Apologies it escaped my first 
> review,
> but I see we program it in the boot cpu path + secondary cpu path: 
> init_timer_interrupt().

Yes, CNTHCTL_EL2 is programmed by init_timer_interrupt() during the 
initialization of both boot and secondary CPUs. Whether it needs to be 
saved here depends on the resume path:
 - If the resume handler (after prepare_resume_ctx() returns 0) 
re-invokes timer initialization, it is already covered.
 - If not, CNTHCTL_EL2 should be added to struct resume_cpu_context and 
included in the save/restore assembly.
If I understand the current flow correctly:
 - Secondary CPUs wake up via hotplug, so they execute 
start_secondary()->init_timer_interrupt() and configure CNTHCTL_EL2.
 - The boot CPU wakes up via hyp_resume() and jumps straight back to C 
code, which appears to bypass init_timer_interrupt() call. At least I 
have not spotted where system_suspend() would re-invoke it.

> The rest looks ok.
>
> Cheers,
> Luca