[PATCH v2 00/15] xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update

[PATCH v2 00/15] xen/arm: PSCI 1.1 and SMCCC-1.1 support and XSA-254 variant 2 update

[Julien Grall]

Hi all,

Arm has recently published a SMC Calling Convention (SMCCC)
specification update [1] that provides an optimised calling convention
and optional, discoverable support for mitigating CVE-2017-5715 (XSA-254
variant 2). ARM Trusted Firmware (ATF) has already gained such an
implementation[2].

This series addresses a few things:

    - It provides a Xen implementation of PSCI v1.0, which is a
    prerequisite for being able to discover SMCCC v1.1.
    - It allows Xen to advertise SMCCC v1.1
    - It implements guest support for the ARM_WORKAROUND_1 function that is used
    to mitigate CVE-2017-5715 (if such mitigation is available on the
    hypervisor).
    - It adds Xen support for branch predictor hardening via
    ARM_WORKAROUND_1 if the firmware supports it.

This method is intended to fully replace the initial PSCI_GET_VERSION
approach. Although PSCI_GET_VERSION still works, it has an obvious
overhead and is called on some of the hottest paths. We expect
ARCH_WORKAROUND_1 to be much faster.

This series is based on the "xen/arm: SMCCC fixes and PSCI clean-up" one [3].

Cheers,

[1] https://developer.arm.com/support/security-update/downloads

[2] https://github.com/ARM-software/arm-trusted-firmware/pull/1240

[3] https://lists.xen.org/archives/html/xen-devel/2018-02/msg00447.html

Julien Grall (15):
  xen/arm: psci: Rework the PSCI definitions
  xen/arm: vpsci: Add support for PSCI 1.1
  xen/arm: vsmc: Implement SMCCC 1.1
  xen/arm: vsmc: Implement SMCCC_ARCH_WORKAROUND_1 BP hardening support
  xen/arm: Adapt smccc.h to be able to use it in assembly code
  xen/arm64: Implement a fast path for handling SMCCC_ARCH_WORKAROUND_1
  xen/arm64: Print a per-CPU message with the BP hardening method used
  xen/arm: smccc: Add macros SMCCC_VERSION, SMCCC_VERSION_{MINOR, MAJOR}
  xen/arm: psci: Detect SMCCC version
  xen/arm: smccc: Implement SMCCC v1.1 inline primitive
  xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support
  xen/arm64: Kill PSCI_GET_VERSION as a variant-2 workaround
  xen/arm: vpsci: Remove parameter 'ver' from do_common_cpu
  xen/arm: psci: Consolidate PSCI version print
  xen/arm: psci: Prefix with static any functions not exported

 tools/libxl/libxl_arm.c          |   3 +-
 xen/arch/arm/arm64/bpi.S         |  35 +++-----
 xen/arch/arm/arm64/entry.S       |  56 ++++++++++++-
 xen/arch/arm/cpuerrata.c         |  55 +++++++++----
 xen/arch/arm/domain_build.c      |   1 +
 xen/arch/arm/platforms/seattle.c |   4 +-
 xen/arch/arm/psci.c              |  58 +++++++++----
 xen/arch/arm/vpsci.c             |  90 +++++++++++++++-----
 xen/arch/arm/vsmc.c              |  41 +++++++++
 xen/include/asm-arm/perfc_defn.h |   1 +
 xen/include/asm-arm/processor.h  |   2 +
 xen/include/asm-arm/psci.h       |  38 +++++----
 xen/include/asm-arm/smccc.h      | 174 +++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/vpsci.h      |   2 +-
 14 files changed, 453 insertions(+), 107 deletions(-)

-- 
2.11.0


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

[PATCH v2 01/15] xen/arm: psci: Rework the PSCI definitions

[Julien Grall]

Some PSCI functions are only available in the 32-bit version. After
recent changes, Xen always needs to know whether the call was made using
32-bit id or 64-bit id. So we don't emulate reserved one.

With the current naming scheme, it is not easy to know which call
supports 32-bit and 64-bit id. So rework the definitions to encode the
version in the name. From now the functions will be named PSCI_0_2_FNxx
where xx is 32 or 64.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

---
    Changes in v2:
        - Add Volodymyr's reviewed-by
---
 xen/arch/arm/platforms/seattle.c |  4 ++--
 xen/arch/arm/psci.c              | 10 +++++-----
 xen/arch/arm/vpsci.c             | 22 +++++++++++-----------
 xen/include/asm-arm/psci.h       | 37 +++++++++++++++++++++----------------
 4 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 22c062293f..893cc17972 100644
--- a/xen/arch/arm/platforms/seattle.c
+++ b/xen/arch/arm/platforms/seattle.c
@@ -33,12 +33,12 @@ static const char * const seattle_dt_compat[] __initconst =
  */
 static void seattle_system_reset(void)
 {
-    call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
+    call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
 }
 
 static void seattle_system_off(void)
 {
-    call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
+    call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
 }
 
 PLATFORM_START(seattle, "SEATTLE")
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 1508a3be3a..5dda35cd7c 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -31,9 +31,9 @@
  * (native-width) function ID.
  */
 #ifdef CONFIG_ARM_64
-#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN64(name)
+#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN64_##name
 #else
-#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN32(name)
+#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN32_##name
 #endif
 
 uint32_t psci_ver;
@@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
 void call_psci_system_off(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
+        call_smc(PSCI_0_2_FN32_SYSTEM_OFF, 0, 0, 0);
 }
 
 void call_psci_system_reset(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
+        call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
 }
 
 int __init psci_is_smc_method(const struct dt_device_node *psci)
@@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
         }
     }
 
-    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_VERSION), 0, 0, 0);
+    psci_ver = call_smc(PSCI_0_2_FN32_PSCI_VERSION, 0, 0, 0);
 
     /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 03fd4eb5b5..6ab8ab64d0 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -243,35 +243,35 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
      */
     switch ( fid )
     {
-    case PSCI_0_2_FN32(PSCI_VERSION):
+    case PSCI_0_2_FN32_PSCI_VERSION:
         perfc_incr(vpsci_version);
         PSCI_SET_RESULT(regs, do_psci_0_2_version());
         return true;
 
-    case PSCI_0_2_FN32(CPU_OFF):
+    case PSCI_0_2_FN32_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
         return true;
 
-    case PSCI_0_2_FN32(MIGRATE_INFO_TYPE):
+    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
         PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
         return true;
 
-    case PSCI_0_2_FN32(SYSTEM_OFF):
+    case PSCI_0_2_FN32_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         return true;
 
-    case PSCI_0_2_FN32(SYSTEM_RESET):
+    case PSCI_0_2_FN32_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         return true;
 
-    case PSCI_0_2_FN32(CPU_ON):
-    case PSCI_0_2_FN64(CPU_ON):
+    case PSCI_0_2_FN32_CPU_ON:
+    case PSCI_0_2_FN64_CPU_ON:
     {
         register_t vcpuid = PSCI_ARG(regs, 1);
         register_t epoint = PSCI_ARG(regs, 2);
@@ -282,8 +282,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
         return true;
     }
 
-    case PSCI_0_2_FN32(CPU_SUSPEND):
-    case PSCI_0_2_FN64(CPU_SUSPEND):
+    case PSCI_0_2_FN32_CPU_SUSPEND:
+    case PSCI_0_2_FN64_CPU_SUSPEND:
     {
         uint32_t pstate = PSCI_ARG32(regs, 1);
         register_t epoint = PSCI_ARG(regs, 2);
@@ -294,8 +294,8 @@ bool do_vpsci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
         return true;
     }
 
-    case PSCI_0_2_FN32(AFFINITY_INFO):
-    case PSCI_0_2_FN64(AFFINITY_INFO):
+    case PSCI_0_2_FN32_AFFINITY_INFO:
+    case PSCI_0_2_FN64_AFFINITY_INFO:
     {
         register_t taff = PSCI_ARG(regs, 1);
         uint32_t laff = PSCI_ARG32(regs, 2);
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 3c44468e72..becc9f9ded 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -23,22 +23,27 @@ void call_psci_system_off(void);
 void call_psci_system_reset(void);
 
 /* PSCI v0.2 interface */
-#define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
-                                               ARM_SMCCC_CONV_32,               \
-                                               ARM_SMCCC_OWNER_STANDARD,        \
-                                               PSCI_0_2_FN_##name)
-#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
-                                               ARM_SMCCC_CONV_64,               \
-                                               ARM_SMCCC_OWNER_STANDARD,        \
-                                               PSCI_0_2_FN_##name)
-#define PSCI_0_2_FN_PSCI_VERSION        0
-#define PSCI_0_2_FN_CPU_SUSPEND         1
-#define PSCI_0_2_FN_CPU_OFF             2
-#define PSCI_0_2_FN_CPU_ON              3
-#define PSCI_0_2_FN_AFFINITY_INFO       4
-#define PSCI_0_2_FN_MIGRATE_INFO_TYPE   6
-#define PSCI_0_2_FN_SYSTEM_OFF          8
-#define PSCI_0_2_FN_SYSTEM_RESET        9
+#define PSCI_0_2_FN32(nr) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
+                                             ARM_SMCCC_CONV_32,               \
+                                             ARM_SMCCC_OWNER_STANDARD,        \
+                                             nr)
+#define PSCI_0_2_FN64(nr) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
+                                             ARM_SMCCC_CONV_64,               \
+                                             ARM_SMCCC_OWNER_STANDARD,        \
+                                             nr)
+
+#define PSCI_0_2_FN32_PSCI_VERSION        PSCI_0_2_FN32(0)
+#define PSCI_0_2_FN32_CPU_SUSPEND         PSCI_0_2_FN32(1)
+#define PSCI_0_2_FN32_CPU_OFF             PSCI_0_2_FN32(2)
+#define PSCI_0_2_FN32_CPU_ON              PSCI_0_2_FN32(3)
+#define PSCI_0_2_FN32_AFFINITY_INFO       PSCI_0_2_FN32(4)
+#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_0_2_FN64_CPU_SUSPEND         PSCI_0_2_FN64(1)
+#define PSCI_0_2_FN64_CPU_ON              PSCI_0_2_FN64(3)
+#define PSCI_0_2_FN64_AFFINITY_INFO       PSCI_0_2_FN64(4)
 
 /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
 #define PSCI_0_2_AFFINITY_LEVEL_ON      0
-- 
2.11.0


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

[PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Julien Grall]

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.

    Changes in v2:
        - Return v1.1 on GET_VERSION call as claimed by this patch
        - Order by function ID the calls in FEATURES call
---
 tools/libxl/libxl_arm.c          |  3 ++-
 xen/arch/arm/domain_build.c      |  1 +
 xen/arch/arm/vpsci.c             | 39 ++++++++++++++++++++++++++++++++++++++-
 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, 44 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 6ab8ab64d0..e82b62db1a 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -106,7 +106,11 @@ 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);
+    /*
+     * PSCI is backward compatible from 0.2. So we can bump the version
+     * without any issue.
+     */
+    return PSCI_VERSION(1, 1);
 }
 
 static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
@@ -191,6 +195,29 @@ 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)
+{
+    /* /!\ Ordered by function ID and not name */
+    switch ( psci_func_id )
+    {
+    case PSCI_0_2_FN32_PSCI_VERSION:
+    case PSCI_0_2_FN32_CPU_SUSPEND:
+    case PSCI_0_2_FN64_CPU_SUSPEND:
+    case PSCI_0_2_FN32_CPU_OFF:
+    case PSCI_0_2_FN32_CPU_ON:
+    case PSCI_0_2_FN64_CPU_ON:
+    case PSCI_0_2_FN32_AFFINITY_INFO:
+    case PSCI_0_2_FN64_AFFINITY_INFO:
+    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
+    case PSCI_0_2_FN32_SYSTEM_OFF:
+    case PSCI_0_2_FN32_SYSTEM_RESET:
+    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)
 
@@ -304,6 +331,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 035a41e812..0cca5e6830 100644
--- a/xen/include/asm-arm/vpsci.h
+++ b/xen/include/asm-arm/vpsci.h
@@ -23,7 +23,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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Volodymyr Babchuk]

Hi Julien,

On 08.02.18 21:21, Julien Grall 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.
> 
>      Changes in v2:
>          - Return v1.1 on GET_VERSION call as claimed by this patch
>          - Order by function ID the calls in FEATURES call
> ---
>   tools/libxl/libxl_arm.c          |  3 ++-
>   xen/arch/arm/domain_build.c      |  1 +
>   xen/arch/arm/vpsci.c             | 39 ++++++++++++++++++++++++++++++++++++++-
>   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, 44 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");
What about this place? Should it be "arm,psci-1.1"?

>       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 6ab8ab64d0..e82b62db1a 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -106,7 +106,11 @@ 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);
> +    /*
> +     * PSCI is backward compatible from 0.2. So we can bump the version
> +     * without any issue.
> +     */
> +    return PSCI_VERSION(1, 1);
>   }
>   
>   static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
> @@ -191,6 +195,29 @@ 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)
> +{
> +    /* /!\ Ordered by function ID and not name */
> +    switch ( psci_func_id )
> +    {
> +    case PSCI_0_2_FN32_PSCI_VERSION:
> +    case PSCI_0_2_FN32_CPU_SUSPEND:
> +    case PSCI_0_2_FN64_CPU_SUSPEND:
> +    case PSCI_0_2_FN32_CPU_OFF:
> +    case PSCI_0_2_FN32_CPU_ON:
> +    case PSCI_0_2_FN64_CPU_ON:
> +    case PSCI_0_2_FN32_AFFINITY_INFO:
> +    case PSCI_0_2_FN64_AFFINITY_INFO:
> +    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
> +    case PSCI_0_2_FN32_SYSTEM_OFF:
> +    case PSCI_0_2_FN32_SYSTEM_RESET:
> +    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)
>   
> @@ -304,6 +331,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 035a41e812..0cca5e6830 100644
> --- a/xen/include/asm-arm/vpsci.h
> +++ b/xen/include/asm-arm/vpsci.h
> @@ -23,7 +23,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);
> 

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Julien Grall]

On 02/09/2018 04:07 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

> On 08.02.18 21:21, Julien Grall 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.
>>
>>      Changes in v2:
>>          - Return v1.1 on GET_VERSION call as claimed by this patch
>>          - Order by function ID the calls in FEATURES call
>> ---
>>   tools/libxl/libxl_arm.c          |  3 ++-
>>   xen/arch/arm/domain_build.c      |  1 +
>>   xen/arch/arm/vpsci.c             | 39 
>> ++++++++++++++++++++++++++++++++++++++-
>>   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, 44 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");
> What about this place? Should it be "arm,psci-1.1"?

arm,psci-1.1 compatible string does not exist. Technically after 0.2 you 
should discover the PSCI version through GET_VERSION. So I am not 
entirely sure why arm,psci-1.0 compatible was added.

 From the documentation 
(Documentation/devicetree/bindings/arm/psci.txt), the compatibles means 
the PSCI implementation comply to a given version. Our implementation 
complies to 0.1, 0.2 and 1.0. So I have added 1.0 just in case a guest 
decides to check the compatible.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Volodymyr Babchuk]

On 09.02.18 18:13, Julien Grall wrote:
> 
> 
> On 02/09/2018 04:07 PM, Volodymyr Babchuk wrote:
>> Hi Julien,
> 
> Hi Volodymyr,
> 
>> On 08.02.18 21:21, Julien Grall 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
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.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.
>>>
>>>      Changes in v2:
>>>          - Return v1.1 on GET_VERSION call as claimed by this patch
>>>          - Order by function ID the calls in FEATURES call
>>> ---
>>>   tools/libxl/libxl_arm.c          |  3 ++-
>>>   xen/arch/arm/domain_build.c      |  1 +
>>>   xen/arch/arm/vpsci.c             | 39 
>>> ++++++++++++++++++++++++++++++++++++++-
>>>   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, 44 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");
>> What about this place? Should it be "arm,psci-1.1"?
> 
> arm,psci-1.1 compatible string does not exist. Technically after 0.2 you 
> should discover the PSCI version through GET_VERSION. So I am not 
> entirely sure why arm,psci-1.0 compatible was added.
> 
>  From the documentation 
> (Documentation/devicetree/bindings/arm/psci.txt), the compatibles means 
> the PSCI implementation comply to a given version. Our implementation 
> complies to 0.1, 0.2 and 1.0. So I have added 1.0 just in case a guest 
> decides to check the compatible.

Okay, makes sense

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Wei Liu]

On Thu, Feb 08, 2018 at 07:21:50PM +0000, Julien Grall 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>


Acked-by: Wei Liu <wei.liu2@citrix.com>

I will leave this patch to ARM committers.

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Mirela Simonovic]

Hi Julien,

I've done pretty much the same work in parallel, but there are few 
additional minor changes I've made. Briefly, the difference is in return 
values that some already implemented functions should return starting 
from v1.0 (and even v0.2 errata). Please let me know whether you omitted 
that intentionally.
I can submit these patches if you want. Currently I have few - one for 
each fix, easier to review. I guess all of them should be squashed with 
the patch you submitted.

One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should be 
returned for all optional functions that are not implemented. Is that 
the case? I.e. when there is no case for a particular function ID in 
do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned?


On 02/08/2018 08:21 PM, Julien Grall 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.
>
>      Changes in v2:
>          - Return v1.1 on GET_VERSION call as claimed by this patch
>          - Order by function ID the calls in FEATURES call
> ---
>   tools/libxl/libxl_arm.c          |  3 ++-
>   xen/arch/arm/domain_build.c      |  1 +
>   xen/arch/arm/vpsci.c             | 39 ++++++++++++++++++++++++++++++++++++++-
>   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, 44 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 6ab8ab64d0..e82b62db1a 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -106,7 +106,11 @@ 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);
> +    /*
> +     * PSCI is backward compatible from 0.2. So we can bump the version
> +     * without any issue.
> +     */
> +    return PSCI_VERSION(1, 1);
>   }
>   
>   static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
> @@ -191,6 +195,29 @@ 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)
> +{
> +    /* /!\ Ordered by function ID and not name */
> +    switch ( psci_func_id )
> +    {
> +    case PSCI_0_2_FN32_PSCI_VERSION:
> +    case PSCI_0_2_FN32_CPU_SUSPEND:
> +    case PSCI_0_2_FN64_CPU_SUSPEND:

Just a note here - PSCI_FEATURES should return additional information 
just for CPU_SUSPEND (supported power state and mode). AFAIU, that value 
is also 0, so the return code should be fine.

> +    case PSCI_0_2_FN32_CPU_OFF:
> +    case PSCI_0_2_FN32_CPU_ON:
> +    case PSCI_0_2_FN64_CPU_ON:
> +    case PSCI_0_2_FN32_AFFINITY_INFO:
> +    case PSCI_0_2_FN64_AFFINITY_INFO:
> +    case PSCI_0_2_FN32_MIGRATE_INFO_TYPE:
> +    case PSCI_0_2_FN32_SYSTEM_OFF:
> +    case PSCI_0_2_FN32_SYSTEM_RESET:
> +    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)
>   
> @@ -304,6 +331,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 035a41e812..0cca5e6830 100644
> --- a/xen/include/asm-arm/vpsci.h
> +++ b/xen/include/asm-arm/vpsci.h
> @@ -23,7 +23,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);


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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Julien Grall]

On 12/02/2018 20:12, Mirela Simonovic wrote:
> Hi Julien,

Hi Mirela,

Thank you for the review.

> I've done pretty much the same work in parallel, but there are few 
> additional minor changes I've made. Briefly, the difference is in return 
> values that some already implemented functions should return starting 
> from v1.0 (and even v0.2 errata). Please let me know whether you omitted 
> that intentionally.

Could you give a bit more details here? From a brief look we don't seem 
to implement correctly:
	- CPU_OFF: PSCI_DENY should be return on failure (though it should 
never fail in Xen case) and the check on the vCPU state is pointless.
	- MIGRATE_INFO_TYPE: should technically return int32_t instead of 
uint32_t. That not really matter for now.

If you speak about denying SMC64 call from AArch32, then this is already 
done in vsmccc.c (see vsmccc_call).

> I can submit these patches if you want. Currently I have few - one for 
> each fix, easier to review. I guess all of them should be squashed with 
> the patch you submitted.
> 
> One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should be 
> returned for all optional functions that are not implemented. Is that 
> the case? I.e. when there is no case for a particular function ID in 
> do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned?

This is done by vmsccc_handle_call().
See set_user_regs(regs, 0, ARM_SMCC_ERR_UNKNOWN_FUNCTION) which is 
equivalent to PSCI_NOT_SUPPORTED.

[...]

>> +static int32_t do_psci_1_0_features(uint32_t psci_func_id)
>> +{
>> +    /* /!\ Ordered by function ID and not name */
>> +    switch ( psci_func_id )
>> +    {
>> +    case PSCI_0_2_FN32_PSCI_VERSION:
>> +    case PSCI_0_2_FN32_CPU_SUSPEND:
>> +    case PSCI_0_2_FN64_CPU_SUSPEND:
> 
> Just a note here - PSCI_FEATURES should return additional information 
> just for CPU_SUSPEND (supported power state and mode). AFAIU, that value 
> is also 0, so the return code should be fine.

I think so, from what I understood this is inline with CPU_SUSPEND only 
supports 0.2 format.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Mirela Simonovic]

Hi Julien,


On 02/12/2018 10:41 PM, Julien Grall wrote:
>
>
> On 12/02/2018 20:12, Mirela Simonovic wrote:
>> Hi Julien,
>
> Hi Mirela,
>
> Thank you for the review.
>
>> I've done pretty much the same work in parallel, but there are few 
>> additional minor changes I've made. Briefly, the difference is in 
>> return values that some already implemented functions should return 
>> starting from v1.0 (and even v0.2 errata). Please let me know whether 
>> you omitted that intentionally.
>
> Could you give a bit more details here? From a brief look we don't 
> seem to implement correctly:
>     - CPU_OFF: PSCI_DENY should be return on failure (though it should 
> never fail in Xen case) and the check on the vCPU state is pointless.

I believe CPU_OFF is fine today, it never returns.

>     - MIGRATE_INFO_TYPE: should technically return int32_t instead of 
> uint32_t. That not really matter for now.
>
> If you speak about denying SMC64 call from AArch32, then this is 
> already done in vsmccc.c (see vsmccc_call).

Agreed on above, there are 2 more:

1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead 
PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not 
implemented, but in v0.2 it was mandatory, so it couldn't return 
PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). Since 
v0.2 errata and v1.0 release the function is made optional and it should 
return "not supported" error - just removing the function should be fine 
(and mismatching return type issue would be gone).

2. A new error code has been introduced in PSCI v1.0: 
PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions 
which receive an address as the argument when the provided address is 
incorrect. In implementation in Xen this affects CPU_ON and CPU_SUSPEND. 
CPU_ON today returns invalid parameter error and that needs to be 
replaced with invalid address error. I'm not sure for CPU_SUSPEND since 
its implementation doesn't use/check any of the arguments today...

Thanks,
Mirela

>
>> I can submit these patches if you want. Currently I have few - one 
>> for each fix, easier to review. I guess all of them should be 
>> squashed with the patch you submitted.
>>
>> One more note - starting from v1.0, PSCI_NOT_SUPPORTED error should 
>> be returned for all optional functions that are not implemented. Is 
>> that the case? I.e. when there is no case for a particular function 
>> ID in do_vpsci_0_2_call the PSCI_NOT_SUPPORTED error will be returned?
>
> This is done by vmsccc_handle_call().
> See set_user_regs(regs, 0, ARM_SMCC_ERR_UNKNOWN_FUNCTION) which is 
> equivalent to PSCI_NOT_SUPPORTED.
>
> [...]
>
>>> +static int32_t do_psci_1_0_features(uint32_t psci_func_id)
>>> +{
>>> +    /* /!\ Ordered by function ID and not name */
>>> +    switch ( psci_func_id )
>>> +    {
>>> +    case PSCI_0_2_FN32_PSCI_VERSION:
>>> +    case PSCI_0_2_FN32_CPU_SUSPEND:
>>> +    case PSCI_0_2_FN64_CPU_SUSPEND:
>>
>> Just a note here - PSCI_FEATURES should return additional information 
>> just for CPU_SUSPEND (supported power state and mode). AFAIU, that 
>> value is also 0, so the return code should be fine.
>
> I think so, from what I understood this is inline with CPU_SUSPEND 
> only supports 0.2 format.
>
> Cheers,
>


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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Julien Grall]

On 12/02/2018 23:16, Mirela Simonovic wrote:
> Hi Julien,

Hi,

> On 02/12/2018 10:41 PM, Julien Grall wrote:
>>
>>
>> On 12/02/2018 20:12, Mirela Simonovic wrote:
>>> Hi Julien,
>>
>> Hi Mirela,
>>
>> Thank you for the review.
>>
>>> I've done pretty much the same work in parallel, but there are few 
>>> additional minor changes I've made. Briefly, the difference is in 
>>> return values that some already implemented functions should return 
>>> starting from v1.0 (and even v0.2 errata). Please let me know whether 
>>> you omitted that intentionally.
>>
>> Could you give a bit more details here? From a brief look we don't 
>> seem to implement correctly:
>>     - CPU_OFF: PSCI_DENY should be return on failure (though it should 
>> never fail in Xen case) and the check on the vCPU state is pointless.
> 
> I believe CPU_OFF is fine today, it never returns.
> 
>>     - MIGRATE_INFO_TYPE: should technically return int32_t instead of 
>> uint32_t. That not really matter for now.
>>
>> If you speak about denying SMC64 call from AArch32, then this is 
>> already done in vsmccc.c (see vsmccc_call).
> 
> Agreed on above, there are 2 more:
> 
> 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead 
> PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not 
> implemented, but in v0.2 it was mandatory, so it couldn't return 
> PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). Since 
> v0.2 errata and v1.0 release the function is made optional and it should 
> return "not supported" error - just removing the function should be fine 
> (and mismatching return type issue would be gone).

Looking at the spec:

"2 Trusted OS is either not present or does not require migration. A 
system of this type does not require the caller to use the MIGRATE 
function. MIGRATE function calls return NOT_SUPPORTED."

So returning 2 in our case seems to be valid.

> 
> 2. A new error code has been introduced in PSCI v1.0: 
> PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions 
> which receive an address as the argument when the provided address is 
> incorrect. In implementation in Xen this affects CPU_ON and CPU_SUSPEND. 
> CPU_ON today returns invalid parameter error and that needs to be 
> replaced with invalid address error. I'm not sure for CPU_SUSPEND since 
> its implementation doesn't use/check any of the arguments today...
I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by 
PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am not 
sure where we would need to use it in Xen. The error is described as 
"INVALID_ADDRESS is returned when the entry point address is known by 
the implementation to be invalid, because it is in a range that is known 
not to be available to the caller."

The only potential one would be the check on is_thumb, but even there it 
does not match the description. The range is still available to the 
guest. I think that check should just be dropped.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Mirela Simonovic]

Hi Julien,


On 02/13/2018 12:44 AM, Julien Grall wrote:
>
>
> On 12/02/2018 23:16, Mirela Simonovic wrote:
>> Hi Julien,
>
> Hi,
>
>> On 02/12/2018 10:41 PM, Julien Grall wrote:
>>>
>>>
>>> On 12/02/2018 20:12, Mirela Simonovic wrote:
>>>> Hi Julien,
>>>
>>> Hi Mirela,
>>>
>>> Thank you for the review.
>>>
>>>> I've done pretty much the same work in parallel, but there are few 
>>>> additional minor changes I've made. Briefly, the difference is in 
>>>> return values that some already implemented functions should return 
>>>> starting from v1.0 (and even v0.2 errata). Please let me know 
>>>> whether you omitted that intentionally.
>>>
>>> Could you give a bit more details here? From a brief look we don't 
>>> seem to implement correctly:
>>>     - CPU_OFF: PSCI_DENY should be return on failure (though it 
>>> should never fail in Xen case) and the check on the vCPU state is 
>>> pointless.
>>
>> I believe CPU_OFF is fine today, it never returns.
>>
>>>     - MIGRATE_INFO_TYPE: should technically return int32_t instead 
>>> of uint32_t. That not really matter for now.
>>>
>>> If you speak about denying SMC64 call from AArch32, then this is 
>>> already done in vsmccc.c (see vsmccc_call).
>>
>> Agreed on above, there are 2 more:
>>
>> 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead 
>> PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not 
>> implemented, but in v0.2 it was mandatory, so it couldn't return 
>> PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). 
>> Since v0.2 errata and v1.0 release the function is made optional and 
>> it should return "not supported" error - just removing the function 
>> should be fine (and mismatching return type issue would be gone).
>
> Looking at the spec:
>
> "2 Trusted OS is either not present or does not require migration. A 
> system of this type does not require the caller to use the MIGRATE 
> function. MIGRATE function calls return NOT_SUPPORTED."
>
> So returning 2 in our case seems to be valid.
>
>>
>> 2. A new error code has been introduced in PSCI v1.0: 
>> PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions 
>> which receive an address as the argument when the provided address is 
>> incorrect. In implementation in Xen this affects CPU_ON and 
>> CPU_SUSPEND. CPU_ON today returns invalid parameter error and that 
>> needs to be replaced with invalid address error. I'm not sure for 
>> CPU_SUSPEND since its implementation doesn't use/check any of the 
>> arguments today...
> I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by 
> PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am 
> not sure where we would need to use it in Xen. The error is described 
> as "INVALID_ADDRESS is returned when the entry point address is known 
> by the implementation to be invalid, because it is in a range that is 
> known not to be available to the caller."
>
> The only potential one would be the check on is_thumb, but even there 
> it does not match the description. The range is still available to the 
> guest. I think that check should just be dropped.

To be more specific, I was thinking that in xen/arch/arm/vpsci.c line 41 
for psci version other than 0.1 the PSCI_INVALID_ADDRESS error should be 
returned instead PSCI_INVALID_PARAMETERS.

Cheers,
Mirela

>
> Cheers,
>


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

Re: [PATCH v2 02/15] xen/arm: vpsci: Add support for PSCI 1.1

[Julien Grall]

On 14/02/18 19:14, Mirela Simonovic wrote:
> Hi Julien,

Hi,

> On 02/13/2018 12:44 AM, Julien Grall wrote:
>>
>>
>> On 12/02/2018 23:16, Mirela Simonovic wrote:
>>> Hi Julien,
>>
>> Hi,
>>
>>> On 02/12/2018 10:41 PM, Julien Grall wrote:
>>>>
>>>>
>>>> On 12/02/2018 20:12, Mirela Simonovic wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Mirela,
>>>>
>>>> Thank you for the review.
>>>>
>>>>> I've done pretty much the same work in parallel, but there are few 
>>>>> additional minor changes I've made. Briefly, the difference is in 
>>>>> return values that some already implemented functions should return 
>>>>> starting from v1.0 (and even v0.2 errata). Please let me know 
>>>>> whether you omitted that intentionally.
>>>>
>>>> Could you give a bit more details here? From a brief look we don't 
>>>> seem to implement correctly:
>>>>     - CPU_OFF: PSCI_DENY should be return on failure (though it 
>>>> should never fail in Xen case) and the check on the vCPU state is 
>>>> pointless.
>>>
>>> I believe CPU_OFF is fine today, it never returns.
>>>
>>>>     - MIGRATE_INFO_TYPE: should technically return int32_t instead 
>>>> of uint32_t. That not really matter for now.
>>>>
>>>> If you speak about denying SMC64 call from AArch32, then this is 
>>>> already done in vsmccc.c (see vsmccc_call).
>>>
>>> Agreed on above, there are 2 more:
>>>
>>> 1. MIGRATE_INFO_TYPE should return PSCI_NOT_SUPPORTED instead 
>>> PSCI_0_2_TOS_MP_OR_NOT_PRESENT. The function is effectively not 
>>> implemented, but in v0.2 it was mandatory, so it couldn't return 
>>> PSCI_NOT_SUPPORTED (I guess this was some kind of a workaround). 
>>> Since v0.2 errata and v1.0 release the function is made optional and 
>>> it should return "not supported" error - just removing the function 
>>> should be fine (and mismatching return type issue would be gone).
>>
>> Looking at the spec:
>>
>> "2 Trusted OS is either not present or does not require migration. A 
>> system of this type does not require the caller to use the MIGRATE 
>> function. MIGRATE function calls return NOT_SUPPORTED."
>>
>> So returning 2 in our case seems to be valid.
>>
>>>
>>> 2. A new error code has been introduced in PSCI v1.0: 
>>> PSCI_INVALID_ADDRESS. This error should be returned by PSCI functions 
>>> which receive an address as the argument when the provided address is 
>>> incorrect. In implementation in Xen this affects CPU_ON and 
>>> CPU_SUSPEND. CPU_ON today returns invalid parameter error and that 
>>> needs to be replaced with invalid address error. I'm not sure for 
>>> CPU_SUSPEND since its implementation doesn't use/check any of the 
>>> arguments today...
>> I disagree, not all PSCI_INVALID_PARAMETERS should be replaced by 
>> PSCI_INVALID_ADDRESS. They have two distinct meaning. However, I am 
>> not sure where we would need to use it in Xen. The error is described 
>> as "INVALID_ADDRESS is returned when the entry point address is known 
>> by the implementation to be invalid, because it is in a range that is 
>> known not to be available to the caller."
>>
>> The only potential one would be the check on is_thumb, but even there 
>> it does not match the description. The range is still available to the 
>> guest. I think that check should just be dropped.
> 
> To be more specific, I was thinking that in xen/arch/arm/vpsci.c line 41 
> for psci version other than 0.1 the PSCI_INVALID_ADDRESS error should be 
> returned instead PSCI_INVALID_PARAMETERS.

This is exactly the place I was speaking in my previous e-mail. I am not 
entirely convinced we should keep the check or even switch the return to 
PSCI_INVALID_PARAMETERS as the usage does not entirely match the error 
description.

Cheers,

-- 
Julien Grall

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

[PATCH v2 03/15] xen/arm: vsmc: Implement SMCCC 1.1

[Julien Grall]

The new SMC Calling Convention (v1.1) allows for a reduced overhead when
calling into the firmware, and provides a new feature discovery
mechanism. See "Firmware interfaces for mitigating CVE-2017-5715"
ARM DEN 00070A.

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

---
    Changes in v2:
        - Add a humand readable name for the specification
---
 xen/arch/arm/vpsci.c        |  1 +
 xen/arch/arm/vsmc.c         | 23 +++++++++++++++++++++++
 xen/include/asm-arm/smccc.h | 15 +++++++++++++++
 3 files changed, 39 insertions(+)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index e82b62db1a..19ee7caeb4 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -212,6 +212,7 @@ static int32_t do_psci_1_0_features(uint32_t psci_func_id)
     case PSCI_0_2_FN32_SYSTEM_OFF:
     case PSCI_0_2_FN32_SYSTEM_RESET:
     case PSCI_1_0_FN32_PSCI_FEATURES:
+    case ARM_SMCCC_VERSION_FID:
         return 0;
     default:
         return PSCI_NOT_SUPPORTED;
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 3d3bd95fee..a708aa5e81 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -81,6 +81,26 @@ static bool fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt)
     return true;
 }
 
+/* SMCCC interface for ARM Architecture */
+static bool handle_arch(struct cpu_user_regs *regs)
+{
+    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
+
+    switch ( fid )
+    {
+    case ARM_SMCCC_VERSION_FID:
+        set_user_reg(regs, 0, ARM_SMCCC_VERSION_1_1);
+        return true;
+
+    case ARM_SMCCC_ARCH_FEATURES_FID:
+        /* Nothing supported yet */
+        set_user_reg(regs, 0, -1);
+        return true;
+    }
+
+    return false;
+}
+
 /* SMCCC interface for hypervisor. Tell about itself. */
 static bool handle_hypervisor(struct cpu_user_regs *regs)
 {
@@ -188,6 +208,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
     {
         switch ( smccc_get_owner(funcid) )
         {
+        case ARM_SMCCC_OWNER_ARCH:
+            handled = handle_arch(regs);
+            break;
         case ARM_SMCCC_OWNER_HYPERVISOR:
             handled = handle_hypervisor(regs);
             break;
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 62b3a8cdf5..431389c118 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -16,6 +16,9 @@
 #ifndef __ASM_ARM_SMCCC_H__
 #define __ASM_ARM_SMCCC_H__
 
+#define ARM_SMCCC_VERSION_1_0   0x10000
+#define ARM_SMCCC_VERSION_1_1   0x10001
+
 /*
  * This file provides common defines for ARM SMC Calling Convention as
  * specified in
@@ -100,6 +103,18 @@ static inline uint32_t smccc_get_owner(register_t funcid)
                        ARM_SMCCC_OWNER_##owner,     \
                        0xFF03)
 
+#define ARM_SMCCC_VERSION_FID                       \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_ARCH,        \
+                       0x0)                         \
+
+#define ARM_SMCCC_ARCH_FEATURES_FID                 \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_ARCH,        \
+                       0x1)
+
 /* Only one error code defined in SMCCC */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
 
-- 
2.11.0


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

Re: [PATCH v2 03/15] xen/arm: vsmc: Implement SMCCC 1.1

[Volodymyr Babchuk]

Hi,

On 08.02.18 21:21, Julien Grall wrote:
> The new SMC Calling Convention (v1.1) allows for a reduced overhead
> when calling into the firmware, and provides a new feature discovery 
> mechanism. See "Firmware interfaces for mitigating CVE-2017-5715" ARM
> DEN 00070A.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> --- Changes in v2: - Add a humand readable name for the
> specification --- xen/arch/arm/vpsci.c        |  1 + 
> xen/arch/arm/vsmc.c         | 23 +++++++++++++++++++++++ 
> xen/include/asm-arm/smccc.h | 15 +++++++++++++++ 3 files changed, 39
> insertions(+)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index
> e82b62db1a..19ee7caeb4 100644 --- a/xen/arch/arm/vpsci.c +++
> b/xen/arch/arm/vpsci.c @@ -212,6 +212,7 @@ static int32_t
> do_psci_1_0_features(uint32_t psci_func_id) case
> PSCI_0_2_FN32_SYSTEM_OFF: case PSCI_0_2_FN32_SYSTEM_RESET: case
> PSCI_1_0_FN32_PSCI_FEATURES: +    case ARM_SMCCC_VERSION_FID: return
> 0; default: return PSCI_NOT_SUPPORTED; diff --git
> a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index
> 3d3bd95fee..a708aa5e81 100644 --- a/xen/arch/arm/vsmc.c +++
> b/xen/arch/arm/vsmc.c @@ -81,6 +81,26 @@ static bool
> fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt) 
> return true; }
> 
> +/* SMCCC interface for ARM Architecture */ +static bool
> handle_arch(struct cpu_user_regs *regs) +{ +    uint32_t fid =
> (uint32_t)get_user_reg(regs, 0); + +    switch ( fid ) +    { +
> case ARM_SMCCC_VERSION_FID: +        set_user_reg(regs, 0,
> ARM_SMCCC_VERSION_1_1); +        return true; + +    case
> ARM_SMCCC_ARCH_FEATURES_FID: +        /* Nothing supported yet */ +
> set_user_reg(regs, 0, -1); +        return true; +    } + +    return
> false; +} + /* SMCCC interface for hypervisor. Tell about itself. */ 
> static bool handle_hypervisor(struct cpu_user_regs *regs) { @@ -188,6
> +208,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) 
> { switch ( smccc_get_owner(funcid) ) { +        case
> ARM_SMCCC_OWNER_ARCH: +            handled = handle_arch(regs); +
> break; case ARM_SMCCC_OWNER_HYPERVISOR: handled =
> handle_hypervisor(regs); break; diff --git
> a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index
> 62b3a8cdf5..431389c118 100644 --- a/xen/include/asm-arm/smccc.h +++
> b/xen/include/asm-arm/smccc.h @@ -16,6 +16,9 @@ #ifndef
> __ASM_ARM_SMCCC_H__ #define __ASM_ARM_SMCCC_H__
> 
> +#define ARM_SMCCC_VERSION_1_0   0x10000 +#define
> ARM_SMCCC_VERSION_1_1   0x10001 + /* * This file provides common
> defines for ARM SMC Calling Convention as * specified in @@ -100,6
> +103,18 @@ static inline uint32_t smccc_get_owner(register_t funcid) 
> ARM_SMCCC_OWNER_##owner,     \ 0xFF03)
> 
> +#define ARM_SMCCC_VERSION_FID                       \ +
> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
> ARM_SMCCC_CONV_32,           \ +
> ARM_SMCCC_OWNER_ARCH,        \ +                       0x0)
> \ + +#define ARM_SMCCC_ARCH_FEATURES_FID                 \ +
> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
> ARM_SMCCC_CONV_32,           \ +
> ARM_SMCCC_OWNER_ARCH,        \ +                       0x1) + /* Only
> one error code defined in SMCCC */ #define
> ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> 
> 

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 03/15] xen/arm: vsmc: Implement SMCCC 1.1

[Julien Grall]

On 02/09/2018 04:08 PM, Volodymyr Babchuk wrote:
> Hi,

Hi Volodymyr,

Thank you for the review. I have noticed that your e-mail client seem to 
mess up with the e-mail sent sometimes (see below). You may want to 
configure it to avoid that.

Cheers,

> On 08.02.18 21:21, Julien Grall wrote:
>> The new SMC Calling Convention (v1.1) allows for a reduced overhead
>> when calling into the firmware, and provides a new feature discovery 
>> mechanism. See "Firmware interfaces for mitigating CVE-2017-5715" ARM
>> DEN 00070A.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> 
>> --- Changes in v2: - Add a humand readable name for the
>> specification --- xen/arch/arm/vpsci.c        |  1 + 
>> xen/arch/arm/vsmc.c         | 23 +++++++++++++++++++++++ 
>> xen/include/asm-arm/smccc.h | 15 +++++++++++++++ 3 files changed, 39
>> insertions(+)
>>
>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index
>> e82b62db1a..19ee7caeb4 100644 --- a/xen/arch/arm/vpsci.c +++
>> b/xen/arch/arm/vpsci.c @@ -212,6 +212,7 @@ static int32_t
>> do_psci_1_0_features(uint32_t psci_func_id) case
>> PSCI_0_2_FN32_SYSTEM_OFF: case PSCI_0_2_FN32_SYSTEM_RESET: case
>> PSCI_1_0_FN32_PSCI_FEATURES: +    case ARM_SMCCC_VERSION_FID: return
>> 0; default: return PSCI_NOT_SUPPORTED; diff --git
>> a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index
>> 3d3bd95fee..a708aa5e81 100644 --- a/xen/arch/arm/vsmc.c +++
>> b/xen/arch/arm/vsmc.c @@ -81,6 +81,26 @@ static bool
>> fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt) 
>> return true; }
>>
>> +/* SMCCC interface for ARM Architecture */ +static bool
>> handle_arch(struct cpu_user_regs *regs) +{ +    uint32_t fid =
>> (uint32_t)get_user_reg(regs, 0); + +    switch ( fid ) +    { +
>> case ARM_SMCCC_VERSION_FID: +        set_user_reg(regs, 0,
>> ARM_SMCCC_VERSION_1_1); +        return true; + +    case
>> ARM_SMCCC_ARCH_FEATURES_FID: +        /* Nothing supported yet */ +
>> set_user_reg(regs, 0, -1); +        return true; +    } + +    return
>> false; +} + /* SMCCC interface for hypervisor. Tell about itself. */ 
>> static bool handle_hypervisor(struct cpu_user_regs *regs) { @@ -188,6
>> +208,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) { 
>> switch ( smccc_get_owner(funcid) ) { +        case
>> ARM_SMCCC_OWNER_ARCH: +            handled = handle_arch(regs); +
>> break; case ARM_SMCCC_OWNER_HYPERVISOR: handled =
>> handle_hypervisor(regs); break; diff --git
>> a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index
>> 62b3a8cdf5..431389c118 100644 --- a/xen/include/asm-arm/smccc.h +++
>> b/xen/include/asm-arm/smccc.h @@ -16,6 +16,9 @@ #ifndef
>> __ASM_ARM_SMCCC_H__ #define __ASM_ARM_SMCCC_H__
>>
>> +#define ARM_SMCCC_VERSION_1_0   0x10000 +#define
>> ARM_SMCCC_VERSION_1_1   0x10001 + /* * This file provides common
>> defines for ARM SMC Calling Convention as * specified in @@ -100,6
>> +103,18 @@ static inline uint32_t smccc_get_owner(register_t funcid) 
>> ARM_SMCCC_OWNER_##owner,     \ 0xFF03)
>>
>> +#define ARM_SMCCC_VERSION_FID                       \ +
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
>> ARM_SMCCC_CONV_32,           \ +
>> ARM_SMCCC_OWNER_ARCH,        \ +                       0x0)
>> \ + +#define ARM_SMCCC_ARCH_FEATURES_FID                 \ +
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
>> ARM_SMCCC_CONV_32,           \ +
>> ARM_SMCCC_OWNER_ARCH,        \ +                       0x1) + /* Only
>> one error code defined in SMCCC */ #define
>> ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>>
>>
> 

-- 
Julien Grall

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

Re: [PATCH v2 03/15] xen/arm: vsmc: Implement SMCCC 1.1

[Volodymyr Babchuk]

On 09.02.18 18:15, Julien Grall wrote:
> 
> 
> On 02/09/2018 04:08 PM, Volodymyr Babchuk wrote:
>> Hi,
> 
> Hi Volodymyr,
> 
> Thank you for the review. I have noticed that your e-mail client seem to 
> mess up with the e-mail sent sometimes (see below). You may want to 
> configure it to avoid that.
Oops, sorry for this. Looks like I hit CTRL+R in Thunderbird accidentally.

>> On 08.02.18 21:21, Julien Grall wrote:
>>> The new SMC Calling Convention (v1.1) allows for a reduced overhead
>>> when calling into the firmware, and provides a new feature discovery 
>>> mechanism. See "Firmware interfaces for mitigating CVE-2017-5715" ARM
>>> DEN 00070A.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>
>>> --- Changes in v2: - Add a humand readable name for the
>>> specification --- xen/arch/arm/vpsci.c        |  1 + 
>>> xen/arch/arm/vsmc.c         | 23 +++++++++++++++++++++++ 
>>> xen/include/asm-arm/smccc.h | 15 +++++++++++++++ 3 files changed, 39
>>> insertions(+)
>>>
>>> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index
>>> e82b62db1a..19ee7caeb4 100644 --- a/xen/arch/arm/vpsci.c +++
>>> b/xen/arch/arm/vpsci.c @@ -212,6 +212,7 @@ static int32_t
>>> do_psci_1_0_features(uint32_t psci_func_id) case
>>> PSCI_0_2_FN32_SYSTEM_OFF: case PSCI_0_2_FN32_SYSTEM_RESET: case
>>> PSCI_1_0_FN32_PSCI_FEATURES: +    case ARM_SMCCC_VERSION_FID: return
>>> 0; default: return PSCI_NOT_SUPPORTED; diff --git
>>> a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c index
>>> 3d3bd95fee..a708aa5e81 100644 --- a/xen/arch/arm/vsmc.c +++
>>> b/xen/arch/arm/vsmc.c @@ -81,6 +81,26 @@ static bool
>>> fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt) 
>>> return true; }
>>>
>>> +/* SMCCC interface for ARM Architecture */ +static bool
>>> handle_arch(struct cpu_user_regs *regs) +{ +    uint32_t fid =
>>> (uint32_t)get_user_reg(regs, 0); + +    switch ( fid ) +    { +
>>> case ARM_SMCCC_VERSION_FID: +        set_user_reg(regs, 0,
>>> ARM_SMCCC_VERSION_1_1); +        return true; + +    case
>>> ARM_SMCCC_ARCH_FEATURES_FID: +        /* Nothing supported yet */ +
>>> set_user_reg(regs, 0, -1); +        return true; +    } + +    return
>>> false; +} + /* SMCCC interface for hypervisor. Tell about itself. */ 
>>> static bool handle_hypervisor(struct cpu_user_regs *regs) { @@ -188,6
>>> +208,9 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) 
>>> { switch ( smccc_get_owner(funcid) ) { +        case
>>> ARM_SMCCC_OWNER_ARCH: +            handled = handle_arch(regs); +
>>> break; case ARM_SMCCC_OWNER_HYPERVISOR: handled =
>>> handle_hypervisor(regs); break; diff --git
>>> a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h index
>>> 62b3a8cdf5..431389c118 100644 --- a/xen/include/asm-arm/smccc.h +++
>>> b/xen/include/asm-arm/smccc.h @@ -16,6 +16,9 @@ #ifndef
>>> __ASM_ARM_SMCCC_H__ #define __ASM_ARM_SMCCC_H__
>>>
>>> +#define ARM_SMCCC_VERSION_1_0   0x10000 +#define
>>> ARM_SMCCC_VERSION_1_1   0x10001 + /* * This file provides common
>>> defines for ARM SMC Calling Convention as * specified in @@ -100,6
>>> +103,18 @@ static inline uint32_t smccc_get_owner(register_t funcid) 
>>> ARM_SMCCC_OWNER_##owner,     \ 0xFF03)
>>>
>>> +#define ARM_SMCCC_VERSION_FID                       \ +
>>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
>>> ARM_SMCCC_CONV_32,           \ +
>>> ARM_SMCCC_OWNER_ARCH,        \ +                       0x0)
>>> \ + +#define ARM_SMCCC_ARCH_FEATURES_FID                 \ +
>>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \ +
>>> ARM_SMCCC_CONV_32,           \ +
>>> ARM_SMCCC_OWNER_ARCH,        \ +                       0x1) + /* Only
>>> one error code defined in SMCCC */ #define
>>> ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>>>
>>>
>>
> 

-- 
Volodymyr Babchuk

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

[PATCH v2 04/15] xen/arm: vsmc: Implement SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Julien Grall]

SMCCC 1.1 offers firmware-based CPU workarounds. In particular,
SMCCC_ARCH_WORKAROUND_1 provides BP hardening for variant 2 of XSA-254
(CVE-2017-5715).

If the hypervisor has some mitigation for this issue, report that we
deal with it using SMCCC_ARCH_WORKAROUND_1, as we apply the hypervisor
workaround on every guest exit.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

---
    Changes in v2:
        - Add Volodymyr's reviewed-by
---
 xen/arch/arm/vsmc.c         | 22 ++++++++++++++++++++--
 xen/include/asm-arm/smccc.h |  6 ++++++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index a708aa5e81..144a1cd761 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -18,6 +18,7 @@
 #include <xen/lib.h>
 #include <xen/types.h>
 #include <public/arch-arm/smccc.h>
+#include <asm/cpufeature.h>
 #include <asm/monitor.h>
 #include <asm/regs.h>
 #include <asm/smccc.h>
@@ -93,8 +94,25 @@ static bool handle_arch(struct cpu_user_regs *regs)
         return true;
 
     case ARM_SMCCC_ARCH_FEATURES_FID:
-        /* Nothing supported yet */
-        set_user_reg(regs, 0, -1);
+    {
+        uint32_t arch_func_id = get_user_reg(regs, 1);
+        int ret = -1;
+
+        switch ( arch_func_id )
+        {
+        case ARM_SMCCC_ARCH_WORKAROUND_1_FID:
+            if ( cpus_have_cap(ARM_HARDEN_BRANCH_PREDICTOR) )
+                ret = 0;
+            break;
+        }
+
+        set_user_reg(regs, 0, ret);
+
+        return true;
+    }
+
+    case ARM_SMCCC_ARCH_WORKAROUND_1_FID:
+        /* No return value */
         return true;
     }
 
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 431389c118..b790fac17c 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -115,6 +115,12 @@ static inline uint32_t smccc_get_owner(register_t funcid)
                        ARM_SMCCC_OWNER_ARCH,        \
                        0x1)
 
