Re: [RFC PATCH 1/2]powerpc: foundation code to handle CR5 for local_t

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2014-11-27 at 10:56 -0600, Segher Boessenkool wrote:
> On Thu, Nov 27, 2014 at 05:48:40PM +0530, Madhavan Srinivasan wrote:
> > Here is the design of this patch. Since local_* operations 
> > are only need to be atomic to interrupts (IIUC), patch uses 
> > one of the Condition Register (CR) fields as a flag variable. When 
> > entering the local_*, specific bit in the CR5 field is set
> > and on exit, bit is cleared. CR bit checking is done in the
> > interrupt return path. If CR5[EQ] bit set and if we return 
> > to kernel, we reset to start of local_* operation.
> 
> Have you tested this with (upcoming) GCC 5.0?  GCC now uses CR5,
> and it likes to use it very much, it might be more convenient to
> use e.g. CR1 (which is allocated almost last, only before CR0).

We use CR1 all over the place in your asm code. Any other suggestion ?

What's the damage of -ffixed-cr5 on gcc5 ? won't it just use CR4 or 6
instead ?

> > --- a/arch/powerpc/include/asm/exception-64s.h
> > +++ b/arch/powerpc/include/asm/exception-64s.h
> > @@ -306,7 +306,26 @@ do_kvm_##n:								\
> >  	std	r10,0(r1);		/* make stack chain pointer	*/ \
> >  	std	r0,GPR0(r1);		/* save r0 in stackframe	*/ \
> >  	std	r10,GPR1(r1);		/* save r1 in stackframe	*/ \
> > -	beq	4f;			/* if from kernel mode		*/ \
> > +BEGIN_FTR_SECTION;							   \
> > +	lis	r9,4096;		/* Create a mask with HV and PR */ \
> > +	rldicr	r9,r9,32,31;		/* bits, AND with the MSR       */ \
> > +	mr	r10,r9;			/* to check for Hyp state       */ \
> > +	ori	r9,r9,16384;						   \
> > +	and	r9,r12,r9;						   \
> > +	cmpd	cr3,r10,r9;							   \
> > +	beq	cr3,66f;		/* Jump if we come from Hyp mode*/ \
> > +	mtcrf	0x04,r10;		/* Clear CR5 if coming from usr */ \
> 
> Wow, such nastiness, only to avoid using dot insns (since you need to keep
> the current CR0 value for the following beq 4f).  And CR0 already holds the
> PR bit, so you need only to check the HV bit anyway?  Some restructuring
> would make this a lot simpler and clearer.
> 
> > +	/*
> > +	 * Now that we are about to exit from interrupt, lets check for
> > +	 * cr5 eq bit. If it is set, then we may be in the middle of
> > +	 * local_t update. In this case, we should rewind the NIP
> > +	 * accordingly.
> > +	 */
> > +	mfcr	r3
> > +	andi.	r4,r3,0x200
> > +	beq	63f
> 
> This is just   bne cr5,63f   isn't it?
> 
> > index 72e783e..edb75a9 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -637,7 +637,7 @@ masked_##_H##interrupt:					\
> >  	rldicl	r10,r10,48,1; /* clear MSR_EE */	\
> >  	rotldi	r10,r10,16;				\
> >  	mtspr	SPRN_##_H##SRR1,r10;			\
> > -2:	mtcrf	0x80,r9;				\
> > +2:	mtcrf	0x90,r9;				\
> >  	ld	r9,PACA_EXGEN+EX_R9(r13);		\
> >  	ld	r10,PACA_EXGEN+EX_R10(r13);		\
> >  	ld	r11,PACA_EXGEN+EX_R11(r13);		\
> 
> What does this do?
> 
> 
> Segher
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev