[PATCH 0/3] Add support for THUMB guest kernel

[PATCH 0/3] Add support for THUMB guest kernel

[Julien Grall]

Hi,

By default, Linaro Kernel is compiled with THUMB2 enabled. This small patch
series allow a guest kernel to use THUMB instruction.

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  |    3 +++
 xen/arch/arm/traps.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

-- 
1.7.10.4

[PATCH 1/3] xen/arm: Don't emulate the MMIO access if the instruction syndrome is invalid

[Julien Grall]

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>
---
 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

[PATCH 2/3] xen/arm: Allow secondary cpus to start in THUMB

[Julien Grall]

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>
---
 xen/arch/arm/psci.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index 18feead..574c343 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -43,6 +43,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 in THUMB mode if it's requested by the kernel */
+    if ( entry_point & 1 )
+        ctxt->user_regs.cpsr |= PSR_THUMB;
     ctxt->flags = VGCF_online;
 
     domain_lock(d);
-- 
1.7.10.4

[PATCH 3/3] 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>
---
 xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index d6dc37d..da2bef6 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,31 @@ 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);
+
+    if ( rc )
+        return rc;
+
+    switch ( len )
+    {
+    /* 16-bit instruction */
+    case 2:
+        *instr &= 0xffff;
+        break;
+    /* 32-bit instruction */
+    case 4:
+        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
+        break;
+    }
+
+    return 0;
+}
+
 static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
                                      struct hsr_dabt dabt)
 {
@@ -1021,6 +1047,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 ( (regs->cpsr & PSR_THUMB) && dabt.write )
+    {
+        uint32_t instr = 0;
+
+        rc = read_instruction(regs, dabt.len ? 4 : 2, &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;
-- 
1.7.10.4


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

Re: [PATCH 0/3] Add support for THUMB guest kernel

[Ian Campbell]

On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> Hi,
> 
> By default, Linaro Kernel is compiled with THUMB2 enabled. This small patch
> series allow a guest kernel to use THUMB instruction.

Cool. I'm actually working on right now on a patch to correctly handle
the ITSTATE machine cranking and conditional instructions, which will be
needed for this too.

Ian.

> 
> 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  |    3 +++
>  xen/arch/arm/traps.c |   51 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 

Re: [PATCH 2/3] xen/arm: Allow secondary cpus to start in THUMB

[Ian Campbell]

On Tue, 2013-07-23 at 19:05 +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>
> ---
>  xen/arch/arm/psci.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> index 18feead..574c343 100644
> --- a/xen/arch/arm/psci.c
> +++ b/xen/arch/arm/psci.c
> @@ -43,6 +43,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 in THUMB mode if it's requested by the kernel */
> +    if ( entry_point & 1 )
> +        ctxt->user_regs.cpsr |= PSR_THUMB;

Do we also need to clear bit 0 of the entry point, or does ERET get that
right for us?

>      ctxt->flags = VGCF_online;
>  
>      domain_lock(d);

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

[Ian Campbell]

On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> 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>
> ---
>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index d6dc37d..da2bef6 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,31 @@ 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);
> +
> +    if ( rc )
> +        return rc;
> +
> +    switch ( len )
> +    {
> +    /* 16-bit instruction */
> +    case 2:
> +        *instr &= 0xffff;
> +        break;
> +    /* 32-bit instruction */
> +    case 4:
> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;

Since you only ever consult bits 12..15 or 0..3 of the result couldn't
you just read two bytes from pc+2 instead of reading 4 bytes and
swapping etc?

> +        break;
> +    }
> +
> +    return 0;
> +}
> +
>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>                                       struct hsr_dabt dabt)
>  {
> @@ -1021,6 +1047,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 ( (regs->cpsr & PSR_THUMB) && dabt.write )

Is there some way to more precisely identify the processors with this
errata? It'd be better to avoid this rigmarole when we can...

I'd think about implementing this as a pseudo-cpuinfo thing setup either
via identify_cpu or perhaps via a processor callback in proc-v7.S and
friends. Then you can define and use something like cpu_has_errata766422
in the conditional and force it to zero for arm64 builds.


> +    {
> +        uint32_t instr = 0;
> +
> +        rc = read_instruction(regs, dabt.len ? 4 : 2, &instr);

You might as well just pass dabt.len directly I think, since you just
decode it again with a switch.

> +        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;



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

Re: [PATCH 2/3] xen/arm: Allow secondary cpus to start in THUMB

[Julien Grall]

On 23 July 2013 19:12, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2013-07-23 at 19:05 +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>
>> ---
>>  xen/arch/arm/psci.c |    3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> index 18feead..574c343 100644
>> --- a/xen/arch/arm/psci.c
>> +++ b/xen/arch/arm/psci.c
>> @@ -43,6 +43,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 in THUMB mode if it's requested by the kernel */
>> +    if ( entry_point & 1 )
>> +        ctxt->user_regs.cpsr |= PSR_THUMB;
>
> Do we also need to clear bit 0 of the entry point, or does ERET get that
> right for us?

ERET will clear bit 0. So no need to clear it.
--
Julien Grall

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

[Julien Grall]

On 23 July 2013 19:18, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
>> 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>
>> ---
>>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 47 insertions(+)
>>
>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> index d6dc37d..da2bef6 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,31 @@ 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);
>> +
>> +    if ( rc )
>> +        return rc;
>> +
>> +    switch ( len )
>> +    {
>> +    /* 16-bit instruction */
>> +    case 2:
>> +        *instr &= 0xffff;
>> +        break;
>> +    /* 32-bit instruction */
>> +    case 4:
>> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
>
> Since you only ever consult bits 12..15 or 0..3 of the result couldn't
> you just read two bytes from pc+2 instead of reading 4 bytes and
> swapping etc?

I was thinking to provide a "high level" function to retrieve an
instruction just in case we need it in the future.

>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  static void do_trap_data_abort_guest(struct cpu_user_regs *regs,
>>                                       struct hsr_dabt dabt)
>>  {
>> @@ -1021,6 +1047,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 ( (regs->cpsr & PSR_THUMB) && dabt.write )
>
> Is there some way to more precisely identify the processors with this
> errata? It'd be better to avoid this rigmarole when we can...

Only r0p4 (for instance the arndale board) should be impacted by this errata.

> I'd think about implementing this as a pseudo-cpuinfo thing setup either
> via identify_cpu or perhaps via a processor callback in proc-v7.S and
> friends. Then you can define and use something like cpu_has_errata766422
> in the conditional and force it to zero for arm64 builds.

Sounds good. I will rework the patch with this solution.

>> +    {
>> +        uint32_t instr = 0;
>> +
>> +        rc = read_instruction(regs, dabt.len ? 4 : 2, &instr);
>
> You might as well just pass dabt.len directly I think, since you just
> decode it again with a switch.

If a high-level helper to read an instruction is not needed, I will directly
mix this function with the decode part below.

>> +        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;
>
>



--
Julien Grall

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

Re: [PATCH 2/3] xen/arm: Allow secondary cpus to start in THUMB

[Ian Campbell]

On Tue, 2013-07-23 at 22:28 +0100, Julien Grall wrote:
> On 23 July 2013 19:12, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Tue, 2013-07-23 at 19:05 +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>
> >> ---
> >>  xen/arch/arm/psci.c |    3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
> >> index 18feead..574c343 100644
> >> --- a/xen/arch/arm/psci.c
> >> +++ b/xen/arch/arm/psci.c
> >> @@ -43,6 +43,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 in THUMB mode if it's requested by the kernel */
> >> +    if ( entry_point & 1 )
> >> +        ctxt->user_regs.cpsr |= PSR_THUMB;
> >
> > Do we also need to clear bit 0 of the entry point, or does ERET get that
> > right for us?
> 
> ERET will clear bit 0. So no need to clear it.

Perfect!

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

Strictly speaking the PSCI spec (DEN 0022B.b) says that for aarch64
guests we should return INVALID_PARAMETERS (-2) if bit 1 is set.

Ian

Re: [PATCH 2/3] xen/arm: Allow secondary cpus to start in THUMB

[Julien Grall]

On 23 July 2013 23:07, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2013-07-23 at 22:28 +0100, Julien Grall wrote:
>> On 23 July 2013 19:12, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > On Tue, 2013-07-23 at 19:05 +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>
>> >> ---
>> >>  xen/arch/arm/psci.c |    3 +++
>> >>  1 file changed, 3 insertions(+)
>> >>
>> >> diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
>> >> index 18feead..574c343 100644
>> >> --- a/xen/arch/arm/psci.c
>> >> +++ b/xen/arch/arm/psci.c
>> >> @@ -43,6 +43,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 in THUMB mode if it's requested by the kernel */
>> >> +    if ( entry_point & 1 )
>> >> +        ctxt->user_regs.cpsr |= PSR_THUMB;
>> >
>> > Do we also need to clear bit 0 of the entry point, or does ERET get that
>> > right for us?
>>
>> ERET will clear bit 0. So no need to clear it.
>
> Perfect!
>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>
> Strictly speaking the PSCI spec (DEN 0022B.b) says that for aarch64
> guests we should return INVALID_PARAMETERS (-2) if bit 1 is set.

I can update the patch to check if we are running an aarch64 guest.

--
Julien Grall

Re: [PATCH 1/3] xen/arm: Don't emulate the MMIO access if the instruction syndrome is invalid

[Ian Campbell]

On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> When the instruction syndrome is not valid, the transfer register is unknown.

Are there known circumstances when this can happen? Trapped store
multiples or something like that? Did you actually see one?

> 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.

That's not really what it is though. I think this deserves at least a
printed warning but to be honest if we aren't going to emulate the
instruction then there isn't much chance that the guest will be able to
recover from a spurious dabt -- IOW we might as well just shoot the
guest in the head?

> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> ---
>  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;

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

[Ian Campbell]

On Tue, 2013-07-23 at 22:41 +0100, Julien Grall wrote:
> On 23 July 2013 19:18, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> >> 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>
> >> ---
> >>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 47 insertions(+)
> >>
> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> >> index d6dc37d..da2bef6 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,31 @@ 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);
> >> +
> >> +    if ( rc )
> >> +        return rc;
> >> +
> >> +    switch ( len )
> >> +    {
> >> +    /* 16-bit instruction */
> >> +    case 2:
> >> +        *instr &= 0xffff;
> >> +        break;
> >> +    /* 32-bit instruction */
> >> +    case 4:
> >> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
> >
> > Since you only ever consult bits 12..15 or 0..3 of the result couldn't
> > you just read two bytes from pc+2 instead of reading 4 bytes and
> > swapping etc?
> 
> I was thinking to provide a "high level" function to retrieve an
> instruction just in case we need it in the future.

I suppose that does sound potentially useful.

Were does this the idea of swapping them come from though? The ARM ARM
seems (see e.g. section A6.3) to describe things in the order they are
in memory -- is doing otherwise not going to end up confusing us?

[...]
> > I'd think about implementing this as a pseudo-cpuinfo thing setup either
> > via identify_cpu or perhaps via a processor callback in proc-v7.S and
> > friends. Then you can define and use something like cpu_has_errata766422
> > in the conditional and force it to zero for arm64 builds.
> 
> Sounds good. I will rework the patch with this solution.

Thanks.

Ian.


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

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

[Julien Grall]

On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> On Tue, 2013-07-23 at 22:41 +0100, Julien Grall wrote:
>> On 23 July 2013 19:18, Ian Campbell <ian.campbell@citrix.com> wrote:
>> > On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
>> >> 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>
>> >> ---
>> >>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 47 insertions(+)
>> >>
>> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>> >> index d6dc37d..da2bef6 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,31 @@ 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);
>> >> +
>> >> +    if ( rc )
>> >> +        return rc;
>> >> +
>> >> +    switch ( len )
>> >> +    {
>> >> +    /* 16-bit instruction */
>> >> +    case 2:
>> >> +        *instr &= 0xffff;
>> >> +        break;
>> >> +    /* 32-bit instruction */
>> >> +    case 4:
>> >> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
>> >
>> > Since you only ever consult bits 12..15 or 0..3 of the result couldn't
>> > you just read two bytes from pc+2 instead of reading 4 bytes and
>> > swapping etc?
>>
>> I was thinking to provide a "high level" function to retrieve an
>> instruction just in case we need it in the future.
>
> I suppose that does sound potentially useful.
>
> Were does this the idea of swapping them come from though? The ARM ARM
> seems (see e.g. section A6.3) to describe things in the order they are
> in memory -- is doing otherwise not going to end up confusing us?

In THUMB set, a 32-bit instruction consisting of two consecutive
halfwords (see section A6.1).
So the instruction is in "big endian", but Xen will read the word as a
"little endian" things.

But in ARM set, a 32-bit instruction is a single word. I will add a
check to handle this case.

-- 
Julien Grall

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

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

[Ian Campbell]

On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> > On Tue, 2013-07-23 at 22:41 +0100, Julien Grall wrote:
> >> On 23 July 2013 19:18, Ian Campbell <ian.campbell@citrix.com> wrote:
> >> > On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
> >> >> 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>
> >> >> ---
> >> >>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
> >> >>  1 file changed, 47 insertions(+)
> >> >>
> >> >> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> >> >> index d6dc37d..da2bef6 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,31 @@ 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);
> >> >> +
> >> >> +    if ( rc )
> >> >> +        return rc;
> >> >> +
> >> >> +    switch ( len )
> >> >> +    {
> >> >> +    /* 16-bit instruction */
> >> >> +    case 2:
> >> >> +        *instr &= 0xffff;
> >> >> +        break;
> >> >> +    /* 32-bit instruction */
> >> >> +    case 4:
> >> >> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
> >> >
> >> > Since you only ever consult bits 12..15 or 0..3 of the result couldn't
> >> > you just read two bytes from pc+2 instead of reading 4 bytes and
> >> > swapping etc?
> >>
> >> I was thinking to provide a "high level" function to retrieve an
> >> instruction just in case we need it in the future.
> >
> > I suppose that does sound potentially useful.
> >
> > Were does this the idea of swapping them come from though? The ARM ARM
> > seems (see e.g. section A6.3) to describe things in the order they are
> > in memory -- is doing otherwise not going to end up confusing us?
> 
> In THUMB set, a 32-bit instruction consisting of two consecutive
> halfwords (see section A6.1).
> So the instruction is in "big endian", but Xen will read the word as a
> "little endian" things.

Are you saying that the 16-bit sub-words are in the opposite order to
what I would have expected and what seems most natural from a decode
PoV?

Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
        1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12

(where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
and imm12 is the remainder of the second halfword).

I would have expected that the halfword with the "11111" pattern (which
identifies this as a 32-bit thumb instruction) would have to come first,
so the decode knows to look for the second. IOW the halfword with 11111
should be at PC and the Rt/imm12 halfword should be at PC+2. So if we
copy 4 bytes from guest PC we should end up with things in the order
given above (and in the manual) and swapping shouldn't be needed.
Perhaps I need to go have some coffee...

Have you tested and actually observed this case on real h/w?

> 
> But in ARM set, a 32-bit instruction is a single word. I will add a
> check to handle this case.
> 



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

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

[Julien Grall]

On 07/24/2013 03:55 PM, Ian Campbell wrote:
> On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
>> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
>>> On Tue, 2013-07-23 at 22:41 +0100, Julien Grall wrote:
>>>> On 23 July 2013 19:18, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>>> On Tue, 2013-07-23 at 19:05 +0100, Julien Grall wrote:
>>>>>> 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>
>>>>>> ---
>>>>>>  xen/arch/arm/traps.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>>>  1 file changed, 47 insertions(+)
>>>>>>
>>>>>> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
>>>>>> index d6dc37d..da2bef6 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,31 @@ 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);
>>>>>> +
>>>>>> +    if ( rc )
>>>>>> +        return rc;
>>>>>> +
>>>>>> +    switch ( len )
>>>>>> +    {
>>>>>> +    /* 16-bit instruction */
>>>>>> +    case 2:
>>>>>> +        *instr &= 0xffff;
>>>>>> +        break;
>>>>>> +    /* 32-bit instruction */
>>>>>> +    case 4:
>>>>>> +        *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16;
>>>>>
>>>>> Since you only ever consult bits 12..15 or 0..3 of the result couldn't
>>>>> you just read two bytes from pc+2 instead of reading 4 bytes and
>>>>> swapping etc?
>>>>
>>>> I was thinking to provide a "high level" function to retrieve an
>>>> instruction just in case we need it in the future.
>>>
>>> I suppose that does sound potentially useful.
>>>
>>> Were does this the idea of swapping them come from though? The ARM ARM
>>> seems (see e.g. section A6.3) to describe things in the order they are
>>> in memory -- is doing otherwise not going to end up confusing us?
>>
>> In THUMB set, a 32-bit instruction consisting of two consecutive
>> halfwords (see section A6.1).
>> So the instruction is in "big endian", but Xen will read the word as a
>> "little endian" things.
> 
> Are you saying that the 16-bit sub-words are in the opposite order to
> what I would have expected and what seems most natural from a decode
> PoV?

> Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
>         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
> 
> (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
> and imm12 is the remainder of the second halfword).
> 
> I would have expected that the halfword with the "11111" pattern (which
> identifies this as a 32-bit thumb instruction) would have to come first,
> so the decode knows to look for the second. IOW the halfword with 11111
> should be at PC and the Rt/imm12 halfword should be at PC+2. So if we
> copy 4 bytes from guest PC we should end up with things in the order
> given above (and in the manual) and swapping shouldn't be needed.
> Perhaps I need to go have some coffee...

The ARM ARM specification describes a THUMB 32 bit instruction with
   HW1 HW2

HW1 => [31:16] => most significant
HW2 => [15:0] => less significant

raw_copy_from_copy will directly read 4 bytes and store in an uint32_t.
As Xen is running in little endian, it ends up in:
HW2 => [31:16]
HW1 => [15:0]

Which is false. If it's more clear, I can do something like this

uint16_t a[2];
rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);

...

switch ( len )
{
...
case 4:
   instr = a[0] << 16 | a[1];
...
}

> Have you tested and actually observed this case on real h/w?

I tried on the arndale board.

-- 
Julien


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

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

[Tim Deegan]

Hi,

At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
> On 07/24/2013 03:55 PM, Ian Campbell wrote:
> > On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
> >> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> >>> Were does this the idea of swapping them come from though? The ARM ARM
> >>> seems (see e.g. section A6.3) to describe things in the order they are
> >>> in memory -- is doing otherwise not going to end up confusing us?
> >>
> >> In THUMB set, a 32-bit instruction consisting of two consecutive
> >> halfwords (see section A6.1).
> >> So the instruction is in "big endian", but Xen will read the word as a
> >> "little endian" things.
> > 
> > Are you saying that the 16-bit sub-words are in the opposite order to
> > what I would have expected and what seems most natural from a decode
> > PoV?
> 
> > Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
> >         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
> > 
> > (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
> > and imm12 is the remainder of the second halfword).
> > 
> > I would have expected that the halfword with the "11111" pattern (which
> > identifies this as a 32-bit thumb instruction) would have to come first,
> > so the decode knows to look for the second. IOW the halfword with 11111
> > should be at PC and the Rt/imm12 halfword should be at PC+2.

Yes.  This seems to be pretty much explicit in section A6.1 -- you can
read 16 bits and decide from those whether you need to read another 16.

"If bits [15:11] of the halfword being decoded take any of the following
 values, the halfword is the first halfword of a 32-bit instruction"

> > So if we
> > copy 4 bytes from guest PC we should end up with things in the order
> > given above (and in the manual) and swapping shouldn't be needed.

Sadly, no.  Instruction memory is always little-endian, but:

"The Thumb instruction stream is a sequence of halfword-aligned
 halfwords. Each Thumb instruction is either a single 16-bit halfword
 in that stream, or a 32-bit instruction consisting of two consecutive
 halfwords in that stream."

So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
with the magic 5-bit pattern) will be stored in memory as a mixed-endian
monstrosity:

PC:   B
PC+1: A
PC+2: D
PC+3: C

A little-endian halfword read of PC gives you AB; another halfword read
at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.

We don't necessarily have to do the bit-shuffling explicitly: we could
do thumb32 decode with the shuffle implicit in the layout used for
decoding.

> uint16_t a[2];
> rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);

You definitely should read exactly the correct number of bytes -- if we
are decoding a 16-bit instruction at the end of a page we don't want to
trigger a fault by reading 32 bits and crossing a page boundary.

Oh, and one other comment that I've already lost the context for: can
you please rename the instruction fetch-and-shuffle function to have
'thumb' in its name somewhere?

Cheers,

Tim.

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

[Ian Campbell]

On Mon, 2013-07-29 at 15:46 +0100, Tim Deegan wrote:
> Hi,
> 
> At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
> > On 07/24/2013 03:55 PM, Ian Campbell wrote:
> > > On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
> > >> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> > >>> Were does this the idea of swapping them come from though? The ARM ARM
> > >>> seems (see e.g. section A6.3) to describe things in the order they are
> > >>> in memory -- is doing otherwise not going to end up confusing us?
> > >>
> > >> In THUMB set, a 32-bit instruction consisting of two consecutive
> > >> halfwords (see section A6.1).
> > >> So the instruction is in "big endian", but Xen will read the word as a
> > >> "little endian" things.
> > > 
> > > Are you saying that the 16-bit sub-words are in the opposite order to
> > > what I would have expected and what seems most natural from a decode
> > > PoV?
> > 
> > > Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
> > >         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
> > > 
> > > (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
> > > and imm12 is the remainder of the second halfword).
> > > 
> > > I would have expected that the halfword with the "11111" pattern (which
> > > identifies this as a 32-bit thumb instruction) would have to come first,
> > > so the decode knows to look for the second. IOW the halfword with 11111
> > > should be at PC and the Rt/imm12 halfword should be at PC+2.
> 
> Yes.  This seems to be pretty much explicit in section A6.1 -- you can
> read 16 bits and decide from those whether you need to read another 16.
> 
> "If bits [15:11] of the halfword being decoded take any of the following
>  values, the halfword is the first halfword of a 32-bit instruction"
> 
> > > So if we
> > > copy 4 bytes from guest PC we should end up with things in the order
> > > given above (and in the manual) and swapping shouldn't be needed.
> 
> Sadly, no.  Instruction memory is always little-endian, but:
> 
> "The Thumb instruction stream is a sequence of halfword-aligned
>  halfwords. Each Thumb instruction is either a single 16-bit halfword
>  in that stream, or a 32-bit instruction consisting of two consecutive
>  halfwords in that stream."
> 
> So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
> with the magic 5-bit pattern) will be stored in memory as a mixed-endian
> monstrosity:
> 
> PC:   B
> PC+1: A
> PC+2: D
> PC+3: C
> 
> A little-endian halfword read of PC gives you AB; another halfword read
> at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.

