Re: [PATCH v2] xen/arm32: Introduce alternative runtime patching

[Julien Grall <julien.grall@arm.com>]

Hi Wei,

On 28/03/2017 08:23, Wei Chen wrote:
> diff --git a/xen/include/asm-arm/arm32/insn.h b/xen/include/asm-arm/arm32/insn.h
> new file mode 100644
> index 0000000..4cda69e
> --- /dev/null
> +++ b/xen/include/asm-arm/arm32/insn.h
> @@ -0,0 +1,65 @@
> +/*
> +  * Copyright (C) 2017 ARM Ltd.
> +  *
> +  * This program is free software; you can redistribute it and/or modify
> +  * it under the terms of the GNU General Public License version 2 as
> +  * published by the Free Software Foundation.
> +  *
> +  * 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.
> +  *
> +  * You should have received a copy of the GNU General Public License
> +  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +  */
> +#ifndef __ARCH_ARM_ARM32_INSN
> +#define __ARCH_ARM_ARM32_INSN
> +
> +#include <xen/types.h>
> +
> +#define __AARCH32_INSN_FUNCS(abbr, mask, val)   \
> +static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
> +{                                                                 \
> +    return (code & (mask)) == (val);                              \
> +}
> +
> +/*
> + * From ARM DDI 0406C.c Section A8.8.18 and A8.8.25. We can see that
> + * unconditional blx and conditional b have the same value field and imm
> + * length. And from ARM DDI 0406C.c Section A5.7 Table A5-23, we can see
> + * that the blx is the only one unconditional instruction has the same
> + * value with conditional branch instructions. So we define the b and blx
> + * in the same macro to check them at the same time.
> + */

I don't think this is true. The encodings are:
      - b   cccc1010xxxxxxxxxxxxxxxxxxxxxxxx
      - bl  cccc1011xxxxxxxxxxxxxxxxxxxxxxxx
      - blx 1111101Hxxxxxxxxxxxxxxxxxxxxxxxx

where cccc != 0b1111. So both helpers (aarch32_insn_is_{b_or_blx,bl}) 
will recognize the blx instruction depending on the value of bit H.

That's why I suggested to introduce a new helper checking for blx.

> +__AARCH32_INSN_FUNCS(b_or_blx,  0x0F000000, 0x0A000000)
> +__AARCH32_INSN_FUNCS(bl,        0x0F000000, 0x0B000000)
> +
> +int32_t aarch32_get_branch_offset(uint32_t insn);
> +uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset);
> +
> +/* Wrapper for common code */
> +static inline bool insn_is_branch_imm(uint32_t insn)
> +{
> +    return ( aarch32_insn_is_b_or_blx(insn) || aarch32_insn_is_bl(insn) );
> +}
> +
> +static inline int32_t insn_get_branch_offset(uint32_t insn)
> +{
> +    return aarch32_get_branch_offset(insn);
> +}
> +
> +static inline uint32_t insn_set_branch_offset(uint32_t insn, int32_t offset)
> +{
> +    return aarch32_set_branch_offset(insn, offset);
> +}
> +
> +#endif /* !__ARCH_ARM_ARM32_INSN */
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
> index a205ceb..3489179 100644
> --- a/xen/include/asm-arm/insn.h
> +++ b/xen/include/asm-arm/insn.h
> @@ -5,6 +5,8 @@
>
>  #if defined(CONFIG_ARM_64)
>  # include <asm/arm64/insn.h>
> +#elif defined(CONFIG_ARM_32)
> +# include <asm/arm32/insn.h>
>  #else
>  # error "unknown ARM variant"
>  #endif
>

Cheers,

-- 
Julien Grall

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