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

[Volodymyr Babchuk <volodymyr_babchuk@xxxxxxxx>]

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@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?
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,
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel