[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxxx>
> 
> ---
>     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@xxxxxxxxxx>
> + * 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.

> +
> +        if ( (hw1 & 0x0110) == 0x0100 )
> +            /* NEON instruction */
> +            goto bad_thumb2;
> +
> +        if ( (hw1 & 0x0070) == 0x0070 )
> +            /* Undefined opcodes */
> +            goto bad_thumb2;
> +
> +        /* Store/Load single data item */
> +        if ( rt == 15 )
> +            /* XXX: Rt == 15 is only invalid for store instruction */

So the check should be "rt == 15 && load" ?

> +            goto bad_thumb2;
> +
> +        if ( !load && sign )
> +            /* Store instruction doesn't support sign extension */
> +            goto bad_thumb2;
> +
> +        update_dabt(dabt, rt, (hw1 >> 5) & 3, sign);
> +
> +        break;
> +    }
> +    default:
> +        goto bad_thumb2;
> +    }
> +
> +    return 0;
> +
> +bad_thumb2:
> +    printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n",
> +           current->domain->domain_id, hw1, hw2);

Should this be gdprintk or gprintk?

> +
> +    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;
> +        int reg = instr & 7;
> +
> +        switch ( opB & 0x3 )
> +        {
> +        case 0: /* Non-signed word */
> +            update_dabt(dabt, reg, 2, 0);
> +            break;
> +        case 1: /* Non-signed halfword */
> +            update_dabt(dabt, reg, 1, 0);
> +            break;
> +        case 2: /* Non-signed byte */
> +            update_dabt(dabt, reg, 0, 0);
> +            break;
> +        case 3: /* Signed byte */
> +            update_dabt(dabt, reg, 0, 1);
> +            break;
> +        }
> +
> +        break;
> +    }
> +    case 6:
> +        /* Load/Store word immediate offset */
> +        update_dabt(dabt, instr & 7, 2, 0);
> +        break;
> +    case 7:
> +        /* Load/Store byte immediate offset */
> +        update_dabt(dabt, instr & 7, 0, 0);
> +        break;
> +    case 8:
> +        /* Load/Store halfword immediate offset */
> +        update_dabt(dabt, instr & 7, 1, 0);
> +        break;
> +    case 9:
> +        /* Load/Store word sp offset */
> +        update_dabt(dabt, (instr >> 8) & 7, 2, 0);
> +        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);

gdprint etc?

> +    return 1;
> +}
> +
> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt 
> *dabt)
> +{
> +    if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB )
> +        return decode_thumb(regs->pc, dabt);
> +
> +    /* TODO: Handle ARM instruction */

You log unhandled thumb instructions but not ARM one, I think you should
log here too.

> +
> +    return 1;
> +}
> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h
> new file mode 100644
> index 0000000..4613763
> --- /dev/null
> +++ b/xen/arch/arm/decode.h
> @@ -0,0 +1,49 @@
> +/*
> + * xen/arch/arm/decode.h
> + *
> + * Instruction decoder
> + *
> + * Julien Grall <julien.grall@xxxxxxxxxx>
> + * 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>
> +
> +/**
> + * Decode an instruction from pc
> + * /!\ This function is not intended to fully decode an instruction. It
> + * considers that the instruction is valid.
> + *
> + * This function will get:
> + *  - The transfer register
> + *  - Sign bit
> + *  - Size
> + */
> +
> +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:
> + */



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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.