Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-15 Thread Julien Grall 



On 11/15/18 6:57 PM, Stefano Stabellini wrote:

On Wed, 14 Nov 2018, Julien Grall wrote:

On 14/11/2018 23:04, Stefano Stabellini wrote:

On Wed, 14 Nov 2018, Julien Grall wrote:

    -    return -ENOSYS;


Why do you return -ENOSYS until this patch? Should not have it be 0?



To distinguish that Xen suspend wasn't supported until we at least do
something useful in suspend procedure. This is not so important, we can
return 0 but needs to be fixed in previous patches.


If you return 0 before hand you can more easily bisect this series and know
where it suspend/resume breaks.


Why 0 improves bisectability? 0 prevents other checks from figuring out
that there was an error.


But this code is exactly replacing -ENOSYS by state (that would be 0 in
success. So what's wrong to return 0 rather than -ENOSYS in that patch
implement the dummy system_state?


Also, the feature is not bisectable until the
full series is applied.


Really? I was under the impression you can still do some sort of
suspend/resume patch by patch. Although, you would not do a full
suspend/resume.


You are saying that we could call the function and return successfully
even if the function does nothing, simply by returning 0. That would
make suspend bisectable within this series, patch by patch.

I think it's impressive that Mirela managed to write the series this
way, and if suspend is actually bisectable patch by patch simply by
returning 0 here, it would be amazing, and certainly worth doing.
However, if it is not the case, I wouldn't ask Mirela to make the effort
to make suspend bisectable patch by patch beyond returning 0 here, it
would be good to have but not required.


This wasn't my intention :).

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-15 Thread Stefano Stabellini 
On Wed, 14 Nov 2018, Julien Grall wrote:
> On 14/11/2018 23:04, Stefano Stabellini wrote:
> > On Wed, 14 Nov 2018, Julien Grall wrote:
> >> Hi Mirela,
> >>
> >> On 14/11/2018 13:00, Mirela Simonovic wrote:
> >>>
> >>>
> >>> On 11/14/2018 11:52 AM, Julien Grall wrote:
>  Hi Mirela,
> 
>  On 12/11/2018 11:30, Mirela Simonovic wrote:
> > Non-boot CPUs have to be disabled on suspend and enabled on resume
> > (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> > CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> > platform capabilities, this may lead to the physical powering down of
> > CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> > each non-boot CPU).
> >
> > Signed-off-by: Mirela Simonovic 
> > Signed-off-by: Saeed Nowshadi 
> > ---
> >    xen/arch/arm/suspend.c | 15 ++-
> >    1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index 575afd5eb8..dae1b1f7d6 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -1,4 +1,5 @@
> >    #include 
> > +#include 
> >    #include 
> >    #include 
> >    #include 
> > @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint,
> > register_t cid)
> >    /* Xen suspend. Note: data is not used (suspend is the suspend to 
> > RAM)
> > */
> >    static long system_suspend(void *data)
> >    {
> > +    int status;
> > +
> >    BUG_ON(system_state != SYS_STATE_active);
> >      system_state = SYS_STATE_suspend;
> >    freeze_domains();
> >    +    status = disable_nonboot_cpus();
> > +    if ( status )
> > +    {
> > +    system_state = SYS_STATE_resume;
> > +    goto resume_nonboot_cpus;
> > +    }
> > +
> >    system_state = SYS_STATE_resume;
> >    +resume_nonboot_cpus:
> > +    enable_nonboot_cpus();
> >    thaw_domains();
> >    system_state = SYS_STATE_active;
> > +    dsb(sy);
> 
>  Why do you need a dsb(sy) here?
> 
> >>>
> >>> Updated value of system_state variable needs to be visible to other CPUs
> >>> before we move on
> >>
> >> We tend to write the reason on top of barrier why they are necessary. But 
> >> I am
> >> still unsure to understand why this is important. What would happen if 
> >> move on
> >> without it?
> > 
> > That is a good question. I went through the code and it seems that the
> > only effect could be potentially taking the wrong path in
> > cpupool_cpu_add, but since that's called from a .notifier_call I don't
> > think it can happen in practice. It is always difficult to prove that
> > we don't need a barrier, it is easier to prove when we need a barrier,
> > but in this case it does look like we do not need it after all.
> 
> It is also very easy to add barrier everywhere so we are sure what to do 
> ;). If you need a barrier, then you need to give plausible explanation.
> 
> In that case, if you need barrier here for system_state. Then what 
> wouldn't you need it in other places where system_state is updated/read?

Right, no plausible explanation here, so no barrier.


