[PATCH 0/2] xen/arm32: Use alternative to skip the check of pending serrors

[PATCH 0/2] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to avoid add overhead to check the
option at every trap, we want to use the alternative to skip
this check.

In this series:
1. Introduce alternative patching support for ARM32.
2. Update the ARM32 SErrors handle code to use the alternative.

---
This series is the follow-up of the
"Provide a command line option to choose how to handle "
https://lists.xen.org/archives/html/xen-devel/2017-03/msg01423.html

Wei Chen (2):
  xen/arm32: Introduce alternative runtime patching
  xen/arm32: Use alternative to skip the check of pending serrors

 xen/arch/arm/Kconfig             |  2 +-
 xen/arch/arm/arm32/Makefile      |  1 +
 xen/arch/arm/arm32/entry.S       | 27 ++++-------
 xen/arch/arm/arm32/insn.c        | 99 ++++++++++++++++++++++++++++++++++++++++
 xen/common/livepatch.c           |  1 +
 xen/include/asm-arm/arm32/insn.h | 57 +++++++++++++++++++++++
 xen/include/asm-arm/insn.h       |  2 +
 7 files changed, 170 insertions(+), 19 deletions(-)
 create mode 100644 xen/arch/arm/arm32/insn.c
 create mode 100644 xen/include/asm-arm/arm32/insn.h

-- 
2.7.4


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

[PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Wei Chen]

This patch is based on the implementation of ARM64, it introduces
alternative runtime patching to ARM32. This allows to patch assembly
instruction at runtime to either fix hardware bugs or optimize for
certain hardware features on ARM32 platform.

Xen hypervisor is using ARM execution state only on ARM32 platform,
Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
TBB and TBH) are not considered in alternatives.

The left ARM32 branch instructions are BX, BLX, BL and B. The
instruction BX is taking a register in parameter, so we don't need to
rewrite it. The instructions BLX, BL and B are using the similar
encoding for the offset and will avoid specific case when extracting
and updating the offset.

In this patch, we include alternative.h header file to livepatch.c
directly for ARM32 compilation issues. When the alternative patching
config is enabled, the livepatch.c will use the alternative functions.
In this case, we should include the alternative header file to this
file. But for ARM64, it does not include this header file directly.
It includes this header file indirectly through:
sched.h->domain.h->page.h->alternative.h.
But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
and we don't have the reason to include it to ARM32 page.h now. So we
have to include the alternative.h directly in livepatch.c.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/Kconfig             |  2 +-
 xen/arch/arm/arm32/Makefile      |  1 +
 xen/arch/arm/arm32/insn.c        | 99 ++++++++++++++++++++++++++++++++++++++++
 xen/common/livepatch.c           |  1 +
 xen/include/asm-arm/arm32/insn.h | 57 +++++++++++++++++++++++
 xen/include/asm-arm/insn.h       |  2 +
 6 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 xen/arch/arm/arm32/insn.c
 create mode 100644 xen/include/asm-arm/arm32/insn.h

diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
index 2e023d1..43123e6 100644
--- a/xen/arch/arm/Kconfig
+++ b/xen/arch/arm/Kconfig
@@ -12,11 +12,11 @@ config ARM_32
 config ARM_64
 	def_bool y
 	depends on 64BIT
-	select HAS_ALTERNATIVE
 	select HAS_GICV3
 
 config ARM
 	def_bool y
+	select HAS_ALTERNATIVE
 	select HAS_ARM_HDLCD
 	select HAS_DEVICE_TREE
 	select HAS_MEM_ACCESS
diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
index 4395693..0ac254f 100644
--- a/xen/arch/arm/arm32/Makefile
+++ b/xen/arch/arm/arm32/Makefile
@@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
 obj-y += domctl.o
 obj-y += domain.o
 obj-y += entry.o
+obj-y += insn.o
 obj-$(CONFIG_LIVEPATCH) += livepatch.o
 obj-y += proc-v7.o proc-caxx.o
 obj-y += smpboot.o
diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
new file mode 100644
index 0000000..91a3010
--- /dev/null
+++ b/xen/arch/arm/arm32/insn.c
@@ -0,0 +1,99 @@
+/*
+  * 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/>.
+  */
+#include <xen/lib.h>
+#include <xen/bitops.h>
+#include <xen/sizes.h>
+#include <asm/bug.h>
+#include <asm/insn.h>
+
+/* Mask of branch instructions' immediate. */
+#define BRANCH_INSN_IMM_MASK    GENMASK(23, 0)
+/* Shift of branch instructions' immediate. */
+#define BRANCH_INSN_IMM_SHIFT   0
+
+static uint32_t branch_insn_encode_immediate(uint32_t insn, int32_t offset)
+{
+    uint32_t imm;
+
+    /*
+     * Encode the offset to imm. All ARM32 instructions must be word aligned.
+     * Therefore the offset value's bits [1:0] equal to zero.
+     * (see ARM DDI 0406C.b A8.8.18/A8.8.25 for more encode/decode details
+     * about ARM32 branch instructions)
+     */
+    imm = ((offset >> 2) & BRANCH_INSN_IMM_MASK) << BRANCH_INSN_IMM_SHIFT;
+
+    /* Update the immediate field. */
+    insn &= ~(BRANCH_INSN_IMM_MASK << BRANCH_INSN_IMM_SHIFT);
+    insn |= imm;
+
+    return insn;
+}
+
+/*
+ * 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__);
+        return BUG_OPCODE;
+    }
+
+    /* B/BL support [-32M, 32M) offset (see ARM DDI 0406C.b A4.3). */
+    if ( offset < -SZ_32M || offset >= SZ_32M )
+    {
+        printk(XENLOG_ERR
+               "%s: new branch offset out of range.\n", __func__);
+        return BUG_OPCODE;
+    }
+
+    return branch_insn_encode_immediate(insn, offset);
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index 246e673..f14bcbc 100644
--- a/xen/common/livepatch.c
+++ b/xen/common/livepatch.c
@@ -25,6 +25,7 @@
 #include <xen/livepatch.h>
 #include <xen/livepatch_payload.h>
 
+#include <asm/alternative.h>
 #include <asm/event.h>
 
 /*
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)
+__AARCH32_INSN_FUNCS(bl, 0x0F000000, 0x0B000000)
+
+int32_t aarch32_get_branch_offset(uint32_t insn);
+uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset);
+
+/* Wrapper for common code */
+static inline bool insn_is_branch_imm(uint32_t insn)
+{
+    return ( aarch32_insn_is_b(insn) || aarch32_insn_is_bl(insn) );
+}
+
+static inline int32_t insn_get_branch_offset(uint32_t insn)
+{
+    return aarch32_get_branch_offset(insn);
+}
+
+static inline uint32_t insn_set_branch_offset(uint32_t insn, int32_t offset)
+{
+    return aarch32_set_branch_offset(insn, offset);
+}
+
+#endif /* !__ARCH_ARM_ARM32_INSN */
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
index a205ceb..3489179 100644
--- a/xen/include/asm-arm/insn.h
+++ b/xen/include/asm-arm/insn.h
@@ -5,6 +5,8 @@
 
 #if defined(CONFIG_ARM_64)
 # include <asm/arm64/insn.h>
+#elif defined(CONFIG_ARM_32)
+# include <asm/arm32/insn.h>
 #else
 # error "unknown ARM variant"
 #endif
-- 
2.7.4


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

[PATCH 2/2] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

We have provided an option to administrator to determine how to
handle the SErrors. In order to skip the check of pending SError,
in conventional way, we have to read the option every time before
we try to check the pending SError. This will add overhead to check
the option at every trap.

The ARM32 supports the alternative patching feature. We can use an
ALTERNATIVE to avoid checking option at every trap. We added a new
cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
enabled when the option is not diverse.

Signed-off-by: Wei Chen <Wei.Chen@arm.com>
---
 xen/arch/arm/arm32/entry.S | 27 +++++++++------------------
 1 file changed, 9 insertions(+), 18 deletions(-)

diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 79929ca..105cae8 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -1,6 +1,6 @@
 #include <asm/asm_defns.h>
 #include <asm/regs.h>
-#include <asm/cpufeature.h>
+#include <asm/alternative.h>
 #include <public/xen.h>
 
 #define SAVE_ONE_BANKED(reg)    mrs r11, reg; str r11, [sp, #UREGS_##reg]
