[PATCH 0/3] xen/arm: SMCCC fixes and PSCI clean-up

[PATCH 0/3] xen/arm: SMCCC fixes and PSCI clean-up

[Julien Grall]

Hi all,

This small patch series contains SMCCC fixes (see #2) and PSCI clean-up.

Cheers,

Julien Grall (3):
  xen/arm: vpsci: Removing dummy MIGRATE and MIGRATE_INFO_UP_CPU
  xen/arm: vsmc: Don't implement function ID that doesn't exist
  xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

 xen/arch/arm/vpsci.c             | 149 +++++++++++++++++++++++++++++++++------
 xen/arch/arm/vsmc.c              | 122 ++++----------------------------
 xen/include/asm-arm/perfc_defn.h |   2 -
 xen/include/asm-arm/psci.h       |  23 +-----
 xen/include/asm-arm/smccc.h      |  20 +++++-
 5 files changed, 160 insertions(+), 156 deletions(-)

-- 
2.11.0


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

[PATCH 1/3] xen/arm: vpsci: Removing dummy MIGRATE and MIGRATE_INFO_UP_CPU

[Julien Grall]

The PSCI call MIGRATE and MIGRATE_INFO_UP_CPU are optional and
implemented as just returning PSCI_NOT_SUPPORTED (aka UNKNOWN_FUNCTION
for SMCCC).

The new SMCCC framework is able to deal with unimplemented function and
return the proper error code. So remove the implementations for both
function.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vpsci.c             | 10 ----------
 xen/arch/arm/vsmc.c              | 14 --------------
 xen/include/asm-arm/perfc_defn.h |  2 --
 xen/include/asm-arm/psci.h       |  2 --
 4 files changed, 28 deletions(-)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index cd724904ef..979d32ed6d 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -172,21 +172,11 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity,
     return PSCI_0_2_AFFINITY_LEVEL_OFF;
 }
 
-int32_t do_psci_0_2_migrate(uint32_t target_cpu)
-{
-    return PSCI_NOT_SUPPORTED;
-}
-
 uint32_t do_psci_0_2_migrate_info_type(void)
 {
     return PSCI_0_2_TOS_MP_OR_NOT_PRESENT;
 }
 
-register_t do_psci_0_2_migrate_info_up_cpu(void)
-{
-    return PSCI_NOT_SUPPORTED;
-}
-
 void do_psci_0_2_system_off( void )
 {
     struct domain *d = current->domain;
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index c9064de37a..563c2d8dda 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -157,11 +157,6 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
         return true;
 
-    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
-        perfc_incr(vpsci_migrate_info_up_cpu);
-        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
-        return true;
-
     case PSCI_0_2_FN_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
@@ -206,15 +201,6 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         return true;
     }
 
-    case PSCI_0_2_FN_MIGRATE:
-    {
-        uint32_t tcpu = PSCI_ARG32(regs, 1);
-
-        perfc_incr(vpsci_cpu_migrate);
-        PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
-        return true;
-    }
-
     case ARM_SMCCC_FUNC_CALL_COUNT:
         return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
 
diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
index 5f957ee6ec..a7acb7d21c 100644
--- a/xen/include/asm-arm/perfc_defn.h
+++ b/xen/include/asm-arm/perfc_defn.h
@@ -27,12 +27,10 @@ PERFCOUNTER(vpsci_cpu_on,              "vpsci: cpu_on")
 PERFCOUNTER(vpsci_cpu_off,             "vpsci: cpu_off")
 PERFCOUNTER(vpsci_version,             "vpsci: version")
 PERFCOUNTER(vpsci_migrate_info_type,   "vpsci: migrate_info_type")
-PERFCOUNTER(vpsci_migrate_info_up_cpu, "vpsci: migrate_info_up_cpu")
 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_cpu_migrate,         "vpsci: cpu_migrate")
 
 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 635ea5dae4..f7e2139031 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -37,9 +37,7 @@ int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
                        register_t context_id);
 int32_t do_psci_0_2_affinity_info(register_t target_affinity,
                               uint32_t lowest_affinity_level);
-int32_t do_psci_0_2_migrate(uint32_t target_cpu);
 uint32_t do_psci_0_2_migrate_info_type(void);
-register_t do_psci_0_2_migrate_info_up_cpu(void);
 void do_psci_0_2_system_off(void);
 void do_psci_0_2_system_reset(void);
 
-- 
2.11.0


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

Re: [PATCH 1/3] xen/arm: vpsci: Removing dummy MIGRATE and MIGRATE_INFO_UP_CPU

[Julien Grall]

Hi,

On 24/01/18 18:34, Julien Grall wrote:
> The PSCI call MIGRATE and MIGRATE_INFO_UP_CPU are optional and
> implemented as just returning PSCI_NOT_SUPPORTED (aka UNKNOWN_FUNCTION
> for SMCCC).
> 
> The new SMCCC framework is able to deal with unimplemented function and
> return the proper error code. So remove the implementations for both
> function.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>   xen/arch/arm/vpsci.c             | 10 ----------
>   xen/arch/arm/vsmc.c              | 14 --------------
>   xen/include/asm-arm/perfc_defn.h |  2 --
>   xen/include/asm-arm/psci.h       |  2 --

I should have probably remove the PSCI_0_2_FN_MIGRATE_* define in psci.h 
as well.

I will resend a series for that once I get feedback.

Cheers,

>   4 files changed, 28 deletions(-)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index cd724904ef..979d32ed6d 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -172,21 +172,11 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity,
>       return PSCI_0_2_AFFINITY_LEVEL_OFF;
>   }
>   
> -int32_t do_psci_0_2_migrate(uint32_t target_cpu)
> -{
> -    return PSCI_NOT_SUPPORTED;
> -}
> -
>   uint32_t do_psci_0_2_migrate_info_type(void)
>   {
>       return PSCI_0_2_TOS_MP_OR_NOT_PRESENT;
>   }
>   
> -register_t do_psci_0_2_migrate_info_up_cpu(void)
> -{
> -    return PSCI_NOT_SUPPORTED;
> -}
> -
>   void do_psci_0_2_system_off( void )
>   {
>       struct domain *d = current->domain;
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index c9064de37a..563c2d8dda 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -157,11 +157,6 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>           PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
>           return true;
>   
> -    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> -        perfc_incr(vpsci_migrate_info_up_cpu);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
> -        return true;
> -
>       case PSCI_0_2_FN_SYSTEM_OFF:
>           perfc_incr(vpsci_system_off);
>           do_psci_0_2_system_off();
> @@ -206,15 +201,6 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>           return true;
>       }
>   
> -    case PSCI_0_2_FN_MIGRATE:
> -    {
> -        uint32_t tcpu = PSCI_ARG32(regs, 1);
> -
> -        perfc_incr(vpsci_cpu_migrate);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> -        return true;
> -    }
> -
>       case ARM_SMCCC_FUNC_CALL_COUNT:
>           return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
>   
> diff --git a/xen/include/asm-arm/perfc_defn.h b/xen/include/asm-arm/perfc_defn.h
> index 5f957ee6ec..a7acb7d21c 100644
> --- a/xen/include/asm-arm/perfc_defn.h
> +++ b/xen/include/asm-arm/perfc_defn.h
> @@ -27,12 +27,10 @@ PERFCOUNTER(vpsci_cpu_on,              "vpsci: cpu_on")
>   PERFCOUNTER(vpsci_cpu_off,             "vpsci: cpu_off")
>   PERFCOUNTER(vpsci_version,             "vpsci: version")
>   PERFCOUNTER(vpsci_migrate_info_type,   "vpsci: migrate_info_type")
> -PERFCOUNTER(vpsci_migrate_info_up_cpu, "vpsci: migrate_info_up_cpu")
>   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_cpu_migrate,         "vpsci: cpu_migrate")
>   
>   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 635ea5dae4..f7e2139031 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -37,9 +37,7 @@ int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
>                          register_t context_id);
>   int32_t do_psci_0_2_affinity_info(register_t target_affinity,
>                                 uint32_t lowest_affinity_level);
> -int32_t do_psci_0_2_migrate(uint32_t target_cpu);
>   uint32_t do_psci_0_2_migrate_info_type(void);
> -register_t do_psci_0_2_migrate_info_up_cpu(void);
>   void do_psci_0_2_system_off(void);
>   void do_psci_0_2_system_reset(void);
>   
> 

-- 
Julien Grall

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

Re: [PATCH 1/3] xen/arm: vpsci: Removing dummy MIGRATE and MIGRATE_INFO_UP_CPU

[Volodymyr Babchuk]

Hi Julien,