> >   
> >    -    return -ENOSYS;
> 
>  Why do you return -ENOSYS until this patch? Should not have it be 0?
> 
> >>>
> >>> To distinguish that Xen suspend wasn't supported until we at least do
> >>> something useful in suspend procedure. This is not so important, we can
> >>> return 0 but needs to be fixed in previous patches.
> >>
> >> If you return 0 before hand you can more easily bisect this series and know
> >> where it suspend/resume breaks.
> > 
> > Why 0 improves bisectability? 0 prevents other checks from figuring out
> > that there was an error.
> 
> But this code is exactly replacing -ENOSYS by state (that would be 0 in 
> success. So what's wrong to return 0 rather than -ENOSYS in that patch 
> implement the dummy system_state?
> 
> > Also, the feature is not bisectable until the
> > full series is applied.
> 
> Really? I was under the impression you can still do some sort of 
> suspend/resume patch by patch. Although, you would not do a full 
> suspend/resume.

You are saying that we could call the function and return successfully
even if the function does nothing, simply by returning 0. That would
make suspend bisectable within this series, patch by patch.

I think it's impressive that Mirela managed to write the series this
way, and if suspend is actually bisectable patch by patch simply by
returning 0 here, it would be amazing, and certainly worth doing.
However, if it is not the case, I wouldn't ask Mirela to make the effort
to make suspend bisectable patch by patch beyond returning 0 here, it
would be good to have but not required.___
Xen-devel mailing list

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Julien Grall 


On 14/11/2018 23:04, Stefano Stabellini wrote:
> On Wed, 14 Nov 2018, Julien Grall wrote:
>> Hi Mirela,
>>
>> On 14/11/2018 13:00, Mirela Simonovic wrote:
>>>
>>>
>>> On 11/14/2018 11:52 AM, Julien Grall wrote:
 Hi Mirela,

 On 12/11/2018 11:30, Mirela Simonovic wrote:
> Non-boot CPUs have to be disabled on suspend and enabled on resume
> (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> platform capabilities, this may lead to the physical powering down of
> CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> each non-boot CPU).
>
> Signed-off-by: Mirela Simonovic 
> Signed-off-by: Saeed Nowshadi 
> ---
>    xen/arch/arm/suspend.c | 15 ++-
>    1 file changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> index 575afd5eb8..dae1b1f7d6 100644
> --- a/xen/arch/arm/suspend.c
> +++ b/xen/arch/arm/suspend.c
> @@ -1,4 +1,5 @@
>    #include 
> +#include 
>    #include 
>    #include 
>    #include 
> @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint,
> register_t cid)
>    /* Xen suspend. Note: data is not used (suspend is the suspend to RAM)
> */
>    static long system_suspend(void *data)
>    {
> +    int status;
> +
>    BUG_ON(system_state != SYS_STATE_active);
>      system_state = SYS_STATE_suspend;
>    freeze_domains();
>    +    status = disable_nonboot_cpus();
> +    if ( status )
> +    {
> +    system_state = SYS_STATE_resume;
> +    goto resume_nonboot_cpus;
> +    }
> +
>    system_state = SYS_STATE_resume;
>    +resume_nonboot_cpus:
> +    enable_nonboot_cpus();
>    thaw_domains();
>    system_state = SYS_STATE_active;
> +    dsb(sy);

 Why do you need a dsb(sy) here?

>>>
>>> Updated value of system_state variable needs to be visible to other CPUs
>>> before we move on
>>
>> We tend to write the reason on top of barrier why they are necessary. But I 
>> am
>> still unsure to understand why this is important. What would happen if move 
>> on
>> without it?
> 
> That is a good question. I went through the code and it seems that the
> only effect could be potentially taking the wrong path in
> cpupool_cpu_add, but since that's called from a .notifier_call I don't
> think it can happen in practice. It is always difficult to prove that
> we don't need a barrier, it is easier to prove when we need a barrier,
> but in this case it does look like we do not need it after all.

It is also very easy to add barrier everywhere so we are sure what to do 
;). If you need a barrier, then you need to give plausible explanation.

In that case, if you need barrier here for system_state. Then what 
wouldn't you need it in other places where system_state is updated/read?
> 
>   
>    -    return -ENOSYS;

 Why do you return -ENOSYS until this patch? Should not have it be 0?

>>>
>>> To distinguish that Xen suspend wasn't supported until we at least do
>>> something useful in suspend procedure. This is not so important, we can
>>> return 0 but needs to be fixed in previous patches.
>>
>> If you return 0 before hand you can more easily bisect this series and know
>> where it suspend/resume breaks.
> 
> Why 0 improves bisectability? 0 prevents other checks from figuring out
> that there was an error.

