[PATCH 0/2] Handle SMCs and HVCs in conformance with SMCCC

[PATCH 0/2] Handle SMCs and HVCs in conformance with SMCCC

[Volodymyr Babchuk]

Hello all,

This patch series adds a generic way to handle standard calls
that are defined in ARM SMC calling convention (SMCCC).

First patch adds generic handler and second one moves PSCI
handling code to that generic handler.

With this patch series guest can query hypervisor in a standard
way to determine which virtualization system is used.
The same applies to PSCI calls. Now guest can tell if PSCI calls
are handled by hypervisor or by, say, ARM TF.

Also those patches are needed for upcoming TEE support.

It was tested on RCAR H3 Platform.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Volodymyr Babchuk]

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 <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 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
+
+/* 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:
+ */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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 <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Volodymyr Babchuk]

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 <volodymyr_babchuk@epam.com>
>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  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,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Volodymyr Babchuk]

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 <volodymyr_babchuk@epam.com>
>> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>> ---
>>  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,
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Volodymyr Babchuk]

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?




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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 <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  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;
> +}

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

(+ 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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Edgar E. Iglesias]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Edgar E. Iglesias]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Julien Grall]

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,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

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

[Stefano Stabellini]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/2] arm: traps: handle PSCI calls inside `smccc.c`

[Volodymyr Babchuk]

PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `smccc.c`. This patch just moves PSCI
handler calls from `traps.c` to `smccc.c`.

PSCI is considered as two different "services" in terms of SMCCC.
Older PSCI 1.0 is treated as "architecture service", while never
PSCI 2.0 is defined as "standard secure service".

Also old accessors PSCI_ARG() and PSCI_RESULT_REG() were replaced
with generic set_user_reg()/get_user_reg() functions.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 xen/arch/arm/smccc.c | 135 +++++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c | 134 +++++---------------------------------------------
 2 files changed, 147 insertions(+), 122 deletions(-)

diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
index 5d10964..0014aff 100644
--- a/xen/arch/arm/smccc.c
+++ b/xen/arch/arm/smccc.c
@@ -42,6 +42,14 @@
 #define XEN_SMCCC_MINOR_REVISION 1
 #define XEN_SMCCC_FUNCTION_COUNT 3
 
+#define SSC_SMCCC_UID ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
+                                    0x92, 0x20, 0xce, 0x16, \
+                                    0x41, 0xe5, 0x9f, 0x6f)
+
+#define SSC_SMCCC_MAJOR_REVISION 0
+#define SSC_SMCCC_MINOR_REVISION 1
+#define SSC_SMCCC_FUNCTION_COUNT 13
+
 /* SMCCC interface for hypervisor. Tell about self */
 static bool handle_hypervisor(struct cpu_user_regs *regs, const union hsr hsr)
 {
@@ -64,6 +72,127 @@ static bool handle_hypervisor(struct cpu_user_regs *regs, const union hsr hsr)
     return false;
 }
 
+/* old (arvm7) PSCI interface */
+static bool handle_arch(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
+    {
+    case PSCI_cpu_off:
+    {
+        uint32_t pstate = get_user_reg(regs, 1);
+        perfc_incr(vpsci_cpu_off);
+        set_user_reg(regs, 0, do_psci_cpu_off(pstate));
+    }
+    return true;
+    case PSCI_cpu_on:
+    {
+        uint32_t vcpuid = get_user_reg(regs, 1);
+        register_t epoint = get_user_reg(regs, 2);
+        perfc_incr(vpsci_cpu_on);
+        set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
+    }
+    return true;
+    }
+    return false;
+}
+
+/* helper function for checking arm mode 32/64 bit */
+static inline int psci_mode_check(struct domain *d, register_t fid)
+{
+        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
+}
+
+/* PSCI 2.0 interface */
+static bool handle_ssc(struct cpu_user_regs *regs, const union hsr hsr)
+{
+    register_t fid = get_user_reg(regs, 0);
+
+    switch ( ARM_SMCCC_FUNC_NUM(fid) )
+    {
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+        perfc_incr(vpsci_version);
+        set_user_reg(regs, 0, do_psci_0_2_version());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+        perfc_incr(vpsci_cpu_off);
+        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+        perfc_incr(vpsci_migrate_info_type);
+        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+        perfc_incr(vpsci_migrate_info_up_cpu);
+        if ( psci_mode_check(current->domain, fid) )
+            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+        perfc_incr(vpsci_system_off);
+        do_psci_0_2_system_off();
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+        perfc_incr(vpsci_system_reset);
+        do_psci_0_2_system_reset();
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+        perfc_incr(vpsci_cpu_on);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t vcpuid = get_user_reg(regs,1);
+            register_t epoint = get_user_reg(regs,2);
+            register_t cid = get_user_reg(regs,3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+        perfc_incr(vpsci_cpu_suspend);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t pstate = get_user_reg(regs,1) & 0xFFFFFFFF;
+            register_t epoint = get_user_reg(regs,2);
+            register_t cid = get_user_reg(regs,3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+        perfc_incr(vpsci_cpu_affinity_info);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t taff = get_user_reg(regs,1);
+            uint32_t laff = get_user_reg(regs,2) & 0xFFFFFFFF;
+            set_user_reg(regs, 0,
+                         do_psci_0_2_affinity_info(taff, laff));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+        perfc_incr(vpsci_cpu_migrate);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t tcpu = get_user_reg(regs,1) & 0xFFFFFFFF;
+            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+        set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
+        set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
+        set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
+        set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
 /**
  * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
  */
@@ -76,6 +205,12 @@ void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)
     case ARM_SMCCC_OWNER_HYPERVISOR:
         handled = handle_hypervisor(regs, hsr);
         break;
+    case ARM_SMCCC_OWNER_ARCH:
+        handled = handle_arch(regs, hsr);
+        break;
+    case ARM_SMCCC_OWNER_STANDARD:
+        handled = handle_ssc(regs, hsr);
+        break;
     }
 
     if ( !handled )
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2d0b058..61ddd43 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -39,7 +39,6 @@
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
-#include <asm/psci.h>
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
@@ -1451,123 +1450,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 #endif
 
 #ifdef CONFIG_ARM_64
-#define PSCI_RESULT_REG(reg) (reg)->x0
-#define PSCI_ARG(reg,n) (reg)->x##n
-#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
-#else
-#define PSCI_RESULT_REG(reg) (reg)->r0
-#define PSCI_ARG(reg,n) (reg)->r##n
-#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
-#endif
-
-/* helper function for checking arm mode 32/64 bit */
-static inline int psci_mode_check(struct domain *d, register_t fid)
-{
-        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
-}
-
-static void do_trap_psci(struct cpu_user_regs *regs)
-{
-    register_t fid = PSCI_ARG(regs,0);
-
-    /* preloading in case psci_mode_check fails */
-    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
-    switch( fid )
-    {
-    case PSCI_cpu_off:
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            perfc_incr(vpsci_cpu_off);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
-        }
-        break;
-    case PSCI_cpu_on:
-        {
-            uint32_t vcpuid = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            perfc_incr(vpsci_cpu_on);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
-        }
-        break;
-    case PSCI_0_2_FN_PSCI_VERSION:
-        perfc_incr(vpsci_version);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_version();
-        break;
-    case PSCI_0_2_FN_CPU_OFF:
-        perfc_incr(vpsci_cpu_off);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_cpu_off();
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-        perfc_incr(vpsci_migrate_info_type);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_type();
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
-    case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
-        perfc_incr(vpsci_migrate_info_up_cpu);
-        if ( psci_mode_check(current->domain, fid) )
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();
-        break;
-    case PSCI_0_2_FN_SYSTEM_OFF:
-        perfc_incr(vpsci_system_off);
-        do_psci_0_2_system_off();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
-        break;
-    case PSCI_0_2_FN_SYSTEM_RESET:
-        perfc_incr(vpsci_system_reset);
-        do_psci_0_2_system_reset();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
-        break;
-    case PSCI_0_2_FN_CPU_ON:
-    case PSCI_0_2_FN64_CPU_ON:
-        perfc_incr(vpsci_cpu_on);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t vcpuid = PSCI_ARG(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_on(vcpuid, epoint, cid);
-        }
-        break;
-    case PSCI_0_2_FN_CPU_SUSPEND:
-    case PSCI_0_2_FN64_CPU_SUSPEND:
-        perfc_incr(vpsci_cpu_suspend);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_suspend(pstate, epoint, cid);
-        }
-        break;
-    case PSCI_0_2_FN_AFFINITY_INFO:
-    case PSCI_0_2_FN64_AFFINITY_INFO:
-        perfc_incr(vpsci_cpu_affinity_info);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t taff = PSCI_ARG(regs,1);
-            uint32_t laff = PSCI_ARG32(regs,2);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_affinity_info(taff, laff);
-        }
-        break;
-    case PSCI_0_2_FN_MIGRATE:
-    case PSCI_0_2_FN64_MIGRATE:
-        perfc_incr(vpsci_cpu_migrate);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
-        }
-        break;
-    default:
-        domain_crash_synchronous();
-        return;
-    }
-}
-
-#ifdef CONFIG_ARM_64
 #define HYPERCALL_RESULT_REG(r) (r)->x0
 #define HYPERCALL_ARG1(r) (r)->x0
 #define HYPERCALL_ARG2(r) (r)->x1
@@ -2890,8 +2772,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
+        {
+            if ( !smccc_handle_call(regs, hsr) )
+                domain_crash_synchronous();
+        }
+        else
+            do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
         break;
 #ifdef CONFIG_ARM_64
     case HSR_EC_HVC64:
