[Xen-devel,3/7] xen/arm: vpsci: Add support for PSCI 1.1

Message ID 20180205132011.27996-4-julien.grall@arm.com
State Superseded
Headers show
Series
  • xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update
Related show

Commit Message

Julien Grall Feb. 5, 2018, 1:20 p.m.
At the moment, Xen provides virtual PSCI interface compliant with 0.1
and 0.2. Since them, the specification has been updated and the latest
version is 1.1 (see ARM DEN 0022D).

From an implementation point of view, only PSCI_FEATURES is mandatory.
The rest is optional and can be left unimplemented for now.

At the same time, the compatible for PSCI node have been updated to
expose "arm,psci-1.0".

Signed-off-by: Julien Grall <julien.grall@arm.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: mirela.simonovic@aggios.com

---
    We may want to provide a way for the toolstack to specify a PSCI
    version. This could be useful if a guest is expecting a given
    version.
---
 tools/libxl/libxl_arm.c          |  3 ++-
 xen/arch/arm/domain_build.c      |  1 +
 xen/arch/arm/vpsci.c             | 34 +++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/perfc_defn.h |  1 +
 xen/include/asm-arm/psci.h       |  1 +
 xen/include/asm-arm/vpsci.h      |  2 +-
 6 files changed, 39 insertions(+), 3 deletions(-)

Comments

Volodymyr Babchuk Feb. 6, 2018, 4:07 p.m. | #1
Hi,

On 5 February 2018 at 15:20, Julien Grall <julien.grall@arm.com> wrote:
> At the moment, Xen provides virtual PSCI interface compliant with 0.1
> and 0.2. Since them, the specification has been updated and the latest
> version is 1.1 (see ARM DEN 0022D).
>
> From an implementation point of view, only PSCI_FEATURES is mandatory.
> The rest is optional and can be left unimplemented for now.
>
> At the same time, the compatible for PSCI node have been updated to
> expose "arm,psci-1.0".
>
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: mirela.simonovic@aggios.com
>
> ---
>     We may want to provide a way for the toolstack to specify a PSCI
>     version. This could be useful if a guest is expecting a given
>     version.
> ---
>  tools/libxl/libxl_arm.c          |  3 ++-
>  xen/arch/arm/domain_build.c      |  1 +
>  xen/arch/arm/vpsci.c             | 34 +++++++++++++++++++++++++++++++++-
>  xen/include/asm-arm/perfc_defn.h |  1 +
>  xen/include/asm-arm/psci.h       |  1 +
>  xen/include/asm-arm/vpsci.h      |  2 +-
>  6 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
> index 3e46554301..86f59c0d80 100644
> --- a/tools/libxl/libxl_arm.c
> +++ b/tools/libxl/libxl_arm.c
> @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>      res = fdt_begin_node(fdt, "psci");
>      if (res) return res;
>
> -    res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci");
> +    res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0",
> +                              "arm,psci-0.2", "arm,psci");
>      if (res) return res;
>
>      res = fdt_property_string(fdt, "method", "hvc");
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index 155c952349..941688a2ce 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -637,6 +637,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>  {
>      int res;
>      const char compat[] =
> +        "arm,psci-1.0""\0"
>          "arm,psci-0.2""\0"
>          "arm,psci";
>
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 17dab42cf4..025250a119 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -115,7 +115,7 @@ static int32_t do_psci_cpu_off(uint32_t power_state)
>
>  static uint32_t do_psci_0_2_version(void)
>  {
> -    return PSCI_VERSION(0, 2);
> +    return PSCI_VERSION(1, 0);
>  }
I'm confused a bit with the versions. In the commit message you
mentioned version 1.1. But there you return version 1,0 from a
function named do_psci_0_2_version().

>
>  static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
> @@ -199,6 +199,28 @@ static void do_psci_0_2_system_reset(void)
>      domain_shutdown(d,SHUTDOWN_reboot);
>  }
>
> +static int32_t do_psci_1_0_features(uint32_t psci_func_id)
> +{
> +    switch ( psci_func_id )
> +    {
> +    case PSCI_0_2_FN32_PSCI_VERSION:
> +    case PSCI_0_2_FN32_CPU_OFF:
> +    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
> +    case PSCI_0_2_FN32_SYSTEM_OFF:
> +    case PSCI_0_2_FN32_SYSTEM_RESET:
> +    case PSCI_0_2_FN32_CPU_ON:
> +    case PSCI_0_2_FN64_CPU_ON:
> +    case PSCI_0_2_FN32_CPU_SUSPEND:
> +    case PSCI_0_2_FN64_CPU_SUSPEND:
> +    case PSCI_0_2_FN32_AFFINITY_INFO:
> +    case PSCI_0_2_FN64_AFFINITY_INFO:
> +    case PSCI_1_0_FN32_PSCI_FEATURES:
Are those functions sorted in a some order?

> +        return 0;
> +    default:
> +        return PSCI_NOT_SUPPORTED;
> +    }
> +}
> +
>  #define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>  #define PSCI_ARG(reg, n) get_user_reg(reg, n)
>
> @@ -311,6 +333,16 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
>          PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>          return true;
>      }
> +
> +    case PSCI_1_0_FN32_PSCI_FEATURES:
> +    {
> +        uint32_t psci_func_id = PSCI_ARG32(regs, 1);
> +
> +        perfc_incr(vpsci_features);
> +        PSCI_SET_RESULT(regs, do_psci_1_0_features(psci_func_id));
> +        return true;
> +    }
> +
>      default:
>          return false;
>      }
> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> index a7acb7d21c..87866264ca 100644
> --- a/xen/include/asm-arm/perfc_defn.h
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -31,6 +31,7 @@ PERFCOUNTER(vpsci_system_off,          "vpsci: system_off")
>  PERFCOUNTER(vpsci_system_reset,        "vpsci: system_reset")
>  PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
>  PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
> +PERFCOUNTER(vpsci_features,            "vpsci: features")
>
>  PERFCOUNTER(vgicd_reads,                "vgicd: read")
>  PERFCOUNTER(vgicd_writes,               "vgicd: write")
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index becc9f9ded..e2629eed01 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -40,6 +40,7 @@ void call_psci_system_reset(void);
>  #define PSCI_0_2_FN32_MIGRATE_INFO_TYPE   PSCI_0_2_FN32(6)
>  #define PSCI_0_2_FN32_SYSTEM_OFF          PSCI_0_2_FN32(8)
>  #define PSCI_0_2_FN32_SYSTEM_RESET        PSCI_0_2_FN32(9)
> +#define PSCI_1_0_FN32_PSCI_FEATURES       PSCI_0_2_FN32(10)
>
>  #define PSCI_0_2_FN64_CPU_SUSPEND         PSCI_0_2_FN64(1)
>  #define PSCI_0_2_FN64_CPU_ON              PSCI_0_2_FN64(3)
> diff --git a/xen/include/asm-arm/vpsci.h b/xen/include/asm-arm/vpsci.h
> index d6a890f6a2..6d98c3651c 100644
> --- a/xen/include/asm-arm/vpsci.h
> +++ b/xen/include/asm-arm/vpsci.h
> @@ -4,7 +4,7 @@
>  #include <asm/psci.h>
>
>  /* Number of function implemented by virtual PSCI (only 0.2 or later) */
> -#define VPSCI_NR_FUNCS  11
> +#define VPSCI_NR_FUNCS  12
>
>  /* Functions handle PSCI calls from the guests */
>  bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
> --
> 2.11.0
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel
Julien Grall Feb. 6, 2018, 5:44 p.m. | #2
On 02/06/2018 04:07 PM, Volodymyr Babchuk wrote:
> Hi,

Hi Volodymyr,

Thank you for the review.

