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

2018-02-12 Thread Julien Grall 



On 12/02/18 14:43, Volodymyr Babchuk wrote:

Hi Julien,

On 09.02.18 19:09, Julien Grall wrote:



On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:

Julien,


On 08.02.18 21:21, Julien Grall wrote:

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

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

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c


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

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


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


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


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


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



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


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


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


Cheers,

--
Julien Grall

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

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

2018-02-12 Thread Volodymyr Babchuk 

Hi Julien,

On 09.02.18 19:09, Julien Grall wrote:



On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:

Julien,


On 08.02.18 21:21, Julien Grall wrote:

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

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

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c


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

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


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


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


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


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





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


And this variable actually is not related to PSCI.


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



See my comments above :)

--
Volodymyr Babchuk

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

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

2018-02-09 Thread Julien Grall 



On 02/09/2018 05:04 PM, Volodymyr Babchuk wrote:

Julien,


On 08.02.18 21:21, Julien Grall wrote:

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

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

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c


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

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


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





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


And this variable actually is not related to PSCI.


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


  static uint32_t psci_cpu_on_nr;
@@ -57,6 +58,14 @@ void call_psci_system_reset(void)
  call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
  }
+static int __init psci_features(uint32_t psci_func_id)
+{
+    if ( psci_ver < PSCI_VERSION(1, 0) )
+    return PSCI_NOT_SUPPORTED;
+
+    return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
+}
+
  int __init psci_is_smc_method(const struct dt_device_node *psci)
  {
  int ret;
@@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct 
dt_device_node *psci)

  return 0;
  }
+static void __init psci_init_smccc(void)
+{
+    /* PSCI is using at least SMCC 1.0 calling convention. */
+    smccc_ver = ARM_SMCCC_VERSION_1_0;
+
+    if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
+    {
+    uint32_t ret;
+
+    ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
+    if ( ret != ARM_SMCCC_NOT_SUPPORTED )
+    smccc_ver = ret;
+    }
+
+    printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
+   SMCCC_VERSION_MAJOR(smccc_ver), 
SMCCC_VERSION_MINOR(smccc_ver));

+}
+
  int __init psci_init_0_1(void)
  {
  int ret;
@@ -173,7 +200,12 @@ int __init psci_init(void)
  if ( ret )
  ret = psci_init_0_1();
-    return ret;
+    if ( ret )
+    return ret;
+
+    psci_init_smccc();
+
+    return 0;
  }
  /*
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index caa2c9cc1b..bc067892c7 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -52,6 +52,8 @@
  #ifndef __ASSEMBLY__
+extern uint32_t smccc_ver;
+
  /* Check if this is fast call. */
  static inline bool smccc_is_fast_call(register_t funcid)
  {
@@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t 
funcid)

    ARM_SMCCC_OWNER_ARCH, \
    0x8000)
-/* Only one error code defined in SMCCC */
+/* SMCCC error codes */
  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+#define ARM_SMCCC_NOT_SUPPORTED (-1)


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


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


Will do.



  /* SMCCC function identifier range which is reserved for existing 
APIs */

  #define ARM_SMCCC_RESERVED_RANGE_START  0x0





Cheers,

--
Julien Grall

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

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

2018-02-09 Thread Volodymyr Babchuk 

Julien,


On 08.02.18 21:21, Julien Grall wrote:

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

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

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c


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


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


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

+uint32_t smccc_ver;


And this variable actually is not related to PSCI.
  
  static uint32_t psci_cpu_on_nr;
  
@@ -57,6 +58,14 @@ void call_psci_system_reset(void)

  call_smc(PSCI_0_2_FN32_SYSTEM_RESET, 0, 0, 0);
  }
  
+static int __init psci_features(uint32_t psci_func_id)

+{
+if ( psci_ver < PSCI_VERSION(1, 0) )
+return PSCI_NOT_SUPPORTED;
+
+return call_smc(PSCI_1_0_FN32_PSCI_FEATURES, psci_func_id, 0, 0);
+}
+
  int __init psci_is_smc_method(const struct dt_device_node *psci)
  {
  int ret;
@@ -82,6 +91,24 @@ int __init psci_is_smc_method(const struct dt_device_node 
*psci)
  return 0;
  }
  
+static void __init psci_init_smccc(void)

+{
+/* PSCI is using at least SMCC 1.0 calling convention. */
+smccc_ver = ARM_SMCCC_VERSION_1_0;
+
+if ( psci_features(ARM_SMCCC_VERSION_FID) != PSCI_NOT_SUPPORTED )
+{
+uint32_t ret;
+
+ret = call_smc(ARM_SMCCC_VERSION_FID, 0, 0, 0);
+if ( ret != ARM_SMCCC_NOT_SUPPORTED )
+smccc_ver = ret;
+}
+
+printk(XENLOG_INFO "Using SMC Calling Convention v%u.%u\n",
+   SMCCC_VERSION_MAJOR(smccc_ver), SMCCC_VERSION_MINOR(smccc_ver));
+}
+
  int __init psci_init_0_1(void)
  {
  int ret;
@@ -173,7 +200,12 @@ int __init psci_init(void)
  if ( ret )
  ret = psci_init_0_1();
  
-return ret;

+if ( ret )
+return ret;
+
+psci_init_smccc();
+
+return 0;
  }
  
  /*

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index caa2c9cc1b..bc067892c7 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -52,6 +52,8 @@
  
  #ifndef __ASSEMBLY__
  
+extern uint32_t smccc_ver;

+
  /* Check if this is fast call. */
  static inline bool smccc_is_fast_call(register_t funcid)
  {
@@ -137,8 +139,9 @@ static inline uint32_t smccc_get_owner(register_t funcid)
ARM_SMCCC_OWNER_ARCH, \
0x8000)
  
-/* Only one error code defined in SMCCC */

+/* SMCCC error codes */
  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+#define ARM_SMCCC_NOT_SUPPORTED (-1)


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


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


  
  /* SMCCC function identifier range which is reserved for existing APIs */

  #define ARM_SMCCC_RESERVED_RANGE_START  0x0



--
Volodymyr Babchuk

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

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

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

Signed-off-by: Julien Grall 

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

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


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