[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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 <volodymyr_babchuk@xxxxxxxx>
---
 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 0000000..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_SHIFT            31
+
+#define ARM_SMCCC_SMC_32                0U
+#define ARM_SMCCC_SMC_64                1U

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_MASK            0x3F

Missing U.

+#define ARM_SMCCC_OWNER_SHIFT           24
+
+#define ARM_SMCCC_FUNC_MASK             0xFFFF
+
+/* 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_ARCH            0
+#define ARM_SMCCC_OWNER_CPU             1
+#define ARM_SMCCC_OWNER_SIP             2
+#define ARM_SMCCC_OWNER_OEM             3
+#define ARM_SMCCC_OWNER_STANDARD        4
+#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_REVISION    0xFF03
+
+/* 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.