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
