Re: [PATCH 2/8] KVM: PPC: Book3S PR: Keep volatile reg values in vcpu rather than shadow_vcpu

From: Paul Mackerras <paulus@samba.org>
To: Alexander Graf <agraf@suse.de>
Cc: kvm-ppc@vger.kernel.org, kvm@vger.kernel.org
Subject: Re: [PATCH 2/8] KVM: PPC: Book3S PR: Keep volatile reg values in vcpu rather than shadow_vcpu
Date: Sat, 3 Aug 2013 12:00:13 +1000	[thread overview]
Message-ID: <20130803020013.GA11119@iris.ozlabs.ibm.com> (raw)
In-Reply-To: <B57170FF-3C03-42DC-A011-FF5EF3297723@suse.de>

On Thu, Jul 25, 2013 at 03:54:17PM +0200, Alexander Graf wrote:
> 
> On 11.07.2013, at 13:50, Paul Mackerras wrote:
> 
> > diff --git a/arch/powerpc/kvm/book3s_interrupts.S b/arch/powerpc/kvm/book3s_interrupts.S
> > index 17cfae5..c935195 100644
> > --- a/arch/powerpc/kvm/book3s_interrupts.S
> > +++ b/arch/powerpc/kvm/book3s_interrupts.S
> > @@ -26,8 +26,12 @@
> > 
> > #if defined(CONFIG_PPC_BOOK3S_64)
> > #define FUNC(name) 		GLUE(.,name)
> > +#define GET_SHADOW_VCPU(reg)    mr	reg, r13
> 
> Is this correct?

Yes, it's just a copy of what's in book3s_segment.S already.  For
64-bit the shadow vcpu is in the PACA (and the offsets defined in
asm-offsets.c are expressed as offsets from the PACA).

> > kvm_start_lightweight:
> > +	/* Copy registers into shadow vcpu so we can access them in real mode */
> > +	GET_SHADOW_VCPU(r3)
> > +	PPC_LL	r5, VCPU_GPR(R0)(r4)
> > +	PPC_LL	r6, VCPU_GPR(R1)(r4)
> > +	PPC_LL	r7, VCPU_GPR(R2)(r4)
> > +	PPC_LL	r8, VCPU_GPR(R3)(r4)
> > +	PPC_STL	r5, SVCPU_R0(r3)
> > +	PPC_STL	r6, SVCPU_R1(r3)
> > +	PPC_STL	r7, SVCPU_R2(r3)
> > +	PPC_STL	r8, SVCPU_R3(r3)
> > +	PPC_LL	r5, VCPU_GPR(R4)(r4)
> > +	PPC_LL	r6, VCPU_GPR(R5)(r4)
> > +	PPC_LL	r7, VCPU_GPR(R6)(r4)
> > +	PPC_LL	r8, VCPU_GPR(R7)(r4)
> > +	PPC_STL	r5, SVCPU_R4(r3)
> > +	PPC_STL	r6, SVCPU_R5(r3)
> > +	PPC_STL	r7, SVCPU_R6(r3)
> > +	PPC_STL	r8, SVCPU_R7(r3)
> > +	PPC_LL	r5, VCPU_GPR(R8)(r4)
> > +	PPC_LL	r6, VCPU_GPR(R9)(r4)
> > +	PPC_LL	r7, VCPU_GPR(R10)(r4)
> > +	PPC_LL	r8, VCPU_GPR(R11)(r4)
> > +	PPC_STL	r5, SVCPU_R8(r3)
> > +	PPC_STL	r6, SVCPU_R9(r3)
> > +	PPC_STL	r7, SVCPU_R10(r3)
> > +	PPC_STL	r8, SVCPU_R11(r3)
> > +	PPC_LL	r5, VCPU_GPR(R12)(r4)
> > +	PPC_LL	r6, VCPU_GPR(R13)(r4)
> > +	lwz	r7, VCPU_CR(r4)
> > +	PPC_LL	r8, VCPU_XER(r4)
> > +	PPC_STL	r5, SVCPU_R12(r3)
> > +	PPC_STL	r6, SVCPU_R13(r3)
> > +	stw	r7, SVCPU_CR(r3)
> > +	stw	r8, SVCPU_XER(r3)
> > +	PPC_LL	r5, VCPU_CTR(r4)
> > +	PPC_LL	r6, VCPU_LR(r4)
> > +	PPC_LL	r7, VCPU_PC(r4)
> > +	PPC_STL	r5, SVCPU_CTR(r3)
> > +	PPC_STL	r6, SVCPU_LR(r3)
> > +	PPC_STL	r7, SVCPU_PC(r3)
> 
> Can we put this and the reverse copy into C functions and just call them from here and below? It'd definitely improve readability. We could even skip the whole thing on systems where we're not limited by an RMA.

Sure, will do.

> Why are we even using a shadow vcpu on book3s_32? We can always just access the real vcpu, no? Hrm.

At this stage the vcpu is still part of the vcpu_book3s struct, which
is vmalloc'd, so no we can't access it in real mode.  However, I have
a patch series which I'm about to post that will make the vcpu be
kmalloc'd, and then 32-bit could access it directly in real mode, as
could 64-bit bare-metal platforms if that made sense.

> I think it makes sense to get rid of 99% of the svcpu logic. Remove all bits in C code that ever refer to an svcpu. Only use the vcpu as reference for everything. Until you hit the real mode barrier on book3s_64. There explicitly copy the few things we need in real mode into the PACA and only refer to the PACA in rm code.
> 
> It'd be nice if we could speed up that code path on systems that don't need to jump through hoops to get to their vcpu data (G5s, PowerStation, etc), but I don't think it's worth the added complexity. We should rather try to make the code easy to understand :).

I agree completely. :)

Paul.