Re: [RFC PATCH 3/4] powerpc ppc-opcode: move ppc instuction encoding from test_emulate_step

On Thu, 2020-04-02 at 12:34 +0530, Naveen N. Rao wrote:

Michael Ellerman wrote:

quoted

"Naveen N. Rao" [off-list ref] writes:

quoted

Balamuruhan S wrote:

quoted

Few ppc instructions are encoded in test_emulate_step.c, consolidate
them to
ppc-opcode.h, fix redefintion errors in bpf_jit caused due to this
consolidation.
Reuse the macros from ppc-opcode.h

...

quoted

quoted

diff --git a/arch/powerpc/net/bpf_jit32.h

b/arch/powerpc/net/bpf_jit32.h
index 4ec2a9f14f84..8a9f16a7262e 100644

--- a/arch/powerpc/net/bpf_jit32.h
+++ b/arch/powerpc/net/bpf_jit32.h

@@ -76,13 +76,13 @@ DECLARE_LOAD_FUNC(sk_load_byte_msh);
 		else {	PPC_ADDIS(r, base, IMM_HA(i));			
      \
 			PPC_LBZ(r, r, IMM_L(i)); } } while(0)

-#define PPC_LD_OFFS(r, base, i) do { if ((i) < 32768) PPC_LD(r, base,

i);     \
+#define _OFFS(r, base, i) do { if ((i) < 32768) EMIT(PPC_ENCODE_LD(r,
base, i));     \

	   ^^^^^
Should be PPC_LD_OFFS. For the next version, please also build ppc32 and 
booke codebase to confirm that your changes in those areas are fine.

PPC_ENCODE_* also looks quite verbose, so perhaps PPC_ENC_* might be 
better. Otherwise, this patchset looks good to me and should help reuse 
some of those macros, especially from the eBPF codebase.

Michael,
Can you let us know if this looks ok to you? Based on your feedback, we 
will also update the eBPF codebase.

I didn't really like the first patch which does the mass renaming. It
creates a huge amount of churn.

sorry for that.

quoted

I think I'd be happier if this series just did what it needs, and then
maybe at the end there's a patch to update all the existing names, which
I may or may not take.

Ok.

I will work on it.

quoted

As far as the naming, currently we have:

PPC_INST_FOO - just the opcode

PPC_FOO(x) - macro to encode the opcode with x and (usually) also emit a
            .long and stringify.

And you need an in-between that gives you the full instruction but
without the .long and stringify, right?

Yes.

quoted

So how about PPC_RAW_FOO() for just the numeric value, without the .long
and stringify.

Sure, thanks for the feedback -- that makes sense.

Thanks for the feedback.

quoted

We also seem to have a lot of PPC_INST_FOO's that are only ever used in
the PPC_INST macro. I'm inclined to fold those into the PPC_INST macro,
to avoid people accidentally using the PPC_INST version when they don't
mean to. But that's a separate issue.

Good point -- I do see many uses of PPC_INST_FOO that can be replaced 
with PPC_RAW_FOO once we introduce that. We will take a stab at doing 
this cleanup as a separate patch at the end.

Will make the changes as suggested.

-- Bala

Thanks,
Naveen