@@ -2902,8 +2788,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, &regs->x16, hsr.iss);
+        {
+            if ( !smccc_handle_call(regs, hsr) )
+                domain_crash_synchronous();
+        }
+        else
+            do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
         /*
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] arm: traps: handle PSCI calls inside `smccc.c`

[Julien Grall]

Hi Volodymyr,

On 06/14/2017 03:10 PM, Volodymyr Babchuk wrote:
> PSCI is part of HVC/SMC interface, so it should be handled in
> appropriate place: `smccc.c`. This patch just moves PSCI
> handler calls from `traps.c` to `smccc.c`.
> 
> PSCI is considered as two different "services" in terms of SMCCC.
> Older PSCI 1.0 is treated as "architecture service", while never
> PSCI 2.0 is defined as "standard secure service".
> 
> Also old accessors PSCI_ARG() and PSCI_RESULT_REG() were replaced
> with generic set_user_reg()/get_user_reg() functions.
This is a call to split the patch in multiple small ones to ease the review.

I like the idea of using SMCC for PSCI, and will review the code when it 
will be split.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] arm: traps: handle PSCI calls inside `smccc.c`

[Volodymyr Babchuk]

Hi Julien,

On 14.06.17 17:21, Julien Grall wrote:
>> PSCI is part of HVC/SMC interface, so it should be handled in
>> appropriate place: `smccc.c`. This patch just moves PSCI
>> handler calls from `traps.c` to `smccc.c`.
>>
>> PSCI is considered as two different "services" in terms of SMCCC.
>> Older PSCI 1.0 is treated as "architecture service", while never
>> PSCI 2.0 is defined as "standard secure service".
>>
>> Also old accessors PSCI_ARG() and PSCI_RESULT_REG() were replaced
>> with generic set_user_reg()/get_user_reg() functions.
> This is a call to split the patch in multiple small ones to ease the 
> review.
> 
> I like the idea of using SMCC for PSCI, and will review the code when it 
> will be split.

Okay, then I'll will send a separate patch that reworks PSCI code in 
traps.c, because this change is not relevant for SMCCC patch series.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/2] arm: traps: handle PSCI calls inside `smccc.c`

[Julien Grall]

On 06/14/2017 03:37 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,


> On 14.06.17 17:21, Julien Grall wrote:
>>> PSCI is part of HVC/SMC interface, so it should be handled in
>>> appropriate place: `smccc.c`. This patch just moves PSCI
>>> handler calls from `traps.c` to `smccc.c`.
>>>
>>> PSCI is considered as two different "services" in terms of SMCCC.
>>> Older PSCI 1.0 is treated as "architecture service", while never
>>> PSCI 2.0 is defined as "standard secure service".
>>>
>>> Also old accessors PSCI_ARG() and PSCI_RESULT_REG() were replaced
>>> with generic set_user_reg()/get_user_reg() functions.
>> This is a call to split the patch in multiple small ones to ease the 
>> review.
>>
>> I like the idea of using SMCC for PSCI, and will review the code when 
>> it will be split.
> 
> Okay, then I'll will send a separate patch that reworks PSCI code in 
> traps.c, because this change is not relevant for SMCCC patch series.

I would be ok if you append it to this series. Afterall, it is clean-up 
to implement SMCC properly :).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 0/4] Handle SMCs and HVCs in conformance with SMCCC

[Volodymyr Babchuk]

Hello all,

This is second version. Instead of 2 patches, there are 4 now.
I have divided PSCI patch into two: one changes how PSCI
code accesses registers and second one moves PSCI code with
new accessors to vsmc.c.

Also I had removed redundant 64 bit mode check in PSCI code, as it
does not conforms with SMCCC.

Per-patch changes are described in corresponding patch messages.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/4] arm: traps: psci: use generic register accessors

[Volodymyr Babchuk]

There are standard functions set_user_reg() and get_user_reg(). Use
them instead of PSCI_RESULT_REG()/PSCI_ARG() macros.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/traps.c | 68 ++++++++++++++++++++++------------------------------
 1 file changed, 29 insertions(+), 39 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6cf9ee7..2054c69 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1449,16 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
-#ifdef CONFIG_ARM_64
-#define PSCI_RESULT_REG(reg) (reg)->x0
-#define PSCI_ARG(reg,n) (reg)->x##n
-#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
-#else
-#define PSCI_RESULT_REG(reg) (reg)->r0
-#define PSCI_ARG(reg,n) (reg)->r##n
-#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
-#endif
-
 /* helper function for checking arm mode 32/64 bit */
 static inline int psci_mode_check(struct domain *d, register_t fid)
 {
@@ -1467,65 +1457,65 @@ static inline int psci_mode_check(struct domain *d, register_t fid)
 
 static void do_trap_psci(struct cpu_user_regs *regs)
 {
-    register_t fid = PSCI_ARG(regs,0);
+    register_t fid = get_user_reg(regs,0);
 
     /* preloading in case psci_mode_check fails */
-    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
+    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);
     switch( fid )
     {
     case PSCI_cpu_off:
         {
-            uint32_t pstate = PSCI_ARG32(regs,1);
+            uint32_t pstate = get_user_reg(regs, 1);
             perfc_incr(vpsci_cpu_off);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_off(pstate);
+            set_user_reg(regs, 0, do_psci_cpu_off(pstate));
         }
         break;
     case PSCI_cpu_on:
         {
-            uint32_t vcpuid = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
+            uint32_t vcpuid =  get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
             perfc_incr(vpsci_cpu_on);
-            PSCI_RESULT_REG(regs) = do_psci_cpu_on(vcpuid, epoint);
+            set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
         }
         break;
     case PSCI_0_2_FN_PSCI_VERSION:
         perfc_incr(vpsci_version);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_version();
+        set_user_reg(regs, 0, do_psci_0_2_version());
         break;
     case PSCI_0_2_FN_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_cpu_off();
+        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
         break;
     case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
-        PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_type();
+        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
         break;
     case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
     case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
         perfc_incr(vpsci_migrate_info_up_cpu);
         if ( psci_mode_check(current->domain, fid) )
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate_info_up_cpu();
+            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
         break;
     case PSCI_0_2_FN_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
-        PSCI_RESULT_REG(regs) = PSCI_INTERNAL_FAILURE;
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
         perfc_incr(vpsci_cpu_on);
         if ( psci_mode_check(current->domain, fid) )
         {
-            register_t vcpuid = PSCI_ARG(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_on(vcpuid, epoint, cid);
+            register_t vcpuid = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_CPU_SUSPEND:
@@ -1533,11 +1523,11 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_suspend);
         if ( psci_mode_check(current->domain, fid) )
         {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            register_t epoint = PSCI_ARG(regs,2);
-            register_t cid = PSCI_ARG(regs,3);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_cpu_suspend(pstate, epoint, cid);
+            uint32_t pstate = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_AFFINITY_INFO:
@@ -1545,10 +1535,10 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_affinity_info);
         if ( psci_mode_check(current->domain, fid) )
         {
-            register_t taff = PSCI_ARG(regs,1);
-            uint32_t laff = PSCI_ARG32(regs,2);
-            PSCI_RESULT_REG(regs) =
-                do_psci_0_2_affinity_info(taff, laff);
+            register_t taff = get_user_reg(regs, 1);
+            uint32_t laff = get_user_reg(regs, 2);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_affinity_info(taff, laff));
         }
         break;
     case PSCI_0_2_FN_MIGRATE:
@@ -1556,8 +1546,8 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         perfc_incr(vpsci_cpu_migrate);
         if ( psci_mode_check(current->domain, fid) )
         {
-            uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
+            uint32_t tcpu = get_user_reg(regs, 1);
+            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
         }
         break;
     default:
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/4] arm: traps: psci: use generic register accessors

[Julien Grall]

Hi Volodymyr,

On 22/06/17 17:24, Volodymyr Babchuk wrote:
> There are standard functions set_user_reg() and get_user_reg(). Use
> them instead of PSCI_RESULT_REG()/PSCI_ARG() macros.

Whilst I agree the use of {set,get}_user_reg(), I think we should 
abstract the call to make the code more readable.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/arch/arm/traps.c | 68 ++++++++++++++++++++++------------------------------
>  1 file changed, 29 insertions(+), 39 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 6cf9ee7..2054c69 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1449,16 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>
> -#ifdef CONFIG_ARM_64
> -#define PSCI_RESULT_REG(reg) (reg)->x0
> -#define PSCI_ARG(reg,n) (reg)->x##n
> -#define PSCI_ARG32(reg,n) (uint32_t)( (reg)->x##n & 0x00000000FFFFFFFF )
> -#else
> -#define PSCI_RESULT_REG(reg) (reg)->r0
> -#define PSCI_ARG(reg,n) (reg)->r##n
> -#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
> -#endif
> -
>  /* helper function for checking arm mode 32/64 bit */
>  static inline int psci_mode_check(struct domain *d, register_t fid)
>  {
> @@ -1467,65 +1457,65 @@ static inline int psci_mode_check(struct domain *d, register_t fid)
>
>  static void do_trap_psci(struct cpu_user_regs *regs)
>  {
> -    register_t fid = PSCI_ARG(regs,0);
> +    register_t fid = get_user_reg(regs,0);
>
>      /* preloading in case psci_mode_check fails */
> -    PSCI_RESULT_REG(regs) = PSCI_INVALID_PARAMETERS;
> +    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);

For instance, it is not clear for the user that register 0 is the result 
register. I would prefer if you introduce PSCI_SET_RESULT_REG(...).

>      switch( fid )
>      {
>      case PSCI_cpu_off:
>          {
> -            uint32_t pstate = PSCI_ARG32(regs,1);
> +            uint32_t pstate = get_user_reg(regs, 1);

PSCI_ARG32 is masking the top 32-bit of 64-bit register. Now, you are 
implicitly masking them. Which is much less obvious to read and error-prone.

I would much prefer is you re-implement PSCI_ARG32 in term of 
get_user_reg(...).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/4] arm: smccc: handle SMCs/HVCs according to SMCCC

[Volodymyr Babchuk]

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 `vsmc.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.

But, before SMC is forwarded to standard SMCCC handler, it can be routed
to a domain monitor, if one is installed.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
 - Moved UID definition to xen/include/public/arch-arm/smc.h
 - Renamed smccc.c to vsmc.c and smccc.h to vsmc.h
 - Reformated vsmc.h and commented definitions there
 - Added immediate value check for SMC64, HVC32 and HVC64
 - Added conditional flags check for SMC calls (HVC will be handled
   and checked in the next patch).
 - Added check for 64 bit calls from 32 bit guests
 - Removed HSR value passing as separate argument
 - Various changes in comments
---
 xen/arch/arm/Makefile             |   1 +
 xen/arch/arm/traps.c              |  16 ++++-
 xen/arch/arm/vsmc.c               | 128 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vsmc.h        |  94 ++++++++++++++++++++++++++++
 xen/include/public/arch-arm/smc.h |  45 ++++++++++++++
 5 files changed, 283 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/vsmc.c
 create mode 100644 xen/include/asm-arm/vsmc.h
 create mode 100644 xen/include/public/arch-arm/smc.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..4efd01c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
 obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 obj-y += vm_event.o
 obj-y += vtimer.o
+obj-y += vsmc.o
 obj-y += vpsci.o
 obj-y += vuart.o
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2054c69..66242e5 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/vsmc.h>
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2771,10 +2772,23 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
 {
     int rc = 0;
 
+    if ( !check_conditional_instr(regs, hsr) )
+    {
+        advance_pc(regs, hsr);
+        return;
+    }
+
+    /* If monitor is enabled, let it handle the call */
     if ( current->domain->arch.monitor.privileged_call_enabled )
         rc = monitor_smc();
 
-    if ( rc != 1 )
+    if ( rc == 1 )
+        return;
+
+    /* Use standard routines to handle the call */
+    if ( vsmc_handle_call(regs) )
+        advance_pc(regs, hsr);
+    else
         inject_undef_exception(regs, hsr);
 }
 
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
new file mode 100644
index 0000000..10c4acd
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,128 @@
+/*
+ * xen/arch/arm/vsmc.c
+ *
+ * Generic handler for SMC and HVC calls according to
+ * ARM SMC calling convention
+ *
+ * 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>
+/* 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 <public/arch-arm/smc.h>
+#include <asm/vsmc.h>
+#include <asm/regs.h>
+
+/*
+ * Hypervisor Service version
+ *
+ * We can't use XEN version here, because of SMCCC requirements:
+ * 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.
+ *
+ * Those values are subjected to change, when interface will be extended.
+ * They should not be stored in public/asm-arm/smc.h because they should
+ * be queried by guest using SMC/HVC interface.
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+/* SMCCC interface for hypervisor. Tell about itself. */
+static bool handle_hypervisor(struct cpu_user_regs *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;
+}
+
+/**
+ * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC
+ */
+int vsmc_handle_call(struct cpu_user_regs *regs)
+{
+    bool handled = false;
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /*
+     * Check immediate value for HVC32, HVC64 and SMC64.
+     * It is not so easy to check immediate value for SMC32,
+     * so we will assume that it is 0x0
+     */
+    switch ( hsr.ec )
+    {
+    case HSR_EC_HVC32:
+    case HSR_EC_HVC64:
+    case HSR_EC_SMC64:
+        if ( hsr.iss != 0)
+            return 0;
+        break;
+    case HSR_EC_SMC32:
+        break;
+    default:
+        return 0;
+    }
+
+    /* 64 bit calls are allowed only from 64 bit domains */
+    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+         is_32bit_domain(current->domain) )
+    {
+        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+        return 1;
+    }
+
+    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+    {
+    case ARM_SMCCC_OWNER_HYPERVISOR:
+        handled = handle_hypervisor(regs);
+        break;
+    }
+
+    if ( !handled )
+    {
+        gprintk(XENLOG_INFO, "Unhandled 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);
+    }
+
+    return 1;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
new file mode 100644
index 0000000..d2401c7
--- /dev/null
+++ b/xen/include/asm-arm/vsmc.h
@@ -0,0 +1,94 @@
+/*
+ * 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_VSMC_H__
+#define __ASM_ARM_VSMC_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
+
+/* Check if this is fast call */
+#define ARM_SMCCC_IS_FAST_CALL(smc_val)                         \
+    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+
+/* Check if this is 64 bit call  */
+#define ARM_SMCCC_IS_64(smc_val)                                        \
+    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+
+/* Get function number from function identifier */
+#define ARM_SMCCC_FUNC_NUM(smc_val)	((smc_val) & ARM_SMCCC_FUNC_MASK)
+
+/* Get service owner number from function identifier */
+#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
+    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+/*
+ * Construct function identifier from call type (fast or standard),
+ * calling convention (32 or 64 bit), service owner and function number
+ */
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
+    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
+     ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |              \
+     (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |      \
+     ((func_num) & ARM_SMCCC_FUNC_MASK))
+
+/* List of know 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)
+
+int vsmc_handle_call(struct cpu_user_regs *regs);
+
+#endif  /* __ASM_ARM_VSMC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
new file mode 100644
index 0000000..aac292a
--- /dev/null
+++ b/xen/include/public/arch-arm/smc.h
@@ -0,0 +1,45 @@
+/*
+ * smc.h
+ *
+ * SMC/HVC interface in accordance with SMC Calling Convention.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to
+ * deal in the Software without restriction, including without limitation the
+ * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
+ * sell copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ *
+ * Copyright 2017 (C) EPAM Systems
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
+#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
+
+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)}})
+
+
+/* Hypervisor Service UID. Randomly generated with 3rd party tool  */
+#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
+                                    0x9a, 0xcf, 0x79, 0xd1, \
+                                    0x8d, 0xde, 0xe6, 0x67)
+
+#endif	/* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/4] arm: smccc: handle SMCs/HVCs according to SMCCC

[Julien Grall]

Hi Volodymyr,

On 22/06/17 17:24, 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 `vsmc.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.
>
> But, before SMC is forwarded to standard SMCCC handler, it can be routed
> to a domain monitor, if one is installed.

Please address the comment I made on v1 after you sent it this version :).

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
>  - Moved UID definition to xen/include/public/arch-arm/smc.h
>  - Renamed smccc.c to vsmc.c and smccc.h to vsmc.h
>  - Reformated vsmc.h and commented definitions there
>  - Added immediate value check for SMC64, HVC32 and HVC64
>  - Added conditional flags check for SMC calls (HVC will be handled
>    and checked in the next patch).
>  - Added check for 64 bit calls from 32 bit guests
>  - Removed HSR value passing as separate argument
>  - Various changes in comments
> ---
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/traps.c              |  16 ++++-
>  xen/arch/arm/vsmc.c               | 128 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vsmc.h        |  94 ++++++++++++++++++++++++++++
>  xen/include/public/arch-arm/smc.h |  45 ++++++++++++++
>  5 files changed, 283 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/vsmc.c
>  create mode 100644 xen/include/asm-arm/vsmc.h
>  create mode 100644 xen/include/public/arch-arm/smc.h
>
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 49e1fb2..4efd01c 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
>  obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
>  obj-y += vm_event.o
>  obj-y += vtimer.o
> +obj-y += vsmc.o
>  obj-y += vpsci.o
>  obj-y += vuart.o
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 2054c69..66242e5 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/vsmc.h>
>
>  #include "decode.h"
>  #include "vtimer.h"
> @@ -2771,10 +2772,23 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>  {
>      int rc = 0;
>
> +    if ( !check_conditional_instr(regs, hsr) )

This change is not related to this patch and not explain in the commit 
message. To help reviewing, each patch should do one logical thing and 
not more. In this case, it should go in a separate patch.

Furthermore, if you look at the check_condition_instr implementation, it 
has a check

/* Unconditional Exception classes */
if ( hsr.ec >= 0x10 )
   return 1;

The EC for SMC32 is 0x13. So this is not going to work.

For SMC64, they are always unconditional and those bit is RES0. But you 
cannot assume, they will not be used for other purpose in the future.

Now, looking at the documentation for ISS for SMC32 trap (D7-2271 and 
G6-4957 in ARM DDI 0487B.a), compare to other conditional instruction 
the ISS has an extra field CCKNOWNPASS (bit 19) to tell you whether CV 
and COND are valid.

But on ARMv7, the ISS is UNK/SBZP. Meaning the software cannot rely on 
reading bits as all 0s. I have raised a question internally on how to 
write software compatible ARMv7 and ARMv8 AArch32. I will let you know 
the update.

Meanwhile, I think you can prepare a patch to support CCKNOWNPASS for 
AArch32 and AArch64 (please mention the ARMv7 problem in it so we don't 
merge it until it is been figured out).

Lastly, looking at the check in itself, I think it is wrong as EC 0 is 
always unconditional (see B3-25 in ARM DDI 0406C.c and D7-2259 in ARM 
DDI 0487B.a). It would be nice to have this fixed (in a separate patch) 
but not highly critical as not called for unknown exception at the moment.

> +    {
> +        advance_pc(regs, hsr);
> +        return;
> +    }
> +
> +    /* If monitor is enabled, let it handle the call */
>      if ( current->domain->arch.monitor.privileged_call_enabled )
>          rc = monitor_smc();
>
> -    if ( rc != 1 )
> +    if ( rc == 1 )
> +        return;
> +
> +    /* Use standard routines to handle the call */
> +    if ( vsmc_handle_call(regs) )
> +        advance_pc(regs, hsr);
> +    else
>          inject_undef_exception(regs, hsr);
>  }
>
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> new file mode 100644
> index 0000000..10c4acd
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,128 @@
> +/*
> + * xen/arch/arm/vsmc.c
> + *
> + * Generic handler for SMC and HVC calls according to
> + * ARM SMC calling convention
> + *
> + * 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>

xen/config.h is automatically included by the compiler. So please don't 
include it.

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

I don't see any inclusion of asm/domain.h in this patch.

> +#include <xen/sched.h>
> +#include <xen/stdbool.h>

Please don't include stdbool.h directly. This is already included by 
lib.h anyway and ...

> +#include <xen/types.h>

lib.h already include xen/types.h.

> +#include <public/arch-arm/smc.h>
> +#include <asm/vsmc.h>
> +#include <asm/regs.h>
> +
> +/*
> + * Hypervisor Service version
> + *
> + * We can't use XEN version here, because of SMCCC requirements:
> + * 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.
> + *
> + * Those values are subjected to change, when interface will be extended.
> + * They should not be stored in public/asm-arm/smc.h because they should

s/asm-arm/arch-arm/

> + * be queried by guest using SMC/HVC interface.

I am not sure if it is what we really want. From what you describe about 
the use of the SMCCC version. I would have expected the in-guest code to 
handle OP-TEE tie to a specific version of Xen. Very similar to other 
interface we version (see vm-event).

So I would make those versions public.

> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1
> +
> +/* Number of functions currently supported by Hypervisor Service. */
> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +/* SMCCC interface for hypervisor. Tell about itself. */
> +static bool handle_hypervisor(struct cpu_user_regs *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;
> +}
> +
> +/**
> + * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC
> + */
> +int vsmc_handle_call(struct cpu_user_regs *regs)

This function return either 0 or 1. So I think you can turned the return 
value into a boolean.

> +{
> +    bool handled = false;
> +    const union hsr hsr = { .bits = regs->hsr };
> +
> +    /*
> +     * Check immediate value for HVC32, HVC64 and SMC64.
> +     * It is not so easy to check immediate value for SMC32,

Please detail a bit more why it is not so easy.

> +     * so we will assume that it is 0x0

Missing full stop.

> +     */
> +    switch ( hsr.ec )
> +    {
> +    case HSR_EC_HVC32:
> +    case HSR_EC_HVC64:
> +    case HSR_EC_SMC64:
> +        if ( hsr.iss != 0)

I know the current HVC code is check hsr.iss != 0. But this is actually 
wrong. You cannot rely on the bits [24:16] to be 0 in the future.

> +            return 0;
> +        break;
> +    case HSR_EC_SMC32:
> +        break;
> +    default:
> +        return 0;
> +    }
> +
> +    /* 64 bit calls are allowed only from 64 bit domains */
> +    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
> +         is_32bit_domain(current->domain) )
> +    {
> +        set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
> +        return 1;
> +    }
> +
> +    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
> +    {
> +    case ARM_SMCCC_OWNER_HYPERVISOR:
> +        handled = handle_hypervisor(regs);
> +        break;
> +    }
> +
> +    if ( !handled )
> +    {
> +        gprintk(XENLOG_INFO, "Unhandled 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);
> +    }
> +
> +    return 1;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> new file mode 100644
> index 0000000..d2401c7
> --- /dev/null
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -0,0 +1,94 @@

On v1, I've asked to use the Xen coding style in this file. This has not 
been done.

Please switch *all* the hard tab to soft tab.

> +/*
> + * 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_VSMC_H__
> +#define __ASM_ARM_VSMC_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

By default, any numeric is type int (i.e signed). However 1 << 31 is 
undefined for signed number.

So please use 0U and 1U. A good habit is to always append U or UL 
depending on the type wanted.

I know that we have some places where we don't respect that. I have a 
series in plan to clean-up that.

> +#define ARM_SMCCC_TYPE_SHIFT		31
> +
> +#define ARM_SMCCC_SMC_32		0
> +#define ARM_SMCCC_SMC_64		1

See above.

> +#define ARM_SMCCC_CALL_CONV_SHIFT	30
> +
> +#define ARM_SMCCC_OWNER_MASK		0x3F
> +#define ARM_SMCCC_OWNER_SHIFT		24
> +
> +#define ARM_SMCCC_FUNC_MASK		0xFFFF
> +
> +/* Check if this is fast call */
> +#define ARM_SMCCC_IS_FAST_CALL(smc_val)                         \
> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
> +
> +/* Check if this is 64 bit call  */
> +#define ARM_SMCCC_IS_64(smc_val)                                        \
> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
> +
> +/* Get function number from function identifier */
> +#define ARM_SMCCC_FUNC_NUM(smc_val)	((smc_val) & ARM_SMCCC_FUNC_MASK)
> +
> +/* Get service owner number from function identifier */
> +#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
> +
> +/*
> + * Construct function identifier from call type (fast or standard),
> + * calling convention (32 or 64 bit), service owner and function number
> + */
> +#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
> +    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
> +     ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |              \
> +     (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |      \
> +     ((func_num) & ARM_SMCCC_FUNC_MASK))
> +
> +/* List of know service owners */

s/know/known/

> +#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)
> +
> +int vsmc_handle_call(struct cpu_user_regs *regs);
> +
> +#endif  /* __ASM_ARM_VSMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> new file mode 100644
> index 0000000..aac292a
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smc.h

See above for the coding style.

> @@ -0,0 +1,45 @@
> +/*
> + * smc.h
> + *
> + * SMC/HVC interface in accordance with SMC Calling Convention.
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a copy
> + * of this software and associated documentation files (the "Software"), to
> + * deal in the Software without restriction, including without limitation the
> + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
> + * sell copies of the Software, and to permit persons to whom the Software is
> + * furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice shall be included in
> + * all copies or substantial portions of the Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
> + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
> + * DEALINGS IN THE SOFTWARE.
> + *
> + * Copyright 2017 (C) EPAM Systems
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +#define __XEN_PUBLIC_ARCH_ARM_SMC_H__
> +
> +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)}})

This is public headers, meaning they will get exported to userspace, 
kernel... I am a concerned that name with ARM_* will clash with 
something else.

I think we should at least prefix with XEN_ to prevent that. Same for 
arm_smccc_uid.

> +
> +
> +/* Hypervisor Service UID. Randomly generated with 3rd party tool  */
> +#define XEN_SMCCC_UID ARM_SMCCC_UID(0xa71812dc, 0xc698, 0x4369, \
> +                                    0x9a, 0xcf, 0x79, 0xd1, \
> +                                    0x8d, 0xde, 0xe6, 0x67)
> +
> +#endif	/* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
>

Missing emacs magic here.


Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/4] arm: smccc: handle SMCs/HVCs according to SMCCC

[Julien Grall]

On 30/06/17 16:15, Julien Grall wrote:
> Now, looking at the documentation for ISS for SMC32 trap (D7-2271 and
> G6-4957 in ARM DDI 0487B.a), compare to other conditional instruction
> the ISS has an extra field CCKNOWNPASS (bit 19) to tell you whether CV
> and COND are valid.
>
> But on ARMv7, the ISS is UNK/SBZP. Meaning the software cannot rely on
> reading bits as all 0s. I have raised a question internally on how to
> write software compatible ARMv7 and ARMv8 AArch32. I will let you know
> the update.
>
> Meanwhile, I think you can prepare a patch to support CCKNOWNPASS for
> AArch32 and AArch64 (please mention the ARMv7 problem in it so we don't
> merge it until it is been figured out).

I got an answer on this one. The policy is Should-Be-Zero-Preserved, if 
you do read-modify-write you have to preserve the values. For read-only 
operation it will appear as 0 on older revision and 0/1 on new revision.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 3/4] arm: traps: handle PSCI calls inside `vsmc.c`

