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

[Wei Chen <Wei.Chen@arm.com>]

Hi Julien,

On 2017/3/29 22:07, Julien Grall wrote:
>
>
> On 29/03/17 10:28, Wei Chen wrote:
>> 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.
>
> Because Xen is only using ARM instructions, blx will always have H = 0.
> But this is not what you described in your comment.

Yes, I missed that. I would fix it.

>
>>
>>> 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)
>
> Looking at the code you aarch32_insn_is_* helpers are only used in
> aarch32_insn_is_branch_imm. So why don't you open-code the checks in the
> latter helper?
>

That's a good opinion!

> Cheers,
>


-- 
Regards,
Wei Chen

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