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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.05.2026 09:23, Mykola Kvach wrote:
> From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> 
> The shutdown flow currently uses is_shutting_down and is_shut_down to
> represent the domain shutdown lifecycle. The two flags are not mutually
> exclusive: after shutdown completion is_shutting_down remains set until
> domain_resume() clears both flags.
> 
> Replace the two booleans with an enum domain_shutdown_state. Keep
> domain_shutting_down() as the direct replacement for the old
> is_shutting_down flag: it is true once shutdown has been initiated and
> remains true after completion, until domain_resume(). Add
> domain_shutdown_completed() for users that need the final completed
> state.
> 
> This makes the state transition explicit while avoiding a semantic split
> between "in progress" and "completed" at call sites where the old code
> only cared that shutdown had started and had not yet been reset by
> domain_resume().
> 
> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Mykola Kvach <mykola_kvach@xxxxxxxx>
> ---
> Changes in v3:
> - Keep domain_shutting_down() as a direct replacement for
>   is_shutting_down: true from shutdown start until domain_resume(),
>   including after shutdown completion.
> - Drop domain_in_shutdown_state().
> - Make old is_shutting_down conversions mechanical again; use
>   domain_shutdown_completed() only for old is_shut_down users.

And indeed this is now much easier to reason about, correctness-wise.

> @@ -442,7 +442,8 @@ bool shadow_prealloc(struct domain *d, unsigned int type, 
> unsigned int count)
>          count += paging_logdirty_levels();
>  
>      ret = _shadow_prealloc(d, count);
> -    if ( !ret && (!d->is_shutting_down || d->shutdown_code != 
> SHUTDOWN_crash) )
> +    if ( !ret && (!domain_shutting_down(d) ||
> +                  d->shutdown_code != SHUTDOWN_crash) )

Please can this be

    if ( !ret &&
         (!domain_shutting_down(d) || d->shutdown_code != SHUTDOWN_crash) )

? Overall less indentation and fewer pending open parentheses at line ends.

> --- a/xen/arch/x86/mm/shadow/multi.c
> +++ b/xen/arch/x86/mm/shadow/multi.c
> @@ -2370,7 +2370,8 @@ static int cf_check sh_page_fault(
>       * already used for some special purpose (ioreq pages, or granted pages).
>       * If that happens we'll have killed the guest already but it's still not
>       * safe to propagate entries out of the guest PT so get out now. */
> -    if ( unlikely(d->is_shutting_down && d->shutdown_code == SHUTDOWN_crash) 
> )
> +    if ( unlikely(domain_shutting_down(d) &&
> +                  d->shutdown_code == SHUTDOWN_crash) )

While at it please correct the bogus use of unlikely() as well:

    if ( unlikely(domain_shutting_down(d)) &&
         d->shutdown_code == SHUTDOWN_crash )

> @@ -2494,7 +2495,8 @@ static int cf_check sh_page_fault(
>           && ft == ft_demand_write )
>          sh_unsync(v, gmfn);
>  
> -    if ( unlikely(d->is_shutting_down && d->shutdown_code == SHUTDOWN_crash) 
> )
> +    if ( unlikely(domain_shutting_down(d) &&
> +                  d->shutdown_code == SHUTDOWN_crash) )

Same here then.

> @@ -382,6 +382,12 @@ struct domain_console {
>      char buf[256];
>  };
>  
> +enum domain_shutdown_state {
> +    DOMSHUTDOWN_none,

This likely deserves a comment, as it has to remain first (with value 0).

> +    DOMSHUTDOWN_in_progress,
> +    DOMSHUTDOWN_complete,
> +};

We further may want to make this a packed enum, such that ...

> @@ -552,10 +558,9 @@ struct domain
>      struct rangeset *iomem_caps;
>      struct rangeset *irq_caps;
>  
> -    /* Guest has shut down (inc. reason code)? */
> +    /* Guest shutdown state and associated reason code. */
>      spinlock_t       shutdown_lock;
> -    bool             is_shutting_down; /* in process of shutting down? */
> -    bool             is_shut_down;     /* fully shut down? */
> +    enum domain_shutdown_state shutdown_state;

... it occupies only a single byte here. We could then fit three other
booleans (or alike) next to it.

Jan