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

2017-07-18 Thread Julien Grall 



On 30/06/17 16:15, Julien Grall wrote:

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

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

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


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


Cheers,

--
Julien Grall

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

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

2017-06-30 Thread Julien Grall 

Hi Volodymyr,

On 22/06/17 17:24, Volodymyr Babchuk wrote:

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

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

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

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

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


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



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

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..4efd01c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
 obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 obj-y += vm_event.o
 obj-y += vtimer.o
+obj-y += vsmc.o
 obj-y += vpsci.o
 obj-y += vuart.o

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2054c69..66242e5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -44,6 +44,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "decode.h"
 #include "vtimer.h"
@@ -2771,10 +2772,23 @@ static void do_trap_smc(struct cpu_user_regs *regs, 
const union hsr hsr)
 {
 int rc = 0;

+if ( !check_conditional_instr(regs, hsr) )


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


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


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

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

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


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


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


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


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



+{
+advance_pc(regs, hsr);
+return;
+}
+
+/* If monitor is enabled, let it handle the call */
 if ( current->domain->arch.monitor.privileged_call_enabled )
 rc = monitor_smc();

-if ( rc != 1 )
+if ( rc == 1 )
+return;
+
+/* Use standard routines to handle the call */
+if ( vsmc_handle_call(regs) )
+advance_pc(regs, hsr);
+else
 inject_undef_exception(regs, hsr);
 }

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

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

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

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

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

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

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

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 49e1fb2..4efd01c 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -50,6 +50,7 @@ obj-$(CONFIG_HAS_GICV3) += vgic-v3.o
 obj-$(CONFIG_HAS_ITS) += vgic-v3-its.o
 obj-y += vm_event.o
 obj-y += vtimer.o
+obj-y += vsmc.o
 obj-y += vpsci.o
 obj-y += vuart.o
 
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 2054c69..66242e5 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -44,6 +44,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "decode.h"
 #include "vtimer.h"
@@ -2771,10 +2772,23 @@ static void do_trap_smc(struct cpu_user_regs *regs, 
const union hsr hsr)
 {
 int rc = 0;
 
+if ( !check_conditional_instr(regs, hsr) )
+{
+advance_pc(regs, hsr);
+return;
+}
+
+/* If monitor is enabled, let it handle the call */
 if ( current->domain->arch.monitor.privileged_call_enabled )
 rc = monitor_smc();
 
-if ( rc != 1 )
+if ( rc == 1 )
+return;
+
+/* Use standard routines to handle the call */
+if ( vsmc_handle_call(regs) )
+advance_pc(regs, hsr);
+else
 inject_undef_exception(regs, hsr);
 }
 
diff --git a/xen/arch/arm/vsmc.c b/xen/arch/arm/vsmc.c
new file mode 100644
index 000..10c4acd
--- /dev/null
+++ b/xen/arch/arm/vsmc.c
@@ -0,0 +1,128 @@
+/*
+ * xen/arch/arm/vsmc.c
+ *
+ * Generic handler for SMC and HVC calls according to
+ * ARM SMC calling convention
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+
+#include 
+#include 
+/* Need to include xen/sched.h before asm/domain.h or it breaks build*/
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/*
+ * Hypervisor Service version
+ *
+ * We can't use XEN version here, because of SMCCC requirements:
+ * Major revision should change every time SMC/HVC function is removed.
+ * Minor revision should change every time SMC/HVC function is added.
+ * So, it is SMCCC protocol revision code, not XEN version.
+ *
+ * Those values are subjected to change, when interface will be extended.
+ * They should not be stored in public/asm-arm/smc.h because they should
+ * be queried by guest using SMC/HVC interface.
+ */
+#define XEN_SMCCC_MAJOR_REVISION 0
+#define XEN_SMCCC_MINOR_REVISION 1
+
+/* Number of functions currently supported by Hypervisor Service. */
+#define XEN_SMCCC_FUNCTION_COUNT 3
+
+/* SMCCC interface for hypervisor. Tell about itself.