On 03/17/2017 06:35 AM, Wei Chen wrote:
Hi Julien,
Hi Wei,
Sorry for the late answer, I missed that e-mail.
On 2017/3/17 6:24, Julien Grall wrote:
On 03/16/2017 09:53 AM, Wei Chen wrote:
[...]
+/*
+ * Decode the branch offset from a branch instruction's imm field.
+ * The branch offset is a signed value, so it can be used to compute
+ * a new branch target.
+ */
+int32_t aarch32_get_branch_offset(uint32_t insn)
+{
+ uint32_t imm;
+
+ /* Retrieve imm from branch instruction. */
+ imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & BRANCH_INSN_IMM_MASK;
+
+ /*
+ * Check the imm signed bit. If the imm is a negative value, we
+ * have to extend the imm to a full 32 bit negative value.
+ */
+ if ( imm & BIT(23) )
+ imm |= GENMASK(31, 24);
+
+ return (int32_t)(imm << 2);
+}
+
+/*
+ * Encode the displacement of a branch in the imm field and return the
+ * updated instruction.
+ */
+uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset)
+{
+ if ( offset & 0x3 )
+ {
+ printk(XENLOG_ERR
+ "%s: ARM32 instructions must be word aligned.\n", __func__);
This error message looks wrong to me. The offset is not an instruction.
But do we really care about checking that offset is aligned to 4-bit?
After all we will shift the value later on.
I think we must care about the offset alignment. Even though we will
shift the last two bits later on. But a unaligned offset itself
indicates some problems (Compiler issue or target offset calculate bug).
If we ignore it, we will ignore some potential problems.
I don't think this is really important. I looked at the ARM64
counterpart (see aarch64_set_branch_offset) and we don't do the check.
About the message can we change to:
"Target ARM32 instructions must be placed on word aligned addresses?"
That would be ok.
[...]
/*
diff --git a/xen/include/asm-arm/arm32/insn.h b/xen/include/asm-arm/arm32/insn.h
new file mode 100644
index 0000000..045acc3
--- /dev/null
+++ b/xen/include/asm-arm/arm32/insn.h
@@ -0,0 +1,57 @@
+/*
+ * Copyright (C) 2017 ARM Ltd.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#ifndef __ARCH_ARM_ARM32_INSN
+#define __ARCH_ARM_ARM32_INSN
+
+#include <xen/types.h>
+
+#define __AARCH32_INSN_FUNCS(abbr, mask, val) \
+static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
+{ \
+ return (code & (mask)) == (val); \
+}
+
+__AARCH32_INSN_FUNCS(b, 0x0F000000, 0x0A000000)
Looking at the ARM ARM (A8.8.18 in DDI406C.c) when cond = 0b1111, this
will be a bx. Thankfully we also want to catch them.
I think this needs to be spelled out in the code to help the reader
understand how bx is handled.
Sorry, I am not very clear about this comment. I read the (A5.7 in
DDI406C.c) and could not find bx unconditional instructions list.
Did you mean the unconditional bl/blx?
I meant blx rather than bx in my previous e-mail. Sorry.
Anyhow I think your concern is right. We have to cover the condition
field. Some unconditional instructions may have a conflicted op field
as conditional branch instructions.
The only unconditional instruction with the same encoding is blx. So it
is fine. But this either need to be:
1) properly documented
2) introducing a blx macro
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
