Re: [PATCH 02/16] xen/x86: Move freeze/thaw_domains into common files

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

On Wed, Mar 5, 2025 at 6:48 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 05.03.2025 10:11, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@xxxxxxxxxx>
> >
> > These functions will be reused by suspend/resume support for ARM.
>
> And until then they are going to violate the Misra rule requiring there
> to not be unreachable code.
>
> > --- a/xen/common/domain.c
> > +++ b/xen/common/domain.c
> > @@ -2259,6 +2259,36 @@ int continue_hypercall_on_cpu(
> >      return 0;
> >  }
> >
> > +
> > +void freeze_domains(void)
>
> Nit: No double blank lines please.

Thanks for pointing that out! I'll fix it in the next version of the
patch series.

>
> > +{
> > +    struct domain *d;
> > +
> > +    rcu_read_lock(&domlist_read_lock);
> > +    /*
> > +     * Note that we iterate in order of domain-id. Hence we will pause dom0
> > +     * first which is required for correctness (as only dom0 can add 
> > domains to
> > +     * the domain list). Otherwise we could miss concurrently-created 
> > domains.
> > +     */
> > +    for_each_domain ( d )
> > +        domain_pause(d);
> > +    rcu_read_unlock(&domlist_read_lock);
> > +
> > +    scheduler_disable();
>
> When made generally available I'm unsure having this and ...
>
> > +}
> > +
> > +void thaw_domains(void)
> > +{
> > +    struct domain *d;
> > +
> > +    scheduler_enable();
>
> ... this here is a good idea. Both scheduler operations aren't related
> to what the function names say is being done here.

I have just moved these functions from x86-specific headers to a common one,
but they are still used only for suspend/resume purposes.
It's not a problem for me to adjust the names slightly in the next
version of the
patch series.

>
> Jan

Best regards,
~Mykola