[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



Hello Julien,

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

On 15.06.17 13:48, Julien Grall wrote:
On 14/06/17 15:10, Volodymyr Babchuk wrote:
SMCCC (SMC Call Convention) describes how to handle both HVCs and SMCs.
SMCCC states that both HVC and SMC are valid conduits to call to a different
firmware functions. Thus, for example PSCI calls can be made both by
SMC or HVC. Also SMCCC defines function number coding for such calls.
Besides functional calls there are query calls, which allows underling
OS determine version, UID and number of functions provided by service
provider.

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

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

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

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

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..b8728cf 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -39,6 +39,7 @@ obj-y += psci.o
 obj-y += setup.o
 obj-y += shutdown.o
 obj-y += smc.o
+obj-y += smccc.o
 obj-y += smp.o
 obj-y += smpboot.o
 obj-y += sysctl.o
diff --git a/xen/arch/arm/smccc.c b/xen/arch/arm/smccc.c
new file mode 100644
index 0000000..5d10964
--- /dev/null
+++ b/xen/arch/arm/smccc.c

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

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

s/callling/calling/

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

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

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

+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+
+#include <xen/config.h>
+#include <xen/lib.h>
+#include <xen/perfc.h>

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

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

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

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

You don't use this header here.

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

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

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

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

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

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

So please add a newline for clarity.

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

Tell about itself. + missing full stop.

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

hsr is already part of regs.

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

hsr is already part of regs.

+{
+    bool handled = false;
+

I am a bit surprised, I don't see any check to prevent a 32-bit guest to use SMC64 call.
Should we return ARM_SMCCC_ERR_UNKNOWN_FUNCTION code in this case? Or inject undefined instruction?

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


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

s/Uhandled/Unhandled/

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

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

 #include "decode.h"
 #include "vtimer.h"
@@ -2781,11 +2782,16 @@ static void do_trap_smc(struct cpu_user_regs *regs, const union hsr hsr)
 {

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

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

     int rc = 0;

+    /* Let monitor to handle the call */
     if ( current->domain->arch.monitor.privileged_call_enabled )
         rc = monitor_smc();

-    if ( rc != 1 )
-        inject_undef_exception(regs, hsr);
+    if ( rc == 1 )
+        return;

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

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

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

+    advance_pc(regs, hsr);
 }

 static void enter_hypervisor_head(struct cpu_user_regs *regs)
diff --git a/xen/include/asm-arm/smccc.h b/xen/include/asm-arm/smccc.h
new file mode 100644
index 0000000..9342d5e
--- /dev/null
+++ b/xen/include/asm-arm/smccc.h
@@ -0,0 +1,89 @@
+/*
+ * Copyright (c) 2017, EPAM Systems
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+#ifndef __ASM_ARM_SMCCC_H_

It should be __ASM_ARM_SMCC_H__

+#define __ASM_ARM_SMCCC_H_

Ditto.

+
+#include <xen/types.h>
+
+/*
+ * This file provides common defines for ARM SMC Calling Convention as
+ * specified in
+ * http://infocenter.arm.com/help/topic/com.arm.doc.den0028a/index.html
+ */
+
+#define ARM_SMCCC_STD_CALL        0

Is this file coming from Linux? If so, it should be mention. If not, please use soft tab and not hard tab. This is valid in all this file.Actually, part of this defines are from Linux, another defines was added
by myself. What I should to in this case?

+#define ARM_SMCCC_FAST_CALL        1
+#define ARM_SMCCC_TYPE_SHIFT        31
+
+#define ARM_SMCCC_SMC_32        0
+#define ARM_SMCCC_SMC_64        1
+#define ARM_SMCCC_CALL_CONV_SHIFT    30
+
+#define ARM_SMCCC_OWNER_MASK        0x3F
+#define ARM_SMCCC_OWNER_SHIFT        24
+
+#define ARM_SMCCC_FUNC_MASK        0xFFFF
+
+#define ARM_SMCCC_IS_FAST_CALL(smc_val)    \
+    ((smc_val) & (ARM_SMCCC_FAST_CALL << ARM_SMCCC_TYPE_SHIFT))
+#define ARM_SMCCC_IS_64(smc_val) \
+    ((smc_val) & (ARM_SMCCC_SMC_64 << ARM_SMCCC_CALL_CONV_SHIFT))
+#define ARM_SMCCC_FUNC_NUM(smc_val)    ((smc_val) & ARM_SMCCC_FUNC_MASK)
+#define ARM_SMCCC_OWNER_NUM(smc_val) \
+    (((smc_val) >> ARM_SMCCC_OWNER_SHIFT) & ARM_SMCCC_OWNER_MASK)
+
+#define ARM_SMCCC_CALL_VAL(type, calling_convention, owner, func_num) \
+    (((type) << ARM_SMCCC_TYPE_SHIFT) | \
+    ((calling_convention) << ARM_SMCCC_CALL_CONV_SHIFT) | \
+    (((owner) & ARM_SMCCC_OWNER_MASK) << ARM_SMCCC_OWNER_SHIFT) | \
+    ((func_num) & ARM_SMCCC_FUNC_MASK))

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

+
+#define ARM_SMCCC_OWNER_ARCH        0
+#define ARM_SMCCC_OWNER_CPU        1
+#define ARM_SMCCC_OWNER_SIP        2
+#define ARM_SMCCC_OWNER_OEM        3
+#define ARM_SMCCC_OWNER_STANDARD    4
+#define ARM_SMCCC_OWNER_HYPERVISOR    5
+#define ARM_SMCCC_OWNER_TRUSTED_APP    48
+#define ARM_SMCCC_OWNER_TRUSTED_APP_END    49
+#define ARM_SMCCC_OWNER_TRUSTED_OS    50
+#define ARM_SMCCC_OWNER_TRUSTED_OS_END    63
+
+#define ARM_SMCCC_FUNC_CALL_COUNT    0xFF00
+#define ARM_SMCCC_FUNC_CALL_UID        0xFF01
+#define ARM_SMCCC_FUNC_CALL_REVISION    0xFF03
+
+#define ARM_SMCCC_ERR_UNKNOWN_FUNCTION    (-1)
+
+typedef struct {
+    uint32_t a[4];
+} arm_smccc_uid;
+
+#define ARM_SMCCC_UID(a, b, c, d0, d1, d2, d3, d4, d5, d6, d7)        \
+    ((arm_smccc_uid) {{(a), ((b) << 16 | (c) ),            \
+               ((d0) << 24 | (d1) << 16 | (d2) << 8 | (d3) << 0), \
+               ((d4) << 24 | (d5) << 16 | (d6) << 8 | (d7) << 0)}})
+
+void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr);
+
+#endif

#endif /* __ASM_ARM_SMCC_H__

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


Cheers,


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.