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

[Wei Chen <Wei.Chen@xxxxxxx>]

Hi Julien,

On 2017/3/17 6:24, Julien Grall wrote:
> Hi Wei,
>
> On 03/16/2017 09:53 AM, Wei Chen wrote:
>> This patch is based on the implementation of ARM64, it introduces
>> alternative runtime patching to ARM32. This allows to patch assembly
>> instruction at runtime to either fix hardware bugs or optimize for
>> certain hardware features on ARM32 platform.
>>
>> Xen hypervisor is using ARM execution state only on ARM32 platform,
>> Thumb is not used. So, the Thumb only branch instructions (CBZ, CBNZ,
>> TBB and TBH) are not considered in alternatives.
>>
>> The left ARM32 branch instructions are BX, BLX, BL and B. The
>> instruction BX is taking a register in parameter, so we don't need to
>> rewrite it. The instructions BLX, BL and B are using the similar
>> encoding for the offset and will avoid specific case when extracting
>> and updating the offset.
>>
>> In this patch, we include alternative.h header file to livepatch.c
>> directly for ARM32 compilation issues. When the alternative patching
>> config is enabled, the livepatch.c will use the alternative functions.
>> In this case, we should include the alternative header file to this
>> file. But for ARM64, it does not include this header file directly.
>> It includes this header file indirectly through:
>> sched.h->domain.h->page.h->alternative.h.
>> But, unfortunately, the page.h of ARM32 doesn't include alternative.h,
>> and we don't have the reason to include it to ARM32 page.h now. So we
>> have to include the alternative.h directly in livepatch.c.
>>
>> Signed-off-by: Wei Chen <Wei.Chen@xxxxxxx>
>> ---
>>  xen/arch/arm/Kconfig             |  2 +-
>>  xen/arch/arm/arm32/Makefile      |  1 +
>>  xen/arch/arm/arm32/insn.c        | 99 
>> ++++++++++++++++++++++++++++++++++++++++
>>  xen/common/livepatch.c           |  1 +
>
> CC Ross and Konrad for the livepatch part.
>

Thanks.

>>  xen/include/asm-arm/arm32/insn.h | 57 +++++++++++++++++++++++
>>  xen/include/asm-arm/insn.h       |  2 +
>>  6 files changed, 161 insertions(+), 1 deletion(-)
>>  create mode 100644 xen/arch/arm/arm32/insn.c
>>  create mode 100644 xen/include/asm-arm/arm32/insn.h
>>
>> diff --git a/xen/arch/arm/Kconfig b/xen/arch/arm/Kconfig
>> index 2e023d1..43123e6 100644
>> --- a/xen/arch/arm/Kconfig
>> +++ b/xen/arch/arm/Kconfig
>> @@ -12,11 +12,11 @@ config ARM_32
>>  config ARM_64
>>      def_bool y
>>      depends on 64BIT
>> -    select HAS_ALTERNATIVE
>>      select HAS_GICV3
>>
>>  config ARM
>>      def_bool y
>> +    select HAS_ALTERNATIVE
>>      select HAS_ARM_HDLCD
>>      select HAS_DEVICE_TREE
>>      select HAS_MEM_ACCESS
>> diff --git a/xen/arch/arm/arm32/Makefile b/xen/arch/arm/arm32/Makefile
>> index 4395693..0ac254f 100644
>> --- a/xen/arch/arm/arm32/Makefile
>> +++ b/xen/arch/arm/arm32/Makefile
>> @@ -4,6 +4,7 @@ obj-$(EARLY_PRINTK) += debug.o
>>  obj-y += domctl.o
>>  obj-y += domain.o
>>  obj-y += entry.o
>> +obj-y += insn.o
>>  obj-$(CONFIG_LIVEPATCH) += livepatch.o
>>  obj-y += proc-v7.o proc-caxx.o
>>  obj-y += smpboot.o
>> diff --git a/xen/arch/arm/arm32/insn.c b/xen/arch/arm/arm32/insn.c
>> new file mode 100644
>> index 0000000..91a3010
>> --- /dev/null
>> +++ b/xen/arch/arm/arm32/insn.c
>> @@ -0,0 +1,99 @@
>> +/*
>> +  * 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/>.
>> +  */
>> +#include <xen/lib.h>
>> +#include <xen/bitops.h>
>> +#include <xen/sizes.h>
>> +#include <asm/bug.h>
>
> This is already included by xen/lib.h.
>

I will remove it.

>> +#include <asm/insn.h>
>> +
>> +/* Mask of branch instructions' immediate. */
>> +#define BRANCH_INSN_IMM_MASK    GENMASK(23, 0)
>> +/* Shift of branch instructions' immediate. */
>> +#define BRANCH_INSN_IMM_SHIFT   0
>> +
>> +static uint32_t branch_insn_encode_immediate(uint32_t insn, int32_t offset)
>> +{
>> +    uint32_t imm;
>> +
>> +    /*
>> +     * Encode the offset to imm. All ARM32 instructions must be word 
>> aligned.
>> +     * Therefore the offset value's bits [1:0] equal to zero.
>> +     * (see ARM DDI 0406C.b A8.8.18/A8.8.25 for more encode/decode details
>
> DDI 0406C.b is an old version (July 2012) of the ARM ARM. Please try to
> use the latest version (e.g 0406C.c released in May 2014) when possible.
>

Ok, I will update the referred document.

>> +     * about ARM32 branch instructions)
>> +     */
>> +    imm = ((offset >> 2) & BRANCH_INSN_IMM_MASK) << BRANCH_INSN_IMM_SHIFT;
>> +
>> +    /* Update the immediate field. */
>> +    insn &= ~(BRANCH_INSN_IMM_MASK << BRANCH_INSN_IMM_SHIFT);
>> +    insn |= imm;
>> +
>> +    return insn;
>> +}
>> +
>> +/*
>> + * 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.

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

>> +        return BUG_OPCODE;
>> +    }
>> +
>> +    /* B/BL support [-32M, 32M) offset (see ARM DDI 0406C.b A4.3). */
>> +    if ( offset < -SZ_32M || offset >= SZ_32M )
>> +    {
>> +        printk(XENLOG_ERR
>> +               "%s: new branch offset out of range.\n", __func__);
>> +        return BUG_OPCODE;
>> +    }
>> +
>> +    return branch_insn_encode_immediate(insn, offset);
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
>> index 246e673..f14bcbc 100644
>> --- a/xen/common/livepatch.c
>> +++ b/xen/common/livepatch.c
>> @@ -25,6 +25,7 @@
>>  #include <xen/livepatch.h>
>>  #include <xen/livepatch_payload.h>
>>
>> +#include <asm/alternative.h>
>>  #include <asm/event.h>
>>
>>  /*
>> 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?

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.

>> +__AARCH32_INSN_FUNCS(bl, 0x0F000000, 0x0B000000)
>> +
>> +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)
>> +{
>> +    return ( aarch32_insn_is_b(insn) || aarch32_insn_is_bl(insn) );
>> +}
>> +
>> +static inline int32_t insn_get_branch_offset(uint32_t insn)
>> +{
>> +    return aarch32_get_branch_offset(insn);
>> +}
>> +
>> +static inline uint32_t insn_set_branch_offset(uint32_t insn, int32_t offset)
>> +{
>> +    return aarch32_set_branch_offset(insn, offset);
>> +}
>> +
>> +#endif /* !__ARCH_ARM_ARM32_INSN */
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/insn.h b/xen/include/asm-arm/insn.h
>> index a205ceb..3489179 100644
>> --- a/xen/include/asm-arm/insn.h
>> +++ b/xen/include/asm-arm/insn.h
>> @@ -5,6 +5,8 @@
>>
>>  #if defined(CONFIG_ARM_64)
>>  # include <asm/arm64/insn.h>
>> +#elif defined(CONFIG_ARM_32)
>> +# include <asm/arm32/insn.h>
>>  #else
>>  # error "unknown ARM variant"
>>  #endif
>>
>
> Cheers,
>


-- 
Regards,
Wei Chen

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