On 24.01.18 20:34, Julien Grall wrote:
> The PSCI call MIGRATE and MIGRATE_INFO_UP_CPU are optional and 
> implemented as just returning PSCI_NOT_SUPPORTED (aka
> UNKNOWN_FUNCTION for SMCCC).
> 
> The new SMCCC framework is able to deal with unimplemented function
> and return the proper error code. So remove the implementations for
> both function.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
`Reviewed-by: Volodymr Babchuk <volodymyr_babchuk@epam.com>`

> --- xen/arch/arm/vpsci.c             | 10 ---------- 
> xen/arch/arm/vsmc.c              | 14 -------------- 
> xen/include/asm-arm/perfc_defn.h |  2 -- xen/include/asm-arm/psci.h
> |  2 -- 4 files changed, 28 deletions(-)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c index
> cd724904ef..979d32ed6d 100644 --- a/xen/arch/arm/vpsci.c +++
> b/xen/arch/arm/vpsci.c @@ -172,21 +172,11 @@ int32_t
> do_psci_0_2_affinity_info(register_t target_affinity, return
> PSCI_0_2_AFFINITY_LEVEL_OFF; }
> 
> -int32_t do_psci_0_2_migrate(uint32_t target_cpu) -{ -    return
> PSCI_NOT_SUPPORTED; -} - uint32_t
> do_psci_0_2_migrate_info_type(void) { return
> PSCI_0_2_TOS_MP_OR_NOT_PRESENT; }
> 
> -register_t do_psci_0_2_migrate_info_up_cpu(void) -{ -    return
> PSCI_NOT_SUPPORTED; -} - void do_psci_0_2_system_off( void ) { struct
> domain *d = current->domain; diff --git a/xen/arch/arm/vsmc.c
> b/xen/arch/arm/vsmc.c index c9064de37a..563c2d8dda 100644 ---
> a/xen/arch/arm/vsmc.c +++ b/xen/arch/arm/vsmc.c @@ -157,11 +157,6 @@
> static bool handle_sssc(struct cpu_user_regs *regs) 
> PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type()); return true;
> 
> -    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU: -
> perfc_incr(vpsci_migrate_info_up_cpu); -        PSCI_SET_RESULT(regs,
> do_psci_0_2_migrate_info_up_cpu()); -        return true; - case
> PSCI_0_2_FN_SYSTEM_OFF: perfc_incr(vpsci_system_off); 
> do_psci_0_2_system_off(); @@ -206,15 +201,6 @@ static bool
> handle_sssc(struct cpu_user_regs *regs) return true; }
> 
> -    case PSCI_0_2_FN_MIGRATE: -    { -        uint32_t tcpu =
> PSCI_ARG32(regs, 1); - -        perfc_incr(vpsci_cpu_migrate); -
> PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu)); -        return
> true; -    } - case ARM_SMCCC_FUNC_CALL_COUNT: return
> fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
> 
> diff --git a/xen/include/asm-arm/perfc_defn.h
> b/xen/include/asm-arm/perfc_defn.h index 5f957ee6ec..a7acb7d21c
> 100644 --- a/xen/include/asm-arm/perfc_defn.h +++
> b/xen/include/asm-arm/perfc_defn.h @@ -27,12 +27,10 @@
> PERFCOUNTER(vpsci_cpu_on,              "vpsci: cpu_on") 
> PERFCOUNTER(vpsci_cpu_off,             "vpsci: cpu_off") 
> PERFCOUNTER(vpsci_version,             "vpsci: version") 
> PERFCOUNTER(vpsci_migrate_info_type,   "vpsci: migrate_info_type") 
> -PERFCOUNTER(vpsci_migrate_info_up_cpu, "vpsci:
> migrate_info_up_cpu") 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_cpu_migrate,         "vpsci:
> cpu_migrate")
> 
> 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
> 635ea5dae4..f7e2139031 100644 --- a/xen/include/asm-arm/psci.h +++
> b/xen/include/asm-arm/psci.h @@ -37,9 +37,7 @@ int32_t
> do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point, 
> register_t context_id); int32_t do_psci_0_2_affinity_info(register_t
> target_affinity, uint32_t lowest_affinity_level); -int32_t
> do_psci_0_2_migrate(uint32_t target_cpu); uint32_t
> do_psci_0_2_migrate_info_type(void); -register_t
> do_psci_0_2_migrate_info_up_cpu(void); void
> do_psci_0_2_system_off(void); void do_psci_0_2_system_reset(void);
> 
> 

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

[PATCH 2/3] xen/arm: vsmc: Don't implement function ID that doesn't exist

[Julien Grall]

The current implementation of SMCCC relies on the fact only function
number (bits [15:0]) is enough to identify what to implement.

However, PSCI call are only available in the range 0x84000000-0x8400001F
and 0xC4000000-0xC400001F. Furthermore, not all SMC32 functions have
equivalent in the SMC64. This is the case of:
    * PSCI_VERSION
    * CPU_OFF
    * MIGRATE_INFO_TYPE
    * SYSTEM_OFF
    * SYSTEM_RESET

Similarly call count, uid, revision can only be query using smc32/hvc32
fast calls (See 6.2 in ARM DEN 0028B).

Xen should only implement identifier existing in the specification in
order to avoid potential clash with later revision. Therefore rework the
vsmc code to use the whole function identifier rather than only the
function number.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    This should be backported to Xen 4.10 as we should not implement
    functions identifier that does not exist toprevent clash with a
    later revision.
---
 xen/arch/arm/vsmc.c         | 37 +++++++++++++++++++++----------------
 xen/include/asm-arm/smccc.h | 20 +++++++++++++++++---
 2 files changed, 38 insertions(+), 19 deletions(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 563c2d8dda..7ca2880173 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -84,13 +84,15 @@ static bool fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt)
 /* SMCCC interface for hypervisor. Tell about itself. */
 static bool handle_hypervisor(struct cpu_user_regs *regs)
 {
-    switch ( smccc_get_fn(get_user_reg(regs, 0)) )
+    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
+
+    switch ( fid )
     {
-    case ARM_SMCCC_FUNC_CALL_COUNT:
+    case ARM_SMCCC_FUNC_CALL_COUNT(HYPERVISOR):
         return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT);
-    case ARM_SMCCC_FUNC_CALL_UID:
+    case ARM_SMCCC_FUNC_CALL_UID(HYPERVISOR):
         return fill_uid(regs, XEN_SMCCC_UID);
-    case ARM_SMCCC_FUNC_CALL_REVISION:
+    case ARM_SMCCC_FUNC_CALL_REVISION(HYPERVISOR):
         return fill_revision(regs, XEN_SMCCC_MAJOR_REVISION,
                              XEN_SMCCC_MINOR_REVISION);
     default:
@@ -140,36 +142,37 @@ static bool handle_sssc(struct cpu_user_regs *regs)
 {
     uint32_t fid = (uint32_t)get_user_reg(regs, 0);
 
-    switch ( smccc_get_fn(fid) )
+    switch ( fid )
     {
-    case PSCI_0_2_FN_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_FN_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_FN_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_FN_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_FN_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_FN_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);
@@ -180,7 +183,8 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         return true;
     }
 
-    case PSCI_0_2_FN_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);
@@ -191,7 +195,8 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         return true;
     }
 
-    case PSCI_0_2_FN_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);
@@ -201,13 +206,13 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         return true;
     }
 
-    case ARM_SMCCC_FUNC_CALL_COUNT:
+    case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
         return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
 
-    case ARM_SMCCC_FUNC_CALL_UID:
+    case ARM_SMCCC_FUNC_CALL_UID(STANDARD):
         return fill_uid(regs, SSSC_SMCCC_UID);
 
-    case ARM_SMCCC_FUNC_CALL_REVISION:
+    case ARM_SMCCC_FUNC_CALL_REVISION(STANDARD):
         return fill_revision(regs, SSSC_SMCCC_MAJOR_REVISION,
                              SSSC_SMCCC_MINOR_REVISION);
 
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index f543dea0bb..303517459f 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t funcid)
 #define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
 
 /* List of generic function numbers */
