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@xxxxxxx; kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; 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
--
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
