| Message ID | 1489658019-22486-2-git-send-email-Wei.Chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > --- > 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. > 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. > +#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. > + * 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. > + 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. > +__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,
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@arm.com> >> --- >> 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, >
On Thu, Mar 16, 2017 at 05:53:38PM +0800, 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. OK, thanks for the explanation. Could you also confirm that the test-cases for livepatching does compile for you? make -C xen tests should do it (specifically I am curious if xen_hello_world_func.c compiles fine).
Hi Konrad, On 2017/3/17 22:34, Konrad Rzeszutek Wilk wrote: > On Thu, Mar 16, 2017 at 05:53:38PM +0800, 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. > > OK, thanks for the explanation. > > > Could you also confirm that the test-cases for livepatching > does compile for you? > > make -C xen tests > > should do it (specifically I am curious if xen_hello_world_func.c > compiles fine). > Yes, the test-cases for livepatching can compile successfully. I paste the full compiling log below: arm32@P300:~/X32/Xen32$ make -C xen tests make: Entering directory '/home/arm32/X32/Xen32/xen' make -f Rules.mk _tests make[1]: Entering directory '/home/arm32/X32/Xen32/xen' make -f /home/arm32/X32/Xen32/xen/Rules.mk -C test tests make[2]: Entering directory '/home/arm32/X32/Xen32/xen/test' make -f /home/arm32/X32/Xen32/xen/Rules.mk -C livepatch livepatch make[3]: Entering directory '/home/arm32/X32/Xen32/xen/test/livepatch' arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_hello_world_func.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_hello_world_func.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_hello_world_func.c -o xen_hello_world_func.o (set -e; \ echo "#define NEW_CODE_SZ 0x00000020"; \ echo "#define MINOR_VERSION_SZ 0x00000018"; \ echo "#define MINOR_VERSION_ADDR 0x0023e580"; \ echo "#define OLD_CODE_SZ 0x0000001c") > config.h arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_hello_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_hello_world.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_hello_world.c -o xen_hello_world.o arm-linux-gnueabihf-objcopy -O binary --only-section=.note.gnu.build-id /home/arm32/X32/Xen32/xen/xen-syms note.o.bin arm-linux-gnueabihf-objcopy -I binary -O elf32-littlearm -B arm \ --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S note.o.bin note.o rm -f note.o.bin arm-linux-gnueabihf-ld -EL -EL --build-id=sha1 -r -o xen_hello_world.livepatch xen_hello_world_func.o xen_hello_world.o note.o arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world_func.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world_func.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_bye_world_func.c -o xen_bye_world_func.o arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_bye_world.c -o xen_bye_world.o arm-linux-gnueabihf-objcopy -O binary --only-section=.note.gnu.build-id xen_hello_world.livepatch hello_world_note.o.bin arm-linux-gnueabihf-objcopy -I binary -O elf32-littlearm -B arm \ --rename-section=.data=.livepatch.depends,alloc,load,readonly,data,contents -S hello_world_note.o.bin hello_world_note.o rm -f hello_world_note.o.bin arm-linux-gnueabihf-ld -EL -EL --build-id=sha1 -r -o xen_bye_world.livepatch xen_bye_world_func.o xen_bye_world.o hello_world_note.o arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_replace_world_func.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_replace_world_func.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_replace_world_func.c -o xen_replace_world_func.o arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_replace_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_replace_world.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_replace_world.c -o xen_replace_world.o arm-linux-gnueabihf-ld -EL -EL --build-id=sha1 -r -o xen_replace_world.livepatch xen_replace_world_func.o xen_replace_world.o note.o arm-linux-gnueabihf-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/arm32/X32/Xen32/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_nop.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_nop.o.d -msoft-float -mcpu=cortex-a15 -DCONFIG_EARLY_PRINTK -DEARLY_PRINTK_INC=\"debug-pl011.inc\" -DEARLY_PRINTK_BAUD= -DEARLY_UART_BASE_ADDRESS=0x1c090000 -DEARLY_UART_REG_SHIFT= -I/home/arm32/X32/Xen32/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_nop.c -o xen_nop.o arm-linux-gnueabihf-ld -EL -EL --build-id=sha1 -r -o xen_nop.livepatch xen_nop.o note.o make[3]: Leaving directory '/home/arm32/X32/Xen32/xen/test/livepatch' make[2]: Leaving directory '/home/arm32/X32/Xen32/xen/test' make[1]: Leaving directory '/home/arm32/X32/Xen32/xen' make: Leaving directory '/home/arm32/X32/Xen32/xen'
On Mon, Mar 20, 2017 at 06:45:18AM +0000, Wei Chen wrote: > Hi Konrad, > > On 2017/3/17 22:34, Konrad Rzeszutek Wilk wrote: > > On Thu, Mar 16, 2017 at 05:53:38PM +0800, 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. > > > > OK, thanks for the explanation. > > > > > > Could you also confirm that the test-cases for livepatching > > does compile for you? > > > > make -C xen tests > > > > should do it (specifically I am curious if xen_hello_world_func.c > > compiles fine). > > > > Yes, the test-cases for livepatching can compile successfully. Fantastic! > I paste the full compiling log below: No need for that but thank you! Pls add 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>' on the patch.
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,
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, >
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> +#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 + * 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__); + 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) +__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
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@arm.com> --- xen/arch/arm/Kconfig | 2 +- xen/arch/arm/arm32/Makefile | 1 + xen/arch/arm/arm32/insn.c | 99 ++++++++++++++++++++++++++++++++++++++++ xen/common/livepatch.c | 1 + 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