[1/2] arm: smccc: handle SMCs/HVCs according to SMCCC

Commit Message

Volodymyr Babchuk June 14, 2017, 2:10 p.m. UTC

SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
SMCCC states that both HVC and SMC are valid conduits to call to a different
firmware functions. Thus, for example PSCI calls can be made both by
SMC or HVC. Also SMCCC defines function number coding for such calls.
Besides functional calls there are query calls, which allows underling
OS determine version, UID and number of functions provided by service
provider.

This patch adds new file `smccc.c`, which handles both generic SMCs
and HVC according to SMC. At this moment it implements only one
service: Standard Hypervisor Service.

Standard Hypervisor Service only supports query calls, so caller can
ask about hypervisor UID and determine that it is XEN running.

This change allows more generic handling for SMCs and HVCs and it can
be easily extended to support new services and functions.

Signed-off-by: Volodymyr Babchuk <[e-mail]>
Reviewed-by: Oleksandr Andrushchenko <[e-mail]>
Reviewed-by: Oleksandr Tyshchenko <[e-mail]>
---
 xen/arch/arm/Makefile       |  1 +
 xen/arch/arm/smccc.c        | 96 +++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c        | 10 ++++-
 xen/include/asm-arm/smccc.h | 89 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 194 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/arm/smccc.c
 create mode 100644 xen/include/asm-arm/smccc.h

Comments

Julien Grall June 15, 2017, 10:48 a.m. UTC | #1

Hi Volodymyr,

On 14/06/17 15:10, Volodymyr Babchuk wrote:
> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> SMCCC states that both HVC and SMC are valid conduits to call to a different
> firmware functions. Thus, for example PSCI calls can be made both by
> SMC or HVC. Also SMCCC defines function number coding for such calls.
> Besides functional calls there are query calls, which allows underling
> OS determine version, UID and number of functions provided by service
> provider.
>
> This patch adds new file `smccc.c`, which handles both generic SMCs
> and HVC according to SMC. At this moment it implements only one
> service: Standard Hypervisor Service.
>
> Standard Hypervisor Service only supports query calls, so caller can
> ask about hypervisor UID and determine that it is XEN running.
>
> This change allows more generic handling for SMCs and HVCs and it can
> be easily extended to support new services and functions.
>
> Signed-off-by: Volodymyr Babchuk <[e-mail]>
> Reviewed-by: Oleksandr Andrushchenko <[e-mail]>
> Reviewed-by: Oleksandr Tyshchenko <[e-mail]>
> ---
>  xen/arch/arm/Makefile       |  1 +
>  xen/arch/arm/smccc.c        | 96 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c        | 10 ++++-
>  xen/include/asm-arm/smccc.h | 89 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 194 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/arm/smccc.c
>  create mode 100644 xen/include/asm-arm/smccc.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..b8728cf 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,6 +39,7 @@ obj-y += psci.o
>  obj-y += setup.o
>  obj-y += shutdown.o
>  obj-y += smc.o
> +obj-y += smccc.o
>  obj-y += smp.o
>  obj-y += smpboot.o
>  obj-y += sysctl.o
> diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
> new file mode 100644
> index 0000000..5d10964
> --- /dev/null
> +++ b/xen/arch/arm/smccc.c

I would name this file vsmccc.c to show it is about virtual SMC. Also, I 
would have expected pretty everyone to use the SMCC, so I would even 
name the file vsmc.c

> @@ -0,0 +1,96 @@
> +/*
> + * xen/arch/arm/smccc.c
> + *
> + * Generic handler for SMC and HVC calls according to
> + * ARM SMC callling convention

s/callling/calling/

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.

I know that some of the other headers are wrong about the GPL license. 
But Xen is GPLv2 only. Please update the copyright accordingly. I.e:

  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.

> + *
> + * 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.
> + */
> +
> +
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/perfc.h>

Why this is included here? You don't use it.

> +/* Need to include xen/sched.h before asm/domain.h or it breaks build*/

xen/sched.h will include asm/domain.h. So no need to include the latter 
here.

> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <xen/types.h>
> +#include <asm/domain.h>
> +#include <asm/psci.h>

You don't use this header here.

> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +
> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> +                                    0x9a, 0xcf, 0x79, 0xd1, \
> +                                    0x8d, 0xde, 0xe6, 0x67)

Please mention that this value was generated. This would avoid to wonder 
where this value comes from.

> +
> +/*
> + * We can't use XEN version here:
> + * Major revision should change every time SMC/HVC function is removed.
> + * Minor revision should change every time SMC/HVC function is added.
> + * So, it is SMCCC protocol revision code, not XEN version

It would be nice to say this is a requirement of the spec. Also missing 
full stop.

> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1

I first thought the revision was 0.1.3 and was about to ask why. But 
then noticed XEN_SMCC_FUNCTION_COUNT is not part of the revision.

So please add a newline for clarity.

> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +/* SMCCC interface for hypervisor. Tell about self */

Tell about itself. + missing full stop.

> +static bool handle_hypervisor(struct cpu_user_regs *regs, const union hsr hsr)

hsr is already part of regs.

> +{
> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
> +        return true;
> +    }
> +    return false;
> +}
> +
> +/**
> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
> + */
> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)

hsr is already part of regs.

> +{
> +    bool handled = false;
> +

I am a bit surprised, I don't see any check to prevent a 32-bit guest to 
use SMC64 call.

Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
compliant SMC calls should have the immediate value of zero.


> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_OWNER_HYPERVISOR:
> +        handled = handle_hypervisor(regs, hsr);
> +        break;
> +    }
> +
> +    if ( !handled )
> +    {
> +        printk("Uhandled SMC/HVC: %08"PRIregister"\n", get_user_reg(regs, 0));

s/Uhandled/Unhandled/

Also, please don't use printk. They are not ratelimited. You want to use 
gprintk here.

> +        /* Inform caller that function is not supported */
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +    }
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6cf9ee7..2d0b058 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -44,6 +44,7 @@
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
>  #include <asm/monitor.h>
> +#include <asm/smccc.h>
>
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>  {

I think it would make sense to push this function in the new file.

Also, I was expecting some change in the HVC path as you say that this 
will be used for both HVC and SMC.

>      int rc = 0;
>
> +    /* Let monitor to handle the call */
>      if ( current->domain->arch.monitor.privileged_call_enabled )
>          rc = monitor_smc();
>
> -    if ( rc != 1 )
> -        inject_undef_exception(regs, hsr);
> +    if ( rc == 1 )
> +        return;

It would be nice to explain both in the commit message and the code that 
if monitor is enabled, then all SMCs will be forwarded to the monitor app.

> +
> +    /* Use standard routines to handle the call */
> +    smccc_handle_call(regs, hsr);

It is allowed by the architecture to trap to conditional SMC 
instructions that fail their condition code check (see G1-4435 in ARM 
DDI 0487B.a). So you want to check why it has trapped before calling the 
handler.

> +    advance_pc(regs, hsr);
>  }
>
>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> new file mode 100644
> index 0000000..9342d5e
> --- /dev/null
> +++ b/xen/include/asm-arm/smccc.h
> @@ -0,0 +1,89 @@
> +/*
> + * 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_

It should be __ASM_ARM_SMCC_H__

> +#define __ASM_ARM_SMCCC_H_

Ditto.

> +
> +#include <xen/types.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		0

Is this file coming from Linux? If so, it should be mention. If not, 
please use soft tab and not hard tab. This is valid in all this file.

> +#define ARM_SMCCC_FAST_CALL		1
> +#define ARM_SMCCC_TYPE_SHIFT		31
> +
> +#define ARM_SMCCC_SMC_32		0
> +#define ARM_SMCCC_SMC_64		1
> +#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
> +
> +#define ARM_SMCCC_IS_FAST_CALL(smc_val)	\
> +	((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
> +#define ARM_SMCCC_IS_64(smc_val) \
> +	((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
> +#define ARM_SMCCC_FUNC_NUM(smc_val)	((smc_val) & ARM_SMCCC_FUNC_MASK)
> +#define ARM_SMCCC_OWNER_NUM(smc_val) \
> +	(((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
> +
> +#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))

I would appreciate a bit more documentation of those macros as they are 
a bit difficult to parse. Also some newline would be nice for clarity.

> +
> +#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
> +
> +#define ARM_SMCCC_FUNC_CALL_COUNT	0xFF00
> +#define ARM_SMCCC_FUNC_CALL_UID		0xFF01
> +#define ARM_SMCCC_FUNC_CALL_REVISION	0xFF03
> +
> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION	(-1)
> +
> +typedef struct {
> +	uint32_t a[4];
> +} arm_smccc_uid;
> +
> +#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)		\
> +	((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),			\
> +			   ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
> +			   ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
> +
> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
> +
> +#endif

#endif /* __ASM_ARM_SMCC_H__

> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
>

Cheers,

Stefano Stabellini June 16, 2017, 9:24 p.m. UTC | #2

On Wed, 14 Jun 2017, Volodymyr Babchuk wrote:
> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> SMCCC states that both HVC and SMC are valid conduits to call to a different
> firmware functions. Thus, for example PSCI calls can be made both by
> SMC or HVC. Also SMCCC defines function number coding for such calls.
> Besides functional calls there are query calls, which allows underling
> OS determine version, UID and number of functions provided by service
> provider.
> 
> This patch adds new file `smccc.c`, which handles both generic SMCs
> and HVC according to SMC. At this moment it implements only one
> service: Standard Hypervisor Service.
> 
> Standard Hypervisor Service only supports query calls, so caller can
> ask about hypervisor UID and determine that it is XEN running.
> 
> This change allows more generic handling for SMCs and HVCs and it can
> be easily extended to support new services and functions.
> 
> Signed-off-by: Volodymyr Babchuk <[e-mail]>
> Reviewed-by: Oleksandr Andrushchenko <[e-mail]>
> Reviewed-by: Oleksandr Tyshchenko <[e-mail]>
> ---
>  xen/arch/arm/Makefile       |  1 +
>  xen/arch/arm/smccc.c        | 96 +++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c        | 10 ++++-
>  xen/include/asm-arm/smccc.h | 89 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 194 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/arm/smccc.c
>  create mode 100644 xen/include/asm-arm/smccc.h
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..b8728cf 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -39,6 +39,7 @@ obj-y += psci.o
>  obj-y += setup.o
>  obj-y += shutdown.o
>  obj-y += smc.o
> +obj-y += smccc.o
>  obj-y += smp.o
>  obj-y += smpboot.o
>  obj-y += sysctl.o
> diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
> new file mode 100644
> index 0000000..5d10964
> --- /dev/null
> +++ b/xen/arch/arm/smccc.c
> @@ -0,0 +1,96 @@
> +/*
> + * xen/arch/arm/smccc.c
> + *
> + * Generic handler for SMC and HVC calls according to
> + * ARM SMC callling convention
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * 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.
> + */
> +
> +
> +#include <xen/config.h>
> +#include <xen/lib.h>
> +#include <xen/perfc.h>
> +/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
> +#include <xen/sched.h>
> +#include <xen/stdbool.h>
> +#include <xen/types.h>
> +#include <asm/domain.h>
> +#include <asm/psci.h>
> +#include <asm/smccc.h>
> +#include <asm/regs.h>
> +
> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> +                                    0x9a, 0xcf, 0x79, 0xd1, \
> +                                    0x8d, 0xde, 0xe6, 0x67)
> +
> +/*
> + * We can't use XEN version here:
> + * Major revision should change every time SMC/HVC function is removed.
> + * Minor revision should change every time SMC/HVC function is added.
> + * So, it is SMCCC protocol revision code, not XEN version
> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1
> +#define XEN_SMCCC_FUNCTION_COUNT 3

Both ARM_SMCCC_UID, and XEN_SMCCC_MAJOR/MINOR_REVISION become part of the
Xen public ABI. Please explain in the commit message why you chose them
as indentifier, and add them to a separate new header file under
xen/include/public/arch-arm/ (because they are public).



> +/* SMCCC interface for hypervisor. Tell about self */
> +static bool handle_hypervisor(struct cpu_user_regs *regs, const union hsr hsr)
> +{
> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
> +        return true;
> +    }
> +    return false;
> +}