But this code is exactly replacing -ENOSYS by state (that would be 0 in 
success. So what's wrong to return 0 rather than -ENOSYS in that patch 
implement the dummy system_state?

> Also, the feature is not bisectable until the
> full series is applied.

Really? I was under the impression you can still do some sort of 
suspend/resume patch by patch. Although, you would not do a full 
suspend/resume.

Cheers,

-- 
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Stefano Stabellini 
On Wed, 14 Nov 2018, Julien Grall wrote:
> Hi Mirela,
> 
> On 14/11/2018 13:00, Mirela Simonovic wrote:
> > 
> > 
> > On 11/14/2018 11:52 AM, Julien Grall wrote:
> > > Hi Mirela,
> > > 
> > > On 12/11/2018 11:30, Mirela Simonovic wrote:
> > > > Non-boot CPUs have to be disabled on suspend and enabled on resume
> > > > (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> > > > CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> > > > platform capabilities, this may lead to the physical powering down of
> > > > CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> > > > each non-boot CPU).
> > > > 
> > > > Signed-off-by: Mirela Simonovic 
> > > > Signed-off-by: Saeed Nowshadi 
> > > > ---
> > > >   xen/arch/arm/suspend.c | 15 ++-
> > > >   1 file changed, 14 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > > > index 575afd5eb8..dae1b1f7d6 100644
> > > > --- a/xen/arch/arm/suspend.c
> > > > +++ b/xen/arch/arm/suspend.c
> > > > @@ -1,4 +1,5 @@
> > > >   #include 
> > > > +#include 
> > > >   #include 
> > > >   #include 
> > > >   #include 
> > > > @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint,
> > > > register_t cid)
> > > >   /* Xen suspend. Note: data is not used (suspend is the suspend to RAM)
> > > > */
> > > >   static long system_suspend(void *data)
> > > >   {
> > > > +    int status;
> > > > +
> > > >   BUG_ON(system_state != SYS_STATE_active);
> > > >     system_state = SYS_STATE_suspend;
> > > >   freeze_domains();
> > > >   +    status = disable_nonboot_cpus();
> > > > +    if ( status )
> > > > +    {
> > > > +    system_state = SYS_STATE_resume;
> > > > +    goto resume_nonboot_cpus;
> > > > +    }
> > > > +
> > > >   system_state = SYS_STATE_resume;
> > > >   +resume_nonboot_cpus:
> > > > +    enable_nonboot_cpus();
> > > >   thaw_domains();
> > > >   system_state = SYS_STATE_active;
> > > > +    dsb(sy);
> > > 
> > > Why do you need a dsb(sy) here?
> > > 
> > 
> > Updated value of system_state variable needs to be visible to other CPUs
> > before we move on
> 
> We tend to write the reason on top of barrier why they are necessary. But I am
> still unsure to understand why this is important. What would happen if move on
> without it?

That is a good question. I went through the code and it seems that the
only effect could be potentially taking the wrong path in
cpupool_cpu_add, but since that's called from a .notifier_call I don't
think it can happen in practice. It is always difficult to prove that
we don't need a barrier, it is easier to prove when we need a barrier,
but in this case it does look like we do not need it after all.

 
> > > >   -    return -ENOSYS;
> > > 
> > > Why do you return -ENOSYS until this patch? Should not have it be 0?
> > > 
> > 
> > To distinguish that Xen suspend wasn't supported until we at least do
> > something useful in suspend procedure. This is not so important, we can
> > return 0 but needs to be fixed in previous patches.
> 
> If you return 0 before hand you can more easily bisect this series and know
> where it suspend/resume breaks.

Why 0 improves bisectability? 0 prevents other checks from figuring out
that there was an error. Also, the feature is not bisectable until the
full series is applied.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Julien Grall 

Hi Mirela,

On 14/11/2018 13:00, Mirela Simonovic wrote:



On 11/14/2018 11:52 AM, Julien Grall wrote:

Hi Mirela,

On 12/11/2018 11:30, Mirela Simonovic wrote:

Non-boot CPUs have to be disabled on suspend and enabled on resume
(hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
CPU_OFF to be called by each non-boot CPU. Depending on the underlying
platform capabilities, this may lead to the physical powering down of
CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
each non-boot CPU).

Signed-off-by: Mirela Simonovic 
Signed-off-by: Saeed Nowshadi 
---
  xen/arch/arm/suspend.c | 15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index 575afd5eb8..dae1b1f7d6 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -1,4 +1,5 @@
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, register_t 
cid)

  /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
  static long system_suspend(void *data)
  {
+    int status;
+
  BUG_ON(system_state != SYS_STATE_active);
    system_state = SYS_STATE_suspend;
  freeze_domains();
  +    status = disable_nonboot_cpus();
+    if ( status )
+    {
+    system_state = SYS_STATE_resume;
+    goto resume_nonboot_cpus;
+    }
+
  system_state = SYS_STATE_resume;
  +resume_nonboot_cpus:
+    enable_nonboot_cpus();
  thaw_domains();
  system_state = SYS_STATE_active;
+    dsb(sy);


Why do you need a dsb(sy) here?



Updated value of system_state variable needs to be visible to other CPUs before 
we move on


We tend to write the reason on top of barrier why they are necessary. But I am 
still unsure to understand why this is important. What would happen if move on 
without it?





  -    return -ENOSYS;


Why do you return -ENOSYS until this patch? Should not have it be 0?



To distinguish that Xen suspend wasn't supported until we at least do something 
useful in suspend procedure. This is not so important, we can return 0 but needs 
to be fixed in previous patches.


If you return 0 before hand you can more easily bisect this series and know 
where it suspend/resume breaks.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Mirela Simonovic 



On 11/14/2018 11:52 AM, Julien Grall wrote:

Hi Mirela,

On 12/11/2018 11:30, Mirela Simonovic wrote:

Non-boot CPUs have to be disabled on suspend and enabled on resume
(hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
CPU_OFF to be called by each non-boot CPU. Depending on the underlying
platform capabilities, this may lead to the physical powering down of
CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
each non-boot CPU).

Signed-off-by: Mirela Simonovic 
Signed-off-by: Saeed Nowshadi 
---
  xen/arch/arm/suspend.c | 15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index 575afd5eb8..dae1b1f7d6 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -1,4 +1,5 @@
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, 
register_t cid)
  /* Xen suspend. Note: data is not used (suspend is the suspend to 
RAM) */

  static long system_suspend(void *data)
  {
+int status;
+
  BUG_ON(system_state != SYS_STATE_active);
system_state = SYS_STATE_suspend;
  freeze_domains();
  +status = disable_nonboot_cpus();
+if ( status )
+{
+system_state = SYS_STATE_resume;
+goto resume_nonboot_cpus;
+}
+
  system_state = SYS_STATE_resume;
  +resume_nonboot_cpus:
+enable_nonboot_cpus();
  thaw_domains();
  system_state = SYS_STATE_active;
+dsb(sy);


Why do you need a dsb(sy) here?



Updated value of system_state variable needs to be visible to other CPUs 
before we move on



  -return -ENOSYS;


Why do you return -ENOSYS until this patch? Should not have it be 0?



To distinguish that Xen suspend wasn't supported until we at least do 
something useful in suspend procedure. This is not so important, we can 
return 0 but needs to be fixed in previous patches.



+return status;
  }
