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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

Thank you for the review.

On Wed, Jun 3, 2026 at 3:46 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Ack.

>
> > --- 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 )
>

Ack.

> > @@ -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.

Ack.

>
> > @@ -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).

Ack.

>
> > +    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.

Ack.

Best regards,
Mykola