Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction

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

On 08/08/2013 12:08 PM, Ian Campbell wrote:
> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:
>> Some erratas on ARM processor requires to decode the instruction.
> 
> Errata is already the plural of Erratum, I think.
> 
>> 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>
>>
>> ---
>>     Changes in v4:
>>         - Add warning about the purpose of this function
>>         - Add helper to update ISS (register, sign, size) field of DABT
>>         - Improve decoding for THUMB 2 store instruction
>>         - Only decode thumb instruction if it's a 32-bit guest
>> ---
>>  xen/arch/arm/Makefile |    1 +
>>  xen/arch/arm/decode.c |  168 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/decode.h |   49 +++++++++++++++
>>  3 files changed, 218 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..0e7d8ba
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.c
>> @@ -0,0 +1,168 @@
>> +/*
>> + * 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"
>> +
>> +static void update_dabt(struct hsr_dabt *dabt, int reg,
>> +                        uint8_t size, bool_t sign)
>> +{
>> +    dabt->reg = reg;
>> +    dabt->size = size;
>> +    dabt->sign = sign;
>> +}
>> +
>> +/* TODO: Handle all THUMB2 instruction other than simple store */
> 
> I think we decided to remove this (and the other one).
>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
>> +{
>> +    uint16_t hw2;
>> +    int rc;
>> +    uint16_t rt;
>> +
>> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    rt = (hw2 >> 12) & 0x7;
>> +
>> +    switch ( (hw1 >> 9) & 0xf )
>> +    {
>> +    case 12:
>> +    {
>> +        bool_t sign = !!(hw1 & (1 << 8));
>> +        bool_t load = !!(hw1 & (1 << 4));
> 
> !! is unneeded with the bool type I think.

I have tried to remove the !! and gcc complained:
decode.c:52:28: error: overflow in implicit constant conversion
[-Werror=overflow]
         bool_t sign = (hw1 & (1 << 8));
                            ^

This is because bool_t is defined as char so the value will overflow.

-- 
Julien Grall