Xen devel - Jul 2013 - [PATCH v2 0/3] Add support for THUMB guest kernel

Hi,

This is the second version of this patch series. It allows a guest kernel
to use THUMB set instruction.

The PATCH 1 was acked by Ian C. in the first version. For changes on the other
patches, see each patch.

Cheers,

Julien Grall (3):
  xen/arm: Don''t emulate the MMIO access if the instruction syndrome is
    invalid
  xen/arm: Allow secondary cpus to start in THUMB
  xen/arm: errata 766422: decode thumb store during data abort

 xen/arch/arm/psci.c                   |    8 ++++++
 xen/arch/arm/traps.c                  |   48 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm32/processor.h |    3 +++
 xen/include/asm-arm/arm64/processor.h |    2 ++
 4 files changed, 61 insertions(+)

-- 
1.7.10.4

When the instruction syndrome is not valid, the transfer register is unknown.
If this register is used in the emulation code (it''s the case for the
VGIC),
Xen can retrieve wrong data.

For safety, consider invalid instruction syndrome as wrong memory access.

Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/traps.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index bbd60aa..d6dc37d 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1017,6 +1017,10 @@ static void do_trap_data_abort_guest(struct cpu_user_regs
*regs,
     if ( rc == -EFAULT )
         goto bad_data_abort;
 
+    /* XXX: Decode the instruction if ISS is not valid */
+    if ( !dabt.valid )
+        goto bad_data_abort;
+
     if (handle_mmio(&info))
     {
         regs->pc += dabt.len ? 4 : 2;
-- 
1.7.10.4

Unlike bx, eret will not update the instruction set (THUMB,ARM) according to
the return address. This will result to an unpredicable behaviour for the
processor if the address doesn''t match the right instruction set.

When the kernel is compiled with THUMB2, THUMB bit needs to be set in CPSR
for the secondary cpus.

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

---
    Changes in v2:
        - Return PSCI_EINVAL if an aarch64 guest tries to use THUMB set
---
 xen/arch/arm/psci.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 18feead..200769c 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -24,6 +24,7 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
     struct domain *d = current->domain;
     struct vcpu_guest_context *ctxt;
     int rc;
+    int is_thumb = entry_point & 1;
 
     if ( (vcpuid < 0) || (vcpuid >= MAX_VIRT_CPUS) )
         return PSCI_EINVAL;
@@ -31,6 +32,10 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
     if ( vcpuid >= d->max_vcpus || (v = d->vcpu[vcpuid]) == NULL )
         return PSCI_EINVAL;
 
+    /* THUMB set is not allowed with 64-bit domain */
+    if ( is_pv64_domain(d) && is_thumb )
+        return PSCI_EINVAL;
+
     if ( (ctxt = alloc_vcpu_guest_context()) == NULL )
         return PSCI_DENIED;
 
@@ -43,6 +48,9 @@ int do_psci_cpu_on(uint32_t vcpuid, register_t entry_point)
     ctxt->ttbr1 = 0;
     ctxt->ttbcr = 0; /* Defined Reset Value */
     ctxt->user_regs.cpsr = PSR_GUEST_INIT;
+    /* Start the VCPU with THUMB set if it''s requested by the kernel
*/
+    if ( is_thumb )
+        ctxt->user_regs.cpsr |= PSR_THUMB;
     ctxt->flags = VGCF_online;
 
     domain_lock(d);
-- 
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 v2:
        - Only decode the instruction on affected processor
        - Handle ARM 32-bit instruction in read_instruction
---
 xen/arch/arm/traps.c                  |   44 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/arm32/processor.h |    3 +++
 xen/include/asm-arm/arm64/processor.h |    2 ++
 3 files changed, 49 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d6dc37d..3aa2b8c 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -35,6 +35,7 @@
 #include <asm/regs.h>
 #include <asm/cpregs.h>
 #include <asm/psci.h>
+#include <asm/guest_access.h>
 
 #include "io.h"
 #include "vtimer.h"
@@ -996,6 +997,28 @@ done:
     if (first) unmap_domain_page(first);
 }
 
+static int read_instruction(struct cpu_user_regs *regs, unsigned len,
+                            uint32_t *instr)
+{
+    int rc;
+
+    rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 : 2));
+
+    if ( rc )
+        return rc;
+
+    if ( !len ) /* 16-bit instruction */
+        *instr &= 0xffff;
+    else /* 32-bit instruction */
+    {
+        /* THUMB 32-bit instruction consisting of 2 consecutive halfwords */
+        if ( regs->cpsr & PSR_THUMB )
+            *instr = (*instr & 0xffff) << 16 | (*instr &
0xffff0000) >> 16;
+    }
+
+    return 0;
+}
+
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                                      struct hsr_dabt dabt)
 {
@@ -1021,6 +1044,27 @@ 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 )
+    {
+        uint32_t instr = 0;
+
+        rc = read_instruction(regs, dabt.len, &instr);
+        if ( rc )
+            goto bad_data_abort;
+
+        /* Retrieve the transfer register from the instruction */
+        if ( dabt.len )
+            /* With 32-bit store instruction, the register is in [12..15] */
+            info.dabt.reg = (instr & 0xf000) >> 12;
+        else
+            /* With 16-bit store instruction, the register is in [0..3] */
+            info.dabt.reg = instr & 0x7;
+    }
+
     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..bc82fbc 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -111,6 +111,9 @@ 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() (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 d9fbcb2..ac7f1bd 100644
--- a/xen/include/asm-arm/arm64/processor.h
+++ b/xen/include/asm-arm/arm64/processor.h
@@ -105,6 +105,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 Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> @@ -996,6 +997,28 @@ done:
>      if (first) unmap_domain_page(first);
>  }
>  
> +static int read_instruction(struct cpu_user_regs *regs, unsigned len,
> +                            uint32_t *instr)
> +{
> +    int rc;
> +
> +    rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 :
2));
> +
> +    if ( rc )
> +        return rc;
> +
> +    if ( !len ) /* 16-bit instruction */
> +        *instr &= 0xffff;
> +    else /* 32-bit instruction */
> +    {
> +        /* THUMB 32-bit instruction consisting of 2 consecutive halfwords
*/
Please could you incorporate something like Tim''s description from
<20130729144626.GI37169@ocelot.phlegethon.org> to make it totally
obvious what is going on here.
> +        if ( regs->cpsr & PSR_THUMB )
> +            *instr = (*instr & 0xffff) << 16 | (*instr &
0xffff0000) >> 16;
Please can you add a comment like
          /* else: already in correct order for an ARM instruction */
> +    }
> +
> +    return 0;
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       struct hsr_dabt dabt)
>  {
> @@ -1021,6 +1044,27 @@ 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 )
> +    {
> +        uint32_t instr = 0;
> +
> +        rc = read_instruction(regs, dabt.len, &instr);
> +        if ( rc )
> +            goto bad_data_abort;
> +
> +        /* Retrieve the transfer register from the instruction */
> +        if ( dabt.len )
> +            /* With 32-bit store instruction, the register is in [12..15]
*/
> +            info.dabt.reg = (instr & 0xf000) >> 12;
> +        else
> +            /* With 16-bit store instruction, the register is in [0..3] */
> +            info.dabt.reg = instr & 0x7;
Encoding T2 (store via imm8 offset from sp) has it in 8..10.

Also for clarity I think you should write "With a NN-bit Thumb store
instruction".
> +    }
> +
>      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..bc82fbc 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -111,6 +111,9 @@ 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() (current_cpu_data.midr.bits == 0x410fc0f4)
Do we have unlikely() in Xen? If yes then I think this is a good place
to use it.