[Volodymyr Babchuk]

PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `vsmc.c`. This patch just moves PSCI
handler calls from `traps.c` to `vsmc.c`.

PSCI is considered as two different "services" in terms of SMCCC.
Older PSCI 1.0 is treated as "architecture service", while never
PSCI 2.0 is defined as "standard secure service".

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
---
Splitted this patch into two. Now this patch does not change the way,
how PSCI code accesses the arguments.
---
 xen/arch/arm/traps.c              | 124 ++++------------------------------
 xen/arch/arm/vsmc.c               | 136 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/arch-arm/smc.h |   5 ++
 3 files changed, 153 insertions(+), 112 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 66242e5..e806474 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -39,7 +39,6 @@
 #include <asm/event.h>
 #include <asm/regs.h>
 #include <asm/cpregs.h>
-#include <asm/psci.h>
 #include <asm/mmio.h>
 #include <asm/cpufeature.h>
 #include <asm/flushtlb.h>
@@ -1450,113 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
-/* helper function for checking arm mode 32/64 bit */
-static inline int psci_mode_check(struct domain *d, register_t fid)
-{
-        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
-}
-
-static void do_trap_psci(struct cpu_user_regs *regs)
-{
-    register_t fid = get_user_reg(regs,0);
-
-    /* preloading in case psci_mode_check fails */
-    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);
-    switch( fid )
-    {
-    case PSCI_cpu_off:
-        {
-            uint32_t pstate = get_user_reg(regs, 1);
-            perfc_incr(vpsci_cpu_off);
-            set_user_reg(regs, 0, do_psci_cpu_off(pstate));
-        }
-        break;
-    case PSCI_cpu_on:
-        {
-            uint32_t vcpuid =  get_user_reg(regs, 1);
-            register_t epoint = get_user_reg(regs, 2);
-            perfc_incr(vpsci_cpu_on);
-            set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
-        }
-        break;
-    case PSCI_0_2_FN_PSCI_VERSION:
-        perfc_incr(vpsci_version);
-        set_user_reg(regs, 0, do_psci_0_2_version());
-        break;
-    case PSCI_0_2_FN_CPU_OFF:
-        perfc_incr(vpsci_cpu_off);
-        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-        perfc_incr(vpsci_migrate_info_type);
-        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
-    case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
-        perfc_incr(vpsci_migrate_info_up_cpu);
-        if ( psci_mode_check(current->domain, fid) )
-            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
-        break;
-    case PSCI_0_2_FN_SYSTEM_OFF:
-        perfc_incr(vpsci_system_off);
-        do_psci_0_2_system_off();
-        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_SYSTEM_RESET:
-        perfc_incr(vpsci_system_reset);
-        do_psci_0_2_system_reset();
-        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_CPU_ON:
-    case PSCI_0_2_FN64_CPU_ON:
-        perfc_incr(vpsci_cpu_on);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t vcpuid = get_user_reg(regs, 1);
-            register_t epoint = get_user_reg(regs, 2);
-            register_t cid = get_user_reg(regs, 3);
-            set_user_reg(regs, 0,
-                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
-        }
-        break;
-    case PSCI_0_2_FN_CPU_SUSPEND:
-    case PSCI_0_2_FN64_CPU_SUSPEND:
-        perfc_incr(vpsci_cpu_suspend);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t pstate = get_user_reg(regs, 1);
-            register_t epoint = get_user_reg(regs, 2);
-            register_t cid = get_user_reg(regs, 3);
-            set_user_reg(regs, 0,
-                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
-        }
-        break;
-    case PSCI_0_2_FN_AFFINITY_INFO:
-    case PSCI_0_2_FN64_AFFINITY_INFO:
-        perfc_incr(vpsci_cpu_affinity_info);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            register_t taff = get_user_reg(regs, 1);
-            uint32_t laff = get_user_reg(regs, 2);
-            set_user_reg(regs, 0,
-                         do_psci_0_2_affinity_info(taff, laff));
-        }
-        break;
-    case PSCI_0_2_FN_MIGRATE:
-    case PSCI_0_2_FN64_MIGRATE:
-        perfc_incr(vpsci_cpu_migrate);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t tcpu = get_user_reg(regs, 1);
-            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
-        }
-        break;
-    default:
-        domain_crash_synchronous();
-        return;
-    }
-}
-
 #ifdef CONFIG_ARM_64
 #define HYPERCALL_RESULT_REG(r) (r)->x0
 #define HYPERCALL_ARG1(r) (r)->x0