> On 5 February 2018 at 15:20, Julien Grall <julien.grall@arm.com> wrote:
>> At the moment, Xen provides virtual PSCI interface compliant with 0.1
>> and 0.2. Since them, the specification has been updated and the latest
>> version is 1.1 (see ARM DEN 0022D).
>>
>>  From an implementation point of view, only PSCI_FEATURES is mandatory.
>> The rest is optional and can be left unimplemented for now.
>>
>> At the same time, the compatible for PSCI node have been updated to
>> expose "arm,psci-1.0".
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
>> Cc: mirela.simonovic@aggios.com
>>
>> ---
>>      We may want to provide a way for the toolstack to specify a PSCI
>>      version. This could be useful if a guest is expecting a given
>>      version.
>> ---
>>   tools/libxl/libxl_arm.c          |  3 ++-
>>   xen/arch/arm/domain_build.c      |  1 +
>>   xen/arch/arm/vpsci.c             | 34 +++++++++++++++++++++++++++++++++-
>>   xen/include/asm-arm/perfc_defn.h |  1 +
>>   xen/include/asm-arm/psci.h       |  1 +
>>   xen/include/asm-arm/vpsci.h      |  2 +-
>>   6 files changed, 39 insertions(+), 3 deletions(-)
>>
>> diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
>> index 3e46554301..86f59c0d80 100644
>> --- a/tools/libxl/libxl_arm.c
>> +++ b/tools/libxl/libxl_arm.c
>> @@ -410,7 +410,8 @@ static int make_psci_node(libxl__gc *gc, void *fdt)
>>       res = fdt_begin_node(fdt, "psci");
>>       if (res) return res;
>>
>> -    res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci");
>> +    res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0",
>> +                              "arm,psci-0.2", "arm,psci");
>>       if (res) return res;
>>
>>       res = fdt_property_string(fdt, "method", "hvc");
>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>> index 155c952349..941688a2ce 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -637,6 +637,7 @@ static int make_psci_node(void *fdt, const struct dt_device_node *parent)
>>   {
>>       int res;
>>       const char compat[] =
>> +        "arm,psci-1.0""\0"
>>           "arm,psci-0.2""\0"
>>           "arm,psci";
>>
>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
>> index 17dab42cf4..025250a119 100644
>> --- a/xen/arch/arm/vpsci.c
>> +++ b/xen/arch/arm/vpsci.c
>> @@ -115,7 +115,7 @@ static int32_t do_psci_cpu_off(uint32_t power_state)
>>
>>   static uint32_t do_psci_0_2_version(void)
>>   {
>> -    return PSCI_VERSION(0, 2);
>> +    return PSCI_VERSION(1, 0);
>>   }
> I'm confused a bit with the versions. In the commit message you
> mentioned version 1.1. But there you return version 1,0 from a
> function named do_psci_0_2_version().

So the function names are implemented based on when they were added in 
the specification. This makes slightly easier if we ever decide to 
expose 0.2 (or 1.0/1.1) only PSCI in the guest.

Also, good catch for the returned version. I first implemented the patch 
for 1.0 and forgot to update it.

> 
>>
>>   static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
>> @@ -199,6 +199,28 @@ static void do_psci_0_2_system_reset(void)
>>       domain_shutdown(d,SHUTDOWN_reboot);
>>   }
>>
>> +static int32_t do_psci_1_0_features(uint32_t psci_func_id)
>> +{
>> +    switch ( psci_func_id )
>> +    {
>> +    case PSCI_0_2_FN32_PSCI_VERSION:
>> +    case PSCI_0_2_FN32_CPU_OFF:
>> +    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
>> +    case PSCI_0_2_FN32_SYSTEM_OFF:
>> +    case PSCI_0_2_FN32_SYSTEM_RESET:
>> +    case PSCI_0_2_FN32_CPU_ON:
>> +    case PSCI_0_2_FN64_CPU_ON:
>> +    case PSCI_0_2_FN32_CPU_SUSPEND:
>> +    case PSCI_0_2_FN64_CPU_SUSPEND:
>> +    case PSCI_0_2_FN32_AFFINITY_INFO:
>> +    case PSCI_0_2_FN64_AFFINITY_INFO:
>> +    case PSCI_1_0_FN32_PSCI_FEATURES:
> Are those functions sorted in a some order?

They were meant to be sorted by the function ID. Thought it looks like 
it is not the case, I will fix that in the next version.

Cheers,

Patch

diff --git a/tools/libxl/libxl_arm.c b/tools/libxl/libxl_arm.c
index 3e46554301..86f59c0d80 100644
--- a/tools/libxl/libxl_arm.c
+++ b/tools/libxl/libxl_arm.c
@@ -410,7 +410,8 @@  static int make_psci_node(libxl__gc *gc, void *fdt)
     res = fdt_begin_node(fdt, "psci");
     if (res) return res;
 
-    res = fdt_property_compat(gc, fdt, 2, "arm,psci-0.2","arm,psci");
+    res = fdt_property_compat(gc, fdt, 3, "arm,psci-1.0",
+                              "arm,psci-0.2", "arm,psci");
     if (res) return res;
 
     res = fdt_property_string(fdt, "method", "hvc");
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 155c952349..941688a2ce 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -637,6 +637,7 @@  static int make_psci_node(void *fdt, const struct dt_device_node *parent)
 {
     int res;
     const char compat[] =
+        "arm,psci-1.0""\0"
         "arm,psci-0.2""\0"
         "arm,psci";
 
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 17dab42cf4..025250a119 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -115,7 +115,7 @@  static int32_t do_psci_cpu_off(uint32_t power_state)
 
 static uint32_t do_psci_0_2_version(void)
 {
-    return PSCI_VERSION(0, 2);
+    return PSCI_VERSION(1, 0);
 }
 
 static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
@@ -199,6 +199,28 @@  static void do_psci_0_2_system_reset(void)
     domain_shutdown(d,SHUTDOWN_reboot);
 }
 
+static int32_t do_psci_1_0_features(uint32_t psci_func_id)
+{
+    switch ( psci_func_id )
+    {
+    case PSCI_0_2_FN32_PSCI_VERSION:
+    case PSCI_0_2_FN32_CPU_OFF:
+    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
+    case PSCI_0_2_FN32_SYSTEM_OFF:
+    case PSCI_0_2_FN32_SYSTEM_RESET:
+    case PSCI_0_2_FN32_CPU_ON:
+    case PSCI_0_2_FN64_CPU_ON:
+    case PSCI_0_2_FN32_CPU_SUSPEND:
+    case PSCI_0_2_FN64_CPU_SUSPEND:
+    case PSCI_0_2_FN32_AFFINITY_INFO:
+    case PSCI_0_2_FN64_AFFINITY_INFO:
+    case PSCI_1_0_FN32_PSCI_FEATURES:
+        return 0;
+    default:
+        return PSCI_NOT_SUPPORTED;
+    }
+}
+
 #define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
 #define PSCI_ARG(reg, n) get_user_reg(reg, n)
 
@@ -311,6 +333,16 @@  bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
         PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         return true;
     }
+
+    case PSCI_1_0_FN32_PSCI_FEATURES:
+    {
+        uint32_t psci_func_id = PSCI_ARG32(regs, 1);
+
+        perfc_incr(vpsci_features);
+        PSCI_SET_RESULT(regs, do_psci_1_0_features(psci_func_id));
+        return true;
+    }
+
     default:
         return false;
     }
diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
index a7acb7d21c..87866264ca 100644
--- a/xen/include/asm-arm/perfc_defn.h
+++ b/xen/include/asm-arm/perfc_defn.h
@@ -31,6 +31,7 @@  PERFCOUNTER(vpsci_system_off,          "vpsci: system_off")
 PERFCOUNTER(vpsci_system_reset,        "vpsci: system_reset")
 PERFCOUNTER(vpsci_cpu_suspend,         "vpsci: cpu_suspend")
 PERFCOUNTER(vpsci_cpu_affinity_info,   "vpsci: cpu_affinity_info")
+PERFCOUNTER(vpsci_features,            "vpsci: features")
 
 PERFCOUNTER(vgicd_reads,                "vgicd: read")
 PERFCOUNTER(vgicd_writes,               "vgicd: write")
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index becc9f9ded..e2629eed01 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -40,6 +40,7 @@  void call_psci_system_reset(void);
 #define PSCI_0_2_FN32_MIGRATE_INFO_TYPE   PSCI_0_2_FN32(6)
 #define PSCI_0_2_FN32_SYSTEM_OFF          PSCI_0_2_FN32(8)
 #define PSCI_0_2_FN32_SYSTEM_RESET        PSCI_0_2_FN32(9)
+#define PSCI_1_0_FN32_PSCI_FEATURES       PSCI_0_2_FN32(10)
 
 #define PSCI_0_2_FN64_CPU_SUSPEND         PSCI_0_2_FN64(1)
 #define PSCI_0_2_FN64_CPU_ON              PSCI_0_2_FN64(3)
diff --git a/xen/include/asm-arm/vpsci.h b/xen/include/asm-arm/vpsci.h
index d6a890f6a2..6d98c3651c 100644
--- a/xen/include/asm-arm/vpsci.h
+++ b/xen/include/asm-arm/vpsci.h
@@ -4,7 +4,7 @@ 
 #include <asm/psci.h>
 
 /* Number of function implemented by virtual PSCI (only 0.2 or later) */
-#define VPSCI_NR_FUNCS  11
+#define VPSCI_NR_FUNCS  12
 
 /* Functions handle PSCI calls from the guests */
 bool do_vpsci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);