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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/29 16:40, Julien Grall wrote:
> 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.
>

I think I had made a misunderstanding of the H bit. I always thought
the H bit in ARM instruction set is 0.

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

I think that's not enough. Current macro will mask the conditional bits.
So no matter what the value of H bit, the blx will be recognized in
aarch32_insn_is_{b, bl}.

I think we should update the __AARCH32_INSN_FUNCS to cover the cond
bits.

#define __UNCONDITIONAL_INSN(code)       (((code) >> 28) == 0xF)

#define __AARCH32_INSN_FUNCS(abbr, mask, val)   \
static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
{                                                                 \
     return !__UNCONDITIONAL_INSN(code) && (code & (mask)) == (val);   \
}

#define __AARCH32_UNCOND_INSN_FUNCS(abbr, mask, val)   \
static always_inline bool_t aarch32_insn_is_##abbr(uint32_t code) \
{                                                                 \
     return __UNCONDITIONAL_INSN(code) && (code & (mask)) == (val);   \
}

__AARCH32_UNCOND_INSN_FUNCS(blx,  0x0E000000, 0x0A000000)

>> +__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,
>


-- 
Regards,
Wei Chen

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