Re: [Xen-devel] [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Julien Grall]

On 23/04/18 18:12, Mirela Simonovic wrote:

On Mon, Apr 23, 2018 at 1:21 PM, Julien Grall  wrote:

On 20/04/18 13:25, Mirela Simonovic wrote:

+if ( errno )
+panic("PSCI cpu off failed for CPU%d err=%d\n",
get_processor_id(),
+  errno);
+}
+
   void call_psci_system_off(void)
   {
   if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b2116f0d2d..1ca3d63261 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -395,6 +395,9 @@ void stop_cpu(void)
   /* Make sure the write happens before we sleep forever */
   dsb(sy);
   isb();
+if ( system_state == SYS_STATE_suspend )



I don't think this should be call only on suspend/resume. System shutdown
could also benefit of PSCI CPU off. This is also paving the way to more use
case of turning off a CPU.


Ok, but then we do need to check for PSCI version here.


Or inside call_psci_off to match the other psci_* functions. However, 
you still need to check the PSCI version in the suspend code to deny any 
suspend/resume functionally on platform without PSCI CPU off support.


Cheers,
--
Julien Grall

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

Re: [Xen-devel] [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Mirela Simonovic]

Hi Julien,

On Mon, Apr 23, 2018 at 1:21 PM, Julien Grall  wrote:
> Hi Mirela,
>
> On 20/04/18 13:25, Mirela Simonovic wrote:
>>
>> During the system suspend to RAM non-boot CPUs will be hotplugged.
>> This will be triggered via disable_nonboot_cpus() call. When
>> hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
>> This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
>> down the calling CPU during the suspend.
>> If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
>> when the PSCI CPU_OFF call returns we'll raise panic, because the calling
>> CPU could be enabled afterwards.
>> If a CPU executes stop_cpu() when the system is not suspending the
>> calling CPU will loop in the infinite while/wfi, as it was looping
>> before this change.
>> Note that there is no check for PSCI version in PSCI CPU_OFF
>> implementation because the version will be checked prior to triggering
>> the system suspend.
>
>
> Then the code should contain an ASSERT + comment to make it clear on the
> interface. But I don't think this is the right way to go (see below).
>
>
>>
>> Signed-off-by: Mirela Simonovic 
>>
>> ---
>> CC: Stefano Stabellini 
>> CC: Julien Grall 
>> ---
>> Changes in v2:
>> -Issue PSCI CPU_OFF only if the system is suspending
>> -If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
>> -Fixed commit message
>> ---
>>   xen/arch/arm/psci.c| 11 +++
>>   xen/arch/arm/smpboot.c |  3 +++
>>   xen/include/asm-arm/psci.h |  1 +
>>   3 files changed, 15 insertions(+)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 94b616df9b..7f7b0695a3 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
>>   return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu),
>> __pa(init_secondary), 0);
>>   }
>>   +void call_psci_cpu_off(void)
>> +{
>> +int errno;
>> +
>> +/* If successfull the PSCI cpu_off call doesn't return */
>> +errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
>
>
> CPU_OFF will only return on error. So the if is not necessary below.
>

Correct

>> +if ( errno )
>> +panic("PSCI cpu off failed for CPU%d err=%d\n",
>> get_processor_id(),
>> +  errno);
>> +}
>> +
>>   void call_psci_system_off(void)
>>   {
>>   if ( psci_ver > PSCI_VERSION(0, 1) )
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index b2116f0d2d..1ca3d63261 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -395,6 +395,9 @@ void stop_cpu(void)
>>   /* Make sure the write happens before we sleep forever */
>>   dsb(sy);
>>   isb();
>> +if ( system_state == SYS_STATE_suspend )
>
>
> I don't think this should be call only on suspend/resume. System shutdown
> could also benefit of PSCI CPU off. This is also paving the way to more use
> case of turning off a CPU.

Ok, but then we do need to check for PSCI version here.

Thanks,
Mirela

>
> Cheers,
>
> --
> Julien Grall

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

Re: [Xen-devel] [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Julien Grall]

Hi Mirela,

On 20/04/18 13:25, Mirela Simonovic wrote:

During the system suspend to RAM non-boot CPUs will be hotplugged.
This will be triggered via disable_nonboot_cpus() call. When
hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
down the calling CPU during the suspend.
If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
when the PSCI CPU_OFF call returns we'll raise panic, because the calling
CPU could be enabled afterwards.
If a CPU executes stop_cpu() when the system is not suspending the
calling CPU will loop in the infinite while/wfi, as it was looping
before this change.
Note that there is no check for PSCI version in PSCI CPU_OFF
implementation because the version will be checked prior to triggering
the system suspend.


Then the code should contain an ASSERT + comment to make it clear on the 
interface. But I don't think this is the right way to go (see below).




Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Issue PSCI CPU_OFF only if the system is suspending
-If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
-Fixed commit message
---
  xen/arch/arm/psci.c| 11 +++
  xen/arch/arm/smpboot.c |  3 +++
  xen/include/asm-arm/psci.h |  1 +
  3 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 94b616df9b..7f7b0695a3 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
  return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), 
