Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add DO_KVM kernel hooks

On Wed, 2012-07-04 at 16:29 +0200, Alexander Graf wrote:
 

quoted

+#ifdef CONFIG_KVM_BOOKE_HV
+#define KVM_BOOKE_HV_MFSPR(reg, spr)				\
+	BEGIN_FTR_SECTION					\
+		mfspr	reg, spr;			  	\
+	END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
+#else
+#define KVM_BOOKE_HV_MFSPR(reg, spr)
+#endif

Bleks - this is ugly. Do we really need to open-code the #ifdef here?
Can't the feature section code determine that the feature is disabled
and just always not include the code?

You can't but in any case I don't see the point of the conditional here,
we'll eventually have to load srr1 no ? We can move the load up to here
in all cases or can't we ? If really not, we could have it inside DO_KVM
and be done with it no ?

quoted

+
/* Exception prolog code for all exceptions */
-#define EXCEPTION_PROLOG(n, type, srr0, srr1, addition)		     	    \
+#define EXCEPTION_PROLOG(n, intnum, type, srr0, srr1, addition)		    \
	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers */   \
	mfspr	r13,SPRN_SPRG_PACA;	/* get PACA */			    \
	std	r10,PACA_EX##type+EX_R10(r13);				    \
	std	r11,PACA_EX##type+EX_R11(r13);				    \
	mfcr	r10;			/* save CR */			    \
+	KVM_BOOKE_HV_MFSPR(r11,srr1);			    		    \
+	DO_KVM	intnum,srr1;				    		    \

So if DO_KVM already knows srr1, why explicitly do something with it
the line above, and not in DO_KVM itself?

Yeah that or just move things around in the prolog.

quoted

	addition;			/* additional code for that exc. */ \
	std	r1,PACA_EX##type+EX_R1(r13); /* save old r1 in the PACA */  \
	stw	r10,PACA_EX##type+EX_CR(r13); /* save old CR in the PACA */ \

@@ -69,17 +82,21 @@

	ld	r1,PACA_MC_STACK(r13);					    \
	subi	r1,r1,SPECIAL_EXC_FRAME_SIZE;

-#define NORMAL_EXCEPTION_PROLOG(n, addition)				    \
-	EXCEPTION_PROLOG(n, GEN, SPRN_SRR0, SPRN_SRR1, addition##_GEN(n))
+#define NORMAL_EXCEPTION_PROLOG(n, intnum, addition)			    \
+	EXCEPTION_PROLOG(n, intnum, GEN, SPRN_SRR0, SPRN_SRR1,		    \

We would we want to pass in 2 numbers? Let's please confine this onto
a single ID per interrupt vector. Either we use the hardcoded ones
available here in the KVM code or we use the KVM ones instead of the
hardcoded ones here. But not both please. Just because it's like that
on 32bit doesn't count as an excuse :).

Right. Also I already objected to the explicit passing of the srr's
anyway.

Cheers,
Ben.