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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2025 14:43, Mykola Kvach wrote:
> On Mon, Aug 18, 2025 at 1:15 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 18.08.2025 10:49, Mykola Kvach wrote:
>>> @@ -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.
> 
> Note to self: s/Arm64/Arm/g
> 
>>> +         * - 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.
> 
> That's a good question. In earlier versions of this patch series, both I and
> some other contributors used existed fields from the domain structure, such as
> shutdown_code and is_shutting_down, for this purpose. However, I recall that
> in a previous review, this approach was not well received. See:
> https://lore.kernel.org/all/d24be446-af5a-7881-2db4-b25afac3e1f4@xxxxxxxxxx/
> 
> Technically, there is nothing preventing me from storing this information in
> the domain structure. However, I do not see much benefit in introducing a new
> field to the domain struct when there is already an existing per-vCPU flags
> field that tracks powerdown and pause states. Using one more bit in the
> pause_flags field seems sufficient and avoids further bloating the domain
> structure.

Hmm, yes, I was mis-remembering something here: I thought that much like we
have pause_count both for vCPU-s and for domains, we'd also have pause_flags
for both. Perhaps indeed okay as is then, as far as where to put the flag
goes.

Jan