| Message ID | 1374765692-31370-4-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote: > @@ -996,6 +997,28 @@ done: > if (first) unmap_domain_page(first); > } > > +static int read_instruction(struct cpu_user_regs *regs, unsigned len, > + uint32_t *instr) > +{ > + int rc; > + > + rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 : 2)); > + > + if ( rc ) > + return rc; > + > + if ( !len ) /* 16-bit instruction */ > + *instr &= 0xffff; > + else /* 32-bit instruction */ > + { > + /* THUMB 32-bit instruction consisting of 2 consecutive halfwords */ Please could you incorporate something like Tim's description from <20130729144626.GI37169@ocelot.phlegethon.org> to make it totally obvious what is going on here. > + if ( regs->cpsr & PSR_THUMB ) > + *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16; Please can you add a comment like /* else: already in correct order for an ARM instruction */ > + } > + > + return 0; > +} > + > static void do_trap_data_abort_guest(struct cpu_user_regs *regs, > struct hsr_dabt dabt) > { > @@ -1021,6 +1044,27 @@ 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 ) > + { > + uint32_t instr = 0; > + > + rc = read_instruction(regs, dabt.len, &instr); > + if ( rc ) > + goto bad_data_abort; > + > + /* Retrieve the transfer register from the instruction */ > + if ( dabt.len ) > + /* With 32-bit store instruction, the register is in [12..15] */ > + info.dabt.reg = (instr & 0xf000) >> 12; > + else > + /* With 16-bit store instruction, the register is in [0..3] */ > + info.dabt.reg = instr & 0x7; Encoding T2 (store via imm8 offset from sp) has it in 8..10. Also for clarity I think you should write "With a NN-bit Thumb store instruction". > + } > + > if (handle_mmio(&info)) > { > regs->pc += dabt.len ? 4 : 2; > diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h > index b266252..bc82fbc 100644 > --- a/xen/include/asm-arm/arm32/processor.h > +++ b/xen/include/asm-arm/arm32/processor.h > @@ -111,6 +111,9 @@ 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() (current_cpu_data.midr.bits == 0x410fc0f4) Do we have unlikely() in Xen? If yes then I think this is a good place to use it. Ian.
On 07/29/2013 04:15 PM, Ian Campbell wrote: > On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote: >> @@ -996,6 +997,28 @@ done: >> if (first) unmap_domain_page(first); >> } >> >> +static int read_instruction(struct cpu_user_regs *regs, unsigned len, >> + uint32_t *instr) >> +{ >> + int rc; >> + >> + rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 : 2)); >> + >> + if ( rc ) >> + return rc; >> + >> + if ( !len ) /* 16-bit instruction */ >> + *instr &= 0xffff; >> + else /* 32-bit instruction */ >> + { >> + /* THUMB 32-bit instruction consisting of 2 consecutive halfwords */ > > Please could you incorporate something like Tim's description from > <20130729144626.GI37169@ocelot.phlegethon.org> to make it totally > obvious what is going on here. > >> + if ( regs->cpsr & PSR_THUMB ) >> + *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16; > > Please can you add a comment like > /* else: already in correct order for an ARM instruction */ > >> + } >> + >> + return 0; >> +} >> + >> static void do_trap_data_abort_guest(struct cpu_user_regs *regs, >> struct hsr_dabt dabt) >> { >> @@ -1021,6 +1044,27 @@ 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 ) >> + { >> + uint32_t instr = 0; >> + >> + rc = read_instruction(regs, dabt.len, &instr); >> + if ( rc ) >> + goto bad_data_abort; >> + >> + /* Retrieve the transfer register from the instruction */ >> + if ( dabt.len ) >> + /* With 32-bit store instruction, the register is in [12..15] */ >> + info.dabt.reg = (instr & 0xf000) >> 12; >> + else >> + /* With 16-bit store instruction, the register is in [0..3] */ >> + info.dabt.reg = instr & 0x7; > > Encoding T2 (store via imm8 offset from sp) has it in 8..10. Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if it "is not using the PC as its destination register". So this instruction is consider as invalid and will go to "bad_data_abort". Is a comment is enough to explain why we don't need to decode it? > > Also for clarity I think you should write "With a NN-bit Thumb store > instruction". > >> + } >> + >> if (handle_mmio(&info)) >> { >> regs->pc += dabt.len ? 4 : 2; >> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h >> index b266252..bc82fbc 100644 >> --- a/xen/include/asm-arm/arm32/processor.h >> +++ b/xen/include/asm-arm/arm32/processor.h >> @@ -111,6 +111,9 @@ 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() (current_cpu_data.midr.bits == 0x410fc0f4) > > Do we have unlikely() in Xen? If yes then I think this is a good place > to use it. Yes. I will use it.
On Tue, 2013-07-30 at 18:37 +0100, Julien Grall wrote: > On 07/29/2013 04:15 PM, Ian Campbell wrote: > > On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote: > >> + /* Retrieve the transfer register from the instruction */ > >> + if ( dabt.len ) > >> + /* With 32-bit store instruction, the register is in [12..15] */ > >> + info.dabt.reg = (instr & 0xf000) >> 12; > >> + else > >> + /* With 16-bit store instruction, the register is in [0..3] */ > >> + info.dabt.reg = instr & 0x7; > > > > Encoding T2 (store via imm8 offset from sp) has it in 8..10. > > Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if > it "is not using the PC as its destination register". So this > instruction is consider as invalid and will go to "bad_data_abort". I'm not sure what this has to do with the encoding I pointed to. A8.8.203 STR (immediate, Thumb), Encoding T2: STR<c> <Rt>, [SP, #<imm>] is: 1001 0ttt mmmm mmmm (ttt=Rt, mmmm mmmm=imm) So Rt is in bits 8..10 which != 0..3 which is all you handle above. I can't see any reason why you wouldn't need to handle this case, it is certainly a valid instruction. I think it would be safest to explicitly check for known opcode patterns and handle those while logging any which we don't recognise. This might be doable with a lookup table but it may be too sparse. If we were doing a more full featured instruction decoder then it might be worth it, not sure about this very specific case. Ian.
On 07/31/2013 09:47 AM, Ian Campbell wrote: > On Tue, 2013-07-30 at 18:37 +0100, Julien Grall wrote: >> On 07/29/2013 04:15 PM, Ian Campbell wrote: >>> On Thu, 2013-07-25 at 16:21 +0100, Julien Grall wrote: > >>>> + /* Retrieve the transfer register from the instruction */ >>>> + if ( dabt.len ) >>>> + /* With 32-bit store instruction, the register is in [12..15] */ >>>> + info.dabt.reg = (instr & 0xf000) >> 12; >>>> + else >>>> + /* With 16-bit store instruction, the register is in [0..3] */ >>>> + info.dabt.reg = instr & 0x7; >>> >>> Encoding T2 (store via imm8 offset from sp) has it in 8..10. >> >> Right but ... from ARM DDI 0406C.b B3-1432: an instruction is valid if >> it "is not using the PC as its destination register". So this >> instruction is consider as invalid and will go to "bad_data_abort". > > I'm not sure what this has to do with the encoding I pointed to. > > A8.8.203 STR (immediate, Thumb), Encoding T2: > STR<c> <Rt>, [SP, #<imm>] > is: > 1001 0ttt mmmm mmmm (ttt=Rt, mmmm mmmm=imm) > Oh right, I read 'pc' instead of 'sp'. > So Rt is in bits 8..10 which != 0..3 which is all you handle above. I > can't see any reason why you wouldn't need to handle this case, it is > certainly a valid instruction. > > I think it would be safest to explicitly check for known opcode patterns > and handle those while logging any which we don't recognise. This might > be doable with a lookup table but it may be too sparse. If we were doing > a more full featured instruction decoder then it might be worth it, not > sure about this very specific case. As discussed with Ian, I will create a function decode_instruction. It will decode and fill the HSR.
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index d6dc37d..3aa2b8c 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -35,6 +35,7 @@ #include <asm/regs.h> #include <asm/cpregs.h> #include <asm/psci.h> +#include <asm/guest_access.h> #include "io.h" #include "vtimer.h" @@ -996,6 +997,28 @@ done: if (first) unmap_domain_page(first); } +static int read_instruction(struct cpu_user_regs *regs, unsigned len, + uint32_t *instr) +{ + int rc; + + rc = raw_copy_from_guest(instr, (void * __user)regs->pc, (len ? 4 : 2)); + + if ( rc ) + return rc; + + if ( !len ) /* 16-bit instruction */ + *instr &= 0xffff; + else /* 32-bit instruction */ + { + /* THUMB 32-bit instruction consisting of 2 consecutive halfwords */ + if ( regs->cpsr & PSR_THUMB ) + *instr = (*instr & 0xffff) << 16 | (*instr & 0xffff0000) >> 16; + } + + return 0; +} + static void do_trap_data_abort_guest(struct cpu_user_regs *regs, struct hsr_dabt dabt) { @@ -1021,6 +1044,27 @@ 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 ) + { + uint32_t instr = 0; + + rc = read_instruction(regs, dabt.len, &instr); + if ( rc ) + goto bad_data_abort; + + /* Retrieve the transfer register from the instruction */ + if ( dabt.len ) + /* With 32-bit store instruction, the register is in [12..15] */ + info.dabt.reg = (instr & 0xf000) >> 12; + else + /* With 16-bit store instruction, the register is in [0..3] */ + info.dabt.reg = instr & 0x7; + } + if (handle_mmio(&info)) { regs->pc += dabt.len ? 4 : 2; diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h index b266252..bc82fbc 100644 --- a/xen/include/asm-arm/arm32/processor.h +++ b/xen/include/asm-arm/arm32/processor.h @@ -111,6 +111,9 @@ 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() (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 d9fbcb2..ac7f1bd 100644 --- a/xen/include/asm-arm/arm64/processor.h +++ b/xen/include/asm-arm/arm64/processor.h @@ -105,6 +105,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 */
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 v2: - Only decode the instruction on affected processor - Handle ARM 32-bit instruction in read_instruction --- xen/arch/arm/traps.c | 44 +++++++++++++++++++++++++++++++++ xen/include/asm-arm/arm32/processor.h | 3 +++ xen/include/asm-arm/arm64/processor.h | 2 ++ 3 files changed, 49 insertions(+)