[v3,1/2] xen/arm: Start to implement an ARM decoder instruction

Some erratas on ARM processor requires to decode the instruction.
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>
---
 xen/arch/arm/Makefile |    1 +
 xen/arch/arm/decode.c |  143 +++++++++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/decode.h |   38 +++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 xen/arch/arm/decode.c
 create mode 100644 xen/arch/arm/decode.h

On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote:
> Some erratas on ARM processor requires to decode the instruction.
> 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>
> ---
>  xen/arch/arm/Makefile |    1 +
>  xen/arch/arm/decode.c |  143 +++++++++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/decode.h |   38 +++++++++++++
>  3 files changed, 182 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..e432d2a
> --- /dev/null
> +++ b/xen/arch/arm/decode.c
> @@ -0,0 +1,143 @@
> +/*
> + * 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"
> +
> +/* TODO: Handle all THUMB2 instruction other than simple store */

I think we can take it as a given that things will be added as and when
they are required, no need to mention it IMHO.

> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
> +{
> +    uint16_t hw2;
> +    int rc;
> +    uint16_t op1, op2, op;
> +
> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
> +    if ( rc )
> +        return rc;
> +
> +    /* See A6.3 of DDI 0406C.b */
> +    op1 = (hw1 >> 11) & 0x3;
> +    op2 = (hw1 >> 4) & 0x7f;
> +    op = hw2 >> 15;
> +
> +    if ( op1 == 3 && ((op2 & 0x71) == 0x00) )

Please can we do the op1 decode with a switch. I know there is only one
case now but it will avoid the temptation to add a chain of elses.
`
op2 decoding is a pain. oh well.

> +    {
> +        /* Store single data item */
> +        dabt->reg = (hw2 >> 12) & 0x7;
> +        /* TODO: Handle access size */

Is this difficult to do? Perhaps you need to just decode op2 a bit
further?

Having done that it may turn out to be easier to decode op1 and op2
together as a single switch statement.

> +        return 0;
> +    }
> +
> +    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
> +           current->domain->domain_id, hw1, hw2);
> +
> +    return 1;
> +}
> +
> +/* TODO: Handle all THUMB instructions other than store */
> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
> +{
> +    uint16_t instr;
> +    int rc;
> +
> +    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
> +    if ( rc )
> +        return rc;
> +
> +    switch ( instr >> 12 )
> +    {
> +    case 5:
> +    {
> +        /* Load/Store register */
> +        uint16_t opB = (instr >> 9) & 0x7;
> +
> +        switch ( opB & 0x3 )
> +        {
> +        case 0:
> +            dabt->size = 2;
> +            break;
> +        case 1:
> +            dabt->size = 1;

->sign is uninitialised for these two cases?

Actually, for many of them I think?

> +            break;
> +        case 3:
> +            dabt->sign = 1;
> +            /* Fall-through */
> +        case 2:
> +            dabt->size = 0;
> +            break;
> +        }
> +
> +        dabt->reg = instr & 7;
> +
> +        break;
> +    }
> +    case 6:
> +        /* Load/Store word immediate offset */
> +        dabt->size = 2;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 7:
> +        /* Load/Store byte immediate offset */
> +        dabt->size = 0;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 8:
> +        /* Load/Store halfword immediate offset */
> +        dabt->size = 1;
> +        dabt->reg = instr & 7;
> +        break;
> +    case 9:
> +        /* Load/Store word sp offset */
> +        dabt->size = 2;
> +        dabt->reg = (instr >> 8) & 7;
> +        break;
> +    case 14:
> +        if ( instr & (1 << 11) )
> +            return decode_thumb2(pc, dabt, instr);
> +        goto bad_thumb;
> +    case 15:
> +        return decode_thumb2(pc, dabt, instr);
> +    default:
> +        goto bad_thumb;
> +    }
> +
> +    return 0;
> +
> +bad_thumb:
> +    printk("DOM%u: unhandled THUMB instruction 0x%x\n",
> +           current->domain->domain_id, instr);
> +    return 1;
> +}
> +
> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
> +{
> +    /* XXX: zeroed ISS when decode will be fully implemented */
> +
> +    if ( regs->cpsr & PSR_THUMB )
> +        return decode_thumb(regs->pc, dabt);

Needs an is_pv32_domain too.

> +
> +    /* TODO: Handle ARM instruction */
> +
> +    return 1;
> +}
> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> new file mode 100644
> index 0000000..dc130a3
> --- /dev/null
> +++ b/xen/arch/arm/decode.h
> @@ -0,0 +1,38 @@
> +/*
> + * xen/arch/arm/decode.h
> + *
> + * 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.
> + */
> +
> +#ifndef __ARCH_ARM_DECODE_H_
> +#define __ARCH_ARM_DECODE_H_
> +
> +#include <asm/regs.h>
> +#include <asm/processor.h>
> +
> +int decode_instruction(const struct cpu_user_regs *regs,
> +                       struct hsr_dabt *dabt);
> +
> +#endif /* __ARCH_ARM_DECODE_H_ */
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */

On 07/31/2013 04:34 PM, Ian Campbell wrote:
> On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote:
>> Some erratas on ARM processor requires to decode the instruction.
>> 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>
>> ---
>>  xen/arch/arm/Makefile |    1 +
>>  xen/arch/arm/decode.c |  143 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  xen/arch/arm/decode.h |   38 +++++++++++++
>>  3 files changed, 182 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..e432d2a
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.c
>> @@ -0,0 +1,143 @@
>> +/*
>> + * 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"
>> +
>> +/* TODO: Handle all THUMB2 instruction other than simple store */
> 
> I think we can take it as a given that things will be added as and when
> they are required, no need to mention it IMHO.

I will remove it.

>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1)
>> +{
>> +    uint16_t hw2;
>> +    int rc;
>> +    uint16_t op1, op2, op;
>> +
>> +    rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    /* See A6.3 of DDI 0406C.b */
>> +    op1 = (hw1 >> 11) & 0x3;
>> +    op2 = (hw1 >> 4) & 0x7f;
>> +    op = hw2 >> 15;
>> +
>> +    if ( op1 == 3 && ((op2 & 0x71) == 0x00) )
> 
> Please can we do the op1 decode with a switch. I know there is only one
> case now but it will avoid the temptation to add a chain of elses.
> op2 decoding is a pain. oh well.
>> +    {
>> +        /* Store single data item */
>> +        dabt->reg = (hw2 >> 12) & 0x7;
>> +        /* TODO: Handle access size */
> 
> Is this difficult to do? Perhaps you need to just decode op2 a bit
> further?

It was for simplicity. I will add the support on the next patch series.


> Having done that it may turn out to be easier to decode op1 and op2
> together as a single switch statement.

Sounds good. I will give a try.

>> +        return 0;
>> +    }
>> +
>> +    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
>> +           current->domain->domain_id, hw1, hw2);
>> +
>> +    return 1;
>> +}
>> +
>> +/* TODO: Handle all THUMB instructions other than store */
>> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt)
>> +{
>> +    uint16_t instr;
>> +    int rc;
>> +
>> +    rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr));
>> +    if ( rc )
>> +        return rc;
>> +
>> +    switch ( instr >> 12 )
>> +    {
>> +    case 5:
>> +    {
>> +        /* Load/Store register */
>> +        uint16_t opB = (instr >> 9) & 0x7;
>> +
>> +        switch ( opB & 0x3 )
>> +        {
>> +        case 0:
>> +            dabt->size = 2;
>> +            break;
>> +        case 1:
>> +            dabt->size = 1;
> 
> ->sign is uninitialised for these two cases?
> 
> Actually, for many of them I think?

I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
decode_instruction. Do I still need to set sign to 0?

>> +            break;
>> +        case 3:
>> +            dabt->sign = 1;
>> +            /* Fall-through */
>> +        case 2:
>> +            dabt->size = 0;
>> +            break;
>> +        }
>> +
>> +        dabt->reg = instr & 7;
>> +
>> +        break;
>> +    }
>> +    case 6:
>> +        /* Load/Store word immediate offset */
>> +        dabt->size = 2;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 7:
>> +        /* Load/Store byte immediate offset */
>> +        dabt->size = 0;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 8:
>> +        /* Load/Store halfword immediate offset */
>> +        dabt->size = 1;
>> +        dabt->reg = instr & 7;
>> +        break;
>> +    case 9:
>> +        /* Load/Store word sp offset */
>> +        dabt->size = 2;
>> +        dabt->reg = (instr >> 8) & 7;
>> +        break;
>> +    case 14:
>> +        if ( instr & (1 << 11) )
>> +            return decode_thumb2(pc, dabt, instr);
>> +        goto bad_thumb;
>> +    case 15:
>> +        return decode_thumb2(pc, dabt, instr);
>> +    default:
>> +        goto bad_thumb;
>> +    }
>> +
>> +    return 0;
>> +
>> +bad_thumb:
>> +    printk("DOM%u: unhandled THUMB instruction 0x%x\n",
>> +           current->domain->domain_id, instr);
>> +    return 1;
>> +}
>> +
>> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt)
>> +{
>> +    /* XXX: zeroed ISS when decode will be fully implemented */
>> +
>> +    if ( regs->cpsr & PSR_THUMB )
>> +        return decode_thumb(regs->pc, dabt);
> 
> Needs an is_pv32_domain too.

I will add in the next patch series.

>> +
>> +    /* TODO: Handle ARM instruction */
>> +
>> +    return 1;
>> +}
>> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
>> new file mode 100644
>> index 0000000..dc130a3
>> --- /dev/null
>> +++ b/xen/arch/arm/decode.h
>> @@ -0,0 +1,38 @@
>> +/*
>> + * xen/arch/arm/decode.h
>> + *
>> + * 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.
>> + */
>> +
>> +#ifndef __ARCH_ARM_DECODE_H_
>> +#define __ARCH_ARM_DECODE_H_
>> +
>> +#include <asm/regs.h>
>> +#include <asm/processor.h>
>> +
>> +int decode_instruction(const struct cpu_user_regs *regs,
>> +                       struct hsr_dabt *dabt);
>> +
>> +#endif /* __ARCH_ARM_DECODE_H_ */
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
> 
>

On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:


> >> +        switch ( opB & 0x3 )
> >> +        {
> >> +        case 0:
> >> +            dabt->size = 2;
> >> +            break;
> >> +        case 1:
> >> +            dabt->size = 1;
> > 
> > ->sign is uninitialised for these two cases?
> > 
> > Actually, for many of them I think?
> 
> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
> decode_instruction. Do I still need to set sign to 0?

Well, you need to at least do one or the other!

Even if you are zeroing the iss be default I think it would be useful
from a documentation/clarity perspective to set it explicitly to zero.

Ian.

On 07/31/2013 05:26 PM, Ian Campbell wrote:
> On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:
> 
> 
>>>> +        switch ( opB & 0x3 )
>>>> +        {
>>>> +        case 0:
>>>> +            dabt->size = 2;
>>>> +            break;
>>>> +        case 1:
>>>> +            dabt->size = 1;
>>>
>>> ->sign is uninitialised for these two cases?
>>>
>>> Actually, for many of them I think?
>>
>> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
>> decode_instruction. Do I still need to set sign to 0?
> 
> Well, you need to at least do one or the other!
> 
> Even if you are zeroing the iss be default I think it would be useful
> from a documentation/clarity perspective to set it explicitly to zero.

There is only few functions which deal with signed-register. So if we
add a line "dabt->sign = 0" it's less clear. What about a helper?

update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size,
            bool_t sign)

On Wed, 2013-07-31 at 17:39 +0100, Julien Grall wrote:
> On 07/31/2013 05:26 PM, Ian Campbell wrote:
> > On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote:
> > 
> > 
> >>>> +        switch ( opB & 0x3 )
> >>>> +        {
> >>>> +        case 0:
> >>>> +            dabt->size = 2;
> >>>> +            break;
> >>>> +        case 1:
> >>>> +            dabt->size = 1;
> >>>
> >>> ->sign is uninitialised for these two cases?
> >>>
> >>> Actually, for many of them I think?
> >>
> >> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in
> >> decode_instruction. Do I still need to set sign to 0?
> > 
> > Well, you need to at least do one or the other!
> > 
> > Even if you are zeroing the iss be default I think it would be useful
> > from a documentation/clarity perspective to set it explicitly to zero.
> 
> There is only few functions which deal with signed-register. So if we
> add a line "dabt->sign = 0" it's less clear.

Even if the function doesn't deal with a signed register ever the
consumer of the decoded hsr doesn't know that and needs to know not to
sign extend, so it needs to be cleared explicitly somewhere.

>  What about a helper?
> 
> update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size,
>             bool_t sign)