Good lord. No wonder I didn't get what Julien was trying to explain to
me, he was trying to explain an insanity!

> We don't necessarily have to do the bit-shuffling explicitly: we could
> do thumb32 decode with the shuffle implicit in the layout used for
> decoding.

Since we already know the instruction length in this context that would
seem OK. Although it does make it harder to reason about code which
handles either 16 or 32 bit instructions -- which half contains "bit 0"
becomes different.

If we wanted to be more general we would do a 16-bit read and then check
for the magic pattern before doing a shift and a second 16-bit read,
which mimic the hardware and avoid my tiny brain having to think to hard
about mixed endianness.

> > uint16_t a[2];
> > rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);
> 
> You definitely should read exactly the correct number of bytes -- if we
> are decoding a 16-bit instruction at the end of a page we don't want to
> trigger a fault by reading 32 bits and crossing a page boundary.

Yes.

> Oh, and one other comment that I've already lost the context for: can
> you please rename the instruction fetch-and-shuffle function to have
> 'thumb' in its name somewhere?

ACK!

Ian.

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

[Julien Grall]

On 07/29/2013 03:46 PM, Tim Deegan wrote:
> Hi,
> 
> At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
>> On 07/24/2013 03:55 PM, Ian Campbell wrote:
>>> On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
>>>> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>>> Were does this the idea of swapping them come from though? The ARM ARM
>>>>> seems (see e.g. section A6.3) to describe things in the order they are
>>>>> in memory -- is doing otherwise not going to end up confusing us?
>>>>
>>>> In THUMB set, a 32-bit instruction consisting of two consecutive
>>>> halfwords (see section A6.1).
>>>> So the instruction is in "big endian", but Xen will read the word as a
>>>> "little endian" things.
>>>
>>> Are you saying that the 16-bit sub-words are in the opposite order to
>>> what I would have expected and what seems most natural from a decode
>>> PoV?
>>
>>> Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
>>>         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
>>>
>>> (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
>>> and imm12 is the remainder of the second halfword).
>>>
>>> I would have expected that the halfword with the "11111" pattern (which
>>> identifies this as a 32-bit thumb instruction) would have to come first,
>>> so the decode knows to look for the second. IOW the halfword with 11111
>>> should be at PC and the Rt/imm12 halfword should be at PC+2.
> 
> Yes.  This seems to be pretty much explicit in section A6.1 -- you can
> read 16 bits and decide from those whether you need to read another 16.
> 
> "If bits [15:11] of the halfword being decoded take any of the following
>  values, the halfword is the first halfword of a 32-bit instruction"
> 
>>> So if we
>>> copy 4 bytes from guest PC we should end up with things in the order
>>> given above (and in the manual) and swapping shouldn't be needed.
> 
> Sadly, no.  Instruction memory is always little-endian, but:
> 
> "The Thumb instruction stream is a sequence of halfword-aligned
>  halfwords. Each Thumb instruction is either a single 16-bit halfword
>  in that stream, or a 32-bit instruction consisting of two consecutive
>  halfwords in that stream."
> 
> So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
> with the magic 5-bit pattern) will be stored in memory as a mixed-endian
> monstrosity:
> 
> PC:   B
> PC+1: A
> PC+2: D
> PC+3: C
> 
> A little-endian halfword read of PC gives you AB; another halfword read
> at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.
> 
> We don't necessarily have to do the bit-shuffling explicitly: we could
> do thumb32 decode with the shuffle implicit in the layout used for
> decoding.