@@ -2888,8 +2780,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
+        {
+            if ( !vsmc_handle_call(regs) )
+                domain_crash_synchronous();
+        }
+        else
+            do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
         break;
 #ifdef CONFIG_ARM_64
     case HSR_EC_HVC64:
@@ -2900,8 +2796,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
             return do_debug_trap(regs, hsr.iss & 0x00ff);
 #endif
         if ( hsr.iss == 0 )
-            return do_trap_psci(regs);
-        do_trap_hypercall(regs, &regs->x16, hsr.iss);
+        {
+            if ( !vsmc_handle_call(regs) )
+                domain_crash_synchronous();
+        }
+        else
+            do_trap_hypercall(regs, &regs->x16, hsr.iss);
         break;
     case HSR_EC_SMC64:
         /*
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 10c4acd..5f10fd1 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -22,6 +22,7 @@
 #include <xen/stdbool.h>
 #include <xen/types.h>
 #include <public/arch-arm/smc.h>
+#include <asm/psci.h>
 #include <asm/vsmc.h>
 #include <asm/regs.h>
 
@@ -43,6 +44,14 @@
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
 
+/* Standard Service version. Check comment for Hypervisor Service for rules */
+#define SSC_SMCCC_MAJOR_REVISION 0
+#define SSC_SMCCC_MINOR_REVISION 1
+
+/* Number of functions currently supported by Standard Service Service. */
+#define SSC_SMCCC_FUNCTION_COUNT 13
+
+
 /* SMCCC interface for hypervisor. Tell about itself. */
 static bool handle_hypervisor(struct cpu_user_regs *regs)
 {
@@ -65,6 +74,127 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
     return false;
 }
 