@@ -12,21 +12,6 @@
 #define RESTORE_BANKED(mode) \
         RESTORE_ONE_BANKED(SP_##mode) ; RESTORE_ONE_BANKED(LR_##mode) ; RESTORE_ONE_BANKED(SPSR_##mode)
 
-/*
- * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu feature,
- * the checking of pending SErrors will be skipped.
- *
- * As it is a temporary solution, we are assuming that
- * SKIP_CHECK_PENDING_VSERROR will always be in the first word for
- * cpu_hwcaps. This would have to be dropped as soon as ARM32 gain
- * support of alternative.
- */
-#define SKIP_VSERROR_CHECK                      \
-        ldr r1, =cpu_hwcaps;                    \
-        ldr r1, [r1];                           \
-        tst r1, #SKIP_CHECK_PENDING_VSERROR;    \
-        moveq pc, lr
-
 #define SAVE_ALL                                                        \
         sub sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */      \
         push {r0-r12}; /* Save R0-R12 */                                \
@@ -61,8 +46,13 @@ save_guest_regs:
         SAVE_ONE_BANKED(R8_fiq); SAVE_ONE_BANKED(R9_fiq); SAVE_ONE_BANKED(R10_fiq)
         SAVE_ONE_BANKED(R11_fiq); SAVE_ONE_BANKED(R12_fiq);
 
-        SKIP_VSERROR_CHECK
-
+        /*
+         * If the SKIP_CHECK_PENDING_VSERROR has been set in the cpu feature,
+         * the checking of pending SErrors will be skipped.
+         */
+        ALTERNATIVE("nop",
+                    "b skip_check",
+                    SKIP_CHECK_PENDING_VSERROR)
         /*
          * Start to check pending virtual abort in the gap of Guest -> HYP
          * world switch.
@@ -118,6 +108,7 @@ abort_guest_exit_end:
          */
         bne return_from_trap
 
+skip_check:
         mov pc, lr
 
 #define DEFINE_TRAP_ENTRY(trap)                                         \
-- 
2.7.4


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

Re: [PATCH 2/2] xen/arm32: Use alternative to skip the check of pending serrors

[Julien Grall]

Hi Wei,

On 03/16/2017 09:53 AM, Wei Chen wrote:
> We have provided an option to administrator to determine how to
> handle the SErrors. In order to skip the check of pending SError,
> in conventional way, we have to read the option every time before
> we try to check the pending SError. This will add overhead to check
> the option at every trap.
>
> The ARM32 supports the alternative patching feature. We can use an
> ALTERNATIVE to avoid checking option at every trap. We added a new
> cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
> enabled when the option is not diverse.

Can you please squash this patch in #10 of the SError series?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Julien Grall]

Hi Wei,

On 03/16/2017 09:53 AM, Wei Chen wrote:
> This patch is based on the implementation of ARM64, it introduces
> alternative runtime patching to ARM32. This allows to patch assembly
> instruction at runtime to either fix hardware bugs or optimize for
> certain hardware features on ARM32 platform.
>
> Xen hypervisor is using ARM execution state only on ARM32 platform,
> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
> TBB and TBH) are not considered in alternatives.
>
> The left ARM32 branch instructions are BX, BLX, BL and B. The
> instruction BX is taking a register in parameter, so we don't need to
> rewrite it. The instructions BLX, BL and B are using the similar
> encoding for the offset and will avoid specific case when extracting
> and updating the offset.
>
> In this patch, we include alternative.h header file to livepatch.c
> directly for ARM32 compilation issues. When the alternative patching
> config is enabled, the livepatch.c will use the alternative functions.
> In this case, we should include the alternative header file to this
> file. But for ARM64, it does not include this header file directly.
> It includes this header file indirectly through:
> sched.h->domain.h->page.h->alternative.h.
> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
> and we don't have the reason to include it to ARM32 page.h now. So we
> have to include the alternative.h directly in livepatch.c.
>
> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
> ---
>  xen/arch/arm/Kconfig             |  2 +-
>  xen/arch/arm/arm32/Makefile      |  1 +
>  xen/arch/arm/arm32/insn.c        | 99 ++++++++++++++++++++++++++++++++++++++++
>  xen/common/livepatch.c           |  1 +

CC Ross and Konrad for the livepatch part.

>  xen/include/asm-arm/arm32/insn.h | 57 +++++++++++++++++++++++
>  xen/include/asm-arm/insn.h       |  2 +
>  6 files changed, 161 insertions(+), 1 deletion(-)
>  create mode 100644 xen/arch/arm/arm32/insn.c
>  create mode 100644 xen/include/asm-arm/arm32/insn.h
>
> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
> index 2e023d1..43123e6 100644
> --- a/xen/arch/arm/Kconfig
> +++ b/xen/arch/arm/Kconfig
> @@ -12,11 +12,11 @@ config ARM_32
>  config ARM_64
>  	def_bool y
>  	depends on 64BIT
> -	select HAS_ALTERNATIVE
>  	select HAS_GICV3
>
>  config ARM
>  	def_bool y
> +	select HAS_ALTERNATIVE
>  	select HAS_ARM_HDLCD
>  	select HAS_DEVICE_TREE
>  	select HAS_MEM_ACCESS
> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
> index 4395693..0ac254f 100644
> --- a/xen/arch/arm/arm32/Makefile
> +++ b/xen/arch/arm/arm32/Makefile
> @@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
>  obj-y += domctl.o
>  obj-y += domain.o
>  obj-y += entry.o
> +obj-y += insn.o
>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>  obj-y += proc-v7.o proc-caxx.o
>  obj-y += smpboot.o
> diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
> new file mode 100644
> index 0000000..91a3010
> --- /dev/null
> +++ b/xen/arch/arm/arm32/insn.c
> @@ -0,0 +1,99 @@
> +/*
> +  * 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/>.
> +  */
> +#include <xen/lib.h>
> +#include <xen/bitops.h>
> +#include <xen/sizes.h>
> +#include <asm/bug.h>

This is already included by xen/lib.h.

> +#include <asm/insn.h>
> +
> +/* Mask of branch instructions' immediate. */
> +#define BRANCH_INSN_IMM_MASK    GENMASK(23, 0)
> +/* Shift of branch instructions' immediate. */
> +#define BRANCH_INSN_IMM_SHIFT   0
> +
> +static uint32_t branch_insn_encode_immediate(uint32_t insn, int32_t offset)
> +{
> +    uint32_t imm;
> +
> +    /*
> +     * Encode the offset to imm. All ARM32 instructions must be word aligned.
> +     * Therefore the offset value's bits [1:0] equal to zero.
> +     * (see ARM DDI 0406C.b A8.8.18/A8.8.25 for more encode/decode details

DDI 0406C.b is an old version (July 2012) of the ARM ARM. Please try to 
use the latest version (e.g 0406C.c released in May 2014) when possible.

> +     * about ARM32 branch instructions)
> +     */
> +    imm = ((offset >> 2) & BRANCH_INSN_IMM_MASK) << BRANCH_INSN_IMM_SHIFT;
> +
> +    /* Update the immediate field. */
> +    insn &= ~(BRANCH_INSN_IMM_MASK << BRANCH_INSN_IMM_SHIFT);
> +    insn |= imm;
> +
> +    return insn;
> +}
> +
> +/*
> + * 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.

> +        return BUG_OPCODE;
> +    }
> +
> +    /* B/BL support [-32M, 32M) offset (see ARM DDI 0406C.b A4.3). */
> +    if ( offset < -SZ_32M || offset >= SZ_32M )
> +    {
> +        printk(XENLOG_ERR
> +               "%s: new branch offset out of range.\n", __func__);
> +        return BUG_OPCODE;
> +    }
> +
> +    return branch_insn_encode_immediate(insn, offset);
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 246e673..f14bcbc 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -25,6 +25,7 @@
>  #include <xen/livepatch.h>
>  #include <xen/livepatch_payload.h>
>
> +#include <asm/alternative.h>
>  #include <asm/event.h>
>
>  /*
> 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.

> +__AARCH32_INSN_FUNCS(bl, 0x0F000000, 0x0B000000)
> +
> +int32_t aarch32_get_branch_offset(uint32_t insn);
> +uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset);
> +
> +/* Wrapper for common code */
> +static inline bool insn_is_branch_imm(uint32_t insn)
> +{
> +    return ( aarch32_insn_is_b(insn) || aarch32_insn_is_bl(insn) );
> +}
> +
> +static inline int32_t insn_get_branch_offset(uint32_t insn)
> +{
> +    return aarch32_get_branch_offset(insn);
> +}
> +
> +static inline uint32_t insn_set_branch_offset(uint32_t insn, int32_t offset)
> +{
> +    return aarch32_set_branch_offset(insn, offset);
> +}
> +
> +#endif /* !__ARCH_ARM_ARM32_INSN */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index a205ceb..3489179 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -5,6 +5,8 @@
>
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/insn.h>
>  #else
>  # error "unknown ARM variant"
>  #endif
>

Cheers,

-- 
Julien Grall

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

Re: [PATCH 0/2] xen/arm32: Use alternative to skip the check of pending serrors

[Julien Grall]

Hi,

On 03/16/2017 09:53 AM, Wei Chen wrote:
> We have provided an option to administrator to determine how to
> handle the SErrors. In order to avoid add overhead to check the
> option at every trap, we want to use the alternative to skip
> this check.
>
> In this series:
> 1. Introduce alternative patching support for ARM32.
> 2. Update the ARM32 SErrors handle code to use the alternative.

Stefano, this patch is relying on the bug fix "xen/arm: Register 
re-mapped Xen area as a temporary virtual region" 
<1489483637-27456-1-git-send-email-Wei.Chen@arm.com>. We should avoid to 
commit it before hand as it will break ARM platform.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 2/2] xen/arm32: Use alternative to skip the check of pending serrors

[Wei Chen]

Hi Julien,

On 2017/3/17 1:36, Julien Grall wrote:
> Hi Wei,
>
> On 03/16/2017 09:53 AM, Wei Chen wrote:
>> We have provided an option to administrator to determine how to
>> handle the SErrors. In order to skip the check of pending SError,
>> in conventional way, we have to read the option every time before
>> we try to check the pending SError. This will add overhead to check
>> the option at every trap.
>>
>> The ARM32 supports the alternative patching feature. We can use an
>> ALTERNATIVE to avoid checking option at every trap. We added a new
>> cpufeature named "SKIP_CHECK_PENDING_VSERROR". This feature will be
>> enabled when the option is not diverse.
>
> Can you please squash this patch in #10 of the SError series?
>

