[RFC,5/8] powerpc/slb: Add documentation to runtime patching of SLB encoding

From: "khandual@linux.vnet.ibm.com" <khandual@linux.vnet.ibm.com>

This patch adds some documentation to 'patch_slb_encoding' function
explaining about how it clears the existing immediate value in the
given instruction and inserts a new one there.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
 arch/powerpc/mm/slb.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

On Tue, 2015-07-21 at 12:28 +0530, Anshuman Khandual wrote:
> From: "khandual@linux.vnet.ibm.com" <khandual@linux.vnet.ibm.com>
> 
> This patch adds some documentation to 'patch_slb_encoding' function
> explaining about how it clears the existing immediate value in the
> given instruction and inserts a new one there.
> 
> diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> index dcba4c2..8083a9e 100644
> --- a/arch/powerpc/mm/slb.c
> +++ b/arch/powerpc/mm/slb.c
> @@ -278,7 +278,13 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
>  static inline void patch_slb_encoding(unsigned int *insn_addr,
>  				      unsigned int immed)
>  {
> -	int insn = (*insn_addr & 0xffff0000) | immed;
> +	/*
> +	 * Currently this patches only "li" and "cmpldi"
> +	 * instructions with an immediate value. Here it
> +	 * just clears the existing immediate value from
> +	 * the instruction and inserts a new one there.
> +	 */
> +	unsigned int insn = (*insn_addr & 0xffff0000) | immed;
>  	patch_instruction(insn_addr, insn);
>  }


How about:

	/*
	 * This function patches either an li or a cmpldi instruction with
	 * a new immediate value. This relies on the fact that both li
	 * (which is actually ori) and cmpldi both take a 16-bit immediate
	 * value, and it is situated in the same location in the instruction,
	 * ie. bits 0-15.
	 * To patch the value we read the existing instruction, clear the
	 * immediate value, and or in our new value, then write the instruction
	 * back.
	 */

cheers

On Wed, Jul 22, 2015 at 03:51:03PM +1000, Michael Ellerman wrote:
> On Tue, 2015-07-21 at 12:28 +0530, Anshuman Khandual wrote:
> > From: "khandual@linux.vnet.ibm.com" <khandual@linux.vnet.ibm.com>
> > 
> > This patch adds some documentation to 'patch_slb_encoding' function
> > explaining about how it clears the existing immediate value in the
> > given instruction and inserts a new one there.
> > 
> > diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> > index dcba4c2..8083a9e 100644
> > --- a/arch/powerpc/mm/slb.c
> > +++ b/arch/powerpc/mm/slb.c
> > @@ -278,7 +278,13 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
> >  static inline void patch_slb_encoding(unsigned int *insn_addr,
> >  				      unsigned int immed)
> >  {
> > -	int insn = (*insn_addr & 0xffff0000) | immed;
> > +	/*
> > +	 * Currently this patches only "li" and "cmpldi"
> > +	 * instructions with an immediate value. Here it
> > +	 * just clears the existing immediate value from
> > +	 * the instruction and inserts a new one there.
> > +	 */
> > +	unsigned int insn = (*insn_addr & 0xffff0000) | immed;
> >  	patch_instruction(insn_addr, insn);
> >  }
> 
> 
> How about:
> 
> 	/*
> 	 * This function patches either an li or a cmpldi instruction with
> 	 * a new immediate value. This relies on the fact that both li
> 	 * (which is actually ori) and cmpldi both take a 16-bit immediate

Hmm, li is actually encoded as addi with r0 as source register...

> 	 * value, and it is situated in the same location in the instruction,
> 	 * ie. bits 0-15.

In PPC documentation, it's rather bits 16-31 (big endian bit order).
Or say lower half which is endian agnostic.

    Cheers,
    Gabriel

On Wed, 2015-07-22 at 07:57 +0200, Gabriel Paubert wrote:
> On Wed, Jul 22, 2015 at 03:51:03PM +1000, Michael Ellerman wrote:
> > On Tue, 2015-07-21 at 12:28 +0530, Anshuman Khandual wrote:
> > > From: "khandual@linux.vnet.ibm.com" <khandual@linux.vnet.ibm.com>
> > > 
> > > This patch adds some documentation to 'patch_slb_encoding' function
> > > explaining about how it clears the existing immediate value in the
> > > given instruction and inserts a new one there.
> > > 
> > > diff --git a/arch/powerpc/mm/slb.c b/arch/powerpc/mm/slb.c
> > > index dcba4c2..8083a9e 100644
> > > --- a/arch/powerpc/mm/slb.c
> > > +++ b/arch/powerpc/mm/slb.c
> > > @@ -278,7 +278,13 @@ void switch_slb(struct task_struct *tsk, struct mm_struct *mm)
> > >  static inline void patch_slb_encoding(unsigned int *insn_addr,
> > >  				      unsigned int immed)
> > >  {
> > > -	int insn = (*insn_addr & 0xffff0000) | immed;
> > > +	/*
> > > +	 * Currently this patches only "li" and "cmpldi"
> > > +	 * instructions with an immediate value. Here it
> > > +	 * just clears the existing immediate value from
> > > +	 * the instruction and inserts a new one there.
> > > +	 */
> > > +	unsigned int insn = (*insn_addr & 0xffff0000) | immed;
> > >  	patch_instruction(insn_addr, insn);
> > >  }
> > 
> > 
> > How about:
> > 
> > 	/*
> > 	 * This function patches either an li or a cmpldi instruction with
> > 	 * a new immediate value. This relies on the fact that both li
> > 	 * (which is actually ori) and cmpldi both take a 16-bit immediate
> 
> Hmm, li is actually encoded as addi with r0 as source register...

Correct.

> > 	 * value, and it is situated in the same location in the instruction,
> > 	 * ie. bits 0-15.
> 
> In PPC documentation, it's rather bits 16-31 (big endian bit order).
> Or say lower half which is endian agnostic.

Yeah, but who reads the PPC documentation ;)

In the kernel we almost always use the sane bit numbering, so I'd use that, but
maybe "low 16-bits" will avoid confusion.

cheers

On Wed, Jul 22, 2015 at 03:51:03PM +1000, Michael Ellerman wrote:
> How about:
> 
> 	/*
> 	 * This function patches either an li or a cmpldi instruction with
> 	 * a new immediate value. This relies on the fact that both li
> 	 * (which is actually ori) and cmpldi both take a 16-bit immediate
> 	 * value, and it is situated in the same location in the instruction,
> 	 * ie. bits 0-15.
> 	 * To patch the value we read the existing instruction, clear the
> 	 * immediate value, and or in our new value, then write the instruction
> 	 * back.
> 	 */

As Gabriel says, li is addi.  It takes a 16-bit sign-extended immediate,
while cmpldi takes a 16-bit zero-extended immediate.  This function
doesn't deal with that difference, it probably should (I didn't check if
the callers take care; there should be an assertion somewhere).


Segher