+/* old (arvm7) PSCI interface */
+static bool handle_arch(struct cpu_user_regs *regs)
+{
+    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
+    {
+    case PSCI_cpu_off:
+    {
+        uint32_t pstate = get_user_reg(regs, 1);
+        perfc_incr(vpsci_cpu_off);
+        set_user_reg(regs, 0, do_psci_cpu_off(pstate));
+    }
+    return true;
+    case PSCI_cpu_on:
+    {
+        uint32_t vcpuid = get_user_reg(regs, 1);
+        register_t epoint = get_user_reg(regs, 2);
+        perfc_incr(vpsci_cpu_on);
+        set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
+    }
+    return true;
+    }
+    return false;
+}
+
+/* helper function for checking arm mode 32/64 bit */
+static inline int psci_mode_check(struct domain *d, register_t fid)
+{
+        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
+}
+
+/* PSCI 2.0 interface */
+static bool handle_ssc(struct cpu_user_regs *regs)
+{
+    register_t fid = get_user_reg(regs, 0);
+
+    switch ( ARM_SMCCC_FUNC_NUM(fid) )
+    {
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+        perfc_incr(vpsci_version);
+        set_user_reg(regs, 0, do_psci_0_2_version());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+        perfc_incr(vpsci_cpu_off);
+        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+        perfc_incr(vpsci_migrate_info_type);
+        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+        perfc_incr(vpsci_migrate_info_up_cpu);
+        if ( psci_mode_check(current->domain, fid) )
+            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+        perfc_incr(vpsci_system_off);
+        do_psci_0_2_system_off();
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+        perfc_incr(vpsci_system_reset);
+        do_psci_0_2_system_reset();
+        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+        perfc_incr(vpsci_cpu_on);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t vcpuid = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+        perfc_incr(vpsci_cpu_suspend);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t pstate = get_user_reg(regs, 1);
+            register_t epoint = get_user_reg(regs, 2);
+            register_t cid = get_user_reg(regs, 3);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+        perfc_incr(vpsci_cpu_affinity_info);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            register_t taff = get_user_reg(regs, 1);
+            uint32_t laff = get_user_reg(regs,2);
+            set_user_reg(regs, 0,
+                         do_psci_0_2_affinity_info(taff, laff));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+        perfc_incr(vpsci_cpu_migrate);
+        if ( psci_mode_check(current->domain, fid) )
+        {
+            uint32_t tcpu = get_user_reg(regs, 1);
+            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+        set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
+        set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
+        set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
+        set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
 /**
  * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC
  */
@@ -105,6 +235,12 @@ int vsmc_handle_call(struct cpu_user_regs *regs)
     case ARM_SMCCC_OWNER_HYPERVISOR:
         handled = handle_hypervisor(regs);
         break;
+    case ARM_SMCCC_OWNER_ARCH:
+        handled = handle_arch(regs);
+        break;
+    case ARM_SMCCC_OWNER_STANDARD:
+        handled = handle_ssc(regs);
+        break;
     }
 
     if ( !handled )
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
index aac292a..6a5a556 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -42,4 +42,9 @@ typedef struct {
                                     0x9a, 0xcf, 0x79, 0xd1, \
                                     0x8d, 0xde, 0xe6, 0x67)
 
+/* Standard Service Call UID. Randomly generated with 3rd party tool */
+#define SSC_SMCCC_UID ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
+                                    0x92, 0x20, 0xce, 0x16, \
+                                    0x41, 0xe5, 0x9f, 0x6f)
+
 #endif	/* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/4] arm: traps: handle PSCI calls inside `vsmc.c`

[Stefano Stabellini]

On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
> PSCI is part of HVC/SMC interface, so it should be handled in
> appropriate place: `vsmc.c`. This patch just moves PSCI
> handler calls from `traps.c` to `vsmc.c`.
> 
> PSCI is considered as two different "services" in terms of SMCCC.
> Older PSCI 1.0 is treated as "architecture service", while never
> PSCI 2.0 is defined as "standard secure service".
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> Reviewed-by: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
> ---
> Splitted this patch into two. Now this patch does not change the way,
> how PSCI code accesses the arguments.
> ---
>  xen/arch/arm/traps.c              | 124 ++++------------------------------
>  xen/arch/arm/vsmc.c               | 136 ++++++++++++++++++++++++++++++++++++++
>  xen/include/public/arch-arm/smc.h |   5 ++
>  3 files changed, 153 insertions(+), 112 deletions(-)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 66242e5..e806474 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -39,7 +39,6 @@
>  #include <asm/event.h>
>  #include <asm/regs.h>
>  #include <asm/cpregs.h>
> -#include <asm/psci.h>
>  #include <asm/mmio.h>
>  #include <asm/cpufeature.h>
>  #include <asm/flushtlb.h>
> @@ -1450,113 +1449,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>  
> -/* helper function for checking arm mode 32/64 bit */
> -static inline int psci_mode_check(struct domain *d, register_t fid)
> -{
> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> -}
> -
> -static void do_trap_psci(struct cpu_user_regs *regs)
> -{
> -    register_t fid = get_user_reg(regs,0);
> -
> -    /* preloading in case psci_mode_check fails */
> -    set_user_reg(regs, 0, PSCI_INVALID_PARAMETERS);
> -    switch( fid )
> -    {
> -    case PSCI_cpu_off:
> -        {
> -            uint32_t pstate = get_user_reg(regs, 1);
> -            perfc_incr(vpsci_cpu_off);
> -            set_user_reg(regs, 0, do_psci_cpu_off(pstate));
> -        }
> -        break;
> -    case PSCI_cpu_on:
> -        {
> -            uint32_t vcpuid =  get_user_reg(regs, 1);
> -            register_t epoint = get_user_reg(regs, 2);
> -            perfc_incr(vpsci_cpu_on);
> -            set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
> -        }
> -        break;
> -    case PSCI_0_2_FN_PSCI_VERSION:
> -        perfc_incr(vpsci_version);
> -        set_user_reg(regs, 0, do_psci_0_2_version());
> -        break;
> -    case PSCI_0_2_FN_CPU_OFF:
> -        perfc_incr(vpsci_cpu_off);
> -        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
> -        break;
> -    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> -        perfc_incr(vpsci_migrate_info_type);
> -        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
> -        break;
> -    case PSCI_0_2_FN_MIGRATE_INFO_UP_CPU:
> -    case PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU:
> -        perfc_incr(vpsci_migrate_info_up_cpu);
> -        if ( psci_mode_check(current->domain, fid) )
> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> -        break;
> -    case PSCI_0_2_FN_SYSTEM_OFF:
> -        perfc_incr(vpsci_system_off);
> -        do_psci_0_2_system_off();
> -        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
> -        break;
> -    case PSCI_0_2_FN_SYSTEM_RESET:
> -        perfc_incr(vpsci_system_reset);
> -        do_psci_0_2_system_reset();
> -        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
> -        break;
> -    case PSCI_0_2_FN_CPU_ON:
> -    case PSCI_0_2_FN64_CPU_ON:
> -        perfc_incr(vpsci_cpu_on);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            register_t vcpuid = get_user_reg(regs, 1);
> -            register_t epoint = get_user_reg(regs, 2);
> -            register_t cid = get_user_reg(regs, 3);
> -            set_user_reg(regs, 0,
> -                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> -        }
> -        break;
> -    case PSCI_0_2_FN_CPU_SUSPEND:
> -    case PSCI_0_2_FN64_CPU_SUSPEND:
> -        perfc_incr(vpsci_cpu_suspend);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            uint32_t pstate = get_user_reg(regs, 1);
> -            register_t epoint = get_user_reg(regs, 2);
> -            register_t cid = get_user_reg(regs, 3);
> -            set_user_reg(regs, 0,
> -                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> -        }
> -        break;
> -    case PSCI_0_2_FN_AFFINITY_INFO:
> -    case PSCI_0_2_FN64_AFFINITY_INFO:
> -        perfc_incr(vpsci_cpu_affinity_info);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            register_t taff = get_user_reg(regs, 1);
> -            uint32_t laff = get_user_reg(regs, 2);
> -            set_user_reg(regs, 0,
> -                         do_psci_0_2_affinity_info(taff, laff));
> -        }
> -        break;
> -    case PSCI_0_2_FN_MIGRATE:
> -    case PSCI_0_2_FN64_MIGRATE:
> -        perfc_incr(vpsci_cpu_migrate);
> -        if ( psci_mode_check(current->domain, fid) )
> -        {
> -            uint32_t tcpu = get_user_reg(regs, 1);
> -            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
> -        }
> -        break;
> -    default:
> -        domain_crash_synchronous();
> -        return;
> -    }
> -}
> -
>  #ifdef CONFIG_ARM_64
>  #define HYPERCALL_RESULT_REG(r) (r)->x0
>  #define HYPERCALL_ARG1(r) (r)->x0
> @@ -2888,8 +2780,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>              return do_debug_trap(regs, hsr.iss & 0x00ff);
>  #endif
>          if ( hsr.iss == 0 )
> -            return do_trap_psci(regs);
> -        do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
> +        {
> +            if ( !vsmc_handle_call(regs) )
> +                domain_crash_synchronous();
> +        }
> +        else
> +            do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
>          break;
>  #ifdef CONFIG_ARM_64
>      case HSR_EC_HVC64:
> @@ -2900,8 +2796,12 @@ asmlinkage void do_trap_guest_sync(struct cpu_user_regs *regs)
>              return do_debug_trap(regs, hsr.iss & 0x00ff);
>  #endif
>          if ( hsr.iss == 0 )
> -            return do_trap_psci(regs);
> -        do_trap_hypercall(regs, &regs->x16, hsr.iss);
> +        {
> +            if ( !vsmc_handle_call(regs) )
> +                domain_crash_synchronous();
> +        }
> +        else
> +            do_trap_hypercall(regs, &regs->x16, hsr.iss);
>          break;
>      case HSR_EC_SMC64:
>          /*
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 10c4acd..5f10fd1 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -22,6 +22,7 @@
>  #include <xen/stdbool.h>
>  #include <xen/types.h>
>  #include <public/arch-arm/smc.h>
> +#include <asm/psci.h>
>  #include <asm/vsmc.h>
>  #include <asm/regs.h>
>  
> @@ -43,6 +44,14 @@
>  /* Number of functions currently supported by Hypervisor Service. */
>  #define XEN_SMCCC_FUNCTION_COUNT 3
>  
> +/* Standard Service version. Check comment for Hypervisor Service for rules */
> +#define SSC_SMCCC_MAJOR_REVISION 0
> +#define SSC_SMCCC_MINOR_REVISION 1
> +
> +/* Number of functions currently supported by Standard Service Service. */
> +#define SSC_SMCCC_FUNCTION_COUNT 13
> +
> +
>  /* SMCCC interface for hypervisor. Tell about itself. */
>  static bool handle_hypervisor(struct cpu_user_regs *regs)
>  {
> @@ -65,6 +74,127 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
>      return false;
>  }
>  
> +/* old (arvm7) PSCI interface */
> +static bool handle_arch(struct cpu_user_regs *regs)
> +{
> +    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
> +    {
> +    case PSCI_cpu_off:
> +    {
> +        uint32_t pstate = get_user_reg(regs, 1);
> +        perfc_incr(vpsci_cpu_off);
> +        set_user_reg(regs, 0, do_psci_cpu_off(pstate));
> +    }
> +    return true;

Don't you want the `return true' to be within the { } block? It looks
weird this way.


> +    case PSCI_cpu_on:
> +    {
> +        uint32_t vcpuid = get_user_reg(regs, 1);
> +        register_t epoint = get_user_reg(regs, 2);
> +        perfc_incr(vpsci_cpu_on);
> +        set_user_reg(regs, 0, do_psci_cpu_on(vcpuid, epoint));
> +    }
> +    return true;

Same here.


> +    }
> +    return false;
> +}
> +
> +/* helper function for checking arm mode 32/64 bit */
> +static inline int psci_mode_check(struct domain *d, register_t fid)
> +{
> +        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> +}
> +
> +/* PSCI 2.0 interface */
> +static bool handle_ssc(struct cpu_user_regs *regs)
> +{
> +    register_t fid = get_user_reg(regs, 0);
> +
> +    switch ( ARM_SMCCC_FUNC_NUM(fid) )
> +    {
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):

As we are not using the PSCI_0_2_FN64_* definitions anymore, it would
make sense to add a comment in psci.h on top of them to explain why they
are not used (the function number is the same as the 32-bit
counterpart).


> +        perfc_incr(vpsci_version);
> +        set_user_reg(regs, 0, do_psci_0_2_version());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> +        perfc_incr(vpsci_cpu_off);
> +        set_user_reg(regs, 0, do_psci_0_2_cpu_off());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> +        perfc_incr(vpsci_migrate_info_type);
> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_type());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> +        perfc_incr(vpsci_migrate_info_up_cpu);
> +        if ( psci_mode_check(current->domain, fid) )
> +            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
> +        perfc_incr(vpsci_system_off);
> +        do_psci_0_2_system_off();
> +        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
> +        perfc_incr(vpsci_system_reset);
> +        do_psci_0_2_system_reset();
> +        set_user_reg(regs, 0, PSCI_INTERNAL_FAILURE);
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
> +        perfc_incr(vpsci_cpu_on);
> +        if ( psci_mode_check(current->domain, fid) )
> +        {
> +            register_t vcpuid = get_user_reg(regs, 1);
> +            register_t epoint = get_user_reg(regs, 2);
> +            register_t cid = get_user_reg(regs, 3);
> +            set_user_reg(regs, 0,
> +                         do_psci_0_2_cpu_on(vcpuid, epoint, cid));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
> +        perfc_incr(vpsci_cpu_suspend);
> +        if ( psci_mode_check(current->domain, fid) )
> +        {
> +            uint32_t pstate = get_user_reg(regs, 1);
> +            register_t epoint = get_user_reg(regs, 2);
> +            register_t cid = get_user_reg(regs, 3);
> +            set_user_reg(regs, 0,
> +                         do_psci_0_2_cpu_suspend(pstate, epoint, cid));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
> +        perfc_incr(vpsci_cpu_affinity_info);
> +        if ( psci_mode_check(current->domain, fid) )
> +        {
> +            register_t taff = get_user_reg(regs, 1);
> +            uint32_t laff = get_user_reg(regs,2);
> +            set_user_reg(regs, 0,
> +                         do_psci_0_2_affinity_info(taff, laff));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
> +        perfc_incr(vpsci_cpu_migrate);
> +        if ( psci_mode_check(current->domain, fid) )
> +        {
> +            uint32_t tcpu = get_user_reg(regs, 1);
> +            set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, SSC_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +        set_user_reg(regs, 0, SSC_SMCCC_UID.a[0]);
> +        set_user_reg(regs, 1, SSC_SMCCC_UID.a[1]);
> +        set_user_reg(regs, 2, SSC_SMCCC_UID.a[2]);
> +        set_user_reg(regs, 3, SSC_SMCCC_UID.a[3]);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, SSC_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, SSC_SMCCC_MINOR_REVISION);
> +        return true;
> +    }
> +    return false;
> +}
> +
>  /**
>   * vsmc_handle_call() - handle SMC/HVC call according to ARM SMCCC
>   */
> @@ -105,6 +235,12 @@ int vsmc_handle_call(struct cpu_user_regs *regs)
>      case ARM_SMCCC_OWNER_HYPERVISOR:
>          handled = handle_hypervisor(regs);
>          break;
> +    case ARM_SMCCC_OWNER_ARCH:
> +        handled = handle_arch(regs);
> +        break;
> +    case ARM_SMCCC_OWNER_STANDARD:
> +        handled = handle_ssc(regs);
> +        break;
>      }
>  

NIT: Remove double space in the patch where it was introduced


>      if ( !handled )
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> index aac292a..6a5a556 100644
> --- a/xen/include/public/arch-arm/smc.h
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -42,4 +42,9 @@ typedef struct {
>                                      0x9a, 0xcf, 0x79, 0xd1, \
>                                      0x8d, 0xde, 0xe6, 0x67)
>  

NIT: Remove double space in the patch where it was introduced


> +/* Standard Service Call UID. Randomly generated with 3rd party tool */
> +#define SSC_SMCCC_UID ARM_SMCCC_UID(0xf863386f, 0x4b39, 0x4cbd, \
> +                                    0x92, 0x20, 0xce, 0x16, \
> +                                    0x41, 0xe5, 0x9f, 0x6f)

NIT: please align the \

These are only minor issues. The patch looks pretty good.


>  #endif	/* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> -- 
> 2.7.4
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/4] arm: traps: handle PSCI calls inside `vsmc.c`

[Julien Grall]

Hi,

On 06/30/2017 10:13 PM, Stefano Stabellini wrote:
> On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
>> +    }
>> +    return false;
>> +}
>> +
>> +/* helper function for checking arm mode 32/64 bit */
>> +static inline int psci_mode_check(struct domain *d, register_t fid)
>> +{
>> +        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
>> +}
>> +
>> +/* PSCI 2.0 interface */
>> +static bool handle_ssc(struct cpu_user_regs *regs)
>> +{
>> +    register_t fid = get_user_reg(regs, 0);
>> +
>> +    switch ( ARM_SMCCC_FUNC_NUM(fid) )
>> +    {
>> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> 
> As we are not using the PSCI_0_2_FN64_* definitions anymore, it would
> make sense to add a comment in psci.h on top of them to explain why they
> are not used (the function number is the same as the 32-bit
> counterpart).

Well they are not unused (see arch/arm/psci.c).  But I think it is quite 
confusing to use ARM_SMCC_FUNC_NUM(PSCI_0_2_PSCI_VERSION) to extract the 
function number from the 32-bit version.

It would be much better macro for the function number (i.e abstracting 
0, 1, 2,...).

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Volodymyr Babchuk]

PSCI handling code had helper routine that checked calling convention.
It does not needed anymore, because:

 - Generic handler checks that 64 bit calls can be made only by
   64 bit guests.

 - SMCCC requires that 64-bit handler should support both 32 and 64 bit
   calls even if they originate from 64 bit caller.

This patch removes that extra check.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/arch/arm/vsmc.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 5f10fd1..1983e0e 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
     return false;
 }
 
