Re: [PATCH 02/10][v6] powerpc/Power7: detect load/store instructions
|
| How about I add this to the function header ?
|
| * Please use the table in Appendix F (opcode maps) to determine
| * events selected by this function.
Here is the updated patch with the comment.
---
>From 38d1f9ac67a7f50db593e5875a8de6a2ecbea8e0 Mon Sep 17 00:00:00 2001
From: Sukadev Bhattiprolu
Date: Fri, 23 Aug 2013 18:35:02 -0700
Subject: [PATCH 6/10][v6] powerpc/Power7: detect load/store instructions
Implement instr_is_load_store_2_06() to detect whether a given instruction
is one of the fixed-point or floating-point load/store instructions in the
POWER Instruction Set Architecture v2.06.
This function will be used in a follow-on patch to save memory hierarchy
information of the load/store on a Power7 system. (Power8 systems set some
bits in the SIER to identify load/store operations and hence don't need a
similar functionality).
Based on optimized code from Michael Ellerman and comments from Tom Musta.
Signed-off-by: Sukadev Bhattiprolu
---
Changelog[v6]
- [David Laight, Anshuman Khandual] Add a comment in function
header to help better understand which instructions are selected
by the instr_is_load_store_2_06().
- [Michael Ellerman, Tom Musta]: Optmize the implementation to
avoid for loop.
arch/powerpc/include/asm/code-patching.h |1 +
arch/powerpc/lib/code-patching.c | 48 ++
2 files changed, 49 insertions(+)
diff --git a/arch/powerpc/include/asm/code-patching.h
b/arch/powerpc/include/asm/code-patching.h
index a6f8c7a..9cc3ef1 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_2_06(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..84571aa 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -159,6 +159,54 @@ unsigned int translate_branch(const unsigned int *dest,
const unsigned int *src)
return 0;
}
+/*
+ * Determine if the op code in the instruction corresponds to a load or
+ * store instruction. Ignore the vector load instructions like evlddepx,
+ * evstddepx for now.
+ *
+ * This function is valid for POWER ISA 2.06.
+ *
+ * Reference: PowerISA_V2.06B_Public.pdf, Sections 3.3.2 through 3.3.6
+ * and 4.6.2 through 4.6.4, Appendix F (Opcode Maps).
+ *
+ * Use the tables in Appendix F (Opcode Maps) to identify
+ * instructions selected by this function.
+ */
+int instr_is_load_store_2_06(const unsigned int *instr)
+{
+ unsigned int op, upper, lower;
+
+ op = instr_opcode(*instr);
+
+ if ((op >= 32 && op <= 58) || (op == 61 || op == 62))
+ return true;
+
+ if (op != 31)
+ return false;
+
+ upper = op >> 5;
+ lower = op & 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 == 7 || lower == 12)
+ return true;
+
+ if (lower >= 20) /* && lower <= 23 (implicit) */
+ return true;
+
+ return false;
+}
+
#ifdef CONFIG_CODE_PATCHING_SELFTEST
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/10][v6] powerpc/Power7: detect load/store instructions
Anshuman Khandual [khand...@linux.vnet.ibm.com] wrote:
| On 10/16/2013 01:55 PM, David Laight wrote:
| >> Implement instr_is_load_store_2_06() to detect whether a given instruction
| >> is one of the fixed-point or floating-point load/store instructions in the
| >> POWER Instruction Set Architecture v2.06.
| > ...
|
| The op code encoding is dependent on the ISA version ? Does the basic load
| and store instructions change with newer ISA versions ?
TBH, I don't know whether the encoding is dependent on the ISA version.
We need this for a very narrow/specific purpose on Power7 _and_ did not
want to set up expectations that it will work with all versions. Hence
the horribly named function :-)
| BTW we have got a
| newer version for the ISA "PowerISA_V2.07_PUBLIC.pdf" here at power.org
|
| https://www.power.org/documentation/power-isa-version-2-07/
Yes, but on Power8 there is a bit in the SIER that tells us whether it
is a load or store instruction. We use that and don't need to determine
in software.
Power7 does not have such a bit and we need this only for Power7. We are
not targetting this "memory hierarchy" feature for Power6 or older processors.
|
| Does not sound like a good idea to analyse the instructions with functions
| names which specify ISA version number. Besides, this function does not
| belong to specific processor or platform. It has to be bit generic.
|
| >> +int instr_is_load_store_2_06(const unsigned int *instr)
| >> +{
| >> + unsigned int op, upper, lower;
| >> +
| >> + op = instr_opcode(*instr);
| >> +
| >> + if ((op >= 32 && op <= 58) || (op == 61 || op == 62))
| >> + return true;
| >> +
| >> + if (op != 31)
| >> + return false;
| >> +
| >> + upper = op >> 5;
| >> + lower = op & 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 == 7 || lower == 12)
| >> + return true;
| >> +
| >> + if (lower >= 20) /* && lower <= 23 (implicit) */
| >> + return true;
| >> +
| >> + return false;
| >> +}
| >
| > I can't help feeling the code could do with some comments about
| > which actual instructions are selected where.
|
| Yeah, I agree. At least which category of load-store instructions are
| getting selected in each case.
Like I mentioned in the other message, how about adding a couple
of lines in the function header ?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/10][v6] powerpc/Power7: detect load/store instructions
David Laight [david.lai...@aculab.com] wrote: | | I can't help feeling the code could do with some comments about | which actual instructions are selected where. At a high level, only the load and store instructions are selected. I added a reference to the Appendix F (Opcode maps) in the function header. The opcode maps is a table of upper x lower values. From that table it should be fairly straightforward which instructions are selected. How about I add this to the function header ? * Please use the table in Appendix F (opcode maps) to determine * events selected by this function. There are over 100 instructions selected by this list and wasn't sure if we should list them all. Sukadev -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 02/10][v6] powerpc/Power7: detect load/store instructions
On 10/16/2013 01:55 PM, David Laight wrote:
>> Implement instr_is_load_store_2_06() to detect whether a given instruction
>> is one of the fixed-point or floating-point load/store instructions in the
>> POWER Instruction Set Architecture v2.06.
> ...
The op code encoding is dependent on the ISA version ? Does the basic load
and store instructions change with newer ISA versions ? BTW we have got a
newer version for the ISA "PowerISA_V2.07_PUBLIC.pdf" here at power.org
https://www.power.org/documentation/power-isa-version-2-07/
Does not sound like a good idea to analyse the instructions with functions
names which specify ISA version number. Besides, this function does not
belong to specific processor or platform. It has to be bit generic.
>> +int instr_is_load_store_2_06(const unsigned int *instr)
>> +{
>> +unsigned int op, upper, lower;
>> +
>> +op = instr_opcode(*instr);
>> +
>> +if ((op >= 32 && op <= 58) || (op == 61 || op == 62))
>> +return true;
>> +
>> +if (op != 31)
>> +return false;
>> +
>> +upper = op >> 5;
>> +lower = op & 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 == 7 || lower == 12)
>> +return true;
>> +
>> +if (lower >= 20) /* && lower <= 23 (implicit) */
>> +return true;
>> +
>> +return false;
>> +}
>
> I can't help feeling the code could do with some comments about
> which actual instructions are selected where.
Yeah, I agree. At least which category of load-store instructions are
getting selected in each case.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 02/10][v6] powerpc/Power7: detect load/store instructions
> Implement instr_is_load_store_2_06() to detect whether a given instruction
> is one of the fixed-point or floating-point load/store instructions in the
> POWER Instruction Set Architecture v2.06.
...
> +int instr_is_load_store_2_06(const unsigned int *instr)
> +{
> + unsigned int op, upper, lower;
> +
> + op = instr_opcode(*instr);
> +
> + if ((op >= 32 && op <= 58) || (op == 61 || op == 62))
> + return true;
> +
> + if (op != 31)
> + return false;
> +
> + upper = op >> 5;
> + lower = op & 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 == 7 || lower == 12)
> + return true;
> +
> + if (lower >= 20) /* && lower <= 23 (implicit) */
> + return true;
> +
> + return false;
> +}
I can't help feeling the code could do with some comments about
which actual instructions are selected where.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