-#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
-#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
-#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
+#define ARM_SMCCC_FUNC_CALL_COUNT(owner)            \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_##owner,     \
+                       0xFF00)
+
+#define ARM_SMCCC_FUNC_CALL_UID(owner)              \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_##owner,     \
+                       0xFF01)
+
+#define ARM_SMCCC_FUNC_CALL_REVISION(owner)         \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_##owner,     \
+                       0xFF03)
 
 /* 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 2/3] xen/arm: vsmc: Don't implement function ID that doesn't exist

[Volodymyr Babchuk]

On 24.01.18 20:34, Julien Grall wrote:
> The current implementation of SMCCC relies on the fact only function
> number (bits [15:0]) is enough to identify what to implement.
> 
> However, PSCI call are only available in the range 0x84000000-0x8400001F
> and 0xC4000000-0xC400001F. Furthermore, not all SMC32 functions have
> equivalent in the SMC64. This is the case of:
>      * PSCI_VERSION
>      * CPU_OFF
>      * MIGRATE_INFO_TYPE
>      * SYSTEM_OFF
>      * SYSTEM_RESET
> 
> Similarly call count, uid, revision can only be query using smc32/hvc32
> fast calls (See 6.2 in ARM DEN 0028B).
> 
> Xen should only implement identifier existing in the specification in
> order to avoid potential clash with later revision. Therefore rework the
> vsmc code to use the whole function identifier rather than only the
> function number.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>      This should be backported to Xen 4.10 as we should not implement
>      functions identifier that does not exist toprevent clash with a
>      later revision.
> ---
>   xen/arch/arm/vsmc.c         | 37 +++++++++++++++++++++----------------
>   xen/include/asm-arm/smccc.h | 20 +++++++++++++++++---
>   2 files changed, 38 insertions(+), 19 deletions(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 563c2d8dda..7ca2880173 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -84,13 +84,15 @@ static bool fill_function_call_count(struct cpu_user_regs *regs, uint32_t cnt)
>   /* SMCCC interface for hypervisor. Tell about itself. */
>   static bool handle_hypervisor(struct cpu_user_regs *regs)
>   {
> -    switch ( smccc_get_fn(get_user_reg(regs, 0)) )
> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
> +
> +    switch ( fid )
>       {
> -    case ARM_SMCCC_FUNC_CALL_COUNT:
> +    case ARM_SMCCC_FUNC_CALL_COUNT(HYPERVISOR):
I think, in this case better to rename ARM_SMCCC_FUNC_CALL_COUNT to 
something like ARM_SMCCC_CALL_COUNT. Or ARM_SMCCC_FID_CALL_COUNT.

I proposing this to distinguish function number from function identifier
(as per ARM SMCCC).

>           return fill_function_call_count(regs, XEN_SMCCC_FUNCTION_COUNT);
> -    case ARM_SMCCC_FUNC_CALL_UID:
> +    case ARM_SMCCC_FUNC_CALL_UID(HYPERVISOR):
>           return fill_uid(regs, XEN_SMCCC_UID);
> -    case ARM_SMCCC_FUNC_CALL_REVISION:
> +    case ARM_SMCCC_FUNC_CALL_REVISION(HYPERVISOR):
>           return fill_revision(regs, XEN_SMCCC_MAJOR_REVISION,
>                                XEN_SMCCC_MINOR_REVISION);
>       default:
> @@ -140,36 +142,37 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>   {
>       uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>   
> -    switch ( smccc_get_fn(fid) )
> +    switch ( fid )
>       {
> -    case PSCI_0_2_FN_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_FN_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_FN_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_FN_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_FN_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_FN_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);
> @@ -180,7 +183,8 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>           return true;
>       }
>   
> -    case PSCI_0_2_FN_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);
> @@ -191,7 +195,8 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>           return true;
>       }
>   
> -    case PSCI_0_2_FN_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);
> @@ -201,13 +206,13 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>           return true;
>       }
>   
> -    case ARM_SMCCC_FUNC_CALL_COUNT:
> +    case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>           return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
>   
> -    case ARM_SMCCC_FUNC_CALL_UID:
> +    case ARM_SMCCC_FUNC_CALL_UID(STANDARD):
>           return fill_uid(regs, SSSC_SMCCC_UID);
>   
> -    case ARM_SMCCC_FUNC_CALL_REVISION:
> +    case ARM_SMCCC_FUNC_CALL_REVISION(STANDARD):
>           return fill_revision(regs, SSSC_SMCCC_MAJOR_REVISION,
>                                SSSC_SMCCC_MINOR_REVISION);
>   
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index f543dea0bb..303517459f 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t funcid)
>   #define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
>   
>   /* List of generic function numbers */
> -#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
> -#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
> -#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
So, I thing it is bette to leave
#define ARM_SMCCC_FUNC_CALL_*
and introduce

+#define ARM_SMCCC_FID_CALL_COUNT(owner)             \
+    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
+                       ARM_SMCCC_CONV_32,           \
+                       ARM_SMCCC_OWNER_##owner,     \
+                       ARM_SMCCC_FUNC_CALL_COUNT)

> +#define ARM_SMCCC_FUNC_CALL_COUNT(owner)            \
> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
> +                       ARM_SMCCC_CONV_32,           \
> +                       ARM_SMCCC_OWNER_##owner,     \
> +                       0xFF00)
> +
> +#define ARM_SMCCC_FUNC_CALL_UID(owner)              \
> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
> +                       ARM_SMCCC_CONV_32,           \
> +                       ARM_SMCCC_OWNER_##owner,     \
> +                       0xFF01)
> +
> +#define ARM_SMCCC_FUNC_CALL_REVISION(owner)         \
> +    ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,         \
> +                       ARM_SMCCC_CONV_32,           \
> +                       ARM_SMCCC_OWNER_##owner,     \
> +                       0xFF03)
>   
>   /* Only one error code defined in SMCCC */
>   #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> 

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

Re: [PATCH 2/3] xen/arm: vsmc: Don't implement function ID that doesn't exist

[Julien Grall]

On 26/01/18 18:03, Volodymyr Babchuk wrote:
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index f543dea0bb..303517459f 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t 
>> funcid)
>>   #define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
>>   /* List of generic function numbers */
>> -#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
>> -#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
>> -#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
> So, I thing it is bette to leave
> #define ARM_SMCCC_FUNC_CALL_*
> and introduce

I am ok to use FID, but I don't see any reason to keep ARM_SMCCC_FUNC_* 
one around. No-one can use them directly in Xen and this would add 
confusion.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/3] xen/arm: vsmc: Don't implement function ID that doesn't exist

[Volodymyr Babchuk]

On 26.01.18 20:07, Julien Grall wrote:
> 
> 
> On 26/01/18 18:03, Volodymyr Babchuk wrote:
>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>>> index f543dea0bb..303517459f 100644
>>> --- a/xen/include/asm-arm/smccc.h
>>> +++ b/xen/include/asm-arm/smccc.h
>>> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t 
>>> funcid)
>>>   #define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
>>>   /* List of generic function numbers */
>>> -#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
>>> -#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
>>> -#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>> So, I thing it is bette to leave
>> #define ARM_SMCCC_FUNC_CALL_*
>> and introduce
> 
> I am ok to use FID, but I don't see any reason to keep ARM_SMCCC_FUNC_* 
> one around. No-one can use them directly in Xen and this would add 
> confusion.

Right. So you can nuke them.
Another idea is to leave only one definition and use it like this:

  ARM_SMCCC_FID_CALL(COUNT, HYPERVISOR)

This will reduce number of similar macro definitions. What do you think?

I'm okay with any variant.

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

Re: [PATCH 2/3] xen/arm: vsmc: Don't implement function ID that doesn't exist

[Julien Grall]

Hi,

On 26/01/18 18:12, Volodymyr Babchuk wrote:
> 
> 
> On 26.01.18 20:07, Julien Grall wrote:
>>
>>
>> On 26/01/18 18:03, Volodymyr Babchuk wrote:
>>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>>>> index f543dea0bb..303517459f 100644
>>>> --- a/xen/include/asm-arm/smccc.h
>>>> +++ b/xen/include/asm-arm/smccc.h
>>>> @@ -82,9 +82,23 @@ static inline uint32_t smccc_get_owner(register_t 
>>>> funcid)
>>>>   #define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
>>>>   /* List of generic function numbers */
>>>> -#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
>>>> -#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
>>>> -#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>>> So, I thing it is bette to leave
>>> #define ARM_SMCCC_FUNC_CALL_*
>>> and introduce
>>
>> I am ok to use FID, but I don't see any reason to keep 
>> ARM_SMCCC_FUNC_* one around. No-one can use them directly in Xen and 
>> this would add confusion.
> 
> Right. So you can nuke them.
> Another idea is to leave only one definition and use it like this:
> 
>   ARM_SMCCC_FID_CALL(COUNT, HYPERVISOR)
> 
> This will reduce number of similar macro definitions. What do you think?

Well technically the function names are "Call Count", "Call UID", 
"Revision". Therefore the name is already wrong for revision.

I also don't see a good way to use this macro more than 3 times. Indeed 
fastcall, 32-conv is pretty specific.

So here the benefits seems really limited.

> 
> I'm okay with any variant.

I will stick with the one suggested in the patch I sent.

Cheers,


-- 
Julien Grall

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

[PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

At the moment PSCI function dispatching is done in vsmc.c and the
function implementation in vpsci.c. Some bits of the implementation is
even done in vsmc.c (see PSCI_SYSTEM_RESET).

This means that it is difficult to follow the implementation and also
requires to export functions for each PSCI functions.

Therefore move PSCI dispatching in two new functions do_psci_0_1_call
and do_psci_0_2_call. The former will handle PSCI 0.1 call while the latter
0.2 or later call.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
---
 xen/arch/arm/vpsci.c       | 141 ++++++++++++++++++++++++++++++++++++++++-----
 xen/arch/arm/vsmc.c        |  95 ++----------------------------
 xen/include/asm-arm/psci.h |  21 +------
 3 files changed, 135 insertions(+), 122 deletions(-)

diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
index 979d32ed6d..b3ee193621 100644
--- a/xen/arch/arm/vpsci.c
+++ b/xen/arch/arm/vpsci.c
@@ -91,12 +91,12 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
     return PSCI_SUCCESS;
 }
 
-int32_t do_psci_cpu_on(uint32_t vcpuid, 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 do_psci_cpu_off(uint32_t power_state)
+static int32_t do_psci_cpu_off(uint32_t power_state)
 {
     struct vcpu *v = current;
     if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
@@ -104,13 +104,14 @@ int32_t do_psci_cpu_off(uint32_t power_state)
     return PSCI_SUCCESS;
 }
 
-uint32_t do_psci_0_2_version(void)
+static uint32_t do_psci_0_2_version(void)
 {
     return PSCI_VERSION(0, 2);
 }
 
-register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
-                            register_t context_id)
+static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
+                                          register_t entry_point,
+                                          register_t context_id)
 {
     struct vcpu *v = current;
 
@@ -123,13 +124,14 @@ register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
     return PSCI_SUCCESS;
 }
 
-int32_t do_psci_0_2_cpu_off(void)
+static int32_t do_psci_0_2_cpu_off(void)
 {
     return do_psci_cpu_off(0);
 }
 
-int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
-                       register_t context_id)
+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));
@@ -144,8 +146,8 @@ static const unsigned long target_affinity_mask[] = {
 #endif
 };
 
-int32_t do_psci_0_2_affinity_info(register_t target_affinity,
-                              uint32_t lowest_affinity_level)
+static int32_t do_psci_0_2_affinity_info(register_t target_affinity,
+                                         uint32_t lowest_affinity_level)
 {
     struct domain *d = current->domain;
     struct vcpu *v;
@@ -172,23 +174,136 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity,
     return PSCI_0_2_AFFINITY_LEVEL_OFF;
 }
 
-uint32_t do_psci_0_2_migrate_info_type(void)
+static uint32_t do_psci_0_2_migrate_info_type(void)
 {
     return PSCI_0_2_TOS_MP_OR_NOT_PRESENT;
 }
 
-void do_psci_0_2_system_off( void )
+static void do_psci_0_2_system_off( void )
 {
     struct domain *d = current->domain;
     domain_shutdown(d,SHUTDOWN_poweroff);
 }
 
-void do_psci_0_2_system_reset(void)
+static void do_psci_0_2_system_reset(void)
 {
     struct domain *d = current->domain;
     domain_shutdown(d,SHUTDOWN_reboot);
 }
 
+#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))
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/*
+ * PSCI 0.1 calls. It will return false if the function ID is not
+ * handled.
+ */
+bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid)
+{
+    switch ( (uint32_t)get_user_reg(regs, 0) )
+    {
+    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));
+        return true;
+    }
+    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));
+        return true;
+    }
+    default:
+        return false;
+    }
+}
+
+/*
+ * PSCI 0.2 or later calls. It will return false if the function ID is
+ * not handled.
+ */
+bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
+{
+    switch ( fid )
+    {
+    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):
+        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):
+        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):
+        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):
+        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):
+    {
+        register_t vcpuid = PSCI_ARG(regs, 1);
+        register_t epoint = PSCI_ARG(regs, 2);
+        register_t cid = PSCI_ARG(regs, 3);
+
+        perfc_incr(vpsci_cpu_on);
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
+        return true;
+    }
+
+    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);
+        register_t cid = PSCI_ARG(regs, 3);
+
+        perfc_incr(vpsci_cpu_suspend);
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
+        return true;
+    }
+
+    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);
+
+        perfc_incr(vpsci_cpu_affinity_info);
+        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
+        return true;
+    }
+    default:
+        return false;
+    }
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 7ca2880173..9b48d52896 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -100,41 +100,13 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
     }
 }
 
-#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))
-#else
-#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
-#endif
-
 /* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */
 static bool handle_existing_apis(struct cpu_user_regs *regs)
 {
     /* Only least 32 bits are significant (ARM DEN 0028B, page 12) */
-    switch ( (uint32_t)get_user_reg(regs, 0) )
-    {
-    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));
-        return true;
-    }
-    case PSCI_cpu_on:
-    {
-        uint32_t vcpuid = PSCI_ARG32(regs, 1);
-        register_t epoint = PSCI_ARG(regs, 2);
+    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
 
-        perfc_incr(vpsci_cpu_on);
-        PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
-        return true;
-    }
-    default:
-        return false;
-    }
+    return do_psci_0_1_call(regs, fid);
 }
 
 /* PSCI 0.2 interface and other Standard Secure Calls */
@@ -142,70 +114,11 @@ static bool handle_sssc(struct cpu_user_regs *regs)
 {
     uint32_t fid = (uint32_t)get_user_reg(regs, 0);
 
-    switch ( fid )
-    {
-    case PSCI_0_2_FN32(PSCI_VERSION):
-        perfc_incr(vpsci_version);
-        PSCI_SET_RESULT(regs, do_psci_0_2_version());
+    if ( do_psci_0_2_call(regs, fid) )
         return true;
 
-    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):
-        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):
-        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):
-        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):
-    {
-        register_t vcpuid = PSCI_ARG(regs, 1);
-        register_t epoint = PSCI_ARG(regs, 2);
-        register_t cid = PSCI_ARG(regs, 3);
-
-        perfc_incr(vpsci_cpu_on);
-        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
-        return true;
-    }
-
-    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);
-        register_t cid = PSCI_ARG(regs, 3);
-
-        perfc_incr(vpsci_cpu_suspend);
-        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
-        return true;
-    }
-
-    case PSCI_0_2_FN32(AFFINITY_INFO):
-    case PSCI_0_2_FN64(AFFINITY_INFO):
+    switch ( fid )
     {
-        register_t taff = PSCI_ARG(regs, 1);
-        uint32_t laff = PSCI_ARG32(regs, 2);
-
-        perfc_incr(vpsci_cpu_affinity_info);
-        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
-        return true;
-    }
-
     case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
         return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
 
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index f7e2139031..3075c998f3 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu);
 void call_psci_system_off(void);
 void call_psci_system_reset(void);
 
-/* functions to handle guest PSCI requests */
-int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
-int32_t do_psci_cpu_off(uint32_t power_state);
-int32_t do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
-int32_t do_psci_migrate(uint32_t vcpuid);
-
-/* PSCI 0.2 functions to handle guest PSCI requests */
-uint32_t do_psci_0_2_version(void);
-register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
-                            register_t context_id);
-int32_t do_psci_0_2_cpu_off(void);
-int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
-                       register_t context_id);
-int32_t do_psci_0_2_affinity_info(register_t target_affinity,
-                              uint32_t lowest_affinity_level);
-uint32_t do_psci_0_2_migrate_info_type(void);
-void do_psci_0_2_system_off(void);
-void do_psci_0_2_system_reset(void);
+/* Functions handle PSCI calls from the guests */
+bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
+bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid);
 
 /* PSCI v0.2 interface */
 #define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
-- 
2.11.0


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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

