[PATCH 0/2] xen/arm: Add support for PSCI v1.0

[PATCH 0/2] xen/arm: Add support for PSCI v1.0

[Julien Grall]

Hi all,

This small patch series allows Xen boot on platform where the firmware is
supporting only PSCI 1.0.

Regards,

Julien Grall (2):
  xen/arm: Add support of PSCI v1.0 for the host
  xen/arm: Replace XEN_PSCI_* by PSCI_VERSION(major, minor)

 xen/arch/arm/psci.c        | 15 ++++++++-------
 xen/arch/arm/vpsci.c       | 15 ++++++++-------
 xen/include/asm-arm/psci.h | 17 +++++++++++++----
 3 files changed, 29 insertions(+), 18 deletions(-)

-- 
2.1.4

[PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Julien Grall]

>From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
SYSTEM_RESET) behaves exactly the same.

While there is no compatible string to represent PSCI v1.0 in the DT,
it's possible to detect it using the function PSCI_VERSION.

The compatible string is now used to detect if the platform may support
PSCI v0.2 or higher.

Signed-off-by: Julien Grall <julien.grall@citrix.com>

---

Cc: Andre Przywara <andre.przywara@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
---
 xen/arch/arm/psci.c        |  9 +++++----
 xen/include/asm-arm/psci.h | 13 +++++++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 172c6e7..53ee2e4 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
 
     psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
 
-    if ( psci_ver != XEN_PSCI_V_0_2 )
+    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )
     {
-        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
-        return -EOPNOTSUPP;
+        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);
+        return -EINVAL;
     }
 
     psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
 
-    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
+    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
+           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
 
     return 0;
 }
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 5d17ee3..d8a109f 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -87,6 +87,19 @@ void do_psci_0_2_system_reset(void);
 #define PSCI_0_2_POWER_STATE_TYPE_MASK      \
                     (0x1 << PSCI_0_2_POWER_STATE_TYPE_SHIFT)
 
