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

[Julien Grall <julien.grall@xxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel