Re: [Xen-devel] [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`

Fri, 25 Aug 2017 03:57:07 -0700 

Hi Julien,

Thanks for the review.

On 24.08.17 19:58, Julien Grall wrote:

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:

PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `vsmc.c`. This patch just moves PSCI
handler calls from `traps.c` to `vsmc.c`.

Older PSCI 0.1 uses SMC function identifiers in range that is
resereved for exisings APIs (ARM DEN 0028B, page 16), while newer


s/resereved/reserved/
s/exisings/existing/


PSCI 0.2 is defined as "standard secure service" with own ranges


0.2 and later

"with its own ranges" I think.


(ARM DEN 0028B, page 18).

Signed-off-by: Volodymyr Babchuk <volodymyr_babc...@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushche...@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshche...@epam.com>
---

 * Fixed mistakes about non-existant PSCI 2.0
 * Added special SMC function number handling for PSCI 0.1
 * Fixed coding style in  handle_psci_0_1()
 * Changed how return do_trap_hvc_smccc() is called from traps.c
 * Renamed SSC to SSSC (Standard Secure Service Calls)

---
 xen/arch/arm/traps.c              | 117 +------------------------
xen/arch/arm/vsmc.c | 175 ++++++++++++++++++++++++++++++++++++-- 
 xen/include/asm-arm/smccc.h       |   4 +
 xen/include/asm-arm/vsmc.h        |   1 +
 xen/include/public/arch-arm/smc.h |   8 ++
 5 files changed, 183 insertions(+), 122 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4141a89..517e013 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1451,119 +1451,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code) 
 }
 #endif

-#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
-#define PSCI_ARG(reg, n) get_user_reg(reg, n)
-
-#ifdef CONFIG_ARM_64
-#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
-#else
-#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
-#endif
-
-/* helper function for checking arm mode 32/64 bit */
-static inline int psci_mode_check(struct domain *d, register_t fid)
-{
-        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
-}
-
-static void do_trap_psci(struct cpu_user_regs *regs)
-{
-    register_t fid = PSCI_ARG(regs,0);
-
-    /* preloading in case psci_mode_check fails */
-    PSCI_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
-    switch( fid )
-    {
-    case PSCI_cpu_off:
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            perfc_incr(vpsci_cpu_off);
-            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
-        }
-        break;
-    case PSCI_cpu_on:
-        {
-            uint32_t vcpuid = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            perfc_incr(vpsci_cpu_on);
-            PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
-        }
-        break;
-    case PSCI_0_2_FN_PSCI_VERSION:
-        perfc_incr(vpsci_version);
-        PSCI_SET_RESULT(regs, do_psci_0_2_version());
-        break;
-    case PSCI_0_2_FN_CPU_OFF:
-        perfc_incr(vpsci_cpu_off);
-        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-        perfc_incr(vpsci_migrate_info_type);
-        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
-    case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
-        perfc_incr(vpsci_migrate_info_up_cpu);
-        if ( psci_mode_check(current->domain, fid) )
-            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
-        break;
-    case PSCI_0_2_FN_SYSTEM_OFF:
-        perfc_incr(vpsci_system_off);
-        do_psci_0_2_system_off();
-        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_SYSTEM_RESET:
-        perfc_incr(vpsci_system_reset);
-        do_psci_0_2_system_reset();
-        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_CPU_ON:
-    case PSCI_0_2_FN64_CPU_ON:
-        perfc_incr(vpsci_cpu_on);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t vcpuid = PSCI_ARG(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
- PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); 
-        }
-        break;
-    case PSCI_0_2_FN_CPU_SUSPEND:
-    case PSCI_0_2_FN64_CPU_SUSPEND:
-        perfc_incr(vpsci_cpu_suspend);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
- PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); 
-        }
-        break;
-    case PSCI_0_2_FN_AFFINITY_INFO:
-    case PSCI_0_2_FN64_AFFINITY_INFO:
-        perfc_incr(vpsci_cpu_affinity_info);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t taff = PSCI_ARG(regs,1);
-            uint32_t laff = PSCI_ARG32(regs,2);
- PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); 
-        }
-        break;
-    case PSCI_0_2_FN_MIGRATE:
-    case PSCI_0_2_FN64_MIGRATE:
-        perfc_incr(vpsci_cpu_migrate);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
-        }
-        break;
-    default:
-        domain_crash_synchronous();
-        return;
-    }
-}
-
 #ifdef CONFIG_ARM_64
 #define HYPERCALL_RESULT_REG(r) (r)->x0
 #define HYPERCALL_ARG1(r) (r)->x0
@@ -2252,7 +2139,7 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs) 
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
+            return do_trap_hvc_smccc(regs);
         do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
         break;
 #ifdef CONFIG_ARM_64
@@ -2264,7 +2151,7 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs) 
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
+            return do_trap_hvc_smccc(regs);
         do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 0a81294..956d4ef 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -19,6 +19,7 @@
 #include <xen/types.h>
 #include <public/arch-arm/smc.h>
 #include <asm/monitor.h>
+#include <asm/psci.h>
 #include <asm/regs.h>
 #include <asm/smccc.h>
 #include <asm/traps.h>
@@ -27,6 +28,9 @@
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
+/* Number of functions currently supported by Standard Service Service Calls. */ 
+#define SSSC_SMCCC_FUNCTION_COUNT 13
+
 static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
 {
 #define FILL_UUID(n) \
@@ -62,6 +66,133 @@ static bool handle_hypervisor(struct cpu_user_regs *regs) 
     return false;
 }

+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg, n) get_user_reg(reg, n)
+
+#ifdef CONFIG_ARM_64
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/* PSCI 0.1 interface */
+static bool handle_psci_0_1(struct cpu_user_regs *regs)
+{
+    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )


I am not sure to understand the mask here.


+    {
+    case PSCI_cpu_off:
+    {
+        uint32_t pstate = PSCI_ARG32(regs, 1);
Missing newline here. I am ok with you do the clean-up in this patch rather separately. 


+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
+        return true;
+    }
+    case PSCI_cpu_on:
+    {
+        uint32_t vcpuid = PSCI_ARG32(regs, 1);
+        register_t epoint = PSCI_ARG(regs, 2);


Ditto.


+        perfc_incr(vpsci_cpu_on);
+        PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
+        return true;
+    }
+    }
+    return false;
+}
+
+/* helper function for checking arm mode 32/64 bit */
+static inline int psci_mode_check(struct domain *d, register_t fid)
+{
+    return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
+}
+
+/* PSCI 0.2 interface and other Standard Secure Calls */
+static bool handle_sssc(struct cpu_user_regs *regs)
+{
+    register_t fid = PSCI_ARG(regs, 0);
+
+    switch ( ARM_SMCCC_FUNC_NUM(fid) )
+    {
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+        perfc_incr(vpsci_version);
+        PSCI_SET_RESULT(regs, do_psci_0_2_version());
+        return true;
It is a bit hard to read. Some newline between each "case" would help for clarity I think. 


+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+        perfc_incr(vpsci_migrate_info_type);
+        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+        perfc_incr(vpsci_migrate_info_up_cpu);
+        if ( psci_mode_check(current->domain, fid) )
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+        perfc_incr(vpsci_system_off);
+        do_psci_0_2_system_off();
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+        perfc_incr(vpsci_system_reset);
+        do_psci_0_2_system_reset();
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+        perfc_incr(vpsci_cpu_on);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t vcpuid = PSCI_ARG(regs, 1);
+            register_t epoint = PSCI_ARG(regs, 2);
+            register_t cid = PSCI_ARG(regs, 3);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid)); 
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+        perfc_incr(vpsci_cpu_suspend);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t pstate = PSCI_ARG32(regs, 1);
+            register_t epoint = PSCI_ARG(regs, 2);
+            register_t cid = PSCI_ARG(regs, 3);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid)); 
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+        perfc_incr(vpsci_cpu_affinity_info);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t taff = PSCI_ARG(regs, 1);
+            uint32_t laff = PSCI_ARG32(regs, 2);
+ PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff)); 
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+        perfc_incr(vpsci_cpu_migrate);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t tcpu = PSCI_ARG32(regs, 1);
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+    {
+        static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;


Newline here please. But can't we just do:

return fill_uuid(regs, SSC_SMCCC_UID);

This would make the code simpler.
I'm not sure that I got this. fill_uuid() returns void, while handle_sssc() returns bool. 


+        fill_uuid(regs, psci_uuid);
+        return true;
+    }
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);


Same here.

Under "same" you meant newline, correct?

+        return true;
+    }
+    return false;
+}
+
 /*
  * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
  * returns true if that was valid SMCCC call (even if function number
@@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) 
 {
     bool handled = false;
     const union hsr hsr = { .bits = regs->hsr };
+    register_t func_id = get_user_reg(regs, 0);


Please introduce func_id in patch #6 rather than doing this rework here.



     /*
      * Check immediate value for HVC32, HVC64 and SMC64.
@@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs) 
     }

     /* 64 bit calls are allowed only from 64 bit domains. */
-    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+    if ( ARM_SMCCC_IS_64(func_id) &&
          is_32bit_domain(current->domain) )
     {
         set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
         return true;
     }

-    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+    /*
+     * Special case: identifier range for existing APIs.
+     * This range is described in SMCCC (ARM DEN 0028B, page 16),
+     * but it does not conforms to standard function identifier
+     * encoding.
+     */
+    if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
+         func_id <= ARM_SMCCC_RESERVED_RANGE_END )
+        handled = handle_psci_0_1(regs);
The region "reserved for existing APIs" is not only for psci 0.1. You should make it clear in the name. 


+    else
     {
-    case ARM_SMCCC_OWNER_HYPERVISOR:
-        handled = handle_hypervisor(regs);
-        break;
+        switch ( ARM_SMCCC_OWNER_NUM(func_id) )
+        {
+        case ARM_SMCCC_OWNER_HYPERVISOR:
+            handled = handle_hypervisor(regs);
+            break;
+        case ARM_SMCCC_OWNER_STANDARD:
+            handled = handle_sssc(regs);
+            break;
+        }
     }

     if ( !handled )
     {
-        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
-                get_user_reg(regs, 0));
+ gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", func_id); 
         /* Inform caller that function is not supported. */
         set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
     }
@@ -150,6 +296,21 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr) 
         inject_undef_exception(regs, hsr);
 }

+/* This function will be called from traps.c */
+void do_trap_hvc_smccc(struct cpu_user_regs *regs)
+{
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /*
+     * vsmccc_handle_call() will return false if this call is not
+     * SMCCC compatbile (i.e. immediate value != 0). As it is not
s/compatbile/compatible/. And you want to use e.g instead of i.e because "immediate value != 0" is not the only way to return false. 


+     * compatible, we can't be sure that guest will understand
+     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+     */
+    if ( !vsmccc_handle_call(regs) )
+        inject_undef_exception(regs, hsr);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 67da3fb..7c0003c 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -80,6 +80,10 @@
 /* Only one error code defined in SMCCC */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+/* SMCCC function identifier range which is reserved for existing APIs */ 
+#define ARM_SMCCC_RESERVED_RANGE_START  0x0
+#define ARM_SMCCC_RESERVED_RANGE_END    0x0100FFFF
+
 #endif  /* __ASM_ARM_SMCCC_H__ */

 /*
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
index 31aaa55..90ff610 100644
--- a/xen/include/asm-arm/vsmc.h
+++ b/xen/include/asm-arm/vsmc.h
@@ -17,6 +17,7 @@
 #include <xen/types.h>

 void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+void do_trap_hvc_smccc(struct cpu_user_regs *regs);

 #endif  /* __ASM_ARM_VSMC_H__ */
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h 
index 3d3cd90..c5327e3 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -46,6 +46,14 @@
#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \ 
                       0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)

+/* Standard Service Service Call version. */
+#define SSSC_SMCCC_MAJOR_REVISION 0
+#define SSSC_SMCCC_MINOR_REVISION 1
+
+/* Standard Service Call UID. Randomly generated with uuidgen. */
+#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\ 
+                      0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
+
 #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */


Cheers,



 /*





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

Reply via email to