[PATCH v4 0/2] Implement errata 766422

[PATCH v4 0/2] Implement errata 766422

[Julien Grall]

Hi,

This is fourth version of this patch series. It allows a guest kernel to
use THUMB set instructions on the processor affected by the errata 766422 (for
instance the Arndale board).

This patch series is divided in two patch:
    - The first patch implements a basic decoder to fill the ISS field
      of the hsr_dabt struct
    - The second patch implements the errata

Since the last version, only the patch #1 has been modified. See the changes in
the patch.

Julien Grall (2):
  xen/arm: Start to implement an ARM decoder instruction
  xen/arm: errata 766422: decode thumb store during data abort

 xen/arch/arm/Makefile                 |    1 +
 xen/arch/arm/decode.c                 |  168 +++++++++++++++++++++++++++++++++
 xen/arch/arm/decode.h                 |   49 ++++++++++
 xen/arch/arm/traps.c                  |   12 +++
 xen/include/asm-arm/arm32/processor.h |    4 +
 xen/include/asm-arm/arm64/processor.h |    2 +
 6 files changed, 236 insertions(+)
 create mode 100644 xen/arch/arm/decode.c
 create mode 100644 xen/arch/arm/decode.h

-- 
1.7.10.4

[PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction

[Julien Grall]

Some erratas on ARM processor requires to decode the instruction.
The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt.

For the moment, the decoder only supports:
    - THUMB2 store instruction
    - THUMB single load/store instruction

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v4:
        - Add warning about the purpose of this function
        - Add helper to update ISS (register, sign, size) field of DABT
        - Improve decoding for THUMB 2 store instruction
        - Only decode thumb instruction if it's a 32-bit guest
---
 xen/arch/arm/Makefile |    1 +
 xen/arch/arm/decode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/decode.h |   49 +++++++++++++++
 3 files changed, 218 insertions(+)
 create mode 100644 xen/arch/arm/decode.c
 create mode 100644 xen/arch/arm/decode.h

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 5ae5831..5c13a65 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -30,6 +30,7 @@ obj-y += vtimer.o
 obj-y += vpl011.o
 obj-y += hvm.o
 obj-y += device.o
+obj-y += decode.o
 
 #obj-bin-y += ....o
 
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
new file mode 100644
index 0000000..0e7d8ba
--- /dev/null
+++ b/xen/arch/arm/decode.c
@@ -0,0 +1,168 @@
+/*
+ * xen/arch/arm/decode.c
+ *
+ * Instruction decoder
+ *
+ * Julien Grall <julien.grall@linaro.org>
+ * Copyright (C) 2013 Linaro Limited.
+ *
+ * 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.
+ *
+ * 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/types.h>
+#include <xen/sched.h>
+#include <asm/current.h>
+#include <asm/guest_access.h>
+#include <xen/lib.h>
+
+#include "decode.h"
+
+static void update_dabt(struct hsr_dabt *dabt, int reg,
+                        uint8_t size, bool_t sign)
+{
+    dabt->reg = reg;
+    dabt->size = size;
+    dabt->sign = sign;
+}
+
+/* TODO: Handle all THUMB2 instruction other than simple store */
+static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
+{
+    uint16_t hw2;
+    int rc;
+    uint16_t rt;
+
+    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
+    if ( rc )
+        return rc;
+
+    rt = (hw2 >> 12) & 0x7;
+
+    switch ( (hw1 >> 9) & 0xf )
+    {
+    case 12:
+    {
+        bool_t sign = !!(hw1 & (1 << 8));
+        bool_t load = !!(hw1 & (1 << 4));
+
+        if ( (hw1 & 0x0110) == 0x0100 )
+            /* NEON instruction */
+            goto bad_thumb2;
+
+        if ( (hw1 & 0x0070) == 0x0070 )
+            /* Undefined opcodes */
+            goto bad_thumb2;
+
+        /* Store/Load single data item */
+        if ( rt == 15 )
+            /* XXX: Rt == 15 is only invalid for store instruction */
+            goto bad_thumb2;
+
+        if ( !load && sign )
+            /* Store instruction doesn't support sign extension */
+            goto bad_thumb2;
+
+        update_dabt(dabt, rt, (hw1 >> 5) & 3, sign);
+
+        break;
+    }
+    default:
+        goto bad_thumb2;
+    }
+
+    return 0;
+
+bad_thumb2:
+    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
+           current->domain->domain_id, hw1, hw2);
+
+    return 1;
+}
+
+/* TODO: Handle all THUMB instructions other than store */
+static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
+{
+    uint16_t instr;
+    int rc;
+
+    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
+    if ( rc )
+        return rc;
+
+    switch ( instr >> 12 )
+    {
+    case 5:
+    {
+        /* Load/Store register */
+        uint16_t opB = (instr >> 9) & 0x7;
+        int reg = instr & 7;
+
+        switch ( opB & 0x3 )
+        {
+        case 0: /* Non-signed word */
+            update_dabt(dabt, reg, 2, 0);
+            break;
+        case 1: /* Non-signed halfword */
+            update_dabt(dabt, reg, 1, 0);
+            break;
+        case 2: /* Non-signed byte */
+            update_dabt(dabt, reg, 0, 0);
+            break;
+        case 3: /* Signed byte */
+            update_dabt(dabt, reg, 0, 1);
+            break;
+        }
+
+        break;
+    }
+    case 6:
+        /* Load/Store word immediate offset */
+        update_dabt(dabt, instr & 7, 2, 0);
+        break;
+    case 7:
+        /* Load/Store byte immediate offset */
+        update_dabt(dabt, instr & 7, 0, 0);
+        break;
+    case 8:
+        /* Load/Store halfword immediate offset */
+        update_dabt(dabt, instr & 7, 1, 0);
+        break;
+    case 9:
+        /* Load/Store word sp offset */
+        update_dabt(dabt, (instr >> 8) & 7, 2, 0);
+        break;
+    case 14:
+        if ( instr & (1 << 11) )
+            return decode_thumb2(pc, dabt, instr);
+        goto bad_thumb;
+    case 15:
+        return decode_thumb2(pc, dabt, instr);
+    default:
+        goto bad_thumb;
+    }
+
+    return 0;
+
+bad_thumb:
+    printk("DOM%u: unhandled THUMB instruction 0x%x\n",
+           current->domain->domain_id, instr);
+    return 1;
+}
+
+int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
+{
+    if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB )
+        return decode_thumb(regs->pc, dabt);
+
+    /* TODO: Handle ARM instruction */
+
+    return 1;
+}
diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
new file mode 100644
index 0000000..4613763
--- /dev/null
+++ b/xen/arch/arm/decode.h
@@ -0,0 +1,49 @@
+/*
+ * xen/arch/arm/decode.h
+ *
+ * Instruction decoder
+ *
+ * Julien Grall <julien.grall@linaro.org>
+ * Copyright (C) 2013 Linaro Limited.
+ *
+ * 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.
+ *
+ * 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 __ARCH_ARM_DECODE_H_
+#define __ARCH_ARM_DECODE_H_
+
+#include <asm/regs.h>
+#include <asm/processor.h>
+
+/**
+ * Decode an instruction from pc
+ * /!\ This function is not intended to fully decode an instruction. It
+ * considers that the instruction is valid.
+ *
+ * This function will get:
+ *  - The transfer register
+ *  - Sign bit
+ *  - Size
+ */
+
+int decode_instruction(const struct cpu_user_regs *regs,
+                       struct hsr_dabt *dabt);
+
+#endif /* __ARCH_ARM_DECODE_H_ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
1.7.10.4

[PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort

[Julien Grall]

From the errata document:

When a non-secure non-hypervisor memory operation instruction generates a
stage2 page table translation fault, a trap to the hypervisor will be triggered.
For an architecturally defined subset of instructions, the Hypervisor Syndrome
Register (HSR) will have the Instruction Syndrome Valid (ISV) bit set to 1’b1,
and the Rt field should reflect the source register (for stores) or destination
register for loads.
On Cortex-A15, for Thumb and ThumbEE stores, the Rt value may be incorrect
and should not be used, even if the ISV bit is set. All loads, and all ARM
instruction set loads and stores, will have the correct Rt value if the ISV
bit is set.

To avoid this issue, Xen needs to decode thumb store instruction and update
the transfer register.

Signed-off-by: Julien Grall <julien.grall@linaro.org>

---
    Changes in v3:
        - Use unlikely to check if the processor is affected by the errata
        - Move the decoder in another patch and use it
    Changes in v2:
        - Only decode the instruction on affected processor
        - Handle ARM 32-bit instruction in read_instruction
---
 xen/arch/arm/traps.c                  |   12 ++++++++++++
 xen/include/asm-arm/arm32/processor.h |    4 ++++
 xen/include/asm-arm/arm64/processor.h |    2 ++
 3 files changed, 18 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 6b5fa51..47e9ba4 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -36,6 +36,7 @@
 #include <asm/cpregs.h>
 #include <asm/psci.h>
 
+#include "decode.h"
 #include "io.h"
 #include "vtimer.h"
 #include <asm/gic.h>
@@ -1336,6 +1337,17 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
     if ( !dabt.valid )
         goto bad_data_abort;
 
+    /*
+     * Errata 766422: Thumb store translation fault to Hypervisor may
+     * not have correct HSR Rt value.
+     */
+    if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
+    {
+        rc = decode_instruction(regs, &info.dabt);
+        if ( rc )
+            goto bad_data_abort;
+    }
+
     if (handle_mmio(&info))
     {
         advance_pc(regs, hsr);
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index b266252..a21d104 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -111,6 +111,10 @@ struct cpu_user_regs
 #define READ_SYSREG(R...)       READ_SYSREG32(R)
 #define WRITE_SYSREG(V, R...)   WRITE_SYSREG32(V, R)
 
+/* Errata 766422: only Cortex A15 r0p4 is affected */
+#define cpu_has_errata_766422()                             \
+    (unlikely(current_cpu_data.midr.bits == 0x410fc0f4))
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ARM_ARM32_PROCESSOR_H */
diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h
index c8d4609..5d9b952 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -104,6 +104,8 @@ struct cpu_user_regs
 #define READ_SYSREG(name)     READ_SYSREG64(name)
 #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name)
 
+#define cpu_has_errata_766422() 0
+
 #endif /* __ASSEMBLY__ */
 
 #endif /* __ASM_ARM_ARM64_PROCESSOR_H */
-- 
1.7.10.4


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

Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction

[Ian Campbell]

On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
> Some erratas on ARM processor requires to decode the instruction.

Errata is already the plural of Erratum, I think.

> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt.
> 
> For the moment, the decoder only supports:
>     - THUMB2 store instruction
>     - THUMB single load/store instruction
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v4:
>         - Add warning about the purpose of this function
>         - Add helper to update ISS (register, sign, size) field of DABT
>         - Improve decoding for THUMB 2 store instruction
>         - Only decode thumb instruction if it's a 32-bit guest
> ---
>  xen/arch/arm/Makefile |    1 +
>  xen/arch/arm/decode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/decode.h |   49 +++++++++++++++
>  3 files changed, 218 insertions(+)
>  create mode 100644 xen/arch/arm/decode.c
>  create mode 100644 xen/arch/arm/decode.h
> 
> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> index 5ae5831..5c13a65 100644
> --- a/xen/arch/arm/Makefile
> +++ b/xen/arch/arm/Makefile
> @@ -30,6 +30,7 @@ obj-y += vtimer.o
>  obj-y += vpl011.o
>  obj-y += hvm.o
>  obj-y += device.o
> +obj-y += decode.o
>  
>  #obj-bin-y += ....o
>  
> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
> new file mode 100644
> index 0000000..0e7d8ba
> --- /dev/null
> +++ b/xen/arch/arm/decode.c
> @@ -0,0 +1,168 @@
> +/*
> + * xen/arch/arm/decode.c
> + *
> + * Instruction decoder
> + *
> + * Julien Grall <julien.grall@linaro.org>
> + * Copyright (C) 2013 Linaro Limited.
> + *
> + * 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.
> + *
> + * 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/types.h>
> +#include <xen/sched.h>
> +#include <asm/current.h>
> +#include <asm/guest_access.h>
> +#include <xen/lib.h>
> +
> +#include "decode.h"
> +
> +static void update_dabt(struct hsr_dabt *dabt, int reg,
> +                        uint8_t size, bool_t sign)
> +{
> +    dabt->reg = reg;
> +    dabt->size = size;
> +    dabt->sign = sign;
> +}
> +
> +/* TODO: Handle all THUMB2 instruction other than simple store */

I think we decided to remove this (and the other one).
> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
> +{
> +    uint16_t hw2;
> +    int rc;
> +    uint16_t rt;
> +
> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
> +    if ( rc )
> +        return rc;
> +
> +    rt = (hw2 >> 12) & 0x7;
> +
> +    switch ( (hw1 >> 9) & 0xf )
> +    {
> +    case 12:
> +    {
> +        bool_t sign = !!(hw1 & (1 << 8));
> +        bool_t load = !!(hw1 & (1 << 4));

!! is unneeded with the bool type I think.

> +
> +        if ( (hw1 & 0x0110) == 0x0100 )
> +            /* NEON instruction */
> +            goto bad_thumb2;
> +
> +        if ( (hw1 & 0x0070) == 0x0070 )
> +            /* Undefined opcodes */
> +            goto bad_thumb2;
> +
> +        /* Store/Load single data item */
> +        if ( rt == 15 )
> +            /* XXX: Rt == 15 is only invalid for store instruction */

So the check should be "rt == 15 && load" ?

> +            goto bad_thumb2;
> +
> +        if ( !load && sign )
> +            /* Store instruction doesn't support sign extension */
> +            goto bad_thumb2;
> +
> +        update_dabt(dabt, rt, (hw1 >> 5) & 3, sign);
> +
> +        break;
> +    }
> +    default:
> +        goto bad_thumb2;
> +    }
> +
> +    return 0;
> +
> +bad_thumb2:
> +    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
> +           current->domain->domain_id, hw1, hw2);

Should this be gdprintk or gprintk?

> +
> +    return 1;
> +}
> +
> +/* TODO: Handle all THUMB instructions other than store */
> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
> +{
> +    uint16_t instr;
> +    int rc;
> +
> +    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
> +    if ( rc )
> +        return rc;
> +
> +    switch ( instr >> 12 )
> +    {
> +    case 5:
> +    {
> +        /* Load/Store register */
> +        uint16_t opB = (instr >> 9) & 0x7;
> +        int reg = instr & 7;
> +
> +        switch ( opB & 0x3 )
> +        {
> +        case 0: /* Non-signed word */
> +            update_dabt(dabt, reg, 2, 0);
> +            break;
> +        case 1: /* Non-signed halfword */
> +            update_dabt(dabt, reg, 1, 0);
> +            break;
> +        case 2: /* Non-signed byte */
> +            update_dabt(dabt, reg, 0, 0);
> +            break;
> +        case 3: /* Signed byte */
> +            update_dabt(dabt, reg, 0, 1);
> +            break;
> +        }
> +
> +        break;
> +    }
> +    case 6:
> +        /* Load/Store word immediate offset */
> +        update_dabt(dabt, instr & 7, 2, 0);
> +        break;
> +    case 7:
> +        /* Load/Store byte immediate offset */
> +        update_dabt(dabt, instr & 7, 0, 0);
> +        break;
> +    case 8:
> +        /* Load/Store halfword immediate offset */
> +        update_dabt(dabt, instr & 7, 1, 0);
> +        break;
> +    case 9:
> +        /* Load/Store word sp offset */
> +        update_dabt(dabt, (instr >> 8) & 7, 2, 0);
> +        break;
> +    case 14:
> +        if ( instr & (1 << 11) )
> +            return decode_thumb2(pc, dabt, instr);
> +        goto bad_thumb;
> +    case 15:
> +        return decode_thumb2(pc, dabt, instr);
> +    default:
> +        goto bad_thumb;
> +    }
> +
> +    return 0;
> +
> +bad_thumb:
> +    printk("DOM%u: unhandled THUMB instruction 0x%x\n",
> +           current->domain->domain_id, instr);

gdprint etc?

> +    return 1;
> +}
> +
> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
> +{
> +    if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB )
> +        return decode_thumb(regs->pc, dabt);
> +
> +    /* TODO: Handle ARM instruction */

You log unhandled thumb instructions but not ARM one, I think you should
log here too.

> +
> +    return 1;
> +}
> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> new file mode 100644
> index 0000000..4613763
> --- /dev/null
> +++ b/xen/arch/arm/decode.h
> @@ -0,0 +1,49 @@
> +/*
> + * xen/arch/arm/decode.h
> + *
> + * Instruction decoder
> + *
> + * Julien Grall <julien.grall@linaro.org>
> + * Copyright (C) 2013 Linaro Limited.
> + *
> + * 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.
> + *
> + * 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 __ARCH_ARM_DECODE_H_
> +#define __ARCH_ARM_DECODE_H_
> +
> +#include <asm/regs.h>
> +#include <asm/processor.h>
> +
> +/**
> + * Decode an instruction from pc
> + * /!\ This function is not intended to fully decode an instruction. It
> + * considers that the instruction is valid.
> + *
> + * This function will get:
> + *  - The transfer register
> + *  - Sign bit
> + *  - Size
> + */
> +
> +int decode_instruction(const struct cpu_user_regs *regs,
> +                       struct hsr_dabt *dabt);
> +
> +#endif /* __ARCH_ARM_DECODE_H_ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort

[Ian Campbell]

On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
> +    /*
> +     * Errata 766422: Thumb store translation fault to Hypervisor may
> +     * not have correct HSR Rt value.
> +     */
> +    if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
> +    {
> +        rc = decode_instruction(regs, &info.dabt);
> +        if ( rc )
> +            goto bad_data_abort;

Consider logging here so we know which caller of decode_instruction
failed?

Either way:

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.

Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction

[Julien Grall]

On 08/08/2013 12:08 PM, Ian Campbell wrote:
> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
>> Some erratas on ARM processor requires to decode the instruction.
> 
> Errata is already the plural of Erratum, I think.

Right, I will fix it.

> 
>> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt.
>>
>> For the moment, the decoder only supports:
>>     - THUMB2 store instruction
>>     - THUMB single load/store instruction
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v4:
>>         - Add warning about the purpose of this function
>>         - Add helper to update ISS (register, sign, size) field of DABT
>>         - Improve decoding for THUMB 2 store instruction
>>         - Only decode thumb instruction if it's a 32-bit guest
>> ---
>>  xen/arch/arm/Makefile |    1 +
>>  xen/arch/arm/decode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/decode.h |   49 +++++++++++++++
>>  3 files changed, 218 insertions(+)
>>  create mode 100644 xen/arch/arm/decode.c
>>  create mode 100644 xen/arch/arm/decode.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 5ae5831..5c13a65 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -30,6 +30,7 @@ obj-y += vtimer.o
>>  obj-y += vpl011.o
>>  obj-y += hvm.o
>>  obj-y += device.o
>> +obj-y += decode.o
>>  
>>  #obj-bin-y += ....o
>>  
>> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
>> new file mode 100644
>> index 0000000..0e7d8ba
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * xen/arch/arm/decode.c
>> + *
>> + * Instruction decoder
>> + *
>> + * Julien Grall <julien.grall@linaro.org>
>> + * Copyright (C) 2013 Linaro Limited.
>> + *
>> + * 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.
>> + *
>> + * 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/types.h>
>> +#include <xen/sched.h>
>> +#include <asm/current.h>
>> +#include <asm/guest_access.h>
>> +#include <xen/lib.h>
>> +
>> +#include "decode.h"
>> +
>> +static void update_dabt(struct hsr_dabt *dabt, int reg,
>> +                        uint8_t size, bool_t sign)
>> +{
>> +    dabt->reg = reg;
>> +    dabt->size = size;
>> +    dabt->sign = sign;
>> +}
>> +
>> +/* TODO: Handle all THUMB2 instruction other than simple store */
> 
> I think we decided to remove this (and the other one).

Oh right, I forgot to remove it.

>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
>> +{
>> +    uint16_t hw2;
>> +    int rc;
>> +    uint16_t rt;
>> +
>> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    rt = (hw2 >> 12) & 0x7;
>> +
>> +    switch ( (hw1 >> 9) & 0xf )
>> +    {
>> +    case 12:
>> +    {
>> +        bool_t sign = !!(hw1 & (1 << 8));
>> +        bool_t load = !!(hw1 & (1 << 4));
> 
> !! is unneeded with the bool type I think.

I was not sure, I will remove the '!!'.

>> +
>> +        if ( (hw1 & 0x0110) == 0x0100 )
>> +            /* NEON instruction */
>> +            goto bad_thumb2;
>> +
>> +        if ( (hw1 & 0x0070) == 0x0070 )
>> +            /* Undefined opcodes */
>> +            goto bad_thumb2;
>> +
>> +        /* Store/Load single data item */
>> +        if ( rt == 15 )
>> +            /* XXX: Rt == 15 is only invalid for store instruction */
> 
> So the check should be "rt == 15 && load" ?

No, when rt == 15 the instructions are memory hints (pld, pldw,..) and
the layout is different.

>> +            goto bad_thumb2;
>> +
>> +        if ( !load && sign )
>> +            /* Store instruction doesn't support sign extension */
>> +            goto bad_thumb2;
>> +
>> +        update_dabt(dabt, rt, (hw1 >> 5) & 3, sign);
>> +
>> +        break;
>> +    }
>> +    default:
>> +        goto bad_thumb2;
>> +    }
>> +
>> +    return 0;
>> +
>> +bad_thumb2:
>> +    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
>> +           current->domain->domain_id, hw1, hw2);
> 
> Should this be gdprintk or gprintk?

I will use gdprintk.

> 
>> +
>> +    return 1;
>> +}
>> +
>> +/* TODO: Handle all THUMB instructions other than store */
>> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>> +{
>> +    uint16_t instr;
>> +    int rc;
>> +
>> +    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    switch ( instr >> 12 )
>> +    {
>> +    case 5:
>> +    {
>> +        /* Load/Store register */
>> +        uint16_t opB = (instr >> 9) & 0x7;
>> +        int reg = instr & 7;
>> +
>> +        switch ( opB & 0x3 )
>> +        {
>> +        case 0: /* Non-signed word */
>> +            update_dabt(dabt, reg, 2, 0);
>> +            break;
>> +        case 1: /* Non-signed halfword */
>> +            update_dabt(dabt, reg, 1, 0);
>> +            break;
>> +        case 2: /* Non-signed byte */
>> +            update_dabt(dabt, reg, 0, 0);
>> +            break;
>> +        case 3: /* Signed byte */
>> +            update_dabt(dabt, reg, 0, 1);
>> +            break;
>> +        }
>> +
>> +        break;
>> +    }
>> +    case 6:
>> +        /* Load/Store word immediate offset */
>> +        update_dabt(dabt, instr & 7, 2, 0);
>> +        break;
>> +    case 7:
>> +        /* Load/Store byte immediate offset */
>> +        update_dabt(dabt, instr & 7, 0, 0);
>> +        break;
>> +    case 8:
>> +        /* Load/Store halfword immediate offset */
>> +        update_dabt(dabt, instr & 7, 1, 0);
>> +        break;
>> +    case 9:
>> +        /* Load/Store word sp offset */
>> +        update_dabt(dabt, (instr >> 8) & 7, 2, 0);
>> +        break;
>> +    case 14:
>> +        if ( instr & (1 << 11) )
>> +            return decode_thumb2(pc, dabt, instr);
>> +        goto bad_thumb;
>> +    case 15:
>> +        return decode_thumb2(pc, dabt, instr);
>> +    default:
>> +        goto bad_thumb;
>> +    }
>> +
>> +    return 0;
>> +
>> +bad_thumb:
>> +    printk("DOM%u: unhandled THUMB instruction 0x%x\n",
>> +           current->domain->domain_id, instr);
> 
> gdprint etc?
> 
>> +    return 1;
>> +}
>> +
>> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
>> +{
>> +    if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB )
>> +        return decode_thumb(regs->pc, dabt);
>> +
>> +    /* TODO: Handle ARM instruction */
> 
> You log unhandled thumb instructions but not ARM one, I think you should
> log here too.

I'll add a warning.

>> +
>> +    return 1;
>> +}
>> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
>> new file mode 100644
>> index 0000000..4613763
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.h
>> @@ -0,0 +1,49 @@
>> +/*
>> + * xen/arch/arm/decode.h
>> + *
>> + * Instruction decoder
>> + *
>> + * Julien Grall <julien.grall@linaro.org>
>> + * Copyright (C) 2013 Linaro Limited.
>> + *
>> + * 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.
>> + *
>> + * 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 __ARCH_ARM_DECODE_H_
>> +#define __ARCH_ARM_DECODE_H_
>> +
>> +#include <asm/regs.h>
>> +#include <asm/processor.h>
>> +
>> +/**
>> + * Decode an instruction from pc
>> + * /!\ This function is not intended to fully decode an instruction. It
>> + * considers that the instruction is valid.
>> + *
>> + * This function will get:
>> + *  - The transfer register
>> + *  - Sign bit
>> + *  - Size
>> + */
>> +
>> +int decode_instruction(const struct cpu_user_regs *regs,
>> +                       struct hsr_dabt *dabt);
>> +
>> +#endif /* __ARCH_ARM_DECODE_H_ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
> 


-- 
Julien Grall

Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort

[Julien Grall]

On 08/08/2013 12:09 PM, Ian Campbell wrote:
> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
>> +    /*
>> +     * Errata 766422: Thumb store translation fault to Hypervisor may
>> +     * not have correct HSR Rt value.
>> +     */
>> +    if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
>> +    {
>> +        rc = decode_instruction(regs, &info.dabt);
>> +        if ( rc )
>> +            goto bad_data_abort;
> 
> Consider logging here so we know which caller of decode_instruction
> failed?

I will add a printk.

> Either way:
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Ian.
> 


-- 
Julien Grall

Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort

[Ian Campbell]

On Thu, 2013-08-08 at 13:04 +0100, Julien Grall wrote:
> On 08/08/2013 12:09 PM, Ian Campbell wrote:
> > On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
> >> +    /*
> >> +     * Errata 766422: Thumb store translation fault to Hypervisor may
> >> +     * not have correct HSR Rt value.
> >> +     */
> >> +    if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write )
> >> +    {
> >> +        rc = decode_instruction(regs, &info.dabt);
> >> +        if ( rc )
> >> +            goto bad_data_abort;
> > 
> > Consider logging here so we know which caller of decode_instruction
> > failed?
> 
> I will add a printk.

gdprintk ;-)

> 
> > Either way:
> > 
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Ian.
> > 
> 
> 

Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction

[Julien Grall]

On 08/08/2013 12:08 PM, Ian Campbell wrote:
> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
>> Some erratas on ARM processor requires to decode the instruction.
> 
> Errata is already the plural of Erratum, I think.
> 
>> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt.
>>
>> For the moment, the decoder only supports:
>>     - THUMB2 store instruction
>>     - THUMB single load/store instruction
>>
>> Signed-off-by: Julien Grall <julien.grall@linaro.org>
>>
>> ---
>>     Changes in v4:
>>         - Add warning about the purpose of this function
>>         - Add helper to update ISS (register, sign, size) field of DABT
>>         - Improve decoding for THUMB 2 store instruction
>>         - Only decode thumb instruction if it's a 32-bit guest
>> ---
>>  xen/arch/arm/Makefile |    1 +
>>  xen/arch/arm/decode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/decode.h |   49 +++++++++++++++
>>  3 files changed, 218 insertions(+)
>>  create mode 100644 xen/arch/arm/decode.c
>>  create mode 100644 xen/arch/arm/decode.h
>>
>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>> index 5ae5831..5c13a65 100644
>> --- a/xen/arch/arm/Makefile
>> +++ b/xen/arch/arm/Makefile
>> @@ -30,6 +30,7 @@ obj-y += vtimer.o
>>  obj-y += vpl011.o
>>  obj-y += hvm.o
>>  obj-y += device.o
>> +obj-y += decode.o
>>  
>>  #obj-bin-y += ....o
>>  
>> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c
>> new file mode 100644
>> index 0000000..0e7d8ba
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * xen/arch/arm/decode.c
>> + *
>> + * Instruction decoder
>> + *
>> + * Julien Grall <julien.grall@linaro.org>
>> + * Copyright (C) 2013 Linaro Limited.
>> + *
>> + * 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.
>> + *
>> + * 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/types.h>
>> +#include <xen/sched.h>
>> +#include <asm/current.h>
>> +#include <asm/guest_access.h>
>> +#include <xen/lib.h>
>> +
>> +#include "decode.h"
>> +
>> +static void update_dabt(struct hsr_dabt *dabt, int reg,
>> +                        uint8_t size, bool_t sign)
>> +{
>> +    dabt->reg = reg;
>> +    dabt->size = size;
>> +    dabt->sign = sign;
>> +}
>> +
>> +/* TODO: Handle all THUMB2 instruction other than simple store */
> 
> I think we decided to remove this (and the other one).
>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
>> +{
>> +    uint16_t hw2;
>> +    int rc;
>> +    uint16_t rt;
>> +
>> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    rt = (hw2 >> 12) & 0x7;
>> +
>> +    switch ( (hw1 >> 9) & 0xf )
>> +    {
>> +    case 12:
>> +    {
>> +        bool_t sign = !!(hw1 & (1 << 8));
>> +        bool_t load = !!(hw1 & (1 << 4));
> 
> !! is unneeded with the bool type I think.

I have tried to remove the !! and gcc complained:
decode.c:52:28: error: overflow in implicit constant conversion
[-Werror=overflow]
         bool_t sign = (hw1 & (1 << 8));
                            ^

This is because bool_t is defined as char so the value will overflow.

-- 
Julien Grall