| Message ID | 20210204071432.116439-1-sandipan@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v3,1/2] powerpc: sstep: Fix load-store and update emulation | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (44158b256b30415079588d0fcb1bccbdc2ccd009) |
| snowpatch_ozlabs/checkpatch | warning | total: 1 errors, 1 warnings, 0 checks, 20 lines checked |
| snowpatch_ozlabs/needsstable | warning | Please consider tagging this patch for stable! |
On 04/02/21 12:44 pm, Sandipan Das wrote: > The Power ISA says that the fixed-point load and update > instructions must neither use R0 for the base address (RA) > nor have the destination (RT) and the base address (RA) as > the same register. Similarly, for fixed-point stores and > floating-point loads and stores, the instruction is invalid > when R0 is used as the base address (RA). > > This is applicable to the following instructions. > * Load Byte and Zero with Update (lbzu) > * Load Byte and Zero with Update Indexed (lbzux) > * Load Halfword and Zero with Update (lhzu) > * Load Halfword and Zero with Update Indexed (lhzux) > * Load Halfword Algebraic with Update (lhau) > * Load Halfword Algebraic with Update Indexed (lhaux) > * Load Word and Zero with Update (lwzu) > * Load Word and Zero with Update Indexed (lwzux) > * Load Word Algebraic with Update Indexed (lwaux) > * Load Doubleword with Update (ldu) > * Load Doubleword with Update Indexed (ldux) > * Load Floating Single with Update (lfsu) > * Load Floating Single with Update Indexed (lfsux) > * Load Floating Double with Update (lfdu) > * Load Floating Double with Update Indexed (lfdux) > * Store Byte with Update (stbu) > * Store Byte with Update Indexed (stbux) > * Store Halfword with Update (sthu) > * Store Halfword with Update Indexed (sthux) > * Store Word with Update (stwu) > * Store Word with Update Indexed (stwux) > * Store Doubleword with Update (stdu) > * Store Doubleword with Update Indexed (stdux) > * Store Floating Single with Update (stfsu) > * Store Floating Single with Update Indexed (stfsux) > * Store Floating Double with Update (stfdu) > * Store Floating Double with Update Indexed (stfdux) > > E.g. the following behaviour is observed for an invalid > load and update instruction having RA = RT. > > While an userspace program having an instruction word like > 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting > receiving a SIGILL on a Power system (observed on P8 and > P9), the outcome of executing that instruction word varies > and its behaviour can be considered to be undefined. > > Attaching an uprobe at that instruction's address results > in emulation which currently performs the load as well as > writes the effective address back to the base register. > This might not match the outcome from hardware. > > To remove any inconsistencies, this adds additional checks > for the aforementioned instructions to make sure that the > emulation infrastructure treats them as unknown. The kernel > can then fallback to executing such instructions on hardware. > > Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()") > Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> > --- > Previous versions can be found at: > v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/ > v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/ > > Changes in v3: > - Dropped CONFIG_PPC_FPU check as suggested by Michael. > - Consolidated the checks as suggested by Naveen. Sorry. Missed these in the changelog: - Consolidated load/store changes into a single patch. - Included floating-point load/store and update instructions. - Sandipan
On 2021/02/04 12:44PM, Sandipan Das wrote: > The Power ISA says that the fixed-point load and update > instructions must neither use R0 for the base address (RA) > nor have the destination (RT) and the base address (RA) as > the same register. Similarly, for fixed-point stores and > floating-point loads and stores, the instruction is invalid > when R0 is used as the base address (RA). > > This is applicable to the following instructions. > * Load Byte and Zero with Update (lbzu) > * Load Byte and Zero with Update Indexed (lbzux) > * Load Halfword and Zero with Update (lhzu) > * Load Halfword and Zero with Update Indexed (lhzux) > * Load Halfword Algebraic with Update (lhau) > * Load Halfword Algebraic with Update Indexed (lhaux) > * Load Word and Zero with Update (lwzu) > * Load Word and Zero with Update Indexed (lwzux) > * Load Word Algebraic with Update Indexed (lwaux) > * Load Doubleword with Update (ldu) > * Load Doubleword with Update Indexed (ldux) > * Load Floating Single with Update (lfsu) > * Load Floating Single with Update Indexed (lfsux) > * Load Floating Double with Update (lfdu) > * Load Floating Double with Update Indexed (lfdux) > * Store Byte with Update (stbu) > * Store Byte with Update Indexed (stbux) > * Store Halfword with Update (sthu) > * Store Halfword with Update Indexed (sthux) > * Store Word with Update (stwu) > * Store Word with Update Indexed (stwux) > * Store Doubleword with Update (stdu) > * Store Doubleword with Update Indexed (stdux) > * Store Floating Single with Update (stfsu) > * Store Floating Single with Update Indexed (stfsux) > * Store Floating Double with Update (stfdu) > * Store Floating Double with Update Indexed (stfdux) > > E.g. the following behaviour is observed for an invalid > load and update instruction having RA = RT. > > While an userspace program having an instruction word like > 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting > receiving a SIGILL on a Power system (observed on P8 and > P9), the outcome of executing that instruction word varies > and its behaviour can be considered to be undefined. > > Attaching an uprobe at that instruction's address results > in emulation which currently performs the load as well as > writes the effective address back to the base register. > This might not match the outcome from hardware. > > To remove any inconsistencies, this adds additional checks > for the aforementioned instructions to make sure that the > emulation infrastructure treats them as unknown. The kernel > can then fallback to executing such instructions on hardware. > > Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()") > Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> > --- > Previous versions can be found at: > v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/ > v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/ > > Changes in v3: > - Dropped CONFIG_PPC_FPU check as suggested by Michael. > - Consolidated the checks as suggested by Naveen. > > Changes in v2: > - Jump to unknown_opcode instead of returning -1 for invalid > instruction forms. > > --- > arch/powerpc/lib/sstep.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > index e96cff845ef7..9138967eb82e 100644 > --- a/arch/powerpc/lib/sstep.c > +++ b/arch/powerpc/lib/sstep.c > @@ -3017,6 +3017,20 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs, > > } > > + if (OP_IS_LOAD_STORE(op->type) && (op->type & UPDATE)) { > + switch (GETTYPE(op->type)) { > + case LOAD: > + if (ra == rd) > + goto unknown_opcode; > + fallthrough; > + case STORE: > + case LOAD_FP: > + case STORE_FP: > + if (ra == 0) > + goto unknown_opcode; > + } > + } > + > #ifdef CONFIG_VSX > if ((GETTYPE(op->type) == LOAD_VSX || > GETTYPE(op->type) == STORE_VSX) && I'm afraid there is one more thing. scripts/checkpatch.pl reports: WARNING: 'an userspace' may be misspelled - perhaps 'a userspace'? #52: While an userspace program having an instruction word like ^^^^^^^^^^^^ ERROR: switch and case should be at the same indent #96: FILE: arch/powerpc/lib/sstep.c:3021: + switch (GETTYPE(op->type)) { + case LOAD: [...] + case STORE: + case LOAD_FP: + case STORE_FP: - Naveen
On 04/02/21 1:09 pm, Naveen N. Rao wrote: > [...] > > I'm afraid there is one more thing. scripts/checkpatch.pl reports: > > WARNING: 'an userspace' may be misspelled - perhaps 'a userspace'? > #52: > While an userspace program having an instruction word like > ^^^^^^^^^^^^ > > ERROR: switch and case should be at the same indent > #96: FILE: arch/powerpc/lib/sstep.c:3021: > + switch (GETTYPE(op->type)) { > + case LOAD: > [...] > + case STORE: > + case LOAD_FP: > + case STORE_FP: > > Yikes! Thanks for pointing that out. Sending v4. - Sandipan
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index e96cff845ef7..9138967eb82e 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -3017,6 +3017,20 @@ int analyse_instr(struct instruction_op *op, const struct pt_regs *regs, } + if (OP_IS_LOAD_STORE(op->type) && (op->type & UPDATE)) { + switch (GETTYPE(op->type)) { + case LOAD: + if (ra == rd) + goto unknown_opcode; + fallthrough; + case STORE: + case LOAD_FP: + case STORE_FP: + if (ra == 0) + goto unknown_opcode; + } + } + #ifdef CONFIG_VSX if ((GETTYPE(op->type) == LOAD_VSX || GETTYPE(op->type) == STORE_VSX) &&
The Power ISA says that the fixed-point load and update instructions must neither use R0 for the base address (RA) nor have the destination (RT) and the base address (RA) as the same register. Similarly, for fixed-point stores and floating-point loads and stores, the instruction is invalid when R0 is used as the base address (RA). This is applicable to the following instructions. * Load Byte and Zero with Update (lbzu) * Load Byte and Zero with Update Indexed (lbzux) * Load Halfword and Zero with Update (lhzu) * Load Halfword and Zero with Update Indexed (lhzux) * Load Halfword Algebraic with Update (lhau) * Load Halfword Algebraic with Update Indexed (lhaux) * Load Word and Zero with Update (lwzu) * Load Word and Zero with Update Indexed (lwzux) * Load Word Algebraic with Update Indexed (lwaux) * Load Doubleword with Update (ldu) * Load Doubleword with Update Indexed (ldux) * Load Floating Single with Update (lfsu) * Load Floating Single with Update Indexed (lfsux) * Load Floating Double with Update (lfdu) * Load Floating Double with Update Indexed (lfdux) * Store Byte with Update (stbu) * Store Byte with Update Indexed (stbux) * Store Halfword with Update (sthu) * Store Halfword with Update Indexed (sthux) * Store Word with Update (stwu) * Store Word with Update Indexed (stwux) * Store Doubleword with Update (stdu) * Store Doubleword with Update Indexed (stdux) * Store Floating Single with Update (stfsu) * Store Floating Single with Update Indexed (stfsux) * Store Floating Double with Update (stfdu) * Store Floating Double with Update Indexed (stfdux) E.g. the following behaviour is observed for an invalid load and update instruction having RA = RT. While an userspace program having an instruction word like 0xe9ce0001, i.e. ldu r14, 0(r14), runs without getting receiving a SIGILL on a Power system (observed on P8 and P9), the outcome of executing that instruction word varies and its behaviour can be considered to be undefined. Attaching an uprobe at that instruction's address results in emulation which currently performs the load as well as writes the effective address back to the base register. This might not match the outcome from hardware. To remove any inconsistencies, this adds additional checks for the aforementioned instructions to make sure that the emulation infrastructure treats them as unknown. The kernel can then fallback to executing such instructions on hardware. Fixes: 0016a4cf5582 ("powerpc: Emulate most Book I instructions in emulate_step()") Signed-off-by: Sandipan Das <sandipan@linux.ibm.com> --- Previous versions can be found at: v2: https://lore.kernel.org/linuxppc-dev/20210203063841.431063-1-sandipan@linux.ibm.com/ v1: https://lore.kernel.org/linuxppc-dev/20201119054139.244083-1-sandipan@linux.ibm.com/ Changes in v3: - Dropped CONFIG_PPC_FPU check as suggested by Michael. - Consolidated the checks as suggested by Naveen. Changes in v2: - Jump to unknown_opcode instead of returning -1 for invalid instruction forms. --- arch/powerpc/lib/sstep.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)