> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Tuesday, August 05, 2014 4:23 AM
> To: Bhushan Bharat-R65777
> Cc: ag...@suse.de; kvm-...@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;
>
> > @@ -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 ?
> 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.
Thanks
-Bharat
>
> -Scott
>
