* [V2 PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code
@ 2013-10-31 18:38 Tom
2013-10-31 18:38 ` [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode Tom
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Tom @ 2013-10-31 18:38 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Tom Musta
From: Tom Musta <tommusta@gmail.com>
This patch series addresses bugs in the PowerPC single-step emulation
code (arch/powerpc/lib/sstep.c) pertaining to Little Endian.
The existing code has a chicken switch for little endian. The first
patch softens the restriction so that only cross-endian modes are not
supported.
There is a general problem with unaligned little endian loads and stores.
This is addressed by the second patch.
Finally, there is a problem with unaligned single precision floating point
loads and stores which is addressed by the third patch.
V2: fixed bug in MSR[LE] check identified by Andreas Schwab and
Geert Uytterhoeven.
Tom Musta (3):
powerpc: Enable emulate_step In Little Endian Mode
powerpc: Fix Unaligned Fixed Point Loads and Stores
powerpc: Fix Unaligned LE Floating Point Loads and Stores
arch/powerpc/lib/sstep.c | 109 +++++++++++++++++++++++++++++++++++++++------
1 files changed, 94 insertions(+), 15 deletions(-)
^ permalink raw reply [flat|nested] 14+ messages in thread* [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode 2013-10-31 18:38 [V2 PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code Tom @ 2013-10-31 18:38 ` Tom 2013-11-04 2:28 ` Paul Mackerras 2013-10-31 18:38 ` [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores Tom 2013-10-31 18:38 ` [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating " Tom 2 siblings, 1 reply; 14+ messages in thread From: Tom @ 2013-10-31 18:38 UTC (permalink / raw) To: linuxppc-dev; +Cc: Tom Musta From: Tom Musta <tommusta@gmail.com> This patch modifies the endian chicken switch in the single step emulation code (emulate_step()). The old (big endian) code bailed early if a load or store instruction was to be emulated in little endian mode. The new code modifies the check and only bails in a cross-endian situation (LE mode in a kernel compiled for BE and vice verse). V2: fixed bug in MSR[LE] check identified by Andreas Schwab and Geert Uytterhoeven. Signed-off-by: Tom Musta <tommusta@gmail.com> --- arch/powerpc/lib/sstep.c | 12 +++++++++--- 1 files changed, 9 insertions(+), 3 deletions(-) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index b1faa15..7bfaa9d 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -1222,12 +1222,18 @@ int __kprobes emulate_step(struct pt_regs *regs, unsigned int instr) } /* - * Following cases are for loads and stores, so bail out - * if we're in little-endian mode. + * Following cases are for loads and stores and this + * implementation does not support cross-endian. So + * bail out if this is the case. */ +#ifdef __BIG_ENDIAN__ if (regs->msr & MSR_LE) return 0; - +#endif +#ifdef __LITTLE_ENDIAN__ + if (!(regs->msr & MSR_LE)) + return 0; +#endif /* * Save register RA in case it's an update form load or store * and the access faults. -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode 2013-10-31 18:38 ` [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode Tom @ 2013-11-04 2:28 ` Paul Mackerras 0 siblings, 0 replies; 14+ messages in thread From: Paul Mackerras @ 2013-11-04 2:28 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Thu, Oct 31, 2013 at 01:38:56PM -0500, Tom wrote: > From: Tom Musta <tommusta@gmail.com> > > This patch modifies the endian chicken switch in the single step "Chicken switch" is IBM jargon, perhaps best avoided where possible in commit messages. > emulation code (emulate_step()). The old (big endian) code bailed > early if a load or store instruction was to be emulated in little > endian mode. > > The new code modifies the check and only bails in a cross-endian > situation (LE mode in a kernel compiled for BE and vice verse). The patch adds #ifdefs inside code, which is generally frowned upon in kernel code as it can make the code flow harder to see. Perhaps you could do something like if ((regs->msr & MSR_LE) != (MSR_KERNEL & MSR_LE)) as an alternative that wouldn't require an #ifdef. Or, define a symbol that is 0 in a BE kernel and MSR_LE in a LE kernel, and compare to that. Paul. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores 2013-10-31 18:38 [V2 PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code Tom 2013-10-31 18:38 ` [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode Tom @ 2013-10-31 18:38 ` Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-11-04 2:43 ` Paul Mackerras 2013-10-31 18:38 ` [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating " Tom 2 siblings, 2 replies; 14+ messages in thread From: Tom @ 2013-10-31 18:38 UTC (permalink / raw) To: linuxppc-dev; +Cc: Tom Musta From: Tom Musta <tommusta@gmail.com> This patch modifies the unaligned access routines of the sstep.c module so that it properly reverses the bytes of storage operands in the little endian kernel kernel. Signed-off-by: Tom Musta <tommusta@gmail.com> --- arch/powerpc/lib/sstep.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 45 insertions(+), 0 deletions(-) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index 7bfaa9d..c8743e1 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -212,11 +212,19 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea, { int err; unsigned long x, b, c; +#ifdef __LITTLE_ENDIAN__ + int len = nb; /* save a copy of the length for byte reversal */ +#endif /* unaligned, do this in pieces */ x = 0; for (; nb > 0; nb -= c) { +#ifdef __LITTLE_ENDIAN__ + c = 1; +#endif +#ifdef __BIG_ENDIAN__ c = max_align(ea); +#endif if (c > nb) c = max_align(nb); err = read_mem_aligned(&b, ea, c); @@ -225,7 +233,24 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea, x = (x << (8 * c)) + b; ea += c; } +#ifdef __LITTLE_ENDIAN__ + switch (len) { + case 2: + *dest = byterev_2(x); + break; + case 4: + *dest = byterev_4(x); + break; +#ifdef __powerpc64__ + case 8: + *dest = byterev_8(x); + break; +#endif + } +#endif +#ifdef __BIG_ENDIAN__ *dest = x; +#endif return 0; } @@ -273,9 +298,29 @@ static int __kprobes write_mem_unaligned(unsigned long val, unsigned long ea, int err; unsigned long c; +#ifdef __LITTLE_ENDIAN__ + switch (nb) { + case 2: + val = byterev_2(val); + break; + case 4: + val = byterev_4(val); + break; +#ifdef __powerpc64__ + case 8: + val = byterev_8(val); + break; +#endif + } +#endif /* unaligned or little-endian, do this in pieces */ for (; nb > 0; nb -= c) { +#ifdef __LITTLE_ENDIAN__ + c = 1; +#endif +#ifdef __BIG_ENDIAN__ c = max_align(ea); +#endif if (c > nb) c = max_align(nb); err = write_mem_aligned(val >> (nb - c) * 8, ea, c); -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores 2013-10-31 18:38 ` [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores Tom @ 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-11-04 2:43 ` Paul Mackerras 1 sibling, 0 replies; 14+ messages in thread From: Benjamin Herrenschmidt @ 2013-11-04 2:34 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Thu, 2013-10-31 at 13:38 -0500, Tom wrote: > From: Tom Musta <tommusta@gmail.com> > > This patch modifies the unaligned access routines of the sstep.c > module so that it properly reverses the bytes of storage operands > in the little endian kernel kernel. Do that patch differ from v1 ? (I already merged v1) Cheers, Ben. > Signed-off-by: Tom Musta <tommusta@gmail.com> > --- > arch/powerpc/lib/sstep.c | 45 +++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 45 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > index 7bfaa9d..c8743e1 100644 > --- a/arch/powerpc/lib/sstep.c > +++ b/arch/powerpc/lib/sstep.c > @@ -212,11 +212,19 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea, > { > int err; > unsigned long x, b, c; > +#ifdef __LITTLE_ENDIAN__ > + int len = nb; /* save a copy of the length for byte reversal */ > +#endif > > /* unaligned, do this in pieces */ > x = 0; > for (; nb > 0; nb -= c) { > +#ifdef __LITTLE_ENDIAN__ > + c = 1; > +#endif > +#ifdef __BIG_ENDIAN__ > c = max_align(ea); > +#endif > if (c > nb) > c = max_align(nb); > err = read_mem_aligned(&b, ea, c); > @@ -225,7 +233,24 @@ static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea, > x = (x << (8 * c)) + b; > ea += c; > } > +#ifdef __LITTLE_ENDIAN__ > + switch (len) { > + case 2: > + *dest = byterev_2(x); > + break; > + case 4: > + *dest = byterev_4(x); > + break; > +#ifdef __powerpc64__ > + case 8: > + *dest = byterev_8(x); > + break; > +#endif > + } > +#endif > +#ifdef __BIG_ENDIAN__ > *dest = x; > +#endif > return 0; > } > > @@ -273,9 +298,29 @@ static int __kprobes write_mem_unaligned(unsigned long val, unsigned long ea, > int err; > unsigned long c; > > +#ifdef __LITTLE_ENDIAN__ > + switch (nb) { > + case 2: > + val = byterev_2(val); > + break; > + case 4: > + val = byterev_4(val); > + break; > +#ifdef __powerpc64__ > + case 8: > + val = byterev_8(val); > + break; > +#endif > + } > +#endif > /* unaligned or little-endian, do this in pieces */ > for (; nb > 0; nb -= c) { > +#ifdef __LITTLE_ENDIAN__ > + c = 1; > +#endif > +#ifdef __BIG_ENDIAN__ > c = max_align(ea); > +#endif > if (c > nb) > c = max_align(nb); > err = write_mem_aligned(val >> (nb - c) * 8, ea, c); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores 2013-10-31 18:38 ` [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt @ 2013-11-04 2:43 ` Paul Mackerras 1 sibling, 0 replies; 14+ messages in thread From: Paul Mackerras @ 2013-11-04 2:43 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Thu, Oct 31, 2013 at 01:38:57PM -0500, Tom wrote: > From: Tom Musta <tommusta@gmail.com> > > This patch modifies the unaligned access routines of the sstep.c > module so that it properly reverses the bytes of storage operands > in the little endian kernel kernel. This has rather a lot of #ifdefs inside function definitions, and for little-endian it does the unaligned accesses one byte at a time. You could avoid all the #ifdefs if you define the combining function in an endian-dependant way and make read_mem_unaligned look something like this: #ifdef __LITTLE_ENDIAN__ #define combine_pieces(x, b, c, nd) ((x) + ((b) << (8 * (nd)))) #else #define combine_pieces(x, b, c, nd) (((x) << (8 * (c))) + (b)) #endif static int __kprobes read_mem_unaligned(unsigned long *dest, unsigned long ea, int nb, struct pt_regs *regs) { int err; int nd; unsigned long x, b, c; /* unaligned, do this in pieces */ x = 0; for (nd = 0; nd < nb; nd += c) { c = max_align(ea); if (c > nb - nd) c = max_align(nb - nd); err = read_mem_aligned(&b, ea, c); if (err) return err; x = combine_pieces(x, b, c, nd); ea += c; } *dest = x; return 0; } and do something analogous for write_mem_unaligned(). Paul. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-10-31 18:38 [V2 PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code Tom 2013-10-31 18:38 ` [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode Tom 2013-10-31 18:38 ` [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores Tom @ 2013-10-31 18:38 ` Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-12-11 3:54 ` Paul Mackerras 2 siblings, 2 replies; 14+ messages in thread From: Tom @ 2013-10-31 18:38 UTC (permalink / raw) To: linuxppc-dev; +Cc: Tom Musta From: Tom Musta <tommusta@gmail.com> This patch addresses unaligned single precision floating point loads and stores in the single-step code. The old implementation improperly treated an 8 byte structure as an array of two 4 byte words, which is a classic little endian bug. Signed-off-by: Tom Musta <tommusta@gmail.com> --- arch/powerpc/lib/sstep.c | 52 +++++++++++++++++++++++++++++++++++---------- 1 files changed, 40 insertions(+), 12 deletions(-) diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index c8743e1..1cfd150 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -355,22 +355,36 @@ static int __kprobes do_fp_load(int rn, int (*func)(int, unsigned long), struct pt_regs *regs) { int err; - unsigned long val[sizeof(double) / sizeof(long)]; + union { + double dbl; + unsigned long ul[2]; + struct { +#ifdef __BIG_ENDIAN__ + unsigned _pad_; + unsigned word; +#endif +#ifdef __LITTLE_ENDIAN__ + unsigned word; + unsigned _pad_; +#endif + } single; + } data; unsigned long ptr; if (!address_ok(regs, ea, nb)) return -EFAULT; if ((ea & 3) == 0) return (*func)(rn, ea); - ptr = (unsigned long) &val[0]; + ptr = (unsigned long) &data.ul; if (sizeof(unsigned long) == 8 || nb == 4) { - err = read_mem_unaligned(&val[0], ea, nb, regs); - ptr += sizeof(unsigned long) - nb; + err = read_mem_unaligned(&data.ul[0], ea, nb, regs); + if (nb == 4) + ptr = (unsigned long)&(data.single.word); } else { /* reading a double on 32-bit */ - err = read_mem_unaligned(&val[0], ea, 4, regs); + err = read_mem_unaligned(&data.ul[0], ea, 4, regs); if (!err) - err = read_mem_unaligned(&val[1], ea + 4, 4, regs); + err = read_mem_unaligned(&data.ul[1], ea + 4, 4, regs); } if (err) return err; @@ -382,28 +396,42 @@ static int __kprobes do_fp_store(int rn, int (*func)(int, unsigned long), struct pt_regs *regs) { int err; - unsigned long val[sizeof(double) / sizeof(long)]; + union { + double dbl; + unsigned long ul[2]; + struct { +#ifdef __BIG_ENDIAN__ + unsigned _pad_; + unsigned word; +#endif +#ifdef __LITTLE_ENDIAN__ + unsigned word; + unsigned _pad_; +#endif + } single; + } data; unsigned long ptr; if (!address_ok(regs, ea, nb)) return -EFAULT; if ((ea & 3) == 0) return (*func)(rn, ea); - ptr = (unsigned long) &val[0]; + ptr = (unsigned long) &data.ul[0]; if (sizeof(unsigned long) == 8 || nb == 4) { - ptr += sizeof(unsigned long) - nb; + if (nb == 4) + ptr = (unsigned long)&(data.single.word); err = (*func)(rn, ptr); if (err) return err; - err = write_mem_unaligned(val[0], ea, nb, regs); + err = write_mem_unaligned(data.ul[0], ea, nb, regs); } else { /* writing a double on 32-bit */ err = (*func)(rn, ptr); if (err) return err; - err = write_mem_unaligned(val[0], ea, 4, regs); + err = write_mem_unaligned(data.ul[0], ea, 4, regs); if (!err) - err = write_mem_unaligned(val[1], ea + 4, 4, regs); + err = write_mem_unaligned(data.ul[1], ea + 4, 4, regs); } return err; } -- 1.7.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-10-31 18:38 ` [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating " Tom @ 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-11-04 13:29 ` Tom Musta 2013-12-11 3:54 ` Paul Mackerras 1 sibling, 1 reply; 14+ messages in thread From: Benjamin Herrenschmidt @ 2013-11-04 2:34 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Thu, 2013-10-31 at 13:38 -0500, Tom wrote: > From: Tom Musta <tommusta@gmail.com> > > This patch addresses unaligned single precision floating point loads > and stores in the single-step code. The old implementation > improperly treated an 8 byte structure as an array of two 4 byte > words, which is a classic little endian bug. Do that patch differ from v1 ? I also already merged v1 of this one (the only one I didn't merge is the emulate_step one) Cheers, Ben. > Signed-off-by: Tom Musta <tommusta@gmail.com> > --- > arch/powerpc/lib/sstep.c | 52 +++++++++++++++++++++++++++++++++++---------- > 1 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > index c8743e1..1cfd150 100644 > --- a/arch/powerpc/lib/sstep.c > +++ b/arch/powerpc/lib/sstep.c > @@ -355,22 +355,36 @@ static int __kprobes do_fp_load(int rn, int (*func)(int, unsigned long), > struct pt_regs *regs) > { > int err; > - unsigned long val[sizeof(double) / sizeof(long)]; > + union { > + double dbl; > + unsigned long ul[2]; > + struct { > +#ifdef __BIG_ENDIAN__ > + unsigned _pad_; > + unsigned word; > +#endif > +#ifdef __LITTLE_ENDIAN__ > + unsigned word; > + unsigned _pad_; > +#endif > + } single; > + } data; > unsigned long ptr; > > if (!address_ok(regs, ea, nb)) > return -EFAULT; > if ((ea & 3) == 0) > return (*func)(rn, ea); > - ptr = (unsigned long) &val[0]; > + ptr = (unsigned long) &data.ul; > if (sizeof(unsigned long) == 8 || nb == 4) { > - err = read_mem_unaligned(&val[0], ea, nb, regs); > - ptr += sizeof(unsigned long) - nb; > + err = read_mem_unaligned(&data.ul[0], ea, nb, regs); > + if (nb == 4) > + ptr = (unsigned long)&(data.single.word); > } else { > /* reading a double on 32-bit */ > - err = read_mem_unaligned(&val[0], ea, 4, regs); > + err = read_mem_unaligned(&data.ul[0], ea, 4, regs); > if (!err) > - err = read_mem_unaligned(&val[1], ea + 4, 4, regs); > + err = read_mem_unaligned(&data.ul[1], ea + 4, 4, regs); > } > if (err) > return err; > @@ -382,28 +396,42 @@ static int __kprobes do_fp_store(int rn, int (*func)(int, unsigned long), > struct pt_regs *regs) > { > int err; > - unsigned long val[sizeof(double) / sizeof(long)]; > + union { > + double dbl; > + unsigned long ul[2]; > + struct { > +#ifdef __BIG_ENDIAN__ > + unsigned _pad_; > + unsigned word; > +#endif > +#ifdef __LITTLE_ENDIAN__ > + unsigned word; > + unsigned _pad_; > +#endif > + } single; > + } data; > unsigned long ptr; > > if (!address_ok(regs, ea, nb)) > return -EFAULT; > if ((ea & 3) == 0) > return (*func)(rn, ea); > - ptr = (unsigned long) &val[0]; > + ptr = (unsigned long) &data.ul[0]; > if (sizeof(unsigned long) == 8 || nb == 4) { > - ptr += sizeof(unsigned long) - nb; > + if (nb == 4) > + ptr = (unsigned long)&(data.single.word); > err = (*func)(rn, ptr); > if (err) > return err; > - err = write_mem_unaligned(val[0], ea, nb, regs); > + err = write_mem_unaligned(data.ul[0], ea, nb, regs); > } else { > /* writing a double on 32-bit */ > err = (*func)(rn, ptr); > if (err) > return err; > - err = write_mem_unaligned(val[0], ea, 4, regs); > + err = write_mem_unaligned(data.ul[0], ea, 4, regs); > if (!err) > - err = write_mem_unaligned(val[1], ea + 4, 4, regs); > + err = write_mem_unaligned(data.ul[1], ea + 4, 4, regs); > } > return err; > } ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-11-04 2:34 ` Benjamin Herrenschmidt @ 2013-11-04 13:29 ` Tom Musta 0 siblings, 0 replies; 14+ messages in thread From: Tom Musta @ 2013-11-04 13:29 UTC (permalink / raw) To: Benjamin Herrenschmidt; +Cc: linuxppc-dev On 11/3/2013 8:34 PM, Benjamin Herrenschmidt wrote: > On Thu, 2013-10-31 at 13:38 -0500, Tom wrote: >> From: Tom Musta <tommusta@gmail.com> >> >> This patch addresses unaligned single precision floating point loads >> and stores in the single-step code. The old implementation >> improperly treated an 8 byte structure as an array of two 4 byte >> words, which is a classic little endian bug. > > Do that patch differ from v1 ? I also already merged v1 of this > one (the only one I didn't merge is the emulate_step one) > > Cheers, > Ben. > Ben: Only patch 1/3 (Enable emulate_step in Little Endian Mode) differs in V2. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-10-31 18:38 ` [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating " Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt @ 2013-12-11 3:54 ` Paul Mackerras 2013-12-11 4:57 ` Paul Mackerras 1 sibling, 1 reply; 14+ messages in thread From: Paul Mackerras @ 2013-12-11 3:54 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Thu, Oct 31, 2013 at 01:38:58PM -0500, Tom wrote: > From: Tom Musta <tommusta@gmail.com> > > This patch addresses unaligned single precision floating point loads > and stores in the single-step code. The old implementation > improperly treated an 8 byte structure as an array of two 4 byte > words, which is a classic little endian bug. > > Signed-off-by: Tom Musta <tommusta@gmail.com> > --- > arch/powerpc/lib/sstep.c | 52 +++++++++++++++++++++++++++++++++++---------- > 1 files changed, 40 insertions(+), 12 deletions(-) > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > index c8743e1..1cfd150 100644 > --- a/arch/powerpc/lib/sstep.c > +++ b/arch/powerpc/lib/sstep.c > @@ -355,22 +355,36 @@ static int __kprobes do_fp_load(int rn, int (*func)(int, unsigned long), > struct pt_regs *regs) > { > int err; > - unsigned long val[sizeof(double) / sizeof(long)]; > + union { > + double dbl; > + unsigned long ul[2]; > + struct { > +#ifdef __BIG_ENDIAN__ > + unsigned _pad_; > + unsigned word; > +#endif > +#ifdef __LITTLE_ENDIAN__ > + unsigned word; > + unsigned _pad_; > +#endif > + } single; > + } data; > unsigned long ptr; > > if (!address_ok(regs, ea, nb)) > return -EFAULT; > if ((ea & 3) == 0) > return (*func)(rn, ea); > - ptr = (unsigned long) &val[0]; > + ptr = (unsigned long) &data.ul; > if (sizeof(unsigned long) == 8 || nb == 4) { > - err = read_mem_unaligned(&val[0], ea, nb, regs); > - ptr += sizeof(unsigned long) - nb; > + err = read_mem_unaligned(&data.ul[0], ea, nb, regs); > + if (nb == 4) > + ptr = (unsigned long)&(data.single.word); > } else { > /* reading a double on 32-bit */ > - err = read_mem_unaligned(&val[0], ea, 4, regs); > + err = read_mem_unaligned(&data.ul[0], ea, 4, regs); > if (!err) > - err = read_mem_unaligned(&val[1], ea + 4, 4, regs); > + err = read_mem_unaligned(&data.ul[1], ea + 4, 4, regs); This breaks 32-bit big-endian (as well as making the code longer and more complex). In fact, to make this work for 64-bit little-endian, all you really needed to do was ifdef out the statement: ptr += sizeof(unsigned long) - nb; Paul. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-12-11 3:54 ` Paul Mackerras @ 2013-12-11 4:57 ` Paul Mackerras 2013-12-12 15:08 ` Tom Musta 0 siblings, 1 reply; 14+ messages in thread From: Paul Mackerras @ 2013-12-11 4:57 UTC (permalink / raw) To: Tom; +Cc: linuxppc-dev On Wed, Dec 11, 2013 at 02:54:40PM +1100, Paul Mackerras wrote: > On Thu, Oct 31, 2013 at 01:38:58PM -0500, Tom wrote: > > From: Tom Musta <tommusta@gmail.com> > > > > This patch addresses unaligned single precision floating point loads > > and stores in the single-step code. The old implementation > > improperly treated an 8 byte structure as an array of two 4 byte > > words, which is a classic little endian bug. > > > > Signed-off-by: Tom Musta <tommusta@gmail.com> > > --- > > arch/powerpc/lib/sstep.c | 52 +++++++++++++++++++++++++++++++++++---------- > > 1 files changed, 40 insertions(+), 12 deletions(-) > > > > diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c > > index c8743e1..1cfd150 100644 > > --- a/arch/powerpc/lib/sstep.c > > +++ b/arch/powerpc/lib/sstep.c > > @@ -355,22 +355,36 @@ static int __kprobes do_fp_load(int rn, int (*func)(int, unsigned long), > > struct pt_regs *regs) > > { > > int err; > > - unsigned long val[sizeof(double) / sizeof(long)]; > > + union { > > + double dbl; > > + unsigned long ul[2]; > > + struct { > > +#ifdef __BIG_ENDIAN__ > > + unsigned _pad_; > > + unsigned word; > > +#endif > > +#ifdef __LITTLE_ENDIAN__ > > + unsigned word; > > + unsigned _pad_; > > +#endif > > + } single; > > + } data; > > unsigned long ptr; > > > > if (!address_ok(regs, ea, nb)) > > return -EFAULT; > > if ((ea & 3) == 0) > > return (*func)(rn, ea); > > - ptr = (unsigned long) &val[0]; > > + ptr = (unsigned long) &data.ul; > > if (sizeof(unsigned long) == 8 || nb == 4) { > > - err = read_mem_unaligned(&val[0], ea, nb, regs); > > - ptr += sizeof(unsigned long) - nb; > > + err = read_mem_unaligned(&data.ul[0], ea, nb, regs); > > + if (nb == 4) > > + ptr = (unsigned long)&(data.single.word); > > } else { > > /* reading a double on 32-bit */ > > - err = read_mem_unaligned(&val[0], ea, 4, regs); > > + err = read_mem_unaligned(&data.ul[0], ea, 4, regs); > > if (!err) > > - err = read_mem_unaligned(&val[1], ea + 4, 4, regs); > > + err = read_mem_unaligned(&data.ul[1], ea + 4, 4, regs); > > This breaks 32-bit big-endian (as well as making the code longer and > more complex). And in fact none of this code will get executed in little-endian mode anyway, since we still have this in the middle of emulate_step(): /* * Following cases are for loads and stores, so bail out * if we're in little-endian mode. */ if (regs->msr & MSR_LE) return 0; Paul. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-12-11 4:57 ` Paul Mackerras @ 2013-12-12 15:08 ` Tom Musta 2013-12-12 20:33 ` Tom Musta 0 siblings, 1 reply; 14+ messages in thread From: Tom Musta @ 2013-12-12 15:08 UTC (permalink / raw) To: Paul Mackerras; +Cc: linuxppc-dev On 12/10/2013 10:57 PM, Paul Mackerras wrote: > On Wed, Dec 11, 2013 at 02:54:40PM +1100, Paul Mackerras wrote: >> This breaks 32-bit big-endian (as well as making the code longer and >> more complex). > > And in fact none of this code will get executed in little-endian mode > anyway, since we still have this in the middle of emulate_step(): > > /* > * Following cases are for loads and stores, so bail out > * if we're in little-endian mode. > */ > if (regs->msr & MSR_LE) > return 0; > > Paul. > See patch 1/3 to explain how it becomes relevant in LE. I will take another look at the change. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-12-12 15:08 ` Tom Musta @ 2013-12-12 20:33 ` Tom Musta 2013-12-12 21:19 ` Paul Mackerras 0 siblings, 1 reply; 14+ messages in thread From: Tom Musta @ 2013-12-12 20:33 UTC (permalink / raw) To: Paul Mackerras; +Cc: linuxppc-dev On 12/12/2013 9:08 AM, Tom Musta wrote: > On 12/10/2013 10:57 PM, Paul Mackerras wrote: >> On Wed, Dec 11, 2013 at 02:54:40PM +1100, Paul Mackerras wrote: > >>> This breaks 32-bit big-endian (as well as making the code longer and >>> more complex). >> >> And in fact none of this code will get executed in little-endian mode >> anyway, since we still have this in the middle of emulate_step(): >> >> /* >> * Following cases are for loads and stores, so bail out >> * if we're in little-endian mode. >> */ >> if (regs->msr & MSR_LE) >> return 0; >> >> Paul. >> > > See patch 1/3 to explain how it becomes relevant in LE. > > I will take another look at the change. > It appears that patch 1/3 never got picked up, even though I thought Ben & I had worked through that. And I agree that the code could be simpler. I will work up a patch to address these two issues. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating Point Loads and Stores 2013-12-12 20:33 ` Tom Musta @ 2013-12-12 21:19 ` Paul Mackerras 0 siblings, 0 replies; 14+ messages in thread From: Paul Mackerras @ 2013-12-12 21:19 UTC (permalink / raw) To: Tom Musta; +Cc: linuxppc-dev On Thu, Dec 12, 2013 at 02:33:36PM -0600, Tom Musta wrote: > On 12/12/2013 9:08 AM, Tom Musta wrote: > > On 12/10/2013 10:57 PM, Paul Mackerras wrote: > >> On Wed, Dec 11, 2013 at 02:54:40PM +1100, Paul Mackerras wrote: > > > >>> This breaks 32-bit big-endian (as well as making the code longer and > >>> more complex). > >> > >> And in fact none of this code will get executed in little-endian mode > >> anyway, since we still have this in the middle of emulate_step(): > >> > >> /* > >> * Following cases are for loads and stores, so bail out > >> * if we're in little-endian mode. > >> */ > >> if (regs->msr & MSR_LE) > >> return 0; > >> > >> Paul. > >> > > > > See patch 1/3 to explain how it becomes relevant in LE. > > > > I will take another look at the change. > > > > It appears that patch 1/3 never got picked up, even though I thought Ben & I > had worked through that. > > And I agree that the code could be simpler. I will work up a patch to address > these two issues. The other thing that's important for us to know is how you are testing these changes. For something like this I'd like to see a description of the tests you have done in the commit message. I have been hacking on sstep.c pretty heavily myself recently, so we will need to coordinate on the changes. Paul. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2013-12-12 21:19 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-31 18:38 [V2 PATCH 0/3] powerpc: Fix Little Endian Bugs in Single Step Code Tom 2013-10-31 18:38 ` [V2 PATCH 1/3] powerpc: Enable emulate_step In Little Endian Mode Tom 2013-11-04 2:28 ` Paul Mackerras 2013-10-31 18:38 ` [V2 PATCH 2/3] powerpc: Fix Unaligned Fixed Point Loads and Stores Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-11-04 2:43 ` Paul Mackerras 2013-10-31 18:38 ` [V2 PATCH 3/3] powerpc: Fix Unaligned LE Floating " Tom 2013-11-04 2:34 ` Benjamin Herrenschmidt 2013-11-04 13:29 ` Tom Musta 2013-12-11 3:54 ` Paul Mackerras 2013-12-11 4:57 ` Paul Mackerras 2013-12-12 15:08 ` Tom Musta 2013-12-12 20:33 ` Tom Musta 2013-12-12 21:19 ` Paul Mackerras
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).