Re: [PATCH v2] xen/domain: make shutdown state explicit

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.06.2026 08:46, Mykola Kvach wrote:
> On Wed, Apr 8, 2026 at 4:06 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 01.04.2026 08:41, Mykola Kvach wrote:
>>> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
>>>
>>> The shutdown flow currently overloads is_shutting_down and
>>> is_shut_down to represent multiple phases of the shutdown lifecycle.
>>> Some users treat is_shutting_down narrowly as "shutdown still needs to
>>> be driven to completion", while others rely on it more broadly as
>>> "the domain is no longer in its normal running state".
>>>
>>> Make the lifecycle explicit by introducing enum
>>> domain_shutdown_state and helper predicates whose names match their
>>> semantics: domain_shutting_down() for the transient phase,
>>> domain_shutdown_completed() for the final state, and
>>> domain_in_shutdown_state() for checks that need the union of both.
>>>
>>> The conversion is intentionally not mechanical. The old flags were not
>>> mutually exclusive: once a domain became fully shut down,
>>> is_shutting_down remained set. As a result, sites that previously
>>> used the absence of is_shutting_down to exclude both the transient and
>>> completed states now use domain_in_shutdown_state(), sites that care
>>> specifically about the final state use domain_shutdown_completed(),
>>> and only paths that still have work to do before shutdown finalization
>>> use domain_shutting_down().
>>
>> In how far is the distinction between domain_shutting_down() and
>> domain_in_shutdown_state() really necessary? What you describe above
>> still leaves me pretty clueless as to almost(?) all of the uses of the
>> latter (where I think the former could be used just as well). Many
>> paths simply can't be taken anymore for a fully shut down domain, so
>> the distinction between the two is moot there.
>>
>> I'm also concerned of the name domain_in_shutdown_state() itself. To
>> me this is far closer to domain_shutdown_completed() than to the union
>> of both states. See how e.g. x86 CPUs can be "in shutdown state", i.e.
>> not responding to anything but a reset or init signal.
>>
>> IOW an entirely mechanical replacement might in fact be easier to look
>> at. And in the one rare case where the distinction might indeed be
>> relevant, "domain_shutting_down() || domain_shutdown_completed()" could
>> be used.
>>
>>> @@ -1423,9 +1423,12 @@ int domain_shutdown(struct domain *d, u8 reason)
>>>      return 0;
>>>  }
>>>
>>> -void domain_resume(struct domain *d)
>>> +int domain_resume(struct domain *d)
>>>  {
>>>      struct vcpu *v;
>>> +    enum domain_shutdown_state shutdown_state;
>>> +    unsigned int shutdown_code;
>>> +    int rc = 0;
>>>
>>>      /*
>>>       * Some code paths assume that shutdown status does not get reset under
>>> @@ -1435,10 +1438,18 @@ void domain_resume(struct domain *d)
>>>
>>>      spin_lock(&d->shutdown_lock);
>>>
>>> -    d->is_shutting_down = d->is_shut_down = 0;
>>> +    shutdown_state = d->shutdown_state;
>>> +    shutdown_code = d->shutdown_code;
>>> +
>>> +    if ( !domain_shutdown_completed(d) )
>>> +    {
>>> +        rc = -EINVAL;
>>> +        goto out_unlock;
>>> +    }
>>
>> I can't help the impression that you're mixing two things here - adjustment
>> to how shutdown state is tracked, and enforcement of completed shutdown
>> right here. This separate change likely would better be split out, and then
>> come with a description saying not only what is being changed, but also why,
>> and why the change is correct for both call sites.
> 
> In v3 I have dropped this part, so the patch no longer changes
> domain_resume() behaviour or its callers. domain_resume() remains void,
> and the patch only makes the existing shutdown state tracking explicit.
> 
> One follow-up question about the dropped change: do you think resume-state
> validation would be worth pursuing separately?
> 
> AFAICT, soft reset already has its own DOMCTL path, so I do not mean to treat
> XEN_DOMCTL_resumedomain as a soft-reset interface. The distinction I had in
> mind is rather that we may want different validation rules for the generic
> resume helper and for a resume-from-suspend path. In particular, soft reset
> could keep the current semantics, while a resume-from-suspend path would only
> clear the shutdown state if the domain had previously entered suspend.
> 
> If this distinction makes sense, I can look into it as a separate patch with
> its own justification for the affected call sites. Otherwise I will leave it
> out.

I guess I'd need to see a patch to really build an opinion. Generally yes,
state validation (even if e.g. only by way of assertions) is a useful thing
to have, I think.

Jan