-/* helper function for checking arm mode 32/64 bit */
-static inline int psci_mode_check(struct domain *d, register_t fid)
-{
-        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
-}
-
 /* PSCI 2.0 interface */
 static bool handle_ssc(struct cpu_user_regs *regs)
 {
@@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
         perfc_incr(vpsci_migrate_info_up_cpu);
-        if ( psci_mode_check(current->domain, fid) )
-            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
+        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
         perfc_incr(vpsci_system_off);
@@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
         perfc_incr(vpsci_cpu_on);
-        if ( psci_mode_check(current->domain, fid) )
         {
             register_t vcpuid = get_user_reg(regs, 1);
             register_t epoint = get_user_reg(regs, 2);
@@ -151,7 +143,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
         perfc_incr(vpsci_cpu_suspend);
-        if ( psci_mode_check(current->domain, fid) )
         {
             uint32_t pstate = get_user_reg(regs, 1);
             register_t epoint = get_user_reg(regs, 2);
@@ -162,7 +153,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
         perfc_incr(vpsci_cpu_affinity_info);
-        if ( psci_mode_check(current->domain, fid) )
         {
             register_t taff = get_user_reg(regs, 1);
             uint32_t laff = get_user_reg(regs,2);
@@ -172,7 +162,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
         return true;
     case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
         perfc_incr(vpsci_cpu_migrate);
-        if ( psci_mode_check(current->domain, fid) )
         {
             uint32_t tcpu = get_user_reg(regs, 1);
             set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Stefano Stabellini]

On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
> PSCI handling code had helper routine that checked calling convention.
> It does not needed anymore, because:
> 
>  - Generic handler checks that 64 bit calls can be made only by
>    64 bit guests.
> 
>  - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>    calls even if they originate from 64 bit caller.
> 
> This patch removes that extra check.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/arch/arm/vsmc.c | 13 +------------
>  1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 5f10fd1..1983e0e 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>      return false;
>  }
>  
> -/* helper function for checking arm mode 32/64 bit */
> -static inline int psci_mode_check(struct domain *d, register_t fid)
> -{
> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> -}
> -
>  /* PSCI 2.0 interface */
>  static bool handle_ssc(struct cpu_user_regs *regs)
>  {
> @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>          perfc_incr(vpsci_migrate_info_up_cpu);
> -        if ( psci_mode_check(current->domain, fid) )
> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>          perfc_incr(vpsci_system_off);
> @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>          perfc_incr(vpsci_cpu_on);
> -        if ( psci_mode_check(current->domain, fid) )

I would prefer if the `return true' was within the { } block. But anyway
it's just a code style issue, so:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


>          {
>              register_t vcpuid = get_user_reg(regs, 1);
>              register_t epoint = get_user_reg(regs, 2);
> @@ -151,7 +143,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
>          perfc_incr(vpsci_cpu_suspend);
> -        if ( psci_mode_check(current->domain, fid) )
>          {
>              uint32_t pstate = get_user_reg(regs, 1);
>              register_t epoint = get_user_reg(regs, 2);
> @@ -162,7 +153,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
>          perfc_incr(vpsci_cpu_affinity_info);
> -        if ( psci_mode_check(current->domain, fid) )
>          {
>              register_t taff = get_user_reg(regs, 1);
>              uint32_t laff = get_user_reg(regs,2);
> @@ -172,7 +162,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>          return true;
>      case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
>          perfc_incr(vpsci_cpu_migrate);
> -        if ( psci_mode_check(current->domain, fid) )
>          {
>              uint32_t tcpu = get_user_reg(regs, 1);
>              set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
> -- 
> 2.7.4
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Julien Grall]

Hi,

On 06/30/2017 10:19 PM, Stefano Stabellini wrote:
> On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
>> PSCI handling code had helper routine that checked calling convention.
>> It does not needed anymore, because:
>>
>>   - Generic handler checks that 64 bit calls can be made only by
>>     64 bit guests.
>>
>>   - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>>     calls even if they originate from 64 bit caller.
>>
>> This patch removes that extra check.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/vsmc.c | 13 +------------
>>   1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 5f10fd1..1983e0e 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>>       return false;
>>   }
>>   
>> -/* helper function for checking arm mode 32/64 bit */
>> -static inline int psci_mode_check(struct domain *d, register_t fid)
>> -{
>> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
>> -}
>> -
>>   /* PSCI 2.0 interface */
>>   static bool handle_ssc(struct cpu_user_regs *regs)
>>   {
>> @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>>           perfc_incr(vpsci_migrate_info_up_cpu);
>> -        if ( psci_mode_check(current->domain, fid) )
>> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>>           perfc_incr(vpsci_system_off);
>> @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>>           perfc_incr(vpsci_cpu_on);
>> -        if ( psci_mode_check(current->domain, fid) )
> 
> I would prefer if the `return true' was within the { } block. But anyway
> it's just a code style issue, so:

Well, I think we should keep the coding style consistent within 
arch/arm. If we have the return true within {} in other place. Then this 
should be done here.

In general, { } should only be used to en-globe everything in a case or 
for if/else/while/for with more than a line. All the other kind of { } 
should be avoided. I particularly dislike any code doing

code

{
    variable definition;

    code
}

code

Unless you have a strong reason to do it (avoiding reworking the code is 
not one), I will nack any code resulting to that.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Stefano Stabellini]

On Sun, 2 Jul 2017, Julien Grall wrote:
> Hi,
> 
> On 06/30/2017 10:19 PM, Stefano Stabellini wrote:
> > On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
> > > PSCI handling code had helper routine that checked calling convention.
> > > It does not needed anymore, because:
> > > 
> > >   - Generic handler checks that 64 bit calls can be made only by
> > >     64 bit guests.
> > > 
> > >   - SMCCC requires that 64-bit handler should support both 32 and 64 bit
> > >     calls even if they originate from 64 bit caller.
> > > 
> > > This patch removes that extra check.
> > > 
> > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > > ---
> > >   xen/arch/arm/vsmc.c | 13 +------------
> > >   1 file changed, 1 insertion(+), 12 deletions(-)
> > > 
> > > diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> > > index 5f10fd1..1983e0e 100644
> > > --- a/xen/arch/arm/vsmc.c
> > > +++ b/xen/arch/arm/vsmc.c
> > > @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
> > >       return false;
> > >   }
> > >   -/* helper function for checking arm mode 32/64 bit */
> > > -static inline int psci_mode_check(struct domain *d, register_t fid)
> > > -{
> > > -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> > > -}
> > > -
> > >   /* PSCI 2.0 interface */
> > >   static bool handle_ssc(struct cpu_user_regs *regs)
> > >   {
> > > @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
> > >           return true;
> > >       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> > >           perfc_incr(vpsci_migrate_info_up_cpu);
> > > -        if ( psci_mode_check(current->domain, fid) )
> > > -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> > > +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> > >           return true;
> > >       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
> > >           perfc_incr(vpsci_system_off);
> > > @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
> > >           return true;
> > >       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
> > >           perfc_incr(vpsci_cpu_on);
> > > -        if ( psci_mode_check(current->domain, fid) )
> > 
> > I would prefer if the `return true' was within the { } block. But anyway
> > it's just a code style issue, so:
> 
> Well, I think we should keep the coding style consistent within arch/arm. If
> we have the return true within {} in other place. Then this should be done
> here.
> 
> In general, { } should only be used to en-globe everything in a case or for
> if/else/while/for with more than a line. All the other kind of { } should be
> avoided. I particularly dislike any code doing
> 
> code
> 
> {
>    variable definition;
> 
>    code
> }
> 
> code
> 
> Unless you have a strong reason to do it (avoiding reworking the code is not
> one), I will nack any code resulting to that.

Right, care to submit a patch for CODING_STYLE? I noticed there are no
entries on this topic.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Julien Grall]

Hi,

On 07/03/2017 06:29 PM, Stefano Stabellini wrote:
> On Sun, 2 Jul 2017, Julien Grall wrote:
>> Hi,
>>
>> On 06/30/2017 10:19 PM, Stefano Stabellini wrote:
>>> On Thu, 22 Jun 2017, Volodymyr Babchuk wrote:
>>>> PSCI handling code had helper routine that checked calling convention.
>>>> It does not needed anymore, because:
>>>>
>>>>    - Generic handler checks that 64 bit calls can be made only by
>>>>      64 bit guests.
>>>>
>>>>    - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>>>>      calls even if they originate from 64 bit caller.
>>>>
>>>> This patch removes that extra check.
>>>>
>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>> ---
>>>>    xen/arch/arm/vsmc.c | 13 +------------
>>>>    1 file changed, 1 insertion(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>>>> index 5f10fd1..1983e0e 100644
>>>> --- a/xen/arch/arm/vsmc.c
>>>> +++ b/xen/arch/arm/vsmc.c
>>>> @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>>>>        return false;
>>>>    }
>>>>    -/* helper function for checking arm mode 32/64 bit */
>>>> -static inline int psci_mode_check(struct domain *d, register_t fid)
>>>> -{
>>>> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
>>>> -}
>>>> -
>>>>    /* PSCI 2.0 interface */
>>>>    static bool handle_ssc(struct cpu_user_regs *regs)
>>>>    {
>>>> @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>>>            return true;
>>>>        case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>>>>            perfc_incr(vpsci_migrate_info_up_cpu);
>>>> -        if ( psci_mode_check(current->domain, fid) )
>>>> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>>>> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>>>>            return true;
>>>>        case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>>>>            perfc_incr(vpsci_system_off);
>>>> @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>>>            return true;
>>>>        case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>>>>            perfc_incr(vpsci_cpu_on);
>>>> -        if ( psci_mode_check(current->domain, fid) )
>>>
>>> I would prefer if the `return true' was within the { } block. But anyway
>>> it's just a code style issue, so:
>>
>> Well, I think we should keep the coding style consistent within arch/arm. If
>> we have the return true within {} in other place. Then this should be done
>> here.
>>
>> In general, { } should only be used to en-globe everything in a case or for
>> if/else/while/for with more than a line. All the other kind of { } should be
>> avoided. I particularly dislike any code doing
>>
>> code
>>
>> {
>>     variable definition;
>>
>>     code
>> }
>>
>> code
>>
>> Unless you have a strong reason to do it (avoiding reworking the code is not
>> one), I will nack any code resulting to that.
> 
> Right, care to submit a patch for CODING_STYLE? I noticed there are no
> entries on this topic.

I can write down a patch assuming the other hypervisor maintainers are 
happy with it.

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Julien Grall]

Hi Volodymyr

On 06/22/2017 05:25 PM, Volodymyr Babchuk wrote:
> PSCI handling code had helper routine that checked calling convention.
> It does not needed anymore, because:
> 
>   - Generic handler checks that 64 bit calls can be made only by
>     64 bit guests.
> 
>   - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>     calls even if they originate from 64 bit caller.
> 
> This patch removes that extra check.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>   xen/arch/arm/vsmc.c | 13 +------------
>   1 file changed, 1 insertion(+), 12 deletions(-)
> 
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 5f10fd1..1983e0e 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>       return false;
>   }
>   
> -/* helper function for checking arm mode 32/64 bit */
> -static inline int psci_mode_check(struct domain *d, register_t fid)
> -{
> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
> -}
> -
>   /* PSCI 2.0 interface */
>   static bool handle_ssc(struct cpu_user_regs *regs)
>   {
> @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>           perfc_incr(vpsci_migrate_info_up_cpu);
> -        if ( psci_mode_check(current->domain, fid) )
> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>           perfc_incr(vpsci_system_off);
> @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>           perfc_incr(vpsci_cpu_on);
> -        if ( psci_mode_check(current->domain, fid) )

Please re-indent/re-organize the code correctly rather than just 
dropping. We want to keep the code nice even if it requires more changes.

>           {
>               register_t vcpuid = get_user_reg(regs, 1);
>               register_t epoint = get_user_reg(regs, 2);
> @@ -151,7 +143,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
>           perfc_incr(vpsci_cpu_suspend);
> -        if ( psci_mode_check(current->domain, fid) )

Ditto

>           {
>               uint32_t pstate = get_user_reg(regs, 1);
>               register_t epoint = get_user_reg(regs, 2);
> @@ -162,7 +153,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
>           perfc_incr(vpsci_cpu_affinity_info);
> -        if ( psci_mode_check(current->domain, fid) )

Ditto

>           {
>               register_t taff = get_user_reg(regs, 1);
>               uint32_t laff = get_user_reg(regs,2);
> @@ -172,7 +162,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>           return true;
>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
>           perfc_incr(vpsci_cpu_migrate);
> -        if ( psci_mode_check(current->domain, fid) )

Ditto

>           {
>               uint32_t tcpu = get_user_reg(regs, 1);
>               set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/4] vsmc: psci: remove 64 bit mode check

[Julien Grall]

On 07/02/2017 08:34 PM, Julien Grall wrote:
> Hi Volodymyr
> 
> On 06/22/2017 05:25 PM, Volodymyr Babchuk wrote:
>> PSCI handling code had helper routine that checked calling convention.
>> It does not needed anymore, because:
>>
>>   - Generic handler checks that 64 bit calls can be made only by
>>     64 bit guests.
>>
>>   - SMCCC requires that 64-bit handler should support both 32 and 64 bit
>>     calls even if they originate from 64 bit caller.
>>
>> This patch removes that extra check.
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>   xen/arch/arm/vsmc.c | 13 +------------
>>   1 file changed, 1 insertion(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 5f10fd1..1983e0e 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -98,12 +98,6 @@ static bool handle_arch(struct cpu_user_regs *regs)
>>       return false;
>>   }
>> -/* helper function for checking arm mode 32/64 bit */
>> -static inline int psci_mode_check(struct domain *d, register_t fid)
>> -{
>> -        return !( is_64bit_domain(d)^( (fid & PSCI_0_2_64BIT) >> 30 ) );
>> -}
>> -
>>   /* PSCI 2.0 interface */
>>   static bool handle_ssc(struct cpu_user_regs *regs)
>>   {
>> @@ -125,8 +119,7 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>>           perfc_incr(vpsci_migrate_info_up_cpu);
>> -        if ( psci_mode_check(current->domain, fid) )
>> -            set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>> +        set_user_reg(regs, 0, do_psci_0_2_migrate_info_up_cpu());
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>>           perfc_incr(vpsci_system_off);
>> @@ -140,7 +133,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>>           perfc_incr(vpsci_cpu_on);
>> -        if ( psci_mode_check(current->domain, fid) )
> 
> Please re-indent/re-organize the code correctly rather than just 
> dropping. We want to keep the code nice even if it requires more changes.

*dropping the if.

>>           {
>>               register_t vcpuid = get_user_reg(regs, 1);
>>               register_t epoint = get_user_reg(regs, 2);
>> @@ -151,7 +143,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
>>           perfc_incr(vpsci_cpu_suspend);
>> -        if ( psci_mode_check(current->domain, fid) )
> 
> Ditto
> 
>>           {
>>               uint32_t pstate = get_user_reg(regs, 1);
>>               register_t epoint = get_user_reg(regs, 2);
>> @@ -162,7 +153,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
>>           perfc_incr(vpsci_cpu_affinity_info);
>> -        if ( psci_mode_check(current->domain, fid) )
> 
> Ditto
> 
>>           {
>>               register_t taff = get_user_reg(regs, 1);
>>               uint32_t laff = get_user_reg(regs,2);
>> @@ -172,7 +162,6 @@ static bool handle_ssc(struct cpu_user_regs *regs)
>>           return true;
>>       case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
>>           perfc_incr(vpsci_cpu_migrate);
>> -        if ( psci_mode_check(current->domain, fid) )
> 
> Ditto
> 
>>           {
>>               uint32_t tcpu = get_user_reg(regs, 1);
>>               set_user_reg(regs, 0, do_psci_0_2_migrate(tcpu));
>>
> 

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 0/4] Handle SMCs and HVCs in conformance with SMCCC

[Julien Grall]

On 22/06/17 17:24, Volodymyr Babchuk wrote:
> Hello all,

Hi,

Please send new series in a distinct thread rather using the cover 
letter of the first version as "In-Reply-To".

Cheers,

-- 
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel