[PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC

[PATCH v4 00/11] Handle SMCs and HVCs in conformance with SMCCC

[Volodymyr Babchuk]

Hello all,

v4:

 * Added patch with public definitiod for xen_uuid_t
 * Added patch with immediate value mask for SMC, HVC and SVC
 * Added patch with header smccc.h (generic SMCCC definitions)
 * Added patches that add and enable XENFEAT_ARM_SMCCC_supported
 * Removed patch that added inject_undef_exception() and friends
   to the processor.h

This patch series depends on Julien's patches for traps.c cleanup ([1]).

There was discussion about SMCCC bindings (e.g. how to tell guest, that
it can safelly call SMCCC routines). As temporary solution, we'll
provide XENFEAT_ARM_SMCCC_supported feature. More generic solution
is still under discussion.

[1] https://www.mail-archive.com/xen-devel@lists.xen.org/msg117839.html

---
v3:

This is third version. Instead of 4 patches, there are 7 now.
As part of the series, I make some functions in traps.c
available globally, moved SMC conditional check into
separate patch, changed how PSCI functiond numbers are defined.

---
v2:

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.

---
v1:

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

Volodymyr Babchuk (11):
  arm: traps: use generic register accessors in the PSCI code
  arm: traps: check if SMC was conditional before handling it
  public: xen.h: add definitions for UUID handling
  arm: processor.h: add definition for immediate value mask
  arm: add SMCCC protocol definitions.
  arm: smccc: handle SMCs according to SMCCC
  arm: traps: handle PSCI calls inside `vsmc.c`
  arm: PSCI: use definitions provided by asm/smccc.h
  arm: vsmc: remove 64 bit mode check in PSCI handler
  public: add XENFEAT_ARM_SMCCC_supported feature
  arm: enable XENFEAT_ARM_SMCCC_supported feature

 xen/arch/arm/Makefile             |   1 +
 xen/arch/arm/platforms/seattle.c  |   5 +-
 xen/arch/arm/psci.c               |  10 +-
 xen/arch/arm/traps.c              | 133 +---------------
 xen/arch/arm/vsmc.c               | 318 ++++++++++++++++++++++++++++++++++++++
 xen/common/kernel.c               |   3 +
 xen/include/asm-arm/processor.h   |   3 +
 xen/include/asm-arm/psci.h        |  44 +++---
 xen/include/asm-arm/smccc.h       |  96 ++++++++++++
 xen/include/asm-arm/vsmc.h        |  31 ++++
 xen/include/public/arch-arm/smc.h |  66 ++++++++
 xen/include/public/features.h     |   3 +
 xen/include/public/xen.h          |   9 ++
 13 files changed, 561 insertions(+), 161 deletions(-)
 create mode 100644 xen/arch/arm/vsmc.c
 create mode 100644 xen/include/asm-arm/smccc.h
 create mode 100644 xen/include/asm-arm/vsmc.h
 create mode 100644 xen/include/public/arch-arm/smc.h

-- 
2.7.4


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

[PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code

[Volodymyr Babchuk]

There are standard functions set_user_reg() and get_user_reg(). We can
use them in PSCI_SET_RESULT()/PSCI_ARG() macroses instead of relying on
CONFIG_ARM_64 definition.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

* Added space into reg,n
* Used 32-bit constant tin PSCI_ARG32

---
xen/arch/arm/traps.c | 40 ++++++++++++++++++----------------------
 1 file changed, 18 insertions(+), 22 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 13efb58..66f12cb 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,14 +1450,13 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg, n) get_user_reg(reg, n)
+
 #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 )
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
 #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)
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
 #endif
 
 /* helper function for checking arm mode 32/64 bit */
@@ -1471,14 +1470,14 @@ 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;
+    PSCI_SET_RESULT(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);
+            PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
         }
         break;
     case PSCI_cpu_on:
@@ -1486,36 +1485,36 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             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);
+            PSCI_SET_RESULT(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();
+        PSCI_SET_RESULT(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();
+        PSCI_SET_RESULT(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();
+        PSCI_SET_RESULT(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();
+            PSCI_SET_RESULT(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;
+        PSCI_SET_RESULT(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;
+        PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         break;
     case PSCI_0_2_FN_CPU_ON:
     case PSCI_0_2_FN64_CPU_ON:
@@ -1525,8 +1524,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             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);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_CPU_SUSPEND:
@@ -1537,8 +1535,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
             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);
+            PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         break;
     case PSCI_0_2_FN_AFFINITY_INFO:
@@ -1548,8 +1545,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         {
             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);
+            PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         }
         break;
     case PSCI_0_2_FN_MIGRATE:
@@ -1558,7 +1554,7 @@ static void do_trap_psci(struct cpu_user_regs *regs)
         if ( psci_mode_check(current->domain, fid) )
         {
             uint32_t tcpu = PSCI_ARG32(regs,1);
-            PSCI_RESULT_REG(regs) = do_psci_0_2_migrate(tcpu);
+            PSCI_SET_RESULT(regs, 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

[PATCH v4 02/11] arm: traps: check if SMC was conditional before handling it

[Volodymyr Babchuk]

Trapped SMC instruction can fail condition check on ARMv8 arhcitecture
(ARM DDI 0487B.a page D7-2271). So we need to check if condition was meet.

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

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 66f12cb..82cd2b1 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2159,6 +2159,12 @@ 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 ( current->domain->arch.monitor.privileged_call_enabled )
         rc = monitor_smc();
 
-- 
2.7.4


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

[PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Added type xen_uuid_t. This type represents UUID as an array of 16
bytes in big endian format.

Added macro XEN_DEFINE_UUID that constructs UUID in the usual way:

 XEN_DEFINE_UUID(00112233, 4455, 6677, 8899, aabbccddeeff)

will construct UUID 00112233-4455-6677-8899-aabbccddeeff presented as
 {0x00, 0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0x88,
  0x99, 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xff}

NB: This is compatible with Linux kernel and with libuuid, but it is not
compatible with Microsoft, as they use mixed-endian encoding (some
components are little-endian, some are big-endian).

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/public/xen.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e..d1a4765 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
 __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
 __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
 
+typedef uint8_t xen_uuid_t[16];
+#define XEN_DEFINE_UUID(a, b, c, d, e1, e2, e3, e4, e5, e6)             \
+    {((a) >> 24) & 0xFF, ((a) >> 16) & 0xFF,                            \
+     ((a) >>  8) & 0xFF, ((a) >>  0) & 0xFF,                            \
+     ((b) >>  8) & 0xFF, ((b) >>  0) & 0xFF,                            \
+     ((c) >>  8) & 0xFF, ((c) >>  0) & 0xFF,                            \
+     ((d) >>  8) & 0xFF, ((d) >>  0) & 0xFF,                            \
+            e1, e2, e3, e4, e5, e6}
+
 #endif /* !__ASSEMBLY__ */
 
 /* Default definitions for macros used by domctl/sysctl. */
-- 
2.7.4


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

[PATCH v4 04/11] arm: processor.h: add definition for immediate value mask

[Volodymyr Babchuk]

This patch adds definition HSR_XXC_IMM_MASK. It can be used to extract
immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64 instructions,
as described at ARM ARM (ARM DDI 0487B.a pages D7-2270, D7-2272).

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/asm-arm/processor.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
index 51ce802..89752a7 100644
--- a/xen/include/asm-arm/processor.h
+++ b/xen/include/asm-arm/processor.h
@@ -580,6 +580,9 @@ union hsr {
                               HSR_SYSREG_CRN_MASK|HSR_SYSREG_CRM_MASK|\
                               HSR_SYSREG_OP2_MASK)
 
+/* HSR.EC == HSR_{HVC32, HVC64, SMC64, SVC32, SVC64} */
+#define HSR_XXC_IMM_MASK     (0xffff)
+
 /* Physical Address Register */
 #define PAR_F           (_AC(1,U)<<0)
 
-- 
2.7.4


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

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

[Volodymyr Babchuk]

This patch adds generic definitions used in ARM SMC call convention.
Those definitions was taken from linux header arm-smccc.h, extended
and formatted according to XEN coding style.

They can be used by both SMCCC clients (like PSCI) and by SMCCC
servers (like vPSCI or upcoming generic SMCCC handler).

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/asm-arm/smccc.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)
 create mode 100644 xen/include/asm-arm/smccc.h

diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 0000000..67da3fb
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2015, Linaro Limited
+ * Copyright (c) 2017, EPAM Systems
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __ASM_ARM_SMCCC_H__
+#define __ASM_ARM_SMCCC_H__
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ */
+
+#define ARM_SMCCC_STD_CALL              0U
+#define ARM_SMCCC_FAST_CALL             1U
+#define ARM_SMCCC_TYPE_SHIFT            31
+
+#define ARM_SMCCC_SMC_32                0U
+#define ARM_SMCCC_SMC_64                1U
+#define ARM_SMCCC_CALL_CONV_SHIFT       30
+
+#define ARM_SMCCC_OWNER_MASK            0x3F
+#define ARM_SMCCC_OWNER_SHIFT           24
+
+#define ARM_SMCCC_FUNC_MASK             0xFFFF
+
+/* Check if this is fast call */
+#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
+    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+
+/* Check if this is 64 bit call  */
+#define ARM_SMCCC_IS_64(smc_val)                                        \
+    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+
+/* Get function number from function identifier */
+#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & ARM_SMCCC_FUNC_MASK)
+
+/* Get service owner number from function identifier */
+#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
+    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+/*
+ * Construct function identifier from call type (fast or standard),
+ * calling convention (32 or 64 bit), service owner and function number
+ */
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
+    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
+         ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |          \
+         (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |  \
+         ((func_num) & ARM_SMCCC_FUNC_MASK))
+
+/* List of known service owners */
+#define ARM_SMCCC_OWNER_ARCH            0
+#define ARM_SMCCC_OWNER_CPU             1
+#define ARM_SMCCC_OWNER_SIP             2
+#define ARM_SMCCC_OWNER_OEM             3
+#define ARM_SMCCC_OWNER_STANDARD        4
+#define ARM_SMCCC_OWNER_HYPERVISOR      5
+#define ARM_SMCCC_OWNER_TRUSTED_APP     48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
+#define ARM_SMCCC_OWNER_TRUSTED_OS      50
+#define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
+
+/* List of generic function numbers */
+#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
+#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
+#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
+
+/* Only one error code defined in SMCCC */
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
+
+#endif  /* __ASM_ARM_SMCCC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */
-- 
2.7.4


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

[PATCH v4 06/11] arm: smccc: handle SMCs 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 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, UUID and number of functions provided by service
provider.

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

At this time 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>
---

 * Reworked UUID handling (due to new UUID type definition)
 * Renamed vsmc_handle_call() to vsmccc_handle_call() to emphasis
   that it handles both SMC and HVC
 * Added comment for inject_undef_exception() usage
 * Used HSR_XXC_IMM_MASK insted of 0xFF
 * Added full stops to comments

---

 xen/arch/arm/Makefile             |   1 +
 xen/arch/arm/traps.c              |  18 +----
 xen/arch/arm/vsmc.c               | 160 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-arm/vsmc.h        |  30 +++++++
 xen/include/public/arch-arm/smc.h |  58 ++++++++++++++
 5 files changed, 250 insertions(+), 17 deletions(-)
 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 de00c5e..3d7dde9 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -51,6 +51,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 82cd2b1..4141a89 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -50,6 +50,7 @@
 #include <asm/regs.h>
 #include <asm/traps.h>
 #include <asm/vgic.h>
+#include <asm/vsmc.h>
 #include <asm/vtimer.h>
 
 #include "decode.h"
@@ -2155,23 +2156,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     inject_dabt_exception(regs, info.gva, hsr.len);
 }
 
-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 ( current->domain->arch.monitor.privileged_call_enabled )
-        rc = monitor_smc();
-
-    if ( rc != 1 )
-        inject_undef_exception(regs, hsr);
-}
-
 static void enter_hypervisor_head(struct cpu_user_regs *regs)
 {
     if ( guest_mode(regs) )
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
new file mode 100644
index 0000000..0a81294
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,160 @@
+/*
+ * 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/lib.h>
+#include <xen/types.h>
+#include <public/arch-arm/smc.h>
+#include <asm/monitor.h>
+#include <asm/regs.h>
+#include <asm/smccc.h>
+#include <asm/traps.h>
+#include <asm/vsmc.h>
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
+{
+#define FILL_UUID(n) \
+    set_user_reg(regs, n, (register_t) u[n * 4 + 0] << 24 |                     \
+                                       u[n * 4 + 1] << 16 |                     \
+                                       u[n * 4 + 2] << 8  |                     \
+                                       u[n * 4 + 3] << 0  )
+
+    FILL_UUID(0);
+    FILL_UUID(1);
+    FILL_UUID(2);
+    FILL_UUID(3);
+#undef FILL_UUID
+}
+
+/* SMCCC interface for hypervisor. Tell about itself. */
+static bool handle_hypervisor(struct cpu_user_regs *regs)
+{
+    static const xen_uuid_t xen_uuid = XEN_SMCCC_UID;
+    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:
+        fill_uuid(regs, xen_uuid);
+        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;
+}
+
+/*
+ * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
+ * returns true if that was valid SMCCC call (even if function number
+ * was unkown).
+ */
+static bool vsmccc_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,
+     * because it is not stored in HSR.ISS field. To get immediate
+     * value we need to disassemble instruction at current pc, which
+     * is expensive. 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 & HSR_XXC_IMM_MASK) != 0)
+            return false;
+        break;
+    case HSR_EC_SMC32:
+        break;
+    default:
+        return false;
+    }
+
+    /* 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 true;
+    }
+
+    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 true;
+}
+
+/* This function will be called from traps.c. */
+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 )
+        return;
+
+    /*
+     * Use standard routines to handle the call.
+     * vsmccc_handle_call() will return false if this call is not
+     * SMCCC compatbile (i.e. immediate value != 0). As it is not
+     * compatible, we can't be sure that guest will understand
+     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+     */
+    if ( vsmccc_handle_call(regs) )
+        advance_pc(regs, hsr);
+    else
+        inject_undef_exception(regs, hsr);
+}
+
+/*
+ * 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..31aaa55
--- /dev/null
+++ b/xen/include/asm-arm/vsmc.h
@@ -0,0 +1,30 @@
+/*
+ * 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>
+
+void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+
+#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..3d3cd90
--- /dev/null
+++ b/xen/include/public/arch-arm/smc.h
@@ -0,0 +1,58 @@
+/*
+ * 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__
+
+#include "public/xen.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.
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+
+/* Hypervisor Service UID. Randomly generated with uuidgen. */
+#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
+				      0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
+
+#endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:b
+ */
-- 
2.7.4


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

[PATCH v4 07/11] 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`.

Older PSCI 0.1 uses SMC function identifiers in range that is
resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
PSCI 0.2 is defined as "standard secure service" with own ranges
(ARM DEN 0028B, page 18).

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>
---

 * Fixed mistakes about non-existant PSCI 2.0
 * Added special SMC function number handling for PSCI 0.1
 * Fixed coding style in  handle_psci_0_1()
 * Changed how return do_trap_hvc_smccc() is called from traps.c
 * Renamed SSC to SSSC (Standard Secure Service Calls)

---
 xen/arch/arm/traps.c              | 117 +------------------------
 xen/arch/arm/vsmc.c               | 175 ++++++++++++++++++++++++++++++++++++--
 xen/include/asm-arm/smccc.h       |   4 +
 xen/include/asm-arm/vsmc.h        |   1 +
 xen/include/public/arch-arm/smc.h |   8 ++
 5 files changed, 183 insertions(+), 122 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 4141a89..517e013 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1451,119 +1451,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 }
 #endif
 
-#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
-#define PSCI_ARG(reg, n) get_user_reg(reg, n)
-
-#ifdef CONFIG_ARM_64
-#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
-#else
-#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_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
-    switch( fid )
-    {
-    case PSCI_cpu_off:
-        {
-            uint32_t pstate = PSCI_ARG32(regs,1);
-            perfc_incr(vpsci_cpu_off);
-            PSCI_SET_RESULT(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_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
-        }
-        break;
-    case PSCI_0_2_FN_PSCI_VERSION:
-        perfc_incr(vpsci_version);
-        PSCI_SET_RESULT(regs, do_psci_0_2_version());
-        break;
-    case PSCI_0_2_FN_CPU_OFF:
-        perfc_incr(vpsci_cpu_off);
-        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
-        break;
-    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
-        perfc_incr(vpsci_migrate_info_type);
-        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        break;
-    case PSCI_0_2_FN_SYSTEM_RESET:
-        perfc_incr(vpsci_system_reset);
-        do_psci_0_2_system_reset();
-        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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
@@ -2252,7 +2139,7 @@ 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);
+            return do_trap_hvc_smccc(regs);
         do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
         break;
 #ifdef CONFIG_ARM_64
@@ -2264,7 +2151,7 @@ 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);
+            return do_trap_hvc_smccc(regs);
         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 0a81294..956d4ef 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -19,6 +19,7 @@
 #include <xen/types.h>
 #include <public/arch-arm/smc.h>
 #include <asm/monitor.h>
+#include <asm/psci.h>
 #include <asm/regs.h>
 #include <asm/smccc.h>
 #include <asm/traps.h>
@@ -27,6 +28,9 @@
 /* Number of functions currently supported by Hypervisor Service. */
 #define XEN_SMCCC_FUNCTION_COUNT 3
 
+/* Number of functions currently supported by Standard Service Service Calls. */
+#define SSSC_SMCCC_FUNCTION_COUNT 13
+
 static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
 {
 #define FILL_UUID(n) \
@@ -62,6 +66,133 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
     return false;
 }
 
+#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
+#define PSCI_ARG(reg, n) get_user_reg(reg, n)
+
+#ifdef CONFIG_ARM_64
+#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/* PSCI 0.1 interface */
+static bool handle_psci_0_1(struct cpu_user_regs *regs)
+{
+    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
+    {
+    case PSCI_cpu_off:
+    {
+        uint32_t pstate = PSCI_ARG32(regs, 1);
+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
+        return true;
+    }
+    case PSCI_cpu_on:
+    {
+        uint32_t vcpuid = PSCI_ARG32(regs, 1);
+        register_t epoint = PSCI_ARG(regs, 2);
+        perfc_incr(vpsci_cpu_on);
+        PSCI_SET_RESULT(regs, 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 0.2 interface and other Standard Secure Calls */
+static bool handle_sssc(struct cpu_user_regs *regs)
+{
+    register_t fid = PSCI_ARG(regs, 0);
+
+    switch ( ARM_SMCCC_FUNC_NUM(fid) )
+    {
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+        perfc_incr(vpsci_version);
+        PSCI_SET_RESULT(regs, do_psci_0_2_version());
+        return true;
+    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+        perfc_incr(vpsci_cpu_off);
+        PSCI_SET_RESULT(regs, 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);
+        PSCI_SET_RESULT(regs, 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) )
+            PSCI_SET_RESULT(regs, 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();
+        PSCI_SET_RESULT(regs, 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();
+        PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
+            register_t epoint = PSCI_ARG(regs, 2);
+            register_t cid = PSCI_ARG(regs, 3);
+            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
+            register_t epoint = PSCI_ARG(regs, 2);
+            register_t cid = PSCI_ARG(regs, 3);
+            PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
+            uint32_t laff = PSCI_ARG32(regs, 2);
+            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
+            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
+        }
+        return true;
+    case ARM_SMCCC_FUNC_CALL_COUNT:
+        set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
+        return true;
+    case ARM_SMCCC_FUNC_CALL_UID:
+    {
+        static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
+        fill_uuid(regs, psci_uuid);
+        return true;
+    }
+    case ARM_SMCCC_FUNC_CALL_REVISION:
+        set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
+        set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
+        return true;
+    }
+    return false;
+}
+
 /*
  * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
  * returns true if that was valid SMCCC call (even if function number
@@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
 {
     bool handled = false;
     const union hsr hsr = { .bits = regs->hsr };
+    register_t func_id = get_user_reg(regs, 0);
 
     /*
      * Check immediate value for HVC32, HVC64 and SMC64.
@@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
     }
 
     /* 64 bit calls are allowed only from 64 bit domains. */
-    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
+    if ( ARM_SMCCC_IS_64(func_id) &&
          is_32bit_domain(current->domain) )
     {
         set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
         return true;
     }
 
-    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
+    /*
+     * Special case: identifier range for existing APIs.
+     * This range is described in SMCCC (ARM DEN 0028B, page 16),
+     * but it does not conforms to standard function identifier
+     * encoding.
+     */
+    if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
+         func_id <= ARM_SMCCC_RESERVED_RANGE_END )
+        handled = handle_psci_0_1(regs);
+    else
     {
-    case ARM_SMCCC_OWNER_HYPERVISOR:
-        handled = handle_hypervisor(regs);
-        break;
+        switch ( ARM_SMCCC_OWNER_NUM(func_id) )
+        {
+        case ARM_SMCCC_OWNER_HYPERVISOR:
+            handled = handle_hypervisor(regs);
+            break;
+        case ARM_SMCCC_OWNER_STANDARD:
+            handled = handle_sssc(regs);
+            break;
+        }
     }
 
     if ( !handled )
     {
-        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
-                get_user_reg(regs, 0));
+        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", func_id);
         /* Inform caller that function is not supported. */
         set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
     }
@@ -150,6 +296,21 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
         inject_undef_exception(regs, hsr);
 }
 
+/* This function will be called from traps.c */
+void do_trap_hvc_smccc(struct cpu_user_regs *regs)
+{
+    const union hsr hsr = { .bits = regs->hsr };
+
+    /*
+     * vsmccc_handle_call() will return false if this call is not
+     * SMCCC compatbile (i.e. immediate value != 0). As it is not
+     * compatible, we can't be sure that guest will understand
+     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
+     */
+    if ( !vsmccc_handle_call(regs) )
+        inject_undef_exception(regs, hsr);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
index 67da3fb..7c0003c 100644
--- a/xen/include/asm-arm/smccc.h
+++ b/xen/include/asm-arm/smccc.h
@@ -80,6 +80,10 @@
 /* Only one error code defined in SMCCC */
 #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
 
+/* SMCCC function identifier range which is reserved for existing APIs */
+#define ARM_SMCCC_RESERVED_RANGE_START  0x0
+#define ARM_SMCCC_RESERVED_RANGE_END    0x0100FFFF
+
 #endif  /* __ASM_ARM_SMCCC_H__ */
 
 /*
diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
index 31aaa55..90ff610 100644
--- a/xen/include/asm-arm/vsmc.h
+++ b/xen/include/asm-arm/vsmc.h
@@ -17,6 +17,7 @@
 #include <xen/types.h>
 
 void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+void do_trap_hvc_smccc(struct cpu_user_regs *regs);
 
 #endif  /* __ASM_ARM_VSMC_H__ */
 
diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
index 3d3cd90..c5327e3 100644
--- a/xen/include/public/arch-arm/smc.h
+++ b/xen/include/public/arch-arm/smc.h
@@ -46,6 +46,14 @@
 #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
 				      0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
 
+/* Standard Service Service Call version. */
+#define SSSC_SMCCC_MAJOR_REVISION 0
+#define SSSC_SMCCC_MINOR_REVISION 1
+
+/* Standard Service Call UID. Randomly generated with uuidgen. */
+#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\
+				      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

[PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

[Volodymyr Babchuk]

smccc.h provides definitions to construct SMC call function number according
to SMCCC. We don't need multiple definitions for one thing, and definitions
in smccc.h are more generic than ones used in psci.h.

So psci.h will only provide function codes, while whole SMC function
identifier will be constructed using generic macros from smccc.h.

PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because this
is a new entity. It can lead to problems, if we'll just change value of
PSCI_0_2_FN_xxx without renaming it.

This change also affects vsmc.c and seattle.c, because they both use PSCI
function numbers.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

 * Reworked definitions to minimize their lenght
 * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx

---
 xen/arch/arm/platforms/seattle.c |  5 +++--
 xen/arch/arm/psci.c              | 10 ++++-----
 xen/arch/arm/vsmc.c              | 24 +++++++++++-----------
 xen/include/asm-arm/psci.h       | 44 ++++++++++++++++++----------------------
 4 files changed, 40 insertions(+), 43 deletions(-)

diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 86dce91..fb2ad13 100644
--- a/xen/arch/arm/platforms/seattle.c
+++ b/xen/arch/arm/platforms/seattle.c
@@ -19,6 +19,7 @@
 
 #include <asm/platform.h>
 #include <asm/psci.h>
+#include <asm/vsmc.h>
 
 static const char * const seattle_dt_compat[] __initconst =
 {
@@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] __initconst =
  */
 static void seattle_system_reset(void)
 {
-    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+    call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
 }
 
 static void seattle_system_off(void)
 {
-    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+    call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
 }
 
 PLATFORM_START(seattle, "SEATTLE")
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 34ee97e..645fe58 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -31,9 +31,9 @@
  * (native-width) function ID.
  */
 #ifdef CONFIG_ARM_64
-#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
+#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN64(name)
 #else
-#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
+#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN32(name)
 #endif
 
 uint32_t psci_ver;
@@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
 void call_psci_system_off(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
+        call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
 }
 
 void call_psci_system_reset(void)
 {
     if ( psci_ver > PSCI_VERSION(0, 1) )
-        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
+        call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
 }
 
 int __init psci_is_smc_method(const struct dt_device_node *psci)
@@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
         }
     }
 
-    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
+    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_VERSION), 0, 0, 0);
 
     /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
     if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 956d4ef..46a2fde 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs *regs)
 /* 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 ) );
+    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);
 }
 
 /* PSCI 0.2 interface and other Standard Secure Calls */
@@ -112,34 +112,34 @@ static bool handle_sssc(struct cpu_user_regs *regs)
 
     switch ( ARM_SMCCC_FUNC_NUM(fid) )
     {
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
+    case PSCI_0_2_FUNC_PSCI_VERSION:
         perfc_incr(vpsci_version);
         PSCI_SET_RESULT(regs, do_psci_0_2_version());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
+    case PSCI_0_2_FUNC_CPU_OFF:
         perfc_incr(vpsci_cpu_off);
         PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+    case PSCI_0_2_FUNC_MIGRATE_INFO_TYPE:
         perfc_incr(vpsci_migrate_info_type);
         PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+    case PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU:
         perfc_incr(vpsci_migrate_info_up_cpu);
         if ( psci_mode_check(current->domain, fid) )
             PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
+    case PSCI_0_2_FUNC_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
         do_psci_0_2_system_off();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
-        return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
+       return true;
+    case PSCI_0_2_FUNC_SYSTEM_RESET:
         perfc_incr(vpsci_system_reset);
         do_psci_0_2_system_reset();
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
+    case PSCI_0_2_FUNC_CPU_ON:
         perfc_incr(vpsci_cpu_on);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -149,7 +149,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
+    case PSCI_0_2_FUNC_CPU_SUSPEND:
         perfc_incr(vpsci_cpu_suspend);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -159,7 +159,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
+    case PSCI_0_2_FUNC_AFFINITY_INFO:
         perfc_incr(vpsci_cpu_affinity_info);
         if ( psci_mode_check(current->domain, fid) )
         {
@@ -168,7 +168,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
             PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         }
         return true;
-    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
+    case PSCI_0_2_FUNC_MIGRATE:
         perfc_incr(vpsci_cpu_migrate);
         if ( psci_mode_check(current->domain, fid) )
         {
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index be2458a..14fb98f 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_PSCI_H__
 #define __ASM_PSCI_H__
 
+#include <asm/smccc.h>
+
 /* PSCI return values (inclusive of all PSCI versions) */
 #define PSCI_SUCCESS                 0
 #define PSCI_NOT_SUPPORTED          -1
@@ -41,30 +43,24 @@ register_t do_psci_0_2_migrate_info_up_cpu(void);
 void do_psci_0_2_system_off(void);
 void do_psci_0_2_system_reset(void);
 
-/* PSCI v0.2 interface */
-#define PSCI_0_2_FN_BASE        0x84000000
-#define PSCI_0_2_FN(n)          (PSCI_0_2_FN_BASE + (n))
-#define PSCI_0_2_64BIT          0x40000000
-#define PSCI_0_2_FN64_BASE      \
-                        (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
-#define PSCI_0_2_FN64(n)        (PSCI_0_2_FN64_BASE + (n))
-
-#define PSCI_0_2_FN_PSCI_VERSION        PSCI_0_2_FN(0)
-#define PSCI_0_2_FN_CPU_SUSPEND         PSCI_0_2_FN(1)
-#define PSCI_0_2_FN_CPU_OFF             PSCI_0_2_FN(2)
-#define PSCI_0_2_FN_CPU_ON              PSCI_0_2_FN(3)
-#define PSCI_0_2_FN_AFFINITY_INFO       PSCI_0_2_FN(4)
-#define PSCI_0_2_FN_MIGRATE             PSCI_0_2_FN(5)
-#define PSCI_0_2_FN_MIGRATE_INFO_TYPE   PSCI_0_2_FN(6)
-#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
-#define PSCI_0_2_FN_SYSTEM_OFF          PSCI_0_2_FN(8)
-#define PSCI_0_2_FN_SYSTEM_RESET        PSCI_0_2_FN(9)
-
-#define PSCI_0_2_FN64_CPU_SUSPEND       PSCI_0_2_FN64(1)
-#define PSCI_0_2_FN64_CPU_ON            PSCI_0_2_FN64(3)
-#define PSCI_0_2_FN64_AFFINITY_INFO     PSCI_0_2_FN64(4)
-#define PSCI_0_2_FN64_MIGRATE           PSCI_0_2_FN64(5)
-#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
+#define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
+                                            ARM_SMCCC_SMC_32,                   \
+                                            ARM_SMCCC_OWNER_STANDARD,           \
+                                            PSCI_0_2_FUNC_##name)
+#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
+                                            ARM_SMCCC_SMC_64,                   \
+                                            ARM_SMCCC_OWNER_STANDARD,           \
+                                            PSCI_0_2_FUNC_##name)
+#define PSCI_0_2_FUNC_PSCI_VERSION        0
+#define PSCI_0_2_FUNC_CPU_SUSPEND         1
+#define PSCI_0_2_FUNC_CPU_OFF             2
+#define PSCI_0_2_FUNC_CPU_ON              3
+#define PSCI_0_2_FUNC_AFFINITY_INFO       4
+#define PSCI_0_2_FUNC_MIGRATE             5
+#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
+#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
+#define PSCI_0_2_FUNC_SYSTEM_OFF          8
+#define PSCI_0_2_FUNC_SYSTEM_RESET        9
 
 /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
 #define PSCI_0_2_AFFINITY_LEVEL_ON      0
-- 
2.7.4


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

[PATCH v4 09/11] arm: vsmc: remove 64 bit mode check in PSCI handler

[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>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---

 * Used case xxx:
        {
	}
   formatting as Julien suggested
   
---
 xen/arch/arm/vsmc.c | 63 +++++++++++++++++++++++++----------------------------
 1 file changed, 30 insertions(+), 33 deletions(-)

diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 46a2fde..673b23c 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -99,12 +99,6 @@ static bool handle_psci_0_x(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) || !ARM_SMCCC_IS_64(fid);
-}
-
 /* PSCI 0.2 interface and other Standard Secure Calls */
 static bool handle_sssc(struct cpu_user_regs *regs)
 {
@@ -126,8 +120,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         return true;
     case PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU:
         perfc_incr(vpsci_migrate_info_up_cpu);
-        if ( psci_mode_check(current->domain, fid) )
-            PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
+        PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
         return true;
     case PSCI_0_2_FUNC_SYSTEM_OFF:
         perfc_incr(vpsci_system_off);
@@ -140,42 +133,46 @@ static bool handle_sssc(struct cpu_user_regs *regs)
         PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
         return true;
     case PSCI_0_2_FUNC_CPU_ON:
+    {
+        register_t vcpuid = PSCI_ARG(regs,1);
+        register_t epoint = PSCI_ARG(regs,2);
+        register_t cid = PSCI_ARG(regs,3);
+
         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_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
-        }
+
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
         return true;
+    }
     case PSCI_0_2_FUNC_CPU_SUSPEND:
+    {
+        uint32_t pstate = PSCI_ARG32(regs,1);
+        register_t epoint = PSCI_ARG(regs,2);
+        register_t cid = PSCI_ARG(regs,3);
+
         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_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
-        }
+
+        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
         return true;
+    }
     case PSCI_0_2_FUNC_AFFINITY_INFO:
+    {
+        register_t taff = PSCI_ARG(regs,1);
+        uint32_t laff = PSCI_ARG32(regs,2);
+
         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_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
-        }
+
+        PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
         return true;
+    }
     case PSCI_0_2_FUNC_MIGRATE:
+    {
+        uint32_t tcpu = PSCI_ARG32(regs,1);
+
         perfc_incr(vpsci_cpu_migrate);
-        if ( psci_mode_check(current->domain, fid) )
-        {
-            uint32_t tcpu = PSCI_ARG32(regs, 1);
-            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
-        }
+
+        PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
         return true;
+    }
     case ARM_SMCCC_FUNC_CALL_COUNT:
         set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
         return true;
-- 
2.7.4


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

[PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Volodymyr Babchuk]

This feature indicates that hypervisor is compatible with ARM
SMC calling convention. Hypervisor will not inject an undefined
instruction exception if an invalid SMC function were called and
will not crash a domain if an invlalid HVC functions were called.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/include/public/features.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index 2110b04..1a989b8 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -102,6 +102,9 @@
 /* Guest can use XENMEMF_vnode to specify virtual node for memory op. */
 #define XENFEAT_memory_op_vnode_supported 13
 
+/* arm: Hypervisor supports ARM SMC calling convention. */
+#define XENFEAT_ARM_SMCCC_supported       14
+
 #define XENFEAT_NR_SUBMAPS 1
 
 #endif /* __XEN_PUBLIC_FEATURES_H__ */
-- 
2.7.4


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

[PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature

[Volodymyr Babchuk]

As Xen now supports SMCCC, we can enable this feature to tell
guests that it is safe to call generic SMCs and HVCs.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
 xen/common/kernel.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index ce7cb8a..18c4d51 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                     (1U << XENFEAT_auto_translated_physmap);
             if ( is_hardware_domain(d) )
                 fi.submap |= 1U << XENFEAT_dom0;
+#ifdef CONFIG_ARM
+            fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
+#endif
 #ifdef CONFIG_X86
             switch ( d->guest_type )
             {
-- 
2.7.4


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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
>  __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
>  __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>  
> +typedef uint8_t xen_uuid_t[16];

As expressed before, I'm opposed to this being a plain array. I've
pointed you at the EFI representation as an example; at the very
least I'd expect a wrapper structure around the array (which is
_not_ to say that I would ack such a patch, but at least I wouldn't
nak it).

Jan


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

Re: [PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature

[Jan Beulich]

>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
> As Xen now supports SMCCC, we can enable this feature to tell
> guests that it is safe to call generic SMCs and HVCs.

I think this and patch 10 should be folded.

> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/common/kernel.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
> index ce7cb8a..18c4d51 100644
> --- a/xen/common/kernel.c
> +++ b/xen/common/kernel.c
> @@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>                      (1U << XENFEAT_auto_translated_physmap);
>              if ( is_hardware_domain(d) )
>                  fi.submap |= 1U << XENFEAT_dom0;
> +#ifdef CONFIG_ARM
> +            fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
> +#endif

Pointless parentheses (as other code in context shows you).

With both aspects taken care of
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan


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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 22.08.17 10:26, Jan Beulich wrote:
>>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
>> --- a/xen/include/public/xen.h
>> +++ b/xen/include/public/xen.h
>> @@ -930,6 +930,15 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
>>   __DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
>>   __DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
>>   
>> +typedef uint8_t xen_uuid_t[16];
> 
> As expressed before, I'm opposed to this being a plain array. I've
> pointed you at the EFI representation as an example; at the very
> least I'd expect a wrapper structure around the array (which is
> _not_ to say that I would ack such a patch, but at least I wouldn't
> nak it).

EFI code uses GUID, which is product of Microsoft's NIH syndrome.

Let me cite some parts of RFC 4122:

4.1.  Format

    *The UUID format is 16 octets*; some bits of the eight octet variant
    field specified below determine finer structure.
.....

4.1.2.  Layout and Byte Order
.....
    In the absence of explicit application or presentation protocol
    specification to the contrary, a UUID is encoded as a 128-bit object,
    as follows:

    The fields are encoded as 16 octets, with the sizes and order of the
    fields defined above, and with each field encoded with the Most
    Significant Byte first (known as network byte order).  Note that the
    field names, particularly for multiplexed fields, follow historical
    practice.

    0                   1                   2                   3
     0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                          time_low                             |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |       time_mid                |         time_hi_and_version   |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |clk_seq_hi_res |  clk_seq_low  |         node (0-1)            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |                         node (2-5)                            |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
.....


BTW, GUID handling is incompatible with this RFC, because in GUID
first three fields are stored in LE format, while other fields are 
stored in BE format.

I can't see why you want to map UUID to a certain structure. I can 
create such wrapper, but it will be just dead code, because there are no 
users for it. Frankly, I can't imagine why someone will want to read, 
say, clk_seq_hi_res field of UUID.

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
> I can't see why you want to map UUID to a certain structure.

This is so that the type cannot mistakenly be passed to a function
taking unsigned char *, or be assigned to a variable of that type.
Please see our TYPE_SAFE() macro which we use to specifically
enclose certain scalar types in a structure to that they won't be
compatible with other types deriving from the same scalar base type.

Jan


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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hello Jan

On 23.08.17 11:10, Jan Beulich wrote:
>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>> I can't see why you want to map UUID to a certain structure.
> 
> This is so that the type cannot mistakenly be passed to a function
> taking unsigned char *, or be assigned to a variable of that type.
Right, I see the point there.

> Please see our TYPE_SAFE() macro which we use to specifically
> enclose certain scalar types in a structure to that they won't be
> compatible with other types deriving from the same scalar base type.
I see. So what about

struct xen_uuid_t
{
      uint8_t a[16];
};

then?

One can convert it to union with different representations (array, 
RFC4122 struct, etc) later if there will be need for this.

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 23.08.17 at 13:08, <volodymyr_babchuk@epam.com> wrote:
> On 23.08.17 11:10, Jan Beulich wrote:
>>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>>> I can't see why you want to map UUID to a certain structure.
>> 
>> This is so that the type cannot mistakenly be passed to a function
>> taking unsigned char *, or be assigned to a variable of that type.
> Right, I see the point there.
> 
>> Please see our TYPE_SAFE() macro which we use to specifically
>> enclose certain scalar types in a structure to that they won't be
>> compatible with other types deriving from the same scalar base type.
> I see. So what about
> 
> struct xen_uuid_t
> {
>       uint8_t a[16];
> };
> 
> then?

Yes, that's what I had asked for as the minimal solution. That
would be in line with (but better than) xen_domain_handle_t,
which I've just realized we also have.

> One can convert it to union with different representations (array, 
> RFC4122 struct, etc) later if there will be need for this.

Well, why don't you make it a union but stick to just the array
for now if you dislike making it similar to the EFI one? That way
we can add further representations if needed/desired without
breaking existing consumers.

Jan


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

Re: [PATCH v4 01/11] arm: traps: use generic register accessors in the PSCI code

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> There are standard functions set_user_reg() and get_user_reg(). We can
> use them in PSCI_SET_RESULT()/PSCI_ARG() macroses instead of relying on

s/macroses/macros/ I think.

> CONFIG_ARM_64 definition.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> * Added space into reg,n
> * Used 32-bit constant tin PSCI_ARG32
>
> ---
> xen/arch/arm/traps.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 13efb58..66f12cb 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1450,14 +1450,13 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>
> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> +#define PSCI_ARG(reg, n) get_user_reg(reg, n)
> +
>  #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 )
> +#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)

Please drop the mask, the cast is sufficient.

>  #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)
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)

Please mention in the commit message that you also modify the coding style.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 02/11] arm: traps: check if SMC was conditional before handling it

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> Trapped SMC instruction can fail condition check on ARMv8 arhcitecture

s/arhcitecture/architecture/

> (ARM DDI 0487B.a page D7-2271). So we need to check if condition was meet.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 04/11] arm: processor.h: add definition for immediate value mask

[Julien Grall]

Hi Volodymyr,

Title: It is a too generic, you may want to rename to: "Define HVC/SMC 
immediate mask"

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This patch adds definition HSR_XXC_IMM_MASK. It can be used to extract

s/adds definition/define/

> immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64 instructions,
> as described at ARM ARM (ARM DDI 0487B.a pages D7-2270, D7-2272).

s/at/in the/

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/include/asm-arm/processor.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/xen/include/asm-arm/processor.h b/xen/include/asm-arm/processor.h
> index 51ce802..89752a7 100644
> --- a/xen/include/asm-arm/processor.h
> +++ b/xen/include/asm-arm/processor.h
> @@ -580,6 +580,9 @@ union hsr {
>                                HSR_SYSREG_CRN_MASK|HSR_SYSREG_CRM_MASK|\
>                                HSR_SYSREG_OP2_MASK)
>
> +/* HSR.EC == HSR_{HVC32, HVC64, SMC64, SVC32, SVC64} */
> +#define HSR_XXC_IMM_MASK     (0xffff)
> +
>  /* Physical Address Register */
>  #define PAR_F           (_AC(1,U)<<0)
>
>

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

Hi Volodymyr,

Title: No need for the full stop.

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This patch adds generic definitions used in ARM SMC call convention.
> Those definitions was taken from linux header arm-smccc.h, extended
> and formatted according to XEN coding style.
>
> They can be used by both SMCCC clients (like PSCI) and by SMCCC
> servers (like vPSCI or upcoming generic SMCCC handler).
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/include/asm-arm/smccc.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 92 insertions(+)
>  create mode 100644 xen/include/asm-arm/smccc.h
>
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> new file mode 100644
> index 0000000..67da3fb
> --- /dev/null
> +++ b/xen/include/asm-arm/smccc.h
> @@ -0,0 +1,92 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited

Linaro? Where does this code come from?

> + * Copyright (c) 2017, EPAM Systems
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __ASM_ARM_SMCCC_H__
> +#define __ASM_ARM_SMCCC_H__
> +
> +/*
> + * This file provides common defines for ARM SMC Calling Convention as
> + * specified in
> + * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
> + */
> +
> +#define ARM_SMCCC_STD_CALL              0U
> +#define ARM_SMCCC_FAST_CALL             1U
> +#define ARM_SMCCC_TYPE_SHIFT            31
> +
> +#define ARM_SMCCC_SMC_32                0U
> +#define ARM_SMCCC_SMC_64                1U

I am not sure to understand why you embed SMC in the name? The 
convention is SMC32/HVC32. So I would name

ARM_SMCCC_CONV_{32,64}

> +#define ARM_SMCCC_CALL_CONV_SHIFT       30

Also, I would rename to ARM_SMCCC_CONV to fit with the value above.

> +
> +#define ARM_SMCCC_OWNER_MASK            0x3F

Missing U.

> +#define ARM_SMCCC_OWNER_SHIFT           24
> +
> +#define ARM_SMCCC_FUNC_MASK             0xFFFF
> +
> +/* Check if this is fast call */
> +#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
> +
> +/* Check if this is 64 bit call  */
> +#define ARM_SMCCC_IS_64(smc_val)                                        \
> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
> +
> +/* Get function number from function identifier */
> +#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & ARM_SMCCC_FUNC_MASK)
> +
> +/* Get service owner number from function identifier */
> +#define ARM_SMCCC_OWNER_NUM(smc_val)                                    \
> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)

Can we please use static inline helper for the 4 macros above?

> +
> +/*
> + * Construct function identifier from call type (fast or standard),
> + * calling convention (32 or 64 bit), service owner and function number
> + */
> +#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num)   \
> +    (((type) << ARM_SMCCC_TYPE_SHIFT) |                                 \
> +         ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) |          \
> +         (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) |  \
> +         ((func_num) & ARM_SMCCC_FUNC_MASK))

The indentation looks wrong here. Also I think the (& *MASK) is not 
necessary here. It would be wrong by the user to pass a wrong value and 
even with the masking you would end up to wrong result.

If you are really worry of wrong result, then you should use 
BUILD_BUG_ON()/BUG_ON().

> +
> +/* List of known service owners */
> +#define ARM_SMCCC_OWNER_ARCH            0
> +#define ARM_SMCCC_OWNER_CPU             1
> +#define ARM_SMCCC_OWNER_SIP             2
> +#define ARM_SMCCC_OWNER_OEM             3
> +#define ARM_SMCCC_OWNER_STANDARD        4
> +#define ARM_SMCCC_OWNER_HYPERVISOR      5
> +#define ARM_SMCCC_OWNER_TRUSTED_APP     48
> +#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
> +#define ARM_SMCCC_OWNER_TRUSTED_OS      50
> +#define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
> +
> +/* List of generic function numbers */
> +#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
> +#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
> +
> +/* Only one error code defined in SMCCC */
> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
> +
> +#endif  /* __ASM_ARM_SMCCC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 06/11] arm: smccc: handle SMCs according to SMCCC

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, 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 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, UUID and number of functions provided by service
> provider.
>
> This patch adds new file `vsmc.c`, which handles both generic SMCs
> and HVC according to SMCCC. At this moment it implements only one
> service: Standard Hypervisor Service.
>
> At this time 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>
> ---
>
>  * Reworked UUID handling (due to new UUID type definition)
>  * Renamed vsmc_handle_call() to vsmccc_handle_call() to emphasis
>    that it handles both SMC and HVC
>  * Added comment for inject_undef_exception() usage
>  * Used HSR_XXC_IMM_MASK insted of 0xFF
>  * Added full stops to comments
>
> ---
>
>  xen/arch/arm/Makefile             |   1 +
>  xen/arch/arm/traps.c              |  18 +----
>  xen/arch/arm/vsmc.c               | 160 ++++++++++++++++++++++++++++++++++++++
>  xen/include/asm-arm/vsmc.h        |  30 +++++++
>  xen/include/public/arch-arm/smc.h |  58 ++++++++++++++
>  5 files changed, 250 insertions(+), 17 deletions(-)
>  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 de00c5e..3d7dde9 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -51,6 +51,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 82cd2b1..4141a89 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -50,6 +50,7 @@
>  #include <asm/regs.h>
>  #include <asm/traps.h>
>  #include <asm/vgic.h>
> +#include <asm/vsmc.h>
>  #include <asm/vtimer.h>
>
>  #include "decode.h"
> @@ -2155,23 +2156,6 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>      inject_dabt_exception(regs, info.gva, hsr.len);
>  }
>
> -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 ( current->domain->arch.monitor.privileged_call_enabled )
> -        rc = monitor_smc();
> -
> -    if ( rc != 1 )
> -        inject_undef_exception(regs, hsr);
> -}
> -

In general we try to avoid code movement in the same patch as new code. 
I am ok with that one, but please avoid this in the future.

>  static void enter_hypervisor_head(struct cpu_user_regs *regs)
>  {
>      if ( guest_mode(regs) )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> new file mode 100644
> index 0000000..0a81294
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,160 @@
> +/*
> + * 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/lib.h>
> +#include <xen/types.h>
> +#include <public/arch-arm/smc.h>
> +#include <asm/monitor.h>
> +#include <asm/regs.h>
> +#include <asm/smccc.h>
> +#include <asm/traps.h>
> +#include <asm/vsmc.h>
> +
> +/* Number of functions currently supported by Hypervisor Service. */
> +#define XEN_SMCCC_FUNCTION_COUNT 3
> +
> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
> +{
> +#define FILL_UUID(n) \
> +    set_user_reg(regs, n, (register_t) u[n * 4 + 0] << 24 |                     \
> +                                       u[n * 4 + 1] << 16 |                     \
> +                                       u[n * 4 + 2] << 8  |                     \
> +                                       u[n * 4 + 3] << 0  )

This does not seem to match the spec: "Bytes 0...3 with byte 0 in the 
low-order bits".

Which I understand as byte 0 should be in [0...7], byte 1 [8..15]...

> +
> +    FILL_UUID(0);
> +    FILL_UUID(1);
> +    FILL_UUID(2);
> +    FILL_UUID(3);
> +#undef FILL_UUID

This function is really hard to parse and I don't think the macro is 
worth it.

I would do something like:

for (i = 0; i < 4)
{
     uint8_t *bytes = &u[n * 4];
     uint32_t r;

     r = bytes[0];
     r |= bytes[1] << 8;
     r |= bytes[2] << 16;
     r |= bytes[3] << 24;

     set_user_reg(...);
}

Plus some comment on top explaining the layout. Yes, it takes more line, 
but at least it is easier to read it.

> +}
> +
> +/* SMCCC interface for hypervisor. Tell about itself. */
> +static bool handle_hypervisor(struct cpu_user_regs *regs)
> +{
> +    static const xen_uuid_t xen_uuid = XEN_SMCCC_UID;

Missing newline here.

> +    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:
> +        fill_uuid(regs, xen_uuid);
> +        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;

default:
        return false;

> +    }
> +    return false;

And drop that one.

> +}
> +
> +/*
> + * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
> + * returns true if that was valid SMCCC call (even if function number
> + * was unkown).

s/unkown/unknown/

> + */
> +static bool vsmccc_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,
> +     * because it is not stored in HSR.ISS field. To get immediate
> +     * value we need to disassemble instruction at current pc, which
> +     * is expensive. 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 & HSR_XXC_IMM_MASK) != 0)
> +            return false;
> +        break;
> +    case HSR_EC_SMC32:
> +        break;
> +    default:
> +        return false;
> +    }
> +
> +    /* 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 true;
> +    }
> +
> +    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 true;
> +}
> +
> +/* This function will be called from traps.c. */

I don't think this is comment is necessary.

> +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 )
> +        return;
> +
> +    /*
> +     * Use standard routines to handle the call.
> +     * vsmccc_handle_call() will return false if this call is not
> +     * SMCCC compatbile (i.e. immediate value != 0). As it is not
> +     * compatible, we can't be sure that guest will understand
> +     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
> +     */
> +    if ( vsmccc_handle_call(regs) )
> +        advance_pc(regs, hsr);
> +    else
> +        inject_undef_exception(regs, hsr);
> +}
> +
> +/*
> + * 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..31aaa55
> --- /dev/null
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -0,0 +1,30 @@
> +/*
> + * 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>
> +
> +void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);

I don't see any benefits of having a new header with just two prototype. 
It would make sense to move them in traps.h.

> +
> +#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..3d3cd90
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smc.h

Likely, this should be called smccc.h.

> @@ -0,0 +1,58 @@
> +/*
> + * 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__
> +
> +#include "public/xen.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.
> + */
> +#define XEN_SMCCC_MAJOR_REVISION 0
> +#define XEN_SMCCC_MINOR_REVISION 1
> +
> +/* Hypervisor Service UID. Randomly generated with uuidgen. */
> +#define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
> +				      0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
> +
> +#endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:b
> + */
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, 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`.
>
> Older PSCI 0.1 uses SMC function identifiers in range that is
> resereved for exisings APIs (ARM DEN 0028B, page 16), while newer

