Re: [PATCH v7 08/12] ARM: KVM: World-switch implementation

From: Rusty Russell <rusty@rustcorp.com.au>
To: Avi Kivity <avi@redhat.com>, Rusty Russell <rusty.russell@linaro.org>
Cc: Christoffer Dall <c.dall@virtualopensystems.com>,
	android-virt@lists.cs.columbia.edu, kvm@vger.kernel.org,
	Peter Maydell <peter.maydell@linaro.org>,
	tech@virtualopensystems.com
Subject: Re: [PATCH v7 08/12] ARM: KVM: World-switch implementation
Date: Thu, 29 Mar 2012 08:27:21 +1030	[thread overview]
Message-ID: <87pqbwz8vy.fsf@rustcorp.com.au> (raw)
In-Reply-To: <4F730CB3.6010402@redhat.com>

On Wed, 28 Mar 2012 15:05:55 +0200, Avi Kivity <avi@redhat.com> wrote:
> On 03/23/2012 02:23 AM, Rusty Russell wrote:
> > On Mon, 12 Mar 2012 02:52:41 -0400, Christoffer Dall <c.dall@virtualopensystems.com> wrote:
> > > @@ -236,6 +237,24 @@ int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
> > >  
> > >  int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> > >  {
> > > +	unsigned long cpsr;
> > > +	unsigned long sctlr;
> > > +
> > > +
> > > +	/* Init execution CPSR */
> > > +	asm volatile ("mrs	%[cpsr], cpsr" :
> > > +			[cpsr] "=r" (cpsr));
> > > +	vcpu->arch.regs.cpsr = SVC_MODE | PSR_I_BIT | PSR_F_BIT | PSR_A_BIT |
> > > +				(cpsr & PSR_E_BIT);
> > > +
> > > +	/* Init SCTLR with MMU disabled */
> > > +	asm volatile ("mrc	p15, 0, %[sctlr], c1, c0, 0" :
> > > +			[sctlr] "=r" (sctlr));
> > > +	vcpu->arch.cp15[c1_SCTLR] = sctlr & ~1U;
> > > +
> > > +	/* Compute guest MPIDR */
> > > +	vcpu->arch.cp15[c0_MPIDR] = (read_cpuid_mpidr() & ~0xff)
> > > +				    | vcpu->vcpu_id;
> > >  	return 0;
> > >  }
> >
> > Hi Christoffer!
> >
> >         I started cleaning this up, on the theory that we should use the
> > defined reset values rather than basing them on host values.  Then I
> > discovered that this is ignored by qemu anyway, which overrides the
> > values (except for MPIDR: I set that to 0x80000000 | vcpu->vcpu_id,
> > which seems correct to me).
> >
> > Should this code be dropped altogether?  Peter points out that once we
> > add new regs, they *must* be initialized to correct reset values in case
> > older QEMU knows nothing about them, but for the rest it just seems
> > confusing...
> >
> 
> It's best to initialize everything.  As far as this code is concerned
> qemu is just another user and we shouldn't rely on its implementation.

In which case, qemu should not initialize anything, but read the
registers from the kernel to start with.

Cheers,
Rusty.
-- 
  How could I marry someone with more hair than me?  http://baldalex.org