On 24.01.18 20:34, Julien Grall wrote:
> At the moment PSCI function dispatching is done in vsmc.c and the
> function implementation in vpsci.c. Some bits of the implementation is
> even done in vsmc.c (see PSCI_SYSTEM_RESET).
> 
> This means that it is difficult to follow the implementation and also
> requires to export functions for each PSCI functions.
> 
> Therefore move PSCI dispatching in two new functions do_psci_0_1_call
> and do_psci_0_2_call. The former will handle PSCI 0.1 call while the latter
> 0.2 or later call.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>   xen/arch/arm/vpsci.c       | 141 ++++++++++++++++++++++++++++++++++++++++-----
>   xen/arch/arm/vsmc.c        |  95 ++----------------------------
>   xen/include/asm-arm/psci.h |  21 +------
>   3 files changed, 135 insertions(+), 122 deletions(-)
> 
> diff --git a/xen/arch/arm/vpsci.c b/xen/arch/arm/vpsci.c
> index 979d32ed6d..b3ee193621 100644
> --- a/xen/arch/arm/vpsci.c
> +++ b/xen/arch/arm/vpsci.c
> @@ -91,12 +91,12 @@ static int do_common_cpu_on(register_t target_cpu, register_t entry_point,
>       return PSCI_SUCCESS;
>   }
>   
> -int32_t do_psci_cpu_on(uint32_t vcpuid, 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 do_psci_cpu_off(uint32_t power_state)
> +static int32_t do_psci_cpu_off(uint32_t power_state)
>   {
>       struct vcpu *v = current;
>       if ( !test_and_set_bit(_VPF_down, &v->pause_flags) )
> @@ -104,13 +104,14 @@ int32_t do_psci_cpu_off(uint32_t power_state)
>       return PSCI_SUCCESS;
>   }
>   
> -uint32_t do_psci_0_2_version(void)
> +static uint32_t do_psci_0_2_version(void)
>   {
>       return PSCI_VERSION(0, 2);
>   }
>   
> -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
> -                            register_t context_id)
> +static register_t do_psci_0_2_cpu_suspend(uint32_t power_state,
> +                                          register_t entry_point,
> +                                          register_t context_id)
>   {
>       struct vcpu *v = current;
>   
> @@ -123,13 +124,14 @@ register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
>       return PSCI_SUCCESS;
>   }
>   
> -int32_t do_psci_0_2_cpu_off(void)
> +static int32_t do_psci_0_2_cpu_off(void)
>   {
>       return do_psci_cpu_off(0);
>   }
>   
> -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
> -                       register_t context_id)
> +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));
> @@ -144,8 +146,8 @@ static const unsigned long target_affinity_mask[] = {
>   #endif
>   };
>   
> -int32_t do_psci_0_2_affinity_info(register_t target_affinity,
> -                              uint32_t lowest_affinity_level)
> +static int32_t do_psci_0_2_affinity_info(register_t target_affinity,
> +                                         uint32_t lowest_affinity_level)
>   {
>       struct domain *d = current->domain;
>       struct vcpu *v;
> @@ -172,23 +174,136 @@ int32_t do_psci_0_2_affinity_info(register_t target_affinity,
>       return PSCI_0_2_AFFINITY_LEVEL_OFF;
>   }
>   
> -uint32_t do_psci_0_2_migrate_info_type(void)
> +static uint32_t do_psci_0_2_migrate_info_type(void)
>   {
>       return PSCI_0_2_TOS_MP_OR_NOT_PRESENT;
>   }
>   
> -void do_psci_0_2_system_off( void )
> +static void do_psci_0_2_system_off( void )
>   {
>       struct domain *d = current->domain;
>       domain_shutdown(d,SHUTDOWN_poweroff);
>   }
>   
> -void do_psci_0_2_system_reset(void)
> +static void do_psci_0_2_system_reset(void)
>   {
>       struct domain *d = current->domain;
>       domain_shutdown(d,SHUTDOWN_reboot);
>   }
>   
> +#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))
> +#else
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> +#endif
> +
> +/*
> + * PSCI 0.1 calls. It will return false if the function ID is not
> + * handled.
> + */
> +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid)
> +{
> +    switch ( (uint32_t)get_user_reg(regs, 0) )
> +    {
> +    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));
> +        return true;
> +    }
> +    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));
> +        return true;
> +    }
> +    default:
> +        return false;
> +    }
> +}
> +
> +/*
> + * PSCI 0.2 or later calls. It will return false if the function ID is
> + * not handled.
> + */
> +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid)
> +{
> +    switch ( fid )
> +    {
> +    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):
> +        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):
> +        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):
> +        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):
> +        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):
> +    {
> +        register_t vcpuid = PSCI_ARG(regs, 1);
> +        register_t epoint = PSCI_ARG(regs, 2);
> +        register_t cid = PSCI_ARG(regs, 3);
> +
> +        perfc_incr(vpsci_cpu_on);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> +        return true;
> +    }
> +
> +    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);
> +        register_t cid = PSCI_ARG(regs, 3);
> +
> +        perfc_incr(vpsci_cpu_suspend);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> +        return true;
> +    }
> +
> +    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);
> +
> +        perfc_incr(vpsci_cpu_affinity_info);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> +        return true;
> +    }
> +    default:
> +        return false;
> +    }
> +}
> +
>   /*
>    * Local variables:
>    * mode: C
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 7ca2880173..9b48d52896 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -100,41 +100,13 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
>       }
>   }
>   
> -#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))
> -#else
> -#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> -#endif
> -
>   /* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */
>   static bool handle_existing_apis(struct cpu_user_regs *regs)
>   {
>       /* Only least 32 bits are significant (ARM DEN 0028B, page 12) */
> -    switch ( (uint32_t)get_user_reg(regs, 0) )
> -    {
> -    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));
> -        return true;
> -    }
> -    case PSCI_cpu_on:
> -    {
> -        uint32_t vcpuid = PSCI_ARG32(regs, 1);
> -        register_t epoint = PSCI_ARG(regs, 2);
> +    uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>   
> -        perfc_incr(vpsci_cpu_on);
> -        PSCI_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
> -        return true;
> -    }
> -    default:
> -        return false;
> -    }
> +    return do_psci_0_1_call(regs, fid);
>   }
>   
>   /* PSCI 0.2 interface and other Standard Secure Calls */
> @@ -142,70 +114,11 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>   {
>       uint32_t fid = (uint32_t)get_user_reg(regs, 0);
>   
> -    switch ( fid )
> -    {
> -    case PSCI_0_2_FN32(PSCI_VERSION):
> -        perfc_incr(vpsci_version);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_version());
> +    if ( do_psci_0_2_call(regs, fid) )
>           return true;
>   
> -    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):
> -        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):
> -        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):
> -        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):
> -    {
> -        register_t vcpuid = PSCI_ARG(regs, 1);
> -        register_t epoint = PSCI_ARG(regs, 2);
> -        register_t cid = PSCI_ARG(regs, 3);
> -
> -        perfc_incr(vpsci_cpu_on);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> -        return true;
> -    }
> -
> -    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);
> -        register_t cid = PSCI_ARG(regs, 3);
> -
> -        perfc_incr(vpsci_cpu_suspend);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> -        return true;
> -    }
> -
> -    case PSCI_0_2_FN32(AFFINITY_INFO):
> -    case PSCI_0_2_FN64(AFFINITY_INFO):
> +    switch ( fid )
>       {
> -        register_t taff = PSCI_ARG(regs, 1);
> -        uint32_t laff = PSCI_ARG32(regs, 2);
> -
> -        perfc_incr(vpsci_cpu_affinity_info);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
> -        return true;
> -    }
> -
>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>           return fill_function_call_count(regs, SSSC_SMCCC_FUNCTION_COUNT);
Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
Maybe it is time to introduce function get_psci_0_2_fn_count() and use 
it there, what do you think?

>   
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index f7e2139031..3075c998f3 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu);
>   void call_psci_system_off(void);
>   void call_psci_system_reset(void);
>   
> -/* functions to handle guest PSCI requests */
> -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
> -int32_t do_psci_cpu_off(uint32_t power_state);
> -int32_t do_psci_cpu_suspend(uint32_t power_state, register_t entry_point);
> -int32_t do_psci_migrate(uint32_t vcpuid);
> -
> -/* PSCI 0.2 functions to handle guest PSCI requests */
> -uint32_t do_psci_0_2_version(void);
> -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t entry_point,
> -                            register_t context_id);
> -int32_t do_psci_0_2_cpu_off(void);
> -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t entry_point,
> -                       register_t context_id);
> -int32_t do_psci_0_2_affinity_info(register_t target_affinity,
> -                              uint32_t lowest_affinity_level);
> -uint32_t do_psci_0_2_migrate_info_type(void);
> -void do_psci_0_2_system_off(void);
> -void do_psci_0_2_system_reset(void);
> +/* Functions handle PSCI calls from the guests */
> +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
> +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid);
>   
>   /* PSCI v0.2 interface */
>   #define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
> 

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

Hi,

On 26/01/18 18:09, Volodymyr Babchuk wrote:
> On 24.01.18 20:34, Julien Grall wrote:
>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>> +    switch ( fid )
>>       {
>> -        register_t taff = PSCI_ARG(regs, 1);
>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>> -
>> -        perfc_incr(vpsci_cpu_affinity_info);
>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>> -        return true;
>> -    }
>> -
>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>           return fill_function_call_count(regs, 
>> SSSC_SMCCC_FUNCTION_COUNT);
> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
> Maybe it is time to introduce function get_psci_0_2_fn_count() and use 
> it there, what do you think?

Definitely not a function. It is a static number. But I can think of 
separate the call count.

Cheers,

> 
>> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>> index f7e2139031..3075c998f3 100644
>> --- a/xen/include/asm-arm/psci.h
>> +++ b/xen/include/asm-arm/psci.h
>> @@ -22,24 +22,9 @@ int call_psci_cpu_on(int cpu);
>>   void call_psci_system_off(void);
>>   void call_psci_system_reset(void);
>> -/* functions to handle guest PSCI requests */
>> -int32_t do_psci_cpu_on(uint32_t vcpuid, register_t entry_point);
>> -int32_t do_psci_cpu_off(uint32_t power_state);
>> -int32_t do_psci_cpu_suspend(uint32_t power_state, register_t 
>> entry_point);
>> -int32_t do_psci_migrate(uint32_t vcpuid);
>> -
>> -/* PSCI 0.2 functions to handle guest PSCI requests */
>> -uint32_t do_psci_0_2_version(void);
>> -register_t do_psci_0_2_cpu_suspend(uint32_t power_state, register_t 
>> entry_point,
>> -                            register_t context_id);
>> -int32_t do_psci_0_2_cpu_off(void);
>> -int32_t do_psci_0_2_cpu_on(register_t target_cpu, register_t 
>> entry_point,
>> -                       register_t context_id);
>> -int32_t do_psci_0_2_affinity_info(register_t target_affinity,
>> -                              uint32_t lowest_affinity_level);
>> -uint32_t do_psci_0_2_migrate_info_type(void);
>> -void do_psci_0_2_system_off(void);
>> -void do_psci_0_2_system_reset(void);
>> +/* Functions handle PSCI calls from the guests */
>> +bool do_psci_0_1_call(struct cpu_user_regs *regs, uint32_t fid);
>> +bool do_psci_0_2_call(struct cpu_user_regs *regs, uint32_t fid);
>>   /* PSCI v0.2 interface */
>>   #define PSCI_0_2_FN32(name) 
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
>>

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

Hi,

On 26.01.18 20:15, Julien Grall wrote:
> Hi,
> 
> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>> On 24.01.18 20:34, Julien Grall wrote:
>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>> +    switch ( fid )
>>>       {
>>> -        register_t taff = PSCI_ARG(regs, 1);
>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>> -
>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>>> -        return true;
>>> -    }
>>> -
>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>           return fill_function_call_count(regs, 
>>> SSSC_SMCCC_FUNCTION_COUNT);
>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>> Maybe it is time to introduce function get_psci_0_2_fn_count() and use 
>> it there, what do you think?
> 
> Definitely not a function. It is a static number. But I can think of 
> separate the call count.
Yep, separate call count for vPSCI and for SSSC itself would be a good 
thing.

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

On 26/01/18 18:27, Volodymyr Babchuk wrote:
> Hi,

Hi Volodymyr,