int32_t domain_suspend(register_t epoint, register_t cid)



Cheers,




___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Julien Grall 

Hi,

On 13/11/2018 22:35, Stefano Stabellini wrote:

On Mon, 12 Nov 2018, Mirela Simonovic wrote:

Non-boot CPUs have to be disabled on suspend and enabled on resume
(hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
CPU_OFF to be called by each non-boot CPU. Depending on the underlying
platform capabilities, this may lead to the physical powering down of
CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
each non-boot CPU).

Signed-off-by: Mirela Simonovic 
Signed-off-by: Saeed Nowshadi 
---
  xen/arch/arm/suspend.c | 15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index 575afd5eb8..dae1b1f7d6 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -1,4 +1,5 @@
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, register_t 
cid)
  /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
  static long system_suspend(void *data)
  {
+int status;
+
  BUG_ON(system_state != SYS_STATE_active);
  
  system_state = SYS_STATE_suspend;

  freeze_domains();
  
+status = disable_nonboot_cpus();

+if ( status )
+{
+system_state = SYS_STATE_resume;
+goto resume_nonboot_cpus;
+}
+
  system_state = SYS_STATE_resume;
  
+resume_nonboot_cpus:

+enable_nonboot_cpus();
  thaw_domains();
  system_state = SYS_STATE_active;
+dsb(sy);
  
-return -ENOSYS;

+return status;
  }


I think we need a spin_lock to protect system_suspend from concurrent
calls, or (better) we need to make sure that the caller is only allowed
to call system_suspend if there is just one vcpu active in the system,
and that vcpu is blocked on this PSCI system suspend call.


I don't think this is correct. It is valid to have more than on vCPU running 
when calling system_suspend. An example would be Dom0 suspending while the other 
domain are still running.


This is handled properly via freeze_domains()/thaw_domains().

