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

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

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.

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

That would be ok.


[...]

>>>  /*
>>> 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

Cheers,

-- 
Julien Grall

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