I will squash it to new version of SError series.

> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Wei Chen]

Hi Julien,

On 2017/3/17 6:24, Julien Grall wrote:
> Hi Wei,
>
> On 03/16/2017 09:53 AM, Wei Chen wrote:
>> This patch is based on the implementation of ARM64, it introduces
>> alternative runtime patching to ARM32. This allows to patch assembly
>> instruction at runtime to either fix hardware bugs or optimize for
>> certain hardware features on ARM32 platform.
>>
>> Xen hypervisor is using ARM execution state only on ARM32 platform,
>> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
>> TBB and TBH) are not considered in alternatives.
>>
>> The left ARM32 branch instructions are BX, BLX, BL and B. The
>> instruction BX is taking a register in parameter, so we don't need to
>> rewrite it. The instructions BLX, BL and B are using the similar
>> encoding for the offset and will avoid specific case when extracting
>> and updating the offset.
>>
>> In this patch, we include alternative.h header file to livepatch.c
>> directly for ARM32 compilation issues. When the alternative patching
>> config is enabled, the livepatch.c will use the alternative functions.
>> In this case, we should include the alternative header file to this
>> file. But for ARM64, it does not include this header file directly.
>> It includes this header file indirectly through:
>> sched.h->domain.h->page.h->alternative.h.
>> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
>> and we don't have the reason to include it to ARM32 page.h now. So we
>> have to include the alternative.h directly in livepatch.c.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@arm.com>
>> ---
>>  xen/arch/arm/Kconfig             |  2 +-
>>  xen/arch/arm/arm32/Makefile      |  1 +
>>  xen/arch/arm/arm32/insn.c        | 99 ++++++++++++++++++++++++++++++++++++++++
>>  xen/common/livepatch.c           |  1 +
>
> CC Ross and Konrad for the livepatch part.
>

Thanks.

>>  xen/include/asm-arm/arm32/insn.h | 57 +++++++++++++++++++++++
>>  xen/include/asm-arm/insn.h       |  2 +
>>  6 files changed, 161 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/arch/arm/arm32/insn.c
>>  create mode 100644 xen/include/asm-arm/arm32/insn.h
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 2e023d1..43123e6 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -12,11 +12,11 @@ config ARM_32
>>  config ARM_64
>>  	def_bool y
>>  	depends on 64BIT
>> -	select HAS_ALTERNATIVE
>>  	select HAS_GICV3
>>
>>  config ARM
>>  	def_bool y
>> +	select HAS_ALTERNATIVE
>>  	select HAS_ARM_HDLCD
>>  	select HAS_DEVICE_TREE
>>  	select HAS_MEM_ACCESS
>> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
>> index 4395693..0ac254f 100644
>> --- a/xen/arch/arm/arm32/Makefile
>> +++ b/xen/arch/arm/arm32/Makefile
>> @@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
>>  obj-y += domctl.o
>>  obj-y += domain.o
>>  obj-y += entry.o
>> +obj-y += insn.o
>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>>  obj-y += proc-v7.o proc-caxx.o
>>  obj-y += smpboot.o
>> diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
>> new file mode 100644
>> index 0000000..91a3010
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/insn.c
>> @@ -0,0 +1,99 @@
>> +/*
>> +  * 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/>.
>> +  */
>> +#include <xen/lib.h>
>> +#include <xen/bitops.h>
>> +#include <xen/sizes.h>
>> +#include <asm/bug.h>
>
> This is already included by xen/lib.h.
>

I will remove it.

>> +#include <asm/insn.h>
>> +
>> +/* Mask of branch instructions' immediate. */
>> +#define BRANCH_INSN_IMM_MASK    GENMASK(23, 0)
>> +/* Shift of branch instructions' immediate. */
>> +#define BRANCH_INSN_IMM_SHIFT   0
>> +
>> +static uint32_t branch_insn_encode_immediate(uint32_t insn, int32_t offset)
>> +{
>> +    uint32_t imm;
>> +
>> +    /*
>> +     * Encode the offset to imm. All ARM32 instructions must be word aligned.
>> +     * Therefore the offset value's bits [1:0] equal to zero.
>> +     * (see ARM DDI 0406C.b A8.8.18/A8.8.25 for more encode/decode details
>
> DDI 0406C.b is an old version (July 2012) of the ARM ARM. Please try to
> use the latest version (e.g 0406C.c released in May 2014) when possible.
>

Ok, I will update the referred document.

>> +     * about ARM32 branch instructions)
>> +     */
>> +    imm = ((offset >> 2) & BRANCH_INSN_IMM_MASK) << BRANCH_INSN_IMM_SHIFT;
>> +
>> +    /* Update the immediate field. */
>> +    insn &= ~(BRANCH_INSN_IMM_MASK << BRANCH_INSN_IMM_SHIFT);
>> +    insn |= imm;
>> +
>> +    return insn;
>> +}
>> +
>> +/*
>> + * 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.

About the message can we change to:
"Target ARM32 instructions must be placed on word aligned addresses?"

>> +        return BUG_OPCODE;
>> +    }
>> +
>> +    /* B/BL support [-32M, 32M) offset (see ARM DDI 0406C.b A4.3). */
>> +    if ( offset < -SZ_32M || offset >= SZ_32M )
>> +    {
>> +        printk(XENLOG_ERR
>> +               "%s: new branch offset out of range.\n", __func__);
>> +        return BUG_OPCODE;
>> +    }
>> +
>> +    return branch_insn_encode_immediate(insn, offset);
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>> index 246e673..f14bcbc 100644
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -25,6 +25,7 @@
>>  #include <xen/livepatch.h>
>>  #include <xen/livepatch_payload.h>
>>
>> +#include <asm/alternative.h>
>>  #include <asm/event.h>
>>
>>  /*
>> 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?

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.

>> +__AARCH32_INSN_FUNCS(bl, 0x0F000000, 0x0B000000)
>> +
>> +int32_t aarch32_get_branch_offset(uint32_t insn);
>> +uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset);
>> +
>> +/* Wrapper for common code */
>> +static inline bool insn_is_branch_imm(uint32_t insn)
>> +{
>> +    return ( aarch32_insn_is_b(insn) || aarch32_insn_is_bl(insn) );
>> +}
>> +
>> +static inline int32_t insn_get_branch_offset(uint32_t insn)
>> +{
>> +    return aarch32_get_branch_offset(insn);
>> +}
>> +
>> +static inline uint32_t insn_set_branch_offset(uint32_t insn, int32_t offset)
>> +{
>> +    return aarch32_set_branch_offset(insn, offset);
>> +}
>> +
>> +#endif /* !__ARCH_ARM_ARM32_INSN */
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
>> index a205ceb..3489179 100644
>> --- a/xen/include/asm-arm/insn.h
>> +++ b/xen/include/asm-arm/insn.h
>> @@ -5,6 +5,8 @@
>>
>>  #if defined(CONFIG_ARM_64)
>>  # include <asm/arm64/insn.h>
>> +#elif defined(CONFIG_ARM_32)
>> +# include <asm/arm32/insn.h>
>>  #else
>>  # error "unknown ARM variant"
>>  #endif
>>
>
> Cheers,
>


-- 
Regards,
Wei Chen

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Konrad Rzeszutek Wilk]

On Thu, Mar 16, 2017 at 05:53:38PM +0800, Wei Chen wrote:
> This patch is based on the implementation of ARM64, it introduces
> alternative runtime patching to ARM32. This allows to patch assembly
> instruction at runtime to either fix hardware bugs or optimize for
> certain hardware features on ARM32 platform.
> 
> Xen hypervisor is using ARM execution state only on ARM32 platform,
> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
> TBB and TBH) are not considered in alternatives.
> 
> The left ARM32 branch instructions are BX, BLX, BL and B. The
> instruction BX is taking a register in parameter, so we don't need to
> rewrite it. The instructions BLX, BL and B are using the similar
> encoding for the offset and will avoid specific case when extracting
> and updating the offset.
> 
> In this patch, we include alternative.h header file to livepatch.c
> directly for ARM32 compilation issues. When the alternative patching
> config is enabled, the livepatch.c will use the alternative functions.
> In this case, we should include the alternative header file to this
> file. But for ARM64, it does not include this header file directly.
> It includes this header file indirectly through:
> sched.h->domain.h->page.h->alternative.h.
> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
> and we don't have the reason to include it to ARM32 page.h now. So we
> have to include the alternative.h directly in livepatch.c.

OK, thanks for the explanation.


Could you also confirm that the test-cases for livepatching
does compile for you?

make -C xen tests

should do it (specifically I am curious if xen_hello_world_func.c
compiles fine).

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Wei Chen]

Hi Konrad,

On 2017/3/17 22:34, Konrad Rzeszutek Wilk wrote:
> On Thu, Mar 16, 2017 at 05:53:38PM +0800, Wei Chen wrote:
>> This patch is based on the implementation of ARM64, it introduces
>> alternative runtime patching to ARM32. This allows to patch assembly
>> instruction at runtime to either fix hardware bugs or optimize for
>> certain hardware features on ARM32 platform.
>>
>> Xen hypervisor is using ARM execution state only on ARM32 platform,
>> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
>> TBB and TBH) are not considered in alternatives.
>>
>> The left ARM32 branch instructions are BX, BLX, BL and B. The
>> instruction BX is taking a register in parameter, so we don't need to
>> rewrite it. The instructions BLX, BL and B are using the similar
>> encoding for the offset and will avoid specific case when extracting
>> and updating the offset.
>>
>> In this patch, we include alternative.h header file to livepatch.c
>> directly for ARM32 compilation issues. When the alternative patching
>> config is enabled, the livepatch.c will use the alternative functions.
>> In this case, we should include the alternative header file to this
>> file. But for ARM64, it does not include this header file directly.
>> It includes this header file indirectly through:
>> sched.h->domain.h->page.h->alternative.h.
>> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
>> and we don't have the reason to include it to ARM32 page.h now. So we
>> have to include the alternative.h directly in livepatch.c.
>
> OK, thanks for the explanation.
>
>
> Could you also confirm that the test-cases for livepatching
> does compile for you?
>
> make -C xen tests
>
> should do it (specifically I am curious if xen_hello_world_func.c
> compiles fine).
>

Yes, the test-cases for livepatching can compile successfully.
I paste the full compiling log below:


arm32@P300:~/X32/Xen32$ make -C xen tests
make: Entering directory '/home/arm32/X32/Xen32/xen'
make -f Rules.mk _tests
make[1]: Entering directory '/home/arm32/X32/Xen32/xen'
make -f /home/arm32/X32/Xen32/xen/Rules.mk -C test tests
make[2]: Entering directory '/home/arm32/X32/Xen32/xen/test'
make -f /home/arm32/X32/Xen32/xen/Rules.mk -C livepatch livepatch
make[3]: Entering directory '/home/arm32/X32/Xen32/xen/test/livepatch'
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_hello_world_func.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_hello_world_func.o.d 
-msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_hello_world_func.c -o 
xen_hello_world_func.o
(set -e; \
  echo "#define NEW_CODE_SZ 0x00000020"; \
  echo "#define MINOR_VERSION_SZ 0x00000018"; \
  echo "#define MINOR_VERSION_ADDR 0x0023e580"; \
  echo "#define OLD_CODE_SZ 0x0000001c") > config.h
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_hello_world.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_hello_world.o.d -msoft-float 
-mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_hello_world.c -o xen_hello_world.o
arm-linux-gnueabihf-objcopy -O binary --only-section=.note.gnu.build-id 
/home/arm32/X32/Xen32/xen/xen-syms note.o.bin
arm-linux-gnueabihf-objcopy -I binary -O elf32-littlearm -B arm \
 
--rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents 
-S note.o.bin note.o
rm -f note.o.bin
arm-linux-gnueabihf-ld    -EL -EL     --build-id=sha1 -r -o 
xen_hello_world.livepatch xen_hello_world_func.o xen_hello_world.o note.o
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_bye_world_func.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_bye_world_func.o.d -msoft-float 
-mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_bye_world_func.c -o xen_bye_world_func.o
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float 
-mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_bye_world.c -o xen_bye_world.o
arm-linux-gnueabihf-objcopy -O binary --only-section=.note.gnu.build-id 
xen_hello_world.livepatch hello_world_note.o.bin
arm-linux-gnueabihf-objcopy -I binary -O elf32-littlearm -B arm \
 
--rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents 
-S hello_world_note.o.bin hello_world_note.o
rm -f hello_world_note.o.bin
arm-linux-gnueabihf-ld    -EL -EL     --build-id=sha1 -r -o 
xen_bye_world.livepatch xen_bye_world_func.o xen_bye_world.o 
hello_world_note.o
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_replace_world_func.o"' 
-Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF 
./.xen_replace_world_func.o.d -msoft-float -mcpu=cortex-a15 
-DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" 
-DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 
-DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include 
-fno-stack-protector -fno-exceptions -Wnested-externs 
-DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing 
-std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -c 
xen_replace_world_func.c -o xen_replace_world_func.o
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_replace_world.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_replace_world.o.d -msoft-float 
-mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_replace_world.c -o xen_replace_world.o
arm-linux-gnueabihf-ld    -EL -EL     --build-id=sha1 -r -o 
xen_replace_world.livepatch xen_replace_world_func.o xen_replace_world.o 
note.o
arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 
-Wall -Wstrict-prototypes -Wdeclaration-after-statement 
-Wno-unused-but-set-variable -Wno-unused-local-typedefs   -O1 -nostdinc 
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith 
-pipe -g -D__XEN__ -include 
/home/arm32/X32/Xen32/xen/include/xen/config.h 
'-D__OBJECT_FILE__="xen_nop.o"' -Wa,--strip-local-absolute 
-fno-omit-frame-pointer -MMD -MF ./.xen_nop.o.d -msoft-float 
-mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK 
-DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= 
-DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= 
-I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions 
-Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID 
-fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes 
-Wdeclaration-after-statement -Wno-unused-but-set-variable 
-Wno-unused-local-typedefs   -c xen_nop.c -o xen_nop.o
arm-linux-gnueabihf-ld    -EL -EL     --build-id=sha1 -r -o 
xen_nop.livepatch xen_nop.o note.o
make[3]: Leaving directory '/home/arm32/X32/Xen32/xen/test/livepatch'
make[2]: Leaving directory '/home/arm32/X32/Xen32/xen/test'
make[1]: Leaving directory '/home/arm32/X32/Xen32/xen'
make: Leaving directory '/home/arm32/X32/Xen32/xen'


-- 
Regards,
Wei Chen

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

Re: [PATCH 0/2] xen/arm32: Use alternative to skip the check of pending serrors

[Stefano Stabellini]

On Thu, 16 Mar 2017, Julien Grall wrote:
> Hi,
> 
> On 03/16/2017 09:53 AM, Wei Chen wrote:
> > We have provided an option to administrator to determine how to
> > handle the SErrors. In order to avoid add overhead to check the
> > option at every trap, we want to use the alternative to skip
> > this check.
> > 
> > In this series:
> > 1. Introduce alternative patching support for ARM32.
> > 2. Update the ARM32 SErrors handle code to use the alternative.
> 
> Stefano, this patch is relying on the bug fix "xen/arm: Register re-mapped Xen
> area as a temporary virtual region"
> <1489483637-27456-1-git-send-email-Wei.Chen@arm.com>. We should avoid to
> commit it before hand as it will break ARM platform.

Thanks for the head's up.

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Konrad Rzeszutek Wilk]

On Mon, Mar 20, 2017 at 06:45:18AM +0000, Wei Chen wrote:
> Hi Konrad,
> 
> On 2017/3/17 22:34, Konrad Rzeszutek Wilk wrote:
> > On Thu, Mar 16, 2017 at 05:53:38PM +0800, Wei Chen wrote:
> >> This patch is based on the implementation of ARM64, it introduces
> >> alternative runtime patching to ARM32. This allows to patch assembly
> >> instruction at runtime to either fix hardware bugs or optimize for
> >> certain hardware features on ARM32 platform.
> >>
> >> Xen hypervisor is using ARM execution state only on ARM32 platform,
> >> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
> >> TBB and TBH) are not considered in alternatives.
> >>
> >> The left ARM32 branch instructions are BX, BLX, BL and B. The
> >> instruction BX is taking a register in parameter, so we don't need to
> >> rewrite it. The instructions BLX, BL and B are using the similar
> >> encoding for the offset and will avoid specific case when extracting
> >> and updating the offset.
> >>
> >> In this patch, we include alternative.h header file to livepatch.c
> >> directly for ARM32 compilation issues. When the alternative patching
> >> config is enabled, the livepatch.c will use the alternative functions.
> >> In this case, we should include the alternative header file to this
> >> file. But for ARM64, it does not include this header file directly.
> >> It includes this header file indirectly through:
> >> sched.h->domain.h->page.h->alternative.h.
> >> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
> >> and we don't have the reason to include it to ARM32 page.h now. So we
> >> have to include the alternative.h directly in livepatch.c.
> >
> > OK, thanks for the explanation.
> >
> >
> > Could you also confirm that the test-cases for livepatching
> > does compile for you?
> >
> > make -C xen tests
> >
> > should do it (specifically I am curious if xen_hello_world_func.c
> > compiles fine).
> >
> 
> Yes, the test-cases for livepatching can compile successfully.

Fantastic!
> I paste the full compiling log below:

No need for that but thank you!

Pls add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>'
on the patch.

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Julien Grall]

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

Re: [PATCH 1/2] xen/arm32: Introduce alternative runtime patching

[Wei Chen]

Hi Julien,

On 2017/3/24 18:48, Julien Grall wrote:
> 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.
>

Ok, it seems we'd better to remove this pointless check.

>>
>> About the message can we change to:
>> "Target ARM32 instructions must be placed on word aligned addresses?"
>
> That would be ok.

If so, we don't need this message any more.

>
>
> [...]
>
>>>>  /*
>>>> 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
>

Yes, I re-checked the list, you're right. I will do it in next version.

> Cheers,
>


-- 
Regards,
Wei Chen

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