[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.

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 <sukadev@linux.vnet.ibm.com>
---
Changelog[v6]
	- [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         |   45 ++++++++++++++++++++++++++++++
 2 files changed, 46 insertions(+)

> 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

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.

David Laight [David.Laight@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

Anshuman Khandual [khandual@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 ?