__pa(init_secondary), 0);
  }
  
+void call_psci_cpu_off(void)

+{
+int errno;
+
+/* If successfull the PSCI cpu_off call doesn't return */
+errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);


CPU_OFF will only return on error. So the if is not necessary below.


+if ( errno )
+panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
+  errno);
+}
+
  void call_psci_system_off(void)
  {
  if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b2116f0d2d..1ca3d63261 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -395,6 +395,9 @@ void stop_cpu(void)
  /* Make sure the write happens before we sleep forever */
  dsb(sy);
  isb();
+if ( system_state == SYS_STATE_suspend )


I don't think this should be call only on suspend/resume. System 
shutdown could also benefit of PSCI CPU off. This is also paving the way 
to more use case of turning off a CPU.


Cheers,

--
Julien Grall

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

[Xen-devel] [PATCH v2 03/10] xen/arm: Implement CPU_OFF PSCI call (physical interface)

[Mirela Simonovic]

During the system suspend to RAM non-boot CPUs will be hotplugged.
This will be triggered via disable_nonboot_cpus() call. When
hotplugged the CPU will end up in an infinite wfi loop in stop_cpu().
This patch adds PSCI CPU_OFF call to the EL3 with the aim to get powered
down the calling CPU during the suspend.
If PSCI CPU_OFF call to the EL3 succeeds it will not return. Otherwise,
when the PSCI CPU_OFF call returns we'll raise panic, because the calling
CPU could be enabled afterwards.
If a CPU executes stop_cpu() when the system is not suspending the
calling CPU will loop in the infinite while/wfi, as it was looping
before this change.
Note that there is no check for PSCI version in PSCI CPU_OFF
implementation because the version will be checked prior to triggering
the system suspend.

Signed-off-by: Mirela Simonovic 

---
CC: Stefano Stabellini 
CC: Julien Grall 
---
Changes in v2:
-Issue PSCI CPU_OFF only if the system is suspending
-If PSCI CPU_OFF call fails (unlikely to ever happen) raise panic
-Fixed commit message
---
 xen/arch/arm/psci.c| 11 +++
 xen/arch/arm/smpboot.c |  3 +++
 xen/include/asm-arm/psci.h |  1 +
 3 files changed, 15 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 94b616df9b..7f7b0695a3 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -46,6 +46,17 @@ int call_psci_cpu_on(int cpu)
 return call_smc(psci_cpu_on_nr, cpu_logical_map(cpu), 
__pa(init_secondary), 0);
 }
 
+void call_psci_cpu_off(void)
+{
+int errno;
+
+/* If successfull the PSCI cpu_off call doesn't return */
+errno = call_smc(PSCI_0_2_FN32_CPU_OFF, 0, 0, 0);
+if ( errno )
+panic("PSCI cpu off failed for CPU%d err=%d\n", get_processor_id(),
+  errno);
+}
+
 void call_psci_system_off(void)
 {
 if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index b2116f0d2d..1ca3d63261 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -395,6 +395,9 @@ void stop_cpu(void)
 /* Make sure the write happens before we sleep forever */
 dsb(sy);
 isb();
+if ( system_state == SYS_STATE_suspend )
+call_psci_cpu_off();
+
 while ( 1 )
 wfi();
 }
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 9ac820e94a..832f77afff 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -20,6 +20,7 @@ extern uint32_t psci_ver;
 
 int psci_init(void);
 int call_psci_cpu_on(int cpu);
+void call_psci_cpu_off(void);
 void call_psci_system_off(void);
 void call_psci_system_reset(void);
 
-- 
2.13.0


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