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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/24 18:48, Julien Grall wrote:
> On 03/17/2017 06:35 AM, Wei Chen wrote:
>> Hi Julien,
>
> Hi Wei,
>
> Sorry for the late answer, I missed that e-mail.
>
>> On 2017/3/17 6:24, Julien Grall wrote:
>>> On 03/16/2017 09:53 AM, Wei Chen wrote:
>
> [...]
>
>>>> +/*
>>>> + * Decode the branch offset from a branch instruction's imm field.
>>>> + * The branch offset is a signed value, so it can be used to compute
>>>> + * a new branch target.
>>>> + */
>>>> +int32_t aarch32_get_branch_offset(uint32_t insn)
>>>> +{
>>>> +    uint32_t imm;
>>>> +
>>>> +    /* Retrieve imm from branch instruction. */
>>>> +    imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & BRANCH_INSN_IMM_MASK;
>>>> +
>>>> +    /*
>>>> +     * Check the imm signed bit. If the imm is a negative value, we
>>>> +     * have to extend the imm to a full 32 bit negative value.
>>>> +     */
>>>> +    if ( imm & BIT(23) )
>>>> +        imm |= GENMASK(31, 24);
>>>> +
>>>> +    return (int32_t)(imm << 2);
>>>> +}
>>>> +
>>>> +/*
>>>> + * Encode the displacement of a branch in the imm field and return the
>>>> + * updated instruction.
>>>> + */
>>>> +uint32_t aarch32_set_branch_offset(uint32_t insn, int32_t offset)
>>>> +{
>>>> +    if ( offset & 0x3 )
>>>> +    {
>>>> +        printk(XENLOG_ERR
>>>> +               "%s: ARM32 instructions must be word aligned.\n", 
>>>> __func__);
>>>
>>> This error message looks wrong to me. The offset is not an instruction.
>>> But do we really care about checking that offset is aligned to 4-bit?
>>> After all we will shift the value later on.
>>>
>>
>> I think we must care about the offset alignment. Even though we will
>> shift the last two bits later on. But a unaligned offset itself
>> indicates some problems (Compiler issue or target offset calculate bug).
>> If we ignore it, we will ignore some potential problems.
>
> I don't think this is really important. I looked at the ARM64
> counterpart (see aarch64_set_branch_offset) and we don't do the check.
>

Ok, it seems we'd better to remove this pointless check.

>>
>> About the message can we change to:
>> "Target ARM32 instructions must be placed on word aligned addresses?"
>
> That would be ok.

If so, we don't need this message any more.

>
>
> [...]
>
>>>>  /*
>>>> diff --git a/xen/include/asm-arm/arm32/insn.h 
>>>> b/xen/include/asm-arm/arm32/insn.h
>>>> new file mode 100644
>>>> index 0000000..045acc3
>>>> --- /dev/null
>>>> +++ b/xen/include/asm-arm/arm32/insn.h
>>>> @@ -0,0 +1,57 @@
>>>> +/*
>>>> +  * 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);                              \
>>>> +}
>>>> +
>>>> +__AARCH32_INSN_FUNCS(b,  0x0F000000, 0x0A000000)
>>>
>>> Looking at the ARM ARM (A8.8.18 in DDI406C.c) when cond = 0b1111, this
>>> will be a bx. Thankfully we also want to catch them.
>>>
>>> I think this needs to be spelled out in the code to help the reader
>>> understand how bx is handled.
>>>
>>
>> Sorry, I am not very clear about this comment. I read the (A5.7 in
>> DDI406C.c) and could not find bx unconditional instructions list.
>> Did you mean the unconditional bl/blx?
>
> I meant blx rather than bx in my previous e-mail. Sorry.
>
>>
>> Anyhow I think your concern is right. We have to cover the condition
>> field. Some unconditional instructions may have a conflicted op field
>> as conditional branch instructions.
>
> The only unconditional instruction with the same encoding is blx. So it
> is fine. But this either need to be:
>       1) properly documented
>       2) introducing a blx macro
>

Yes, I re-checked the list, you're right. I will do it in next version.

> Cheers,
>


-- 
Regards,
Wei Chen

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