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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.08.2025 13:04, Mykola Kvach wrote:
> On Fri, Aug 29, 2025 at 10:45 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 28.08.2025 18:32, Volodymyr Babchuk wrote:
>>> Mykola Kvach <xakep.amatop@xxxxxxxxx> writes:
>>>> @@ -880,6 +883,25 @@ void arch_domain_creation_finished(struct domain *d)
>>>>      p2m_domain_creation_finished(d);
>>>>  }
>>>>
>>>> +int arch_domain_resume(struct domain *d)
>>>> +{
>>>> +    int rc;
>>>> +    typeof(d->arch.resume_ctx) *ctx = &d->arch.resume_ctx;
>>>> +
>>>> +    if ( !d->is_shutting_down || d->shutdown_code != SHUTDOWN_suspend )
>>>> +    {
>>>> +        dprintk(XENLOG_WARNING,
>>>> +                "%pd: Invalid domain state for resume: 
>>>> is_shutting_down=%d, shutdown_code=%d\n",
>>>> +                d, d->is_shutting_down, d->shutdown_code);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> This check probably can go into common domain_resume() function, as
>>> there is nothing arch-specific in here. Probably this can be done during
>>> commit, to save us from v12? If commiters are okay with this.
>>
>> Seeing this, I'm learning that ->is_shutting_down can actually go from
>> true to false. Which I think is a problem (not of this series, but a pre-
>> existing one, see [1]). Code elsewhere assumes this to not be possible,
>> see e.g. __domain_crash(), but I'm also unconvinced that e.g. the shutdown
>> deferral logic is prepared to deal with such a case.
> 
> Should I add an extra patch to this series just for fixing this issue,
> or can I do it within the scope of this one?

I'm happy to see you fix this (I don't even see how we could easily do so),
but definitely in a separate patch (and possibly even independent of this
series). My remark wasn't meant to get in the way of your work, it was just
something I noticed and thought would be relevant to call out.

Jan