diff mbox

[v4,05/11] arm: add SMCCC protocol definitions.

Message ID 1503347275-13039-6-git-send-email-volodymyr_babchuk@epam.com (mailing list archive)
State New, archived
Headers show

Commit Message

Volodymyr Babchuk Aug. 21, 2017, 8:27 p.m. UTC 
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@epam.com>
---
 xen/include/asm-arm/smccc.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 xen/include/asm-arm/smccc.h

Comments

Julien Grall Aug. 24, 2017, 3 p.m. UTC | #1 
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@epam.com>
> ---
>  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,
Volodymyr Babchuk Aug. 28, 2017, 8:28 p.m. UTC | #2 
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 <volodymyr_babchuk@epam.com>
>> ---
>>  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?
 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_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 Sept. 13, 2017, 10:04 a.m. UTC | #3 
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 <volodymyr_babchuk@epam.com>
>>> ---
>>>  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?
>  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,
diff mbox

Patch

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
+ * 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
+#define ARM_SMCCC_CALL_CONV_SHIFT       30
+
+#define ARM_SMCCC_OWNER_MASK            0x3F
+#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)
+
+/*
+ * 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))
+
+/* 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
+ */