Hi, This is fourth version of this patch series. It allows a guest kernel to use THUMB set instructions on the processor affected by the errata 766422 (for instance the Arndale board). This patch series is divided in two patch: - The first patch implements a basic decoder to fill the ISS field of the hsr_dabt struct - The second patch implements the errata Since the last version, only the patch #1 has been modified. See the changes in the patch. Julien Grall (2): xen/arm: Start to implement an ARM decoder instruction xen/arm: errata 766422: decode thumb store during data abort xen/arch/arm/Makefile | 1 + xen/arch/arm/decode.c | 168 +++++++++++++++++++++++++++++++++ xen/arch/arm/decode.h | 49 ++++++++++ xen/arch/arm/traps.c | 12 +++ xen/include/asm-arm/arm32/processor.h | 4 + xen/include/asm-arm/arm64/processor.h | 2 + 6 files changed, 236 insertions(+) create mode 100644 xen/arch/arm/decode.c create mode 100644 xen/arch/arm/decode.h -- 1.7.10.4
Julien Grall
2013-Aug-06 18:31 UTC
[PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction
Some erratas on ARM processor requires to decode the instruction. The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. For the moment, the decoder only supports: - THUMB2 store instruction - THUMB single load/store instruction Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v4: - Add warning about the purpose of this function - Add helper to update ISS (register, sign, size) field of DABT - Improve decoding for THUMB 2 store instruction - Only decode thumb instruction if it''s a 32-bit guest --- xen/arch/arm/Makefile | 1 + xen/arch/arm/decode.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/decode.h | 49 +++++++++++++++ 3 files changed, 218 insertions(+) create mode 100644 xen/arch/arm/decode.c create mode 100644 xen/arch/arm/decode.h diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 5ae5831..5c13a65 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -30,6 +30,7 @@ obj-y += vtimer.o obj-y += vpl011.o obj-y += hvm.o obj-y += device.o +obj-y += decode.o #obj-bin-y += ....o diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c new file mode 100644 index 0000000..0e7d8ba --- /dev/null +++ b/xen/arch/arm/decode.c @@ -0,0 +1,168 @@ +/* + * xen/arch/arm/decode.c + * + * Instruction decoder + * + * Julien Grall <julien.grall@linaro.org> + * Copyright (C) 2013 Linaro Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * 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. + */ + +#include <xen/types.h> +#include <xen/sched.h> +#include <asm/current.h> +#include <asm/guest_access.h> +#include <xen/lib.h> + +#include "decode.h" + +static void update_dabt(struct hsr_dabt *dabt, int reg, + uint8_t size, bool_t sign) +{ + dabt->reg = reg; + dabt->size = size; + dabt->sign = sign; +} + +/* TODO: Handle all THUMB2 instruction other than simple store */ +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) +{ + uint16_t hw2; + int rc; + uint16_t rt; + + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); + if ( rc ) + return rc; + + rt = (hw2 >> 12) & 0x7; + + switch ( (hw1 >> 9) & 0xf ) + { + case 12: + { + bool_t sign = !!(hw1 & (1 << 8)); + bool_t load = !!(hw1 & (1 << 4)); + + if ( (hw1 & 0x0110) == 0x0100 ) + /* NEON instruction */ + goto bad_thumb2; + + if ( (hw1 & 0x0070) == 0x0070 ) + /* Undefined opcodes */ + goto bad_thumb2; + + /* Store/Load single data item */ + if ( rt == 15 ) + /* XXX: Rt == 15 is only invalid for store instruction */ + goto bad_thumb2; + + if ( !load && sign ) + /* Store instruction doesn''t support sign extension */ + goto bad_thumb2; + + update_dabt(dabt, rt, (hw1 >> 5) & 3, sign); + + break; + } + default: + goto bad_thumb2; + } + + return 0; + +bad_thumb2: + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", + current->domain->domain_id, hw1, hw2); + + return 1; +} + +/* TODO: Handle all THUMB instructions other than store */ +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) +{ + uint16_t instr; + int rc; + + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); + if ( rc ) + return rc; + + switch ( instr >> 12 ) + { + case 5: + { + /* Load/Store register */ + uint16_t opB = (instr >> 9) & 0x7; + int reg = instr & 7; + + switch ( opB & 0x3 ) + { + case 0: /* Non-signed word */ + update_dabt(dabt, reg, 2, 0); + break; + case 1: /* Non-signed halfword */ + update_dabt(dabt, reg, 1, 0); + break; + case 2: /* Non-signed byte */ + update_dabt(dabt, reg, 0, 0); + break; + case 3: /* Signed byte */ + update_dabt(dabt, reg, 0, 1); + break; + } + + break; + } + case 6: + /* Load/Store word immediate offset */ + update_dabt(dabt, instr & 7, 2, 0); + break; + case 7: + /* Load/Store byte immediate offset */ + update_dabt(dabt, instr & 7, 0, 0); + break; + case 8: + /* Load/Store halfword immediate offset */ + update_dabt(dabt, instr & 7, 1, 0); + break; + case 9: + /* Load/Store word sp offset */ + update_dabt(dabt, (instr >> 8) & 7, 2, 0); + break; + case 14: + if ( instr & (1 << 11) ) + return decode_thumb2(pc, dabt, instr); + goto bad_thumb; + case 15: + return decode_thumb2(pc, dabt, instr); + default: + goto bad_thumb; + } + + return 0; + +bad_thumb: + printk("DOM%u: unhandled THUMB instruction 0x%x\n", + current->domain->domain_id, instr); + return 1; +} + +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) +{ + if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB ) + return decode_thumb(regs->pc, dabt); + + /* TODO: Handle ARM instruction */ + + return 1; +} diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h new file mode 100644 index 0000000..4613763 --- /dev/null +++ b/xen/arch/arm/decode.h @@ -0,0 +1,49 @@ +/* + * xen/arch/arm/decode.h + * + * Instruction decoder + * + * Julien Grall <julien.grall@linaro.org> + * Copyright (C) 2013 Linaro Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * 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. + */ + +#ifndef __ARCH_ARM_DECODE_H_ +#define __ARCH_ARM_DECODE_H_ + +#include <asm/regs.h> +#include <asm/processor.h> + +/** + * Decode an instruction from pc + * /!\ This function is not intended to fully decode an instruction. It + * considers that the instruction is valid. + * + * This function will get: + * - The transfer register + * - Sign bit + * - Size + */ + +int decode_instruction(const struct cpu_user_regs *regs, + struct hsr_dabt *dabt); + +#endif /* __ARCH_ARM_DECODE_H_ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */ -- 1.7.10.4
Julien Grall
2013-Aug-06 18:31 UTC
[PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort
From the errata document: When a non-secure non-hypervisor memory operation instruction generates a stage2 page table translation fault, a trap to the hypervisor will be triggered. For an architecturally defined subset of instructions, the Hypervisor Syndrome Register (HSR) will have the Instruction Syndrome Valid (ISV) bit set to 1’b1, and the Rt field should reflect the source register (for stores) or destination register for loads. On Cortex-A15, for Thumb and ThumbEE stores, the Rt value may be incorrect and should not be used, even if the ISV bit is set. All loads, and all ARM instruction set loads and stores, will have the correct Rt value if the ISV bit is set. To avoid this issue, Xen needs to decode thumb store instruction and update the transfer register. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v3: - Use unlikely to check if the processor is affected by the errata - Move the decoder in another patch and use it Changes in v2: - Only decode the instruction on affected processor - Handle ARM 32-bit instruction in read_instruction --- xen/arch/arm/traps.c | 12 ++++++++++++ xen/include/asm-arm/arm32/processor.h | 4 ++++ xen/include/asm-arm/arm64/processor.h | 2 ++ 3 files changed, 18 insertions(+) diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index 6b5fa51..47e9ba4 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -36,6 +36,7 @@ #include <asm/cpregs.h> #include <asm/psci.h> +#include "decode.h" #include "io.h" #include "vtimer.h" #include <asm/gic.h> @@ -1336,6 +1337,17 @@ static void do_trap_data_abort_guest(struct cpu_user_regs *regs, if ( !dabt.valid ) goto bad_data_abort; + /* + * Errata 766422: Thumb store translation fault to Hypervisor may + * not have correct HSR Rt value. + */ + if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) + { + rc = decode_instruction(regs, &info.dabt); + if ( rc ) + goto bad_data_abort; + } + if (handle_mmio(&info)) { advance_pc(regs, hsr); diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h index b266252..a21d104 100644 --- a/xen/include/asm-arm/arm32/processor.h +++ b/xen/include/asm-arm/arm32/processor.h @@ -111,6 +111,10 @@ struct cpu_user_regs #define READ_SYSREG(R...) READ_SYSREG32(R) #define WRITE_SYSREG(V, R...) WRITE_SYSREG32(V, R) +/* Errata 766422: only Cortex A15 r0p4 is affected */ +#define cpu_has_errata_766422() \ + (unlikely(current_cpu_data.midr.bits == 0x410fc0f4)) + #endif /* __ASSEMBLY__ */ #endif /* __ASM_ARM_ARM32_PROCESSOR_H */ diff --git a/xen/include/asm-arm/arm64/processor.h b/xen/include/asm-arm/arm64/processor.h index c8d4609..5d9b952 100644 --- a/xen/include/asm-arm/arm64/processor.h +++ b/xen/include/asm-arm/arm64/processor.h @@ -104,6 +104,8 @@ struct cpu_user_regs #define READ_SYSREG(name) READ_SYSREG64(name) #define WRITE_SYSREG(v, name) WRITE_SYSREG64(v, name) +#define cpu_has_errata_766422() 0 + #endif /* __ASSEMBLY__ */ #endif /* __ASM_ARM_ARM64_PROCESSOR_H */ -- 1.7.10.4 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel
Ian Campbell
2013-Aug-08 11:08 UTC
Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction
On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:> Some erratas on ARM processor requires to decode the instruction.Errata is already the plural of Erratum, I think.> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. > > For the moment, the decoder only supports: > - THUMB2 store instruction > - THUMB single load/store instruction > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > > --- > Changes in v4: > - Add warning about the purpose of this function > - Add helper to update ISS (register, sign, size) field of DABT > - Improve decoding for THUMB 2 store instruction > - Only decode thumb instruction if it''s a 32-bit guest > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/decode.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/decode.h | 49 +++++++++++++++ > 3 files changed, 218 insertions(+) > create mode 100644 xen/arch/arm/decode.c > create mode 100644 xen/arch/arm/decode.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 5ae5831..5c13a65 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -30,6 +30,7 @@ obj-y += vtimer.o > obj-y += vpl011.o > obj-y += hvm.o > obj-y += device.o > +obj-y += decode.o > > #obj-bin-y += ....o > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > new file mode 100644 > index 0000000..0e7d8ba > --- /dev/null > +++ b/xen/arch/arm/decode.c > @@ -0,0 +1,168 @@ > +/* > + * xen/arch/arm/decode.c > + * > + * Instruction decoder > + * > + * Julien Grall <julien.grall@linaro.org> > + * Copyright (C) 2013 Linaro Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * 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. > + */ > + > +#include <xen/types.h> > +#include <xen/sched.h> > +#include <asm/current.h> > +#include <asm/guest_access.h> > +#include <xen/lib.h> > + > +#include "decode.h" > + > +static void update_dabt(struct hsr_dabt *dabt, int reg, > + uint8_t size, bool_t sign) > +{ > + dabt->reg = reg; > + dabt->size = size; > + dabt->sign = sign; > +} > + > +/* TODO: Handle all THUMB2 instruction other than simple store */I think we decided to remove this (and the other one).> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) > +{ > + uint16_t hw2; > + int rc; > + uint16_t rt; > + > + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); > + if ( rc ) > + return rc; > + > + rt = (hw2 >> 12) & 0x7; > + > + switch ( (hw1 >> 9) & 0xf ) > + { > + case 12: > + { > + bool_t sign = !!(hw1 & (1 << 8)); > + bool_t load = !!(hw1 & (1 << 4));!! is unneeded with the bool type I think.> + > + if ( (hw1 & 0x0110) == 0x0100 ) > + /* NEON instruction */ > + goto bad_thumb2; > + > + if ( (hw1 & 0x0070) == 0x0070 ) > + /* Undefined opcodes */ > + goto bad_thumb2; > + > + /* Store/Load single data item */ > + if ( rt == 15 ) > + /* XXX: Rt == 15 is only invalid for store instruction */So the check should be "rt == 15 && load" ?> + goto bad_thumb2; > + > + if ( !load && sign ) > + /* Store instruction doesn''t support sign extension */ > + goto bad_thumb2; > + > + update_dabt(dabt, rt, (hw1 >> 5) & 3, sign); > + > + break; > + } > + default: > + goto bad_thumb2; > + } > + > + return 0; > + > +bad_thumb2: > + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", > + current->domain->domain_id, hw1, hw2);Should this be gdprintk or gprintk?> + > + return 1; > +} > + > +/* TODO: Handle all THUMB instructions other than store */ > +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) > +{ > + uint16_t instr; > + int rc; > + > + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); > + if ( rc ) > + return rc; > + > + switch ( instr >> 12 ) > + { > + case 5: > + { > + /* Load/Store register */ > + uint16_t opB = (instr >> 9) & 0x7; > + int reg = instr & 7; > + > + switch ( opB & 0x3 ) > + { > + case 0: /* Non-signed word */ > + update_dabt(dabt, reg, 2, 0); > + break; > + case 1: /* Non-signed halfword */ > + update_dabt(dabt, reg, 1, 0); > + break; > + case 2: /* Non-signed byte */ > + update_dabt(dabt, reg, 0, 0); > + break; > + case 3: /* Signed byte */ > + update_dabt(dabt, reg, 0, 1); > + break; > + } > + > + break; > + } > + case 6: > + /* Load/Store word immediate offset */ > + update_dabt(dabt, instr & 7, 2, 0); > + break; > + case 7: > + /* Load/Store byte immediate offset */ > + update_dabt(dabt, instr & 7, 0, 0); > + break; > + case 8: > + /* Load/Store halfword immediate offset */ > + update_dabt(dabt, instr & 7, 1, 0); > + break; > + case 9: > + /* Load/Store word sp offset */ > + update_dabt(dabt, (instr >> 8) & 7, 2, 0); > + break; > + case 14: > + if ( instr & (1 << 11) ) > + return decode_thumb2(pc, dabt, instr); > + goto bad_thumb; > + case 15: > + return decode_thumb2(pc, dabt, instr); > + default: > + goto bad_thumb; > + } > + > + return 0; > + > +bad_thumb: > + printk("DOM%u: unhandled THUMB instruction 0x%x\n", > + current->domain->domain_id, instr);gdprint etc?> + return 1; > +} > + > +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) > +{ > + if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB ) > + return decode_thumb(regs->pc, dabt); > + > + /* TODO: Handle ARM instruction */You log unhandled thumb instructions but not ARM one, I think you should log here too.> + > + return 1; > +} > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > new file mode 100644 > index 0000000..4613763 > --- /dev/null > +++ b/xen/arch/arm/decode.h > @@ -0,0 +1,49 @@ > +/* > + * xen/arch/arm/decode.h > + * > + * Instruction decoder > + * > + * Julien Grall <julien.grall@linaro.org> > + * Copyright (C) 2013 Linaro Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * 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. > + */ > + > +#ifndef __ARCH_ARM_DECODE_H_ > +#define __ARCH_ARM_DECODE_H_ > + > +#include <asm/regs.h> > +#include <asm/processor.h> > + > +/** > + * Decode an instruction from pc > + * /!\ This function is not intended to fully decode an instruction. It > + * considers that the instruction is valid. > + * > + * This function will get: > + * - The transfer register > + * - Sign bit > + * - Size > + */ > + > +int decode_instruction(const struct cpu_user_regs *regs, > + struct hsr_dabt *dabt); > + > +#endif /* __ARCH_ARM_DECODE_H_ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */
Ian Campbell
2013-Aug-08 11:09 UTC
Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort
On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote:> + /* > + * Errata 766422: Thumb store translation fault to Hypervisor may > + * not have correct HSR Rt value. > + */ > + if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) > + { > + rc = decode_instruction(regs, &info.dabt); > + if ( rc ) > + goto bad_data_abort;Consider logging here so we know which caller of decode_instruction failed? Either way: Acked-by: Ian Campbell <ian.campbell@citrix.com> Ian.
Julien Grall
2013-Aug-08 11:52 UTC
Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction
On 08/08/2013 12:08 PM, Ian Campbell wrote:> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote: >> Some erratas on ARM processor requires to decode the instruction. > > Errata is already the plural of Erratum, I think.Right, I will fix it.> >> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. >> >> For the moment, the decoder only supports: >> - THUMB2 store instruction >> - THUMB single load/store instruction >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Changes in v4: >> - Add warning about the purpose of this function >> - Add helper to update ISS (register, sign, size) field of DABT >> - Improve decoding for THUMB 2 store instruction >> - Only decode thumb instruction if it''s a 32-bit guest >> --- >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/decode.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/decode.h | 49 +++++++++++++++ >> 3 files changed, 218 insertions(+) >> create mode 100644 xen/arch/arm/decode.c >> create mode 100644 xen/arch/arm/decode.h >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 5ae5831..5c13a65 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -30,6 +30,7 @@ obj-y += vtimer.o >> obj-y += vpl011.o >> obj-y += hvm.o >> obj-y += device.o >> +obj-y += decode.o >> >> #obj-bin-y += ....o >> >> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c >> new file mode 100644 >> index 0000000..0e7d8ba >> --- /dev/null >> +++ b/xen/arch/arm/decode.c >> @@ -0,0 +1,168 @@ >> +/* >> + * xen/arch/arm/decode.c >> + * >> + * Instruction decoder >> + * >> + * Julien Grall <julien.grall@linaro.org> >> + * Copyright (C) 2013 Linaro Limited. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * 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. >> + */ >> + >> +#include <xen/types.h> >> +#include <xen/sched.h> >> +#include <asm/current.h> >> +#include <asm/guest_access.h> >> +#include <xen/lib.h> >> + >> +#include "decode.h" >> + >> +static void update_dabt(struct hsr_dabt *dabt, int reg, >> + uint8_t size, bool_t sign) >> +{ >> + dabt->reg = reg; >> + dabt->size = size; >> + dabt->sign = sign; >> +} >> + >> +/* TODO: Handle all THUMB2 instruction other than simple store */ > > I think we decided to remove this (and the other one).Oh right, I forgot to remove it.>> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) >> +{ >> + uint16_t hw2; >> + int rc; >> + uint16_t rt; >> + >> + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); >> + if ( rc ) >> + return rc; >> + >> + rt = (hw2 >> 12) & 0x7; >> + >> + switch ( (hw1 >> 9) & 0xf ) >> + { >> + case 12: >> + { >> + bool_t sign = !!(hw1 & (1 << 8)); >> + bool_t load = !!(hw1 & (1 << 4)); > > !! is unneeded with the bool type I think.I was not sure, I will remove the ''!!''.>> + >> + if ( (hw1 & 0x0110) == 0x0100 ) >> + /* NEON instruction */ >> + goto bad_thumb2; >> + >> + if ( (hw1 & 0x0070) == 0x0070 ) >> + /* Undefined opcodes */ >> + goto bad_thumb2; >> + >> + /* Store/Load single data item */ >> + if ( rt == 15 ) >> + /* XXX: Rt == 15 is only invalid for store instruction */ > > So the check should be "rt == 15 && load" ?No, when rt == 15 the instructions are memory hints (pld, pldw,..) and the layout is different.>> + goto bad_thumb2; >> + >> + if ( !load && sign ) >> + /* Store instruction doesn''t support sign extension */ >> + goto bad_thumb2; >> + >> + update_dabt(dabt, rt, (hw1 >> 5) & 3, sign); >> + >> + break; >> + } >> + default: >> + goto bad_thumb2; >> + } >> + >> + return 0; >> + >> +bad_thumb2: >> + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", >> + current->domain->domain_id, hw1, hw2); > > Should this be gdprintk or gprintk?I will use gdprintk.> >> + >> + return 1; >> +} >> + >> +/* TODO: Handle all THUMB instructions other than store */ >> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) >> +{ >> + uint16_t instr; >> + int rc; >> + >> + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); >> + if ( rc ) >> + return rc; >> + >> + switch ( instr >> 12 ) >> + { >> + case 5: >> + { >> + /* Load/Store register */ >> + uint16_t opB = (instr >> 9) & 0x7; >> + int reg = instr & 7; >> + >> + switch ( opB & 0x3 ) >> + { >> + case 0: /* Non-signed word */ >> + update_dabt(dabt, reg, 2, 0); >> + break; >> + case 1: /* Non-signed halfword */ >> + update_dabt(dabt, reg, 1, 0); >> + break; >> + case 2: /* Non-signed byte */ >> + update_dabt(dabt, reg, 0, 0); >> + break; >> + case 3: /* Signed byte */ >> + update_dabt(dabt, reg, 0, 1); >> + break; >> + } >> + >> + break; >> + } >> + case 6: >> + /* Load/Store word immediate offset */ >> + update_dabt(dabt, instr & 7, 2, 0); >> + break; >> + case 7: >> + /* Load/Store byte immediate offset */ >> + update_dabt(dabt, instr & 7, 0, 0); >> + break; >> + case 8: >> + /* Load/Store halfword immediate offset */ >> + update_dabt(dabt, instr & 7, 1, 0); >> + break; >> + case 9: >> + /* Load/Store word sp offset */ >> + update_dabt(dabt, (instr >> 8) & 7, 2, 0); >> + break; >> + case 14: >> + if ( instr & (1 << 11) ) >> + return decode_thumb2(pc, dabt, instr); >> + goto bad_thumb; >> + case 15: >> + return decode_thumb2(pc, dabt, instr); >> + default: >> + goto bad_thumb; >> + } >> + >> + return 0; >> + >> +bad_thumb: >> + printk("DOM%u: unhandled THUMB instruction 0x%x\n", >> + current->domain->domain_id, instr); > > gdprint etc? > >> + return 1; >> +} >> + >> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) >> +{ >> + if ( is_pv32_domain(current->domain) && regs->cpsr & PSR_THUMB ) >> + return decode_thumb(regs->pc, dabt); >> + >> + /* TODO: Handle ARM instruction */ > > You log unhandled thumb instructions but not ARM one, I think you should > log here too.I''ll add a warning.>> + >> + return 1; >> +} >> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h >> new file mode 100644 >> index 0000000..4613763 >> --- /dev/null >> +++ b/xen/arch/arm/decode.h >> @@ -0,0 +1,49 @@ >> +/* >> + * xen/arch/arm/decode.h >> + * >> + * Instruction decoder >> + * >> + * Julien Grall <julien.grall@linaro.org> >> + * Copyright (C) 2013 Linaro Limited. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * 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. >> + */ >> + >> +#ifndef __ARCH_ARM_DECODE_H_ >> +#define __ARCH_ARM_DECODE_H_ >> + >> +#include <asm/regs.h> >> +#include <asm/processor.h> >> + >> +/** >> + * Decode an instruction from pc >> + * /!\ This function is not intended to fully decode an instruction. It >> + * considers that the instruction is valid. >> + * >> + * This function will get: >> + * - The transfer register >> + * - Sign bit >> + * - Size >> + */ >> + >> +int decode_instruction(const struct cpu_user_regs *regs, >> + struct hsr_dabt *dabt); >> + >> +#endif /* __ARCH_ARM_DECODE_H_ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ > >-- Julien Grall
Julien Grall
2013-Aug-08 12:04 UTC
Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort
On 08/08/2013 12:09 PM, Ian Campbell wrote:> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote: >> + /* >> + * Errata 766422: Thumb store translation fault to Hypervisor may >> + * not have correct HSR Rt value. >> + */ >> + if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) >> + { >> + rc = decode_instruction(regs, &info.dabt); >> + if ( rc ) >> + goto bad_data_abort; > > Consider logging here so we know which caller of decode_instruction > failed?I will add a printk.> Either way: > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > Ian. >-- Julien Grall
Ian Campbell
2013-Aug-08 12:12 UTC
Re: [PATCH v4 2/2] xen/arm: errata 766422: decode thumb store during data abort
On Thu, 2013-08-08 at 13:04 +0100, Julien Grall wrote:> On 08/08/2013 12:09 PM, Ian Campbell wrote: > > On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote: > >> + /* > >> + * Errata 766422: Thumb store translation fault to Hypervisor may > >> + * not have correct HSR Rt value. > >> + */ > >> + if ( cpu_has_errata_766422() && (regs->cpsr & PSR_THUMB) && dabt.write ) > >> + { > >> + rc = decode_instruction(regs, &info.dabt); > >> + if ( rc ) > >> + goto bad_data_abort; > > > > Consider logging here so we know which caller of decode_instruction > > failed? > > I will add a printk.gdprintk ;-)> > > Either way: > > > > Acked-by: Ian Campbell <ian.campbell@citrix.com> > > > > Ian. > > > >
Julien Grall
2013-Aug-08 12:13 UTC
Re: [PATCH v4 1/2] xen/arm: Start to implement an ARM decoder instruction
On 08/08/2013 12:08 PM, Ian Campbell wrote:> On Tue, 2013-08-06 at 19:31 +0100, Julien Grall wrote: >> Some erratas on ARM processor requires to decode the instruction. > > Errata is already the plural of Erratum, I think. > >> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. >> >> For the moment, the decoder only supports: >> - THUMB2 store instruction >> - THUMB single load/store instruction >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> >> --- >> Changes in v4: >> - Add warning about the purpose of this function >> - Add helper to update ISS (register, sign, size) field of DABT >> - Improve decoding for THUMB 2 store instruction >> - Only decode thumb instruction if it''s a 32-bit guest >> --- >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/decode.c | 168 +++++++++++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/decode.h | 49 +++++++++++++++ >> 3 files changed, 218 insertions(+) >> create mode 100644 xen/arch/arm/decode.c >> create mode 100644 xen/arch/arm/decode.h >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 5ae5831..5c13a65 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -30,6 +30,7 @@ obj-y += vtimer.o >> obj-y += vpl011.o >> obj-y += hvm.o >> obj-y += device.o >> +obj-y += decode.o >> >> #obj-bin-y += ....o >> >> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c >> new file mode 100644 >> index 0000000..0e7d8ba >> --- /dev/null >> +++ b/xen/arch/arm/decode.c >> @@ -0,0 +1,168 @@ >> +/* >> + * xen/arch/arm/decode.c >> + * >> + * Instruction decoder >> + * >> + * Julien Grall <julien.grall@linaro.org> >> + * Copyright (C) 2013 Linaro Limited. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * 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. >> + */ >> + >> +#include <xen/types.h> >> +#include <xen/sched.h> >> +#include <asm/current.h> >> +#include <asm/guest_access.h> >> +#include <xen/lib.h> >> + >> +#include "decode.h" >> + >> +static void update_dabt(struct hsr_dabt *dabt, int reg, >> + uint8_t size, bool_t sign) >> +{ >> + dabt->reg = reg; >> + dabt->size = size; >> + dabt->sign = sign; >> +} >> + >> +/* TODO: Handle all THUMB2 instruction other than simple store */ > > I think we decided to remove this (and the other one). >> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) >> +{ >> + uint16_t hw2; >> + int rc; >> + uint16_t rt; >> + >> + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); >> + if ( rc ) >> + return rc; >> + >> + rt = (hw2 >> 12) & 0x7; >> + >> + switch ( (hw1 >> 9) & 0xf ) >> + { >> + case 12: >> + { >> + bool_t sign = !!(hw1 & (1 << 8)); >> + bool_t load = !!(hw1 & (1 << 4)); > > !! is unneeded with the bool type I think.I have tried to remove the !! and gcc complained: decode.c:52:28: error: overflow in implicit constant conversion [-Werror=overflow] bool_t sign = (hw1 & (1 << 8)); ^ This is because bool_t is defined as char so the value will overflow. -- Julien Grall