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

[Balamuruhan S <bala24@linux.ibm.com>]

On Thu, 2020-04-02 at 12:34 +0530, Naveen N. Rao wrote:
> Michael Ellerman wrote:
> > "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> writes:
> > > Balamuruhan S wrote:
> > > > 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
> > ...
> > > > 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.

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

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

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