Re: [Xen-devel] [PATCH v4 05/11] arm: add SMCCC protocol definitions.

2017-09-13 Thread Julien Grall 



On 08/28/2017 09:28 PM, Volodymyr Babchuk wrote:

Hi Julien,


Hi,



On 24.08.17 18:00, Julien Grall wrote:

Hi Volodymyr,

Title: No need for the full stop.

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

This patch adds generic definitions used in ARM SMC call convention.
Those definitions was taken from linux header arm-smccc.h, extended
and formatted according to XEN coding style.

They can be used by both SMCCC clients (like PSCI) and by SMCCC
servers (like vPSCI or upcoming generic SMCCC handler).

Signed-off-by: Volodymyr Babchuk 
---
 xen/include/asm-arm/smccc.h | 92 
+

 1 file changed, 92 insertions(+)
 create mode 100644 xen/include/asm-arm/smccc.h

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 000..67da3fb
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015, Linaro Limited


Linaro? Where does this code come from?

 From the linux kernel. I think, I mentioned that in previous comments.
But this code was extended by me. And now there will be more changes.
Should I add remark about code origin somewhere?


Yes, the origin of code (repo + commit ID) + changes you made at least 
in the commit message.


Cheers,

--
Julien Grall

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

Re: [Xen-devel] [PATCH v4 05/11] arm: add SMCCC protocol definitions.

2017-08-28 Thread Volodymyr Babchuk 

Hi Julien,

On 24.08.17 18:00, Julien Grall wrote:

Hi Volodymyr,

Title: No need for the full stop.

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

This patch adds generic definitions used in ARM SMC call convention.
Those definitions was taken from linux header arm-smccc.h, extended
and formatted according to XEN coding style.

They can be used by both SMCCC clients (like PSCI) and by SMCCC
servers (like vPSCI or upcoming generic SMCCC handler).

Signed-off-by: Volodymyr Babchuk 
---
 xen/include/asm-arm/smccc.h | 92 
+

 1 file changed, 92 insertions(+)
 create mode 100644 xen/include/asm-arm/smccc.h

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 000..67da3fb
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015, Linaro Limited


Linaro? Where does this code come from?

From the linux kernel. I think, I mentioned that in previous comments.
But this code was extended by me. And now there will be more changes.
Should I add remark about code origin somewhere?


+ * Copyright (c) 2017, EPAM Systems
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __ASM_ARM_SMCCC_H__
+#define __ASM_ARM_SMCCC_H__
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ */
+
+#define ARM_SMCCC_STD_CALL  0U
+#define ARM_SMCCC_FAST_CALL 1U
+#define ARM_SMCCC_TYPE_SHIFT31
+
+#define ARM_SMCCC_SMC_320U
+#define ARM_SMCCC_SMC_641U


I am not sure to understand why you embed SMC in the name? The 
convention is SMC32/HVC32. So I would name


ARM_SMCCC_CONV_{32,64}


+#define ARM_SMCCC_CALL_CONV_SHIFT   30


Also, I would rename to ARM_SMCCC_CONV to fit with the value above.


+
+#define ARM_SMCCC_OWNER_MASK0x3F


Missing U.


+#define ARM_SMCCC_OWNER_SHIFT   24
+
+#define ARM_SMCCC_FUNC_MASK 0x
+
+/* Check if this is fast call */
+#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
+((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+
+/* Check if this is 64 bit call  */
+#define 
ARM_SMCCC_IS_64(smc_val)\

+((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+
+/* Get function number from function identifier */
+#define ARM_SMCCC_FUNC_NUM(smc_val) ((smc_val) & 
ARM_SMCCC_FUNC_MASK)

+
+/* Get service owner number from function identifier */
+#define 
ARM_SMCCC_OWNER_NUM(smc_val)\

+(((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)


Can we please use static inline helper for the 4 macros above?


+
+/*
+ * Construct function identifier from call type (fast or standard),
+ * calling convention (32 or 64 bit), service owner and function number
+ */
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, 
func_num)   \
+(((type) << ARM_SMCCC_TYPE_SHIFT) 
| \
+ ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) 
|  \
+ (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) 
|  \

+ ((func_num) & ARM_SMCCC_FUNC_MASK))


The indentation looks wrong here. Also I think the (& *MASK) is not 
necessary here. It would be wrong by the user to pass a wrong value and 
even with the masking you would end up to wrong result.


If you are really worry of wrong result, then you should use 
BUILD_BUG_ON()/BUG_ON().



+
+/* List of known service owners */
+#define ARM_SMCCC_OWNER_ARCH0
+#define ARM_SMCCC_OWNER_CPU 1
+#define ARM_SMCCC_OWNER_SIP 2
+#define ARM_SMCCC_OWNER_OEM 3
+#define ARM_SMCCC_OWNER_STANDARD4
+#define ARM_SMCCC_OWNER_HYPERVISOR  5
+#define ARM_SMCCC_OWNER_TRUSTED_APP 48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
+#define ARM_SMCCC_OWNER_TRUSTED_OS  50
+#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_REVISION0xFF03
+
+/* Only one error code defined in SMCCC */
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+
+#endif  /* __ASM_ARM_SMCCC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */



Cheers,

Re: [Xen-devel] [PATCH v4 05/11] arm: add SMCCC protocol definitions.

2017-08-24 Thread Julien Grall 

Hi Volodymyr,

Title: No need for the full stop.

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

This patch adds generic definitions used in ARM SMC call convention.
Those definitions was taken from linux header arm-smccc.h, extended
and formatted according to XEN coding style.

They can be used by both SMCCC clients (like PSCI) and by SMCCC
servers (like vPSCI or upcoming generic SMCCC handler).

Signed-off-by: Volodymyr Babchuk 
---
 xen/include/asm-arm/smccc.h | 92 +
 1 file changed, 92 insertions(+)
 create mode 100644 xen/include/asm-arm/smccc.h

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 000..67da3fb
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015, Linaro Limited


Linaro? Where does this code come from?


+ * Copyright (c) 2017, EPAM Systems
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __ASM_ARM_SMCCC_H__
+#define __ASM_ARM_SMCCC_H__
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ */
+
+#define ARM_SMCCC_STD_CALL  0U
+#define ARM_SMCCC_FAST_CALL 1U
+#define ARM_SMCCC_TYPE_SHIFT31
+
+#define ARM_SMCCC_SMC_320U
+#define ARM_SMCCC_SMC_641U


I am not sure to understand why you embed SMC in the name? The 
convention is SMC32/HVC32. So I would name


ARM_SMCCC_CONV_{32,64}


+#define ARM_SMCCC_CALL_CONV_SHIFT   30


Also, I would rename to ARM_SMCCC_CONV to fit with the value above.


+
+#define ARM_SMCCC_OWNER_MASK0x3F


Missing U.


+#define ARM_SMCCC_OWNER_SHIFT   24
+
+#define ARM_SMCCC_FUNC_MASK 0x
+
+/* Check if this is fast call */
+#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
+((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+
+/* Check if this is 64 bit call  */
+#define ARM_SMCCC_IS_64(smc_val)\
+((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+
+/* Get function number from function identifier */
+#define ARM_SMCCC_FUNC_NUM(smc_val) ((smc_val) & ARM_SMCCC_FUNC_MASK)
+
+/* Get service owner number from function identifier */
+#define ARM_SMCCC_OWNER_NUM(smc_val)\
+(((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)


Can we please use static inline helper for the 4 macros above?


+
+/*
+ * Construct function identifier from call type (fast or standard),
+ * calling convention (32 or 64 bit), service owner and function number
+ */
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
+(((type) << ARM_SMCCC_TYPE_SHIFT) | \
+ ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |  \
+ (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |  \
+ ((func_num) & ARM_SMCCC_FUNC_MASK))


The indentation looks wrong here. Also I think the (& *MASK) is not 
necessary here. It would be wrong by the user to pass a wrong value and 
even with the masking you would end up to wrong result.


If you are really worry of wrong result, then you should use 
BUILD_BUG_ON()/BUG_ON().



+
+/* List of known service owners */
+#define ARM_SMCCC_OWNER_ARCH0
+#define ARM_SMCCC_OWNER_CPU 1
+#define ARM_SMCCC_OWNER_SIP 2
+#define ARM_SMCCC_OWNER_OEM 3
+#define ARM_SMCCC_OWNER_STANDARD4
+#define ARM_SMCCC_OWNER_HYPERVISOR  5
+#define ARM_SMCCC_OWNER_TRUSTED_APP 48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
+#define ARM_SMCCC_OWNER_TRUSTED_OS  50
+#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_REVISION0xFF03
+
+/* Only one error code defined in SMCCC */
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+
+#endif  /* __ASM_ARM_SMCCC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */



Cheers,

--
Julien Grall

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