Re: [PATCH v9 1/4] xen/arm: Implement PSCI SYSTEM_SUSPEND call for guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2025 10:49, Mykola Kvach wrote:
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -90,6 +90,16 @@ static void ctxt_switch_from(struct vcpu *p)
>      if ( is_idle_vcpu(p) )
>          return;
>  
> +    /* Arch timer */
> +    p->arch.cntkctl = READ_SYSREG(CNTKCTL_EL1);
> +    virt_timer_save(p);
> +
> +    /* VGIC */
> +    gic_save_state(p);
> +
> +    if ( test_bit(_VPF_suspended, &p->pause_flags) )
> +        return;

As I had to look at the Arm side uses of the new bit to at least try to
follow the comment further down, I came across this. And now I wonder:
Why would one of the pause flags need special casing here?

> @@ -1360,13 +1357,33 @@ void domain_resume(struct domain *d)
>  
>      for_each_vcpu ( d, v )
>      {
> +        /*
> +         * No need to conditionally clear _VPF_suspended here:
> +         * - This bit is only set on Arm64, and only after a successful 
> suspend.
> +         * - domain_resume_nopause() may also be called from paths other than
> +         *   the suspend/resume flow, such as "soft-reset" actions (e.g.,
> +         *   on_poweroff), as part of the Xenstore control/shutdown protocol.
> +         *   These require guest acknowledgement to complete the operation.
> +         * So clearing the bit unconditionally is safe.
> +         */
> +        clear_bit(_VPF_suspended, &v->pause_flags);

Seeing that you set this bit for a single vCPU in a domain only, I wonder why
it needs to be a per-vCPU flag.

Apart from this, with the comment I still fear I wouldn't feel capable of
ack-ing this, as there's too much Arm-only interaction in here. It's not even
clear whether this could easily be re-used by another port.

Jan