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:

quoted

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 ?

quoted

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

quoted

+	/*
+	 * 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?

quoted

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