I think we need to keep the bit-shuffling explicitly. It's less
confusing than having "non-coherent" shift during the decoding.

>> uint16_t a[2];
>> rc = raw_copy_from_guest(instr, (void * __user)regs->pc, len);
> 
> You definitely should read exactly the correct number of bytes -- if we
> are decoding a 16-bit instruction at the end of a page we don't want to
> trigger a fault by reading 32 bits and crossing a page boundary.

I already read either 16 or 32 bits depending of the instruction len.
The array is bigger to fit to the both instruction.

> Oh, and one other comment that I've already lost the context for: can
> you please rename the instruction fetch-and-shuffle function to have
> 'thumb' in its name somewhere?

Actually, I have sent a version for this patch series
(http://www.gossamer-threads.com/lists/xen/devel/291808). It's also
support ARM instruction decoding.

Cheers,

-- 
Julien

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

[Ian Campbell]

On Mon, 2013-07-29 at 15:56 +0100, Julien Grall wrote:
> On 07/29/2013 03:46 PM, Tim Deegan wrote:
> > Hi,
> > 
> > At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
> >> On 07/24/2013 03:55 PM, Ian Campbell wrote:
> >>> On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
> >>>> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> >>>>> Were does this the idea of swapping them come from though? The ARM ARM
> >>>>> seems (see e.g. section A6.3) to describe things in the order they are
> >>>>> in memory -- is doing otherwise not going to end up confusing us?
> >>>>
> >>>> In THUMB set, a 32-bit instruction consisting of two consecutive
> >>>> halfwords (see section A6.1).
> >>>> So the instruction is in "big endian", but Xen will read the word as a
> >>>> "little endian" things.
> >>>
> >>> Are you saying that the 16-bit sub-words are in the opposite order to
> >>> what I would have expected and what seems most natural from a decode
> >>> PoV?
> >>
> >>> Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
> >>>         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
> >>>
> >>> (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
> >>> and imm12 is the remainder of the second halfword).
> >>>
> >>> I would have expected that the halfword with the "11111" pattern (which
> >>> identifies this as a 32-bit thumb instruction) would have to come first,
> >>> so the decode knows to look for the second. IOW the halfword with 11111
> >>> should be at PC and the Rt/imm12 halfword should be at PC+2.
> > 
> > Yes.  This seems to be pretty much explicit in section A6.1 -- you can
> > read 16 bits and decide from those whether you need to read another 16.
> > 
> > "If bits [15:11] of the halfword being decoded take any of the following
> >  values, the halfword is the first halfword of a 32-bit instruction"
> > 
> >>> So if we
> >>> copy 4 bytes from guest PC we should end up with things in the order
> >>> given above (and in the manual) and swapping shouldn't be needed.
> > 
> > Sadly, no.  Instruction memory is always little-endian, but:
> > 
> > "The Thumb instruction stream is a sequence of halfword-aligned
> >  halfwords. Each Thumb instruction is either a single 16-bit halfword
> >  in that stream, or a 32-bit instruction consisting of two consecutive
> >  halfwords in that stream."
> > 
> > So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
> > with the magic 5-bit pattern) will be stored in memory as a mixed-endian
> > monstrosity:
> > 
> > PC:   B
> > PC+1: A
> > PC+2: D
> > PC+3: C
> > 
> > A little-endian halfword read of PC gives you AB; another halfword read
> > at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.
> > 
> > We don't necessarily have to do the bit-shuffling explicitly: we could
> > do thumb32 decode with the shuffle implicit in the layout used for
> > decoding.
> 
> I think we need to keep the bit-shuffling explicitly. It's less
> confusing than having "non-coherent" shift during the decoding.

