* [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC
@ 2017-08-31 20:09 Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
` (9 more replies)
0 siblings, 10 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Hello all,
v5:
* Patches that add end enable XENFEAT_ARM_SMCCC_supported were
squashed together
* All other chages are described in corresponding patches
This patch series still depend on Julien's patches for traps.c cleanup ([1]).
---
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 (10):
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 and enable XENFEAT_ARM_SMCCC_supported feature
xen/arch/arm/Makefile | 1 +
xen/arch/arm/platforms/seattle.c | 4 +-
xen/arch/arm/psci.c | 10 +-
xen/arch/arm/traps.c | 132 +-------------
xen/arch/arm/vsmc.c | 339 ++++++++++++++++++++++++++++++++++++
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 | 105 +++++++++++
xen/include/asm-arm/traps.h | 4 +
xen/include/public/arch-arm/smccc.h | 66 +++++++
xen/include/public/features.h | 3 +
xen/include/public/xen.h | 13 ++
13 files changed, 566 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/public/arch-arm/smccc.h
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 9:59 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 02/10] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
` (8 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
There are standard functions set_user_reg() and get_user_reg(). We can
use them in PSCI_SET_RESULT()/PSCI_ARG() macros instead of relying on
CONFIG_ARM_64 definition.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
* removed 0xFFFFFFFF mask
* coding style left unchanged, because it will be fixed in next patches
* spelling fixes
---
xen/arch/arm/traps.c | 38 +++++++++++++++++---------------------
1 file changed, 17 insertions(+), 21 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 13efb58..4569c62 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,13 +1450,12 @@ 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)
#else
-#define PSCI_RESULT_REG(reg) (reg)->r0
-#define PSCI_ARG(reg,n) (reg)->r##n
#define PSCI_ARG32(reg,n) PSCI_ARG(reg,n)
#endif
@@ -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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 02/10] arm: traps: check if SMC was conditional before handling it
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 03/10] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
` (7 subsequent siblings)
9 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
Trapped SMC instruction can fail condition check on ARMv8 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>
---
* added Julien's R-b tag
---
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 4569c62..9132fe1 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 03/10] public: xen.h: add definitions for UUID handling
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 02/10] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-01 9:42 ` Ian Jackson
2017-08-31 20:09 ` [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
` (6 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, 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>
---
* Array was wrapped into a structure
---
xen/include/public/xen.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 2ac6b1e..3dc81e3 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -930,6 +930,19 @@ __DEFINE_XEN_GUEST_HANDLE(uint16, uint16_t);
__DEFINE_XEN_GUEST_HANDLE(uint32, uint32_t);
__DEFINE_XEN_GUEST_HANDLE(uint64, uint64_t);
+typedef struct
+{
+ uint8_t a[16];
+} xen_uuid_t;
+
+#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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (2 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 03/10] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 10:02 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 05/10] arm: add SMCCC protocol definitions Volodymyr Babchuk
` (5 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, Volodymyr Babchuk
This patch define HSR_XXC_IMM_MASK. It can be used to extract
immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64
instructions, as described in the ARM ARM
(ARM DDI 0487B.a pages D7-2270, D7-2272).
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
* spelling fixes
---
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 05/10] arm: add SMCCC protocol definitions
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (3 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 10:07 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
` (4 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, 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. Some of the macros were
converted to inlined functions to ease parsing.
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>
---
* Accessor macros were converted to inlined functions
* ARM_SMCCC_SMC_{32,64} renamed to ARM_SMCCC_CONV_{32,64}
* Fixed indentation for ARM_SMCCC_CALL_VAL
---
xen/include/asm-arm/smccc.h | 105 ++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 105 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..f543dea
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,105 @@
+/*
+ * 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_CONV_32 0U
+#define ARM_SMCCC_CONV_64 1U
+#define ARM_SMCCC_CONV_SHIFT 30
+
+#define ARM_SMCCC_OWNER_MASK 0x3FU
+#define ARM_SMCCC_OWNER_SHIFT 24
+
+#define ARM_SMCCC_FUNC_MASK 0xFFFFU
+
+/* Check if this is fast call. */
+static inline bool smccc_is_fast_call(register_t funcid)
+{
+ return funcid & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT);
+}
+
+/* Chek if this is 64-bit call. */
+static inline bool smccc_is_conv_64(register_t funcid)
+{
+ return funcid & (ARM_SMCCC_CONV_64 << ARM_SMCCC_CONV_SHIFT);
+}
+
+/* Get function number from function identifier. */
+static inline uint32_t smccc_get_fn(register_t funcid)
+{
+ return funcid & ARM_SMCCC_FUNC_MASK;
+}
+
+/* Get service owner number from function identifier. */
+static inline uint32_t smccc_get_owner(register_t funcid)
+{
+ return (funcid >> 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_CONV_SHIFT) | \
+ (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) | \
+ (func_num))
+
+/* 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)
+
+/* 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__ */
+
+/*
+ * 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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (4 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 05/10] arm: add SMCCC protocol definitions Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 10:17 ` Julien Grall
2017-09-13 11:11 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
` (3 subsequent siblings)
9 siblings, 2 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, 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 fill_uuid() function
* dropped vsmc.h header. Function prototypes moved to traps.h
* public/arch-arm/smc.h header renamed to smccc.h
* introduced `register_t funcid` in vsmccc_handle_call()x
* spelling fixes
* coding style fixes
---
xen/arch/arm/Makefile | 1 +
xen/arch/arm/traps.c | 17 ----
xen/arch/arm/vsmc.c | 168 ++++++++++++++++++++++++++++++++++++
xen/include/asm-arm/traps.h | 3 +
xen/include/public/arch-arm/smccc.h | 58 +++++++++++++
5 files changed, 230 insertions(+), 17 deletions(-)
create mode 100644 xen/arch/arm/vsmc.c
create mode 100644 xen/include/public/arch-arm/smccc.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 9132fe1..f3b64b4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -2155,23 +2155,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..97a6be3
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,168 @@
+/*
+ * 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/smccc.h>
+#include <asm/monitor.h>
+#include <asm/regs.h>
+#include <asm/smccc.h>
+#include <asm/traps.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)
+{
+ int n;
+
+ /*
+ * UUIDs are returned in registers r0..r3, four bytes per register,
+ * first byte is stored in low-order bits of a register.
+ * (ARM DEN 0028B page 14)
+ */
+ for (n = 0; n < 4; n++)
+ {
+ const uint8_t *bytes = u->a + n * 4;
+ uint32_t r;
+
+ r = bytes[0];
+ r |= bytes[1] << 8;
+ r |= bytes[2] << 16;
+ r |= bytes[3] << 24;
+
+ set_user_reg(regs, n, r);
+ }
+}
+
+/* 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 ( smccc_get_fn(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;
+ }
+}
+
+/*
+ * vsmccc_handle_call() - handle SMC/HVC call according to ARM SMCCC.
+ * returns true if that was valid SMCCC call (even if function number
+ * was unknown).
+ */
+static bool vsmccc_handle_call(struct cpu_user_regs *regs)
+{
+ bool handled = false;
+ const union hsr hsr = { .bits = regs->hsr };
+ register_t funcid = get_user_reg(regs, 0);
+
+ /*
+ * 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 ( smccc_is_conv_64(funcid) && is_32bit_domain(current->domain) )
+ {
+ set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+ return true;
+ }
+
+ switch ( smccc_get_owner(funcid) )
+ {
+ case ARM_SMCCC_OWNER_HYPERVISOR:
+ handled = handle_hypervisor(regs);
+ break;
+ }
+
+ if ( !handled )
+ {
+ gprintk(XENLOG_INFO, "Unhandled SMC/HVC: %08"PRIregister"\n", funcid);
+
+ /* Inform caller that function is not supported. */
+ set_user_reg(regs, 0, ARM_SMCCC_ERR_UNKNOWN_FUNCTION);
+ }
+
+ return true;
+}
+
+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 compatible (e.g. 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/traps.h b/xen/include/asm-arm/traps.h
index f88cbf6..6efd1c5 100644
--- a/xen/include/asm-arm/traps.h
+++ b/xen/include/asm-arm/traps.h
@@ -31,6 +31,9 @@ void do_cp14_64(struct cpu_user_regs *regs, const union hsr hsr);
void do_cp14_dbg(struct cpu_user_regs *regs, const union hsr hsr);
void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
+/* SMCCC handling */
+void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr);
+
#endif /* __ASM_ARM_TRAPS__ */
/*
* Local variables:
diff --git a/xen/include/public/arch-arm/smccc.h b/xen/include/public/arch-arm/smccc.h
new file mode 100644
index 0000000..a1d00ae
--- /dev/null
+++ b/xen/include/public/arch-arm/smccc.h
@@ -0,0 +1,58 @@
+/*
+ * smccc.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_SMCCC_H__
+#define __XEN_PUBLIC_ARCH_ARM_SMCCC_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_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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (5 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 11:53 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
` (2 subsequent siblings)
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Volodymyr Babchuk
PSCI is part of HVC/SMC interface, so it should be handled in
appropriate place: `vsmc.c`. This patch moves PSCI handler
calls from `traps.c` to `vsmc.c`. Also it corrects coding
style of the PSCI handler functions.
Older PSCI 0.1 uses SMC function identifiers in range that is
reserved for existing APIs (ARM DEN 0028B, page 16), while newer
PSCI 0.2 and later is defined as "standard secure service" with its
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>
---
* handle_psci_0_x() renamed to handle_existing_apis()
* spelling fixes
* fixed coding style for moved PSCI code
* previously introduced `funcid` moved to previous patch
---
xen/arch/arm/traps.c | 117 +---------------------
xen/arch/arm/vsmc.c | 189 +++++++++++++++++++++++++++++++++++-
xen/include/asm-arm/traps.h | 1 +
xen/include/public/arch-arm/smccc.h | 8 ++
4 files changed, 196 insertions(+), 119 deletions(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f3b64b4..d00ff36 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1450,119 +1450,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)
-#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
@@ -2251,7 +2138,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 *)®s->r12, hsr.iss);
break;
#ifdef CONFIG_ARM_64
@@ -2263,7 +2150,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, ®s->x16, hsr.iss);
break;
case HSR_EC_SMC64:
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 97a6be3..d3120a5 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/smccc.h>
#include <asm/monitor.h>
+#include <asm/psci.h>
#include <asm/regs.h>
#include <asm/smccc.h>
#include <asm/traps.h>
@@ -26,6 +27,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)
{
int n;
@@ -71,6 +75,154 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
}
}
+#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))
+#else
+#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
+#endif
+
+/* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */
+static bool handle_existing_apis(struct cpu_user_regs *regs)
+{
+ switch ( PSCI_ARG32(regs, 0) )
+ {
+ 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;
+ }
+ default:
+ 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 ( smccc_get_fn(fid) )
+ {
+ case smccc_get_fn(PSCI_0_2_FN_PSCI_VERSION):
+ perfc_incr(vpsci_version);
+ PSCI_SET_RESULT(regs, do_psci_0_2_version());
+ return true;
+
+ case smccc_get_fn(PSCI_0_2_FN_CPU_OFF):
+ perfc_incr(vpsci_cpu_off);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
+ return true;
+
+ case smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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;
+
+ default:
+ 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
@@ -110,11 +262,26 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
return true;
}
- switch ( smccc_get_owner(funcid) )
+ /*
+ * 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 ( funcid >= ARM_SMCCC_RESERVED_RANGE_START &&
+ funcid <= ARM_SMCCC_RESERVED_RANGE_END )
+ handled = handle_existing_apis(regs);
+ else
{
- case ARM_SMCCC_OWNER_HYPERVISOR:
- handled = handle_hypervisor(regs);
- break;
+ switch ( smccc_get_owner(funcid) )
+ {
+ case ARM_SMCCC_OWNER_HYPERVISOR:
+ handled = handle_hypervisor(regs);
+ break;
+ case ARM_SMCCC_OWNER_STANDARD:
+ handled = handle_sssc(regs);
+ break;
+ }
}
if ( !handled )
@@ -158,6 +325,20 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
inject_undef_exception(regs, hsr);
}
+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 compatible (e.g. 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/traps.h b/xen/include/asm-arm/traps.h
index 6efd1c5..0b91aa7 100644
--- a/xen/include/asm-arm/traps.h
+++ b/xen/include/asm-arm/traps.h
@@ -33,6 +33,7 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
/* SMCCC handling */
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_TRAPS__ */
/*
diff --git a/xen/include/public/arch-arm/smccc.h b/xen/include/public/arch-arm/smccc.h
index a1d00ae..caead6e 100644
--- a/xen/include/public/arch-arm/smccc.h
+++ b/xen/include/public/arch-arm/smccc.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_SMCCC_H__ */
/*
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (6 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-09-13 11:58 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 09/10] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 10/10] public: add and enable XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
9 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, 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.
Function psci_mode_check() in vsmc.c will be removed in a next patch,
so there are no need to review it. I had to rework it, because
PSCI_0_2_64BIT definition is dropped now.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
* removed #include <vsmc.h> from seattle.c
* PSCI_0_2_FUNC_xxx renamed back to PSCI_0_2_FN_xxx
* mentioned psci_mode_check() in the commit message
---
xen/arch/arm/platforms/seattle.c | 4 ++--
xen/arch/arm/psci.c | 10 ++++-----
xen/arch/arm/vsmc.c | 22 ++++++++++----------
xen/include/asm-arm/psci.h | 44 ++++++++++++++++++----------------------
4 files changed, 38 insertions(+), 42 deletions(-)
diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
index 86dce91..22c0622 100644
--- a/xen/arch/arm/platforms/seattle.c
+++ b/xen/arch/arm/platforms/seattle.c
@@ -33,12 +33,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..be4e8e6 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 d3120a5..5421bd2 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -114,7 +114,7 @@ static bool handle_existing_apis(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) || !smccc_is_conv_64(fid);
}
/* PSCI 0.2 interface and other Standard Secure Calls */
@@ -124,40 +124,40 @@ static bool handle_sssc(struct cpu_user_regs *regs)
switch ( smccc_get_fn(fid) )
{
- case smccc_get_fn(PSCI_0_2_FN_PSCI_VERSION):
+ case PSCI_0_2_FN_PSCI_VERSION:
perfc_incr(vpsci_version);
PSCI_SET_RESULT(regs, do_psci_0_2_version());
return true;
- case smccc_get_fn(PSCI_0_2_FN_CPU_OFF):
+ case PSCI_0_2_FN_CPU_OFF:
perfc_incr(vpsci_cpu_off);
PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
return true;
- case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
+ 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());
return true;
- case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
+ case 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 smccc_get_fn(PSCI_0_2_FN_SYSTEM_OFF):
+ 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);
return true;
- case smccc_get_fn(PSCI_0_2_FN_SYSTEM_RESET):
+ 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);
return true;
- case smccc_get_fn(PSCI_0_2_FN_CPU_ON):
+ case PSCI_0_2_FN_CPU_ON:
perfc_incr(vpsci_cpu_on);
if ( psci_mode_check(current->domain, fid) )
{
@@ -169,7 +169,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
}
return true;
- case smccc_get_fn(PSCI_0_2_FN_CPU_SUSPEND):
+ case PSCI_0_2_FN_CPU_SUSPEND:
perfc_incr(vpsci_cpu_suspend);
if ( psci_mode_check(current->domain, fid) )
{
@@ -181,7 +181,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
}
return true;
- case smccc_get_fn(PSCI_0_2_FN_AFFINITY_INFO):
+ case PSCI_0_2_FN_AFFINITY_INFO:
perfc_incr(vpsci_cpu_affinity_info);
if ( psci_mode_check(current->domain, fid) )
{
@@ -191,7 +191,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
}
return true;
- case smccc_get_fn(PSCI_0_2_FN_MIGRATE):
+ case PSCI_0_2_FN_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..8ab8d0a 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,3 +43,3 @@ 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_CONV_32, \
+ ARM_SMCCC_OWNER_STANDARD, \
+ PSCI_0_2_FN_##name)
+#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
+ ARM_SMCCC_CONV_64, \
+ ARM_SMCCC_OWNER_STANDARD, \
+ PSCI_0_2_FN_##name)
+#define PSCI_0_2_FN_PSCI_VERSION 0
+#define PSCI_0_2_FN_CPU_SUSPEND 1
+#define PSCI_0_2_FN_CPU_OFF 2
+#define PSCI_0_2_FN_CPU_ON 3
+#define PSCI_0_2_FN_AFFINITY_INFO 4
+#define PSCI_0_2_FN_MIGRATE 5
+#define PSCI_0_2_FN_MIGRATE_INFO_TYPE 6
+#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU 7
+#define PSCI_0_2_FN_SYSTEM_OFF 8
+#define PSCI_0_2_FN_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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 09/10] arm: vsmc: remove 64 bit mode check in PSCI handler
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (7 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 10/10] public: add and enable XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
9 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Julien Grall, Stefano Stabellini, 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>
---
xen/arch/arm/vsmc.c | 62 ++++++++++++++++++++++-------------------------------
1 file changed, 26 insertions(+), 36 deletions(-)
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
index 5421bd2..e778173 100644
--- a/xen/arch/arm/vsmc.c
+++ b/xen/arch/arm/vsmc.c
@@ -111,12 +111,6 @@ static bool handle_existing_apis(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) || !smccc_is_conv_64(fid);
-}
-
/* PSCI 0.2 interface and other Standard Secure Calls */
static bool handle_sssc(struct cpu_user_regs *regs)
{
@@ -141,8 +135,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
case 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());
+ PSCI_SET_RESULT(regs, do_psci_0_2_migrate_info_up_cpu());
return true;
case PSCI_0_2_FN_SYSTEM_OFF:
@@ -158,48 +151,45 @@ static bool handle_sssc(struct cpu_user_regs *regs)
return true;
case 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);
+ {
+ 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));
- }
+ perfc_incr(vpsci_cpu_on);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_on(vcpuid, epoint, cid));
return true;
+ }
case 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);
+ {
+ 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));
- }
+ perfc_incr(vpsci_cpu_suspend);
+ PSCI_SET_RESULT(regs, do_psci_0_2_cpu_suspend(pstate, epoint, cid));
return true;
+ }
case PSCI_0_2_FN_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_FN_MIGRATE:
- perfc_incr(vpsci_cpu_migrate);
- if ( psci_mode_check(current->domain, fid) )
- {
- uint32_t tcpu = PSCI_ARG32(regs, 1);
+ {
+ uint32_t tcpu = PSCI_ARG32(regs, 1);
- PSCI_SET_RESULT(regs, do_psci_0_2_migrate(tcpu));
- }
+ perfc_incr(vpsci_cpu_migrate);
+ 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);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v5 10/10] public: add and enable XENFEAT_ARM_SMCCC_supported feature
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
` (8 preceding siblings ...)
2017-08-31 20:09 ` [PATCH v5 09/10] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
@ 2017-08-31 20:09 ` Volodymyr Babchuk
9 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-08-31 20:09 UTC (permalink / raw)
To: xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall,
Jan Beulich, Volodymyr Babchuk
This feature indicates that hypervisor is compatible with ARM
SMC calling convention. Previously hypervisor would inject an
undefined instruction exception if an invalid SMC function were
called or would crash a domain if an invalid HVC function
were invoked.
XENFEAT_ARM_SMCCC_supported feature means that it safe to invoke
SMC/HVC calls that are compatible with SMC calling convention.
Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---
xen/common/kernel.c | 3 +++
xen/include/public/features.h | 3 +++
2 files changed, 6 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 )
{
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
^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v5 03/10] public: xen.h: add definitions for UUID handling
2017-08-31 20:09 ` [PATCH v5 03/10] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
@ 2017-09-01 9:42 ` Ian Jackson
0 siblings, 0 replies; 26+ messages in thread
From: Ian Jackson @ 2017-09-01 9:42 UTC (permalink / raw)
To: Volodymyr Babchuk
Cc: Edgar E. Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Tim Deegan, xen-devel, Julien Grall, Jan Beulich
Volodymyr Babchuk writes ("[PATCH v5 03/10] public: xen.h: add definitions for UUID handling"):
> 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)
The commit message implies the uuid value is supplied in hex.
But:
> +#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}}
Nothing pastes 0x onto the front of these constants.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
@ 2017-09-13 9:59 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-13 9:59 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> There are standard functions set_user_reg() and get_user_reg(). We can
> use them in PSCI_SET_RESULT()/PSCI_ARG() macros instead of relying on
> CONFIG_ARM_64 definition.
>
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask
2017-08-31 20:09 ` [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
@ 2017-09-13 10:02 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-13 10:02 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> This patch define HSR_XXC_IMM_MASK. It can be used to extract
NIT: s/define/defines/
> immediate value for trapped HVC32, HVC64, SMC64, SVC32, SVC64
> instructions, as described in the ARM ARM
> (ARM DDI 0487B.a pages D7-2270, D7-2272).
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
>
> * spelling fixes
>
> ---
> 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)
>
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 05/10] arm: add SMCCC protocol definitions
2017-08-31 20:09 ` [PATCH v5 05/10] arm: add SMCCC protocol definitions Volodymyr Babchuk
@ 2017-09-13 10:07 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-13 10:07 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr,
On 08/31/2017 09:09 PM, 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. Some of the macros were
> converted to inlined functions to ease parsing.
>
> 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>
> ---
>
> * Accessor macros were converted to inlined functions
> * ARM_SMCCC_SMC_{32,64} renamed to ARM_SMCCC_CONV_{32,64}
> * Fixed indentation for ARM_SMCCC_CALL_VAL
>
> ---
>
> xen/include/asm-arm/smccc.h | 105 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 105 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..f543dea
> --- /dev/null
> +++ b/xen/include/asm-arm/smccc.h
> @@ -0,0 +1,105 @@
> +/*
> + * Copyright (c) 2015, Linaro Limited
See my reply on v4. The rest looks good to me.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
@ 2017-09-13 10:17 ` Julien Grall
2017-09-13 11:11 ` Julien Grall
1 sibling, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-13 10:17 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr,
On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> diff --git a/xen/include/public/arch-arm/smccc.h b/xen/include/public/arch-arm/smccc.h
> new file mode 100644
> index 0000000..a1d00ae
> --- /dev/null
> +++ b/xen/include/public/arch-arm/smccc.h
> @@ -0,0 +1,58 @@
> +/*
> + * smccc.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_SMCCC_H__
> +#define __XEN_PUBLIC_ARCH_ARM_SMCCC_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)
The indentation looks wrong here.
> +
> +#endif /* __XEN_PUBLIC_ARCH_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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
2017-09-13 10:17 ` Julien Grall
@ 2017-09-13 11:11 ` Julien Grall
2017-09-19 21:44 ` Volodymyr Babchuk
1 sibling, 1 reply; 26+ messages in thread
From: Julien Grall @ 2017-09-13 11:11 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi,
On 08/31/2017 09:09 PM, 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 fill_uuid() function
> * dropped vsmc.h header. Function prototypes moved to traps.h
> * public/arch-arm/smc.h header renamed to smccc.h
> * introduced `register_t funcid` in vsmccc_handle_call()x
> * spelling fixes
> * coding style fixes
>
> ---
> xen/arch/arm/Makefile | 1 +
> xen/arch/arm/traps.c | 17 ----
> xen/arch/arm/vsmc.c | 168 ++++++++++++++++++++++++++++++++++++
> xen/include/asm-arm/traps.h | 3 +
> xen/include/public/arch-arm/smccc.h | 58 +++++++++++++
> 5 files changed, 230 insertions(+), 17 deletions(-)
> create mode 100644 xen/arch/arm/vsmc.c
> create mode 100644 xen/include/public/arch-arm/smccc.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 9132fe1..f3b64b4 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -2155,23 +2155,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..97a6be3
> --- /dev/null
> +++ b/xen/arch/arm/vsmc.c
> @@ -0,0 +1,168 @@
> +/*
> + * 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/smccc.h>
> +#include <asm/monitor.h>
> +#include <asm/regs.h>
> +#include <asm/smccc.h>
> +#include <asm/traps.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)
Actually why do you pass a pointer for u? This requires every caller to
introduce temporary variable because the UUID is usually a define.
With your current solution each caller as to do:
xen_uuid_t foo = MY_UUID;
fill_uuid(regs, &foo);
return true;
What I suggested in the previous version is to get fill_uuid return
true. So you make each caller simpler.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c`
2017-08-31 20:09 ` [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
@ 2017-09-13 11:53 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-13 11:53 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Volodymyr,
On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
> PSCI is part of HVC/SMC interface, so it should be handled in
> appropriate place: `vsmc.c`. This patch moves PSCI handler
> calls from `traps.c` to `vsmc.c`. Also it corrects coding
> style of the PSCI handler functions.
>
> Older PSCI 0.1 uses SMC function identifiers in range that is
> reserved for existing APIs (ARM DEN 0028B, page 16), while newer
> PSCI 0.2 and later is defined as "standard secure service" with its
> 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>
> ---
>
> * handle_psci_0_x() renamed to handle_existing_apis()
> * spelling fixes
> * fixed coding style for moved PSCI code
> * previously introduced `funcid` moved to previous patch
>
> ---
> xen/arch/arm/traps.c | 117 +---------------------
> xen/arch/arm/vsmc.c | 189 +++++++++++++++++++++++++++++++++++-
> xen/include/asm-arm/traps.h | 1 +
> xen/include/public/arch-arm/smccc.h | 8 ++
> 4 files changed, 196 insertions(+), 119 deletions(-)
>
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c,
> index f3b64b4..d00ff36 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -1450,119 +1450,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)
> -#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
> @@ -2251,7 +2138,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 *)®s->r12, hsr.iss);
> break;
> #ifdef CONFIG_ARM_64
> @@ -2263,7 +2150,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, ®s->x16, hsr.iss);
> break;
> case HSR_EC_SMC64:
> diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
> index 97a6be3..d3120a5 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/smccc.h>
> #include <asm/monitor.h>
> +#include <asm/psci.h>
> #include <asm/regs.h>
> #include <asm/smccc.h>
> #include <asm/traps.h>
> @@ -26,6 +27,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)
> {
> int n;
> @@ -71,6 +75,154 @@ static bool handle_hypervisor(struct cpu_user_regs *regs)
> }
> }
>
> +#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))
> +#else
> +#define PSCI_ARG32(reg, n) PSCI_ARG(reg, n)
> +#endif
> +
> +/* Existing (pre SMCCC) APIs. This includes PSCI 0.1 interface */
> +static bool handle_existing_apis(struct cpu_user_regs *regs)
> +{
> + switch ( PSCI_ARG32(regs, 0) )
It is a bit odd to use PSCI_ARG32 here for a function called
"handle_existing_apis". What are you trying to achieve and why the
32-bit is necessary here and not in other places (such as handle_sssc)?
> + {
> + 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;
> + }
> + default:
> + 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 ( smccc_get_fn(fid) )
> + {
> + case smccc_get_fn(PSCI_0_2_FN_PSCI_VERSION):
> + perfc_incr(vpsci_version);
> + PSCI_SET_RESULT(regs, do_psci_0_2_version());
> + return true;
> +
> + case smccc_get_fn(PSCI_0_2_FN_CPU_OFF):
> + perfc_incr(vpsci_cpu_off);
> + PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> + return true;
> +
> + case smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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 smccc_get_fn(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;
See my comment on patch #6.
> + }
> +
> + 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;
As suggest on v4, you could introduce a helper to do that for you and
return true. This would make the implementation of
ARM_SMCCC_FUNC_CALL_REVISION easier to read.
> +
> + default:
> + 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
> @@ -110,11 +262,26 @@ static bool vsmccc_handle_call(struct cpu_user_regs *regs)
> return true;
> }
>
> - switch ( smccc_get_owner(funcid) )
> + /*
> + * 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 ( funcid >= ARM_SMCCC_RESERVED_RANGE_START &&
> + funcid <= ARM_SMCCC_RESERVED_RANGE_END )
> + handled = handle_existing_apis(regs);
> + else
> {
> - case ARM_SMCCC_OWNER_HYPERVISOR:
> - handled = handle_hypervisor(regs);
> - break;
> + switch ( smccc_get_owner(funcid) )
> + {
> + case ARM_SMCCC_OWNER_HYPERVISOR:
> + handled = handle_hypervisor(regs);
> + break;
> + case ARM_SMCCC_OWNER_STANDARD:
> + handled = handle_sssc(regs);
> + break;
> + }
> }
>
> if ( !handled )
> @@ -158,6 +325,20 @@ void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
> inject_undef_exception(regs, hsr);
> }
>
> +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 compatible (e.g. 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/traps.h b/xen/include/asm-arm/traps.h
> index 6efd1c5..0b91aa7 100644
> --- a/xen/include/asm-arm/traps.h
> +++ b/xen/include/asm-arm/traps.h
> @@ -33,6 +33,7 @@ void do_cp(struct cpu_user_regs *regs, const union hsr hsr);
>
> /* SMCCC handling */
> 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_TRAPS__ */
> /*
> diff --git a/xen/include/public/arch-arm/smccc.h b/xen/include/public/arch-arm/smccc.h
> index a1d00ae..caead6e 100644
> --- a/xen/include/public/arch-arm/smccc.h
> +++ b/xen/include/public/arch-arm/smccc.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_SMCCC_H__ */
>
> /*
>
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h
2017-08-31 20:09 ` [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
@ 2017-09-13 11:58 ` Julien Grall
2017-09-21 18:28 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2017-09-13 11:58 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Volodymyr,
On 08/31/2017 09:09 PM, 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.
>
> Function psci_mode_check() in vsmc.c will be removed in a next patch,
> so there are no need to review it. I had to rework it, because
> PSCI_0_2_64BIT definition is dropped now.
>
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
>
> * removed #include <vsmc.h> from seattle.c
> * PSCI_0_2_FUNC_xxx renamed back to PSCI_0_2_FN_xxx
> * mentioned psci_mode_check() in the commit message
>
> ---
> xen/arch/arm/platforms/seattle.c | 4 ++--
> xen/arch/arm/psci.c | 10 ++++-----
> xen/arch/arm/vsmc.c | 22 ++++++++++----------
> xen/include/asm-arm/psci.h | 44 ++++++++++++++++++----------------------
> 4 files changed, 38 insertions(+), 42 deletions(-)
>
> diff --git a/xen/arch/arm/platforms/seattle.c b/xen/arch/arm/platforms/seattle.c
> index 86dce91..22c0622 100644
> --- a/xen/arch/arm/platforms/seattle.c
> +++ b/xen/arch/arm/platforms/seattle.c
> @@ -33,12 +33,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..be4e8e6 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)
Why this sudden double tabulation?
> #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)
Ditto.
> #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 d3120a5..5421bd2 100644
> --- a/xen/arch/arm/vsmc.c
> +++ b/xen/arch/arm/vsmc.c
> @@ -114,7 +114,7 @@ static bool handle_existing_apis(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) || !smccc_is_conv_64(fid);
> }
>
> /* PSCI 0.2 interface and other Standard Secure Calls */
> @@ -124,40 +124,40 @@ static bool handle_sssc(struct cpu_user_regs *regs)
>
> switch ( smccc_get_fn(fid) )
> {
> - case smccc_get_fn(PSCI_0_2_FN_PSCI_VERSION):
> + case PSCI_0_2_FN_PSCI_VERSION:
> perfc_incr(vpsci_version);
> PSCI_SET_RESULT(regs, do_psci_0_2_version());
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_CPU_OFF):
> + case PSCI_0_2_FN_CPU_OFF:
> perfc_incr(vpsci_cpu_off);
> PSCI_SET_RESULT(regs, do_psci_0_2_cpu_off());
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_TYPE):
> + 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());
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_MIGRATE_INFO_UP_CPU):
> + case 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 smccc_get_fn(PSCI_0_2_FN_SYSTEM_OFF):
> + 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);
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_SYSTEM_RESET):
> + 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);
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_CPU_ON):
> + case PSCI_0_2_FN_CPU_ON:
> perfc_incr(vpsci_cpu_on);
> if ( psci_mode_check(current->domain, fid) )
> {
> @@ -169,7 +169,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
> }
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_CPU_SUSPEND):
> + case PSCI_0_2_FN_CPU_SUSPEND:
> perfc_incr(vpsci_cpu_suspend);
> if ( psci_mode_check(current->domain, fid) )
> {
> @@ -181,7 +181,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
> }
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_AFFINITY_INFO):
> + case PSCI_0_2_FN_AFFINITY_INFO:
> perfc_incr(vpsci_cpu_affinity_info);
> if ( psci_mode_check(current->domain, fid) )
> {
> @@ -191,7 +191,7 @@ static bool handle_sssc(struct cpu_user_regs *regs)
> }
> return true;
>
> - case smccc_get_fn(PSCI_0_2_FN_MIGRATE):
> + case PSCI_0_2_FN_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..8ab8d0a 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,3 +43,3 @@ 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_CONV_32, \
> + ARM_SMCCC_OWNER_STANDARD, \
> + PSCI_0_2_FN_##name)
Again, the indentation looks wrong here.
> +#define PSCI_0_2_FN64(name) ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> + ARM_SMCCC_CONV_64, \
> + ARM_SMCCC_OWNER_STANDARD, \
> + PSCI_0_2_FN_##name)
Ditto.
> +#define PSCI_0_2_FN_PSCI_VERSION 0
> +#define PSCI_0_2_FN_CPU_SUSPEND 1
> +#define PSCI_0_2_FN_CPU_OFF 2
> +#define PSCI_0_2_FN_CPU_ON 3
> +#define PSCI_0_2_FN_AFFINITY_INFO 4
> +#define PSCI_0_2_FN_MIGRATE 5
> +#define PSCI_0_2_FN_MIGRATE_INFO_TYPE 6
> +#define PSCI_0_2_FN_MIGRATE_INFO_UP_CPU 7
> +#define PSCI_0_2_FN_SYSTEM_OFF 8
> +#define PSCI_0_2_FN_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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-13 11:11 ` Julien Grall
@ 2017-09-19 21:44 ` Volodymyr Babchuk
2017-09-20 17:21 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-09-19 21:44 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Julien,
On 13.09.17 14:11, Julien Grall wrote:
> Hi,
>
> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)
>
> Actually why do you pass a pointer for u? This requires every caller to
> introduce temporary variable because the UUID is usually a define.
Hmm, another way probably is to pass a whole structure as a parameter.
Are you suggesting this approach? Something like
fill_uuid(regs, (xen_uuid_t)MY_UUID)?
> With your current solution each caller as to do:
>
> xen_uuid_t foo = MY_UUID;
>
> fill_uuid(regs, &foo);
>
> return true;
>
> What I suggested in the previous version is to get fill_uuid return
> true. So you make each caller simpler.
Yes, but it will not be correct semantically. There will arise many
questions:
1. Why helper function that only writes data returns bool?
2. If it returns true, can it return false?
3. Should we check its return value before passing it further?
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-19 21:44 ` Volodymyr Babchuk
@ 2017-09-20 17:21 ` Julien Grall
2017-09-20 18:11 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2017-09-20 17:21 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
On 19/09/17 22:44, Volodymyr Babchuk wrote:
> Hi Julien,
Hi Volodymyr,
>
> On 13.09.17 14:11, Julien Grall wrote:
>> Hi,
>>
>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>
>>> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t *u)
>>
>> Actually why do you pass a pointer for u? This requires every caller
>> to introduce temporary variable because the UUID is usually a define.
> Hmm, another way probably is to pass a whole structure as a parameter.
> Are you suggesting this approach? Something like
> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
Something list that. But why do you need the cast? MY_UUID is supposed
to be a xen_uuid_t. No?
>
>> With your current solution each caller as to do:
>>
>> xen_uuid_t foo = MY_UUID;
>>
>> fill_uuid(regs, &foo);
>>
>> return true;
>>
>> What I suggested in the previous version is to get fill_uuid return
>> true. So you make each caller simpler.
> Yes, but it will not be correct semantically. There will arise many
> questions:
> 1. Why helper function that only writes data returns bool?
> 2. If it returns true, can it return false?
> 3. Should we check its return value before passing it further?
I really don't see how
return fill_uuid(regs, MY_UUID);
would be semantically incorrect or even raise all those questions. It is
perfectly fine to always return true. We have place like that and it
helps to streamline the code.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-20 17:21 ` Julien Grall
@ 2017-09-20 18:11 ` Volodymyr Babchuk
2017-09-20 20:02 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-09-20 18:11 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich
Hi Julien,
On 20.09.17 20:21, Julien Grall wrote:
>
>
> On 19/09/17 22:44, Volodymyr Babchuk wrote:
>> Hi Julien,
>
> Hi Volodymyr,
>
>>
>> On 13.09.17 14:11, Julien Grall wrote:
>>> Hi,
>>>
>>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>>
>>>> +static void fill_uuid(struct cpu_user_regs *regs, const
xen_uuid_t *u)
>>>
>>> Actually why do you pass a pointer for u? This requires every caller
>>> to introduce temporary variable because the UUID is usually a define.
>> Hmm, another way probably is to pass a whole structure as a parameter.
>> Are you suggesting this approach? Something like
>> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
>
> Something list that. But why do you need the cast? MY_UUID is supposed
> to be a xen_uuid_t. No?
It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
you remember that thread, there is a requirement to make public headers
compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
Instead it is defined as a plain initializer list.
>>
>>> With your current solution each caller as to do:
>>>
>>> xen_uuid_t foo = MY_UUID;
>>>
>>> fill_uuid(regs, &foo);
>>>
>>> return true;
>>>
>>> What I suggested in the previous version is to get fill_uuid return
>>> true. So you make each caller simpler.
>> Yes, but it will not be correct semantically. There will arise many
>> questions:
>> 1. Why helper function that only writes data returns bool?
>> 2. If it returns true, can it return false?
>> 3. Should we check its return value before passing it further?
>
>
> I really don't see how
>
> return fill_uuid(regs, MY_UUID);
>
> would be semantically incorrect or even raise all those questions. It is
> perfectly fine to always return true. We have place like that and it
> helps to streamline the code.This is a somewhat arguable topic.
Yes, your variant produces less lines of code.
But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
implies that there are some logic in `fill_uid()` and it can return
different results, depending on its parameters. Which is not true and,
thus, misleading.
Just try to look at this from stranger's point of view. Usually you
don't declare function as a `bool` just to spare a line of code. If you
see boolean function, you expect that there are some reason, some logic
behind this.
Look, I have an idea how to resolve this.
fill_uid() will check that MY_UUID != {ffffffff, fffff, fffff, ffff,
ffffffffffff}.
If UUID is invalid, it will print warning to console and return false.
If UUID is valid, it will fill registers and return true.
Now it will be semantically correct to define it as `bool fill_uuid()`
This can work for `fill_uuid()`. But you also expressed the same idea
regarding code that return version. I can create helper function that
fills registers with version info. But there I don't see any excuse to
declare that helper as boolean.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-20 18:11 ` Volodymyr Babchuk
@ 2017-09-20 20:02 ` Julien Grall
2017-09-20 20:26 ` Volodymyr Babchuk
0 siblings, 1 reply; 26+ messages in thread
From: Julien Grall @ 2017-09-20 20:02 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, nd
On 20/09/2017 19:11, Volodymyr Babchuk wrote:
> On 20.09.17 20:21, Julien Grall wrote:
>>
>>
>> On 19/09/17 22:44, Volodymyr Babchuk wrote:
>>> Hi Julien,
>>
>> Hi Volodymyr,
>>
>>>
>>> On 13.09.17 14:11, Julien Grall wrote:
>>>> Hi,
>>>>
>>>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>>>
>>>>> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t
> *u)
>>>>
>>>> Actually why do you pass a pointer for u? This requires every caller
>>>> to introduce temporary variable because the UUID is usually a define.
>>> Hmm, another way probably is to pass a whole structure as a parameter.
>>> Are you suggesting this approach? Something like
>>> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
>>
>> Something list that. But why do you need the cast? MY_UUID is supposed
>> to be a xen_uuid_t. No?
> It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
> you remember that thread, there is a requirement to make public headers
> compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
> Instead it is defined as a plain initializer list.
In that case why don't introduce a version for non-strict ansi? This
would introduce a bit of safety and avoid cast a bit unexplained. (see
how __DECL_REG(...,...) is done in include/public/asm-arm.h?
>
>>>
>>>> With your current solution each caller as to do:
>>>>
>>>> xen_uuid_t foo = MY_UUID;
>>>>
>>>> fill_uuid(regs, &foo);
>>>>
>>>> return true;
>>>>
>>>> What I suggested in the previous version is to get fill_uuid return
>>>> true. So you make each caller simpler.
>>> Yes, but it will not be correct semantically. There will arise many
>>> questions:
>>> 1. Why helper function that only writes data returns bool?
>>> 2. If it returns true, can it return false?
>>> 3. Should we check its return value before passing it further?
>>
>>
>> I really don't see how
>>
>> return fill_uuid(regs, MY_UUID);
>>
>> would be semantically incorrect or even raise all those questions. It is
>> perfectly fine to always return true. We have place like that and it
>> helps to streamline the code.This is a somewhat arguable topic.
> Yes, your variant produces less lines of code.
> But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
> implies that there are some logic in `fill_uid()` and it can return
> different results, depending on its parameters. Which is not true and,
> thus, misleading.
> Just try to look at this from stranger's point of view. Usually you
> don't declare function as a `bool` just to spare a line of code. If you
> see boolean function, you expect that there are some reason, some logic
> behind this.
You didn't get my point. When doing emulation, most of the time case in
a switch have similar prototype. It gets an input and may (or may not)
return an error. It is easier to think of each case with the same
prototype rather than using different one for the sake of avoiding using
bool because you would always return true. It can also help to switch to
an array instead.
>
> Look, I have an idea how to resolve this.
> fill_uid() will check that MY_UUID != {ffffffff, fffff, fffff, ffff,
> ffffffffffff}.
> If UUID is invalid, it will print warning to console and return false.
> If UUID is valid, it will fill registers and return true.
> Now it will be semantically correct to define it as `bool fill_uuid()`
Just no. UUID are coming from the hypervisor and most of the time (if
not always) static. There are clearly no point to always check the UUID
is valid when it is likely is.
>
> This can work for `fill_uuid()`. But you also expressed the same idea
> regarding code that return version. I can create helper function that
> fills registers with version info. But there I don't see any excuse to
> declare that helper as boolean.
The idea is you are going to abstract each case and make easier to
potentially use an array rather than switch in some cases. So you want
to return bool for every function.
Anyway, there are other bugs to fix and it seems unhelpful to argue
here. I will write a follow-up because I don't like the idea of trying
to adapt prototype just because "it does not make sense to return true...".
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-20 20:02 ` Julien Grall
@ 2017-09-20 20:26 ` Volodymyr Babchuk
2017-09-21 14:48 ` Julien Grall
0 siblings, 1 reply; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-09-20 20:26 UTC (permalink / raw)
To: Julien Grall, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, nd
On 20.09.17 23:02, Julien Grall wrote:
>
>
> On 20/09/2017 19:11, Volodymyr Babchuk wrote:
>> On 20.09.17 20:21, Julien Grall wrote:
>>>
>>>
>>> On 19/09/17 22:44, Volodymyr Babchuk wrote:
>>>> Hi Julien,
>>>
>>> Hi Volodymyr,
>>>
>>>>
>>>> On 13.09.17 14:11, Julien Grall wrote:
>>>>> Hi,
>>>>>
>>>>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>>>>
>>>>>> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t
>> *u)
>>>>>
>>>>> Actually why do you pass a pointer for u? This requires every caller
>>>>> to introduce temporary variable because the UUID is usually a define.
>>>> Hmm, another way probably is to pass a whole structure as a parameter.
>>>> Are you suggesting this approach? Something like
>>>> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
>>>
>>> Something list that. But why do you need the cast? MY_UUID is supposed
>>> to be a xen_uuid_t. No?
>> It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
>> you remember that thread, there is a requirement to make public headers
>> compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
>> Instead it is defined as a plain initializer list.
>
> In that case why don't introduce a version for non-strict ansi? This
> would introduce a bit of safety and avoid cast a bit unexplained. (see
> how __DECL_REG(...,...) is done in include/public/asm-arm.h?
I believe you meant arch-arm.h.
Just to be clear, you are proposing to introduce one
#define XEN_DEFINE_UUID in a public header, and another one in a private
header?
Public one will be strictly ANSI-compatible, while private one will be
only gcc-compatible? That is doable, but it will require #ifdef magic
across different headers.
If other maintainers are okay with that, I can do it in this way.
>>
>>>>
>>>>> With your current solution each caller as to do:
>>>>>
>>>>> xen_uuid_t foo = MY_UUID;
>>>>>
>>>>> fill_uuid(regs, &foo);
>>>>>
>>>>> return true;
>>>>>
>>>>> What I suggested in the previous version is to get fill_uuid return
>>>>> true. So you make each caller simpler.
>>>> Yes, but it will not be correct semantically. There will arise many
>>>> questions:
>>>> 1. Why helper function that only writes data returns bool?
>>>> 2. If it returns true, can it return false?
>>>> 3. Should we check its return value before passing it further?
>>>
>>>
>>> I really don't see how
>>>
>>> return fill_uuid(regs, MY_UUID);
>>>
>>> would be semantically incorrect or even raise all those questions. It is
>>> perfectly fine to always return true. We have place like that and it
>>> helps to streamline the code.This is a somewhat arguable topic.
>> Yes, your variant produces less lines of code.
>> But it is harder to read, actually. `return fill_uid(regs, MY_UUID)`
>> implies that there are some logic in `fill_uid()` and it can return
>> different results, depending on its parameters. Which is not true and,
>> thus, misleading.
>> Just try to look at this from stranger's point of view. Usually you
>> don't declare function as a `bool` just to spare a line of code. If you
>> see boolean function, you expect that there are some reason, some logic
>> behind this.
>
> You didn't get my point. When doing emulation, most of the time case in
> a switch have similar prototype. It gets an input and may (or may not)
> return an error. It is easier to think of each case with the same
> prototype rather than using different one for the sake of avoiding using
> bool because you would always return true. It can also help to switch to
> an array instead.
Aha, looks like I'm beginning to see what you are mean. Okay I'll make
those helpers to return bool.
>>
>> Look, I have an idea how to resolve this.
>> fill_uid() will check that MY_UUID != {ffffffff, fffff, fffff, ffff,
>> ffffffffffff}.
>> If UUID is invalid, it will print warning to console and return false.
>> If UUID is valid, it will fill registers and return true.
>> Now it will be semantically correct to define it as `bool fill_uuid()`
>
> Just no. UUID are coming from the hypervisor and most of the time (if
> not always) static. There are clearly no point to always check the UUID
> is valid when it is likely is.
Good point
>
>>
>> This can work for `fill_uuid()`. But you also expressed the same idea
>> regarding code that return version. I can create helper function that
>> fills registers with version info. But there I don't see any excuse to
>> declare that helper as boolean.
>
> The idea is you are going to abstract each case and make easier to
> potentially use an array rather than switch in some cases. So you want
> to return bool for every function.
>
> Anyway, there are other bugs to fix and it seems unhelpful to argue
> here. I will write a follow-up because I don't like the idea of trying
> to adapt prototype just because "it does not make sense to return true...".
Okay, I think I got your idea. And, anyways, you are maintainer :)
Will do as you say.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC
2017-09-20 20:26 ` Volodymyr Babchuk
@ 2017-09-21 14:48 ` Julien Grall
0 siblings, 0 replies; 26+ messages in thread
From: Julien Grall @ 2017-09-21 14:48 UTC (permalink / raw)
To: Volodymyr Babchuk, xen-devel
Cc: Edgar E . Iglesias, Stefano Stabellini, Wei Liu, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Jan Beulich, nd
On 20/09/17 21:26, Volodymyr Babchuk wrote:
>
>
> On 20.09.17 23:02, Julien Grall wrote:
>>
>>
>> On 20/09/2017 19:11, Volodymyr Babchuk wrote:
>>> On 20.09.17 20:21, Julien Grall wrote:
>>>>
>>>>
>>>> On 19/09/17 22:44, Volodymyr Babchuk wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Volodymyr,
>>>>
>>>>>
>>>>> On 13.09.17 14:11, Julien Grall wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 08/31/2017 09:09 PM, Volodymyr Babchuk wrote:
>>>>>
>>>>>>> +static void fill_uuid(struct cpu_user_regs *regs, const xen_uuid_t
>>> *u)
>>>>>>
>>>>>> Actually why do you pass a pointer for u? This requires every caller
>>>>>> to introduce temporary variable because the UUID is usually a define.
>>>>> Hmm, another way probably is to pass a whole structure as a parameter.
>>>>> Are you suggesting this approach? Something like
>>>>> fill_uuid(regs, (xen_uuid_t)MY_UUID)?
>>>>
>>>> Something list that. But why do you need the cast? MY_UUID is supposed
>>>> to be a xen_uuid_t. No?
>>> It have no type. It is just an initializer list like {1,2,3,4,5,6}. If
>>> you remember that thread, there is a requirement to make public headers
>>> compatible with c89. So I can't define MY_UUID as (xen_uuid_t){1,2,3}.
>>> Instead it is defined as a plain initializer list.
>>
>> In that case why don't introduce a version for non-strict ansi? This
>> would introduce a bit of safety and avoid cast a bit unexplained. (see
>> how __DECL_REG(...,...) is done in include/public/asm-arm.h?
> I believe you meant arch-arm.h.
>
> Just to be clear, you are proposing to introduce one
> #define XEN_DEFINE_UUID in a public header, and another one in a private
> header?
No the two in the public header. One version for strict-ansi compiler,
the other for gcc-compatible one.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h
2017-09-13 11:58 ` Julien Grall
@ 2017-09-21 18:28 ` Volodymyr Babchuk
0 siblings, 0 replies; 26+ messages in thread
From: Volodymyr Babchuk @ 2017-09-21 18:28 UTC (permalink / raw)
To: Julien Grall, xen-devel; +Cc: Edgar E . Iglesias, Stefano Stabellini
Hi Julien,
On 13.09.17 14:58, Julien Grall wrote:
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 34ee97e..be4e8e6 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)
>
> Why this sudden double tabulation?
Because original code used tab to do indentation, not spaces.
I'm converting this to spaces.
But I can leave tabs, if you wish.
>> #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)
>
> Ditto.
>
Ditto :)
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-09-21 18:28 UTC | newest]
Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-31 20:09 [PATCH v5 00/10] Handle SMCs and HVCs in conformance with SMCCC Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 01/10] arm: traps: use generic register accessors in the PSCI code Volodymyr Babchuk
2017-09-13 9:59 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 02/10] arm: traps: check if SMC was conditional before handling it Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 03/10] public: xen.h: add definitions for UUID handling Volodymyr Babchuk
2017-09-01 9:42 ` Ian Jackson
2017-08-31 20:09 ` [PATCH v5 04/10] arm: processor.h: add definition for immediate value mask Volodymyr Babchuk
2017-09-13 10:02 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 05/10] arm: add SMCCC protocol definitions Volodymyr Babchuk
2017-09-13 10:07 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 06/10] arm: smccc: handle SMCs according to SMCCC Volodymyr Babchuk
2017-09-13 10:17 ` Julien Grall
2017-09-13 11:11 ` Julien Grall
2017-09-19 21:44 ` Volodymyr Babchuk
2017-09-20 17:21 ` Julien Grall
2017-09-20 18:11 ` Volodymyr Babchuk
2017-09-20 20:02 ` Julien Grall
2017-09-20 20:26 ` Volodymyr Babchuk
2017-09-21 14:48 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 07/10] arm: traps: handle PSCI calls inside `vsmc.c` Volodymyr Babchuk
2017-09-13 11:53 ` Julien Grall
2017-08-31 20:09 ` [PATCH v5 08/10] arm: PSCI: use definitions provided by asm/smccc.h Volodymyr Babchuk
2017-09-13 11:58 ` Julien Grall
2017-09-21 18:28 ` Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 09/10] arm: vsmc: remove 64 bit mode check in PSCI handler Volodymyr Babchuk
2017-08-31 20:09 ` [PATCH v5 10/10] public: add and enable XENFEAT_ARM_SMCCC_supported feature Volodymyr Babchuk
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.