+/* PSCI version decoding (independent of PSCI version) */
+#define PSCI_VERSION_MAJOR_SHIFT            16
+#define PSCI_VERSION_MINOR_MASK             \
+        ((1U << PSCI_VERSION_MAJOR_SHIFT) - 1)
+#define PSCI_VERSION_MAJOR_MASK             ~PSCI_VERSION_MINOR_MASK
+#define PSCI_VERSION_MAJOR(ver)             \
+        (((ver) & PSCI_VERSION_MAJOR_MASK) >> PSCI_VERSION_MAJOR_SHIFT)
+#define PSCI_VERSION_MINOR(ver)             \
+        ((ver) & PSCI_VERSION_MINOR_MASK)
+
+#define PSCI_VERSION(major, minor)          \
+    (((major) << PSCI_VERSION_MAJOR_SHIFT) | (minor))
+
 #endif /* __ASM_PSCI_H__ */
 
 /*
-- 
2.1.4

Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Ian Campbell]

On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
> From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
> the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
> SYSTEM_RESET) behaves exactly the same.
> 
> While there is no compatible string to represent PSCI v1.0 in the DT,
> it's possible to detect it using the function PSCI_VERSION.
> 
> The compatible string is now used to detect if the platform may support
> PSCI v0.2 or higher.

The actual implementation here looks for precisely 0.2 or 1.0, not >= 0.2
as suggested by this statement.

The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION) that
for any 1.y version must be compatible with 1.x when y>x (for those
functions which existed in 1.x, y might have more).

IOW an OS supporting 1.0 should work with any 1.x.

(which begs the question why there is not a "arm,psci-1.x" compat string,
Mark/Andre?)

> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>
> 
> ---
> 
> Cc: Andre Przywara <andre.przywara@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> ---
>  xen/arch/arm/psci.c        |  9 +++++----
>  xen/include/asm-arm/psci.h | 13 +++++++++++++
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 172c6e7..53ee2e4 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
>  
>      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>  
> -    if ( psci_ver != XEN_PSCI_V_0_2 )
> +    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )

Based on the above I think this should read:

    if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_MAJOR_VERSION(psci_ver) != 1 )

>      {
> -        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
> -        return -EOPNOTSUPP;
> +        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);

Conflicting with what?

I think perhaps you meant "Unrecognised" or "Unsupported"?

Also please format the version like you did below with %u.%u.

>      }
>  
>      psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>  
> -    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
> +    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
> +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
>  
>      return 0;
>  }

Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Mark Rutland]

On Fri, Oct 09, 2015 at 04:08:17PM +0100, Ian Campbell wrote:
> On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
> > From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
> > the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
> > SYSTEM_RESET) behaves exactly the same.
> > 
> > While there is no compatible string to represent PSCI v1.0 in the DT,
> > it's possible to detect it using the function PSCI_VERSION.
> > 
> > The compatible string is now used to detect if the platform may support
> > PSCI v0.2 or higher.
> 
> The actual implementation here looks for precisely 0.2 or 1.0, not >= 0.2
> as suggested by this statement.
> 
> The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION) that
> for any 1.y version must be compatible with 1.x when y>x (for those
> functions which existed in 1.x, y might have more).
> 
> IOW an OS supporting 1.0 should work with any 1.x.

This _should_ be the case. Linux assumes future versions are compatible
in this manner.

There was some minor incompatibility going from 0.2 to 1.0 (the new
state parameter format) that could trip up existing PSCI 0.2 aware OSs,
but hopefully we won't have that kind of thing happen again.

> (which begs the question why there is not a "arm,psci-1.x" compat string,
> Mark/Andre?)

There will be one, in fact (see Lorenzo's pull request [1]), but it's
only necessary to allow us to describe PSCI 1.0 implementations which
are not strictly PSCI 0.2 compatible. Otherwise it's harmless to add for
PSCI 1.0 compatible systems, but "arm,psci-0.2" should be sufficient.

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-October/374547.html

> 
> > 
> > Signed-off-by: Julien Grall <julien.grall@citrix.com>
> > 
> > ---
> > 
> > Cc: Andre Przywara <andre.przywara@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > ---
> >  xen/arch/arm/psci.c        |  9 +++++----
> >  xen/include/asm-arm/psci.h | 13 +++++++++++++
> >  2 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> > index 172c6e7..53ee2e4 100644
> > --- a/xen/arch/arm/psci.c
> > +++ b/xen/arch/arm/psci.c
> > @@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
> >  
> >      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> >  
> > -    if ( psci_ver != XEN_PSCI_V_0_2 )
> > +    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )
> 
> Based on the above I think this should read:
> 
>     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_MAJOR_VERSION(psci_ver) != 1 )
> 
> >      {
> > -        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
> > -        return -EOPNOTSUPP;
> > +        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);
> 
> Conflicting with what?
> 
> I think perhaps you meant "Unrecognised" or "Unsupported"?
> 
> Also please format the version like you did below with %u.%u.
> 
> >      }
> >  
> >      psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
> >  
> > -    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
> > +    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
> > +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
> >  
> >      return 0;
> >  }
> 

Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Ian Campbell]

On Fri, 2015-10-09 at 16:17 +0100, Mark Rutland wrote:
> On Fri, Oct 09, 2015 at 04:08:17PM +0100, Ian Campbell wrote:
> > On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
> > > From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
> > > the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
> > > SYSTEM_RESET) behaves exactly the same.
> > > 
> > > While there is no compatible string to represent PSCI v1.0 in the DT,
> > > it's possible to detect it using the function PSCI_VERSION.
> > > 
> > > The compatible string is now used to detect if the platform may
> > > support
> > > PSCI v0.2 or higher.
> > 
> > The actual implementation here looks for precisely 0.2 or 1.0, not >=
> > 0.2
> > as suggested by this statement.
> > 
> > The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION)
> > that
> > for any 1.y version must be compatible with 1.x when y>x (for those
> > functions which existed in 1.x, y might have more).
> > 
> > IOW an OS supporting 1.0 should work with any 1.x.
> 
> This _should_ be the case. Linux assumes future versions are compatible
> in this manner.

OK, I think we can follow suite then.

> There was some minor incompatibility going from 0.2 to 1.0 (the new
> state parameter format) that could trip up existing PSCI 0.2 aware OSs,
> but hopefully we won't have that kind of thing happen again.

Well, the spec has promised not this time around, so I should hope so too ;
-) (or it would have to be 2.x)

> 
> > (which begs the question why there is not a "arm,psci-1.x" compat
> > string,
> > Mark/Andre?)
> 
> There will be one, in fact (see Lorenzo's pull request [1]), but it's
> only necessary to allow us to describe PSCI 1.0 implementations which
> are not strictly PSCI 0.2 compatible. Otherwise it's harmless to add for
> PSCI 1.0 compatible systems, but "arm,psci-0.2" should be sufficient.

Are "PSCI 1.0 implementations which are not strictly PSCI 0.2 compatible"
valid 1.0 implementations? I think so.

In that case it is not correct to say that in addition to all 1.x being
compatible 0.2 can also be treated as being in the 1.x "compatibility
bucket", without due care.

Anyway, I don't think these corner cases affect Xen.

Julien, will you add the new compat string in your next version?

Ian.

Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Julien Grall]

On 09/10/15 16:30, Ian Campbell wrote:
> Julien, will you add the new compat string in your next version?

I will do.

Regards,

-- 
Julien Grall

Re: [PATCH 1/2] xen/arm: Add support of PSCI v1.0 for the host

[Julien Grall]

Hi Ian,

On 09/10/15 16:08, Ian Campbell wrote:
> On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
>> From Xen point of view, PSCI v0.2 and PSCI v1.0 are very similar. All
>> the PSCI calls used within Xen (PSCI_VERSION, CPU_ON, SYSTEM_OFF and
>> SYSTEM_RESET) behaves exactly the same.
>>
>> While there is no compatible string to represent PSCI v1.0 in the DT,
>> it's possible to detect it using the function PSCI_VERSION.
>>
>> The compatible string is now used to detect if the platform may support
>> PSCI v0.2 or higher.
> 
> The actual implementation here looks for precisely 0.2 or 1.0, not >= 0.2
> as suggested by this statement.

The first implementation I did was based on the Linux one which is
working checking if the PSCI version if >= 0.2.

Although I changed my mind before sending the patch because I was worry
to see Xen breaking badly when booting on another version of PSCI.

> 
> The PSCI 1.0 spec says (section 5.3.1, intended use of PSCI_VERSION) that
> for any 1.y version must be compatible with 1.x when y>x (for those
> functions which existed in 1.x, y might have more).
> IOW an OS supporting 1.0 should work with any 1.x.

Right, I will update the check.

> (which begs the question why there is not a "arm,psci-1.x" compat string,
> Mark/Andre?)
> 
>>
>> Signed-off-by: Julien Grall <julien.grall@citrix.com>
>>
>> ---
>>
>> Cc: Andre Przywara <andre.przywara@arm.com>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> ---
>>  xen/arch/arm/psci.c        |  9 +++++----
>>  xen/include/asm-arm/psci.h | 13 +++++++++++++
>>  2 files changed, 18 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 172c6e7..53ee2e4 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -122,15 +122,16 @@ int __init psci_init_0_2(void)
>>  
>>      psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>>  
>> -    if ( psci_ver != XEN_PSCI_V_0_2 )
>> +    if ( psci_ver != PSCI_VERSION(0, 2) && psci_ver != PSCI_VERSION(1, 0) )
> 
> Based on the above I think this should read:
> 
>     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_MAJOR_VERSION(psci_ver) != 1 )
> 
>>      {
>> -        printk("Error: PSCI version %#x is not supported.\n", psci_ver);
>> -        return -EOPNOTSUPP;
>> +        printk("Error: Conflicting PSCI version detected (%#x)\n", psci_ver);
> 
> Conflicting with what?
> I think perhaps you meant "Unrecognised" or "Unsupported"?

It's just a mistake. When I first wrote the patch the check was:
PSCI_MAJOR_VERSION(psci_vers) == 0 && PSCI_MINOR_VERSION(psci_vers) < 2

Although, I was worry about allowing to many version of PSCI. I will use
your suggestion.

> Also please format the version like you did below with %u.%u.

I will do.

> 
>>      }
>>  
>>      psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>>  
>> -    printk(XENLOG_INFO "Using PSCI-0.2 for SMP bringup\n");
>> +    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
>> +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
>>  
>>      return 0;
>>  }
> 

Regards,

-- 
Julien Grall

[PATCH 2/2] xen/arm: Replace XEN_PSCI_* by PSCI_VERSION(major, minor)

[Julien Grall]

It will avoid to introduce a new XEN_PSCI_* define every time we support
a new version of PSCI in Xen.

Also fix the coding style in modified place.

Signed-off-by: Julien Grall <julien.grall@citrix.com>
---
 xen/arch/arm/psci.c        |  6 +++---
 xen/arch/arm/vpsci.c       | 15 ++++++++-------
 xen/include/asm-arm/psci.h |  4 ----
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 53ee2e4..ae3df47 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -46,13 +46,13 @@ int call_psci_cpu_on(int cpu)
 
 void call_psci_system_off(void)
 {
-    if ( psci_ver > XEN_PSCI_V_0_1 )
+    if ( psci_ver > PSCI_VERSION(0, 1) )
         call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
 }
 
 void call_psci_system_reset(void)
 {
-    if ( psci_ver > XEN_PSCI_V_0_1 )
+    if ( psci_ver > PSCI_VERSION(0, 1) )
         call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
 }
 
@@ -100,7 +100,7 @@ int __init psci_init_0_1(void)
         return -ENOENT;
     }
 
-    psci_ver = XEN_PSCI_V_0_1;
+    psci_ver = PSCI_VERSION(0, 1);
 
     printk(XENLOG_INFO "Using PSCI-0.1 for SMP bringup\n");
 
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index aebe1e2..0e024f7 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -41,8 +41,8 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     if ( is_64bit_domain(d) && is_thumb )
         return PSCI_INVALID_PARAMETERS;
 
-    if( ( ver == XEN_PSCI_V_0_2 ) &&
-            ( !test_bit(_VPF_down, &v->pause_flags) ) )
+    if ( (ver == PSCI_VERSION(0, 2)) &&
+            !test_bit(_VPF_down, &v->pause_flags) )
         return PSCI_ALREADY_ON;
 
     if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
@@ -59,14 +59,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     if ( is_32bit_domain(d) )
     {
         ctxt->user_regs.cpsr = PSR_GUEST32_INIT;
-        if( ver == XEN_PSCI_V_0_2 )
+        if ( ver == PSCI_VERSION(0, 2) )
             ctxt->user_regs.r0_usr = context_id;
     }
 #ifdef CONFIG_ARM_64
     else
     {
         ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
-        if( ver == XEN_PSCI_V_0_2 )
+        if ( ver == PSCI_VERSION(0, 2) )
             ctxt->user_regs.x0 = context_id;
     }
 #endif
@@ -94,7 +94,7 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
 
 int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
 {
-    return do_common_cpu_on(vcpuid,entry_point,0,XEN_PSCI_V_0_1);
+    return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1));
 }
 
 int32_t do_psci_cpu_off(uint32_t power_state)
@@ -107,7 +107,7 @@ int32_t do_psci_cpu_off(uint32_t power_state)
 
 uint32_t do_psci_0_2_version(void)
 {
-    return XEN_PSCI_V_0_2;
+    return PSCI_VERSION(0, 2);
 }
 
 register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
@@ -132,7 +132,8 @@ int32_t do_psci_0_2_cpu_off(void)
 int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
                        register_t context_id)
 {
-    return do_common_cpu_on(target_cpu,entry_point,context_id,XEN_PSCI_V_0_2);
+    return do_common_cpu_on(target_cpu, entry_point, context_id,
+                            PSCI_VERSION(0, 2));
 }
 
 static const unsigned long target_affinity_mask[] = {
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index d8a109f..be2458a 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -41,10 +41,6 @@ register_t do_psci_0_2_migrate_info_up_cpu(void);
 void do_psci_0_2_system_off(void);
 void do_psci_0_2_system_reset(void);
 
-/* PSCI version */
-#define XEN_PSCI_V_0_1 1
-#define XEN_PSCI_V_0_2 2
-
 /* PSCI v0.2 interface */
 #define PSCI_0_2_FN_BASE        0x84000000
 #define PSCI_0_2_FN(n)          (PSCI_0_2_FN_BASE + (n))
-- 
2.1.4

Re: [PATCH 2/2] xen/arm: Replace XEN_PSCI_* by PSCI_VERSION(major, minor)

[Ian Campbell]

On Thu, 2015-10-08 at 19:45 +0100, Julien Grall wrote:
> It will avoid to introduce a new XEN_PSCI_* define every time we support
> a new version of PSCI in Xen.
> 
> Also fix the coding style in modified place.
> 
> Signed-off-by: Julien Grall <julien.grall@citrix.com>

Acked-by: Ian Campbell <ian.campbell@citrix.com>