Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and exception

[Scott Wood <scottwood@freescale.com>]

On Mon, 2014-08-04 at 22:41 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, August 05, 2014 4:23 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 5/5 v2] KVM: PPC: BOOKE: Emulate debug registers and
> > exception
> > 
> > On Mon, 2014-08-04 at 13:32 +0530, Bharat Bhushan wrote:
> > > @@ -735,7 +745,27 @@ static int kvmppc_handle_debug(struct kvm_run *run,
> > struct kvm_vcpu *vcpu)
> > >  	struct debug_reg *dbg_reg = &(vcpu->arch.shadow_dbg_reg);
> > >  	u32 dbsr = vcpu->arch.dbsr;
> > >
> > > -	/* Clear guest dbsr (vcpu->arch.dbsr).
> > > +	if (vcpu->guest_debug == 0) {
> > > +		/*
> > > +		 * Debug resources belong to Guest.
> > > +		 * Imprecise debug event are not injected
> > > +		 */
> > > +		if (dbsr & DBSR_IDE)
> > > +			return RESUME_GUEST;
> > 
> > This is incorrect.  DBSR_IDE shouldn't *cause* an injection, but it shouldn't
> > inhibit it either.
> 
> Will this work ?
> 		If ((dbsr & DBSR_IDE) && !(dbsr & ~DBSR_IDE))
> 			Return RESUME_GUEST; 

I suppose it could, but it would be cleaner to just change "dbsr" to
"(dbsr & ~DBSR_IDE)" in the next if-statement (maybe factoring out each
&& term of that if-statement to variables to make it more readable).

> > > @@ -828,6 +858,8 @@ static void kvmppc_restart_interrupt(struct kvm_vcpu
> > *vcpu,
> > >  	case BOOKE_INTERRUPT_DEBUG:
> > >  		/* Save DBSR before preemption is enabled */
> > >  		vcpu->arch.dbsr = mfspr(SPRN_DBSR);
> > > +		/* MASK out DBSR_MRR */
> > > +		vcpu->arch.dbsr &= ~DBSR_MRR;
> > >  		kvmppc_clear_dbsr();
> > >  		break;
> > >  	}
> > 
> > DBSR[MRR] can only be set once per host system reset.  There's no need to filter
> > it out here; just make sure the host clears it at some point before this point.
> 
> Can you please suggest where ? somewhere in KVM initialization ?

Sure, KVM init works given that there's no real reason for non-KVM code
to care.

> > The MRR value doesn't currently survive past kvmppc_clear_dbsr(), so this isn't
> > helping to preserve it for the host's benefit...
> > 
> > > @@ -1858,6 +1890,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct
> > > kvm_vcpu *vcpu,
> > >
> > >  	if (!(dbg->control & KVM_GUESTDBG_ENABLE)) {
> > >  		vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> > > +		vcpu->arch.dbg_reg.dbcr0 = 0;
> > 
> > Again, it's not clear why we need shadow debug registers here.  "Just in case we
> > implement something that can't be implemented" isn't a good reason to keep
> > complexity around.
> 
> One reason was that setting EDM in guest visible register, For this we
> need shadow_reg is used to save/restore state in h/w register (which
> does not have DBCR0_EDM) but debug_reg have DBCR0_EDM.

If that's the only reason, then I'd get rid of the shadow and just OR in
DCBR0_EDM when reading the register, if vcpu->guest_debug is nonzero.

-Scott