diff mbox

[5/9,v5] powerpc: implement is_instr_load_store().

Message ID 1380672911-12812-6-git-send-email-sukadev@linux.vnet.ibm.com (mailing list archive)
State Superseded
Headers show

Commit Message

Sukadev Bhattiprolu Oct. 2, 2013, 12:15 a.m. UTC 
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.

Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/include/asm/code-patching.h |    1 +
 arch/powerpc/lib/code-patching.c         |   90 ++++++++++++++++++++++++++++++
 2 files changed, 91 insertions(+)

Comments

Michael Ellerman Oct. 3, 2013, 5:35 a.m. UTC | #1 
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
Sukadev Bhattiprolu Oct. 3, 2013, 7:03 p.m. UTC | #2 
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
Tom Musta Oct. 3, 2013, 7:52 p.m. UTC | #3 
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.
Michael Ellerman Oct. 8, 2013, 3:28 a.m. UTC | #4 
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
Michael Ellerman Oct. 8, 2013, 4 a.m. UTC | #5 
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
Sukadev Bhattiprolu Oct. 8, 2013, 7:31 p.m. UTC | #6 
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
Michael Ellerman Oct. 9, 2013, 1:03 a.m. UTC | #7 
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
Michael Ellerman Oct. 9, 2013, 1:27 a.m. UTC | #8 
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 mbox

Patch

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