Volodymyr Babchuk June 19, 2017, 9:59 p.m. UTC | #3

Hello Julien,

Thank you for review. It is my first time, when I'm submitting patch to 
XEN, so I have some questions.

On 15.06.17 13:48, Julien Grall wrote:
> On 14/06/17 15:10, Volodymyr Babchuk wrote:
>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>> SMCCC states that both HVC and SMC are valid conduits to call to a 
>> different
>> firmware functions. Thus, for example PSCI calls can be made both by
>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>> Besides functional calls there are query calls, which allows underling
>> OS determine version, UID and number of functions provided by service
>> provider.
>>
>> This patch adds new file `smccc.c`, which handles both generic SMCs
>> and HVC according to SMC. At this moment it implements only one
>> service: Standard Hypervisor Service.
>>
>> Standard Hypervisor Service only supports query calls, so caller can
>> ask about hypervisor UID and determine that it is XEN running.
>>
>> This change allows more generic handling for SMCs and HVCs and it can
>> be easily extended to support new services and functions.
>>
>> Signed-off-by: Volodymyr Babchuk <[e-mail]>
>> Reviewed-by: Oleksandr Andrushchenko <[e-mail]>
>> Reviewed-by: Oleksandr Tyshchenko <[e-mail]>
>> ---
>>  xen/arch/arm/Makefile       |  1 +
>>  xen/arch/arm/smccc.c        | 96 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/traps.c        | 10 ++++-
>>  xen/include/asm-arm/smccc.h | 89 
>> +++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 194 insertions(+), 2 deletions(-)
>>  create mode 100644 xen/arch/arm/smccc.c
>>  create mode 100644 xen/include/asm-arm/smccc.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 49e1fb2..b8728cf 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -39,6 +39,7 @@ obj-y += psci.o
>>  obj-y += setup.o
>>  obj-y += shutdown.o
>>  obj-y += smc.o
>> +obj-y += smccc.o
>>  obj-y += smp.o
>>  obj-y += smpboot.o
>>  obj-y += sysctl.o
>> diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
>> new file mode 100644
>> index 0000000..5d10964
>> --- /dev/null
>> +++ b/xen/arch/arm/smccc.c
> 
> I would name this file vsmccc.c to show it is about virtual SMC. Also, I 
> would have expected pretty everyone to use the SMCC, so I would even 
> name the file vsmc.c
> 
>> @@ -0,0 +1,96 @@
>> +/*
>> + * xen/arch/arm/smccc.c
>> + *
>> + * Generic handler for SMC and HVC calls according to
>> + * ARM SMC callling convention
> 
> s/callling/calling/
> 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
> 
> I know that some of the other headers are wrong about the GPL license. 
> But Xen is GPLv2 only. Please update the copyright accordingly. I.e:
> 
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> 
>> + *
>> + * 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.
>> + */
>> +
>> +
>> +#include <xen/config.h>
>> +#include <xen/lib.h>
>> +#include <xen/perfc.h>
> 
> Why this is included here? You don't use it.
> 
>> +/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
> 
> xen/sched.h will include asm/domain.h. So no need to include the latter 
> here.
> 
>> +#include <xen/sched.h>
>> +#include <xen/stdbool.h>
>> +#include <xen/types.h>
>> +#include <asm/domain.h>
>> +#include <asm/psci.h>
> 
> You don't use this header here.
> 
>> +#include <asm/smccc.h>
>> +#include <asm/regs.h>
>> +
>> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
>> +                                    0x9a, 0xcf, 0x79, 0xd1, \
>> +                                    0x8d, 0xde, 0xe6, 0x67)
> 
> Please mention that this value was generated. This would avoid to wonder 
> where this value comes from.
> 
>> +
>> +/*
>> + * We can't use XEN version here:
>> + * Major revision should change every time SMC/HVC function is removed.
>> + * Minor revision should change every time SMC/HVC function is added.
>> + * So, it is SMCCC protocol revision code, not XEN version
> 
> It would be nice to say this is a requirement of the spec. Also missing 
> full stop.
> 
>> + */
>> +#define XEN_SMCCC_MAJOR_REVISION 0
>> +#define XEN_SMCCC_MINOR_REVISION 1
> 
> I first thought the revision was 0.1.3 and was about to ask why. But 
> then noticed XEN_SMCC_FUNCTION_COUNT is not part of the revision.
> 
> So please add a newline for clarity.
> 
>> +#define XEN_SMCCC_FUNCTION_COUNT 3
>> +
>> +/* SMCCC interface for hypervisor. Tell about self */
> 
> Tell about itself. + missing full stop.
> 
>> +static bool handle_hypervisor(struct cpu_user_regs *regs, const union 
>> hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_FUNC_CALL_COUNT:
>> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_UID:
>> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
>> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
>> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
>> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_REVISION:
>> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
>> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>> +/**
>> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>> + */
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    bool handled = false;
>> +
> 
> I am a bit surprised, I don't see any check to prevent a 32-bit guest to 
> use SMC64 call.
Should we return ARM_SMCCC_ERR_UNKNOWN_FUNCTION code in this case? Or 
inject undefined instruction?

> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
> compliant SMC calls should have the immediate value of zero.
Looks like HSR does not hold immediate value (as if in case of HVC/SVC). 
That means that I need to map memory at PC and decode instruction 
manually. It is a bit complex, I think. Should we do this?

> 
>> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_OWNER_HYPERVISOR:
>> +        handled = handle_hypervisor(regs, hsr);
>> +        break;
>> +    }
>> +
>> +    if ( !handled )
>> +    {
>> +        printk("Uhandled SMC/HVC: %08"PRIregister"\n", 
>> get_user_reg(regs, 0));
> 
> s/Uhandled/Unhandled/
> 
> Also, please don't use printk. They are not ratelimited. You want to use 
> gprintk here.
> 
>> +        /* Inform caller that function is not supported */
>> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..2d0b058 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/cpufeature.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/monitor.h>
>> +#include <asm/smccc.h>
>>
>>  #include "decode.h"
>>  #include "vtimer.h"
>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs 
>> *regs, const union hsr hsr)
>>  {
> 
> I think it would make sense to push this function in the new file.
> 
> Also, I was expecting some change in the HVC path as you say that this 
> will be used for both HVC and SMC.
> 
>>      int rc = 0;
>>
>> +    /* Let monitor to handle the call */
>>      if ( current->domain->arch.monitor.privileged_call_enabled )
>>          rc = monitor_smc();
>>
>> -    if ( rc != 1 )
>> -        inject_undef_exception(regs, hsr);
>> +    if ( rc == 1 )
>> +        return;
> 
> It would be nice to explain both in the commit message and the code that 
> if monitor is enabled, then all SMCs will be forwarded to the monitor app.
> 
>> +
>> +    /* Use standard routines to handle the call */
>> +    smccc_handle_call(regs, hsr);
> 
> It is allowed by the architecture to trap to conditional SMC 
> instructions that fail their condition code check (see G1-4435 in ARM 
> DDI 0487B.a). So you want to check why it has trapped before calling the 
> handler.
> 
>> +    advance_pc(regs, hsr);
>>  }
>>
>>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> new file mode 100644
>> index 0000000..9342d5e
>> --- /dev/null
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -0,0 +1,89 @@
>> +/*
>> + * 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_
> 
> It should be __ASM_ARM_SMCC_H__
> 
>> +#define __ASM_ARM_SMCCC_H_
> 
> Ditto.
> 
>> +
>> +#include <xen/types.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        0
> 
> Is this file coming from Linux? If so, it should be mention. If not, 
> please use soft tab and not hard tab. This is valid in all this file.Actually, part of this defines are from Linux, another defines was added 
by myself. What I should to in this case?

>> +#define ARM_SMCCC_FAST_CALL        1
>> +#define ARM_SMCCC_TYPE_SHIFT        31
>> +
>> +#define ARM_SMCCC_SMC_32        0
>> +#define ARM_SMCCC_SMC_64        1
>> +#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
>> +
>> +#define ARM_SMCCC_IS_FAST_CALL(smc_val)    \
>> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
>> +#define ARM_SMCCC_IS_64(smc_val) \
>> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
>> +#define ARM_SMCCC_FUNC_NUM(smc_val)    ((smc_val) & ARM_SMCCC_FUNC_MASK)
>> +#define ARM_SMCCC_OWNER_NUM(smc_val) \
>> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
>> +
>> +#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))
> 
> I would appreciate a bit more documentation of those macros as they are 
> a bit difficult to parse. Also some newline would be nice for clarity.
> 
>> +
>> +#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
>> +
>> +#define ARM_SMCCC_FUNC_CALL_COUNT    0xFF00
>> +#define ARM_SMCCC_FUNC_CALL_UID        0xFF01
>> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>> +
>> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION    (-1)
>> +
>> +typedef struct {
>> +    uint32_t a[4];
>> +} arm_smccc_uid;
>> +
>> +#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)        \
>> +    ((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),            \
>> +               ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
>> +               ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> +
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
>> +
>> +#endif
> 
> #endif /* __ASM_ARM_SMCC_H__
> 
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Cheers,
>

Julien Grall June 20, 2017, 11:55 a.m. UTC | #4

On 06/19/2017 10:59 PM, Volodymyr Babchuk wrote:
> Hello Julien,

Hi Volodymyr,

[...]

>>> +/**
>>> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>>> + */
>>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
>>
>> hsr is already part of regs.
>>
>>> +{
>>> +    bool handled = false;
>>> +
>>
>> I am a bit surprised, I don't see any check to prevent a 32-bit guest 
>> to use SMC64 call.
> Should we return ARM_SMCCC_ERR_UNKNOWN_FUNCTION code in this case? Or 
> inject undefined instruction?

We should follow what the spec says here. Per section 2.7 (ARM DEN 
0028B), you should return ARM_SMCC_ERR_UNKOWN_FUNCTION for SMC64/HVC64 
call from AArch32.

>> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
>> compliant SMC calls should have the immediate value of zero.
> Looks like HSR does not hold immediate value (as if in case of HVC/SVC). 
> That means that I need to map memory at PC and decode instruction 
> manually. It is a bit complex, I think. Should we do this?

Well, the immediate is present for the ISS encoding for exception from 
SMC executed in AArch64 state. So you can decode the immediate here.

For SMC executed in AArch32 state, indeed the immediate is not present.

I had a quick look at the arm trusted firmware. I haven't spot any 
decoding of the immediate from an instruction.

My main concern is any non-zero value are reserved. If they are used in 
the future, we may emulate them by mistake. So we should definitely 
handle it for AArch64. For AArch32, we could decode the instruction, but 
that would be time consuming for the benefits of future extension but 
add overhead on the SMC call. So I would just consider it as always 0 
with a suitable comment on top explaining why.

[...]

>>> +
>>> +#include <xen/types.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        0
>>
>> Is this file coming from Linux? If so, it should be mention. If not, 
>> please use soft tab and not hard tab. This is valid in all this 
>> file.Actually, part of this defines are from Linux, another defines 
>> was added 
> by myself. What I should to in this case?

Using Xen coding style always (see CODING_STYLE).

Cheers,

Volodymyr Babchuk June 22, 2017, 4:29 p.m. UTC | #5

Hi Julien,

On 15.06.17 13:48, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 14/06/17 15:10, Volodymyr Babchuk wrote:
>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>> SMCCC states that both HVC and SMC are valid conduits to call to a 
>> different
>> firmware functions. Thus, for example PSCI calls can be made both by
>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>> Besides functional calls there are query calls, which allows underling
>> OS determine version, UID and number of functions provided by service
>> provider.
>>
>> This patch adds new file `smccc.c`, which handles both generic SMCs
>> and HVC according to SMC. At this moment it implements only one
>> service: Standard Hypervisor Service.
>>
>> Standard Hypervisor Service only supports query calls, so caller can
>> ask about hypervisor UID and determine that it is XEN running.
>>
>> This change allows more generic handling for SMCs and HVCs and it can
>> be easily extended to support new services and functions.
>>
>> Signed-off-by: Volodymyr Babchuk <[e-mail]>
>> Reviewed-by: Oleksandr Andrushchenko <[e-mail]>
>> Reviewed-by: Oleksandr Tyshchenko <[e-mail]>
>> ---
>>  xen/arch/arm/Makefile       |  1 +
>>  xen/arch/arm/smccc.c        | 96 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/traps.c        | 10 ++++-
>>  xen/include/asm-arm/smccc.h | 89 
>> +++++++++++++++++++++++++++++++++++++++++
>>  4 files changed, 194 insertions(+), 2 deletions(-)
>>  create mode 100644 xen/arch/arm/smccc.c
>>  create mode 100644 xen/include/asm-arm/smccc.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 49e1fb2..b8728cf 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -39,6 +39,7 @@ obj-y += psci.o
>>  obj-y += setup.o
>>  obj-y += shutdown.o
>>  obj-y += smc.o
>> +obj-y += smccc.o
>>  obj-y += smp.o
>>  obj-y += smpboot.o
>>  obj-y += sysctl.o
>> diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
>> new file mode 100644
>> index 0000000..5d10964
>> --- /dev/null
>> +++ b/xen/arch/arm/smccc.c
> 
> I would name this file vsmccc.c to show it is about virtual SMC. Also, I 
> would have expected pretty everyone to use the SMCC, so I would even 
> name the file vsmc.c
> 
>> @@ -0,0 +1,96 @@
>> +/*
>> + * xen/arch/arm/smccc.c
>> + *
>> + * Generic handler for SMC and HVC calls according to
>> + * ARM SMC callling convention
> 
> s/callling/calling/
> 
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
> 
> I know that some of the other headers are wrong about the GPL license. 
> But Xen is GPLv2 only. Please update the copyright accordingly. I.e:
> 
>   * This program is free software; you can redistribute it and/or modify
>   * it under the terms of the GNU General Public License version 2 as
>   * published by the Free Software Foundation.
> 
>> + *
>> + * 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.
>> + */
>> +
>> +
>> +#include <xen/config.h>
>> +#include <xen/lib.h>
>> +#include <xen/perfc.h>
> 
> Why this is included here? You don't use it.
> 
>> +/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
> 
> xen/sched.h will include asm/domain.h. So no need to include the latter 
> here.
> 
>> +#include <xen/sched.h>
>> +#include <xen/stdbool.h>
>> +#include <xen/types.h>
>> +#include <asm/domain.h>
>> +#include <asm/psci.h>
> 
> You don't use this header here.
> 
>> +#include <asm/smccc.h>
>> +#include <asm/regs.h>
>> +
>> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
>> +                                    0x9a, 0xcf, 0x79, 0xd1, \
>> +                                    0x8d, 0xde, 0xe6, 0x67)
> 
> Please mention that this value was generated. This would avoid to wonder 
> where this value comes from.
> 
>> +
>> +/*
>> + * We can't use XEN version here:
>> + * Major revision should change every time SMC/HVC function is removed.
>> + * Minor revision should change every time SMC/HVC function is added.
>> + * So, it is SMCCC protocol revision code, not XEN version
> 
> It would be nice to say this is a requirement of the spec. Also missing 
> full stop.
> 
>> + */
>> +#define XEN_SMCCC_MAJOR_REVISION 0
>> +#define XEN_SMCCC_MINOR_REVISION 1
> 
> I first thought the revision was 0.1.3 and was about to ask why. But 
> then noticed XEN_SMCC_FUNCTION_COUNT is not part of the revision.
> 
> So please add a newline for clarity.
> 
>> +#define XEN_SMCCC_FUNCTION_COUNT 3
>> +
>> +/* SMCCC interface for hypervisor. Tell about self */
> 
> Tell about itself. + missing full stop.
> 
>> +static bool handle_hypervisor(struct cpu_user_regs *regs, const union 
>> hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_FUNC_CALL_COUNT:
>> +        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_UID:
>> +        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
>> +        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
>> +        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
>> +        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_REVISION:
>> +        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
>> +        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>> +/**
>> + * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>> + */
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
> 
> hsr is already part of regs.
> 
>> +{
>> +    bool handled = false;
>> +
> 
> I am a bit surprised, I don't see any check to prevent a 32-bit guest to 
> use SMC64 call.
> 
> Furthermore, based from the SMCCC spec (see 2.9 in ARM DEN 0028B), the 
> compliant SMC calls should have the immediate value of zero.
> 
> 
>> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
>> +    {
>> +    case ARM_SMCCC_OWNER_HYPERVISOR:
>> +        handled = handle_hypervisor(regs, hsr);
>> +        break;
>> +    }
>> +
>> +    if ( !handled )
>> +    {
>> +        printk("Uhandled SMC/HVC: %08"PRIregister"\n", 
>> get_user_reg(regs, 0));
> 
> s/Uhandled/Unhandled/
> 
> Also, please don't use printk. They are not ratelimited. You want to use 
> gprintk here.
> 
>> +        /* Inform caller that function is not supported */
>> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>> +    }
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 6cf9ee7..2d0b058 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -44,6 +44,7 @@
>>  #include <asm/cpufeature.h>
>>  #include <asm/flushtlb.h>
>>  #include <asm/monitor.h>
>> +#include <asm/smccc.h>
>>
>>  #include "decode.h"
>>  #include "vtimer.h"
>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs 
>> *regs, const union hsr hsr)
>>  {
> 
> I think it would make sense to push this function in the new file.
Unfortunately, I can't do this, because it uses local functions such as
inject_undef_exception() or advance_pc().

> Also, I was expecting some change in the HVC path as you say that this 
> will be used for both HVC and SMC.
Actually, I plan to use this particular function to handle only SMCs,
because it does SMC-specific tasks, such as calling to a monitor.

HVCs will be handled in different call path in the next patch,
because currently HVC callpath is used by PSCI code.

>>      int rc = 0;
>>
>> +    /* Let monitor to handle the call */
>>      if ( current->domain->arch.monitor.privileged_call_enabled )
>>          rc = monitor_smc();
>>
>> -    if ( rc != 1 )
>> -        inject_undef_exception(regs, hsr);
>> +    if ( rc == 1 )
>> +        return;
> 
> It would be nice to explain both in the commit message and the code that 
> if monitor is enabled, then all SMCs will be forwarded to the monitor app.
> 
>> +
>> +    /* Use standard routines to handle the call */
>> +    smccc_handle_call(regs, hsr);
> 
> It is allowed by the architecture to trap to conditional SMC 
> instructions that fail their condition code check (see G1-4435 in ARM 
> DDI 0487B.a). So you want to check why it has trapped before calling the 
> handler.
> 
>> +    advance_pc(regs, hsr);
>>  }
>>
>>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> new file mode 100644
>> index 0000000..9342d5e
>> --- /dev/null
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -0,0 +1,89 @@
>> +/*
>> + * 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_
> 
> It should be __ASM_ARM_SMCC_H__
> 
>> +#define __ASM_ARM_SMCCC_H_
> 
> Ditto.
> 
>> +
>> +#include <xen/types.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        0
> 
> Is this file coming from Linux? If so, it should be mention. If not, 
> please use soft tab and not hard tab. This is valid in all this file.
> 
>> +#define ARM_SMCCC_FAST_CALL        1
>> +#define ARM_SMCCC_TYPE_SHIFT        31
>> +
>> +#define ARM_SMCCC_SMC_32        0
>> +#define ARM_SMCCC_SMC_64        1
>> +#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
>> +
>> +#define ARM_SMCCC_IS_FAST_CALL(smc_val)    \
>> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
>> +#define ARM_SMCCC_IS_64(smc_val) \
>> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
>> +#define ARM_SMCCC_FUNC_NUM(smc_val)    ((smc_val) & ARM_SMCCC_FUNC_MASK)
>> +#define ARM_SMCCC_OWNER_NUM(smc_val) \
>> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
>> +
>> +#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))
> 
> I would appreciate a bit more documentation of those macros as they are 
> a bit difficult to parse. Also some newline would be nice for clarity.
> 
>> +
>> +#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
>> +
>> +#define ARM_SMCCC_FUNC_CALL_COUNT    0xFF00
>> +#define ARM_SMCCC_FUNC_CALL_UID        0xFF01
>> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>> +
>> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION    (-1)
>> +
>> +typedef struct {
>> +    uint32_t a[4];
>> +} arm_smccc_uid;
>> +
>> +#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)        \
>> +    ((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),            \
>> +               ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
>> +               ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
>> +
>> +void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
>> +
>> +#endif
> 
> #endif /* __ASM_ARM_SMCC_H__
> 
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>>
> 
> Cheers,
>

Julien Grall June 27, 2017, 11:08 a.m. UTC | #6

On 06/22/2017 05:29 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

> On 15.06.17 13:48, Julien Grall wrote:
>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>> index 6cf9ee7..2d0b058 100644
>>> --- a/xen/arch/arm/traps.c
>>> +++ b/xen/arch/arm/traps.c
>>> @@ -44,6 +44,7 @@
>>>  #include <asm/cpufeature.h>
>>>  #include <asm/flushtlb.h>
>>>  #include <asm/monitor.h>
>>> +#include <asm/smccc.h>
>>>
>>>  #include "decode.h"
>>>  #include "vtimer.h"
>>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs 
>>> *regs, const union hsr hsr)
>>>  {
>>
>> I think it would make sense to push this function in the new file.
> Unfortunately, I can't do this, because it uses local functions such as
> inject_undef_exception() or advance_pc().

The code is not set in stone. It is perfectly fine to rework it if it 
does not fit.

> 
>> Also, I was expecting some change in the HVC path as you say that this 
>> will be used for both HVC and SMC.
> Actually, I plan to use this particular function to handle only SMCs,
> because it does SMC-specific tasks, such as calling to a monitor.
> 
> HVCs will be handled in different call path in the next patch,
> because currently HVC callpath is used by PSCI code.

But your commit title says: "handle SMCs/HVCs according to SMCCC". So 
something does not match...

Cheers,

Volodymyr Babchuk June 27, 2017, 2:40 p.m. UTC | #7

Hello Julien,


On 27.06.17 14:08, Julien Grall wrote:
> On 06/22/2017 05:29 PM, Volodymyr Babchuk wrote:
>> Hi Julien,
>
> Hi Volodymyr,
>
>> On 15.06.17 13:48, Julien Grall wrote:
>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>> index 6cf9ee7..2d0b058 100644
>>>> --- a/xen/arch/arm/traps.c
>>>> +++ b/xen/arch/arm/traps.c
>>>> @@ -44,6 +44,7 @@
>>>>  #include <asm/cpufeature.h>
>>>>  #include <asm/flushtlb.h>
>>>>  #include <asm/monitor.h>
>>>> +#include <asm/smccc.h>
>>>>
>>>>  #include "decode.h"
>>>>  #include "vtimer.h"
>>>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct 
>>>> cpu_user_regs *regs, const union hsr hsr)
>>>>  {
>>>
>>> I think it would make sense to push this function in the new file.
>> Unfortunately, I can't do this, because it uses local functions such as
>> inject_undef_exception() or advance_pc().
>
> The code is not set in stone. It is perfectly fine to rework it if it 
> does not fit.
Okay. Probably, I can put forward declarations of the mentioned 
functions into as-arm/processor.h.
Should I leave implementation of those functions in the traps.c or 
should I move them into processor.c ?
>
>>
>>> Also, I was expecting some change in the HVC path as you say that 
>>> this will be used for both HVC and SMC.
>> Actually, I plan to use this particular function to handle only SMCs,
>> because it does SMC-specific tasks, such as calling to a monitor.
>>
>> HVCs will be handled in different call path in the next patch,
>> because currently HVC callpath is used by PSCI code.
>
> But your commit title says: "handle SMCs/HVCs according to SMCCC". So 
> something does not match...
Yes, you are right. I'll rework the patches.

Will you review the V2 series or should I push V3?

Julien Grall June 27, 2017, 2:54 p.m. UTC | #8

On 27/06/17 15:40, Volodymyr Babchuk wrote:
> Hello Julien,
>
>
> On 27.06.17 14:08, Julien Grall wrote:
>> On 06/22/2017 05:29 PM, Volodymyr Babchuk wrote:
>>> Hi Julien,
>>
>> Hi Volodymyr,
>>
>>> On 15.06.17 13:48, Julien Grall wrote:
>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>> index 6cf9ee7..2d0b058 100644
>>>>> --- a/xen/arch/arm/traps.c
>>>>> +++ b/xen/arch/arm/traps.c
>>>>> @@ -44,6 +44,7 @@
>>>>>  #include <asm/cpufeature.h>
>>>>>  #include <asm/flushtlb.h>
>>>>>  #include <asm/monitor.h>
>>>>> +#include <asm/smccc.h>
>>>>>
>>>>>  #include "decode.h"
>>>>>  #include "vtimer.h"
>>>>> @@ -2781,11 +2782,16 @@ static void do_trap_smc(struct
>>>>> cpu_user_regs *regs, const union hsr hsr)
>>>>>  {
>>>>
>>>> I think it would make sense to push this function in the new file.
>>> Unfortunately, I can't do this, because it uses local functions such as
>>> inject_undef_exception() or advance_pc().
>>
>> The code is not set in stone. It is perfectly fine to rework it if it
>> does not fit.
> Okay. Probably, I can put forward declarations of the mentioned
> functions into as-arm/processor.h.
> Should I leave implementation of those functions in the traps.c or
> should I move them into processor.c ?
>>
>>>
>>>> Also, I was expecting some change in the HVC path as you say that
>>>> this will be used for both HVC and SMC.
>>> Actually, I plan to use this particular function to handle only SMCs,
>>> because it does SMC-specific tasks, such as calling to a monitor.
>>>
>>> HVCs will be handled in different call path in the next patch,
>>> because currently HVC callpath is used by PSCI code.
>>
>> But your commit title says: "handle SMCs/HVCs according to SMCCC". So
>> something does not match...
> Yes, you are right. I'll rework the patches.
>
> Will you review the V2 series or should I push V3?

I will review the v2 series. I will try to do it today or tomorrow.

Cheers,

Julien Grall Aug. 1, 2017, 10:59 a.m. UTC | #9

(+ Edgar, Mark, Dave)

Hi,

On 14/06/17 15:10, Volodymyr Babchuk wrote:
> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> SMCCC states that both HVC and SMC are valid conduits to call to a different
> firmware functions. Thus, for example PSCI calls can be made both by
> SMC or HVC. Also SMCCC defines function number coding for such calls.
> Besides functional calls there are query calls, which allows underling
> OS determine version, UID and number of functions provided by service
> provider.
>
> This patch adds new file `smccc.c`, which handles both generic SMCs
> and HVC according to SMC. At this moment it implements only one
> service: Standard Hypervisor Service.
>
> Standard Hypervisor Service only supports query calls, so caller can
> ask about hypervisor UID and determine that it is XEN running.
>
> This change allows more generic handling for SMCs and HVCs and it can
> be easily extended to support new services and functions.

I have already reviewed the code and one thing I missed is how a domain 
will know that Xen supports SMCCC.

Currently, Xen will:
	- inject an undefined instruction for any SMC issued by a guest
	- crash the guest (quite bad) for any unknown HCV #0

So a guest needs to be aware whether Xen supports SMCCC convention or 
not. I am not aware of any bindings in the device-tree for doing that.

The other issue is not all the firmware may be SMCCC capable. We may 
want in the future to support other convention to allow baremetal OS 
running on Xen. This means a guest should be able to detect the 
convention used.

I don't have a clear answer here yet, but thought it would be good to 
start a conversation.

Cheers,

Edgar E. Iglesias Aug. 1, 2017, 2:25 p.m. UTC | #10

On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
> (+ Edgar, Mark, Dave)
> 
> Hi,

Hi Julien,

I'll share some thoughts based on our platforms.


> On 14/06/17 15:10, Volodymyr Babchuk wrote:
> >SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> >SMCCC states that both HVC and SMC are valid conduits to call to a different
> >firmware functions. Thus, for example PSCI calls can be made both by
> >SMC or HVC. Also SMCCC defines function number coding for such calls.
> >Besides functional calls there are query calls, which allows underling
> >OS determine version, UID and number of functions provided by service
> >provider.
> >
> >This patch adds new file `smccc.c`, which handles both generic SMCs
> >and HVC according to SMC. At this moment it implements only one
> >service: Standard Hypervisor Service.
> >
> >Standard Hypervisor Service only supports query calls, so caller can
> >ask about hypervisor UID and determine that it is XEN running.
> >
> >This change allows more generic handling for SMCs and HVCs and it can
> >be easily extended to support new services and functions.
> 
> I have already reviewed the code and one thing I missed is how a domain will
> know that Xen supports SMCCC.
> 
> Currently, Xen will:
> 	- inject an undefined instruction for any SMC issued by a guest
> 	- crash the guest (quite bad) for any unknown HCV #0
> 
> So a guest needs to be aware whether Xen supports SMCCC convention or not. I
> am not aware of any bindings in the device-tree for doing that.

On our platforms, SW probes the DT for specific service classes and then
probes for specific versions via SMC calls using the standard Version FIDs.
If the DT does not specify the firmware node, I don't think any SMCs will be
issued but the guest may not be functional (as the firmware interface is
mandatory).

I don't know of a generic DT node/compat that tells guests if SMCC probing
is allowed or not. Perhaps there should be one, or there should be yet
another service specific one for Hypervisors. I don't know.

For example, these are the nodes we've got (PSCI and EEMI/SIP):
        psci {
                compatible = "arm,psci-0.2";
                method = "smc";
        };

        pmufw: firmware {
                compatible = "xlnx,zynqmp-pm";
                method = "smc";
                interrupt-parent = <&gic>;
                interrupts = <0 35 4>;
        };

SW that does not have DT support, will either directly probe the SMC
interface or in some cases just assume it's there and use it.

ZynqMP-wise, Xen is in a little bit of an akward position by messing
guests up if they probe for non-existing SMC services but I don't think
it's that bad. Mainly because there's really very little ZynqMP guests
that need the firmware would be able todo without it.

For other platforms and services, I guess FW may very well be optional
and probing makes more sence. So getting support for gracefully returning
Unknown FID still important...


> The other issue is not all the firmware may be SMCCC capable. We may want in
> the future to support other convention to allow baremetal OS running on Xen.
> This means a guest should be able to detect the convention used.

Perhaps this could be done by injecting DT fragments like we do for passthrough?

Cheers,
Edgar

> 
> I don't have a clear answer here yet, but thought it would be good to start
> a conversation.
> 
> Cheers,
> 
> -- 
> Julien Grall

Stefano Stabellini Aug. 1, 2017, 8:40 p.m. UTC | #11

On Tue, 1 Aug 2017, Edgar E. Iglesias wrote:
> On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
> > (+ Edgar, Mark, Dave)
> > 
> > Hi,
> 
> Hi Julien,
> 
> I'll share some thoughts based on our platforms.
> 
> 
> > On 14/06/17 15:10, Volodymyr Babchuk wrote:
> > >SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> > >SMCCC states that both HVC and SMC are valid conduits to call to a different
> > >firmware functions. Thus, for example PSCI calls can be made both by
> > >SMC or HVC. Also SMCCC defines function number coding for such calls.
> > >Besides functional calls there are query calls, which allows underling
> > >OS determine version, UID and number of functions provided by service
> > >provider.
> > >
> > >This patch adds new file `smccc.c`, which handles both generic SMCs
> > >and HVC according to SMC. At this moment it implements only one
> > >service: Standard Hypervisor Service.
> > >
> > >Standard Hypervisor Service only supports query calls, so caller can
> > >ask about hypervisor UID and determine that it is XEN running.
> > >
> > >This change allows more generic handling for SMCs and HVCs and it can
> > >be easily extended to support new services and functions.
> > 
> > I have already reviewed the code and one thing I missed is how a domain will
> > know that Xen supports SMCCC.
> > 
> > Currently, Xen will:
> > 	- inject an undefined instruction for any SMC issued by a guest
> > 	- crash the guest (quite bad) for any unknown HCV #0
> > 
> > So a guest needs to be aware whether Xen supports SMCCC convention or not. I
> > am not aware of any bindings in the device-tree for doing that.
> 
> On our platforms, SW probes the DT for specific service classes and then
> probes for specific versions via SMC calls using the standard Version FIDs.
> If the DT does not specify the firmware node, I don't think any SMCs will be
> issued but the guest may not be functional (as the firmware interface is
> mandatory).
> 
> I don't know of a generic DT node/compat that tells guests if SMCC probing
> is allowed or not. Perhaps there should be one, or there should be yet
> another service specific one for Hypervisors. I don't know.
> 
> For example, these are the nodes we've got (PSCI and EEMI/SIP):
>         psci {
>                 compatible = "arm,psci-0.2";
>                 method = "smc";
>         };
> 
>         pmufw: firmware {
>                 compatible = "xlnx,zynqmp-pm";
>                 method = "smc";
>                 interrupt-parent = <&gic>;
>                 interrupts = <0 35 4>;
>         };
> 
> SW that does not have DT support, will either directly probe the SMC
> interface or in some cases just assume it's there and use it.
> 
> ZynqMP-wise, Xen is in a little bit of an akward position by messing
> guests up if they probe for non-existing SMC services but I don't think
> it's that bad. Mainly because there's really very little ZynqMP guests
> that need the firmware would be able todo without it.
> 
> For other platforms and services, I guess FW may very well be optional
> and probing makes more sence. So getting support for gracefully returning
> Unknown FID still important...

Indeed, but unfortunately older versions of Xen don't do that. That's
why I think we'll have to introduce a feature flag under the Xen
hypervisor node on device tree. The presence of the flag could mean "it
is safe to probe via SMC/HVC". I think it makes sense for the flag to be
Xen specific, because we are talking about Xen behavior. Applications
that know they are running on a new enough Xen can skip the check (this
is going to be the case in most embedded scenarios).


> > The other issue is not all the firmware may be SMCCC capable. We may want in
> > the future to support other convention to allow baremetal OS running on Xen.
> > This means a guest should be able to detect the convention used.
> 
> Perhaps this could be done by injecting DT fragments like we do for passthrough?

Ideally the firmware protocol could be detected by HVC/SMC probing (once
that is deemed to be safe via device tree). However, if it is not
possible to establish the right protocol via HVC/SMC probing, then the
supported firmware protocol could be advertised via device tree, ideally
in a non-Xen specific fashion.

Julien Grall Aug. 1, 2017, 9:02 p.m. UTC | #12

Hi,

On 01/08/2017 21:40, Stefano Stabellini wrote:
> On Tue, 1 Aug 2017, Edgar E. Iglesias wrote:
>> On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
>>> (+ Edgar, Mark, Dave)
>>>
>>> Hi,
>>
>> Hi Julien,
>>
>> I'll share some thoughts based on our platforms.
>>
>>
>>> On 14/06/17 15:10, Volodymyr Babchuk wrote:
>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>>>> SMCCC states that both HVC and SMC are valid conduits to call to a different
>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>>> Besides functional calls there are query calls, which allows underling
>>>> OS determine version, UID and number of functions provided by service
>>>> provider.
>>>>
>>>> This patch adds new file `smccc.c`, which handles both generic SMCs
>>>> and HVC according to SMC. At this moment it implements only one
>>>> service: Standard Hypervisor Service.
>>>>
>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>
>>>> This change allows more generic handling for SMCs and HVCs and it can
>>>> be easily extended to support new services and functions.
>>>
>>> I have already reviewed the code and one thing I missed is how a domain will
>>> know that Xen supports SMCCC.
>>>
>>> Currently, Xen will:
>>> 	- inject an undefined instruction for any SMC issued by a guest
>>> 	- crash the guest (quite bad) for any unknown HCV #0
>>>
>>> So a guest needs to be aware whether Xen supports SMCCC convention or not. I
>>> am not aware of any bindings in the device-tree for doing that.
>>
>> On our platforms, SW probes the DT for specific service classes and then
>> probes for specific versions via SMC calls using the standard Version FIDs.
>> If the DT does not specify the firmware node, I don't think any SMCs will be
>> issued but the guest may not be functional (as the firmware interface is
>> mandatory).
>>
>> I don't know of a generic DT node/compat that tells guests if SMCC probing
>> is allowed or not. Perhaps there should be one, or there should be yet
>> another service specific one for Hypervisors. I don't know.
>>
>> For example, these are the nodes we've got (PSCI and EEMI/SIP):
>>         psci {
>>                 compatible = "arm,psci-0.2";
>>                 method = "smc";
>>         };
>>
>>         pmufw: firmware {
>>                 compatible = "xlnx,zynqmp-pm";
>>                 method = "smc";
>>                 interrupt-parent = <&gic>;
>>                 interrupts = <0 35 4>;
>>         };
>>
>> SW that does not have DT support, will either directly probe the SMC
>> interface or in some cases just assume it's there and use it.
>>
>> ZynqMP-wise, Xen is in a little bit of an akward position by messing
>> guests up if they probe for non-existing SMC services but I don't think
>> it's that bad. Mainly because there's really very little ZynqMP guests
>> that need the firmware would be able todo without it.
>>
>> For other platforms and services, I guess FW may very well be optional
>> and probing makes more sence. So getting support for gracefully returning
>> Unknown FID still important...
>
> Indeed, but unfortunately older versions of Xen don't do that. That's
> why I think we'll have to introduce a feature flag under the Xen
> hypervisor node on device tree. The presence of the flag could mean "it
> is safe to probe via SMC/HVC". I think it makes sense for the flag to be
> Xen specific, because we are talking about Xen behavior. Applications
> that know they are running on a new enough Xen can skip the check (this
> is going to be the case in most embedded scenarios).

This is not Xen specific behavior. Per ARM ARM, SMC will be undefined 
when EL3 is not present. Today Xen is behaving the same way as EL3 was 
not existing on the platform.

So we need a generic way to say "SMC is supported and is SMCCC capable".

Cheers,

Edgar E. Iglesias Aug. 1, 2017, 9:22 p.m. UTC | #13

On Tue, Aug 01, 2017 at 10:02:45PM +0100, Julien Grall wrote:
> Hi,
> 
> On 01/08/2017 21:40, Stefano Stabellini wrote:
> >On Tue, 1 Aug 2017, Edgar E. Iglesias wrote:
> >>On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
> >>>(+ Edgar, Mark, Dave)
> >>>
> >>>Hi,
> >>
> >>Hi Julien,
> >>
> >>I'll share some thoughts based on our platforms.
> >>
> >>
> >>>On 14/06/17 15:10, Volodymyr Babchuk wrote:
> >>>>SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
> >>>>SMCCC states that both HVC and SMC are valid conduits to call to a different
> >>>>firmware functions. Thus, for example PSCI calls can be made both by
> >>>>SMC or HVC. Also SMCCC defines function number coding for such calls.
> >>>>Besides functional calls there are query calls, which allows underling
> >>>>OS determine version, UID and number of functions provided by service
> >>>>provider.
> >>>>
> >>>>This patch adds new file `smccc.c`, which handles both generic SMCs
> >>>>and HVC according to SMC. At this moment it implements only one
> >>>>service: Standard Hypervisor Service.
> >>>>
> >>>>Standard Hypervisor Service only supports query calls, so caller can
> >>>>ask about hypervisor UID and determine that it is XEN running.
> >>>>
> >>>>This change allows more generic handling for SMCs and HVCs and it can
> >>>>be easily extended to support new services and functions.
> >>>
> >>>I have already reviewed the code and one thing I missed is how a domain will
> >>>know that Xen supports SMCCC.
> >>>
> >>>Currently, Xen will:
> >>>	- inject an undefined instruction for any SMC issued by a guest
> >>>	- crash the guest (quite bad) for any unknown HCV #0
> >>>
> >>>So a guest needs to be aware whether Xen supports SMCCC convention or not. I
> >>>am not aware of any bindings in the device-tree for doing that.
> >>
> >>On our platforms, SW probes the DT for specific service classes and then
> >>probes for specific versions via SMC calls using the standard Version FIDs.
> >>If the DT does not specify the firmware node, I don't think any SMCs will be
> >>issued but the guest may not be functional (as the firmware interface is
> >>mandatory).
> >>
> >>I don't know of a generic DT node/compat that tells guests if SMCC probing
> >>is allowed or not. Perhaps there should be one, or there should be yet
> >>another service specific one for Hypervisors. I don't know.
> >>
> >>For example, these are the nodes we've got (PSCI and EEMI/SIP):
> >>        psci {
> >>                compatible = "arm,psci-0.2";
> >>                method = "smc";
> >>        };
> >>
> >>        pmufw: firmware {
> >>                compatible = "xlnx,zynqmp-pm";
> >>                method = "smc";
> >>                interrupt-parent = <&gic>;
> >>                interrupts = <0 35 4>;
> >>        };
> >>
> >>SW that does not have DT support, will either directly probe the SMC
> >>interface or in some cases just assume it's there and use it.
> >>
> >>ZynqMP-wise, Xen is in a little bit of an akward position by messing
> >>guests up if they probe for non-existing SMC services but I don't think
> >>it's that bad. Mainly because there's really very little ZynqMP guests
> >>that need the firmware would be able todo without it.
> >>
> >>For other platforms and services, I guess FW may very well be optional
> >>and probing makes more sence. So getting support for gracefully returning
> >>Unknown FID still important...
> >
> >Indeed, but unfortunately older versions of Xen don't do that. That's
> >why I think we'll have to introduce a feature flag under the Xen
> >hypervisor node on device tree. The presence of the flag could mean "it
> >is safe to probe via SMC/HVC". I think it makes sense for the flag to be
> >Xen specific, because we are talking about Xen behavior. Applications
> >that know they are running on a new enough Xen can skip the check (this
> >is going to be the case in most embedded scenarios).
> 
> This is not Xen specific behavior. Per ARM ARM, SMC will be undefined when
> EL3 is not present. Today Xen is behaving the same way as EL3 was not
> existing on the platform.
> 
> So we need a generic way to say "SMC is supported and is SMCCC capable".

Would it be unthinkable to treat the lack of "Unknown FID" returns as a bug
and backport the "fix" and leave it at that?

Cheers,
Edgar

Julien Grall Aug. 1, 2017, 10:07 p.m. UTC | #14

Hi Edgar,

On 01/08/2017 22:22, Edgar E. Iglesias wrote:
> On Tue, Aug 01, 2017 at 10:02:45PM +0100, Julien Grall wrote:
>> Hi,
>>
>> On 01/08/2017 21:40, Stefano Stabellini wrote:
>>> On Tue, 1 Aug 2017, Edgar E. Iglesias wrote:
>>>> On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
>>>>> (+ Edgar, Mark, Dave)
>>>>>
>>>>> Hi,
>>>>
>>>> Hi Julien,
>>>>
>>>> I'll share some thoughts based on our platforms.
>>>>
>>>>
>>>>> On 14/06/17 15:10, Volodymyr Babchuk wrote:
>>>>>> SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
>>>>>> SMCCC states that both HVC and SMC are valid conduits to call to a different
>>>>>> firmware functions. Thus, for example PSCI calls can be made both by
>>>>>> SMC or HVC. Also SMCCC defines function number coding for such calls.
>>>>>> Besides functional calls there are query calls, which allows underling
>>>>>> OS determine version, UID and number of functions provided by service
>>>>>> provider.
>>>>>>
>>>>>> This patch adds new file `smccc.c`, which handles both generic SMCs
>>>>>> and HVC according to SMC. At this moment it implements only one
>>>>>> service: Standard Hypervisor Service.
>>>>>>
>>>>>> Standard Hypervisor Service only supports query calls, so caller can
>>>>>> ask about hypervisor UID and determine that it is XEN running.
>>>>>>
>>>>>> This change allows more generic handling for SMCs and HVCs and it can
>>>>>> be easily extended to support new services and functions.
>>>>>
>>>>> I have already reviewed the code and one thing I missed is how a domain will
>>>>> know that Xen supports SMCCC.
>>>>>
>>>>> Currently, Xen will:
>>>>> 	- inject an undefined instruction for any SMC issued by a guest
>>>>> 	- crash the guest (quite bad) for any unknown HCV #0
>>>>>
>>>>> So a guest needs to be aware whether Xen supports SMCCC convention or not. I
>>>>> am not aware of any bindings in the device-tree for doing that.
>>>>
>>>> On our platforms, SW probes the DT for specific service classes and then
>>>> probes for specific versions via SMC calls using the standard Version FIDs.
>>>> If the DT does not specify the firmware node, I don't think any SMCs will be
>>>> issued but the guest may not be functional (as the firmware interface is
>>>> mandatory).
>>>>
>>>> I don't know of a generic DT node/compat that tells guests if SMCC probing
>>>> is allowed or not. Perhaps there should be one, or there should be yet
>>>> another service specific one for Hypervisors. I don't know.
>>>>
>>>> For example, these are the nodes we've got (PSCI and EEMI/SIP):
>>>>        psci {
>>>>                compatible = "arm,psci-0.2";
>>>>                method = "smc";
>>>>        };
>>>>
>>>>        pmufw: firmware {
>>>>                compatible = "xlnx,zynqmp-pm";
>>>>                method = "smc";
>>>>                interrupt-parent = <&gic>;
>>>>                interrupts = <0 35 4>;
>>>>        };
>>>>
>>>> SW that does not have DT support, will either directly probe the SMC
>>>> interface or in some cases just assume it's there and use it.
>>>>
>>>> ZynqMP-wise, Xen is in a little bit of an akward position by messing
>>>> guests up if they probe for non-existing SMC services but I don't think
>>>> it's that bad. Mainly because there's really very little ZynqMP guests
>>>> that need the firmware would be able todo without it.
>>>>
>>>> For other platforms and services, I guess FW may very well be optional
>>>> and probing makes more sence. So getting support for gracefully returning
>>>> Unknown FID still important...
>>>
>>> Indeed, but unfortunately older versions of Xen don't do that. That's
>>> why I think we'll have to introduce a feature flag under the Xen
>>> hypervisor node on device tree. The presence of the flag could mean "it
>>> is safe to probe via SMC/HVC". I think it makes sense for the flag to be
>>> Xen specific, because we are talking about Xen behavior. Applications
>>> that know they are running on a new enough Xen can skip the check (this
>>> is going to be the case in most embedded scenarios).
>>
>> This is not Xen specific behavior. Per ARM ARM, SMC will be undefined when
>> EL3 is not present. Today Xen is behaving the same way as EL3 was not
>> existing on the platform.
>>
>> So we need a generic way to say "SMC is supported and is SMCCC capable".
>
> Would it be unthinkable to treat the lack of "Unknown FID" returns as a bug
> and backport the "fix" and leave it at that?

At the moment, this is not an option for me. New Linux should work on 
any Xen we shipped in the past. We should really avoid to break that 
unless there are a strong reason too.

Plus that will not solve the problem that we may want support other 
convention in the future.

I think the way forward is to define a binding that will advertise this 
feature.

Cheers,

Stefano Stabellini Aug. 2, 2017, 6:29 p.m. UTC | #15

On Tue, 1 Aug 2017, Julien Grall wrote:
> Hi Edgar,
> 
> On 01/08/2017 22:22, Edgar E. Iglesias wrote:
> > On Tue, Aug 01, 2017 at 10:02:45PM +0100, Julien Grall wrote:
> > > Hi,
> > > 
> > > On 01/08/2017 21:40, Stefano Stabellini wrote:
> > > > On Tue, 1 Aug 2017, Edgar E. Iglesias wrote:
> > > > > On Tue, Aug 01, 2017 at 11:59:00AM +0100, Julien Grall wrote:
> > > > > > (+ Edgar, Mark, Dave)
> > > > > > 
> > > > > > Hi,
> > > > > 
> > > > > Hi Julien,
> > > > > 
> > > > > I'll share some thoughts based on our platforms.
> > > > > 
> > > > > 
> > > > > > On 14/06/17 15:10, Volodymyr Babchuk wrote:
> > > > > > > SMCCC (SMC Call Convention) describes how to handle both HVCs and
> > > > > > > SMCs.
> > > > > > > SMCCC states that both HVC and SMC are valid conduits to call to a
> > > > > > > different
> > > > > > > firmware functions. Thus, for example PSCI calls can be made both
> > > > > > > by
> > > > > > > SMC or HVC. Also SMCCC defines function number coding for such
> > > > > > > calls.
> > > > > > > Besides functional calls there are query calls, which allows
> > > > > > > underling
> > > > > > > OS determine version, UID and number of functions provided by
> > > > > > > service
> > > > > > > provider.
> > > > > > > 
> > > > > > > This patch adds new file `smccc.c`, which handles both generic
> > > > > > > SMCs
> > > > > > > and HVC according to SMC. At this moment it implements only one
> > > > > > > service: Standard Hypervisor Service.
> > > > > > > 
> > > > > > > Standard Hypervisor Service only supports query calls, so caller
> > > > > > > can
> > > > > > > ask about hypervisor UID and determine that it is XEN running.
> > > > > > > 
> > > > > > > This change allows more generic handling for SMCs and HVCs and it
> > > > > > > can
> > > > > > > be easily extended to support new services and functions.
> > > > > > 
> > > > > > I have already reviewed the code and one thing I missed is how a
> > > > > > domain will
> > > > > > know that Xen supports SMCCC.
> > > > > > 
> > > > > > Currently, Xen will:
> > > > > > 	- inject an undefined instruction for any SMC issued by a
> > > > > > guest
> > > > > > 	- crash the guest (quite bad) for any unknown HCV #0
> > > > > > 
> > > > > > So a guest needs to be aware whether Xen supports SMCCC convention
> > > > > > or not. I
> > > > > > am not aware of any bindings in the device-tree for doing that.
> > > > > 
> > > > > On our platforms, SW probes the DT for specific service classes and
> > > > > then
> > > > > probes for specific versions via SMC calls using the standard Version
> > > > > FIDs.
> > > > > If the DT does not specify the firmware node, I don't think any SMCs
> > > > > will be
> > > > > issued but the guest may not be functional (as the firmware interface
> > > > > is
> > > > > mandatory).
> > > > > 
> > > > > I don't know of a generic DT node/compat that tells guests if SMCC
> > > > > probing
> > > > > is allowed or not. Perhaps there should be one, or there should be yet
> > > > > another service specific one for Hypervisors. I don't know.
> > > > > 
> > > > > For example, these are the nodes we've got (PSCI and EEMI/SIP):
> > > > >        psci {
> > > > >                compatible = "arm,psci-0.2";
> > > > >                method = "smc";
> > > > >        };
> > > > > 
> > > > >        pmufw: firmware {
> > > > >                compatible = "xlnx,zynqmp-pm";
> > > > >                method = "smc";
> > > > >                interrupt-parent = <&gic>;
> > > > >                interrupts = <0 35 4>;
> > > > >        };
> > > > > 
> > > > > SW that does not have DT support, will either directly probe the SMC
> > > > > interface or in some cases just assume it's there and use it.
> > > > > 
> > > > > ZynqMP-wise, Xen is in a little bit of an akward position by messing
> > > > > guests up if they probe for non-existing SMC services but I don't
> > > > > think
> > > > > it's that bad. Mainly because there's really very little ZynqMP guests
> > > > > that need the firmware would be able todo without it.
> > > > > 
> > > > > For other platforms and services, I guess FW may very well be optional
> > > > > and probing makes more sence. So getting support for gracefully
> > > > > returning
> > > > > Unknown FID still important...
> > > > 
> > > > Indeed, but unfortunately older versions of Xen don't do that. That's
> > > > why I think we'll have to introduce a feature flag under the Xen
> > > > hypervisor node on device tree. The presence of the flag could mean "it
> > > > is safe to probe via SMC/HVC". I think it makes sense for the flag to be
> > > > Xen specific, because we are talking about Xen behavior. Applications
> > > > that know they are running on a new enough Xen can skip the check (this
> > > > is going to be the case in most embedded scenarios).
> > > 
> > > This is not Xen specific behavior. Per ARM ARM, SMC will be undefined when
> > > EL3 is not present. Today Xen is behaving the same way as EL3 was not
> > > existing on the platform.
> > > 
> > > So we need a generic way to say "SMC is supported and is SMCCC capable".
> > 
> > Would it be unthinkable to treat the lack of "Unknown FID" returns as a bug
> > and backport the "fix" and leave it at that?
> 
> At the moment, this is not an option for me. New Linux should work on any Xen
> we shipped in the past. We should really avoid to break that unless there are
> a strong reason too.
> 
> Plus that will not solve the problem that we may want support other convention
> in the future.
> 
> I think the way forward is to define a binding that will advertise this
> feature.

+1

Patch

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..b8728cf 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -39,6 +39,7 @@  obj-y += psci.o
 obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smc.o
+obj-y += smccc.o
 obj-y += smp.o
 obj-y += smpboot.o
 obj-y += sysctl.o
diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
new file mode 100644
index 0000000..5d10964
--- /dev/null
+++ b/xen/arch/arm/smccc.c
@@ -0,0 +1,96 @@ 
+/*
+ * xen/arch/arm/smccc.c
+ *
+ * Generic handler for SMC and HVC calls according to
+ * ARM SMC callling convention
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * 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.
+ */
+
+
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/perfc.h>
+/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
+#include <xen/sched.h>
+#include <xen/stdbool.h>
+#include <xen/types.h>
+#include <asm/domain.h>
+#include <asm/psci.h>
+#include <asm/smccc.h>
+#include <asm/regs.h>
+
+#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
+                                    0x9a, 0xcf, 0x79, 0xd1, \
+                                    0x8d, 0xde, 0xe6, 0x67)
+
+/*
+ * We can't use XEN version here:
+ * Major revision should change every time SMC/HVC function is removed.
+ * Minor revision should change every time SMC/HVC function is added.
+ * So, it is SMCCC protocol revision code, not XEN version
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+/* SMCCC interface for hypervisor. Tell about self */
+static bool handle_hypervisor(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    switch ( ARM_SMCCC_FUNC_NUM(get_user_reg(regs, 0)) )
+    {
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, XEN_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+        set_user_reg(regs, 0, XEN_SMCCC_UID.a[0]);
+        set_user_reg(regs, 1, XEN_SMCCC_UID.a[1]);
+        set_user_reg(regs, 2, XEN_SMCCC_UID.a[2]);
+        set_user_reg(regs, 3, XEN_SMCCC_UID.a[3]);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, XEN_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, XEN_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
+/**
+ * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
+ */
+void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    bool handled = false;
+
+    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+    {
+    case ARM_SMCCC_OWNER_HYPERVISOR:
+        handled = handle_hypervisor(regs, hsr);
+        break;
+    }
+
+    if ( !handled )
+    {
+        printk("Uhandled SMC/HVC: %08"PRIregister"\n", get_user_reg(regs, 0));
+        /* Inform caller that function is not supported */
+        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+    }
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6cf9ee7..2d0b058 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -44,6 +44,7 @@ 
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
 #include <asm/monitor.h>
+#include <asm/smccc.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2781,11 +2782,16 @@  static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int rc = 0;
 
+    /* Let monitor to handle the call */
     if ( current->domain->arch.monitor.privileged_call_enabled )
         rc = monitor_smc();
 
-    if ( rc != 1 )
-        inject_undef_exception(regs, hsr);
+    if ( rc == 1 )
+        return;
+
+    /* Use standard routines to handle the call */
+    smccc_handle_call(regs, hsr);
+    advance_pc(regs, hsr);
 }
 
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 0000000..9342d5e
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,89 @@ 
+/*
+ * 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_
+
+#include <xen/types.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		0
+#define ARM_SMCCC_FAST_CALL		1
+#define ARM_SMCCC_TYPE_SHIFT		31
+
+#define ARM_SMCCC_SMC_32		0
+#define ARM_SMCCC_SMC_64		1
+#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
+
+#define ARM_SMCCC_IS_FAST_CALL(smc_val)	\
+	((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+#define ARM_SMCCC_IS_64(smc_val) \
+	((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+#define ARM_SMCCC_FUNC_NUM(smc_val)	((smc_val) & ARM_SMCCC_FUNC_MASK)
+#define ARM_SMCCC_OWNER_NUM(smc_val) \
+	(((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+#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))
+
+#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
+
+#define ARM_SMCCC_FUNC_CALL_COUNT	0xFF00
+#define ARM_SMCCC_FUNC_CALL_UID		0xFF01
+#define ARM_SMCCC_FUNC_CALL_REVISION	0xFF03
+
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION	(-1)
+
+typedef struct {
+	uint32_t a[4];
+} arm_smccc_uid;
+
+#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)		\
+	((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),			\
+			   ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
+			   ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
+
+void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
+
+#endif
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */