Xen devel - Jul 2013 - [PATCH v3 0/2] Implement errata 766422

Hi,

This is the third 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).

The previous version of this patch series was called "Add support for THUMB
guest kernel", and the first 2 patches is now upstream.

I divided the third patch in two part:
    - The first patch implements a basic decoder to fill the ISS field
    of the hsr_dabt struct
    - The second patch implements the errata

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                 |  143 +++++++++++++++++++++++++++++++++
 xen/arch/arm/decode.h                 |   38 +++++++++
 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, 200 insertions(+)
 create mode 100644 xen/arch/arm/decode.c
 create mode 100644 xen/arch/arm/decode.h

-- 
1.7.10.4

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>
---
 xen/arch/arm/Makefile |    1 +
 xen/arch/arm/decode.c |  143 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/decode.h |   38 +++++++++++++
 3 files changed, 182 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..e432d2a
--- /dev/null
+++ b/xen/arch/arm/decode.c
@@ -0,0 +1,143 @@
+/*
+ * 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"
+
+/* 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 op1, op2, op;
+
+    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
+    if ( rc )
+        return rc;
+
+    /* See A6.3 of DDI 0406C.b */
+    op1 = (hw1 >> 11) & 0x3;
+    op2 = (hw1 >> 4) & 0x7f;
+    op = hw2 >> 15;
+
+    if ( op1 == 3 && ((op2 & 0x71) == 0x00) )
+    {
+        /* Store single data item */
+        dabt->reg = (hw2 >> 12) & 0x7;
+        /* TODO: Handle access size */
+        return 0;
+    }
+
+    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;
+
+        switch ( opB & 0x3 )
+        {
+        case 0:
+            dabt->size = 2;
+            break;
+        case 1:
+            dabt->size = 1;
+            break;
+        case 3:
+            dabt->sign = 1;
+            /* Fall-through */
+        case 2:
+            dabt->size = 0;
+            break;
+        }
+
+        dabt->reg = instr & 7;
+
+        break;
+    }
+    case 6:
+        /* Load/Store word immediate offset */
+        dabt->size = 2;
+        dabt->reg = instr & 7;
+        break;
+    case 7:
+        /* Load/Store byte immediate offset */
+        dabt->size = 0;
+        dabt->reg = instr & 7;
+        break;
+    case 8:
+        /* Load/Store halfword immediate offset */
+        dabt->size = 1;
+        dabt->reg = instr & 7;
+        break;
+    case 9:
+        /* Load/Store word sp offset */
+        dabt->size = 2;
+        dabt->reg = (instr >> 8) & 7;
+        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)
+{
+    /* XXX: zeroed ISS when decode will be fully implemented */
+
+    if ( 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..dc130a3
--- /dev/null
+++ b/xen/arch/arm/decode.h
@@ -0,0 +1,38 @@
+/*
+ * 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>
+
+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

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 1b9209d..83ce414 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>
@@ -1229,6 +1230,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))
     {
         regs->pc += dabt.len ? 4 : 2;
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

On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote:
> 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>
> ---
>  xen/arch/arm/Makefile |    1 +
>  xen/arch/arm/decode.c |  143
+++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/decode.h |   38 +++++++++++++
>  3 files changed, 182 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..e432d2a
> --- /dev/null
> +++ b/xen/arch/arm/decode.c
> @@ -0,0 +1,143 @@
> +/*
> + * 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"
> +
> +/* TODO: Handle all THUMB2 instruction other than simple store */
I think we can take it as a given that things will be added as and when
they are required, no need to mention it IMHO.
> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t
hw1)
> +{
> +    uint16_t hw2;
> +    int rc;
> +    uint16_t op1, op2, op;
> +
> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof
(hw2));
> +    if ( rc )
> +        return rc;
> +
> +    /* See A6.3 of DDI 0406C.b */
> +    op1 = (hw1 >> 11) & 0x3;
> +    op2 = (hw1 >> 4) & 0x7f;
> +    op = hw2 >> 15;
> +
> +    if ( op1 == 3 && ((op2 & 0x71) == 0x00) )
Please can we do the op1 decode with a switch. I know there is only one
case now but it will avoid the temptation to add a chain of elses.
`
op2 decoding is a pain. oh well.
> +    {
> +        /* Store single data item */
> +        dabt->reg = (hw2 >> 12) & 0x7;
> +        /* TODO: Handle access size */
Is this difficult to do? Perhaps you need to just decode op2 a bit
further?

Having done that it may turn out to be easier to decode op1 and op2
together as a single switch statement.
> +        return 0;
> +    }
> +
> +    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;
> +
> +        switch ( opB & 0x3 )
> +        {
> +        case 0:
> +            dabt->size = 2;
> +            break;
> +        case 1:
> +            dabt->size = 1;
->sign is uninitialised for these two cases?

Actually, for many of them I think?
> +            break;
> +        case 3:
> +            dabt->sign = 1;
> +            /* Fall-through */
> +        case 2:
> +            dabt->size = 0;
> +            break;
> +        }
> +
> +        dabt->reg = instr & 7;
> +
> +        break;
> +    }
> +    case 6:
> +        /* Load/Store word immediate offset */
> +        dabt->size = 2;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 7:
> +        /* Load/Store byte immediate offset */
> +        dabt->size = 0;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 8:
> +        /* Load/Store halfword immediate offset */
> +        dabt->size = 1;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 9:
> +        /* Load/Store word sp offset */
> +        dabt->size = 2;
> +        dabt->reg = (instr >> 8) & 7;
> +        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)
> +{
> +    /* XXX: zeroed ISS when decode will be fully implemented */
> +
> +    if ( regs->cpsr & PSR_THUMB )
> +        return decode_thumb(regs->pc, dabt);
Needs an is_pv32_domain too.
> +
> +    /* 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..dc130a3
> --- /dev/null
> +++ b/xen/arch/arm/decode.h
> @@ -0,0 +1,38 @@
> +/*
> + * 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>
> +
> +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:
> + */

On 07/31/2013 04:34 PM, Ian Campbell wrote:
> On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote:
>> 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>
>> ---
>>  xen/arch/arm/Makefile |    1 +
>>  xen/arch/arm/decode.c |  143
+++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/decode.h |   38 +++++++++++++
>>  3 files changed, 182 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..e432d2a
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.c
>> @@ -0,0 +1,143 @@
>> +/*
>> + * 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"
>> +
>> +/* TODO: Handle all THUMB2 instruction other than simple store */
> 
> I think we can take it as a given that things will be added as and when
> they are required, no need to mention it IMHO.
I will remove it.
>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt,
uint16_t hw1)
>> +{
>> +    uint16_t hw2;
>> +    int rc;
>> +    uint16_t op1, op2, op;
>> +
>> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof
(hw2));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    /* See A6.3 of DDI 0406C.b */
>> +    op1 = (hw1 >> 11) & 0x3;
>> +    op2 = (hw1 >> 4) & 0x7f;
>> +    op = hw2 >> 15;
>> +
>> +    if ( op1 == 3 && ((op2 & 0x71) == 0x00) )
> 
> Please can we do the op1 decode with a switch. I know there is only one
> case now but it will avoid the temptation to add a chain of elses.
> op2 decoding is a pain. oh well.
>> +    {
>> +        /* Store single data item */
>> +        dabt->reg = (hw2 >> 12) & 0x7;
>> +        /* TODO: Handle access size */
> 
> Is this difficult to do? Perhaps you need to just decode op2 a bit
> further?
It was for simplicity. I will add the support on the next patch series.

> Having done that it may turn out to be easier to decode op1 and op2
> together as a single switch statement.
Sounds good. I will give a try.
>> +        return 0;
>> +    }
>> +
>> +    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;
>> +
>> +        switch ( opB & 0x3 )
>> +        {
>> +        case 0:
>> +            dabt->size = 2;
>> +            break;
>> +        case 1:
>> +            dabt->size = 1;
> 
> ->sign is uninitialised for these two cases?
> 
> Actually, for many of them I think?
I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
decode_instruction. Do I still need to set sign to 0?
>> +            break;
>> +        case 3:
>> +            dabt->sign = 1;
>> +            /* Fall-through */
>> +        case 2:
>> +            dabt->size = 0;
>> +            break;
>> +        }
>> +
>> +        dabt->reg = instr & 7;
>> +
>> +        break;
>> +    }
>> +    case 6:
>> +        /* Load/Store word immediate offset */
>> +        dabt->size = 2;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 7:
>> +        /* Load/Store byte immediate offset */
>> +        dabt->size = 0;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 8:
>> +        /* Load/Store halfword immediate offset */
>> +        dabt->size = 1;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 9:
>> +        /* Load/Store word sp offset */
>> +        dabt->size = 2;
>> +        dabt->reg = (instr >> 8) & 7;
>> +        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)
>> +{
>> +    /* XXX: zeroed ISS when decode will be fully implemented */
>> +
>> +    if ( regs->cpsr & PSR_THUMB )
>> +        return decode_thumb(regs->pc, dabt);
> 
> Needs an is_pv32_domain too.
I will add in the next patch series.
>> +
>> +    /* 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..dc130a3
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * 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>
>> +
>> +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:
>> + */
> 
>

On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:

> >> +        switch ( opB & 0x3 )
> >> +        {
> >> +        case 0:
> >> +            dabt->size = 2;
> >> +            break;
> >> +        case 1:
> >> +            dabt->size = 1;
> > 
> > ->sign is uninitialised for these two cases?
> > 
> > Actually, for many of them I think?
> 
> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
> decode_instruction. Do I still need to set sign to 0?
Well, you need to at least do one or the other!

Even if you are zeroing the iss be default I think it would be useful
from a documentation/clarity perspective to set it explicitly to zero.

Ian.

On 07/31/2013 05:26 PM, Ian Campbell wrote:
> On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:
> 
> 
>>>> +        switch ( opB & 0x3 )
>>>> +        {
>>>> +        case 0:
>>>> +            dabt->size = 2;
>>>> +            break;
>>>> +        case 1:
>>>> +            dabt->size = 1;
>>>
>>> ->sign is uninitialised for these two cases?
>>>
>>> Actually, for many of them I think?
>>
>> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO
in
>> decode_instruction. Do I still need to set sign to 0?
> 
> Well, you need to at least do one or the other!
> 
> Even if you are zeroing the iss be default I think it would be useful
> from a documentation/clarity perspective to set it explicitly to zero.
There is only few functions which deal with signed-register. So if we
add a line "dabt->sign = 0" it''s less clear. What about a
helper?

update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size,
            bool_t sign)

-- 
Julien

On Wed, 2013-07-31 at 17:39 +0100, Julien Grall wrote:
> On 07/31/2013 05:26 PM, Ian Campbell wrote:
> > On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:
> > 
> > 
> >>>> +        switch ( opB & 0x3 )
> >>>> +        {
> >>>> +        case 0:
> >>>> +            dabt->size = 2;
> >>>> +            break;
> >>>> +        case 1:
> >>>> +            dabt->size = 1;
> >>>
> >>> ->sign is uninitialised for these two cases?
> >>>
> >>> Actually, for many of them I think?
> >>
> >> I plan to zeroed the ISS field (ie sign, reg...) by default. See
TODO in
> >> decode_instruction. Do I still need to set sign to 0?
> > 
> > Well, you need to at least do one or the other!
> > 
> > Even if you are zeroing the iss be default I think it would be useful
> > from a documentation/clarity perspective to set it explicitly to zero.
> 
> There is only few functions which deal with signed-register. So if we
> add a line "dabt->sign = 0" it''s less clear.
Even if the function doesn''t deal with a signed register ever the
consumer of the decoded hsr doesn''t know that and needs to know not to
sign extend, so it needs to be cleared explicitly somewhere.
>  What about a helper?
> 
> update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size,
>             bool_t sign)