+#define ARM_SMCCC_ARCH_WORKAROUND_1_FID             \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                      ARM_SMCCC_CONV_32,            \
+                      ARM_SMCCC_OWNER_ARCH,         \
+                      0x8000)
+
 /* Only one error code defined in SMCCC */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
 
-- 
2.11.0


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

Re: [PATCH v2 04/15] xen/arm: vsmc: Implement SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Stefano Stabellini]

On Thu, 8 Feb 2018, Julien Grall wrote:
> SMCCC 1.1 offers firmware-based CPU workarounds. In particular,
> SMCCC_ARCH_WORKAROUND_1 provides BP hardening for variant 2 of XSA-254
> (CVE-2017-5715).
> 
> If the hypervisor has some mitigation for this issue, report that we
> deal with it using SMCCC_ARCH_WORKAROUND_1, as we apply the hypervisor
> workaround on every guest exit.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Add Volodymyr's reviewed-by
> ---
>  xen/arch/arm/vsmc.c         | 22 ++++++++++++++++++++--
>  xen/include/asm-arm/smccc.h |  6 ++++++
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index a708aa5e81..144a1cd761 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -18,6 +18,7 @@
>  #include <xen/lib.h>
>  #include <xen/types.h>
>  #include <public/arch-arm/smccc.h>
> +#include <asm/cpufeature.h>
>  #include <asm/monitor.h>
>  #include <asm/regs.h>
>  #include <asm/smccc.h>
> @@ -93,8 +94,25 @@ static bool handle_arch(struct cpu_user_regs *regs)
>          return true;
>  
>      case ARM_SMCCC_ARCH_FEATURES_FID:
> -        /* Nothing supported yet */
> -        set_user_reg(regs, 0, -1);
> +    {
> +        uint32_t arch_func_id = get_user_reg(regs, 1);
> +        int ret = -1;
> +
> +        switch ( arch_func_id )
> +        {
> +        case ARM_SMCCC_ARCH_WORKAROUND_1_FID:
> +            if ( cpus_have_cap(ARM_HARDEN_BRANCH_PREDICTOR) )
> +                ret = 0;
> +            break;
> +        }
> +
> +        set_user_reg(regs, 0, ret);
> +
> +        return true;
> +    }
> +
> +    case ARM_SMCCC_ARCH_WORKAROUND_1_FID:
> +        /* No return value */
>          return true;
>      }
>  
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 431389c118..b790fac17c 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -115,6 +115,12 @@ static inline uint32_t smccc_get_owner(register_t funcid)
>                         ARM_SMCCC_OWNER_ARCH,        \
>                         0x1)
>  
> +#define ARM_SMCCC_ARCH_WORKAROUND_1_FID             \
> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
> +                      ARM_SMCCC_CONV_32,            \
> +                      ARM_SMCCC_OWNER_ARCH,         \
> +                      0x8000)
> +
>  /* Only one error code defined in SMCCC */
>  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>  
> -- 
> 2.11.0
> 

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

[PATCH v2 05/15] xen/arm: Adapt smccc.h to be able to use it in assembly code

[Julien Grall]

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

---
    Changes in v2:
        - Add Volodymyr's reviewed-by
---
 xen/include/asm-arm/smccc.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index b790fac17c..d24ccb51d8 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -25,18 +25,20 @@
  * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
  */
 
-#define ARM_SMCCC_STD_CALL              0U
-#define ARM_SMCCC_FAST_CALL             1U
+#define ARM_SMCCC_STD_CALL              _AC(0,U)
+#define ARM_SMCCC_FAST_CALL             _AC(1,U)
 #define ARM_SMCCC_TYPE_SHIFT            31
 
-#define ARM_SMCCC_CONV_32               0U
-#define ARM_SMCCC_CONV_64               1U
+#define ARM_SMCCC_CONV_32               _AC(0,U)
+#define ARM_SMCCC_CONV_64               _AC(1,U)
 #define ARM_SMCCC_CONV_SHIFT            30
 
-#define ARM_SMCCC_OWNER_MASK            0x3FU
+#define ARM_SMCCC_OWNER_MASK            _AC(0x3F,U)
 #define ARM_SMCCC_OWNER_SHIFT           24
 
-#define ARM_SMCCC_FUNC_MASK             0xFFFFU
+#define ARM_SMCCC_FUNC_MASK             _AC(0xFFFF,U)
+
+#ifndef __ASSEMBLY__
 
 /* Check if this is fast call. */
 static inline bool smccc_is_fast_call(register_t funcid)
@@ -62,6 +64,8 @@ static inline uint32_t smccc_get_owner(register_t funcid)
     return (funcid >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK;
 }
 
+#endif
+
 /*
  * Construct function identifier from call type (fast or standard),
  * calling convention (32 or 64 bit), service owner and function number.
-- 
2.11.0


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

Re: [PATCH v2 05/15] xen/arm: Adapt smccc.h to be able to use it in assembly code

[Stefano Stabellini]

On Thu, 8 Feb 2018, Julien Grall wrote:
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>


> ---
>     Changes in v2:
>         - Add Volodymyr's reviewed-by
> ---
>  xen/include/asm-arm/smccc.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index b790fac17c..d24ccb51d8 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -25,18 +25,20 @@
>   * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>   */
>  
> -#define ARM_SMCCC_STD_CALL              0U
> -#define ARM_SMCCC_FAST_CALL             1U
> +#define ARM_SMCCC_STD_CALL              _AC(0,U)
> +#define ARM_SMCCC_FAST_CALL             _AC(1,U)
>  #define ARM_SMCCC_TYPE_SHIFT            31
>  
> -#define ARM_SMCCC_CONV_32               0U
> -#define ARM_SMCCC_CONV_64               1U
> +#define ARM_SMCCC_CONV_32               _AC(0,U)
> +#define ARM_SMCCC_CONV_64               _AC(1,U)
>  #define ARM_SMCCC_CONV_SHIFT            30
>  
> -#define ARM_SMCCC_OWNER_MASK            0x3FU
> +#define ARM_SMCCC_OWNER_MASK            _AC(0x3F,U)
>  #define ARM_SMCCC_OWNER_SHIFT           24
>  
> -#define ARM_SMCCC_FUNC_MASK             0xFFFFU
> +#define ARM_SMCCC_FUNC_MASK             _AC(0xFFFF,U)
> +
> +#ifndef __ASSEMBLY__
>  
>  /* Check if this is fast call. */
>  static inline bool smccc_is_fast_call(register_t funcid)
> @@ -62,6 +64,8 @@ static inline uint32_t smccc_get_owner(register_t funcid)
>      return (funcid >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK;
>  }
>  
> +#endif
> +
>  /*
>   * Construct function identifier from call type (fast or standard),
>   * calling convention (32 or 64 bit), service owner and function number.
> -- 
> 2.11.0
> 

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

[PATCH v2 06/15] xen/arm64: Implement a fast path for handling SMCCC_ARCH_WORKAROUND_1

[Julien Grall]

The function SMCCC_ARCH_WORKAROUND_1 will be called by the guest for
hardening the branch predictor. So we want the handling to be as fast as
possible.

As the mitigation is applied on every guest exit, we can check for the
call before saving all the context and return very early.

For now, only provide a fast path for HVC64 call. Because the code rely
on 2 registers, x0 and x1 are saved in advance.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

---
    guest_sync only handle 64-bit guest, so I have only implemented the
    64-bit side for now. We can discuss whether it is useful to
    implement it for 32-bit guests.

    We could also consider to implement the fast path for SMC64,
    althought a guest should always use HVC.

    Changes in v2:
        - Add Volodymyr's reviewed-by
---
 xen/arch/arm/arm64/entry.S      | 56 +++++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/processor.h |  2 ++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 6d99e46f0f..67f96d518f 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -1,6 +1,7 @@
 #include <asm/asm_defns.h>
 #include <asm/regs.h>
 #include <asm/alternative.h>
+#include <asm/smccc.h>
 #include <public/xen.h>
 
 /*
@@ -90,8 +91,12 @@ lr      .req    x30             /* link register */
         .endm
 /*
  * Save state on entry to hypervisor, restore on exit
+ *
+ * save_x0_x1: Does the macro needs to save x0/x1 (default 1). If 0,
+ * we rely on the on x0/x1 to have been saved at the correct position on
+ * the stack before.
  */
-        .macro  entry, hyp, compat
+        .macro  entry, hyp, compat, save_x0_x1=1
         sub     sp, sp, #(UREGS_SPSR_el1 - UREGS_LR) /* CPSR, PC, SP, LR */
         push    x28, x29
         push    x26, x27
@@ -107,7 +112,16 @@ lr      .req    x30             /* link register */
         push    x6, x7
         push    x4, x5
         push    x2, x3
+        /*
+         * The caller may already have saved x0/x1 on the stack at the
+         * correct address and corrupt them with another value. Only
+         * save them if save_x0_x1 == 1.
+         */
+        .if \save_x0_x1 == 1
         push    x0, x1
+        .else
+        sub     sp, sp, #16
+        .endif
 
         .if \hyp == 1        /* Hypervisor mode */
 
@@ -200,7 +214,45 @@ hyp_irq:
         exit    hyp=1
 
 guest_sync:
