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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

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.

Best regards,
Mykola