So a spinlock would be a better solution here. If we can't acquire the lock then 
the function would return -EBUSY.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-14 Thread Julien Grall 

Hi Mirela,

On 12/11/2018 11:30, Mirela Simonovic wrote:

Non-boot CPUs have to be disabled on suspend and enabled on resume
(hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
CPU_OFF to be called by each non-boot CPU. Depending on the underlying
platform capabilities, this may lead to the physical powering down of
CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
each non-boot CPU).

Signed-off-by: Mirela Simonovic 
Signed-off-by: Saeed Nowshadi 
---
  xen/arch/arm/suspend.c | 15 ++-
  1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index 575afd5eb8..dae1b1f7d6 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -1,4 +1,5 @@
  #include 
+#include 
  #include 
  #include 
  #include 
@@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, register_t 
cid)
  /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
  static long system_suspend(void *data)
  {
+int status;
+
  BUG_ON(system_state != SYS_STATE_active);
  
  system_state = SYS_STATE_suspend;

  freeze_domains();
  
+status = disable_nonboot_cpus();

+if ( status )
+{
+system_state = SYS_STATE_resume;
+goto resume_nonboot_cpus;
+}
+
  system_state = SYS_STATE_resume;
  
+resume_nonboot_cpus:

+enable_nonboot_cpus();
  thaw_domains();
  system_state = SYS_STATE_active;
+dsb(sy);


Why do you need a dsb(sy) here?

  
-return -ENOSYS;


Why do you return -ENOSYS until this patch? Should not have it be 0?


+return status;
  }
  
  int32_t domain_suspend(register_t epoint, register_t cid)




Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-13 Thread Stefano Stabellini 
On Mon, 12 Nov 2018, Mirela Simonovic wrote:
> Non-boot CPUs have to be disabled on suspend and enabled on resume
> (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> platform capabilities, this may lead to the physical powering down of
> CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> each non-boot CPU).
> 
> Signed-off-by: Mirela Simonovic 
> Signed-off-by: Saeed Nowshadi 
> ---
>  xen/arch/arm/suspend.c | 15 ++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> index 575afd5eb8..dae1b1f7d6 100644
> --- a/xen/arch/arm/suspend.c
> +++ b/xen/arch/arm/suspend.c
> @@ -1,4 +1,5 @@
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, register_t 
> cid)
>  /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
>  static long system_suspend(void *data)
>  {
> +int status;
> +
>  BUG_ON(system_state != SYS_STATE_active);
>  
>  system_state = SYS_STATE_suspend;
>  freeze_domains();
>  
> +status = disable_nonboot_cpus();
> +if ( status )
> +{
> +system_state = SYS_STATE_resume;
> +goto resume_nonboot_cpus;
> +}
> +
>  system_state = SYS_STATE_resume;
>  
> +resume_nonboot_cpus:
> +enable_nonboot_cpus();
>  thaw_domains();
>  system_state = SYS_STATE_active;
> +dsb(sy);
>  
> -return -ENOSYS;
> +return status;
>  }

I think we need a spin_lock to protect system_suspend from concurrent
calls, or (better) we need to make sure that the caller is only allowed
to call system_suspend if there is just one vcpu active in the system,
and that vcpu is blocked on this PSCI system suspend call.


>  int32_t domain_suspend(register_t epoint, register_t cid)
> -- 
> 2.13.0
> 

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume

2018-11-12 Thread Mirela Simonovic 
Non-boot CPUs have to be disabled on suspend and enabled on resume
(hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
CPU_OFF to be called by each non-boot CPU. Depending on the underlying
platform capabilities, this may lead to the physical powering down of
CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
each non-boot CPU).

Signed-off-by: Mirela Simonovic 
Signed-off-by: Saeed Nowshadi 
---
 xen/arch/arm/suspend.c | 15 ++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index 575afd5eb8..dae1b1f7d6 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -1,4 +1,5 @@
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint, register_t 
cid)
 /* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
 static long system_suspend(void *data)
 {
+int status;
+
 BUG_ON(system_state != SYS_STATE_active);
 
 system_state = SYS_STATE_suspend;
 freeze_domains();
 
+status = disable_nonboot_cpus();
+if ( status )
+{
+system_state = SYS_STATE_resume;
+goto resume_nonboot_cpus;
+}
+
 system_state = SYS_STATE_resume;
 
+resume_nonboot_cpus:
+enable_nonboot_cpus();
 thaw_domains();
 system_state = SYS_STATE_active;
+dsb(sy);
 
-return -ENOSYS;
+return status;
 }
 
 int32_t domain_suspend(register_t epoint, register_t cid)
-- 
2.13.0


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel