| Message ID | CAJ-hshaxuJBMCFquT7J50rjuDYWer_0i1=2QmnOKGAxx4n=FUw@mail.gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Sep 06, 2013 at 10:13:00AM -0500, Tom Musta wrote: > To: linuxppc-dev@lists.ozlabs.org > Subject: [PATCH] powerpc: OE=1 Form Instructions Not Decoded Correctly > From: Tom Musta <tommusta@gmail.com> > > PowerISA uses instruction bit 21 to indicate that the overflow (OV) bit > of the XER is to be set, as well as its corresponding sticky bit (SO). > This patch addresses two defects in the implementation of the PowerISA > single step code for this category of instructions: (a) the OE=1 case > is not correctly accounted for in the case statement for the extended > opcode handling. (b) the implementation is not setting XER[OV] and > XER[SO]. Are you seeing any actual problems arising from the OE=1 instructions not being emulated? This code was designed primarily for emulating instructions in the kernel, which is written in C, and the C compiler doesn't emit OE=1 instructions -- or at least it didn't in the past. So, does the impetus for this change come because the C compiler is now emitting these instructions, or because this code is being used on non-kernel instructions, or just for completeness? Your patch description needs to include answers to these kinds of questions. Also, you need to indent your code correctly according to Documentation/CodingStyle. Paul.
On Mon, 2013-09-09 at 08:35 +1000, Paul Mackerras wrote: > On Fri, Sep 06, 2013 at 10:13:00AM -0500, Tom Musta wrote: > > To: linuxppc-dev@lists.ozlabs.org > > Subject: [PATCH] powerpc: OE=1 Form Instructions Not Decoded Correctly > > From: Tom Musta <tommusta@gmail.com> > > > > PowerISA uses instruction bit 21 to indicate that the overflow (OV) bit > > of the XER is to be set, as well as its corresponding sticky bit (SO). > > This patch addresses two defects in the implementation of the PowerISA > > single step code for this category of instructions: (a) the OE=1 case > > is not correctly accounted for in the case statement for the extended > > opcode handling. (b) the implementation is not setting XER[OV] and > > XER[SO]. > > Are you seeing any actual problems arising from the OE=1 instructions > not being emulated? This code was designed primarily for emulating > instructions in the kernel, which is written in C, and the C compiler > doesn't emit OE=1 instructions -- or at least it didn't in the past. > So, does the impetus for this change come because the C compiler is > now emitting these instructions, or because this code is being used on > non-kernel instructions, or just for completeness? Your patch > description needs to include answers to these kinds of questions. Isn't that code occasionally used with uprobes too nowadays ? Cheers, Ben. > Also, you need to indent your code correctly according to > Documentation/CodingStyle. > > Paul. > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
On Mon, 2013-09-09 at 08:35 +1000, Paul Mackerras wrote: > > Also, you need to indent your code correctly according to > Documentation/CodingStyle. Looks like the patch has been mangled by the mailer ... it's in HTML to begin with :-) Tom, you need to find a mailer setup that works for sending patches, basically that sends them in plain text without any mangling of tabs and spaces and without word wrapping. I personally use "evolution" under Linux which has a "preformatted" style mode for that but I'm sure there are plenty of other options. Cheers, Ben.
> Looks like the patch has been mangled by the mailer ... it's in HTML to > begin with :-) Frustrating. I did use a plain text option on the mail client and did a test send to a few accounts before posting to the mailing list. The patch was verified by checkpatch.pl before it got mangled. I will find a better setup and resubmit. > Isn't that code occasionally used with uprobes too nowadays ? Yes. I believe so. Tom Musta (tmusta@us.ibm.com) Senior Software Engineer Blue Gene Kernel Development IBM Rochester (507) 253-4119 (T/L 553-4119)
> > Isn't that code occasionally used with uprobes too nowadays ? > > Yes. I believe so. I'm going to back-pedal a little. I reread code and can connect single step code to kprobes but not necessarily to uprobes. So I am not sure that this code is used with uprobes.
Hi Tom, On Mon, 9 Sep 2013 07:44:54 -0500 Tom Musta <tmusta@us.ibm.com> wrote: > > > Looks like the patch has been mangled by the mailer ... it's in HTML to > > begin with :-) > > Frustrating. I did use a plain text option on the mail client and did a > test send to a few accounts before posting to the mailing list. The > patch was verified by checkpatch.pl before it got mangled. > > I will find a better setup and resubmit. Also please find a mailer that does *not* respond to the envelope recipient (linuxppc-dev-bounces+tmusta=us.ibm.com@lists.ozlabs.org in this case) as that just tends to irritate list owners.
On Mon, Sep 09, 2013 at 03:20:58PM -0500, Tom Musta wrote: > > > Isn't that code occasionally used with uprobes too nowadays ? > > > > Yes. I believe so. > > I'm going to back-pedal a little. I reread code and can connect > single step code to kprobes but not necessarily to uprobes. So > I am not sure that this code is used with uprobes. Yes, emulate_step() is used for uprobes too. Ananth
diff --git a/arch/powerpc/lib/sstep.c b/arch/powerpc/lib/sstep.c index d220b88..a4f343d 100644 --- a/arch/powerpc/lib/sstep.c +++ b/arch/powerpc/lib/sstep.c @@ -31,6 +31,13 @@ extern char system_call_common[]; #define XER_OV 0x40000000U #define XER_CA 0x20000000U +#define U32_MSK 0xFFFFFFFF00000000ul +#define L32_MSK 0x00000000FFFFFFFFul +#define SGN_32_MSK 0x0000000080000000ul +#define SGN_64_MSK 0x8000000000000000ul +#define SGN_EXT_32(x) (((x) & SGN_32_MSK) ? ((x) | U32_MSK) : ((x) & L32_MSK)) +#define L32(x) ((x) & L32_MSK) + #ifdef CONFIG_PPC_FPU /*