s/resereved/reserved/
s/exisings/existing/

> PSCI 0.2 is defined as "standard secure service" with own ranges

0.2 and later

"with its own ranges" I think.

> (ARM DEN 0028B, page 18).
>
> 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>
> ---
>
>  * Fixed mistakes about non-existant PSCI 2.0
>  * Added special SMC function number handling for PSCI 0.1
>  * Fixed coding style in  handle_psci_0_1()
>  * Changed how return do_trap_hvc_smccc() is called from traps.c
>  * Renamed SSC to SSSC (Standard Secure Service Calls)
>
> ---
>  xen/arch/arm/traps.c              | 117 +------------------------
>  xen/arch/arm/vsmc.c               | 175 ++++++++++++++++++++++++++++++++++++--
>  xen/include/asm-arm/smccc.h       |   4 +
>  xen/include/asm-arm/vsmc.h        |   1 +
>  xen/include/public/arch-arm/smc.h |   8 ++
>  5 files changed, 183 insertions(+), 122 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index 4141a89..517e013 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1451,119 +1451,6 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  }
>  #endif
>
> -#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> -#define PSCI_ARG(reg, n) get_user_reg(reg, n)
> -
> -#ifdef CONFIG_ARM_64
> -#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
> -#else
> -#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_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
> -    switch( fid )
> -    {
> -    case PSCI_cpu_off:
> -        {
> -            uint32_t pstate = PSCI_ARG32(regs,1);
> -            perfc_incr(vpsci_cpu_off);
> -            PSCI_SET_RESULT(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_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
> -        }
> -        break;
> -    case PSCI_0_2_FN_PSCI_VERSION:
> -        perfc_incr(vpsci_version);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_version());
> -        break;
> -    case PSCI_0_2_FN_CPU_OFF:
> -        perfc_incr(vpsci_cpu_off);
> -        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> -        break;
> -    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
> -        perfc_incr(vpsci_migrate_info_type);
> -        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> -        break;
> -    case PSCI_0_2_FN_SYSTEM_RESET:
> -        perfc_incr(vpsci_system_reset);
> -        do_psci_0_2_system_reset();
> -        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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
> @@ -2252,7 +2139,7 @@ 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);
> +            return do_trap_hvc_smccc(regs);
>          do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
>          break;
>  #ifdef CONFIG_ARM_64
> @@ -2264,7 +2151,7 @@ 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);
> +            return do_trap_hvc_smccc(regs);
>          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 0a81294..956d4ef 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -19,6 +19,7 @@
>  #include <xen/types.h>
>  #include <public/arch-arm/smc.h>
>  #include <asm/monitor.h>
> +#include <asm/psci.h>
>  #include <asm/regs.h>
>  #include <asm/smccc.h>
>  #include <asm/traps.h>
> @@ -27,6 +28,9 @@
>  /* Number of functions currently supported by Hypervisor Service. */
>  #define XEN_SMCCC_FUNCTION_COUNT 3
>
> +/* Number of functions currently supported by Standard Service Service Calls. */
> +#define SSSC_SMCCC_FUNCTION_COUNT 13
> +
>  static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
>  {
>  #define FILL_UUID(n) \
> @@ -62,6 +66,133 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
>      return false;
>  }
>
> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
> +#define PSCI_ARG(reg, n) get_user_reg(reg, n)
> +
> +#ifdef CONFIG_ARM_64
> +#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
> +#else
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> +#endif
> +
> +/* PSCI 0.1 interface */
> +static bool handle_psci_0_1(struct cpu_user_regs *regs)
> +{
> +    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )

I am not sure to understand the mask here.

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

Missing newline here. I am ok with you do the clean-up in this patch 
rather separately.

> +        perfc_incr(vpsci_cpu_off);
> +        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
> +        return true;
> +    }
> +    case PSCI_cpu_on:
> +    {
> +        uint32_t vcpuid = PSCI_ARG32(regs, 1);
> +        register_t epoint = PSCI_ARG(regs, 2);

Ditto.

> +        perfc_incr(vpsci_cpu_on);
> +        PSCI_SET_RESULT(regs, 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 0.2 interface and other Standard Secure Calls */
> +static bool handle_sssc(struct cpu_user_regs *regs)
> +{
> +    register_t fid = PSCI_ARG(regs, 0);
> +
> +    switch ( ARM_SMCCC_FUNC_NUM(fid) )
> +    {
> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> +        perfc_incr(vpsci_version);
> +        PSCI_SET_RESULT(regs, do_psci_0_2_version());
> +        return true;

It is a bit hard to read. Some newline between each "case" would help 
for clarity I think.

> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> +        perfc_incr(vpsci_cpu_off);
> +        PSCI_SET_RESULT(regs, 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);
> +        PSCI_SET_RESULT(regs, 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) )
> +            PSCI_SET_RESULT(regs, 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();
> +        PSCI_SET_RESULT(regs, 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();
> +        PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
> +            register_t epoint = PSCI_ARG(regs, 2);
> +            register_t cid = PSCI_ARG(regs, 3);
> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
> +            register_t epoint = PSCI_ARG(regs, 2);
> +            register_t cid = PSCI_ARG(regs, 3);
> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
> +            uint32_t laff = PSCI_ARG32(regs, 2);
> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
> +            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
> +        }
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_COUNT:
> +        set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
> +        return true;
> +    case ARM_SMCCC_FUNC_CALL_UID:
> +    {
> +        static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;

Newline here please. But can't we just do:

return fill_uuid(regs, SSC_SMCCC_UID);

This would make the code simpler.

> +        fill_uuid(regs, psci_uuid);
> +        return true;
> +    }
> +    case ARM_SMCCC_FUNC_CALL_REVISION:
> +        set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
> +        set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);

Same here.

> +        return true;
> +    }
> +    return false;
> +}
> +
>  /*
>   * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
>   * returns true if that was valid SMCCC call (even if function number
> @@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>  {
>      bool handled = false;
>      const union hsr hsr = { .bits = regs->hsr };
> +    register_t func_id = get_user_reg(regs, 0);

Please introduce func_id in patch #6 rather than doing this rework here.

>
>      /*
>       * Check immediate value for HVC32, HVC64 and SMC64.
> @@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
>      }
>
>      /* 64 bit calls are allowed only from 64 bit domains. */
> -    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
> +    if ( ARM_SMCCC_IS_64(func_id) &&
>           is_32bit_domain(current->domain) )
>      {
>          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>          return true;
>      }
>
> -    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
> +    /*
> +     * Special case: identifier range for existing APIs.
> +     * This range is described in SMCCC (ARM DEN 0028B, page 16),
> +     * but it does not conforms to standard function identifier
> +     * encoding.
> +     */
> +    if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
> +         func_id <= ARM_SMCCC_RESERVED_RANGE_END )
> +        handled = handle_psci_0_1(regs);

The region "reserved for existing APIs" is not only for psci 0.1. You 
should make it clear in the name.

