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

[Scott Wood <scottwood@freescale.com>]

On Wed, 2014-07-30 at 01:43 -0500, Bhushan Bharat-R65777 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 29, 2014 3:58 AM
> > To: Bhushan Bharat-R65777
> > Cc: agraf@suse.de; kvm-ppc@vger.kernel.org; kvm@vger.kernel.org; Yoder Stuart-
> > B08248
> > Subject: Re: [PATCH 6/6] KVM: PPC: BOOKE: Emulate debug registers and exception
> > 
> >  Userspace might be interested in
> > the raw value,
> 
> With the current design, If userspace is interested then it will not
> get the DBSR.

Oh, because DBSR isn't currently implemented in sregs or one reg?

>  But why userspace will be interested?

Do you expose all of the hardware's debugging features in your
high-level interface?

> > plus it's a change from the current API semantics.
> 
> Can you please let us know how ?

It looked like it was removing dbsr visibility and the requirement for
userspace to clear dbsr.  I guess the old way was that the value in
vcpu->arch.dbsr didn't matter until the next debug exception, when it
would be overwritten by the new SPRN_DBSR?

> > > +	case SPRN_DBCR2:
> > > +		/*
> > > +		 * If userspace is debugging guest then guest
> > > +		 * can not access debug registers.
> > > +		 */
> > > +		if (vcpu->guest_debug)
> > > +			break;
> > > +
> > > +		debug_inst = true;
> > > +		vcpu->arch.dbg_reg.dbcr2 = spr_val;
> > > +		vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> > >  		break;
> > 
> > In what circumstances can the architected and shadow registers differ?
> 
> As of now they are same. But I think that if we want to implement other features like "Freeze Timer (FT)" then they can be different.

I don't think we can possibly implement Freeze Timer.
 
> > >  	case SPRN_DBSR:
> > > +		/*
> > > +		 * If userspace is debugging guest then guest
> > > +		 * can not access debug registers.
> > > +		 */
> > > +		if (vcpu->guest_debug)
> > > +			break;
> > > +
> > >  		vcpu->arch.dbsr &= ~spr_val;
> > > +		if (vcpu->arch.dbsr == 0)
> > > +			kvmppc_core_dequeue_debug(vcpu);
> > >  		break;
> > 
> > Not all DBSR bits cause an exception, e.g. IDE and MRR.
> 
> I am not sure what we should in that case ?
>
> As we are currently emulating a subset of debug events (IAC, DAC, IC,
> BT and TIE --- DBCR0 emulation) then we should expose status of those
> events in guest dbsr and rest should be cleared ?

I'm not saying they need to be exposed to the guest, but I don't see
where you filter out bits like these.

> > > @@ -273,6 +397,10 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int
> > sprn, ulong spr_val)
> > >  		emulated = EMULATE_FAIL;
> > >  	}
> > >
> > > +	if (debug_inst) {
> > > +		switch_booke_debug_regs(&vcpu->arch.shadow_dbg_reg);
> > > +		current->thread.debug = vcpu->arch.shadow_dbg_reg;
> > > +	}
> > 
> > Could you explain what's going on with regard to copying the registers
> > into current->thread.debug?  Why is it done after loading the registers
> > into the hardware (is there a race if we get preempted in the middle)?
> 
> Yes, and this was something I was not clear when writing this code.
> Should we have preempt disable-enable around this.

Can they be reordered instead?

-Scott