Whatever we do it needs a big comment along the lines of what Tim wrote
above.

Ian.

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

[Julien Grall]

On 07/29/2013 04:00 PM, Ian Campbell wrote:
> On Mon, 2013-07-29 at 15:56 +0100, Julien Grall wrote:
>> On 07/29/2013 03:46 PM, Tim Deegan wrote:
>>> Hi,
>>>
>>> At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
>>>> On 07/24/2013 03:55 PM, Ian Campbell wrote:
>>>>> On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
>>>>>> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
>>>>>>> Were does this the idea of swapping them come from though? The ARM ARM
>>>>>>> seems (see e.g. section A6.3) to describe things in the order they are
>>>>>>> in memory -- is doing otherwise not going to end up confusing us?
>>>>>>
>>>>>> In THUMB set, a 32-bit instruction consisting of two consecutive
>>>>>> halfwords (see section A6.1).
>>>>>> So the instruction is in "big endian", but Xen will read the word as a
>>>>>> "little endian" things.
>>>>>
>>>>> Are you saying that the 16-bit sub-words are in the opposite order to
>>>>> what I would have expected and what seems most natural from a decode
>>>>> PoV?
>>>>
>>>>> Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
>>>>>         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
>>>>>
>>>>> (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
>>>>> and imm12 is the remainder of the second halfword).
>>>>>
>>>>> I would have expected that the halfword with the "11111" pattern (which
>>>>> identifies this as a 32-bit thumb instruction) would have to come first,
>>>>> so the decode knows to look for the second. IOW the halfword with 11111
>>>>> should be at PC and the Rt/imm12 halfword should be at PC+2.
>>>
>>> Yes.  This seems to be pretty much explicit in section A6.1 -- you can
>>> read 16 bits and decide from those whether you need to read another 16.
>>>
>>> "If bits [15:11] of the halfword being decoded take any of the following
>>>  values, the halfword is the first halfword of a 32-bit instruction"
>>>
>>>>> So if we
>>>>> copy 4 bytes from guest PC we should end up with things in the order
>>>>> given above (and in the manual) and swapping shouldn't be needed.
>>>
>>> Sadly, no.  Instruction memory is always little-endian, but:
>>>
>>> "The Thumb instruction stream is a sequence of halfword-aligned
>>>  halfwords. Each Thumb instruction is either a single 16-bit halfword
>>>  in that stream, or a 32-bit instruction consisting of two consecutive
>>>  halfwords in that stream."
>>>
>>> So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
>>> with the magic 5-bit pattern) will be stored in memory as a mixed-endian
>>> monstrosity:
>>>
>>> PC:   B
>>> PC+1: A
>>> PC+2: D
>>> PC+3: C
>>>
>>> A little-endian halfword read of PC gives you AB; another halfword read
>>> at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.
>>>
>>> We don't necessarily have to do the bit-shuffling explicitly: we could
>>> do thumb32 decode with the shuffle implicit in the layout used for
>>> decoding.
>>
>> I think we need to keep the bit-shuffling explicitly. It's less
>> confusing than having "non-coherent" shift during the decoding.
> 
> Whatever we do it needs a big comment along the lines of what Tim wrote
> above.

I will add the comment in the third version of this patch series.

-- 
Julien

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

[Ian Campbell]

On Mon, 2013-07-29 at 16:04 +0100, Julien Grall wrote:
> On 07/29/2013 04:00 PM, Ian Campbell wrote:
> > On Mon, 2013-07-29 at 15:56 +0100, Julien Grall wrote:
> >> On 07/29/2013 03:46 PM, Tim Deegan wrote:
> >>> Hi,
> >>>
> >>> At 16:19 +0100 on 24 Jul (1374682769), Julien Grall wrote:
> >>>> On 07/24/2013 03:55 PM, Ian Campbell wrote:
> >>>>> On Tue, 2013-07-23 at 23:43 +0100, Julien Grall wrote:
> >>>>>> On 23 July 2013 23:16, Ian Campbell <ian.campbell@citrix.com> wrote:
> >>>>>>> Were does this the idea of swapping them come from though? The ARM ARM
> >>>>>>> seems (see e.g. section A6.3) to describe things in the order they are
> >>>>>>> in memory -- is doing otherwise not going to end up confusing us?
> >>>>>>
> >>>>>> In THUMB set, a 32-bit instruction consisting of two consecutive
> >>>>>> halfwords (see section A6.1).
> >>>>>> So the instruction is in "big endian", but Xen will read the word as a
> >>>>>> "little endian" things.
> >>>>>
> >>>>> Are you saying that the 16-bit sub-words are in the opposite order to
> >>>>> what I would have expected and what seems most natural from a decode
> >>>>> PoV?
> >>>>
> >>>>> Consider the T3 encoding of STR (imm, thumb) from A8.8.203, which is:
> >>>>>         1 1 1 1 1 0 0 0 1 1 0 0 Rn | Rt imm12
> >>>>>
> >>>>> (where Rn == bits 0..3 of the first halfword, Rt is 15..12 of the second
> >>>>> and imm12 is the remainder of the second halfword).
> >>>>>
> >>>>> I would have expected that the halfword with the "11111" pattern (which
> >>>>> identifies this as a 32-bit thumb instruction) would have to come first,
> >>>>> so the decode knows to look for the second. IOW the halfword with 11111
> >>>>> should be at PC and the Rt/imm12 halfword should be at PC+2.
> >>>
> >>> Yes.  This seems to be pretty much explicit in section A6.1 -- you can
> >>> read 16 bits and decide from those whether you need to read another 16.
> >>>
> >>> "If bits [15:11] of the halfword being decoded take any of the following
> >>>  values, the halfword is the first halfword of a 32-bit instruction"
> >>>
> >>>>> So if we
> >>>>> copy 4 bytes from guest PC we should end up with things in the order
> >>>>> given above (and in the manual) and swapping shouldn't be needed.
> >>>
> >>> Sadly, no.  Instruction memory is always little-endian, but:
> >>>
> >>> "The Thumb instruction stream is a sequence of halfword-aligned
> >>>  halfwords. Each Thumb instruction is either a single 16-bit halfword
> >>>  in that stream, or a 32-bit instruction consisting of two consecutive
> >>>  halfwords in that stream."
> >>>
> >>> So a 32-bit thumb instruction with bytes ABCD (where byte A is the one 
> >>> with the magic 5-bit pattern) will be stored in memory as a mixed-endian
> >>> monstrosity:
> >>>
> >>> PC:   B
> >>> PC+1: A
> >>> PC+2: D
> >>> PC+3: C
> >>>
> >>> A little-endian halfword read of PC gives you AB; another halfword read
> >>> at PC+2 gives CD, and a 32-bit little-endian read gives CDAB.
> >>>
> >>> We don't necessarily have to do the bit-shuffling explicitly: we could
> >>> do thumb32 decode with the shuffle implicit in the layout used for
> >>> decoding.
> >>
> >> I think we need to keep the bit-shuffling explicitly. It's less
> >> confusing than having "non-coherent" shift during the decoding.
> > 
> > Whatever we do it needs a big comment along the lines of what Tim wrote
> > above.
> 
> I will add the comment in the third version of this patch series.

FYI I am in the process of applying the first 2 patches of this series,
so you might want to wait an hour or so.

Ian.