> +    else
>      {
> -    case ARM_SMCCC_OWNER_HYPERVISOR:
> -        handled = handle_hypervisor(regs);
> -        break;
> +        switch ( ARM_SMCCC_OWNER_NUM(func_id) )
> +        {
> +        case ARM_SMCCC_OWNER_HYPERVISOR:
> +            handled = handle_hypervisor(regs);
> +            break;
> +        case ARM_SMCCC_OWNER_STANDARD:
> +            handled = handle_sssc(regs);
> +            break;
> +        }
>      }
>
>      if ( !handled )
>      {
> -        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
> -                get_user_reg(regs, 0));
> +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", func_id);
>          /* Inform caller that function is not supported. */
>          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>      }
> @@ -150,6 +296,21 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
>          inject_undef_exception(regs, hsr);
>  }
>
> +/* This function will be called from traps.c */
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs)
> +{
> +    const union hsr hsr = { .bits = regs->hsr };
> +
> +    /*
> +     * vsmccc_handle_call() will return false if this call is not
> +     * SMCCC compatbile (i.e. immediate value != 0). As it is not

s/compatbile/compatible/. And you want to use e.g instead of i.e because 
"immediate value != 0" is not the only way to return false.

> +     * compatible, we can't be sure that guest will understand
> +     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
> +     */
> +    if ( !vsmccc_handle_call(regs) )
> +        inject_undef_exception(regs, hsr);
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
> index 67da3fb..7c0003c 100644
> --- a/xen/include/asm-arm/smccc.h
> +++ b/xen/include/asm-arm/smccc.h
> @@ -80,6 +80,10 @@
>  /* Only one error code defined in SMCCC */
>  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>
> +/* SMCCC function identifier range which is reserved for existing APIs */
> +#define ARM_SMCCC_RESERVED_RANGE_START  0x0
> +#define ARM_SMCCC_RESERVED_RANGE_END    0x0100FFFF
> +
>  #endif  /* __ASM_ARM_SMCCC_H__ */
>
>  /*
> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
> index 31aaa55..90ff610 100644
> --- a/xen/include/asm-arm/vsmc.h
> +++ b/xen/include/asm-arm/vsmc.h
> @@ -17,6 +17,7 @@
>  #include <xen/types.h>
>
>  void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
> +void do_trap_hvc_smccc(struct cpu_user_regs *regs);
>
>  #endif  /* __ASM_ARM_VSMC_H__ */
>
> diff --git a/xen/include/public/arch-arm/smc.h b/xen/include/public/arch-arm/smc.h
> index 3d3cd90..c5327e3 100644
> --- a/xen/include/public/arch-arm/smc.h
> +++ b/xen/include/public/arch-arm/smc.h
> @@ -46,6 +46,14 @@
>  #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 0x9acf, \
>  				      0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
>
> +/* Standard Service Service Call version. */
> +#define SSSC_SMCCC_MAJOR_REVISION 0
> +#define SSSC_SMCCC_MINOR_REVISION 1
> +
> +/* Standard Service Call UID. Randomly generated with uuidgen. */
> +#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 0x9220,\
> +				      0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
> +
>  #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */

Cheers,

>
>  /*
>

-- 
Julien Grall

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

Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

[Julien Grall]

Hi Volodymyr,

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> smccc.h provides definitions to construct SMC call function number according
> to SMCCC. We don't need multiple definitions for one thing, and definitions
> in smccc.h are more generic than ones used in psci.h.
>
> So psci.h will only provide function codes, while whole SMC function
> identifier will be constructed using generic macros from smccc.h.
>
> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because this
> is a new entity. It can lead to problems, if we'll just change value of
> PSCI_0_2_FN_xxx without renaming it.

I don't think "new entity" is a good reason to rename them. And the 
previous naming was kind of nice to read. More that you still use 
PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.

So what is the exact problem? Is it because you are worry to miss some 
of them?

>
> This change also affects vsmc.c and seattle.c, because they both use PSCI
> function numbers.

This paragraph could be dropped.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
>  * Reworked definitions to minimize their lenght
>  * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
>
> ---
>  xen/arch/arm/platforms/seattle.c |  5 +++--
>  xen/arch/arm/psci.c              | 10 ++++-----
>  xen/arch/arm/vsmc.c              | 24 +++++++++++-----------
>  xen/include/asm-arm/psci.h       | 44 ++++++++++++++++++----------------------
>  4 files changed, 40 insertions(+), 43 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 86dce91..fb2ad13 100644
> --- a/xen/arch/arm/platforms/seattle.c
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -19,6 +19,7 @@
>
>  #include <asm/platform.h>
>  #include <asm/psci.h>
> +#include <asm/vsmc.h>

Again, vsmc.h stands for "virtual SMC". Here you use for the "physical 
SMC". So please don't include vsmc.h here.

>
>  static const char * const seattle_dt_compat[] __initconst =
>  {
> @@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] __initconst =
>   */
>  static void seattle_system_reset(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>  }
>
>  static void seattle_system_off(void)
>  {
> -    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +    call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>  }
>
>  PLATFORM_START(seattle, "SEATTLE")
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 34ee97e..645fe58 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -31,9 +31,9 @@
>   * (native-width) function ID.
>   */
>  #ifdef CONFIG_ARM_64
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN64_##name
> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN64(name)
>  #else
> -#define PSCI_0_2_FN_NATIVE(name)	PSCI_0_2_FN_##name
> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN32(name)
>  #endif
>
>  uint32_t psci_ver;
> @@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
>  void call_psci_system_off(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>  }
>
>  void call_psci_system_reset(void)
>  {
>      if ( psci_ver > PSCI_VERSION(0, 1) )
> -        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
> +        call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>  }
>
>  int __init psci_is_smc_method(const struct dt_device_node *psci)
> @@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
>          }
>      }
>
> -    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
> +    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_VERSION), 0, 0, 0);
>
>      /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
>      if ( psci_ver != PSCI_VERSION(0, 2) && PSCI_VERSION_MAJOR(psci_ver) != 1 )
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 956d4ef..46a2fde 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs *regs)
>  /* 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 ) );
> +    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);

I don't see any reason to do the renaming in psci_mode_check given that 
you will remove this function in the next patch.

>  }
>
>  /* PSCI 0.2 interface and other Standard Secure Calls */
> @@ -112,34 +112,34 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>
>      switch ( ARM_SMCCC_FUNC_NUM(fid) )
>      {
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
> +    case PSCI_0_2_FUNC_PSCI_VERSION:
>          perfc_incr(vpsci_version);
>          PSCI_SET_RESULT(regs, do_psci_0_2_version());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
> +    case PSCI_0_2_FUNC_CPU_OFF:
>          perfc_incr(vpsci_cpu_off);
>          PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> +    case PSCI_0_2_FUNC_MIGRATE_INFO_TYPE:
>          perfc_incr(vpsci_migrate_info_type);
>          PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> +    case PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU:
>          perfc_incr(vpsci_migrate_info_up_cpu);
>          if ( psci_mode_check(current->domain, fid) )
>              PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
> +    case PSCI_0_2_FUNC_SYSTEM_OFF:
>          perfc_incr(vpsci_system_off);
>          do_psci_0_2_system_off();
>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
> -        return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
> +       return true;
> +    case PSCI_0_2_FUNC_SYSTEM_RESET:
>          perfc_incr(vpsci_system_reset);
>          do_psci_0_2_system_reset();
>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
> +    case PSCI_0_2_FUNC_CPU_ON:
>          perfc_incr(vpsci_cpu_on);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -149,7 +149,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
> +    case PSCI_0_2_FUNC_CPU_SUSPEND:
>          perfc_incr(vpsci_cpu_suspend);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -159,7 +159,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
> +    case PSCI_0_2_FUNC_AFFINITY_INFO:
>          perfc_incr(vpsci_cpu_affinity_info);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> @@ -168,7 +168,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>              PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, laff));
>          }
>          return true;
> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
> +    case PSCI_0_2_FUNC_MIGRATE:
>          perfc_incr(vpsci_cpu_migrate);
>          if ( psci_mode_check(current->domain, fid) )
>          {
> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
> index be2458a..14fb98f 100644
> --- a/xen/include/asm-arm/psci.h
> +++ b/xen/include/asm-arm/psci.h
> @@ -1,6 +1,8 @@
>  #ifndef __ASM_PSCI_H__
>  #define __ASM_PSCI_H__
>
> +#include <asm/smccc.h>
> +
>  /* PSCI return values (inclusive of all PSCI versions) */
>  #define PSCI_SUCCESS                 0
>  #define PSCI_NOT_SUPPORTED          -1
> @@ -41,30 +43,24 @@ register_t do_psci_0_2_migrate_info_up_cpu(void);
>  void do_psci_0_2_system_off(void);
>  void do_psci_0_2_system_reset(void);
>
> -/* PSCI v0.2 interface */

Why did you drop this comment?

> -#define PSCI_0_2_FN_BASE        0x84000000
> -#define PSCI_0_2_FN(n)          (PSCI_0_2_FN_BASE + (n))
> -#define PSCI_0_2_64BIT          0x40000000
> -#define PSCI_0_2_FN64_BASE      \
> -                        (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
> -#define PSCI_0_2_FN64(n)        (PSCI_0_2_FN64_BASE + (n))
> -
> -#define PSCI_0_2_FN_PSCI_VERSION        PSCI_0_2_FN(0)
> -#define PSCI_0_2_FN_CPU_SUSPEND         PSCI_0_2_FN(1)
> -#define PSCI_0_2_FN_CPU_OFF             PSCI_0_2_FN(2)
> -#define PSCI_0_2_FN_CPU_ON              PSCI_0_2_FN(3)
> -#define PSCI_0_2_FN_AFFINITY_INFO       PSCI_0_2_FN(4)
> -#define PSCI_0_2_FN_MIGRATE             PSCI_0_2_FN(5)
> -#define PSCI_0_2_FN_MIGRATE_INFO_TYPE   PSCI_0_2_FN(6)
> -#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
> -#define PSCI_0_2_FN_SYSTEM_OFF          PSCI_0_2_FN(8)
> -#define PSCI_0_2_FN_SYSTEM_RESET        PSCI_0_2_FN(9)
> -
> -#define PSCI_0_2_FN64_CPU_SUSPEND       PSCI_0_2_FN64(1)
> -#define PSCI_0_2_FN64_CPU_ON            PSCI_0_2_FN64(3)
> -#define PSCI_0_2_FN64_AFFINITY_INFO     PSCI_0_2_FN64(4)
> -#define PSCI_0_2_FN64_MIGRATE           PSCI_0_2_FN64(5)
> -#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
> +#define PSCI_0_2_FN32(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
> +                                            ARM_SMCCC_SMC_32,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD,           \
> +                                            PSCI_0_2_FUNC_##name)

The indentation looks wrong here.

> +#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
> +                                            ARM_SMCCC_SMC_64,                   \
> +                                            ARM_SMCCC_OWNER_STANDARD,           \
> +                                            PSCI_0_2_FUNC_##name)


Ditto.

> +#define PSCI_0_2_FUNC_PSCI_VERSION        0
> +#define PSCI_0_2_FUNC_CPU_SUSPEND         1
> +#define PSCI_0_2_FUNC_CPU_OFF             2
> +#define PSCI_0_2_FUNC_CPU_ON              3
> +#define PSCI_0_2_FUNC_AFFINITY_INFO       4
> +#define PSCI_0_2_FUNC_MIGRATE             5
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
> +#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
> +#define PSCI_0_2_FUNC_SYSTEM_OFF          8
> +#define PSCI_0_2_FUNC_SYSTEM_RESET        9
>
>  /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
>  #define PSCI_0_2_AFFINITY_LEVEL_ON      0
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 11/11] arm: enable XENFEAT_ARM_SMCCC_supported feature

[Julien Grall]

On 22/08/17 08:29, Jan Beulich wrote:
>>>> On 21.08.17 at 22:27, <volodymyr_babchuk@epam.com> wrote:
>> As Xen now supports SMCCC, we can enable this feature to tell
>> guests that it is safe to call generic SMCs and HVCs.
>
> I think this and patch 10 should be folded.

+1.

>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  xen/common/kernel.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/common/kernel.c b/xen/common/kernel.c
>> index ce7cb8a..18c4d51 100644
>> --- a/xen/common/kernel.c
>> +++ b/xen/common/kernel.c
>> @@ -332,6 +332,9 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
>>                      (1U << XENFEAT_auto_translated_physmap);
>>              if ( is_hardware_domain(d) )
>>                  fi.submap |= 1U << XENFEAT_dom0;
>> +#ifdef CONFIG_ARM
>> +            fi.submap |= (1U << XENFEAT_ARM_SMCCC_supported);
>> +#endif
>
> Pointless parentheses (as other code in context shows you).
>
> With both aspects taken care of
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> Jan
>

-- 
Julien Grall

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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Julien Grall]

On 21/08/17 21:27, Volodymyr Babchuk wrote:
> This feature indicates that hypervisor is compatible with ARM
> SMC calling convention. Hypervisor will not inject an undefined
> instruction exception if an invalid SMC function were called and
> will not crash a domain if an invlalid HVC functions were called.

s/invlalid/invalid/

The last sentence is misleading. Xen will still inject and undefined 
instruction for some SMC/HVC. You may want to rework it to make it clear.

>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>  xen/include/public/features.h | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/xen/include/public/features.h b/xen/include/public/features.h
> index 2110b04..1a989b8 100644
> --- a/xen/include/public/features.h
> +++ b/xen/include/public/features.h
> @@ -102,6 +102,9 @@
>  /* Guest can use XENMEMF_vnode to specify virtual node for memory op. */
>  #define XENFEAT_memory_op_vnode_supported 13
>
> +/* arm: Hypervisor supports ARM SMC calling convention. */
> +#define XENFEAT_ARM_SMCCC_supported       14
> +
>  #define XENFEAT_NR_SUBMAPS 1
>
>  #endif /* __XEN_PUBLIC_FEATURES_H__ */
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`

[Volodymyr Babchuk]

Hi Julien,

Thanks for the review.

On 24.08.17 19:58, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 21/08/17 21:27, 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`.
>>
>> Older PSCI 0.1 uses SMC function identifiers in range that is
>> resereved for exisings APIs (ARM DEN 0028B, page 16), while newer
> 
> s/resereved/reserved/
> s/exisings/existing/
> 
>> PSCI 0.2 is defined as "standard secure service" with own ranges
> 
> 0.2 and later
> 
> "with its own ranges" I think.
> 
>> (ARM DEN 0028B, page 18).
>>
>> 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>
>> ---
>>
>>  * Fixed mistakes about non-existant PSCI 2.0
>>  * Added special SMC function number handling for PSCI 0.1
>>  * Fixed coding style in  handle_psci_0_1()
>>  * Changed how return do_trap_hvc_smccc() is called from traps.c
>>  * Renamed SSC to SSSC (Standard Secure Service Calls)
>>
>> ---
>>  xen/arch/arm/traps.c              | 117 +------------------------
>>  xen/arch/arm/vsmc.c               | 175 
>> ++++++++++++++++++++++++++++++++++++--
>>  xen/include/asm-arm/smccc.h       |   4 +
>>  xen/include/asm-arm/vsmc.h        |   1 +
>>  xen/include/public/arch-arm/smc.h |   8 ++
>>  5 files changed, 183 insertions(+), 122 deletions(-)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index 4141a89..517e013 100644
>> --- a/xen/arch/arm/traps.c
>> +++ b/xen/arch/arm/traps.c
>> @@ -1451,119 +1451,6 @@ static void do_debug_trap(struct cpu_user_regs 
>> *regs, unsigned int code)
>>  }
>>  #endif
>>
>> -#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> -#define PSCI_ARG(reg, n) get_user_reg(reg, n)
>> -
>> -#ifdef CONFIG_ARM_64
>> -#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
>> -#else
>> -#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_SET_RESULT(regs, PSCI_INVALID_PARAMETERS);
>> -    switch( fid )
>> -    {
>> -    case PSCI_cpu_off:
>> -        {
>> -            uint32_t pstate = PSCI_ARG32(regs,1);
>> -            perfc_incr(vpsci_cpu_off);
>> -            PSCI_SET_RESULT(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_SET_RESULT(regs, do_psci_cpu_on(vcpuid, epoint));
>> -        }
>> -        break;
>> -    case PSCI_0_2_FN_PSCI_VERSION:
>> -        perfc_incr(vpsci_version);
>> -        PSCI_SET_RESULT(regs, do_psci_0_2_version());
>> -        break;
>> -    case PSCI_0_2_FN_CPU_OFF:
>> -        perfc_incr(vpsci_cpu_off);
>> -        PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
>> -        break;
>> -    case PSCI_0_2_FN_MIGRATE_INFO_TYPE:
>> -        perfc_incr(vpsci_migrate_info_type);
>> -        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
>> -        break;
>> -    case PSCI_0_2_FN_SYSTEM_RESET:
>> -        perfc_incr(vpsci_system_reset);
>> -        do_psci_0_2_system_reset();
>> -        PSCI_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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_SET_RESULT(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
>> @@ -2252,7 +2139,7 @@ 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);
>> +            return do_trap_hvc_smccc(regs);
>>          do_trap_hypercall(regs, (register_t *)&regs->r12, hsr.iss);
>>          break;
>>  #ifdef CONFIG_ARM_64
>> @@ -2264,7 +2151,7 @@ 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);
>> +            return do_trap_hvc_smccc(regs);
>>          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 0a81294..956d4ef 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -19,6 +19,7 @@
>>  #include <xen/types.h>
>>  #include <public/arch-arm/smc.h>
>>  #include <asm/monitor.h>
>> +#include <asm/psci.h>
>>  #include <asm/regs.h>
>>  #include <asm/smccc.h>
>>  #include <asm/traps.h>
>> @@ -27,6 +28,9 @@
>>  /* Number of functions currently supported by Hypervisor Service. */
>>  #define XEN_SMCCC_FUNCTION_COUNT 3
>>
>> +/* Number of functions currently supported by Standard Service 
>> Service Calls. */
>> +#define SSSC_SMCCC_FUNCTION_COUNT 13
>> +
>>  static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t u)
>>  {
>>  #define FILL_UUID(n) \
>> @@ -62,6 +66,133 @@ static bool handle_hypervisor(struct cpu_user_regs 
>> *regs)
>>      return false;
>>  }
>>
>> +#define PSCI_SET_RESULT(reg, val) set_user_reg(reg, 0, val)
>> +#define PSCI_ARG(reg, n) get_user_reg(reg, n)
>> +
>> +#ifdef CONFIG_ARM_64
>> +#define PSCI_ARG32(reg, n) (uint32_t)(get_user_reg(reg, n) & 0xFFFFFFFF)
>> +#else
>> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
>> +#endif
>> +
>> +/* PSCI 0.1 interface */
>> +static bool handle_psci_0_1(struct cpu_user_regs *regs)
>> +{
>> +    switch ( get_user_reg(regs,0) & 0xFFFFFFFF )
> 
> I am not sure to understand the mask here.
> 
>> +    {
>> +    case PSCI_cpu_off:
>> +    {
>> +        uint32_t pstate = PSCI_ARG32(regs, 1);
> 
> Missing newline here. I am ok with you do the clean-up in this patch 
> rather separately.
> 
>> +        perfc_incr(vpsci_cpu_off);
>> +        PSCI_SET_RESULT(regs, do_psci_cpu_off(pstate));
>> +        return true;
>> +    }
>> +    case PSCI_cpu_on:
>> +    {
>> +        uint32_t vcpuid = PSCI_ARG32(regs, 1);
>> +        register_t epoint = PSCI_ARG(regs, 2);
> 
> Ditto.
> 
>> +        perfc_incr(vpsci_cpu_on);
>> +        PSCI_SET_RESULT(regs, 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 0.2 interface and other Standard Secure Calls */
>> +static bool handle_sssc(struct cpu_user_regs *regs)
>> +{
>> +    register_t fid = PSCI_ARG(regs, 0);
>> +
>> +    switch ( ARM_SMCCC_FUNC_NUM(fid) )
>> +    {
>> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
>> +        perfc_incr(vpsci_version);
>> +        PSCI_SET_RESULT(regs, do_psci_0_2_version());
>> +        return true;
> 
> It is a bit hard to read. Some newline between each "case" would help 
> for clarity I think.
> 
>> +    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
>> +        perfc_incr(vpsci_cpu_off);
>> +        PSCI_SET_RESULT(regs, 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);
>> +        PSCI_SET_RESULT(regs, 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) )
>> +            PSCI_SET_RESULT(regs, 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();
>> +        PSCI_SET_RESULT(regs, 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();
>> +        PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
>> +            register_t epoint = PSCI_ARG(regs, 2);
>> +            register_t cid = PSCI_ARG(regs, 3);
>> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
>> +            register_t epoint = PSCI_ARG(regs, 2);
>> +            register_t cid = PSCI_ARG(regs, 3);
>> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG(regs, 1);
>> +            uint32_t laff = PSCI_ARG32(regs, 2);
>> +            PSCI_SET_RESULT(regs, 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 = PSCI_ARG32(regs, 1);
>> +            PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
>> +        }
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_COUNT:
>> +        set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
>> +        return true;
>> +    case ARM_SMCCC_FUNC_CALL_UID:
>> +    {
>> +        static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
> 
> Newline here please. But can't we just do:
> 
> return fill_uuid(regs, SSC_SMCCC_UID);
> 
> This would make the code simpler.
I'm not sure that I got this. fill_uuid() returns void, while 
handle_sssc() returns bool.

>> +        fill_uuid(regs, psci_uuid);
>> +        return true;
>> +    }
>> +    case ARM_SMCCC_FUNC_CALL_REVISION:
>> +        set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
>> +        set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
> 
> Same here.
Under "same" you meant newline, correct?
>> +        return true;
>> +    }
>> +    return false;
>> +}
>> +
>>  /*
>>   * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
>>   * returns true if that was valid SMCCC call (even if function number
>> @@ -71,6 +202,7 @@ static bool vsmccc_handle_call(struct cpu_user_regs 
>> *regs)
>>  {
>>      bool handled = false;
>>      const union hsr hsr = { .bits = regs->hsr };
>> +    register_t func_id = get_user_reg(regs, 0);
> 
> Please introduce func_id in patch #6 rather than doing this rework here.
> 
>>
>>      /*
>>       * Check immediate value for HVC32, HVC64 and SMC64.
>> @@ -94,24 +226,38 @@ static bool vsmccc_handle_call(struct 
>> cpu_user_regs *regs)
>>      }
>>
>>      /* 64 bit calls are allowed only from 64 bit domains. */
>> -    if ( ARM_SMCCC_IS_64(get_user_reg(regs, 0)) &&
>> +    if ( ARM_SMCCC_IS_64(func_id) &&
>>           is_32bit_domain(current->domain) )
>>      {
>>          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>>          return true;
>>      }
>>
>> -    switch ( ARM_SMCCC_OWNER_NUM(get_user_reg(regs, 0)) )
>> +    /*
>> +     * Special case: identifier range for existing APIs.
>> +     * This range is described in SMCCC (ARM DEN 0028B, page 16),
>> +     * but it does not conforms to standard function identifier
>> +     * encoding.
>> +     */
>> +    if ( func_id >= ARM_SMCCC_RESERVED_RANGE_START &&
>> +         func_id <= ARM_SMCCC_RESERVED_RANGE_END )
>> +        handled = handle_psci_0_1(regs);
> 
> The region "reserved for existing APIs" is not only for psci 0.1. You 
> should make it clear in the name.
> 
>> +    else
>>      {
>> -    case ARM_SMCCC_OWNER_HYPERVISOR:
>> -        handled = handle_hypervisor(regs);
>> -        break;
>> +        switch ( ARM_SMCCC_OWNER_NUM(func_id) )
>> +        {
>> +        case ARM_SMCCC_OWNER_HYPERVISOR:
>> +            handled = handle_hypervisor(regs);
>> +            break;
>> +        case ARM_SMCCC_OWNER_STANDARD:
>> +            handled = handle_sssc(regs);
>> +            break;
>> +        }
>>      }
>>
>>      if ( !handled )
>>      {
>> -        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n",
>> -                get_user_reg(regs, 0));
>> +        gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", 
>> func_id);
>>          /* Inform caller that function is not supported. */
>>          set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
>>      }
>> @@ -150,6 +296,21 @@ void do_trap_smc(struct cpu_user_regs *regs, 
>> const union hsr hsr)
>>          inject_undef_exception(regs, hsr);
>>  }
>>
>> +/* This function will be called from traps.c */
>> +void do_trap_hvc_smccc(struct cpu_user_regs *regs)
>> +{
>> +    const union hsr hsr = { .bits = regs->hsr };
>> +
>> +    /*
>> +     * vsmccc_handle_call() will return false if this call is not
>> +     * SMCCC compatbile (i.e. immediate value != 0). As it is not
> 
> s/compatbile/compatible/. And you want to use e.g instead of i.e because 
> "immediate value != 0" is not the only way to return false.
> 
>> +     * compatible, we can't be sure that guest will understand
>> +     * ARM_SMCCC_ERR_UNKNOWN_FUNCTION.
>> +     */
>> +    if ( !vsmccc_handle_call(regs) )
>> +        inject_undef_exception(regs, hsr);
>> +}
>> +
>>  /*
>>   * Local variables:
>>   * mode: C
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> index 67da3fb..7c0003c 100644
>> --- a/xen/include/asm-arm/smccc.h
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -80,6 +80,10 @@
>>  /* Only one error code defined in SMCCC */
>>  #define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>>
>> +/* SMCCC function identifier range which is reserved for existing 
>> APIs */
>> +#define ARM_SMCCC_RESERVED_RANGE_START  0x0
>> +#define ARM_SMCCC_RESERVED_RANGE_END    0x0100FFFF
>> +
>>  #endif  /* __ASM_ARM_SMCCC_H__ */
>>
>>  /*
>> diff --git a/xen/include/asm-arm/vsmc.h b/xen/include/asm-arm/vsmc.h
>> index 31aaa55..90ff610 100644
>> --- a/xen/include/asm-arm/vsmc.h
>> +++ b/xen/include/asm-arm/vsmc.h
>> @@ -17,6 +17,7 @@
>>  #include <xen/types.h>
>>
>>  void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
>> +void do_trap_hvc_smccc(struct cpu_user_regs *regs);
>>
>>  #endif  /* __ASM_ARM_VSMC_H__ */
>>
>> diff --git a/xen/include/public/arch-arm/smc.h 
>> b/xen/include/public/arch-arm/smc.h
>> index 3d3cd90..c5327e3 100644
>> --- a/xen/include/public/arch-arm/smc.h
>> +++ b/xen/include/public/arch-arm/smc.h
>> @@ -46,6 +46,14 @@
>>  #define XEN_SMCCC_UID XEN_DEFINE_UUID(0xa71812dc, 0xc698, 0x4369, 
>> 0x9acf, \
>>                        0x79, 0xd1, 0x8d, 0xde, 0xe6, 0x67)
>>
>> +/* Standard Service Service Call version. */
>> +#define SSSC_SMCCC_MAJOR_REVISION 0
>> +#define SSSC_SMCCC_MINOR_REVISION 1
>> +
>> +/* Standard Service Call UID. Randomly generated with uuidgen. */
>> +#define SSSC_SMCCC_UID XEN_DEFINE_UUID(0xf863386f, 0x4b39, 0x4cbd, 
>> 0x9220,\
>> +                      0xce, 0x16, 0x41, 0xe5, 0x9f, 0x6f)
>> +
>>  #endif /* __XEN_PUBLIC_ARCH_ARM_SMC_H__ */
> 
> Cheers,
> 
>>
>>  /*
>>
> 

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

Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

[Volodymyr Babchuk]

Hi,

On 24.08.17 20:22, Julien Grall wrote:
> Hi Volodymyr,
> 
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> smccc.h provides definitions to construct SMC call function number 
>> according
>> to SMCCC. We don't need multiple definitions for one thing, and 
>> definitions
>> in smccc.h are more generic than ones used in psci.h.
>>
>> So psci.h will only provide function codes, while whole SMC function
>> identifier will be constructed using generic macros from smccc.h.
>>
>> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, because 
>> this
>> is a new entity. It can lead to problems, if we'll just change value of
>> PSCI_0_2_FN_xxx without renaming it.
> 
> I don't think "new entity" is a good reason to rename them. And the 
> previous naming was kind of nice to read. More that you still use 
> PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
> 
> So what is the exact problem? Is it because you are worry to miss some 
> of them?
Actually yes. That helped me to find references to PSCI code in seattle.c

>>
>> This change also affects vsmc.c and seattle.c, because they both use PSCI
>> function numbers.
> 
> This paragraph could be dropped.
> 
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>
>>  * Reworked definitions to minimize their lenght
>>  * Described why I replaced PSCI_0_2_FN_xxx with PSCI_0_2_FUNC_xxx
>>
>> ---
>>  xen/arch/arm/platforms/seattle.c |  5 +++--
>>  xen/arch/arm/psci.c              | 10 ++++-----
>>  xen/arch/arm/vsmc.c              | 24 +++++++++++-----------
>>  xen/include/asm-arm/psci.h       | 44 
>> ++++++++++++++++++----------------------
>>  4 files changed, 40 insertions(+), 43 deletions(-)
>>
>> diff --git a/xen/arch/arm/platforms/seattle.c 
>> b/xen/arch/arm/platforms/seattle.c
>> index 86dce91..fb2ad13 100644
>> --- a/xen/arch/arm/platforms/seattle.c
>> +++ b/xen/arch/arm/platforms/seattle.c
>> @@ -19,6 +19,7 @@
>>
>>  #include <asm/platform.h>
>>  #include <asm/psci.h>
>> +#include <asm/vsmc.h>
> 
> Again, vsmc.h stands for "virtual SMC". Here you use for the "physical 
> SMC". So please don't include vsmc.h here.
> 
>>
>>  static const char * const seattle_dt_compat[] __initconst =
>>  {
>> @@ -33,12 +34,12 @@ static const char * const seattle_dt_compat[] 
>> __initconst =
>>   */
>>  static void seattle_system_reset(void)
>>  {
>> -    call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> +    call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>>  }
>>
>>  static void seattle_system_off(void)
>>  {
>> -    call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>> +    call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>>  }
>>
>>  PLATFORM_START(seattle, "SEATTLE")
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 34ee97e..645fe58 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -31,9 +31,9 @@
>>   * (native-width) function ID.
>>   */
>>  #ifdef CONFIG_ARM_64
>> -#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN64_##name
>> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN64(name)
>>  #else
>> -#define PSCI_0_2_FN_NATIVE(name)    PSCI_0_2_FN_##name
>> +#define PSCI_0_2_FN_NATIVE(name)   PSCI_0_2_FN32(name)
>>  #endif
>>
>>  uint32_t psci_ver;
>> @@ -48,13 +48,13 @@ int call_psci_cpu_on(int cpu)
>>  void call_psci_system_off(void)
>>  {
>>      if ( psci_ver > PSCI_VERSION(0, 1) )
>> -        call_smc(PSCI_0_2_FN_SYSTEM_OFF, 0, 0, 0);
>> +        call_smc(PSCI_0_2_FN32(SYSTEM_OFF), 0, 0, 0);
>>  }
>>
>>  void call_psci_system_reset(void)
>>  {
>>      if ( psci_ver > PSCI_VERSION(0, 1) )
>> -        call_smc(PSCI_0_2_FN_SYSTEM_RESET, 0, 0, 0);
>> +        call_smc(PSCI_0_2_FN32(SYSTEM_RESET), 0, 0, 0);
>>  }
>>
>>  int __init psci_is_smc_method(const struct dt_device_node *psci)
>> @@ -144,7 +144,7 @@ int __init psci_init_0_2(void)
>>          }
>>      }
>>
>> -    psci_ver = call_smc(PSCI_0_2_FN_PSCI_VERSION, 0, 0, 0);
>> +    psci_ver = call_smc(PSCI_0_2_FN32(PSCI_VERSION), 0, 0, 0);
>>
>>      /* For the moment, we only support PSCI 0.2 and PSCI 1.x */
>>      if ( psci_ver != PSCI_VERSION(0, 2) && 
>> PSCI_VERSION_MAJOR(psci_ver) != 1 )
>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>> index 956d4ef..46a2fde 100644
>> --- a/xen/arch/arm/vsmc.c
>> +++ b/xen/arch/arm/vsmc.c
>> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs 
>> *regs)
>>  /* 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 ) );
>> +    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);
> 
> I don't see any reason to do the renaming in psci_mode_check given that 
> you will remove this function in the next patch.
Yep, but in comments for the previous series you asked me to make this code
easier to read :)
PSCI_0_2_64BIT is dropped now, so I had to use long definitions from smccc.h

>>  }
>>
>>  /* PSCI 0.2 interface and other Standard Secure Calls */
>> @@ -112,34 +112,34 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>>
>>      switch ( ARM_SMCCC_FUNC_NUM(fid) )
>>      {
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_PSCI_VERSION):
>> +    case PSCI_0_2_FUNC_PSCI_VERSION:
>>          perfc_incr(vpsci_version);
>>          PSCI_SET_RESULT(regs, do_psci_0_2_version());
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_OFF):
>> +    case PSCI_0_2_FUNC_CPU_OFF:
>>          perfc_incr(vpsci_cpu_off);
>>          PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
>> +    case PSCI_0_2_FUNC_MIGRATE_INFO_TYPE:
>>          perfc_incr(vpsci_migrate_info_type);
>>          PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_type());
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
>> +    case PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU:
>>          perfc_incr(vpsci_migrate_info_up_cpu);
>>          if ( psci_mode_check(current->domain, fid) )
>>              PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_OFF):
>> +    case PSCI_0_2_FUNC_SYSTEM_OFF:
>>          perfc_incr(vpsci_system_off);
>>          do_psci_0_2_system_off();
>>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
>> -        return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_SYSTEM_RESET):
>> +       return true;
>> +    case PSCI_0_2_FUNC_SYSTEM_RESET:
>>          perfc_incr(vpsci_system_reset);
>>          do_psci_0_2_system_reset();
>>          PSCI_SET_RESULT(regs, PSCI_INTERNAL_FAILURE);
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_ON):
>> +    case PSCI_0_2_FUNC_CPU_ON:
>>          perfc_incr(vpsci_cpu_on);
>>          if ( psci_mode_check(current->domain, fid) )
>>          {
>> @@ -149,7 +149,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, 
>> cid));
>>          }
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_CPU_SUSPEND):
>> +    case PSCI_0_2_FUNC_CPU_SUSPEND:
>>          perfc_incr(vpsci_cpu_suspend);
>>          if ( psci_mode_check(current->domain, fid) )
>>          {
>> @@ -159,7 +159,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>>              PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, 
>> epoint, cid));
>>          }
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_AFFINITY_INFO):
>> +    case PSCI_0_2_FUNC_AFFINITY_INFO:
>>          perfc_incr(vpsci_cpu_affinity_info);
>>          if ( psci_mode_check(current->domain, fid) )
>>          {
>> @@ -168,7 +168,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>>              PSCI_SET_RESULT(regs, do_psci_0_2_affinity_info(taff, 
>> laff));
>>          }
>>          return true;
>> -    case ARM_SMCCC_FUNC_NUM(PSCI_0_2_FN_MIGRATE):
>> +    case PSCI_0_2_FUNC_MIGRATE:
>>          perfc_incr(vpsci_cpu_migrate);
>>          if ( psci_mode_check(current->domain, fid) )
>>          {
>> diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
>> index be2458a..14fb98f 100644
>> --- a/xen/include/asm-arm/psci.h
>> +++ b/xen/include/asm-arm/psci.h
>> @@ -1,6 +1,8 @@
>>  #ifndef __ASM_PSCI_H__
>>  #define __ASM_PSCI_H__
>>
>> +#include <asm/smccc.h>
>> +
>>  /* PSCI return values (inclusive of all PSCI versions) */
>>  #define PSCI_SUCCESS                 0
>>  #define PSCI_NOT_SUPPORTED          -1
>> @@ -41,30 +43,24 @@ register_t do_psci_0_2_migrate_info_up_cpu(void);
>>  void do_psci_0_2_system_off(void);
>>  void do_psci_0_2_system_reset(void);
>>
>> -/* PSCI v0.2 interface */
> 
> Why did you drop this comment?
> 
>> -#define PSCI_0_2_FN_BASE        0x84000000
>> -#define PSCI_0_2_FN(n)          (PSCI_0_2_FN_BASE + (n))
>> -#define PSCI_0_2_64BIT          0x40000000
>> -#define PSCI_0_2_FN64_BASE      \
>> -                        (PSCI_0_2_FN_BASE + PSCI_0_2_64BIT)
>> -#define PSCI_0_2_FN64(n)        (PSCI_0_2_FN64_BASE + (n))
>> -
>> -#define PSCI_0_2_FN_PSCI_VERSION        PSCI_0_2_FN(0)
>> -#define PSCI_0_2_FN_CPU_SUSPEND         PSCI_0_2_FN(1)
>> -#define PSCI_0_2_FN_CPU_OFF             PSCI_0_2_FN(2)
>> -#define PSCI_0_2_FN_CPU_ON              PSCI_0_2_FN(3)
>> -#define PSCI_0_2_FN_AFFINITY_INFO       PSCI_0_2_FN(4)
>> -#define PSCI_0_2_FN_MIGRATE             PSCI_0_2_FN(5)
>> -#define PSCI_0_2_FN_MIGRATE_INFO_TYPE   PSCI_0_2_FN(6)
>> -#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU PSCI_0_2_FN(7)
>> -#define PSCI_0_2_FN_SYSTEM_OFF          PSCI_0_2_FN(8)
>> -#define PSCI_0_2_FN_SYSTEM_RESET        PSCI_0_2_FN(9)
>> -
>> -#define PSCI_0_2_FN64_CPU_SUSPEND       PSCI_0_2_FN64(1)
>> -#define PSCI_0_2_FN64_CPU_ON            PSCI_0_2_FN64(3)
>> -#define PSCI_0_2_FN64_AFFINITY_INFO     PSCI_0_2_FN64(4)
>> -#define PSCI_0_2_FN64_MIGRATE           PSCI_0_2_FN64(5)
>> -#define PSCI_0_2_FN64_MIGRATE_INFO_UP_CPU   PSCI_0_2_FN64(7)
>> +#define PSCI_0_2_FN32(name) 
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
>> +                                            
>> ARM_SMCCC_SMC_32,                   \
>> +                                            
>> ARM_SMCCC_OWNER_STANDARD,           \
>> +                                            PSCI_0_2_FUNC_##name)
> 
> The indentation looks wrong here.
> 
>> +#define PSCI_0_2_FN64(name) 
>> ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL,             \
>> +                                            
>> ARM_SMCCC_SMC_64,                   \
>> +                                            
>> ARM_SMCCC_OWNER_STANDARD,           \
>> +                                            PSCI_0_2_FUNC_##name)
> 
> 
> Ditto.
> 
>> +#define PSCI_0_2_FUNC_PSCI_VERSION        0
>> +#define PSCI_0_2_FUNC_CPU_SUSPEND         1
>> +#define PSCI_0_2_FUNC_CPU_OFF             2
>> +#define PSCI_0_2_FUNC_CPU_ON              3
>> +#define PSCI_0_2_FUNC_AFFINITY_INFO       4
>> +#define PSCI_0_2_FUNC_MIGRATE             5
>> +#define PSCI_0_2_FUNC_MIGRATE_INFO_TYPE   6
>> +#define PSCI_0_2_FUNC_MIGRATE_INFO_UP_CPU 7
>> +#define PSCI_0_2_FUNC_SYSTEM_OFF          8
>> +#define PSCI_0_2_FUNC_SYSTEM_RESET        9
>>
>>  /* PSCI v0.2 affinity level state returned by AFFINITY_INFO */
>>  #define PSCI_0_2_AFFINITY_LEVEL_ON      0
>>
> 
> Cheers,
> 

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

Re: [PATCH v4 07/11] arm: traps: handle PSCI calls inside `vsmc.c`

[Julien Grall]

On 25/08/17 11:56, Volodymyr Babchuk wrote:
> Hi Julien,

Hi Volodymyr,

> On 24.08.17 19:58, Julien Grall wrote:
>>> +    case ARM_SMCCC_FUNC_CALL_COUNT:
>>> +        set_user_reg(regs, 0, SSSC_SMCCC_FUNCTION_COUNT);
>>> +        return true;
>>> +    case ARM_SMCCC_FUNC_CALL_UID:
>>> +    {
>>> +        static const xen_uuid_t psci_uuid = SSSC_SMCCC_UID;
>>
>> Newline here please. But can't we just do:
>>
>> return fill_uuid(regs, SSC_SMCCC_UID);
>>
>> This would make the code simpler.
> I'm not sure that I got this. fill_uuid() returns void, while 
> handle_sssc() returns bool.

You could make fill_uuid() returning a bool. Overall every caller of 
fill_uuid will do:

fill_uuid(....);
return true;

> 
>>> +        fill_uuid(regs, psci_uuid);
>>> +        return true;
>>> +    }
>>> +    case ARM_SMCCC_FUNC_CALL_REVISION:
>>> +        set_user_reg(regs, 0, SSSC_SMCCC_MAJOR_REVISION);
>>> +        set_user_reg(regs, 1, SSSC_SMCCC_MINOR_REVISION);
>>
>> Same here.
> Under "same" you meant newline, correct?

Hmmm, I don't remember :/. Although, we may want to introduce a helper 
to fill the revision. Those wrapper would simplify the implementation of 
"generic" function.

Cheers,

-- 
Julien Grall

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

Re: [PATCH v4 08/11] arm: PSCI: use definitions provided by asm/smccc.h

[Julien Grall]

On 25/08/17 12:00, Volodymyr Babchuk wrote:
> Hi,
> 
> On 24.08.17 20:22, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> smccc.h provides definitions to construct SMC call function number 
>>> according
>>> to SMCCC. We don't need multiple definitions for one thing, and 
>>> definitions
>>> in smccc.h are more generic than ones used in psci.h.
>>>
>>> So psci.h will only provide function codes, while whole SMC function
>>> identifier will be constructed using generic macros from smccc.h.
>>>
>>> PSCI_0_2_FN_xxx was deliberately renamed to PSCI_0_2_FUNC_xxx, 
>>> because this
>>> is a new entity. It can lead to problems, if we'll just change value of
>>> PSCI_0_2_FN_xxx without renaming it.
>>
>> I don't think "new entity" is a good reason to rename them. And the 
>> previous naming was kind of nice to read. More that you still use 
>> PSCI_0_2_FN{32,64}. We should definitely stay consistent in naming.
>>
>> So what is the exact problem? Is it because you are worry to miss some 
>> of them?
> Actually yes. That helped me to find references to PSCI code in seattle.c

But now you have inconsistent naming which is not better. I would prefer 
much prefer to keep the same name as before.

>>> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
>>> index 956d4ef..46a2fde 100644
>>> --- a/xen/arch/arm/vsmc.c
>>> +++ b/xen/arch/arm/vsmc.c
>>> @@ -102,7 +102,7 @@ static bool handle_psci_0_x(struct cpu_user_regs 
>>> *regs)
>>>  /* 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 ) );
>>> +    return is_64bit_domain(d) || !ARM_SMCCC_IS_64(fid);
>>
>> I don't see any reason to do the renaming in psci_mode_check given 
>> that you will remove this function in the next patch.
> Yep, but in comments for the previous series you asked me to make this code
> easier to read :)
> PSCI_0_2_64BIT is dropped now, so I had to use long definitions from 
> smccc.h

If you remove the code in this series, then there are no need to review 
it. You can just mention in the commit message why you didn't clean-up 
that function.

Cheers,

-- 
Julien Grall

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

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

[Volodymyr Babchuk]

Hi Julien,

On 24.08.17 18:00, Julien Grall wrote:
> Hi Volodymyr,
> 
> Title: No need for the full stop.
> 
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> This patch adds generic definitions used in ARM SMC call convention.
>> Those definitions was taken from linux header arm-smccc.h, extended
>> and formatted according to XEN coding style.
>>
>> They can be used by both SMCCC clients (like PSCI) and by SMCCC
>> servers (like vPSCI or upcoming generic SMCCC handler).
>>
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>>  xen/include/asm-arm/smccc.h | 92 
>> +++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 92 insertions(+)
>>  create mode 100644 xen/include/asm-arm/smccc.h
>>
>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>> new file mode 100644
>> index 0000000..67da3fb
>> --- /dev/null
>> +++ b/xen/include/asm-arm/smccc.h
>> @@ -0,0 +1,92 @@
>> +/*
>> + * Copyright (c) 2015, Linaro Limited
> 
> Linaro? Where does this code come from?
 From the linux kernel. I think, I mentioned that in previous comments.
But this code was extended by me. And now there will be more changes.
Should I add remark about code origin somewhere?

>> + * Copyright (c) 2017, EPAM Systems
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef __ASM_ARM_SMCCC_H__
>> +#define __ASM_ARM_SMCCC_H__
>> +
>> +/*
>> + * This file provides common defines for ARM SMC Calling Convention as
>> + * specified in
>> + * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
>> + */
>> +
>> +#define ARM_SMCCC_STD_CALL              0U
>> +#define ARM_SMCCC_FAST_CALL             1U
>> +#define ARM_SMCCC_TYPE_SHIFT            31
>> +
>> +#define ARM_SMCCC_SMC_32                0U
>> +#define ARM_SMCCC_SMC_64                1U
> 
> I am not sure to understand why you embed SMC in the name? The 
> convention is SMC32/HVC32. So I would name
> 
> ARM_SMCCC_CONV_{32,64}
> 
>> +#define ARM_SMCCC_CALL_CONV_SHIFT       30
> 
> Also, I would rename to ARM_SMCCC_CONV to fit with the value above.
> 
>> +
>> +#define ARM_SMCCC_OWNER_MASK            0x3F
> 
> Missing U.
> 
>> +#define ARM_SMCCC_OWNER_SHIFT           24
>> +
>> +#define ARM_SMCCC_FUNC_MASK             0xFFFF
>> +
>> +/* Check if this is fast call */
>> +#define ARM_SMCCC_IS_FAST_CALL(smc_val) \
>> +    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
>> +
>> +/* Check if this is 64 bit call  */
>> +#define 
>> ARM_SMCCC_IS_64(smc_val)                                        \
>> +    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
>> +
>> +/* Get function number from function identifier */
>> +#define ARM_SMCCC_FUNC_NUM(smc_val)     ((smc_val) & 
>> ARM_SMCCC_FUNC_MASK)
>> +
>> +/* Get service owner number from function identifier */
>> +#define 
>> ARM_SMCCC_OWNER_NUM(smc_val)                                    \
>> +    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
> 
> Can we please use static inline helper for the 4 macros above?
> 
>> +
>> +/*
>> + * Construct function identifier from call type (fast or standard),
>> + * calling convention (32 or 64 bit), service owner and function number
>> + */
>> +#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, 
>> func_num)   \
>> +    (((type) << ARM_SMCCC_TYPE_SHIFT) 
>> |                                 \
>> +         ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) 
>> |          \
>> +         (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) 
>> |  \
>> +         ((func_num) & ARM_SMCCC_FUNC_MASK))
> 
> The indentation looks wrong here. Also I think the (& *MASK) is not 
> necessary here. It would be wrong by the user to pass a wrong value and 
> even with the masking you would end up to wrong result.
> 
> If you are really worry of wrong result, then you should use 
> BUILD_BUG_ON()/BUG_ON().
> 
>> +
>> +/* List of known service owners */
>> +#define ARM_SMCCC_OWNER_ARCH            0
>> +#define ARM_SMCCC_OWNER_CPU             1
>> +#define ARM_SMCCC_OWNER_SIP             2
>> +#define ARM_SMCCC_OWNER_OEM             3
>> +#define ARM_SMCCC_OWNER_STANDARD        4
>> +#define ARM_SMCCC_OWNER_HYPERVISOR      5
>> +#define ARM_SMCCC_OWNER_TRUSTED_APP     48
>> +#define ARM_SMCCC_OWNER_TRUSTED_APP_END 49
>> +#define ARM_SMCCC_OWNER_TRUSTED_OS      50
>> +#define ARM_SMCCC_OWNER_TRUSTED_OS_END  63
>> +
>> +/* List of generic function numbers */
>> +#define ARM_SMCCC_FUNC_CALL_COUNT       0xFF00
>> +#define ARM_SMCCC_FUNC_CALL_UID         0xFF01
>> +#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
>> +
>> +/* Only one error code defined in SMCCC */
>> +#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION  (-1)
>> +
>> +#endif  /* __ASM_ARM_SMCCC_H__ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:b
>> + */
>>
> 
> Cheers,
> 

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 23.08.17 14:29, Jan Beulich wrote:
>>>> On 23.08.17 at 13:08, <volodymyr_babchuk@epam.com> wrote:
>> On 23.08.17 11:10, Jan Beulich wrote:
>>>>>> On 22.08.17 at 16:37, <volodymyr_babchuk@epam.com> wrote:
>>>> I can't see why you want to map UUID to a certain structure.
>>>
>>> This is so that the type cannot mistakenly be passed to a function
>>> taking unsigned char *, or be assigned to a variable of that type.
>> Right, I see the point there.
>>
>>> Please see our TYPE_SAFE() macro which we use to specifically
>>> enclose certain scalar types in a structure to that they won't be
>>> compatible with other types deriving from the same scalar base type.
>> I see. So what about
>>
>> struct xen_uuid_t
>> {
>>        uint8_t a[16];
>> };
>>
>> then?
> 
> Yes, that's what I had asked for as the minimal solution. That
> would be in line with (but better than) xen_domain_handle_t,
> which I've just realized we also have.
> 
>> One can convert it to union with different representations (array,
>> RFC4122 struct, etc) later if there will be need for this.
> 
> Well, why don't you make it a union but stick to just the array
> for now if you dislike making it similar to the EFI one? That way
> we can add further representations if needed/desired without
> breaking existing consumers.
My first intention was to declare union with all possible 
representations, so it would be possible to access the same UUID as an 
array of bytes or, for example, as Microsoft GUID. Like this:

typedef union {
     /* UUID represented as a 128-bit object */
     uint8_t obj[16];

     /* Representation according to RFC 4122 */
     struct {
         __be32  time_low;
         __be16  time_mid;
         __be16  time_hi_and_version;
         __u8    clock_seq_hi_and_reserved;
         __u8    clock_seq_low;
         __u8    node[6];
     } rfc4122;

     /* Microsoft/Intel style GUID representation */
     struct {
         __le32  Data1;
         __le16  Data2;
         __le16  Data3;
         __u8    Data4[8];
     } guid;

     /* SMCCC compatible format */
     struct {
         __le32 r0;
         __le32 r1;
         __le32 r2;
         __le32 r3;
     } smccc;
} xen_uuid_t;


But looks like we can't use something like __packed or 
__attribute__((__packed__)) in the public header. This means that we 
can't rely on right overlapping and users of this union should take care 
to read and write only to one chosen substructure. I think, this is 
error-prone, so it is better to stick to

typedef struct
{
        uint8_t a[16];
} xen_uuid_t;

BTW, I'm very interested how it can be guaranteed that structures 
defined in xen.h will have the same size and alignment on both sides of 
communication channel, taking into account, then we rely only on C89 
standard.

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
> My first intention was to declare union with all possible 
> representations, so it would be possible to access the same UUID as an 
> array of bytes or, for example, as Microsoft GUID. Like this:
> 
> typedef union {
>      /* UUID represented as a 128-bit object */
>      uint8_t obj[16];
> 
>      /* Representation according to RFC 4122 */
>      struct {
>          __be32  time_low;
>          __be16  time_mid;
>          __be16  time_hi_and_version;
>          __u8    clock_seq_hi_and_reserved;
>          __u8    clock_seq_low;
>          __u8    node[6];
>      } rfc4122;
> 
>      /* Microsoft/Intel style GUID representation */
>      struct {
>          __le32  Data1;
>          __le16  Data2;
>          __le16  Data3;
>          __u8    Data4[8];
>      } guid;
> 
>      /* SMCCC compatible format */
>      struct {
>          __le32 r0;
>          __le32 r1;
>          __le32 r2;
>          __le32 r3;
>      } smccc;
> } xen_uuid_t;
> 
> 
> But looks like we can't use something like __packed or 
> __attribute__((__packed__)) in the public header. This means that we 
> can't rely on right overlapping and users of this union should take care 
> to read and write only to one chosen substructure.

I don't see any use of that attribute in the structure definition
above, nor any need to add one - all fields are suitably aligned
anyway. You can't use __be* and __le* types in the public
headers, though - these will need to be uint*_t.

> BTW, I'm very interested how it can be guaranteed that structures 
> defined in xen.h will have the same size and alignment on both sides of 
> communication channel, taking into account, then we rely only on C89 
> standard.

I don't understand this comment.

Jan


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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Sergej Proskurin]

Hi Volodymyr, hi Julien,


On 08/24/2017 07:25 PM, Julien Grall wrote:
>
>
> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>> This feature indicates that hypervisor is compatible with ARM
>> SMC calling convention. Hypervisor will not inject an undefined
>> instruction exception if an invalid SMC function were called and
>> will not crash a domain if an invlalid HVC functions were called.
>
> s/invlalid/invalid/
>
> The last sentence is misleading. Xen will still inject and undefined
> instruction for some SMC/HVC. You may want to rework it to make it clear.
>

Now that you say that Xen will still inject an undefined instruction
exception for some SMCs, I have a to ask for which exactly?
I might be off topic here, so please tell me if you believe this is not
the right place for this question. In this case I will open an new
thread. Right now, I am working with the previous implementation of
do_trap_smc that was extended in this patch. Yet, as far as I
understand, the behavior should not change, which is why I am asking
this quesiton in this thread.

Currently, I am working on SMC guest injections and trying to understand
the resulting behavior. Every time, right after the execution of an
injected SMC instruction, the guest traps into the undefined instruction
exception handler in EL1 and I simply don't understand why. As far as I
understand, as soon an injected SMC instruction gets executed, it should
_transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
that now contains the original instruction instead of the injected SMC),
the guest should simply continue its execution.

Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
to EL2, that trap has priority over this disable". So this means that if
SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
execute an undefined instrcution exception. Which is what I was thinking
about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
the other hand I believe that it is highly unlikely that the EFI loader
explicitly disables SMC's for NS EL1. However, since I don't have access
to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
behavior I am experiencing on my board or not.

It would be of great help if you would provide me with some more clarity
on my case. I am sure that I have missed something that simply needs
clarification. Thank you very much in advance.

Thanks,
~Sergej


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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

Hi Jan,

On 31.08.17 10:34, Jan Beulich wrote:
>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>> My first intention was to declare union with all possible
>> representations, so it would be possible to access the same UUID as an
>> array of bytes or, for example, as Microsoft GUID. Like this:
>>
>> typedef union {
>>       /* UUID represented as a 128-bit object */
>>       uint8_t obj[16];
>>
>>       /* Representation according to RFC 4122 */
>>       struct {
>>           __be32  time_low;
>>           __be16  time_mid;
>>           __be16  time_hi_and_version;
>>           __u8    clock_seq_hi_and_reserved;
>>           __u8    clock_seq_low;
>>           __u8    node[6];
>>       } rfc4122;
>>
>>       /* Microsoft/Intel style GUID representation */
>>       struct {
>>           __le32  Data1;
>>           __le16  Data2;
>>           __le16  Data3;
>>           __u8    Data4[8];
>>       } guid;
>>
>>       /* SMCCC compatible format */
>>       struct {
>>           __le32 r0;
>>           __le32 r1;
>>           __le32 r2;
>>           __le32 r3;
>>       } smccc;
>> } xen_uuid_t;
>>
>>
>> But looks like we can't use something like __packed or
>> __attribute__((__packed__)) in the public header. This means that we
>> can't rely on right overlapping and users of this union should take care
>> to read and write only to one chosen substructure.
> 
> I don't see any use of that attribute in the structure definition
> above, nor any need to add one - all fields are suitably aligned
> anyway. You can't use __be* and __le* types in the public
> headers, though - these will need to be uint*_t.
This is a public header. As I understand it can be used by different 
compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to 
padding or alignment of fields in structures. No one can guarantee that

sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) == 
sizeof(xen_uuid_t.smccc)  == 16

On all platforms. Using any compiler. With any compiler options.

This is implementation defined ([1]). Standard says "This should present 
no problem unless binary data written by one implementation are read by 
another.". But in case of public headers, this structures can be written 
by one implementation and read by another.

>> BTW, I'm very interested how it can be guaranteed that structures
>> defined in xen.h will have the same size and alignment on both sides of
>> communication channel, taking into account, then we rely only on C89
>> standard.
> 
> I don't understand this comment.
See my reply above. There absolutely no guarantees, that MSVC compiler 
will not add extra padding somewhere. This will impair interoperability.
I think, this is why (amongst other reasons) WinAPI structures has 
dwSize as the first field. And this is why _IO* macros in Linux kernel 
use sizeof() to create ioctl number.
But, as I can see, XEN code checks only magic or version.

[1] http://port70.net/~nsz/c/c89/c89-draft.html#A.6.3.8

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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Volodymyr Babchuk]

Hello Sergej,

On 31.08.17 15:20, Sergej Proskurin wrote:
> Hi Volodymyr, hi Julien,
> 
> 
> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> This feature indicates that hypervisor is compatible with ARM
>>> SMC calling convention. Hypervisor will not inject an undefined
>>> instruction exception if an invalid SMC function were called and
>>> will not crash a domain if an invlalid HVC functions were called.
>>
>> s/invlalid/invalid/
>>
>> The last sentence is misleading. Xen will still inject and undefined
>> instruction for some SMC/HVC. You may want to rework it to make it clear.
>>
> 
> Now that you say that Xen will still inject an undefined instruction
> exception for some SMCs, I have a to ask for which exactly?
For ones that are compatible with ARM SMCCC [1]. E.g if you are running 
SMCCC-compatible system and you are calling SMC/HVC with immediate value 
0, then you are safe.

> I might be off topic here, so please tell me if you believe this is not
> the right place for this question. In this case I will open an new
> thread. Right now, I am working with the previous implementation of
> do_trap_smc that was extended in this patch. Yet, as far as I
> understand, the behavior should not change, which is why I am asking
> this quesiton in this thread.
If you are talking about forwarding SMC exception to VM monitor, then 
yes, that should not change.

> Currently, I am working on SMC guest injections and trying to understand
> the resulting behavior. Every time, right after the execution of an
> injected SMC instruction, the guest traps into the undefined instruction
> exception handler in EL1 and I simply don't understand why. As far as I
> understand, as soon an injected SMC instruction gets executed, it should
> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
> that now contains the original instruction instead of the injected SMC),
> the guest should simply continue its execution.
Hm. What do you mean under "SMC instruction injection?".
Current code in hypervisor will always inject undefined instruction 
exception when you  call SMC (unless you installed VM monitor for the 
guest). Also, it will not increase PC. So, if you'll try to remove 
inject_undef_exception() call, you'll get into an infinite loop.

> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
> to EL2, that trap has priority over this disable". So this means that if
> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
> execute an undefined instrcution exception. Which is what I was thinking
> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
> the other hand I believe that it is highly unlikely that the EFI loader
> explicitly disables SMC's for NS EL1. However, since I don't have access
> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
> behavior I am experiencing on my board or not.
According to ARM ARM, hypervisor should trap SMC even if was disabled by 
EL3. I think, that in your case the problem is in current implementation 
of do_trap_smc()

> It would be of great help if you would provide me with some more clarity
> on my case. I am sure that I have missed something that simply needs
> clarification. Thank you very much in advance.
I don't quite understood, what you are trying to achieve. But I think 
that pair of printk()s in do_trap_smc() will reveal much.


[1] 
http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 31.08.17 at 14:24, <volodymyr_babchuk@epam.com> wrote:
> Hi Jan,
> 
> On 31.08.17 10:34, Jan Beulich wrote:
>>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>>> My first intention was to declare union with all possible
>>> representations, so it would be possible to access the same UUID as an
>>> array of bytes or, for example, as Microsoft GUID. Like this:
>>>
>>> typedef union {
>>>       /* UUID represented as a 128-bit object */
>>>       uint8_t obj[16];
>>>
>>>       /* Representation according to RFC 4122 */
>>>       struct {
>>>           __be32  time_low;
>>>           __be16  time_mid;
>>>           __be16  time_hi_and_version;
>>>           __u8    clock_seq_hi_and_reserved;
>>>           __u8    clock_seq_low;
>>>           __u8    node[6];
>>>       } rfc4122;
>>>
>>>       /* Microsoft/Intel style GUID representation */
>>>       struct {
>>>           __le32  Data1;
>>>           __le16  Data2;
>>>           __le16  Data3;
>>>           __u8    Data4[8];
>>>       } guid;
>>>
>>>       /* SMCCC compatible format */
>>>       struct {
>>>           __le32 r0;
>>>           __le32 r1;
>>>           __le32 r2;
>>>           __le32 r3;
>>>       } smccc;
>>> } xen_uuid_t;
>>>
>>>
>>> But looks like we can't use something like __packed or
>>> __attribute__((__packed__)) in the public header. This means that we
>>> can't rely on right overlapping and users of this union should take care
>>> to read and write only to one chosen substructure.
>> 
>> I don't see any use of that attribute in the structure definition
>> above, nor any need to add one - all fields are suitably aligned
>> anyway. You can't use __be* and __le* types in the public
>> headers, though - these will need to be uint*_t.
> This is a public header. As I understand it can be used by different 
> compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to 
> padding or alignment of fields in structures. No one can guarantee that
> 
> sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) == 
> sizeof(xen_uuid_t.smccc)  == 16
> 
> On all platforms. Using any compiler. With any compiler options.
> 
> This is implementation defined ([1]). Standard says "This should present 
> no problem unless binary data written by one implementation are read by 
> another.". But in case of public headers, this structures can be written 
> by one implementation and read by another.

My reference to C89 was to tell you what language constructs
you're allowed to use. For binary layout, conventions also
matter (like gABI and processor specific ABIs). Without that we
wouldn't be able to write any C header in compatible manner.
What helps us greatly is that we're not needing interfaces for
cross-host communication - the entire public interface assumes
that producer and consumer run on the same physical system
(or, for the parts concerning migration, on similar ones).

Jan



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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Volodymyr Babchuk]

On 31.08.17 15:53, Jan Beulich wrote:
>>>> On 31.08.17 at 14:24, <volodymyr_babchuk@epam.com> wrote:
>> Hi Jan,
>>
>> On 31.08.17 10:34, Jan Beulich wrote:
>>>>>> On 30.08.17 at 18:20, <volodymyr_babchuk@epam.com> wrote:
>>>> My first intention was to declare union with all possible
>>>> representations, so it would be possible to access the same UUID as an
>>>> array of bytes or, for example, as Microsoft GUID. Like this:
>>>>
>>>> typedef union {
>>>>        /* UUID represented as a 128-bit object */
>>>>        uint8_t obj[16];
>>>>
>>>>        /* Representation according to RFC 4122 */
>>>>        struct {
>>>>            __be32  time_low;
>>>>            __be16  time_mid;
>>>>            __be16  time_hi_and_version;
>>>>            __u8    clock_seq_hi_and_reserved;
>>>>            __u8    clock_seq_low;
>>>>            __u8    node[6];
>>>>        } rfc4122;
>>>>
>>>>        /* Microsoft/Intel style GUID representation */
>>>>        struct {
>>>>            __le32  Data1;
>>>>            __le16  Data2;
>>>>            __le16  Data3;
>>>>            __u8    Data4[8];
>>>>        } guid;
>>>>
>>>>        /* SMCCC compatible format */
>>>>        struct {
>>>>            __le32 r0;
>>>>            __le32 r1;
>>>>            __le32 r2;
>>>>            __le32 r3;
>>>>        } smccc;
>>>> } xen_uuid_t;
>>>>
>>>>
>>>> But looks like we can't use something like __packed or
>>>> __attribute__((__packed__)) in the public header. This means that we
>>>> can't rely on right overlapping and users of this union should take care
>>>> to read and write only to one chosen substructure.
>>>
>>> I don't see any use of that attribute in the structure definition
>>> above, nor any need to add one - all fields are suitably aligned
>>> anyway. You can't use __be* and __le* types in the public
>>> headers, though - these will need to be uint*_t.
>> This is a public header. As I understand it can be used by different
>> compilers (gcc, icc, msvc, llvm, etc...). C89 have no restrictions to
>> padding or alignment of fields in structures. No one can guarantee that
>>
>> sizeof(xen_uuid_t.rfc422) == sizeof(xen_uuid_t.guid) ==
>> sizeof(xen_uuid_t.smccc)  == 16
>>
>> On all platforms. Using any compiler. With any compiler options.
>>
>> This is implementation defined ([1]). Standard says "This should present
>> no problem unless binary data written by one implementation are read by
>> another.". But in case of public headers, this structures can be written
>> by one implementation and read by another.
> 
> My reference to C89 was to tell you what language constructs
> you're allowed to use. For binary layout, conventions also
> matter (like gABI and processor specific ABIs). Without that we
> wouldn't be able to write any C header in compatible manner.
> What helps us greatly is that we're not needing interfaces for
> cross-host communication - the entire public interface assumes
> that producer and consumer run on the same physical system
> (or, for the parts concerning migration, on similar ones).

So, will it be acceptable to use my approach with that union?

Do you have any ideas how to indicate endianess of the fields, then? I 
can just write it in the comments. But I fear of misuse.


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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Sergej Proskurin]

Hi Volodymyr,


On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
> Hello Sergej,
>
> On 31.08.17 15:20, Sergej Proskurin wrote:
>> Hi Volodymyr, hi Julien,
>>
>>
>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>
>>>
>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>> This feature indicates that hypervisor is compatible with ARM
>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>> instruction exception if an invalid SMC function were called and
>>>> will not crash a domain if an invlalid HVC functions were called.
>>>
>>> s/invlalid/invalid/
>>>
>>> The last sentence is misleading. Xen will still inject and undefined
>>> instruction for some SMC/HVC. You may want to rework it to make it
>>> clear.
>>>
>>
>> Now that you say that Xen will still inject an undefined instruction
>> exception for some SMCs, I have a to ask for which exactly?
> For ones that are compatible with ARM SMCCC [1]. E.g if you are
> running SMCCC-compatible system and you are calling SMC/HVC with
> immediate value 0, then you are safe.
>

Alright, as far as I understand this is exactly what I do right now. I
inject an SMC that is encoded as 0xD4000003.

>> I might be off topic here, so please tell me if you believe this is not
>> the right place for this question. In this case I will open an new
>> thread. Right now, I am working with the previous implementation of
>> do_trap_smc that was extended in this patch. Yet, as far as I
>> understand, the behavior should not change, which is why I am asking
>> this quesiton in this thread.
> If you are talking about forwarding SMC exception to VM monitor, then
> yes, that should not change.

Yes, exactly. Sorry, I forgot to mention that I have a modified
xen-access version running in dom0 that registers an SMC monitor and
also increases the PC by 4 (or dependent on the case, simply leaves it
as it is) on every SMC trap.

>
>> Currently, I am working on SMC guest injections and trying to understand
>> the resulting behavior. Every time, right after the execution of an
>> injected SMC instruction, the guest traps into the undefined instruction
>> exception handler in EL1 and I simply don't understand why. As far as I
>> understand, as soon an injected SMC instruction gets executed, it should
>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>> that now contains the original instruction instead of the injected SMC),
>> the guest should simply continue its execution.
> Hm. What do you mean under "SMC instruction injection?".

My code runs in dom0 and "injects" an SMC instruction to predefined
addresses inside the guest as to simulate software breakpoints. By this,
I mean that the code replaces the original guest instruction at a
certain address with an SMC. Think of a debugger that uses software
breakpoints. The idea is to put back the original instruction right
after the SMC gets called, so that the guest can continue with its
execution. You can find more information about that in [0], yet please
consider that I try to trap the SMC directly in Xen instead of TrustZone.

> Current code in hypervisor will always inject undefined instruction
> exception when you  call SMC (unless you installed VM monitor for the
> guest). Also, it will not increase PC. So, if you'll try to remove
> inject_undef_exception() call, you'll get into an infinite loop.
>

I have a registered SMC monitor running in dom0 that does not reinject
the undefined instruction exception in do_trap_smc(). So there is no
indefinite loop at this point. What I see is that as soon as my code in
xen-access (dom0) increments the trapped guest PC by 4 (and also if it
doesn't) the next instruction inside the guest will be inside the undef
instruction handler (I can see that because I have implemented a single
stepping mechanism for AArch64 in Xen that gets activated right after
the guest executes the injected SMC instruction).

>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
>> to EL2, that trap has priority over this disable". So this means that if
>> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
>> execute an undefined instrcution exception. Which is what I was thinking
>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>> the other hand I believe that it is highly unlikely that the EFI loader
>> explicitly disables SMC's for NS EL1. However, since I don't have access
>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
>> behavior I am experiencing on my board or not.
> According to ARM ARM, hypervisor should trap SMC even if was disabled
> by EL3. I think, that in your case the problem is in current
> implementation of do_trap_smc()
>

Unfortunately, I don't think that this is the problem of do_trap_smc()
(see above). But let me check one more time.

>> It would be of great help if you would provide me with some more clarity
>> on my case. I am sure that I have missed something that simply needs
>> clarification. Thank you very much in advance.
> I don't quite understood, what you are trying to achieve. But I think
> that pair of printk()s in do_trap_smc() will reveal much.
>

Yea, the idea is to inject SMC instructions into the guest to simulate
software breakpoints for guest analysis purposes. Please let me cleanup
my current printk output to better present my issue.

>
> [1]
> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf

Thank you,
~Sergej

[0] http://www.cse.psu.edu/~trj1/papers/most14.pdf


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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Ian Jackson]

Volodymyr Babchuk writes ("Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling"):
> Do you have any ideas how to indicate endianess of the fields, then? I 
> can just write it in the comments. But I fear of misuse.

I definitely prefer your approach of providing only an array.  (It
needs to be in a struct for typesafety, as discussed.)  At least that
way the semantics are clear.

No-one is likely to want to access these individual fields.  If they
do so they are doing something very wrong.

Ian.

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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Volodymyr Babchuk]

Hi Sergej

On 31.08.17 16:51, Sergej Proskurin wrote:
> Hi Volodymyr,
> 
> 
> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
>> Hello Sergej,
>>
>> On 31.08.17 15:20, Sergej Proskurin wrote:
>>> Hi Volodymyr, hi Julien,
>>>
>>>
>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>>
>>>>
>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>>> This feature indicates that hypervisor is compatible with ARM
>>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>>> instruction exception if an invalid SMC function were called and
>>>>> will not crash a domain if an invlalid HVC functions were called.
>>>>
>>>> s/invlalid/invalid/
>>>>
>>>> The last sentence is misleading. Xen will still inject and undefined
>>>> instruction for some SMC/HVC. You may want to rework it to make it
>>>> clear.
>>>>
>>>
>>> Now that you say that Xen will still inject an undefined instruction
>>> exception for some SMCs, I have a to ask for which exactly?
>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
>> running SMCCC-compatible system and you are calling SMC/HVC with
>> immediate value 0, then you are safe.
>>
> 
> Alright, as far as I understand this is exactly what I do right now. I
> inject an SMC that is encoded as 0xD4000003.
Actually, this patch series are not merged yet, so no SMCCC support 
right. But this should not a problem in your case.

>>> I might be off topic here, so please tell me if you believe this is not
>>> the right place for this question. In this case I will open an new
>>> thread. Right now, I am working with the previous implementation of
>>> do_trap_smc that was extended in this patch. Yet, as far as I
>>> understand, the behavior should not change, which is why I am asking
>>> this quesiton in this thread.
>> If you are talking about forwarding SMC exception to VM monitor, then
>> yes, that should not change.
> 
> Yes, exactly. Sorry, I forgot to mention that I have a modified
> xen-access version running in dom0 that registers an SMC monitor and
> also increases the PC by 4 (or dependent on the case, simply leaves it
> as it is) on every SMC trap.
Aha, I see. I never was able to test this feature fully. I played with
my own VM monitor, when I tried to offload SMC handling to another 
domain. But I had to comment out most of the VM monitor code in XEN.

>>
>>> Currently, I am working on SMC guest injections and trying to understand
>>> the resulting behavior. Every time, right after the execution of an
>>> injected SMC instruction, the guest traps into the undefined instruction
>>> exception handler in EL1 and I simply don't understand why. As far as I
>>> understand, as soon an injected SMC instruction gets executed, it should
>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is set).
>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>>> that now contains the original instruction instead of the injected SMC),
>>> the guest should simply continue its execution.
>> Hm. What do you mean under "SMC instruction injection?".
> 
> My code runs in dom0 and "injects" an SMC instruction to predefined
> addresses inside the guest as to simulate software breakpoints. By this,
> I mean that the code replaces the original guest instruction at a
> certain address with an SMC. Think of a debugger that uses software
> breakpoints. The idea is to put back the original instruction right
> after the SMC gets called, so that the guest can continue with its
> execution. You can find more information about that in [0], yet please
> consider that I try to trap the SMC directly in Xen instead of TrustZone.
Yep, I see. Immediate question: do you flush icache after you put 
original instruction back? Then I can't see, why this should not work. 
If only VM monitor core in XEN is not broken. I don't know any users of 
this.
I'm just curious, why are you using SMC, not BRK instruction?

>> Current code in hypervisor will always inject undefined instruction
>> exception when you  call SMC (unless you installed VM monitor for the
>> guest). Also, it will not increase PC. So, if you'll try to remove
>> inject_undef_exception() call, you'll get into an infinite loop.
>>
> 
> I have a registered SMC monitor running in dom0 that does not reinject
> the undefined instruction exception in do_trap_smc(). So there is no
> indefinite loop at this point. What I see is that as soon as my code in
> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
> doesn't) the next instruction inside the guest will be inside the undef
> instruction handler (I can see that because I have implemented a single
> stepping mechanism for AArch64 in Xen that gets activated right after
> the guest executes the injected SMC instruction).
That's strange. Can you print whole vCPU state to determine that PC 
points to the right place? Also you can check DFAR. Probably you can 
even dump memory pointed by DFAR to make sure that you written back 
correct instruction.

>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC instructions
>>> to EL2, that trap has priority over this disable". So this means that if
>>> SMCs are disabled for NS EL1, the guest will trap into the hypervisor on
>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest will
>>> execute an undefined instrcution exception. Which is what I was thinking
>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>>> the other hand I believe that it is highly unlikely that the EFI loader
>>> explicitly disables SMC's for NS EL1. However, since I don't have access
>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason for the
>>> behavior I am experiencing on my board or not.
>> According to ARM ARM, hypervisor should trap SMC even if was disabled
>> by EL3. I think, that in your case the problem is in current
>> implementation of do_trap_smc()
>>
> 
> Unfortunately, I don't think that this is the problem of do_trap_smc()
> (see above). But let me check one more time.
As I said, I know no users for SMC monitor, and I'm not exactly sure 
that it works properly.

> 
>>> It would be of great help if you would provide me with some more clarity
>>> on my case. I am sure that I have missed something that simply needs
>>> clarification. Thank you very much in advance.
>> I don't quite understood, what you are trying to achieve. But I think
>> that pair of printk()s in do_trap_smc() will reveal much.
>>
> 
> Yea, the idea is to inject SMC instructions into the guest to simulate
> software breakpoints for guest analysis purposes. Please let me cleanup
> my current printk output to better present my issue.
> 
>>
>> [1]
>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
> 
> Thank you,
> ~Sergej
> 
> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
> 

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

Re: [PATCH v4 03/11] public: xen.h: add definitions for UUID handling

[Jan Beulich]

>>> On 31.08.17 at 15:21, <volodymyr_babchuk@epam.com> wrote:
> So, will it be acceptable to use my approach with that union?

As per Ian's reply, go with just the containerized uint8_t[].

Jan


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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Sergej Proskurin]

Hi Volodymyr,


On 08/31/2017 04:58 PM, Volodymyr Babchuk wrote:
> Hi Sergej
>
> On 31.08.17 16:51, Sergej Proskurin wrote:
>> Hi Volodymyr,
>>
>>
>> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
>>> Hello Sergej,
>>>
>>> On 31.08.17 15:20, Sergej Proskurin wrote:
>>>> Hi Volodymyr, hi Julien,
>>>>
>>>>
>>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
>>>>>
>>>>>
>>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>>>>> This feature indicates that hypervisor is compatible with ARM
>>>>>> SMC calling convention. Hypervisor will not inject an undefined
>>>>>> instruction exception if an invalid SMC function were called and
>>>>>> will not crash a domain if an invlalid HVC functions were called.
>>>>>
>>>>> s/invlalid/invalid/
>>>>>
>>>>> The last sentence is misleading. Xen will still inject and undefined
>>>>> instruction for some SMC/HVC. You may want to rework it to make it
>>>>> clear.
>>>>>
>>>>
>>>> Now that you say that Xen will still inject an undefined instruction
>>>> exception for some SMCs, I have a to ask for which exactly?
>>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
>>> running SMCCC-compatible system and you are calling SMC/HVC with
>>> immediate value 0, then you are safe.
>>>
>>
>> Alright, as far as I understand this is exactly what I do right now. I
>> inject an SMC that is encoded as 0xD4000003.
> Actually, this patch series are not merged yet, so no SMCCC support
> right. But this should not a problem in your case.
>
>>>> I might be off topic here, so please tell me if you believe this is
>>>> not
>>>> the right place for this question. In this case I will open an new
>>>> thread. Right now, I am working with the previous implementation of
>>>> do_trap_smc that was extended in this patch. Yet, as far as I
>>>> understand, the behavior should not change, which is why I am asking
>>>> this quesiton in this thread.
>>> If you are talking about forwarding SMC exception to VM monitor, then
>>> yes, that should not change.
>>
>> Yes, exactly. Sorry, I forgot to mention that I have a modified
>> xen-access version running in dom0 that registers an SMC monitor and
>> also increases the PC by 4 (or dependent on the case, simply leaves it
>> as it is) on every SMC trap.
> Aha, I see. I never was able to test this feature fully. I played with
> my own VM monitor, when I tried to offload SMC handling to another
> domain. But I had to comment out most of the VM monitor code in XEN.
>
>>>
>>>> Currently, I am working on SMC guest injections and trying to
>>>> understand
>>>> the resulting behavior. Every time, right after the execution of an
>>>> injected SMC instruction, the guest traps into the undefined
>>>> instruction
>>>> exception handler in EL1 and I simply don't understand why. As far
>>>> as I
>>>> understand, as soon an injected SMC instruction gets executed, it
>>>> should
>>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is
>>>> set).
>>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
>>>> that now contains the original instruction instead of the injected
>>>> SMC),
>>>> the guest should simply continue its execution.
>>> Hm. What do you mean under "SMC instruction injection?".
>>
>> My code runs in dom0 and "injects" an SMC instruction to predefined
>> addresses inside the guest as to simulate software breakpoints. By this,
>> I mean that the code replaces the original guest instruction at a
>> certain address with an SMC. Think of a debugger that uses software
>> breakpoints. The idea is to put back the original instruction right
>> after the SMC gets called, so that the guest can continue with its
>> execution. You can find more information about that in [0], yet please
>> consider that I try to trap the SMC directly in Xen instead of
>> TrustZone.
> Yep, I see. Immediate question: do you flush icache after you put
> original instruction back? 

