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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> @@ -1448,9 +1459,17 @@ void domain_resume(struct domain *d)
>          v->paused_for_shutdown = 0;
>      }
>  
> + out_unlock:
>      spin_unlock(&d->shutdown_lock);
>  
>      domain_unpause(d);
> +
> +    if ( rc )
> +        dprintk(XENLOG_WARNING,
> +                "%pd: Invalid domain state for resume: shutdown_state=%u, 
> shutdown_code=%u\n",
> +                d, shutdown_state, shutdown_code);

The shutdown code doesn't affect whether the function would fail. The extra
variable just to log it anyway is therefore a little irritating. Is there
any reason to this that I'm overlooking?

Jan