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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/30 20:55, Julien Grall wrote:
> Hi Wei,
>
> On 30/03/17 10:02, Wei Chen wrote:
>> +/* Unconditional branch instructions */
>> +/*
>> + * From ARM DDI 0406C.c Section A8.8.25. We can see blx has a H bit.
>> + * In an ARM/Thumb instructions mixed running environment, this bit
>> + * can be 1 or 0. Although Xen is only using the ARM instructions
>> + * and the H bit is always 0. We mask this bit to catch both of these
>> + * two encodings for future-proof.
>> + */
>> +__AARCH32_INSN_FUNCS(blx, 0x0E000000, 0x0A000000)
>> +
>> +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)
>> +{
>> +    /* Check conditional branch instructions */
>> +    if ( __CONDITIONAL_INSN(insn) )
>
> This is not what I asked. Whilst I am happy to see a different solution
> this one is completely wrong. b and bl instruction cannot be
> unconditional and a user of the helpers aarch32_insn_is_{b,bl} would
> expect them to do the right things.
>
> By "open-coding" I meant dropping the macro __AARCH32_INS_FUNCS and
> directly implement insn_is_branch_imm with:
>
> return ( (insn & 0x0E000000) == 0x0A000000 );
>
> With the comment on top explain the check.
>

Thanks. This will make the code more clear. I would add comment to code
to explain this check.

> Cheers,
>


-- 
Regards,
Wei Chen

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