> On 26.01.18 20:15, Julien Grall wrote:
>> Hi,
>>
>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>> On 24.01.18 20:34, Julien Grall wrote:
>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>> +    switch ( fid )
>>>>       {
>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>> -
>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>>>> -        return true;
>>>> -    }
>>>> -
>>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>           return fill_function_call_count(regs, 
>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and 
>>> use it there, what do you think?
>>
>> Definitely not a function. It is a static number. But I can think of 
>> separate the call count.
> Yep, separate call count for vPSCI and for SSSC itself would be a good 
> thing.

Looking a bit more into it, this will not make a real improvement. This 
will be equally difficult to remember to update the call count.

Also, based on the recent SMCCC update (2.1.5 [1]):
"The General Service Queries for SMCCC call ranges are described in 
SMCCC section 6.2. These functions are not always well suited to 
firmware that is integrated with multiple sub-services being combined 
into one service range. For example, PSCI and SDEI in the Standard 
Service range. In particular, the ‘call count’ and ‘revision’ functions 
do not provide useful information to the caller when multiple functions 
are provided. As a result, these are not widely used to identify 
firmware services."

So I would not worry that much if the function count is not sync in the 
future. BTW, it is already wrong in Xen 4.10. For standard service, we 
don't implement 13 but 52.

Cheers,

[1] 
https://developer.arm.com/-/media/developer/pdf/ARM%20DEN%200070A%20Firmware%20interfaces%20for%20mitigating%20CVE-2017-5715_V1.0.pdf?revision=d20d1476-b770-4739-aebe-2d3147788e3f&la=en

I don't expect PSCI to be


-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

Hi Julien,

On 30.01.18 20:01, Julien Grall wrote:
> 
> 
> On 26/01/18 18:27, Volodymyr Babchuk wrote:
>> Hi,
> 
> Hi Volodymyr,
> 
>> On 26.01.18 20:15, Julien Grall wrote:
>>> Hi,
>>>
>>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>>> On 24.01.18 20:34, Julien Grall wrote:
>>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>>> +    switch ( fid )
>>>>>       {
>>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>>> -
>>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>>>>> -        return true;
>>>>> -    }
>>>>> -
>>>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>>           return fill_function_call_count(regs, 
>>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and 
>>>> use it there, what do you think?
>>>
>>> Definitely not a function. It is a static number. But I can think of 
>>> separate the call count.
>> Yep, separate call count for vPSCI and for SSSC itself would be a good 
>> thing.
> 
> Looking a bit more into it, this will not make a real improvement. This 
> will be equally difficult to remember to update the call count.
Nevertheless, I think that it is right thing to hold call count in the
same file, where calls are implemented. This increases chances that call 
count will be held in sync.

> Also, based on the recent SMCCC update (2.1.5 [1]):
> "The General Service Queries for SMCCC call ranges are described in 
> SMCCC section 6.2. These functions are not always well suited to 
> firmware that is integrated with multiple sub-services being combined 
> into one service range. For example, PSCI and SDEI in the Standard 
> Service range. In particular, the ‘call count’ and ‘revision’ functions 
> do not provide useful information to the caller when multiple functions 
> are provided. As a result, these are not widely used to identify 
> firmware services."
> 
> So I would not worry that much if the function count is not sync in the 
> future. BTW, it is already wrong in Xen 4.10. For standard service, we 
> don't implement 13 but 52.
2.1.5 deprecates general service queries in SMCCC 1.1. So, either we 
upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and 
should return right number of functions, including changes in Xen 4.10 
that you mentioned.

BTW, personally I'm not happy with call UID and call version 
deprecation. This makes impossible dynamic discovery of TEE, which was 
used in my TEE mediator patch series.


> Cheers,
> 
> [1] 
> https://developer.arm.com/-/media/developer/pdf/ARM%20DEN%200070A%20Firmware%20interfaces%20for%20mitigating%20CVE-2017-5715_V1.0.pdf?revision=d20d1476-b770-4739-aebe-2d3147788e3f&la=en 
> 
> 
> I don't expect PSCI to be
> 
There is some text missing. Are you wanted to add something?

-- 
Volodymyr Babchuk

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

On 30/01/18 18:28, Volodymyr Babchuk wrote:
> Hi Julien,
> 
> On 30.01.18 20:01, Julien Grall wrote:
>>
>>
>> On 26/01/18 18:27, Volodymyr Babchuk wrote:
>>> Hi,
>>
>> Hi Volodymyr,
>>
>>> On 26.01.18 20:15, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>>>> On 24.01.18 20:34, Julien Grall wrote:
>>>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>>>> +    switch ( fid )
>>>>>>       {
>>>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>>>> -
>>>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, 
>>>>>> laff));
>>>>>> -        return true;
>>>>>> -    }
>>>>>> -
>>>>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>>>           return fill_function_call_count(regs, 
>>>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and 
>>>>> use it there, what do you think?
>>>>
>>>> Definitely not a function. It is a static number. But I can think of 
>>>> separate the call count.
>>> Yep, separate call count for vPSCI and for SSSC itself would be a 
>>> good thing.
>>
>> Looking a bit more into it, this will not make a real improvement. 
>> This will be equally difficult to remember to update the call count.
> Nevertheless, I think that it is right thing to hold call count in the
> same file, where calls are implemented. This increases chances that call 
> count will be held in sync.

So you are suggesting to implement a function? If so, that's a no-go 
from my side.

> 
>> Also, based on the recent SMCCC update (2.1.5 [1]):
>> "The General Service Queries for SMCCC call ranges are described in 
>> SMCCC section 6.2. These functions are not always well suited to 
>> firmware that is integrated with multiple sub-services being combined 
>> into one service range. For example, PSCI and SDEI in the Standard 
>> Service range. In particular, the ‘call count’ and ‘revision’ 
>> functions do not provide useful information to the caller when 
>> multiple functions are provided. As a result, these are not widely 
>> used to identify firmware services."
>>
>> So I would not worry that much if the function count is not sync in 
>> the future. BTW, it is already wrong in Xen 4.10. For standard 
>> service, we don't implement 13 but 52.
> 2.1.5 deprecates general service queries in SMCCC 1.1. So, either we 
> upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and 
> should return right number of functions, including changes in Xen 4.10 
> that you mentioned.
> 
> BTW, personally I'm not happy with call UID and call version 
> deprecation. This makes impossible dynamic discovery of TEE, which was 
> used in my TEE mediator patch series.

2.1.5 only deprecates for Architecture service which is not implemented 
for Xen at moment. Nothing is been said for the rest of them OP-TEE. The 
documentation only acknowledge the fact it is difficult for some service 
(such as SSSC) to infer information from that.

I looked at some implementation of SMCCC and those calls are either not 
handled or the number are not correct.

Anyway, I have a SMCCC 1.1 patch series coming up soon.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

On 30.01.18 20:44, Julien Grall wrote:
> 
> 
> On 30/01/18 18:28, Volodymyr Babchuk wrote:
>> Hi Julien,
>>
>> On 30.01.18 20:01, Julien Grall wrote:
>>>
>>>
>>> On 26/01/18 18:27, Volodymyr Babchuk wrote:
>>>> Hi,
>>>
>>> Hi Volodymyr,
>>>
>>>> On 26.01.18 20:15, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>>>>> On 24.01.18 20:34, Julien Grall wrote:
>>>>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>>>>> +    switch ( fid )
>>>>>>>       {
>>>>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>>>>> -
>>>>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, 
>>>>>>> laff));
>>>>>>> -        return true;
>>>>>>> -    }
>>>>>>> -
>>>>>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>>>>           return fill_function_call_count(regs, 
>>>>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and 
>>>>>> use it there, what do you think?
>>>>>
>>>>> Definitely not a function. It is a static number. But I can think 
>>>>> of separate the call count.
>>>> Yep, separate call count for vPSCI and for SSSC itself would be a 
>>>> good thing.
>>>
>>> Looking a bit more into it, this will not make a real improvement. 
>>> This will be equally difficult to remember to update the call count.
>> Nevertheless, I think that it is right thing to hold call count in the
>> same file, where calls are implemented. This increases chances that 
>> call count will be held in sync.
> 
> So you are suggesting to implement a function? If so, that's a no-go 
> from my side.
I'm not insist on function if you can propose alternative.
But why you are against getter function in the first place?

I wanted to propose another approach: define this call count in
vpsci.h, but there is no vpsci.h, instead you use psci.h, which is 
confusing in itself.

>>
>>> Also, based on the recent SMCCC update (2.1.5 [1]):
>>> "The General Service Queries for SMCCC call ranges are described in 
>>> SMCCC section 6.2. These functions are not always well suited to 
>>> firmware that is integrated with multiple sub-services being combined 
>>> into one service range. For example, PSCI and SDEI in the Standard 
>>> Service range. In particular, the ‘call count’ and ‘revision’ 
>>> functions do not provide useful information to the caller when 
>>> multiple functions are provided. As a result, these are not widely 
>>> used to identify firmware services."
>>>
>>> So I would not worry that much if the function count is not sync in 
>>> the future. BTW, it is already wrong in Xen 4.10. For standard 
>>> service, we don't implement 13 but 52.
>> 2.1.5 deprecates general service queries in SMCCC 1.1. So, either we 
>> upgradeSMCCC in Xen to 1.1 or we should be conform with SMCCC 1.0 and 
>> should return right number of functions, including changes in Xen 4.10 
>> that you mentioned.
>>
>> BTW, personally I'm not happy with call UID and call version 
>> deprecation. This makes impossible dynamic discovery of TEE, which was 
>> used in my TEE mediator patch series.
> 
> 2.1.5 only deprecates for Architecture service which is not implemented 
> for Xen at moment. Nothing is been said for the rest of them OP-TEE. The 
> documentation only acknowledge the fact it is difficult for some service 
> (such as SSSC) to infer information from that.
Ah, sorry. I missed that point. But, anyways, if it is deprecated only 
for Architecture service, then all other services still should provide 
right responses to standard queries.

> 
> I looked at some implementation of SMCCC and those calls are either not 
> handled or the number are not correct.
I agree that *some* implementations can not conform to SMCCC.But, I 
thought you want Xen to follow standards as close as possible...

> Anyway, I have a SMCCC 1.1 patch series coming up soon.
That would be great.

-- 
Volodymyr Babchuk

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

On 30 January 2018 at 19:35, Volodymyr Babchuk
<volodymyr_babchuk@epam.com> wrote:
>
>
> On 30.01.18 20:44, Julien Grall wrote:
>>
>>
>>
>> On 30/01/18 18:28, Volodymyr Babchuk wrote:
>>>
>>> Hi Julien,
>>>
>>> On 30.01.18 20:01, Julien Grall wrote:
>>>>
>>>>
>>>>
>>>> On 26/01/18 18:27, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi,
>>>>
>>>>
>>>> Hi Volodymyr,
>>>>
>>>>> On 26.01.18 20:15, Julien Grall wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>>>>>>
>>>>>>> On 24.01.18 20:34, Julien Grall wrote:
>>>>>>>>
>>>>>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>>>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>>>>>> +    switch ( fid )
>>>>>>>>       {
>>>>>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>>>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>>>>>> -
>>>>>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>>>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff,
>>>>>>>> laff));
>>>>>>>> -        return true;
>>>>>>>> -    }
>>>>>>>> -
>>>>>>>>       case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>>>>>           return fill_function_call_count(regs,
>>>>>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>>>>>>
>>>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and
>>>>>>> use it there, what do you think?
>>>>>>
>>>>>>
>>>>>> Definitely not a function. It is a static number. But I can think of
>>>>>> separate the call count.
>>>>>
>>>>> Yep, separate call count for vPSCI and for SSSC itself would be a good
>>>>> thing.
>>>>
>>>>
>>>> Looking a bit more into it, this will not make a real improvement. This
>>>> will be equally difficult to remember to update the call count.
>>>
>>> Nevertheless, I think that it is right thing to hold call count in the
>>> same file, where calls are implemented. This increases chances that call
>>> count will be held in sync.
>>
>>
>> So you are suggesting to implement a function? If so, that's a no-go from
>> my side.
>
> I'm not insist on function if you can propose alternative.
> But why you are against getter function in the first place?

Because a function returning a const value is pretty dumb.

>
> I wanted to propose another approach: define this call count in
> vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing
> in itself.

I thought about vpsci.h, but basically you will have only 2 functions in it and
the number of PSCI calls. That's it.

So it is not going to help to update because the header will unlikely need to
change when adding new PSCI call.

[...]

>>
>> I looked at some implementation of SMCCC and those calls are either not
>> handled or the number are not correct.
>
> I agree that *some* implementations can not conform to SMCCC.But, I thought
> you want Xen to follow standards as close as possible...

It is not about cannot conform but only implements partially SMCCC. I could name
ATF for that (yes).

So it makes me more confident the call count is not something people seem to
care.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

Hi Julien,

On 31.01.18 00:06, Julien Grall wrote:
> On 30 January 2018 at 19:35, Volodymyr Babchuk
> <volodymyr_babchuk@epam.com> wrote:
>>
>>
>> On 30.01.18 20:44, Julien Grall wrote:
>>>
>>>
>>>
>>> On 30/01/18 18:28, Volodymyr Babchuk wrote:
>>>>
>>>> Hi Julien,
>>>>
>>>> On 30.01.18 20:01, Julien Grall wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 26/01/18 18:27, Volodymyr Babchuk wrote:
>>>>>>
>>>>>> Hi,
>>>>>
>>>>>
>>>>> Hi Volodymyr,
>>>>>
>>>>>> On 26.01.18 20:15, Julien Grall wrote:
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> On 26/01/18 18:09, Volodymyr Babchuk wrote:
>>>>>>>>
>>>>>>>> On 24.01.18 20:34, Julien Grall wrote:
>>>>>>>>>
>>>>>>>>> -    case PSCI_0_2_FN32(AFFINITY_INFO):
>>>>>>>>> -    case PSCI_0_2_FN64(AFFINITY_INFO):
>>>>>>>>> +    switch ( fid )
>>>>>>>>>        {
>>>>>>>>> -        register_t taff = PSCI_ARG(regs, 1);
>>>>>>>>> -        uint32_t laff = PSCI_ARG32(regs, 2);
>>>>>>>>> -
>>>>>>>>> -        perfc_incr(vpsci_cpu_affinity_info);
>>>>>>>>> -        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff,
>>>>>>>>> laff));
>>>>>>>>> -        return true;
>>>>>>>>> -    }
>>>>>>>>> -
>>>>>>>>>        case ARM_SMCCC_FUNC_CALL_COUNT(STANDARD):
>>>>>>>>>            return fill_function_call_count(regs,
>>>>>>>>> SSSC_SMCCC_FUNCTION_COUNT);
>>>>>>>>
>>>>>>>> Now definition SSSC_SMCCC_FUNCTION_COUNT depends on code in vscpi.c.
>>>>>>>> Maybe it is time to introduce function get_psci_0_2_fn_count() and
>>>>>>>> use it there, what do you think?
>>>>>>>
>>>>>>>
>>>>>>> Definitely not a function. It is a static number. But I can think of
>>>>>>> separate the call count.
>>>>>>
>>>>>> Yep, separate call count for vPSCI and for SSSC itself would be a good
>>>>>> thing.
>>>>>
>>>>>
>>>>> Looking a bit more into it, this will not make a real improvement. This
>>>>> will be equally difficult to remember to update the call count.
>>>>
>>>> Nevertheless, I think that it is right thing to hold call count in the
>>>> same file, where calls are implemented. This increases chances that call
>>>> count will be held in sync.
>>>
>>>
>>> So you are suggesting to implement a function? If so, that's a no-go from
>>> my side.
>>
>> I'm not insist on function if you can propose alternative.
>> But why you are against getter function in the first place?
> 
> Because a function returning a const value is pretty dumb.

I can't agree with you there. In my opinion - if it is needed for 
clarity - then it is fine.

Nevertheless, you have my opinion. It is up to you what to do with it.

>>
>> I wanted to propose another approach: define this call count in
>> vpsci.h, but there is no vpsci.h, instead you use psci.h, which is confusing
>> in itself.
> 
> I thought about vpsci.h, but basically you will have only 2 functions in it and
> the number of PSCI calls. That's it.

Is  this really a problem? It is quite natural to find declarations for 
something.c in something.h. By moving declaration into different file, 
you are hiding it from anyone who does not carry sacred knowledge (or 
use grep/cscope, yes).
And then, when people decide to extend something.c they continue to put 
declarations into inappropriate.h. Just look at processor.h as a good 
example. All functions it define are implemented either in traps.c or 
domain.c. But functions from processor.c are defined in procinfo.h.
I can tell for sure, that this confuses newbies.


> So it is not going to help to update because the header will unlikely need to
> change when adding new PSCI call.

Yes. But at least we can put comment above switch(fid):

/* Please don't forget to update call count in (v)psci.h */


> [...]
> 
>>>
>>> I looked at some implementation of SMCCC and those calls are either not
>>> handled or the number are not correct.
>>
>> I agree that *some* implementations can not conform to SMCCC.But, I thought
>> you want Xen to follow standards as close as possible...
> 
> It is not about cannot conform but only implements partially SMCCC. I could name
> ATF for that (yes).
> 
> So it makes me more confident the call count is not something people seem to
> care.

So, you propose to fix this only when something will trip over this?

-- 
Volodymyr Babchuk

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

Hi,

On 31/01/18 11:32, Volodymyr Babchuk wrote:
>> I thought about vpsci.h, but basically you will have only 2 functions 
>> in it and
>> the number of PSCI calls. That's it.
> 
> Is  this really a problem? It is quite natural to find declarations for 
> something.c in something.h. By moving declaration into different file, 
> you are hiding it from anyone who does not carry sacred knowledge (or 
> use grep/cscope, yes).
> And then, when people decide to extend something.c they continue to put 
> declarations into inappropriate.h. Just look at processor.h as a good 
> example. All functions it define are implemented either in traps.c or 
> domain.c. But functions from processor.c are defined in procinfo.h.
> I can tell for sure, that this confuses newbies.
> 
> 
>> So it is not going to help to update because the header will unlikely 
>> need to
>> change when adding new PSCI call.
> 
> Yes. But at least we can put comment above switch(fid):
> 
> /* Please don't forget to update call count in (v)psci.h */

See my answer on my own e-mail I sent few minutes ago. I said I will 
create a the vpsci.h.

> 
> 
>> [...]
>>
>>>>
>>>> I looked at some implementation of SMCCC and those calls are either not
>>>> handled or the number are not correct.
>>>
>>> I agree that *some* implementations can not conform to SMCCC.But, I 
>>> thought
>>> you want Xen to follow standards as close as possible...
>>
>> It is not about cannot conform but only implements partially SMCCC. I 
>> could name
>> ATF for that (yes).
>>
>> So it makes me more confident the call count is not something people 
>> seem to
>> care.
> 
> So, you propose to fix this only when something will trip over this?

No, I am just saying that it is not that important if we get it wrong. 
That number does not tell you anything. You can have 2 functions and 
still does not know where they are (it is not always possible to just 
probe a function ID because they may have side-effect).

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

Hi,

On 31.01.18 13:36, Julien Grall wrote:
> Hi,
> 
> On 31/01/18 11:32, Volodymyr Babchuk wrote:
>>> I thought about vpsci.h, but basically you will have only 2 functions 
>>> in it and
>>> the number of PSCI calls. That's it.
>>
>> Is  this really a problem? It is quite natural to find declarations 
>> for something.c in something.h. By moving declaration into different 
>> file, you are hiding it from anyone who does not carry sacred 
>> knowledge (or use grep/cscope, yes).
>> And then, when people decide to extend something.c they continue to 
>> put declarations into inappropriate.h. Just look at processor.h as a 
>> good example. All functions it define are implemented either in 
>> traps.c or domain.c. But functions from processor.c are defined in 
>> procinfo.h.
>> I can tell for sure, that this confuses newbies.
>>
>>
>>> So it is not going to help to update because the header will unlikely 
>>> need to
>>> change when adding new PSCI call.
>>
>> Yes. But at least we can put comment above switch(fid):
>>
>> /* Please don't forget to update call count in (v)psci.h */
> 
> See my answer on my own e-mail I sent few minutes ago. I said I will 
> create a the vpsci.h.
> 

I can't find it anywhere. I even tried Markmail. Would you please point 
it to me?

[...]

-- 
Volodymyr Babchuk

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Julien Grall]

On 31/01/18 14:29, Volodymyr Babchuk wrote:
> Hi,

Hi Volodymyr,

> On 31.01.18 13:36, Julien Grall wrote:
>> Hi,
>>
>> On 31/01/18 11:32, Volodymyr Babchuk wrote:
>>>> I thought about vpsci.h, but basically you will have only 2 
>>>> functions in it and
>>>> the number of PSCI calls. That's it.
>>>
>>> Is  this really a problem? It is quite natural to find declarations 
>>> for something.c in something.h. By moving declaration into different 
>>> file, you are hiding it from anyone who does not carry sacred 
>>> knowledge (or use grep/cscope, yes).
>>> And then, when people decide to extend something.c they continue to 
>>> put declarations into inappropriate.h. Just look at processor.h as a 
>>> good example. All functions it define are implemented either in 
>>> traps.c or domain.c. But functions from processor.c are defined in 
>>> procinfo.h.
>>> I can tell for sure, that this confuses newbies.
>>>
>>>
>>>> So it is not going to help to update because the header will 
>>>> unlikely need to
>>>> change when adding new PSCI call.
>>>
>>> Yes. But at least we can put comment above switch(fid):
>>>
>>> /* Please don't forget to update call count in (v)psci.h */
>>
>> See my answer on my own e-mail I sent few minutes ago. I said I will 
>> create a the vpsci.h.
>>
> 
> I can't find it anywhere. I even tried Markmail. Would you please point 
> it to me?

I wrote it somewhere and thought I sent but I can't find it :/. Sorry 
for that.

I was saying after some thought, I will create a header vpsic.h with 
number in it.

I will also add a comment as you suggested.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 3/3] xen/arm: vpsci: Move PSCI function dispatching from vsmc.c to vpsci.c

[Volodymyr Babchuk]

On 31.01.18 16:54, Julien Grall wrote:

>>> On 31/01/18 11:32, Volodymyr Babchuk wrote:
>>>>> I thought about vpsci.h, but basically you will have only 2 
>>>>> functions in it and
>>>>> the number of PSCI calls. That's it.
>>>>
>>>> Is  this really a problem? It is quite natural to find declarations 
>>>> for something.c in something.h. By moving declaration into different 
>>>> file, you are hiding it from anyone who does not carry sacred 
>>>> knowledge (or use grep/cscope, yes).
>>>> And then, when people decide to extend something.c they continue to 
>>>> put declarations into inappropriate.h. Just look at processor.h as a 
>>>> good example. All functions it define are implemented either in 
>>>> traps.c or domain.c. But functions from processor.c are defined in 
>>>> procinfo.h.
>>>> I can tell for sure, that this confuses newbies.
>>>>
>>>>
>>>>> So it is not going to help to update because the header will 
>>>>> unlikely need to
>>>>> change when adding new PSCI call.
>>>>
>>>> Yes. But at least we can put comment above switch(fid):
>>>>
>>>> /* Please don't forget to update call count in (v)psci.h */
>>>
>>> See my answer on my own e-mail I sent few minutes ago. I said I will 
>>> create a the vpsci.h.
>>>
>>
>> I can't find it anywhere. I even tried Markmail. Would you please 
>> point it to me?
> 
> I wrote it somewhere and thought I sent but I can't find it :/. Sorry 
> for that.
> 
Ah, I see. It is okay.

> I was saying after some thought, I will create a header vpsic.h with 
> number in it.
> 
> I will also add a comment as you suggested.

Thanks. I'm fine with this.



-- 
Volodymyr Babchuk

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