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

[Julien Grall <julien.grall@xxxxxxx>]

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