> -----Original Message-----
> From: Wood Scott-B07421
> Sent: Thursday, July 31, 2014 8:18 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
>
> 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?
That is one reason. Another is that if we give dbsr visibility to userspace then userspace have to clear dbsr in handling KVM_EXIT_DEBUG. And we think there is no gain in doing that because
"
- QEMU cannot inject debug interrupt to guest (as this does not know guest ability to handle debug interrupt; MSR_DE), so will always clear DBSR.
- If QEMU has to always clear DBSR in handling KVM_EXIT_DEBUG then this (clearing dbsr in kernel) avoid doing in SET_SREGS/set_one_reg()
" This makes dbsr not visible to userspace.
Also this (clearing of dbsr) should not be part of this patch, this should be a separate patch. I will do that in next version.
>
> > But why userspace will be interested?
>
> Do you expose all of the hardware's debugging features in your high-level
> interface?
We support h/w breakpoint, watchpoint and IC (single stepping) and status in userspace exit provide all required information to userspace.
>
> > > 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?
But that means old dbsr will be visibility to userspace, which is even bad than not visible, no?
Also this can lead to old dbsr visible to guest once userspace releases debug resources, but this can be solved by clearing dbsr in kvm_arch_vcpu_ioctl_set_guest_debug() -> " if (!(dbg->control & KVM_GUESTDBG_ENABLE)) { }".
>
> > > > + 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.
May be, but in my opinion we should keep this open.
>
> > > > 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.
I am trying to get what all bits should be filtered out, all bits except IACx, DACx, IC, BT and TIE (same as event set filtering done when setting DBCR0) ?
i.e IDE, UDE, MRR, IRPT, RET, CIRPT, CRET should be filtered out?
>
> > > > @@ -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?
Thanks; Yes, that will work :)
Regards
-Bharat
>
> -Scott
>
��.n��������+%������w��{.n�����o�^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
