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

[Julien Grall <julien.grall@linaro.org>]

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