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@xxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; 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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
