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

On 04.07.2012, at 17:27, Caraman Mihai Claudiu-B02008 wrote:

quoted

-----Original Message-----
From: Alexander Graf [mailto:agraf@suse.de]
Sent: Wednesday, July 04, 2012 5:30 PM
To: Caraman Mihai Claudiu-B02008
Cc: <redacted>; KVM list; linuxppc-dev; qemu-
ppc@nongnu.org List; Benjamin Herrenschmidt
Subject: Re: [Qemu-ppc] [RFC PATCH 12/17] PowerPC: booke64: Add =

DO_KVM

quoted

kernel hooks
=20
=20
On 25.06.2012, at 14:26, Mihai Caraman wrote:
=20

quoted

Hook DO_KVM macro to 64-bit booke in a optimal way similar to 32-bit

booke

quoted

see head_fsl_booke.S file. Extend interrupt handlers' parameter list

with

quoted

interrupt vector numbers to accomodate the macro. Rework Guest =

Doorbell

quoted

quoted

handler to use the proper GSRRx save/restore registers.
Only the bolted version of tlb miss handers is addressed now.
=20
Signed-off-by: Mihai Caraman <redacted>
---
arch/powerpc/kernel/exceptions-64e.S |  114 =

++++++++++++++++++++++++---

quoted

-------

quoted

arch/powerpc/mm/tlb_low_64e.S        |   14 +++-
2 files changed, 92 insertions(+), 36 deletions(-)
=20

diff --git a/arch/powerpc/kernel/exceptions-64e.S

b/arch/powerpc/kernel/exceptions-64e.S

quoted

index 06f7aec..a60f81f 100644

--- a/arch/powerpc/kernel/exceptions-64e.S
+++ b/arch/powerpc/kernel/exceptions-64e.S

@@ -25,6 +25,8 @@

#include <asm/ppc-opcode.h>
#include <asm/mmu.h>
#include <asm/hw_irq.h>
+#include <asm/kvm_asm.h>
+#include <asm/kvm_booke_hv_asm.h>
=20
/* XXX This will ultimately add space for a special exception save
*     structure used to save things like SRR0/SRR1, SPRGs, MAS, =

etc...

quoted

quoted

@@ -34,13 +36,24 @@

*/
#define	SPECIAL_EXC_FRAME_SIZE	INT_FRAME_SIZE
=20
+#ifdef CONFIG_KVM_BOOKE_HV
+#define KVM_BOOKE_HV_MFSPR(reg, spr)				=

\

quoted

quoted

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

=20
Bleks - this is ugly.

=20
I agree :) But I opted to keep the optimizations done for 32-bit.
=20

quoted

Do we really need to open-code the #ifdef here?

=20
32-bit implementation fortunately use asm macros, we can't nest =

defines.

=20

quoted

Can't the feature section code determine that the feature is disabled =

and

quoted

just always not include the code?

=20
CPU_FTR_EMB_HV is set even if KVM is not configured.

I don't get the point then. Why not have the whole DO_KVM masked under =
FTR_SECTION_IFSET(CPU_FTR_EMB_HV)? Are there book3s_64 implementations =
without HV? Can't we just mfspr unconditionally in DO_KVM?

=20

quoted

=20

quoted

+
/* Exception prolog code for all exceptions */
-#define EXCEPTION_PROLOG(n, type, srr0, srr1, addition)

\

quoted

+#define EXCEPTION_PROLOG(n, intnum, type, srr0, srr1, addition)

	    \

quoted

	mtspr	SPRN_SPRG_##type##_SCRATCH,r13;	/* get spare registers =

*/

quoted

\

quoted

	mfspr	r13,SPRN_SPRG_PACA;	/* get PACA */			 =

   \

quoted

quoted

	std	r10,PACA_EX##type+EX_R10(r13);				 =

   \

quoted

quoted

	std	r11,PACA_EX##type+EX_R11(r13);				 =

   \

quoted

quoted

	mfcr	r10;			/* save CR */			 =

   \

quoted

quoted

+	KVM_BOOKE_HV_MFSPR(r11,srr1);			    		 =

   \

quoted

quoted

+	DO_KVM	intnum,srr1;				    		 =

   \

quoted

=20
So if DO_KVM already knows srr1, why explicitly do something with it =

the

quoted

line above, and not in DO_KVM itself?

=20
srr1 is used to expand the interrupt handler symbol name while r11 is =

used

for the actual MSR[GS] optimal check:
	mtocrf	0x80, r11

Right, so basically we want

#ifdef CONFIG_KVM
mfspr r11, spr
mtocrf 0x80, r11
beq ...
#endif

right?

=20

quoted

quoted

-/* Guest Doorbell */
-	MASKABLE_EXCEPTION(0x2c0, guest_doorbell, .unknown_exception,

ACK_NONE)

quoted

+/*
+ *	Guest doorbell interrupt
+ *	This general exception use GSRRx save/restore registers
+ */
+	START_EXCEPTION(guest_doorbell);
+	EXCEPTION_PROLOG(0x2c0, BOOKE_INTERRUPT_GUEST_DBELL, GEN,
+			 SPRN_GSRR0, SPRN_GSRR1, PROLOG_ADDITION_NONE)
+	EXCEPTION_COMMON(0x2c0, PACA_EXGEN, INTS_KEEP)
+	addi	r3,r1,STACK_FRAME_OVERHEAD
+	bl	.save_nvgprs
+	INTS_RESTORE_HARD
+	bl	.unknown_exception
+	b	.ret_from_except

=20
This is independent of DO_KVM, right?

=20
Yes, just kvm_handler definitions in bookehv_interrupts.S depends on =

this.

Then please split it out into a separate patch.

=20

quoted

=20

quoted

=20
/* Guest Doorbell critical Interrupt */
	START_EXCEPTION(guest_doorbell_crit);
-	CRIT_EXCEPTION_PROLOG(0x2e0, PROLOG_ADDITION_NONE)
+	CRIT_EXCEPTION_PROLOG(0x2e0, BOOKE_INTERRUPT_GUEST_DBELL_CRIT,
+			      PROLOG_ADDITION_NONE)

=20
Shouldn't this one also use GSRR?

=20
No, this is a critical exception.

Ah, right. Looked at the wrong bit, sorry :).

=20

quoted

quoted

=20
-.macro tlb_prolog_bolted addr
+.macro tlb_prolog_bolted intnum addr
	mtspr	SPRN_SPRG_TLB_SCRATCH,r13
	mfspr	r13,SPRN_SPRG_PACA
	std	r10,PACA_EXTLB+EX_TLB_R10(r13)
	mfcr	r10
	std	r11,PACA_EXTLB+EX_TLB_R11(r13)
+#ifdef CONFIG_KVM_BOOKE_HV
+BEGIN_FTR_SECTION
+	mfspr	r11, SPRN_SRR1
+END_FTR_SECTION_IFSET(CPU_FTR_EMB_HV)
+#endif

=20
This thing really should vanish behind DO_KVM :)

=20
Then let's do it first for 32-bit ;)

You could #ifdef it in DO_KVM for 64-bit for now. IIRC it's not done on =
32-bit because the register value is used even beyond DO_KVM there.


Alex