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

[Julien Grall <julien.grall@xxxxxxxxxx>]

On 23 July 2013 19:18, Ian Campbell <ian.campbell@xxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel