From: Mirela Simonovic <mirela.simonovic@aggios.com>

These functions will be reused by suspend/resume support for ARM.

Signed-off-by: Mirela Simonovic <mirela.simonovic@aggios.com>
Signed-off-by: Saeed Nowshadi <saeed.nowshadi@xilinx.com>
Signed-off-by: Mykyta Poturai <mykyta_poturai@epam.com>
Signed-off-by: Mykola Kvach <mykola_kvach@epam.com>
---
 xen/arch/x86/acpi/power.c | 29 -----------------------------
 xen/common/domain.c       | 30 ++++++++++++++++++++++++++++++
 xen/include/xen/sched.h   |  3 +++
 3 files changed, 33 insertions(+), 29 deletions(-)

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index d0b67614d5..f38398827e 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -137,35 +137,6 @@ static void device_power_up(enum dev_power_saved saved)
     }
 }
 
-static void freeze_domains(void)
-{
-    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();
-}
-
-static void thaw_domains(void)
-{
-    struct domain *d;
-
-    scheduler_enable();
-
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain ( d )
-        domain_unpause(d);
-    rcu_read_unlock(&domlist_read_lock);
-}
-
 static void acpi_sleep_prepare(u32 state)
 {
     void *wakeup_vector_va;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 0c4cc77111..49ff84d2f5 100644
--- 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)
+{
+    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();
+}
+
+void thaw_domains(void)
+{
+    struct domain *d;
+
+    scheduler_enable();
+
+    rcu_read_lock(&domlist_read_lock);
+    for_each_domain ( d )
+        domain_unpause(d);
+    rcu_read_unlock(&domlist_read_lock);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 037c83fda2..177784e6da 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -1059,6 +1059,9 @@ static inline struct vcpu *domain_vcpu(const struct domain *d,
     return vcpu_id >= d->max_vcpus ? NULL : d->vcpu[idx];
 }
 
+void freeze_domains(void);
+void thaw_domains(void);
+
 void cpu_init(void);
 
 /*
-- 
2.43.0

On 05.03.2025 10:11, Mykola Kvach wrote:
> From: Mirela Simonovic <mirela.simonovic@aggios.com>
> 
> 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.

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

Jan

On Wed, Mar 5, 2025 at 6:48 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 05.03.2025 10:11, Mykola Kvach wrote:
> > From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >
> > 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

On 19.03.2025 13:03, Mykola Kvach wrote:
> On Wed, Mar 5, 2025 at 6:48 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 05.03.2025 10:11, Mykola Kvach wrote:
>>> From: Mirela Simonovic <mirela.simonovic@aggios.com>
>>>
>>> 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.

I wasn't after a rename really; my suggestion was to leave the scheduler
calls at the original call sites, and remove them from here.

Jan

On Wed, Mar 19, 2025 at 2:47 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 19.03.2025 13:03, Mykola Kvach wrote:
> > On Wed, Mar 5, 2025 at 6:48 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 05.03.2025 10:11, Mykola Kvach wrote:
> >>> From: Mirela Simonovic <mirela.simonovic@aggios.com>
> >>>
> >>> 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.
>
> I wasn't after a rename really; my suggestion was to leave the scheduler
> calls at the original call sites, and remove them from here.

got it, thank you

>
> Jan