Ian.

On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> When the instruction syndrome is not valid, the transfer register is
unknown.
> If this register is used in the emulation code (it''s the case for
the VGIC),
> Xen can retrieve wrong data.
> 
> For safety, consider invalid instruction syndrome as wrong memory access.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
Applied, thanks.

On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> Unlike bx, eret will not update the instruction set (THUMB,ARM) according
to
> the return address. This will result to an unpredicable behaviour for the
> processor if the address doesn''t match the right instruction set.
> 
> When the kernel is compiled with THUMB2, THUMB bit needs to be set in CPSR
> for the secondary cpus.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
Acked + applied, thanks.
>

On 07/29/2013 04:15 PM, Ian Campbell wrote:
> On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
>> @@ -996,6 +997,28 @@ done:
>>      if (first) unmap_domain_page(first);
>>  }
>>  
>> +static int read_instruction(struct cpu_user_regs *regs, unsigned len,
>> +                            uint32_t *instr)
>> +{
>> +    int rc;
>> +
>> +    rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ?
4 : 2));
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    if ( !len ) /* 16-bit instruction */
>> +        *instr &= 0xffff;
>> +    else /* 32-bit instruction */
>> +    {
>> +        /* THUMB 32-bit instruction consisting of 2 consecutive
halfwords */
> 
> Please could you incorporate something like Tim''s description from
> <20130729144626.GI37169@ocelot.phlegethon.org> to make it totally
> obvious what is going on here.
> 
>> +        if ( regs->cpsr & PSR_THUMB )
>> +            *instr = (*instr & 0xffff) << 16 | (*instr &
0xffff0000) >> 16;
> 
> Please can you add a comment like
>           /* else: already in correct order for an ARM instruction */
> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>                                       struct hsr_dabt dabt)
>>  {
>> @@ -1021,6 +1044,27 @@ 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 )
>> +    {
>> +        uint32_t instr = 0;
>> +
>> +        rc = read_instruction(regs, dabt.len, &instr);
>> +        if ( rc )
>> +            goto bad_data_abort;
>> +
>> +        /* Retrieve the transfer register from the instruction */
>> +        if ( dabt.len )
>> +            /* With 32-bit store instruction, the register is in
[12..15] */
>> +            info.dabt.reg = (instr & 0xf000) >> 12;
>> +        else
>> +            /* With 16-bit store instruction, the register is in
[0..3] */
>> +            info.dabt.reg = instr & 0x7;
> 
> Encoding T2 (store via imm8 offset from sp) has it in 8..10.
Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if
it "is not using the PC as its destination register". So this
instruction is consider as invalid and will go to "bad_data_abort".

Is a comment is enough to explain why we don''t need to decode it?
> 
> Also for clarity I think you should write "With a NN-bit Thumb store
> instruction".
> 
>> +    }
>> +
>>      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..bc82fbc 100644
>> --- a/xen/include/asm-arm/arm32/processor.h
>> +++ b/xen/include/asm-arm/arm32/processor.h
>> @@ -111,6 +111,9 @@ 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() (current_cpu_data.midr.bits ==
0x410fc0f4)
> 
> Do we have unlikely() in Xen? If yes then I think this is a good place
> to use it.
Yes. I will use it.

