Message ID | 1380672911-12812-6-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
On Tue, Oct 01, 2013 at 05:15:06PM -0700, Sukadev Bhattiprolu wrote: > Implement is_instr_load_store() to detect whether a given instruction > is one of the fixed-point or floating-point load/store instructions. > This function will be used in a follow-on patch to save memory hierarchy > information of the load/store. The search over the array is a bit of a pity, especially as the worst case penalises you when you haven't hit a load/store. I think we can do better. If you look at the opcode maps, and in particular the extended table for opcode 31, you'll see there's a reasonable amount of structure. The following is only valid for arch 2.06, ie. it will classify reserved opcodes as being load/store, but I think that's fine for the moment. If we need to use it somewhere in future we can update it. But we should add a big comment saying it's only valid in that case. Anyway, I think the following logic is all we need for opcode 31: bool is_load_store(int ext_opcode) { upper = ext_opcode >> 5; lower = ext_opcode & 0x1f; /* Short circuit as many misses as we can */ if (lower < 3 || lower > 23) return false; if (lower == 3) if (upper >= 16) return true; return false; if (lower == 6) if (upper <= 1) return true; return false; if (lower == 7 || lower == 12) return true; if (lower >= 20) /* && lower <= 23 (implicit) */ return true; return false; } Which is not pretty, but I think it's preferable to the full search over the array. cheers
Michael Ellerman [michael@ellerman.id.au] wrote: | On Tue, Oct 01, 2013 at 05:15:06PM -0700, Sukadev Bhattiprolu wrote: | > Implement is_instr_load_store() to detect whether a given instruction | > is one of the fixed-point or floating-point load/store instructions. | > This function will be used in a follow-on patch to save memory hierarchy | > information of the load/store. | | The search over the array is a bit of a pity, especially as the worst | case penalises you when you haven't hit a load/store. Agree. Will try this out. This is certainly more efficient. | | I think we can do better. If you look at the opcode maps, and in | particular the extended table for opcode 31, you'll see there's a | reasonable amount of structure. | | The following is only valid for arch 2.06, ie. it will classify reserved | opcodes as being load/store, but I think that's fine for the moment. If | we need to use it somewhere in future we can update it. But we should | add a big comment saying it's only valid in that case. | | Anyway, I think the following logic is all we need for opcode 31: | | bool is_load_store(int ext_opcode) how about I call this is_load_store_2_06() and add a comment. Horrible but minimizes chance of misuse. | { | upper = ext_opcode >> 5; | lower = ext_opcode & 0x1f; | | /* Short circuit as many misses as we can */ | if (lower < 3 || lower > 23) | return false; | | if (lower == 3) | if (upper >= 16) | return true; | | return false; | | if (lower == 6) | if (upper <= 1) | return true; | return false; | | if (lower == 7 || lower == 12) | return true; | | if (lower >= 20) /* && lower <= 23 (implicit) */ | return true; | | return false; | } | | | Which is not pretty, but I think it's preferable to the full search over the | array. | | cheers
On 10/3/2013 2:03 PM, Sukadev Bhattiprolu wrote: > Michael Ellerman [michael@ellerman.id.au] wrote: <snip> > | > | if (lower == 6) > | if (upper <= 1) > | return true; > | return false; > v Note that this case covers the lvsl/lvsr instructions, which, despite their names are not actually loads. So you could eliminate this check and do just a little bit better.
On Thu, 2013-10-03 at 14:52 -0500, Tom Musta wrote: > On 10/3/2013 2:03 PM, Sukadev Bhattiprolu wrote: > > Michael Ellerman [michael@ellerman.id.au] wrote: > <snip> > > | > > | if (lower == 6) > > | if (upper <= 1) > > | return true; > > | return false; > > v > Note that this case covers the lvsl/lvsr instructions, which, despite their > names are not actually loads. So you could eliminate this check and do > just a little bit better. Yes you're right Tom, thanks for checking. I saw "Load" in the name and that was good enough for me :) cheers
On Thu, Oct 03, 2013 at 12:03:25PM -0700, Sukadev Bhattiprolu wrote: > Michael Ellerman [michael@ellerman.id.au] wrote: > | On Tue, Oct 01, 2013 at 05:15:06PM -0700, Sukadev Bhattiprolu wrote: > | > Implement is_instr_load_store() to detect whether a given instruction > | > is one of the fixed-point or floating-point load/store instructions. > | > This function will be used in a follow-on patch to save memory hierarchy > | > information of the load/store. > | > | Anyway, I think the following logic is all we need for opcode 31: > | > | bool is_load_store(int ext_opcode) > > how about I call this is_load_store_2_06() and add a comment. Horrible > but minimizes chance of misuse. Actually it's is_opcode_31_load_store_2_06() - which is even more horrible :) But you can probably fold it in to the main routine and then call that is_load_store_2_06(). Or whatever seems best, but yeah I think we should make it very clear that it's only for 2.06. cheers
Michael Ellerman [michael@ellerman.id.au] wrote: | bool is_load_store(int ext_opcode) | { | upper = ext_opcode >> 5; | lower = ext_opcode & 0x1f; | | /* Short circuit as many misses as we can */ | if (lower < 3 || lower > 23) | return false; I see some loads/stores like these which are not covered by the above check. Is it ok to ignore them ? lower == 29: ldepx, stdepx, eviddepx, evstddepx lower == 31: lwepx, lbepx, lfdepx, stfdepx, Looking through the opcode maps, I also see these for primary op code 4: evldd, evlddx, evldwx, evldw, evldh, evldhx. Should we include those also ? Sukadev
On Tue, 2013-10-08 at 12:31 -0700, Sukadev Bhattiprolu wrote: > Michael Ellerman [michael@ellerman.id.au] wrote: > | bool is_load_store(int ext_opcode) > | { > | upper = ext_opcode >> 5; > | lower = ext_opcode & 0x1f; > | > | /* Short circuit as many misses as we can */ > | if (lower < 3 || lower > 23) > | return false; > > I see some loads/stores like these which are not covered by > the above check. Is it ok to ignore them ? > > lower == 29: ldepx, stdepx, eviddepx, evstddepx > > lower == 31: lwepx, lbepx, lfdepx, stfdepx, Those are the external process ID instructions, which I've never heard of anyone using, I think we can ignore them. > Looking through the opcode maps, I also see these for primary > op code 4: > > evldd, evlddx, evldwx, evldw, evldh, evldhx. > > Should we include those also ? Yes I think so. I didn't check any of the other opcodes for you. cheers
On Wed, Oct 09, 2013 at 12:03:19PM +1100, Michael Ellerman wrote: > On Tue, 2013-10-08 at 12:31 -0700, Sukadev Bhattiprolu wrote: > > Michael Ellerman [michael@ellerman.id.au] wrote: > > | bool is_load_store(int ext_opcode) > > | { > > | upper = ext_opcode >> 5; > > | lower = ext_opcode & 0x1f; > > | > > | /* Short circuit as many misses as we can */ > > | if (lower < 3 || lower > 23) > > | return false; > > > > I see some loads/stores like these which are not covered by > > the above check. Is it ok to ignore them ? > > > > lower == 29: ldepx, stdepx, eviddepx, evstddepx > > > > lower == 31: lwepx, lbepx, lfdepx, stfdepx, > > Those are the external process ID instructions, which I've never heard > of anyone using, I think we can ignore them. > > > Looking through the opcode maps, I also see these for primary > > op code 4: > > > > evldd, evlddx, evldwx, evldw, evldh, evldhx. > > > > Should we include those also ? > > Yes I think so. I didn't check any of the other opcodes for you. Paul points out these are for the SPE extension, which we also don't care about. So ignore those as well. cheers
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h index a6f8c7a..3e47fe0 100644 --- a/arch/powerpc/include/asm/code-patching.h +++ b/arch/powerpc/include/asm/code-patching.h @@ -34,6 +34,7 @@ int instr_is_branch_to_addr(const unsigned int *instr, unsigned long addr); unsigned long branch_target(const unsigned int *instr); unsigned int translate_branch(const unsigned int *dest, const unsigned int *src); +int instr_is_load_store(const unsigned int *instr); static inline unsigned long ppc_function_entry(void *func) { diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c index 2bc9db3..7e5dc6f 100644 --- a/arch/powerpc/lib/code-patching.c +++ b/arch/powerpc/lib/code-patching.c @@ -159,6 +159,96 @@ unsigned int translate_branch(const unsigned int *dest, const unsigned int *src) return 0; } +static unsigned int load_store_xval(const unsigned int instr) +{ + return (instr >> 1) & 0x3FF; /* bits 21..30 */ +} + +/* + * Values of bits 21:30 of Fixed-point and Floating-point Load and Store + * instructions. + * + * Reference: PowerISA_V2.06B_Public.pdf, Sections 3.3.2 through 3.3.6 and + * 4.6.2 through 4.6.4. + */ +#define x_lbzx 87 +#define x_lbzux 119 +#define x_lhzx 279 +#define x_lhzux 311 +#define x_lhax 343 +#define x_lhaux 375 +#define x_lwzx 23 +#define x_lwzux 55 +#define x_lwax 341 +#define x_lwaux 373 +#define x_ldx 21 +#define x_ldux 53 +#define x_stbx 215 +#define x_stbux 247 +#define x_sthx 407 +#define x_sthux 439 +#define x_stwx 151 +#define x_stwux 183 +#define x_stdx 149 +#define x_stdux 181 +#define x_lhbrx 790 +#define x_lwbrx 534 +#define x_sthbrx 918 +#define x_stwbrx 662 +#define x_ldbrx 532 +#define x_stdbrx 660 +#define x_lswi 597 +#define x_lswx 533 +#define x_stswi 725 +#define x_stswx 661 +#define x_lfsx 535 +#define x_lfsux 567 +#define x_lfdx 599 +#define x_lfdux 631 +#define x_lfiwax 855 +#define x_lfiwzx 887 +#define x_stfsx 663 +#define x_stfsux 695 +#define x_stfdx 727 +#define x_stfdux 759 +#define x_stfiwax 983 +#define x_lfdpx 791 +#define x_stfdpx 919 + +static unsigned int x_form_load_store[] = { + x_lbzx, x_lbzux, x_lhzx, x_lhzux, x_lhax, + x_lhaux, x_lwzx, x_lwzux, x_lwax, x_lwaux, + x_ldx, x_ldux, x_stbx, x_stbux, x_sthx, + x_sthux, x_stwx, x_stwux, x_stdx, x_stdux, + x_lhbrx, x_lwbrx, x_sthbrx, x_stwbrx, x_ldbrx, + x_stdbrx, x_lswi, x_lswx, x_stswi, x_stswx, + x_lfsx, x_lfsux, x_lfdx, x_lfdux, x_lfiwax, + x_lfiwzx, x_stfsx, x_stfsux, x_stfdx, x_stfdux, + x_stfiwax, x_lfdpx, x_stfdpx +}; + +int instr_is_load_store(const unsigned int *instr) +{ + unsigned int op; + int i, n; + + op = instr_opcode(*instr); + + if ((op >= 32 && op <= 58) || (op == 61 || op == 62)) + return 1; + + if (op == 31) { + n = sizeof(x_form_load_store) / sizeof(int); + + for (i = 0; i < n; i++) { + if (x_form_load_store[i] == load_store_xval(*instr)) + return 1; + } + } + + return 0; +} + #ifdef CONFIG_CODE_PATCHING_SELFTEST