[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



On 06/19/2017 10:59 PM, Volodymyr Babchuk wrote:
Hello Julien,

Hi Volodymyr,

[...]

+/**
+ * smccc_handle_call() - handle SMC/HVC call according to ARM SMCCC
+ */
+void smccc_handle_call(struct cpu_user_regs *regs, const union hsr hsr)

hsr is already part of regs.

+{
+    bool handled = false;
+

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

We should follow what the spec says here. Per section 2.7 (ARM DEN 0028B), you should return ARM_SMCC_ERR_UNKOWN_FUNCTION for SMC64/HVC64 call from AArch32.

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

Well, the immediate is present for the ISS encoding for exception from SMC executed in AArch64 state. So you can decode the immediate here.

For SMC executed in AArch32 state, indeed the immediate is not present.

I had a quick look at the arm trusted firmware. I haven't spot any decoding of the immediate from an instruction.

My main concern is any non-zero value are reserved. If they are used in the future, we may emulate them by mistake. So we should definitely handle it for AArch64. For AArch32, we could decode the instruction, but that would be time consuming for the benefits of future extension but add overhead on the SMC call. So I would just consider it as always 0 with a suitable comment on top explaining why.

[...]

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

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

Using Xen coding style always (see CODING_STYLE).

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@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®.