Yeap. But the current behavior does not let me to go this far, as I the
system jumps into the interrupt handler and single-steps the handler
instead of the instruction of interest.

> Then I can't see, why this should not work. If only VM monitor core in
> XEN is not broken. I don't know any users of this.
> I'm just curious, why are you using SMC, not BRK instruction?
>

I use SMC instructions as the guest can register for BRK events. The
guest cannot register for SMC events. So, in order stay stealthy towards
the guest and also not to cope with BRK re-injections, SMC's seemed to
be the right choice :)

>>> Current code in hypervisor will always inject undefined instruction
>>> exception when you  call SMC (unless you installed VM monitor for the
>>> guest). Also, it will not increase PC. So, if you'll try to remove
>>> inject_undef_exception() call, you'll get into an infinite loop.
>>>
>>
>> I have a registered SMC monitor running in dom0 that does not reinject
>> the undefined instruction exception in do_trap_smc(). So there is no
>> indefinite loop at this point. What I see is that as soon as my code in
>> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
>> doesn't) the next instruction inside the guest will be inside the undef
>> instruction handler (I can see that because I have implemented a single
>> stepping mechanism for AArch64 in Xen that gets activated right after
>> the guest executes the injected SMC instruction).
> That's strange. Can you print whole vCPU state to determine that PC
> points to the right place? Also you can check DFAR. Probably you can
> even dump memory pointed by DFAR to make sure that you written back
> correct instruction.

Yea, I do that. And both the SMC injection, as well as further vCPU
state seems to be correct at this point.

Today, I saw an interesting behavior in my single-stepping
implementation, which is the reason for my late reply. I can't explain
what is going wrong, yet. So I will need to further investigate this
behavior and post and RFC for the single-stepping mechanism as to put
more eyes on the issue. Maybe, this will help solve it.

But anyway, thank you very much for your help! I really appreciate it :)

>
>>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
>>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC
>>>> instructions
>>>> to EL2, that trap has priority over this disable". So this means
>>>> that if
>>>> SMCs are disabled for NS EL1, the guest will trap into the
>>>> hypervisor on
>>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest
>>>> will
>>>> execute an undefined instrcution exception. Which is what I was
>>>> thinking
>>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
>>>> the other hand I believe that it is highly unlikely that the EFI
>>>> loader
>>>> explicitly disables SMC's for NS EL1. However, since I don't have
>>>> access
>>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason
>>>> for the
>>>> behavior I am experiencing on my board or not.
>>> According to ARM ARM, hypervisor should trap SMC even if was disabled
>>> by EL3. I think, that in your case the problem is in current
>>> implementation of do_trap_smc()
>>>
>>
>> Unfortunately, I don't think that this is the problem of do_trap_smc()
>> (see above). But let me check one more time.
> As I said, I know no users for SMC monitor, and I'm not exactly sure
> that it works properly.
>
>>
>>>> It would be of great help if you would provide me with some more
>>>> clarity
>>>> on my case. I am sure that I have missed something that simply needs
>>>> clarification. Thank you very much in advance.
>>> I don't quite understood, what you are trying to achieve. But I think
>>> that pair of printk()s in do_trap_smc() will reveal much.
>>>
>>
>> Yea, the idea is to inject SMC instructions into the guest to simulate
>> software breakpoints for guest analysis purposes. Please let me cleanup
>> my current printk output to better present my issue.
>>
>>>
>>> [1]
>>> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
>>>
>>
>> Thank you,
>> ~Sergej
>>
>> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
>>

Thanks,
~Sergej


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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Julien Grall]

[-- Attachment #1.1: Type: text/plain, Size: 9480 bytes --]

Hello,

Sorry for the formatting, writing from my phone. Ki

On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:

> Hi Volodymyr,
>
>
> On 08/31/2017 04:58 PM, Volodymyr Babchuk wrote:
> > Hi Sergej
> >
> > On 31.08.17 16:51, Sergej Proskurin wrote:
> >> Hi Volodymyr,
> >>
> >>
> >> On 08/31/2017 02:44 PM, Volodymyr Babchuk wrote:
> >>> Hello Sergej,
> >>>
> >>> On 31.08.17 15:20, Sergej Proskurin wrote:
> >>>> Hi Volodymyr, hi Julien,
> >>>>
> >>>>
> >>>> On 08/24/2017 07:25 PM, Julien Grall wrote:
> >>>>>
> >>>>>
> >>>>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
> >>>>>> This feature indicates that hypervisor is compatible with ARM
> >>>>>> SMC calling convention. Hypervisor will not inject an undefined
> >>>>>> instruction exception if an invalid SMC function were called and
> >>>>>> will not crash a domain if an invlalid HVC functions were called.
> >>>>>
> >>>>> s/invlalid/invalid/
> >>>>>
> >>>>> The last sentence is misleading. Xen will still inject and undefined
> >>>>> instruction for some SMC/HVC. You may want to rework it to make it
> >>>>> clear.
> >>>>>
> >>>>
> >>>> Now that you say that Xen will still inject an undefined instruction
> >>>> exception for some SMCs, I have a to ask for which exactly?
> >>> For ones that are compatible with ARM SMCCC [1]. E.g if you are
> >>> running SMCCC-compatible system and you are calling SMC/HVC with
> >>> immediate value 0, then you are safe.
> >>>
> >>
> >> Alright, as far as I understand this is exactly what I do right now. I
> >> inject an SMC that is encoded as 0xD4000003.
> > Actually, this patch series are not merged yet, so no SMCCC support
> > right. But this should not a problem in your case.
> >
> >>>> I might be off topic here, so please tell me if you believe this is
> >>>> not
> >>>> the right place for this question. In this case I will open an new
> >>>> thread. Right now, I am working with the previous implementation of
> >>>> do_trap_smc that was extended in this patch. Yet, as far as I
> >>>> understand, the behavior should not change, which is why I am asking
> >>>> this quesiton in this thread.
> >>> If you are talking about forwarding SMC exception to VM monitor, then
> >>> yes, that should not change.
> >>
> >> Yes, exactly. Sorry, I forgot to mention that I have a modified
> >> xen-access version running in dom0 that registers an SMC monitor and
> >> also increases the PC by 4 (or dependent on the case, simply leaves it
> >> as it is) on every SMC trap.
> > Aha, I see. I never was able to test this feature fully. I played with
> > my own VM monitor, when I tried to offload SMC handling to another
> > domain. But I had to comment out most of the VM monitor code in XEN.
> >
> >>>
> >>>> Currently, I am working on SMC guest injections and trying to
> >>>> understand
> >>>> the resulting behavior. Every time, right after the execution of an
> >>>> injected SMC instruction, the guest traps into the undefined
> >>>> instruction
> >>>> exception handler in EL1 and I simply don't understand why. As far
> >>>> as I
> >>>> understand, as soon an injected SMC instruction gets executed, it
> >>>> should
> >>>> _transparently_ trap into the hypervisor (assuming MDCR_EL2.TDE is
> >>>> set).
> >>>> As soon as the hypervisor returns (e.g. to PC+4 or to the trapping PC
> >>>> that now contains the original instruction instead of the injected
> >>>> SMC),
> >>>> the guest should simply continue its execution.
> >>> Hm. What do you mean under "SMC instruction injection?".
> >>
> >> My code runs in dom0 and "injects" an SMC instruction to predefined
> >> addresses inside the guest as to simulate software breakpoints. By this,
> >> I mean that the code replaces the original guest instruction at a
> >> certain address with an SMC. Think of a debugger that uses software
> >> breakpoints. The idea is to put back the original instruction right
> >> after the SMC gets called, so that the guest can continue with its
> >> execution. You can find more information about that in [0], yet please
> >> consider that I try to trap the SMC directly in Xen instead of
> >> TrustZone.
> > Yep, I see. Immediate question: do you flush icache after you put
> > original instruction back?
>
> Yeap. But the current behavior does not let me to go this far, as I the
> system jumps into the interrupt handler and single-steps the handler
> instead of the instruction of interest.


On your first mail, you started with "smc injection doesn't work", then "I
replace instruction" and now you mention about single-stepping.

This doesn't help at all to understand what you are doing and really not
related to this thread.

So can you please details exactly what you are doing rather than giving
bits by bits?


> > Then I can't see, why this should not work. If only VM monitor core in
> > XEN is not broken. I don't know any users of this.
> > I'm just curious, why are you using SMC, not BRK instruction?
> >
>
> I use SMC instructions as the guest can register for BRK events. The
> guest cannot register for SMC events. So, in order stay stealthy towards
> the guest and also not to cope with BRK re-injections, SMC's seemed to
> be the right choice :


I have already said that using SMC is a pretty bad idea when Tamas added
the trapping and you guys still seem to think it is a good idea...


> >>> Current code in hypervisor will always inject undefined instruction
> >>> exception when you  call SMC (unless you installed VM monitor for the
> >>> guest). Also, it will not increase PC. So, if you'll try to remove
> >>> inject_undef_exception() call, you'll get into an infinite loop.
> >>>
> >>
> >> I have a registered SMC monitor running in dom0 that does not reinject
> >> the undefined instruction exception in do_trap_smc(). So there is no
> >> indefinite loop at this point. What I see is that as soon as my code in
> >> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
> >> doesn't) the next instruction inside the guest will be inside the undef
> >> instruction handler (I can see that because I have implemented a single
> >> stepping mechanism for AArch64 in Xen that gets activated right after
> >> the guest executes the injected SMC instruction).
> > That's strange. Can you print whole vCPU state to determine that PC
> > points to the right place? Also you can check DFAR. Probably you can
> > even dump memory pointed by DFAR to make sure that you written back
> > correct instruction.
>
> Yea, I do that. And both the SMC injection, as well as further vCPU
> state seems to be correct at this point.
>
> Today, I saw an interesting behavior in my single-stepping
> implementation, which is the reason for my late reply. I can't explain
> what is going wrong, yet. So I will need to further investigate this
> behavior and post and RFC for the single-stepping mechanism as to put
> more eyes on the issue. Maybe, this will help solve it.
>
> But anyway, thank you very much for your help! I really appreciate it :)
>

You probably want to look at
https://lists.xen.org/archives/html/xen-devel/2017-08/msg00661.html and
maybe sync-up with this person if you are not working with him.

Cheers,


> >
> >>>> Now, according to ARM DDI0487B.a D1-1873, the following holds: "If
> >>>> HCR_EL2.TSC or HCR.TSC traps attempted EL1 execution of SMC
> >>>> instructions
> >>>> to EL2, that trap has priority over this disable". So this means
> >>>> that if
> >>>> SMCs are disabled for NS EL1, the guest will trap into the
> >>>> hypervisor on
> >>>> SMC execution. Yet, since SMCs are disabled from NS EL1, the guest
> >>>> will
> >>>> execute an undefined instrcution exception. Which is what I was
> >>>> thinking
> >>>> about is currently happening on my ARMv8 dev board (Lemaker Hikey). On
> >>>> the other hand I believe that it is highly unlikely that the EFI
> >>>> loader
> >>>> explicitly disables SMC's for NS EL1. However, since I don't have
> >>>> access
> >>>> to SCR_EL3.SMD from EL2, I can't tell whether this is the reason
> >>>> for the
> >>>> behavior I am experiencing on my board or not.
> >>> According to ARM ARM, hypervisor should trap SMC even if was disabled
> >>> by EL3. I think, that in your case the problem is in current
> >>> implementation of do_trap_smc()
> >>>
> >>
> >> Unfortunately, I don't think that this is the problem of do_trap_smc()
> >> (see above). But let me check one more time.
> > As I said, I know no users for SMC monitor, and I'm not exactly sure
> > that it works properly.
> >
> >>
> >>>> It would be of great help if you would provide me with some more
> >>>> clarity
> >>>> on my case. I am sure that I have missed something that simply needs
> >>>> clarification. Thank you very much in advance.
> >>> I don't quite understood, what you are trying to achieve. But I think
> >>> that pair of printk()s in do_trap_smc() will reveal much.
> >>>
> >>
> >> Yea, the idea is to inject SMC instructions into the guest to simulate
> >> software breakpoints for guest analysis purposes. Please let me cleanup
> >> my current printk output to better present my issue.
> >>
> >>>
> >>> [1]
> >>>
> http://infocenter.arm.com/help/topic/com.arm.doc.den0028b/ARM_DEN0028B_SMC_Calling_Convention.pdf
> >>>
> >>
> >> Thank you,
> >> ~Sergej
> >>
> >> [0] http://www.cse.psu.edu/~trj1/papers/most14.pdf
> >>
>
> Thanks,
> ~Sergej
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>

[-- Attachment #1.2: Type: text/html, Size: 13138 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Sergej Proskurin]

Hi Julien,


On 09/04/2017 08:07 AM, Julien Grall wrote:
> Hello,
>
> Sorry for the formatting, writing from my phone. Ki
>
> On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
>

[...]

>
> On your first mail, you started with "smc injection doesn't work", then "I
> replace instruction" and now you mention about single-stepping.
>
> This doesn't help at all to understand what you are doing and really not
> related to this thread.
>
> So can you please details exactly what you are doing rather than giving
> bits by bits?
>

I will provide more information in a separate thread soon so that the
actual issue, hopefully, will become clearer. Thank you.

>> I use SMC instructions as the guest can register for BRK events. The
>> guest cannot register for SMC events. So, in order stay stealthy towards
>> the guest and also not to cope with BRK re-injections, SMC's seemed to
>> be the right choice :
>
> I have already said that using SMC is a pretty bad idea when Tamas added
> the trapping and you guys still seem to think it is a good idea...

I did not know about this conversation with Tamas. Why do you believe
that using SMC instructions is not a good idea? Could you please refer
me to the particular thread? Thank you.

>>>>> Current code in hypervisor will always inject undefined instruction
>>>>> exception when you  call SMC (unless you installed VM monitor for the
>>>>> guest). Also, it will not increase PC. So, if you'll try to remove
>>>>> inject_undef_exception() call, you'll get into an infinite loop.
>>>>>
>>>> I have a registered SMC monitor running in dom0 that does not reinject
>>>> the undefined instruction exception in do_trap_smc(). So there is no
>>>> indefinite loop at this point. What I see is that as soon as my code in
>>>> xen-access (dom0) increments the trapped guest PC by 4 (and also if it
>>>> doesn't) the next instruction inside the guest will be inside the undef
>>>> instruction handler (I can see that because I have implemented a single
>>>> stepping mechanism for AArch64 in Xen that gets activated right after
>>>> the guest executes the injected SMC instruction).
>>> That's strange. Can you print whole vCPU state to determine that PC
>>> points to the right place? Also you can check DFAR. Probably you can
>>> even dump memory pointed by DFAR to make sure that you written back
>>> correct instruction.
>> Yea, I do that. And both the SMC injection, as well as further vCPU
>> state seems to be correct at this point.
>>
>> Today, I saw an interesting behavior in my single-stepping
>> implementation, which is the reason for my late reply. I can't explain
>> what is going wrong, yet. So I will need to further investigate this
>> behavior and post and RFC for the single-stepping mechanism as to put
>> more eyes on the issue. Maybe, this will help solve it.
>>
>> But anyway, thank you very much for your help! I really appreciate it :)
>>
> You probably want to look at
> https://lists.xen.org/archives/html/xen-devel/2017-08/msg00661.html and
> maybe sync-up with this person if you are not working with him.

Thanks, for mentioning that. Florian is a student of mine who has also
looked at single-stepping on ARMv8. We have collaborated on this topic
together. I will take over on that, as his work goes slightly into a
different direction.

Thanks,
~Sergej


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

Re: [PATCH v4 10/11] public: add XENFEAT_ARM_SMCCC_supported feature

[Julien Grall]

On 04/09/17 10:57, Sergej Proskurin wrote:
> Hi Julien,
> 
> 
> On 09/04/2017 08:07 AM, Julien Grall wrote:
>> Hello,
>>
>> Sorry for the formatting, writing from my phone. Ki
>>
>> On Thu, 31 Aug 2017, 22:18 Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
>>
> 
> [...]
> 
>>
>> On your first mail, you started with "smc injection doesn't work", then "I
>> replace instruction" and now you mention about single-stepping.
>>
>> This doesn't help at all to understand what you are doing and really not
>> related to this thread.
>>
>> So can you please details exactly what you are doing rather than giving
>> bits by bits?
>>
> 
> I will provide more information in a separate thread soon so that the
> actual issue, hopefully, will become clearer. Thank you.
> 
>>> I use SMC instructions as the guest can register for BRK events. The
>>> guest cannot register for SMC events. So, in order stay stealthy towards
>>> the guest and also not to cope with BRK re-injections, SMC's seemed to
>>> be the right choice :
>>
>> I have already said that using SMC is a pretty bad idea when Tamas added
>> the trapping and you guys still seem to think it is a good idea...
> 
> I did not know about this conversation with Tamas. Why do you believe
> that using SMC instructions is not a good idea? Could you please refer
> me to the particular thread? Thank you.

I am not sure on which thread it was discussed. 
https://lists.gt.net/xen/devel/427459 may contain some details.

By definition SMC is for Secure Monitor Call. It might be possible to 
have a guest access the secure firmware (e.g imagine an Android guest 
doing video decoding). Given that you don't identify the immediate of 
the SMC (which is BTW not easily available on ARMv7), you cannot have 
both interacting together. And even with that you don't know if the SMC 
#imm might be used by the firmware...

Furthermore, SMC cannot be executed at EL0 so you can't monitor 
application. They also won't be available if the platform that does not 
have EL3.

So yes SMC is a pretty bad choice here if you want to get a generic 
solution.

Cheers,

-- 
Julien Grall

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

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

[Julien Grall]

On 08/28/2017 09:28 PM, Volodymyr Babchuk wrote:
> Hi Julien,

Hi,

> 
> On 24.08.17 18:00, Julien Grall wrote:
>> Hi Volodymyr,
>>
>> Title: No need for the full stop.
>>
>> On 21/08/17 21:27, Volodymyr Babchuk wrote:
>>> This patch adds generic definitions used in ARM SMC call convention.
>>> Those definitions was taken from linux header arm-smccc.h, extended
>>> and formatted according to XEN coding style.
>>>
>>> They can be used by both SMCCC clients (like PSCI) and by SMCCC
>>> servers (like vPSCI or upcoming generic SMCCC handler).
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>  xen/include/asm-arm/smccc.h | 92 
>>> +++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 92 insertions(+)
>>>  create mode 100644 xen/include/asm-arm/smccc.h
>>>
>>> diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
>>> new file mode 100644
>>> index 0000000..67da3fb
>>> --- /dev/null
>>> +++ b/xen/include/asm-arm/smccc.h
>>> @@ -0,0 +1,92 @@
>>> +/*
>>> + * Copyright (c) 2015, Linaro Limited
>>
>> Linaro? Where does this code come from?
>  From the linux kernel. I think, I mentioned that in previous comments.
> But this code was extended by me. And now there will be more changes.
> Should I add remark about code origin somewhere?

Yes, the origin of code (repo + commit ID) + changes you made at least 
in the commit message.

Cheers,

-- 
Julien Grall

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