-- 
Julien

On Tue, 2013-07-30 at 18:37 +0100, Julien Grall wrote:
> On 07/29/2013 04:15 PM, Ian Campbell wrote:
> > On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> >> +        /* Retrieve the transfer register from the instruction */
> >> +        if ( dabt.len )
> >> +            /* With 32-bit store instruction, the register is in
[12..15] */
> >> +            info.dabt.reg = (instr & 0xf000) >> 12;
> >> +        else
> >> +            /* With 16-bit store instruction, the register is in
[0..3] */
> >> +            info.dabt.reg = instr & 0x7;
> > 
> > Encoding T2 (store via imm8 offset from sp) has it in 8..10.
> 
> Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if
> it "is not using the PC as its destination register". So this
> instruction is consider as invalid and will go to
"bad_data_abort".
I''m not sure what this has to do with the encoding I pointed to.

A8.8.203 STR (immediate, Thumb), Encoding T2:
        STR<c> <Rt>, [SP, #<imm>]
is:
        1001 0ttt mmmm mmmm (ttt=Rt, mmmm mmmm=imm)

So Rt is in bits 8..10 which != 0..3 which is all you handle above. I
can''t see any reason why you wouldn''t need to handle this
case, it is
certainly a valid instruction.

I think it would be safest to explicitly check for known opcode patterns
and handle those while logging any which we don''t recognise. This might
be doable with a lookup table but it may be too sparse. If we were doing
a more full featured instruction decoder then it might be worth it, not
sure about this very specific case.

Ian.

On 07/31/2013 09:47 AM, Ian Campbell wrote:
> On Tue, 2013-07-30 at 18:37 +0100, Julien Grall wrote:
>> On 07/29/2013 04:15 PM, Ian Campbell wrote:
>>> On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote:
> 
>>>> +        /* Retrieve the transfer register from the instruction
*/
>>>> +        if ( dabt.len )
>>>> +            /* With 32-bit store instruction, the register is
in [12..15] */
>>>> +            info.dabt.reg = (instr & 0xf000) >> 12;
>>>> +        else
>>>> +            /* With 16-bit store instruction, the register is
in [0..3] */
>>>> +            info.dabt.reg = instr & 0x7;
>>>
>>> Encoding T2 (store via imm8 offset from sp) has it in 8..10.
>>
>> Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if
>> it "is not using the PC as its destination register". So this
>> instruction is consider as invalid and will go to
"bad_data_abort".
> 
> I''m not sure what this has to do with the encoding I pointed to.
> 
> A8.8.203 STR (immediate, Thumb), Encoding T2:
>         STR<c> <Rt>, [SP, #<imm>]
> is:
>         1001 0ttt mmmm mmmm (ttt=Rt, mmmm mmmm=imm)
>
Oh right, I read ''pc'' instead of ''sp''.
> So Rt is in bits 8..10 which != 0..3 which is all you handle above. I
> can''t see any reason why you wouldn''t need to handle this
case, it is
> certainly a valid instruction.
> 
> I think it would be safest to explicitly check for known opcode patterns
> and handle those while logging any which we don''t recognise. This
might
> be doable with a lookup table but it may be too sparse. If we were doing
> a more full featured instruction decoder then it might be worth it, not
> sure about this very specific case.
As discussed with Ian, I will create a function decode_instruction. It
will decode and fill the HSR.

-- 
Julien