-        entry   hyp=0, compat=0
+        /*
+         * Save x0, x1 in advance
+         */
+        stp     x0, x1, [sp, #-(UREGS_kernel_sizeof - UREGS_X0)]
+
+        /*
+         * x1 is used because x0 may contain the function identifier.
+         * This avoids to restore x0 from the stack.
+         */
+        mrs     x1, esr_el2
+        lsr     x1, x1, #HSR_EC_SHIFT           /* x1 = ESR_EL2.EC */
+        cmp     x1, #HSR_EC_HVC64
+        b.ne    1f                              /* Not a HVC skip fastpath. */
+
+        mrs     x1, esr_el2
+        and     x1, x1, #0xffff                 /* Check the immediate [0:16] */
+        cbnz    x1, 1f                          /* should be 0 for HVC #0 */
+
+        /*
+         * Fastest path possible for ARM_SMCCC_ARCH_WORKAROUND_1.
+         * The workaround has already been applied on the exception
+         * entry from the guest, so let's quickly get back to the guest.
+         */
+        eor     w0, w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
+        cbnz    w0, 1f
+
+        /*
+         * Clobber both x0 and x1 to prevent leakage. Note that thanks
+         * the eor, x0 = 0.
+         */
+        mov     x1, x0
+        eret
+
+1:
+        /*
+         * x0/x1 may have been scratch by the fast path above, so avoid
+         * to save them.
+         */
+        entry   hyp=0, compat=0, save_x0_x1=0
         /*
          * The vSError will be checked while SKIP_SYNCHRONIZE_SERROR_ENTRY_EXIT
          * is not set. If a vSError took place, the initial exception will be
diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index c0f79d0093..222a02dd99 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -306,6 +306,8 @@
 #define HDCR_TPM        (_AC(1,U)<<6)           /* Trap Performance Monitors accesses */
 #define HDCR_TPMCR      (_AC(1,U)<<5)           /* Trap PMCR accesses */
 
+#define HSR_EC_SHIFT                26
+
 #define HSR_EC_UNKNOWN              0x00
 #define HSR_EC_WFI_WFE              0x01
 #define HSR_EC_CP15_32              0x03
-- 
2.11.0


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

[PATCH v2 07/15] xen/arm64: Print a per-CPU message with the BP hardening method used

[Julien Grall]

This will make easier to know whether BP hardening has been enabled for
a CPU and which method is used.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/cpuerrata.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 9c7458ef06..6704648b26 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -79,7 +79,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start,
 static bool __maybe_unused
 install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
                          const char *hyp_vec_start,
-                         const char *hyp_vec_end)
+                         const char *hyp_vec_end,
+                         const char *desc)
 {
     static int last_slot = -1;
     static DEFINE_SPINLOCK(bp_lock);
@@ -94,6 +95,9 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
     if ( !entry->matches(entry) )
         return true;
 
+    printk(XENLOG_INFO "CPU%u will %s on exception entry\n",
+           smp_processor_id(), desc);
+
     /*
      * No need to install hardened vector when the processor has
      * ID_AA64PRF0_EL1.CSV2 set.
@@ -157,7 +161,8 @@ static int enable_psci_bp_hardening(void *data)
      */
     if ( psci_ver >= PSCI_VERSION(0, 2) )
         ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
-                                       __psci_hyp_bp_inval_end);
+                                       __psci_hyp_bp_inval_end,
+                                       "call PSCI get version");
     else if ( !warned )
     {
         ASSERT(system_state < SYS_STATE_active);
-- 
2.11.0


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

Re: [PATCH v2 07/15] xen/arm64: Print a per-CPU message with the BP hardening method used

[Volodymyr Babchuk]

On 08.02.18 21:21, Julien Grall wrote:
> This will make easier to know whether BP hardening has been enabled for
> a CPU and which method is used.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com> 
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/cpuerrata.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 9c7458ef06..6704648b26 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -79,7 +79,8 @@ static bool copy_hyp_vect_bpi(unsigned int slot, const char *hyp_vec_start,
>   static bool __maybe_unused
>   install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
>                            const char *hyp_vec_start,
> -                         const char *hyp_vec_end)
> +                         const char *hyp_vec_end,
> +                         const char *desc)
>   {
>       static int last_slot = -1;
>       static DEFINE_SPINLOCK(bp_lock);
> @@ -94,6 +95,9 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
>       if ( !entry->matches(entry) )
>           return true;
>   
> +    printk(XENLOG_INFO "CPU%u will %s on exception entry\n",
> +           smp_processor_id(), desc);
> +
>       /*
>        * No need to install hardened vector when the processor has
>        * ID_AA64PRF0_EL1.CSV2 set.
> @@ -157,7 +161,8 @@ static int enable_psci_bp_hardening(void *data)
>        */
>       if ( psci_ver >= PSCI_VERSION(0, 2) )
>           ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
> -                                       __psci_hyp_bp_inval_end);
> +                                       __psci_hyp_bp_inval_end,
> +                                       "call PSCI get version");
>       else if ( !warned )
>       {
>           ASSERT(system_state < SYS_STATE_active);
> 

-- 
Volodymyr Babchuk

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

[PATCH v2 08/15] xen/arm: smccc: Add macros SMCCC_VERSION, SMCCC_VERSION_{MINOR, MAJOR}

[Julien Grall]

Add macros SMCCC_VERSION, SMCCC_VERSION_{MINOR, MAJOR} to easily convert
between a 32-bit value and a version number. The encoding is based on
2.2.2 in "Firmware interfaces for mitigation CVE-2017-5715" (ARM DEN 0070A).

Also re-use them to define ARM_SMCCC_VERSION_1_0 and ARM_SMCCC_VERSION_1_1.

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

---
    Changes in v2:
        - Patch added
---
 xen/include/asm-arm/smccc.h | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index d24ccb51d8..caa2c9cc1b 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -16,8 +16,20 @@
 #ifndef __ASM_ARM_SMCCC_H__
 #define __ASM_ARM_SMCCC_H__
 
-#define ARM_SMCCC_VERSION_1_0   0x10000
-#define ARM_SMCCC_VERSION_1_1   0x10001
+#define SMCCC_VERSION_MAJOR_SHIFT            16
+#define SMCCC_VERSION_MINOR_MASK             \
+        ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
+#define SMCCC_VERSION_MAJOR_MASK             ~SMCCC_VERSION_MINOR_MASK
+#define SMCCC_VERSION_MAJOR(ver)             \
+        (((ver) & SMCCC_VERSION_MAJOR_MASK) >> SMCCC_VERSION_MAJOR_SHIFT)
+#define SMCCC_VERSION_MINOR(ver)             \
+        ((ver) & SMCCC_VERSION_MINOR_MASK)
+
+#define SMCCC_VERSION(major, minor)          \
+    (((major) << SMCCC_VERSION_MAJOR_SHIFT) | (minor))
+
+#define ARM_SMCCC_VERSION_1_0   SMCCC_VERSION(1, 0)
+#define ARM_SMCCC_VERSION_1_1   SMCCC_VERSION(1, 1)
 
 /*
  * This file provides common defines for ARM SMC Calling Convention as
-- 
2.11.0


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

Re: [PATCH v2 08/15] xen/arm: smccc: Add macros SMCCC_VERSION, SMCCC_VERSION_{MINOR, MAJOR}

[Volodymyr Babchuk]

On 08.02.18 21:21, Julien Grall wrote:
> Add macros SMCCC_VERSION, SMCCC_VERSION_{MINOR, MAJOR} to easily convert
> between a 32-bit value and a version number. The encoding is based on
> 2.2.2 in "Firmware interfaces for mitigation CVE-2017-5715" (ARM DEN 0070A).
> 
> Also re-use them to define ARM_SMCCC_VERSION_1_0 and ARM_SMCCC_VERSION_1_1.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>      Changes in v2:
>          - Patch added
> ---
>   xen/include/asm-arm/smccc.h | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index d24ccb51d8..caa2c9cc1b 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -16,8 +16,20 @@
>   #ifndef __ASM_ARM_SMCCC_H__
>   #define __ASM_ARM_SMCCC_H__
>   
> -#define ARM_SMCCC_VERSION_1_0   0x10000
> -#define ARM_SMCCC_VERSION_1_1   0x10001
> +#define SMCCC_VERSION_MAJOR_SHIFT            16
> +#define SMCCC_VERSION_MINOR_MASK             \
> +        ((1U << SMCCC_VERSION_MAJOR_SHIFT) - 1)
> +#define SMCCC_VERSION_MAJOR_MASK             ~SMCCC_VERSION_MINOR_MASK
> +#define SMCCC_VERSION_MAJOR(ver)             \
> +        (((ver) & SMCCC_VERSION_MAJOR_MASK) >> SMCCC_VERSION_MAJOR_SHIFT)
> +#define SMCCC_VERSION_MINOR(ver)             \
> +        ((ver) & SMCCC_VERSION_MINOR_MASK)
> +
> +#define SMCCC_VERSION(major, minor)          \
> +    (((major) << SMCCC_VERSION_MAJOR_SHIFT) | (minor))
> +
> +#define ARM_SMCCC_VERSION_1_0   SMCCC_VERSION(1, 0)
> +#define ARM_SMCCC_VERSION_1_1   SMCCC_VERSION(1, 1)
>   
>   /*
>    * This file provides common defines for ARM SMC Calling Convention as
> 

-- 
Volodymyr Babchuk

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

[PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Julien Grall]

PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
earlier) and the function return an error, then we considered SMCCC 1.0
is implemented.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
 xen/include/asm-arm/smccc.h |  5 ++++-
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 5dda35cd7c..bc7b2260e8 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -37,6 +37,7 @@
 #endif
 
 uint32_t psci_ver;
+uint32_t smccc_ver;
 
 static uint32_t psci_cpu_on_nr;
 
@@ -57,6 +58,14 @@ void call_psci_system_reset(void)
         call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
 }
 
+static int __init psci_features(uint32_t psci_func_id)
+{
+    if ( psci_ver < PSCI_VERSION(1, 0) )
+        return PSCI_NOT_SUPPORTED;
+
+    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
+}
+
 int __init psci_is_smc_method(const struct dt_device_node *psci)
 {
     int ret;
@@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct dt_device_node *psci)
     return 0;
 }
 
+static void __init psci_init_smccc(void)
+{
+    /* PSCI is using at least SMCC 1.0 calling convention. */
+    smccc_ver = ARM_SMCCC_VERSION_1_0;
+
+    if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
+    {
+        uint32_t ret;
+
+        ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
+        if ( ret != ARM_SMCCC_NOT_SUPPORTED )
+            smccc_ver = ret;
+    }
+
+    printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
+           SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
+}
+
 int __init psci_init_0_1(void)
 {
     int ret;
@@ -173,7 +200,12 @@ int __init psci_init(void)
     if ( ret )
         ret = psci_init_0_1();
 
-    return ret;
+    if ( ret )
+        return ret;
+
+    psci_init_smccc();
+
+    return 0;
 }
 
 /*
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index caa2c9cc1b..bc067892c7 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -52,6 +52,8 @@
 
 #ifndef __ASSEMBLY__
 
+extern uint32_t smccc_ver;
+
 /* Check if this is fast call. */
 static inline bool smccc_is_fast_call(register_t funcid)
 {
@@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t funcid)
                       ARM_SMCCC_OWNER_ARCH,         \
                       0x8000)
 
-/* Only one error code defined in SMCCC */
+/* SMCCC error codes */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+#define ARM_SMCCC_NOT_SUPPORTED         (-1)
 
 /* SMCCC function identifier range which is reserved for existing APIs */
 #define ARM_SMCCC_RESERVED_RANGE_START  0x0
-- 
2.11.0


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

Re: [PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Volodymyr Babchuk]

Julien,


On 08.02.18 21:21, Julien Grall wrote:
> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
> earlier) and the function return an error, then we considered SMCCC 1.0
> is implemented.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> ---
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
>   xen/include/asm-arm/smccc.h |  5 ++++-
>   2 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c

I find it strange to determine SMCCC version in PSCI code. psci.c is not 
the first place, where I will look for SMCCC version discovery.

I think it is better to add smccc.c, where such functions can reside.

> index 5dda35cd7c..bc7b2260e8 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -37,6 +37,7 @@
>   #endif
>   
>   uint32_t psci_ver;
> +uint32_t smccc_ver;

And this variable actually is not related to PSCI.
>   
>   static uint32_t psci_cpu_on_nr;
>   
> @@ -57,6 +58,14 @@ void call_psci_system_reset(void)
>           call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
>   }
>   
> +static int __init psci_features(uint32_t psci_func_id)
> +{
> +    if ( psci_ver < PSCI_VERSION(1, 0) )
> +        return PSCI_NOT_SUPPORTED;
> +
> +    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
> +}
> +
>   int __init psci_is_smc_method(const struct dt_device_node *psci)
>   {
>       int ret;
> @@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct dt_device_node *psci)
>       return 0;
>   }
>   
> +static void __init psci_init_smccc(void)
> +{
> +    /* PSCI is using at least SMCC 1.0 calling convention. */
> +    smccc_ver = ARM_SMCCC_VERSION_1_0;
> +
> +    if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
> +    {
> +        uint32_t ret;
> +
> +        ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
> +        if ( ret != ARM_SMCCC_NOT_SUPPORTED )
> +            smccc_ver = ret;
> +    }
> +
> +    printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
> +           SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
> +}
> +
>   int __init psci_init_0_1(void)
>   {
>       int ret;
> @@ -173,7 +200,12 @@ int __init psci_init(void)
>       if ( ret )
>           ret = psci_init_0_1();
>   
> -    return ret;
> +    if ( ret )
> +        return ret;
> +
> +    psci_init_smccc();
> +
> +    return 0;
>   }
>   
>   /*
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index caa2c9cc1b..bc067892c7 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -52,6 +52,8 @@
>   
>   #ifndef __ASSEMBLY__
>   
> +extern uint32_t smccc_ver;
> +
>   /* Check if this is fast call. */
>   static inline bool smccc_is_fast_call(register_t funcid)
>   {
> @@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t funcid)
>                         ARM_SMCCC_OWNER_ARCH,         \
>                         0x8000)
>   
> -/* Only one error code defined in SMCCC */
> +/* SMCCC error codes */
>   #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> +#define ARM_SMCCC_NOT_SUPPORTED         (-1)

In patch "xen/arm: vsmc: Implement SMCCC 1.1" you return plain -1 in 
static bool handle_arch(struct cpu_user_regs *regs)

Could you please move definition of ARM_SMCCC_NOT_SUPPORTED into that 
patch and use it in mentioned function or add new patch that changes -1 
to ARM_SMCCC_NOT_SUPPORTED ?

>   
>   /* SMCCC function identifier range which is reserved for existing APIs */
>   #define ARM_SMCCC_RESERVED_RANGE_START  0x0
> 

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Julien Grall]

On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:
> Julien,
> 
> 
> On 08.02.18 21:21, Julien Grall wrote:
>> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
>> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
>> earlier) and the function return an error, then we considered SMCCC 1.0
>> is implemented.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
>>   xen/include/asm-arm/smccc.h |  5 ++++-
>>   2 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> 
> I find it strange to determine SMCCC version in PSCI code. psci.c is not 
> the first place, where I will look for SMCCC version discovery.
> I think it is better to add smccc.c, where such functions can reside.

SMCCC version discovery is based on PSCI, hence it is in the PSCI code. 
I can't see a good reason to create a file with 3 lines at the moment.

> 
>> index 5dda35cd7c..bc7b2260e8 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -37,6 +37,7 @@
>>   #endif
>>   uint32_t psci_ver;
>> +uint32_t smccc_ver;
> 
> And this variable actually is not related to PSCI.

See my comment above. I am not going to create a file just for 3 lines.

>>   static uint32_t psci_cpu_on_nr;
>> @@ -57,6 +58,14 @@ void call_psci_system_reset(void)
>>           call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
>>   }
>> +static int __init psci_features(uint32_t psci_func_id)
>> +{
>> +    if ( psci_ver < PSCI_VERSION(1, 0) )
>> +        return PSCI_NOT_SUPPORTED;
>> +
>> +    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
>> +}
>> +
>>   int __init psci_is_smc_method(const struct dt_device_node *psci)
>>   {
>>       int ret;
>> @@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct 
>> dt_device_node *psci)
>>       return 0;
>>   }
>> +static void __init psci_init_smccc(void)
>> +{
>> +    /* PSCI is using at least SMCC 1.0 calling convention. */
>> +    smccc_ver = ARM_SMCCC_VERSION_1_0;
>> +
>> +    if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
>> +    {
>> +        uint32_t ret;
>> +
>> +        ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
>> +        if ( ret != ARM_SMCCC_NOT_SUPPORTED )
>> +            smccc_ver = ret;
>> +    }
>> +
>> +    printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
>> +           SMCCC_VERSION_MAJOR(smccc_ver), 
>> SMCCC_VERSION_MINOR(smccc_ver));
>> +}
>> +
>>   int __init psci_init_0_1(void)
>>   {
>>       int ret;
>> @@ -173,7 +200,12 @@ int __init psci_init(void)
>>       if ( ret )
>>           ret = psci_init_0_1();
>> -    return ret;
>> +    if ( ret )
>> +        return ret;
>> +
>> +    psci_init_smccc();
>> +
>> +    return 0;
>>   }
>>   /*
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index caa2c9cc1b..bc067892c7 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -52,6 +52,8 @@
>>   #ifndef __ASSEMBLY__
>> +extern uint32_t smccc_ver;
>> +
>>   /* Check if this is fast call. */
>>   static inline bool smccc_is_fast_call(register_t funcid)
>>   {
>> @@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t 
>> funcid)
>>                         ARM_SMCCC_OWNER_ARCH,         \
>>                         0x8000)
>> -/* Only one error code defined in SMCCC */
>> +/* SMCCC error codes */
>>   #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>> +#define ARM_SMCCC_NOT_SUPPORTED         (-1)
> 
> In patch "xen/arm: vsmc: Implement SMCCC 1.1" you return plain -1 in 
> static bool handle_arch(struct cpu_user_regs *regs)
> 
> Could you please move definition of ARM_SMCCC_NOT_SUPPORTED into that 
> patch and use it in mentioned function or add new patch that changes -1 
> to ARM_SMCCC_NOT_SUPPORTED ?

Will do.

> 
>>   /* SMCCC function identifier range which is reserved for existing 
>> APIs */
>>   #define ARM_SMCCC_RESERVED_RANGE_START  0x0
>>
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Volodymyr Babchuk]

Hi Julien,

On 09.02.18 19:09, Julien Grall wrote:
> 
> 
> On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:
>> Julien,
>>
>>
>> On 08.02.18 21:21, Julien Grall wrote:
>>> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
>>> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
>>> earlier) and the function return an error, then we considered SMCCC 1.0
>>> is implemented.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>> ---
>>>      Changes in v2:
>>>          - Patch added
>>> ---
>>>   xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
>>>   xen/include/asm-arm/smccc.h |  5 ++++-
>>>   2 files changed, 37 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>
>> I find it strange to determine SMCCC version in PSCI code. psci.c is 
>> not the first place, where I will look for SMCCC version discovery.
>> I think it is better to add smccc.c, where such functions can reside.
> 
> SMCCC version discovery is based on PSCI, hence it is in the PSCI code. 
> I can't see a good reason to create a file with 3 lines at the moment.

SMCCC version discovery is a Arm Architecture Service function. PSCI 
used to discover if this function is supported at all. Dubious 
architectural solution from my point of view. But it is already done...

We had similar discussions about introducing new files earlier, so you 
know  my point. I would like to see clean codebase where one can 
navigate without grep/cscope. I see no point, why function that calls 
Arm architecture service to identify SMCCC version should reside in PSCI 
code.

Besides, that file will have more than 3 lines at the moment. Your 
current psci_init_smccc is longer right now :)

>>
>>> index 5dda35cd7c..bc7b2260e8 100644
>>> --- a/xen/arch/arm/psci.c
>>> +++ b/xen/arch/arm/psci.c
>>> @@ -37,6 +37,7 @@
>>>   #endif
>>>   uint32_t psci_ver;
>>> +uint32_t smccc_ver;
>>
>> And this variable actually is not related to PSCI.
> 
> See my comment above. I am not going to create a file just for 3 lines.
> 

See my comments above :)

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 09/15] xen/arm: psci: Detect SMCCC version

[Julien Grall]

On 12/02/18 14:43, Volodymyr Babchuk wrote:
> Hi Julien,
> 
> On 09.02.18 19:09, Julien Grall wrote:
>>
>>
>> On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:
>>> Julien,
>>>
>>>
>>> On 08.02.18 21:21, Julien Grall wrote:
>>>> PSCI 1.0 and later allows the SMCCC version to be (indirectly) probed
>>>> via PSCI_FEATURES. If the PSCI_FEATURES does not exist (PSCI 0.2 or
>>>> earlier) and the function return an error, then we considered SMCCC 1.0
>>>> is implemented.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>> ---
>>>>      Changes in v2:
>>>>          - Patch added
>>>> ---
>>>>   xen/arch/arm/psci.c         | 34 +++++++++++++++++++++++++++++++++-
>>>>   xen/include/asm-arm/smccc.h |  5 ++++-
>>>>   2 files changed, 37 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>>>
>>> I find it strange to determine SMCCC version in PSCI code. psci.c is 
>>> not the first place, where I will look for SMCCC version discovery.
>>> I think it is better to add smccc.c, where such functions can reside.
>>
>> SMCCC version discovery is based on PSCI, hence it is in the PSCI 
>> code. I can't see a good reason to create a file with 3 lines at the 
>> moment.
> 
> SMCCC version discovery is a Arm Architecture Service function. PSCI 
> used to discover if this function is supported at all. Dubious 
> architectural solution from my point of view. But it is already done...
> 
> We had similar discussions about introducing new files earlier, so you 
> know  my point. I would like to see clean codebase where one can 
> navigate without grep/cscope. I see no point, why function that calls 
> Arm architecture service to identify SMCCC version should reside in PSCI 
> code.

Good luck for navigating without grep/cscope in a such a big code base.

> 
> Besides, that file will have more than 3 lines at the moment. Your 
> current psci_init_smccc is longer right now :)

You got my point with "3 lines"... It is one function in one file. There 
limited reason to create a file for that, more that I clearly don't want 
to expose psci_features() outside of psci.c for now.

We can revisit this in the future if we need to discover SMCCC in a 
different way.

Cheers,

-- 
Julien Grall

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

[PATCH v2 10/15] xen/arm: smccc: Implement SMCCC v1.1 inline primitive

[Julien Grall]

One of the major improvement of SMCCC v1.1 is that it only clobbers the
first 4 registers, both on 32 and 64bit. This means that it becomes very
easy to provide an inline version of the SMC call primitive, and avoid
performing a function call to stash the registers that woudl otherwise
be clobbered by SMCCC v1.0.

This patch has been adapted to Xen from Linux commit f2d3b2e8759a. The
changes mades are:
    - Using Xen coding style
    - Remove HVC as not used by Xen
    - Add arm_smccc_res structure

 Reviewed-by: Robin Murphy <robin.murphy@arm.com>
 Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
 Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
 Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>

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

---

    Note that the patch is in arm64/for-next/core and should be merged
    in master soon.

    Changes in v2:
        - Patch added
---
 xen/include/asm-arm/smccc.h | 119 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 119 insertions(+)

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index bc067892c7..154772b728 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -78,6 +78,125 @@ static inline uint32_t smccc_get_owner(register_t funcid)
     return (funcid >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK;
 }
 
+/*
+ * struct arm_smccc_res - Result from SMC call
+ * @a0 - @a3 result values from registers 0 to 3
+ */
+struct arm_smccc_res {
+    unsigned long a0;
+    unsigned long a1;
+    unsigned long a2;
+    unsigned long a3;
+};
+
+/* SMCCC v1.1 implementation madness follows */
+#define ___count_args(_0, _1, _2, _3, _4, _5, _6, _7, _8, x, ...) x
+
+#define __count_args(...)                               \
+    ___count_args(__VA_ARGS__, 7, 6, 5, 4, 3, 2, 1, 0)
+
+#define __constraint_write_0                        \
+    "+r" (r0), "=&r" (r1), "=&r" (r2), "=&r" (r3)
+#define __constraint_write_1                        \
+    "+r" (r0), "+r" (r1), "=&r" (r2), "=&r" (r3)
+#define __constraint_write_2                        \
+    "+r" (r0), "+r" (r1), "+r" (r2), "=&r" (r3)
+#define __constraint_write_3                        \
+    "+r" (r0), "+r" (r1), "+r" (r2), "+r" (r3)
+#define __constraint_write_4    __constraint_write_3
+#define __constraint_write_5    __constraint_write_4
+#define __constraint_write_6    __constraint_write_5
+#define __constraint_write_7    __constraint_write_6
+
+#define __constraint_read_0
+#define __constraint_read_1
+#define __constraint_read_2
+#define __constraint_read_3
+#define __constraint_read_4 "r" (r4)
+#define __constraint_read_5 __constraint_read_4, "r" (r5)
+#define __constraint_read_6 __constraint_read_5, "r" (r6)
+#define __constraint_read_7 __constraint_read_6, "r" (r7)
+
+#define __declare_arg_0(a0, res)                        \
+    struct arm_smccc_res    *___res = res;              \
+    register uin32_t        r0 asm("r0") = a0;          \
+    register unsigned long  r1 asm("r1");               \
+    register unsigned long  r2 asm("r2");               \
+    register unsigned long  r3 asm("r3")
+
+#define __declare_arg_1(a0, a1, res)                    \
+    struct arm_smccc_res    *___res = res;              \
+    register uint32_t       r0 asm("r0") = a0;          \
+    register typeof(a1)     r1 asm("r1") = a1;          \
+    register unsigned long  r2 asm("r2");               \
+    register unsigned long  r3 asm("r3")
+
+#define __declare_arg_2(a0, a1, a2, res)                \
+    struct arm_smccc_res    *___res = res;				\
+    register u32            r0 asm("r0") = a0;          \
+    register typeof(a1)     r1 asm("r1") = a1;          \
+    register typeof(a2)     r2 asm("r2") = a2;          \
+    register unsigned long  r3 asm("r3")
+
+#define __declare_arg_3(a0, a1, a2, a3, res)            \
+    struct arm_smccc_res    *___res = res;              \
+    register u32            r0 asm("r0") = a0;          \
+    register typeof(a1)     r1 asm("r1") = a1;          \
+    register typeof(a2)     r2 asm("r2") = a2;          \
+    register typeof(a3)     r3 asm("r3") = a3
+
+#define __declare_arg_4(a0, a1, a2, a3, a4, res)        \
+    __declare_arg_3(a0, a1, a2, a3, res);               \
+    register typeof(a4) r4 asm("r4") = a4
+
+#define __declare_arg_5(a0, a1, a2, a3, a4, a5, res)    \
+    __declare_arg_4(a0, a1, a2, a3, a4, res);           \
+    register typeof(a5) r5 asm("r5") = a5
+
+#define __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res)    \
+    __declare_arg_5(a0, a1, a2, a3, a4, a5, res);           \
+    register typeof(a6) r6 asm("r6") = a6
+
+#define __declare_arg_7(a0, a1, a2, a3, a4, a5, a6, a7, res)    \
+    __declare_arg_6(a0, a1, a2, a3, a4, a5, a6, res);           \
+    register typeof(a7) r7 asm("r7") = a7
+
+#define ___declare_args(count, ...) __declare_arg_ ## count(__VA_ARGS__)
+#define __declare_args(count, ...)  ___declare_args(count, __VA_ARGS__)
+
+#define ___constraints(count)                       \
+    : __constraint_write_ ## count                  \
+    : __constraint_read_ ## count                   \
+    : "memory"
+#define __constraints(count)    ___constraints(count)
+
+/*
+ * arm_smccc_1_1_smc() - make an SMCCC v1.1 compliant SMC call
+ *
+ * This is a variadic macro taking one to eight source arguments, and
+ * an optional return structure.
+ *
+ * @a0-a7: arguments passed in registers 0 to 7
+ * @res: result values from registers 0 to 3
+ *
+ * This macro is used to make SMC calls following SMC Calling Convention v1.1.
+ * The content of the supplied param are copied to registers 0 to 7 prior
+ * to the SMC instruction. The return values are updated with the content
+ * from register 0 to 3 on return from the SMC instruction if not NULL.
+ *
+ * We have an output list that is not necessarily used, and GCC feels
+ * entitled to optimise the whole sequence away. "volatile" is what
+ * makes it stick.
+ */
+#define arm_smccc_1_1_smc(...)                                  \
+    do {                                                        \
+        __declare_args(__count_args(__VA_ARGS__), __VA_ARGS__); \
+        asm volatile("smc #0\n"                                 \
+                     __constraints(__count_args(__VA_ARGS__))); \
+        if ( ___res )                                           \
+        *___res = (typeof(*___res)){r0, r1, r2, r3};            \
+    } while ( 0 )
+
 #endif
 
 /*
-- 
2.11.0


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

[PATCH v2 11/15] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Julien Grall]

Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm64/bpi.S    | 12 ++++++++++++
 xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
 xen/include/asm-arm/smccc.h |  1 +
 3 files changed, 44 insertions(+), 1 deletion(-)

diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
index 4b7f1dc21f..ef237de7bd 100644
--- a/xen/arch/arm/arm64/bpi.S
+++ b/xen/arch/arm/arm64/bpi.S
@@ -16,6 +16,8 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
+#include <asm/smccc.h>
+
 .macro ventry target
     .rept 31
     nop
@@ -81,6 +83,16 @@ ENTRY(__psci_hyp_bp_inval_start)
     add     sp, sp, #(8 * 18)
 ENTRY(__psci_hyp_bp_inval_end)
 
+ENTRY(__smccc_workaround_1_smc_start)
+    sub     sp, sp, #(8 * 4)
+    stp     x2, x3, [sp, #(8 * 0)]
+    stp     x0, x1, [sp, #(8 * 2)]
+    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
+    ldp     x2, x3, [sp, #(8 * 0)]
+    ldp     x0, x1, [sp, #(8 * 2)]
+    add     sp, sp, #(8 * 4)
+ENTRY(__smccc_workaround_1_smc_end)
+
 /*
  * Local variables:
  * mode: ASM
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 6704648b26..6557577bcb 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -147,6 +147,34 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
     return ret;
 }
 
+extern char __smccc_workaround_1_smc_start[], __smccc_workaround_1_smc_end[];
+
+static bool
+check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
+{
+    struct arm_smccc_res res;
+
+    /*
+     * Enable callbacks are called on every CPU based on the
+     * capabilities. So double-check whether the CPU matches the
+     * entry.
+     */
+    if ( !entry->matches(entry) )
+        return false;
+
+    if ( smccc_ver < SMCCC_VERSION(1, 1) )
+        return false;
+
+    arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
+                      ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
+    if ( res.a0 != ARM_SMCCC_SUCCESS )
+        return false;
+
+    return install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
+                                    __smccc_workaround_1_smc_end,
+                                    "call ARM_SMCCC_ARCH_WORKAROUND_1");
+}
+
 extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
 
 static int enable_psci_bp_hardening(void *data)
@@ -154,12 +182,14 @@ static int enable_psci_bp_hardening(void *data)
     bool ret = true;
     static bool warned = false;
 
+    if ( check_smccc_arch_workaround_1(data) )
+        return 0;
     /*
      * The mitigation is using PSCI version function to invalidate the
      * branch predictor. This function is only available with PSCI 0.2
      * and later.
      */
-    if ( psci_ver >= PSCI_VERSION(0, 2) )
+    else if ( psci_ver >= PSCI_VERSION(0, 2) )
         ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
                                        __psci_hyp_bp_inval_end,
                                        "call PSCI get version");
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 154772b728..8342cc33fe 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -261,6 +261,7 @@ struct arm_smccc_res {
 /* SMCCC error codes */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
 #define ARM_SMCCC_NOT_SUPPORTED         (-1)
+#define ARM_SMCCC_SUCCESS               (0)
 
 /* SMCCC function identifier range which is reserved for existing APIs */
 #define ARM_SMCCC_RESERVED_RANGE_START  0x0
-- 
2.11.0


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

Re: [PATCH v2 11/15] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Volodymyr Babchuk]

Hi Julien,

On 08.02.18 21:21, Julien Grall wrote:
> Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
> 
> ---
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/arm64/bpi.S    | 12 ++++++++++++
>   xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
>   xen/include/asm-arm/smccc.h |  1 +
>   3 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
> index 4b7f1dc21f..ef237de7bd 100644
> --- a/xen/arch/arm/arm64/bpi.S
> +++ b/xen/arch/arm/arm64/bpi.S
> @@ -16,6 +16,8 @@
>    * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>    */
>   
> +#include <asm/smccc.h>
> +
>   .macro ventry target
>       .rept 31
>       nop
> @@ -81,6 +83,16 @@ ENTRY(__psci_hyp_bp_inval_start)
>       add     sp, sp, #(8 * 18)
>   ENTRY(__psci_hyp_bp_inval_end)
>   
> +ENTRY(__smccc_workaround_1_smc_start)
> +    sub     sp, sp, #(8 * 4)
> +    stp     x2, x3, [sp, #(8 * 0)]
> +    stp     x0, x1, [sp, #(8 * 2)]
> +    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
> +    ldp     x2, x3, [sp, #(8 * 0)]
> +    ldp     x0, x1, [sp, #(8 * 2)]
> +    add     sp, sp, #(8 * 4)
> +ENTRY(__smccc_workaround_1_smc_end)
> +

This code confuses me. You allocate 32 bytes on stack, save x0-x4 there, 
then you load ARM_SMCCC_ARCH_WORKAROUND_1_FID into w0 and restore values 
of x0-x4, overwriting value written into w0. Am I missing something?

Btw, you can use something like stp	x0, x1, [sp, #-16]! to avoid manual 
adjustment of sp. This will save you two instructions.

>   /*
>    * Local variables:
>    * mode: ASM
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 6704648b26..6557577bcb 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -147,6 +147,34 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
>       return ret;
>   }
>   
> +extern char __smccc_workaround_1_smc_start[], __smccc_workaround_1_smc_end[];
> +
> +static bool
> +check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
> +{
> +    struct arm_smccc_res res;
> +
> +    /*
> +     * Enable callbacks are called on every CPU based on the
> +     * capabilities. So double-check whether the CPU matches the
> +     * entry.
> +     */
> +    if ( !entry->matches(entry) )
> +        return false;
> +
> +    if ( smccc_ver < SMCCC_VERSION(1, 1) )
> +        return false;
> +
> +    arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
> +                      ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
> +    if ( res.a0 != ARM_SMCCC_SUCCESS )
> +        return false;
> +
> +    return install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
> +                                    __smccc_workaround_1_smc_end,
> +                                    "call ARM_SMCCC_ARCH_WORKAROUND_1");
> +}
> +
>   extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
>   
>   static int enable_psci_bp_hardening(void *data)
> @@ -154,12 +182,14 @@ static int enable_psci_bp_hardening(void *data)
>       bool ret = true;
>       static bool warned = false;
>   
> +    if ( check_smccc_arch_workaround_1(data) )
> +        return 0;
>       /*
>        * The mitigation is using PSCI version function to invalidate the
>        * branch predictor. This function is only available with PSCI 0.2
>        * and later.
>        */
> -    if ( psci_ver >= PSCI_VERSION(0, 2) )
> +    else if ( psci_ver >= PSCI_VERSION(0, 2) )
>           ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
>                                          __psci_hyp_bp_inval_end,
>                                          "call PSCI get version");
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 154772b728..8342cc33fe 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -261,6 +261,7 @@ struct arm_smccc_res {
>   /* SMCCC error codes */
>   #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>   #define ARM_SMCCC_NOT_SUPPORTED         (-1)
> +#define ARM_SMCCC_SUCCESS               (0)
>   
>   /* SMCCC function identifier range which is reserved for existing APIs */
>   #define ARM_SMCCC_RESERVED_RANGE_START  0x0
> 

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 11/15] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Julien Grall]

On 12/02/18 16:55, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

> On 08.02.18 21:21, Julien Grall wrote:
>> Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.
>>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>
>> ---
>>      Changes in v2:
>>          - Patch added
>> ---
>>   xen/arch/arm/arm64/bpi.S    | 12 ++++++++++++
>>   xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
>>   xen/include/asm-arm/smccc.h |  1 +
>>   3 files changed, 44 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
>> index 4b7f1dc21f..ef237de7bd 100644
>> --- a/xen/arch/arm/arm64/bpi.S
>> +++ b/xen/arch/arm/arm64/bpi.S
>> @@ -16,6 +16,8 @@
>>    * along with this program.  If not, see 
>> <http://www.gnu.org/licenses/>.
>>    */
>> +#include <asm/smccc.h>
>> +
>>   .macro ventry target
>>       .rept 31
>>       nop
>> @@ -81,6 +83,16 @@ ENTRY(__psci_hyp_bp_inval_start)
>>       add     sp, sp, #(8 * 18)
>>   ENTRY(__psci_hyp_bp_inval_end)
>> +ENTRY(__smccc_workaround_1_smc_start)
>> +    sub     sp, sp, #(8 * 4)
>> +    stp     x2, x3, [sp, #(8 * 0)]
>> +    stp     x0, x1, [sp, #(8 * 2)]
>> +    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
>> +    ldp     x2, x3, [sp, #(8 * 0)]
>> +    ldp     x0, x1, [sp, #(8 * 2)]
>> +    add     sp, sp, #(8 * 4)
>> +ENTRY(__smccc_workaround_1_smc_end)
>> +
> 
> This code confuses me. You allocate 32 bytes on stack, save x0-x4 there, 
> then you load ARM_SMCCC_ARCH_WORKAROUND_1_FID into w0 and restore values 
> of x0-x4, overwriting value written into w0. Am I missing something?

The call to ARM_SMCCC_ARCH_WORKAROUND_1 does not return any value. Even 
if it were, this code is executed on exception entry before jumping into 
the trap helper. So you want to restore all the registers saved.

> 
> Btw, you can use something like stp    x0, x1, [sp, #-16]! to avoid 
> manual adjustment of sp. This will save you two instructions.

It was pointed out on Linux Arm that updating sp once *might* be faster 
on some uarch.

> 
>>   /*
>>    * Local variables:
>>    * mode: ASM
>> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
>> index 6704648b26..6557577bcb 100644
>> --- a/xen/arch/arm/cpuerrata.c
>> +++ b/xen/arch/arm/cpuerrata.c
>> @@ -147,6 +147,34 @@ install_bp_hardening_vec(const struct 
>> arm_cpu_capabilities *entry,
>>       return ret;
>>   }
>> +extern char __smccc_workaround_1_smc_start[], 
>> __smccc_workaround_1_smc_end[];
>> +
>> +static bool
>> +check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
>> +{
>> +    struct arm_smccc_res res;
>> +
>> +    /*
>> +     * Enable callbacks are called on every CPU based on the
>> +     * capabilities. So double-check whether the CPU matches the
>> +     * entry.
>> +     */
>> +    if ( !entry->matches(entry) )
>> +        return false;
>> +
>> +    if ( smccc_ver < SMCCC_VERSION(1, 1) )
>> +        return false;
>> +
>> +    arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
>> +                      ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
>> +    if ( res.a0 != ARM_SMCCC_SUCCESS )
>> +        return false;
>> +
>> +    return 
>> install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
>> +                                    __smccc_workaround_1_smc_end,
>> +                                    "call ARM_SMCCC_ARCH_WORKAROUND_1");
>> +}
>> +
>>   extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
>>   static int enable_psci_bp_hardening(void *data)
>> @@ -154,12 +182,14 @@ static int enable_psci_bp_hardening(void *data)
>>       bool ret = true;
>>       static bool warned = false;
>> +    if ( check_smccc_arch_workaround_1(data) )
>> +        return 0;
>>       /*
>>        * The mitigation is using PSCI version function to invalidate the
>>        * branch predictor. This function is only available with PSCI 0.2
>>        * and later.
>>        */
>> -    if ( psci_ver >= PSCI_VERSION(0, 2) )
>> +    else if ( psci_ver >= PSCI_VERSION(0, 2) )
>>           ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
>>                                          __psci_hyp_bp_inval_end,
>>                                          "call PSCI get version");
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index 154772b728..8342cc33fe 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -261,6 +261,7 @@ struct arm_smccc_res {
>>   /* SMCCC error codes */
>>   #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>>   #define ARM_SMCCC_NOT_SUPPORTED         (-1)
>> +#define ARM_SMCCC_SUCCESS               (0)
>>   /* SMCCC function identifier range which is reserved for existing 
>> APIs */
>>   #define ARM_SMCCC_RESERVED_RANGE_START  0x0
>>
> 

Cheers,

-- 
Julien Grall

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

Re: [PATCH v2 11/15] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Volodymyr Babchuk]

Julien,

On 12.02.18 19:12, Julien Grall wrote:
> On 12/02/18 16:55, Volodymyr Babchuk wrote:
>> Hi Julien,
> 
> Hi Volodymyr,
> 
>> On 08.02.18 21:21, Julien Grall wrote:
>>> Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.
>>>
>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>
>>> ---
>>>      Changes in v2:
>>>          - Patch added
>>> ---
>>>   xen/arch/arm/arm64/bpi.S    | 12 ++++++++++++
>>>   xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
>>>   xen/include/asm-arm/smccc.h |  1 +
>>>   3 files changed, 44 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
>>> index 4b7f1dc21f..ef237de7bd 100644
>>> --- a/xen/arch/arm/arm64/bpi.S
>>> +++ b/xen/arch/arm/arm64/bpi.S
>>> @@ -16,6 +16,8 @@
>>>    * along with this program.  If not, see 
>>> <http://www.gnu.org/licenses/>.
>>>    */
>>> +#include <asm/smccc.h>
>>> +
>>>   .macro ventry target
>>>       .rept 31
>>>       nop
>>> @@ -81,6 +83,16 @@ ENTRY(__psci_hyp_bp_inval_start)
>>>       add     sp, sp, #(8 * 18)
>>>   ENTRY(__psci_hyp_bp_inval_end)
>>> +ENTRY(__smccc_workaround_1_smc_start)
>>> +    sub     sp, sp, #(8 * 4)
>>> +    stp     x2, x3, [sp, #(8 * 0)]
>>> +    stp     x0, x1, [sp, #(8 * 2)]
>>> +    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
>>> +    ldp     x2, x3, [sp, #(8 * 0)]
>>> +    ldp     x0, x1, [sp, #(8 * 2)]
>>> +    add     sp, sp, #(8 * 4)
>>> +ENTRY(__smccc_workaround_1_smc_end)
>>> +
>>
>> This code confuses me. You allocate 32 bytes on stack, save x0-x4 
>> there, then you load ARM_SMCCC_ARCH_WORKAROUND_1_FID into w0 and 
>> restore values of x0-x4, overwriting value written into w0. Am I 
>> missing something?
> 
> The call to ARM_SMCCC_ARCH_WORKAROUND_1 does not return any value. Even 
> if it were, this code is executed on exception entry before jumping into 
> the trap helper. So you want to restore all the registers saved.
> 
I believe you missed smc instruction in the code above.

>>
>> Btw, you can use something like stp    x0, x1, [sp, #-16]! to avoid 
>> manual adjustment of sp. This will save you two instructions.
> 
> It was pointed out on Linux Arm that updating sp once *might* be faster 
> on some uarch.

So is this code is targeted for that some specific uarch? Then I would 
like to see a comment describing why you choose this approach.

[...]

-- 
Volodymyr Babchuk

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

Re: [PATCH v2 11/15] xen/arm64: Add ARM_SMCCC_ARCH_WORKAROUND_1 BP hardening support

[Julien Grall]

On 12/02/18 17:20, Volodymyr Babchuk wrote:
> Julien,

Hi,

> On 12.02.18 19:12, Julien Grall wrote:
>> On 12/02/18 16:55, Volodymyr Babchuk wrote:
>>> Hi Julien,
>>
>> Hi Volodymyr,
>>
>>> On 08.02.18 21:21, Julien Grall wrote:
>>>> Add the detection and runtime code for ARM_SMCCC_ARCH_WORKAROUND_1.
>>>>
>>>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>>>>
>>>> ---
>>>>      Changes in v2:
>>>>          - Patch added
>>>> ---
>>>>   xen/arch/arm/arm64/bpi.S    | 12 ++++++++++++
>>>>   xen/arch/arm/cpuerrata.c    | 32 +++++++++++++++++++++++++++++++-
>>>>   xen/include/asm-arm/smccc.h |  1 +
>>>>   3 files changed, 44 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
>>>> index 4b7f1dc21f..ef237de7bd 100644
>>>> --- a/xen/arch/arm/arm64/bpi.S
>>>> +++ b/xen/arch/arm/arm64/bpi.S
>>>> @@ -16,6 +16,8 @@
>>>>    * along with this program.  If not, see 
>>>> <http://www.gnu.org/licenses/>.
>>>>    */
>>>> +#include <asm/smccc.h>
>>>> +
>>>>   .macro ventry target
>>>>       .rept 31
>>>>       nop
>>>> @@ -81,6 +83,16 @@ ENTRY(__psci_hyp_bp_inval_start)
>>>>       add     sp, sp, #(8 * 18)
>>>>   ENTRY(__psci_hyp_bp_inval_end)
>>>> +ENTRY(__smccc_workaround_1_smc_start)
>>>> +    sub     sp, sp, #(8 * 4)
>>>> +    stp     x2, x3, [sp, #(8 * 0)]
>>>> +    stp     x0, x1, [sp, #(8 * 2)]
>>>> +    mov     w0, #ARM_SMCCC_ARCH_WORKAROUND_1_FID
>>>> +    ldp     x2, x3, [sp, #(8 * 0)]
>>>> +    ldp     x0, x1, [sp, #(8 * 2)]
>>>> +    add     sp, sp, #(8 * 4)
>>>> +ENTRY(__smccc_workaround_1_smc_end)
>>>> +
>>>
>>> This code confuses me. You allocate 32 bytes on stack, save x0-x4 
>>> there, then you load ARM_SMCCC_ARCH_WORKAROUND_1_FID into w0 and 
>>> restore values of x0-x4, overwriting value written into w0. Am I 
>>> missing something?
>>
>> The call to ARM_SMCCC_ARCH_WORKAROUND_1 does not return any value. 
>> Even if it were, this code is executed on exception entry before 
>> jumping into the trap helper. So you want to restore all the registers 
>> saved.
>>
> I believe you missed smc instruction in the code above.

Whoops yes. I will fix it.

> 
>>>
>>> Btw, you can use something like stp    x0, x1, [sp, #-16]! to avoid 
>>> manual adjustment of sp. This will save you two instructions.
>>
>> It was pointed out on Linux Arm that updating sp once *might* be 
>> faster on some uarch.
> 
> So is this code is targeted for that some specific uarch? Then I would 
> like to see a comment describing why you choose this approach.

I can't confirm whether this will improve uarch A, B, C or Z. I just 
followed suggestion on Linux Arm (see [1]) and a personal choice on how 
to write assembly code. It is quite similar that why would I choose the 
other way around?

Cheers,

[1] https://www.spinics.net/lists/arm-kernel/msg626659.html

-- 
Julien Grall

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

[PATCH v2 12/15] xen/arm64: Kill PSCI_GET_VERSION as a variant-2 workaround

[Julien Grall]

Now that we've standardised on SMCCC v1.1 to perform the branch
prediction invalidation, let's drop the previous band-aid. If vendors
haven't updated their firmware to do SMCCC 1.1, they haven't updated
PSCI either, so we don't loose anything.

This is aligned with the Linux commit 3a0a397ff5ff.

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

---
    Note that the patch is in arm64/for-next/core and should be merged
    in master soon.

    Changes in v2:
        - Patch added
---
 xen/arch/arm/arm64/bpi.S | 25 ----------------------
 xen/arch/arm/cpuerrata.c | 54 +++++++++++++++++-------------------------------
 2 files changed, 19 insertions(+), 60 deletions(-)

diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
index ef237de7bd..6270b10c4f 100644
--- a/xen/arch/arm/arm64/bpi.S
+++ b/xen/arch/arm/arm64/bpi.S
@@ -58,31 +58,6 @@ ENTRY(__bp_harden_hyp_vecs_start)
     .endr
 ENTRY(__bp_harden_hyp_vecs_end)
 
-ENTRY(__psci_hyp_bp_inval_start)
-    sub     sp, sp, #(8 * 18)
-    stp     x16, x17, [sp, #(16 * 0)]
-    stp     x14, x15, [sp, #(16 * 1)]
-    stp     x12, x13, [sp, #(16 * 2)]
-    stp     x10, x11, [sp, #(16 * 3)]
-    stp     x8, x9, [sp, #(16 * 4)]
-    stp     x6, x7, [sp, #(16 * 5)]
-    stp     x4, x5, [sp, #(16 * 6)]
-    stp     x2, x3, [sp, #(16 * 7)]
-    stp     x0, x1, [sp, #(16 * 8)]
-    mov     x0, #0x84000000
-    smc     #0
-    ldp     x16, x17, [sp, #(16 * 0)]
-    ldp     x14, x15, [sp, #(16 * 1)]
-    ldp     x12, x13, [sp, #(16 * 2)]
-    ldp     x10, x11, [sp, #(16 * 3)]
-    ldp     x8, x9, [sp, #(16 * 4)]
-    ldp     x6, x7, [sp, #(16 * 5)]
-    ldp     x4, x5, [sp, #(16 * 6)]
-    ldp     x2, x3, [sp, #(16 * 7)]
-    ldp     x0, x1, [sp, #(16 * 8)]
-    add     sp, sp, #(8 * 18)
-ENTRY(__psci_hyp_bp_inval_end)
-
 ENTRY(__smccc_workaround_1_smc_start)
     sub     sp, sp, #(8 * 4)
     stp     x2, x3, [sp, #(8 * 0)]
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 6557577bcb..af453710e4 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -149,10 +149,11 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
 
 extern char __smccc_workaround_1_smc_start[], __smccc_workaround_1_smc_end[];
 
-static bool
-check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
+static int enable_smccc_arch_workaround_1(void *data)
 {
     struct arm_smccc_res res;
+    static bool warned = false;
+    const struct arm_cpu_capabilities *entry = data;
 
     /*
      * Enable callbacks are called on every CPU based on the
@@ -160,47 +161,30 @@ check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
      * entry.
      */
     if ( !entry->matches(entry) )
-        return false;
+        return 0;
 
     if ( smccc_ver < SMCCC_VERSION(1, 1) )
-        return false;
+        goto warn;
 
     arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
                       ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
     if ( res.a0 != ARM_SMCCC_SUCCESS )
-        return false;
-
-    return install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
-                                    __smccc_workaround_1_smc_end,
-                                    "call ARM_SMCCC_ARCH_WORKAROUND_1");
-}
+        goto warn;
 
-extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
+    return !install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
+                                     __smccc_workaround_1_smc_end,
+                                     "call ARM_SMCCC_ARCH_WORKAROUND_1");
 
-static int enable_psci_bp_hardening(void *data)
-{
-    bool ret = true;
-    static bool warned = false;
-
-    if ( check_smccc_arch_workaround_1(data) )
-        return 0;
-    /*
-     * The mitigation is using PSCI version function to invalidate the
-     * branch predictor. This function is only available with PSCI 0.2
-     * and later.
-     */
-    else if ( psci_ver >= PSCI_VERSION(0, 2) )
-        ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
-                                       __psci_hyp_bp_inval_end,
-                                       "call PSCI get version");
-    else if ( !warned )
+warn:
+    if ( !warned )
     {
         ASSERT(system_state < SYS_STATE_active);
-        warning_add("PSCI 0.2 or later is required for the branch predictor hardening.\n");
-        warned = true;
+        warning_add("No support for ARM_SMCCC_ARCH_WORKAROUND_1.\n"
+                    "Please update your firmware.\n");
+        warned = false;
     }
 
-    return !ret;
+    return 0;
 }
 
 #endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
@@ -316,22 +300,22 @@ static const struct arm_cpu_capabilities arm_errata[] = {
     {
         .capability = ARM_HARDEN_BRANCH_PREDICTOR,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A57),
-        .enable = enable_psci_bp_hardening,
+        .enable = enable_smccc_arch_workaround_1,
     },
     {
         .capability = ARM_HARDEN_BRANCH_PREDICTOR,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A72),
-        .enable = enable_psci_bp_hardening,
+        .enable = enable_smccc_arch_workaround_1,
     },
     {
         .capability = ARM_HARDEN_BRANCH_PREDICTOR,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
-        .enable = enable_psci_bp_hardening,
+        .enable = enable_smccc_arch_workaround_1,
     },
     {
         .capability = ARM_HARDEN_BRANCH_PREDICTOR,
         MIDR_ALL_VERSIONS(MIDR_CORTEX_A75),
-        .enable = enable_psci_bp_hardening,
+        .enable = enable_smccc_arch_workaround_1,
     },
 #endif
 #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
-- 
2.11.0


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

Re: [PATCH v2 12/15] xen/arm64: Kill PSCI_GET_VERSION as a variant-2 workaround

[Volodymyr Babchuk]

Hi Julien,

On 08.02.18 21:22, Julien Grall wrote:
> Now that we've standardised on SMCCC v1.1 to perform the branch
> prediction invalidation, let's drop the previous band-aid. If vendors
> haven't updated their firmware to do SMCCC 1.1, they haven't updated
> PSCI either, so we don't loose anything.
> 
> This is aligned with the Linux commit 3a0a397ff5ff.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> 
> ---
>      Note that the patch is in arm64/for-next/core and should be merged
>      in master soon.
> 
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/arm64/bpi.S | 25 ----------------------
>   xen/arch/arm/cpuerrata.c | 54 +++++++++++++++++-------------------------------
>   2 files changed, 19 insertions(+), 60 deletions(-)
> 
> diff --git a/xen/arch/arm/arm64/bpi.S b/xen/arch/arm/arm64/bpi.S
> index ef237de7bd..6270b10c4f 100644
> --- a/xen/arch/arm/arm64/bpi.S
> +++ b/xen/arch/arm/arm64/bpi.S
> @@ -58,31 +58,6 @@ ENTRY(__bp_harden_hyp_vecs_start)
>       .endr
>   ENTRY(__bp_harden_hyp_vecs_end)
>   
> -ENTRY(__psci_hyp_bp_inval_start)
> -    sub     sp, sp, #(8 * 18)
> -    stp     x16, x17, [sp, #(16 * 0)]
> -    stp     x14, x15, [sp, #(16 * 1)]
> -    stp     x12, x13, [sp, #(16 * 2)]
> -    stp     x10, x11, [sp, #(16 * 3)]
> -    stp     x8, x9, [sp, #(16 * 4)]
> -    stp     x6, x7, [sp, #(16 * 5)]
> -    stp     x4, x5, [sp, #(16 * 6)]
> -    stp     x2, x3, [sp, #(16 * 7)]
> -    stp     x0, x1, [sp, #(16 * 8)]
> -    mov     x0, #0x84000000
> -    smc     #0
> -    ldp     x16, x17, [sp, #(16 * 0)]
> -    ldp     x14, x15, [sp, #(16 * 1)]
> -    ldp     x12, x13, [sp, #(16 * 2)]
> -    ldp     x10, x11, [sp, #(16 * 3)]
> -    ldp     x8, x9, [sp, #(16 * 4)]
> -    ldp     x6, x7, [sp, #(16 * 5)]
> -    ldp     x4, x5, [sp, #(16 * 6)]
> -    ldp     x2, x3, [sp, #(16 * 7)]
> -    ldp     x0, x1, [sp, #(16 * 8)]
> -    add     sp, sp, #(8 * 18)
> -ENTRY(__psci_hyp_bp_inval_end)
> -
>   ENTRY(__smccc_workaround_1_smc_start)
>       sub     sp, sp, #(8 * 4)
>       stp     x2, x3, [sp, #(8 * 0)]
> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
> index 6557577bcb..af453710e4 100644
> --- a/xen/arch/arm/cpuerrata.c
> +++ b/xen/arch/arm/cpuerrata.c
> @@ -149,10 +149,11 @@ install_bp_hardening_vec(const struct arm_cpu_capabilities *entry,
>   
>   extern char __smccc_workaround_1_smc_start[], __smccc_workaround_1_smc_end[];
>   
> -static bool
> -check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
> +static int enable_smccc_arch_workaround_1(void *data)
>   {
>       struct arm_smccc_res res;
> +    static bool warned = false;
> +    const struct arm_cpu_capabilities *entry = data;
>   
>       /*
>        * Enable callbacks are called on every CPU based on the
> @@ -160,47 +161,30 @@ check_smccc_arch_workaround_1(const struct arm_cpu_capabilities *entry)
>        * entry.
>        */
>       if ( !entry->matches(entry) )
> -        return false;
> +        return 0;
>   
>       if ( smccc_ver < SMCCC_VERSION(1, 1) )
> -        return false;
> +        goto warn;
>   
>       arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FID,
>                         ARM_SMCCC_ARCH_WORKAROUND_1_FID, &res);
>       if ( res.a0 != ARM_SMCCC_SUCCESS )
> -        return false;
> -
> -    return install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
> -                                    __smccc_workaround_1_smc_end,
> -                                    "call ARM_SMCCC_ARCH_WORKAROUND_1");
> -}
> +        goto warn;
>   
> -extern char __psci_hyp_bp_inval_start[], __psci_hyp_bp_inval_end[];
> +    return !install_bp_hardening_vec(entry,__smccc_workaround_1_smc_start,
> +                                     __smccc_workaround_1_smc_end,
> +                                     "call ARM_SMCCC_ARCH_WORKAROUND_1");
>   
> -static int enable_psci_bp_hardening(void *data)
> -{
> -    bool ret = true;
> -    static bool warned = false;
> -
> -    if ( check_smccc_arch_workaround_1(data) )
> -        return 0;
> -    /*
> -     * The mitigation is using PSCI version function to invalidate the
> -     * branch predictor. This function is only available with PSCI 0.2
> -     * and later.
> -     */
> -    else if ( psci_ver >= PSCI_VERSION(0, 2) )
> -        ret = install_bp_hardening_vec(data, __psci_hyp_bp_inval_start,
> -                                       __psci_hyp_bp_inval_end,
> -                                       "call PSCI get version");
> -    else if ( !warned )
> +warn:
> +    if ( !warned )
>       {
>           ASSERT(system_state < SYS_STATE_active);
> -        warning_add("PSCI 0.2 or later is required for the branch predictor hardening.\n");
> -        warned = true;
> +        warning_add("No support for ARM_SMCCC_ARCH_WORKAROUND_1.\n"
> +                    "Please update your firmware.\n");
> +        warned = false;
>       }
>   
> -    return !ret;
> +    return 0;
>   }
>   
>   #endif /* CONFIG_ARM64_HARDEN_BRANCH_PREDICTOR */
> @@ -316,22 +300,22 @@ static const struct arm_cpu_capabilities arm_errata[] = {
>       {
>           .capability = ARM_HARDEN_BRANCH_PREDICTOR,
>           MIDR_ALL_VERSIONS(MIDR_CORTEX_A57),
> -        .enable = enable_psci_bp_hardening,
> +        .enable = enable_smccc_arch_workaround_1,
>       },
>       {
>           .capability = ARM_HARDEN_BRANCH_PREDICTOR,
>           MIDR_ALL_VERSIONS(MIDR_CORTEX_A72),
> -        .enable = enable_psci_bp_hardening,
> +        .enable = enable_smccc_arch_workaround_1,
>       },
>       {
>           .capability = ARM_HARDEN_BRANCH_PREDICTOR,
>           MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
> -        .enable = enable_psci_bp_hardening,
> +        .enable = enable_smccc_arch_workaround_1,
>       },
>       {
>           .capability = ARM_HARDEN_BRANCH_PREDICTOR,
>           MIDR_ALL_VERSIONS(MIDR_CORTEX_A75),
> -        .enable = enable_psci_bp_hardening,
> +        .enable = enable_smccc_arch_workaround_1,
>       },
>   #endif
>   #ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
> 

-- 
Volodymyr Babchuk

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

[PATCH v2 13/15] xen/arm: vpsci: Remove parameter 'ver' from do_common_cpu

[Julien Grall]

Currently, the behavior of do_common_cpu will slightly change depending
on the PSCI version passed in parameter. Looking at the code, more the
specific 0.2 behavior could move out of the function or adapted for 0.1:

    - x0/r0 can be updated on PSCI 0.1 because general purpose registers
    are undefined upon CPU on.
    - PSCI 0.1 does not defined PSCI_ALREADY_ON. However, it would be
    safer to bail out if the CPU is already on.

Based on this, the parameter 'ver' is removed and do_psci_cpu_on
(implementation for PSCI 0.1) is adapted to avoid returning
PSCI_ALREADY_ON.

Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr.babchuk@epam.com>

---
    The reviewed-by was kept despite move this patch towards the end
    of the series because there was no clash with the rest of the series.

    Changes in v2:
        - Move the patch towards the end of the series as not strictly
        necessary for SP2.
        - Add Volodymyr's reviewed-by
---
 xen/arch/arm/vpsci.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 19ee7caeb4..7ea3ea58e3 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -22,7 +22,7 @@
 #include <public/sched.h>
 
 static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
-                       register_t context_id,int ver)
+                            register_t context_id)
 {
     struct vcpu *v;
     struct domain *d = current->domain;
@@ -40,8 +40,7 @@ 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 == PSCI_VERSION(0, 2)) &&
-            !test_bit(_VPF_down, &v->pause_flags) )
+    if ( !test_bit(_VPF_down, &v->pause_flags) )
         return PSCI_ALREADY_ON;
 
     if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
@@ -55,18 +54,21 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     ctxt->ttbr0 = 0;
     ctxt->ttbr1 = 0;
     ctxt->ttbcr = 0; /* Defined Reset Value */
+
+    /*
+     * x0/r0_usr are always updated because for PSCI 0.1 the general
+     * purpose registers are undefined upon CPU_on.
+     */
     if ( is_32bit_domain(d) )
     {
         ctxt->user_regs.cpsr = PSR_GUEST32_INIT;
-        if ( ver == PSCI_VERSION(0, 2) )
-            ctxt->user_regs.r0_usr = context_id;
+        ctxt->user_regs.r0_usr = context_id;
     }
 #ifdef CONFIG_ARM_64
     else
     {
         ctxt->user_regs.cpsr = PSR_GUEST64_INIT;
-        if ( ver == PSCI_VERSION(0, 2) )
-            ctxt->user_regs.x0 = context_id;
+        ctxt->user_regs.x0 = context_id;
     }
 #endif
 
@@ -93,7 +95,14 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
 
 static int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
 {
-    return do_common_cpu_on(vcpuid, entry_point, 0 , PSCI_VERSION(0, 1));
+    int32_t ret;
+
+    ret = do_common_cpu_on(vcpuid, entry_point, 0);
+    /*
+     * PSCI 0.1 does not define the return code PSCI_ALREADY_ON.
+     * Instead, return PSCI_INVALID_PARAMETERS.
+     */
+    return (ret == PSCI_ALREADY_ON) ? PSCI_INVALID_PARAMETERS : ret;
 }
 
 static int32_t do_psci_cpu_off(uint32_t power_state)
@@ -137,8 +146,7 @@ static 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,
-                            PSCI_VERSION(0, 2));
+    return do_common_cpu_on(target_cpu, entry_point, context_id);
 }
 
 static const unsigned long target_affinity_mask[] = {
-- 
2.11.0


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

[PATCH v2 14/15] xen/arm: psci: Consolidate PSCI version print

[Julien Grall]

Xen is printing the same way the PSCI version for 0.1, 0.2 and later.
The only different is the former is hardcoded.

Furthermore PSCI is now used for other things than SMP bring up. So only
print the PSCI version in psci_init.

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

---
    Changes in v2:
        - Patch added
---
 xen/arch/arm/psci.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index bc7b2260e8..7a8cf54e6d 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -136,8 +136,6 @@ int __init psci_init_0_1(void)
 
     psci_ver = PSCI_VERSION(0, 1);
 
-    printk(XENLOG_INFO "Using PSCI-0.1 for SMP bringup\n");
-
     return 0;
 }
 
@@ -183,9 +181,6 @@ int __init psci_init_0_2(void)
 
     psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
 
-    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
-           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
-
     return 0;
 }
 
@@ -205,6 +200,9 @@ int __init psci_init(void)
 
     psci_init_smccc();
 
+    printk(XENLOG_INFO "Using PSCI v%u.%u\n",
+           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
+
     return 0;
 }
 
-- 
2.11.0


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

Re: [PATCH v2 14/15] xen/arm: psci: Consolidate PSCI version print

[Volodymyr Babchuk]

On 08.02.18 21:22, Julien Grall wrote:
> Xen is printing the same way the PSCI version for 0.1, 0.2 and later.
> The only different is the former is hardcoded.
> 
> Furthermore PSCI is now used for other things than SMP bring up. So only
> print the PSCI version in psci_init.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/psci.c | 8 +++-----
>   1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index bc7b2260e8..7a8cf54e6d 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -136,8 +136,6 @@ int __init psci_init_0_1(void)
>   
>       psci_ver = PSCI_VERSION(0, 1);
>   
> -    printk(XENLOG_INFO "Using PSCI-0.1 for SMP bringup\n");
> -
>       return 0;
>   }
>   
> @@ -183,9 +181,6 @@ int __init psci_init_0_2(void)
>   
>       psci_cpu_on_nr = PSCI_0_2_FN_NATIVE(CPU_ON);
>   
> -    printk(XENLOG_INFO "Using PSCI-%u.%u for SMP bringup\n",
> -           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
> -
>       return 0;
>   }
>   
> @@ -205,6 +200,9 @@ int __init psci_init(void)
>   
>       psci_init_smccc();
>   
> +    printk(XENLOG_INFO "Using PSCI v%u.%u\n",
> +           PSCI_VERSION_MAJOR(psci_ver), PSCI_VERSION_MINOR(psci_ver));
> +
>       return 0;
>   }
>   
> 

-- 
Volodymyr Babchuk

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

[PATCH v2 15/15] xen/arm: psci: Prefix with static any functions not exported

[Julien Grall]

A bunch of PSCI functions are not prefixed with static despite no one is
using them outside the file and the prototype is not available in
psci.h.

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

---

    Changes in v2:
        - Patch added
---
 xen/arch/arm/psci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 7a8cf54e6d..5d94a9a9ae 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -66,7 +66,7 @@ static int __init psci_features(uint32_t psci_func_id)
     return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
 }
 
-int __init psci_is_smc_method(const struct dt_device_node *psci)
+static int __init psci_is_smc_method(const struct dt_device_node *psci)
 {
     int ret;
     const char *prop_str;
@@ -109,7 +109,7 @@ static void __init psci_init_smccc(void)
            SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
 }
 
-int __init psci_init_0_1(void)
+static int __init psci_init_0_1(void)
 {
     int ret;
     const struct dt_device_node *psci;
@@ -139,7 +139,7 @@ int __init psci_init_0_1(void)
     return 0;
 }
 
-int __init psci_init_0_2(void)
+static int __init psci_init_0_2(void)
 {
     static const struct dt_device_match psci_ids[] __initconst =
     {
-- 
2.11.0


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

Re: [PATCH v2 15/15] xen/arm: psci: Prefix with static any functions not exported

[Volodymyr Babchuk]

On 08.02.18 21:22, Julien Grall wrote:
> A bunch of PSCI functions are not prefixed with static despite no one is
> using them outside the file and the prototype is not available in
> psci.h.
> 
> Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

> ---
> 
>      Changes in v2:
>          - Patch added
> ---
>   xen/arch/arm/psci.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 7a8cf54e6d..5d94a9a9ae 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -66,7 +66,7 @@ static int __init psci_features(uint32_t psci_func_id)
>       return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
>   }
>   
> -int __init psci_is_smc_method(const struct dt_device_node *psci)
> +static int __init psci_is_smc_method(const struct dt_device_node *psci)
>   {
>       int ret;
>       const char *prop_str;
> @@ -109,7 +109,7 @@ static void __init psci_init_smccc(void)
>              SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
>   }
>   
> -int __init psci_init_0_1(void)
> +static int __init psci_init_0_1(void)
>   {
>       int ret;
>       const struct dt_device_node *psci;
> @@ -139,7 +139,7 @@ int __init psci_init_0_1(void)
>       return 0;
>   }
>   
> -int __init psci_init_0_2(void)
> +static int __init psci_init_0_2(void)
>   {
>       static const struct dt_device_match psci_ids[] __initconst =
>       {
> 

-- 
Volodymyr Babchuk

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