> -----Original Message-----
> From: Bhushan Bharat-R65777
> Sent: Wednesday, July 02, 2014 5:07 PM
> To: Wood Scott-B07421; Alexander Graf
> Cc: kvm-ppc@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt injection to
> guest
>
>
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Tuesday, July 01, 2014 10:11 PM
> > To: Alexander Graf
> > Cc: Bhushan Bharat-R65777; kvm-ppc@xxxxxxxxxxxxxxx;
> > kvm@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH 2/2] KVM : powerpc/booke: Allow debug interrupt
> > injection to guest
> >
> > On Tue, 2014-07-01 at 18:22 +0200, Alexander Graf wrote:
> > > On 01.07.14 17:35, Scott Wood wrote:
> > > > On Tue, 2014-07-01 at 17:04 +0200, Alexander Graf wrote:
> > > >> On 01.07.14 16:58, Scott Wood wrote:
> > > >>> On Tue, 2014-07-01 at 08:23 +0200, Alexander Graf wrote:
> > > >>>> I don't think QEMU should be aware of these limitations.
> > > >>> OK, but we should at least have some idea of how the whole thing
> > > >>> is supposed to work, in order to determine if this is the
> > > >>> correct behavior for QEMU. I thought the model was that debug
> > > >>> resources are either owned by QEMU or by the guest, and in the
> > > >>> latter case, QEMU would never see the debug exception to begin with.
> > > >> That's bad for a number of reasons. For starters it's different
> > > >> from how
> > > >> x86 handles debug registers - and I hate to be different just for
> > > >> the sake of being different.
> > > > How does it work on x86?
> > >
> > > It overwrites more-or-less random breakpoints with its own ones, but
> > > leaves the others intact ;).
> >
> > Are you talking about software breakpoints or management of hardware
> > debug registers?
> >
> > > >> So if we do want to declare that debug registers are owned by
> > > >> either QEMU or the guest, we should change the semantics for all
> > > >> architectures.
> > > > If we want to say that ownership of the registers is shared, we
> > > > need a plan for how that would actually work.
> > >
> > > I think you're overengineering here :). When do people actually use
> > > gdbstub? Usually when they want to debug a broken guest. We can
> > > either
> > >
> > > * overengineer heavily and reduce the number of registers
> > > available to the guest to always have spares
> > > * overengineer a bit and turn off guest debugging completely when
> > > we use gdbstub
> > > * just leave as much alive as we can, hoping that it helps with
> > > the debugging
> > >
> > > Option 3 is what x86 does - and I think it's a reasonable approach.
> > > This is not an interface that needs to be 100% consistent and bullet
> > > proof, it's a best effort to enable you to debug as much as possible.
> >
> > I'm not insisting on 100% -- just hoping for some
> > explanation/discussion about how it's intended to work for the cases where it
> can.
> >
> > How will MSR[DE] and MSRP[DEP] be handled?
> >
> > How would I go about telling QEMU/KVM that I don't want this shared
> > mode, because I don't want guest to interfere with the debugging I'm
> > trying to do from QEMU?
> >
> > Will guest accesses to debug registers cause a userspace exit when
> > guest_debug is enabled?
> >
> > > >> I think we're in a path that is slow enough already to not worry
> > > >> about performance.
> > > > It's not just about performance, but simplicity of use, and
> > > > consistency of API.
> > > >
> > > > Oh, and it looks like there already exist one reg definitions and
> > > > implementations for most of the debug registers.
> > >
> > > For BookE? Where?
> >
> > arch/powerpc/kvm/booke.c: KVM_REG_PPC_IACn, KVM_REG_PPC_DACn
>
> I tried to quickly prototype what I think we want to do (this is not tested)
Hi Scott,
There is one problem which is stopping us to share debug resource between qemu and guest, looking for suggestions:
- As qemu is also using debug resource, We have to set MSR_DE and set MSRP_DEP (guest will not be able to clear MSR_DE). So qemu set debug events will always cause the debug interrupts.
- Now guest is also using debug resources and for some reason if guest wants to clear MSR_DE (disable debug interrupt) But it will not be able to disable as MSRP_DEP is set and KVM will not come to know guest willingness to disable MSR_DE.
- If the debug interrupts occurs then we will exit to QEMU and this may not a QEMU set event so it will inject interrupt to guest (using one-reg or set-sregs)
- Now KVM, when handling one-reg/sregs request to inject debug interrupt, do not know whether guest can handle the debug interrupt or not (as guest might have tried to set/clear MSR_DE).
Thanks
-Bharat
>
> -----------------------------------------------------------------------
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> index e8b3982..746b5c6 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -179,6 +179,9 @@ extern int kvmppc_xics_get_xive(struct kvm *kvm, u32 irq,
> u32 *server, extern int kvmppc_xics_int_on(struct kvm *kvm, u32 irq); extern
> int kvmppc_xics_int_off(struct kvm *kvm, u32 irq);
>
> +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu); void
> +kvmppc_core_queue_debug(struct kvm_vcpu *vcpu);
> +
> /*
> * Cuts out inst bits with ordering according to spec.
> * That means the leftmost bit is zero. All given bits are included.
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c index
> 9f13056..0b7e4e4 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -235,6 +235,16 @@ void kvmppc_core_dequeue_dec(struct kvm_vcpu *vcpu)
> clear_bit(BOOKE_IRQPRIO_DECREMENTER, &vcpu->arch.pending_exceptions); }
>
> +void kvmppc_core_queue_debug(struct kvm_vcpu *vcpu) {
> + kvmppc_booke_queue_irqprio(vcpu, BOOKE_IRQPRIO_DEBUG); }
> +
> +void kvmppc_core_dequeue_debug(struct kvm_vcpu *vcpu) {
> + clear_bit(BOOKE_IRQPRIO_DEBUG, &vcpu->arch.pending_exceptions); }
> +
> void kvmppc_core_queue_external(struct kvm_vcpu *vcpu,
> struct kvm_interrupt *irq) { @@ -841,6 +851,20
> @@ 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;
>
> + /* Userspace (QEMU) is not using debug resource, so inject debug interrupt
> + * directly to guest debug.
> + */
> + if (vcpu->guest_debug == 0) {
> + if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> + kvmppc_core_queue_debug(vcpu);
> +
> + /* Inject a program interrupt if trap debug is not allowed */
> + if ((dbsr & DBSR_TIE) && !(vcpu->arch.shared->msr & MSR_DE))
> + kvmppc_core_queue_program(vcpu, ESR_PTR);
> +
> + return RESUME_GUEST;
> + }
> +
> run->debug.arch.status = 0;
> run->debug.arch.address = vcpu->arch.pc;
>
> @@ -1920,7 +1944,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu
> *vcpu,
> vcpu->guest_debug = dbg->control;
> vcpu->arch.shadow_dbg_reg.dbcr0 = 0;
> /* Set DBCR0_EDM in guest visible DBCR0 register. */
> - vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
> +// vcpu->arch.dbg_reg.dbcr0 = DBCR0_EDM;
>
> if (vcpu->guest_debug & KVM_GUESTDBG_SINGLESTEP)
> vcpu->arch.shadow_dbg_reg.dbcr0 |= DBCR0_IDM | DBCR0_IC; diff --git
> a/arch/powerpc/kvm/booke_emulate.c b/arch/powerpc/kvm/booke_emulate.c
> index aaff1b7..ef8de7f 100644
> --- a/arch/powerpc/kvm/booke_emulate.c
> +++ b/arch/powerpc/kvm/booke_emulate.c
> @@ -26,6 +26,7 @@
> #define OP_19_XOP_RFMCI 38
> #define OP_19_XOP_RFI 50
> #define OP_19_XOP_RFCI 51
> +#define OP_19_XOP_RFDI 39
>
> #define OP_31_XOP_MFMSR 83
> #define OP_31_XOP_WRTEE 131
> @@ -38,10 +39,24 @@ static void kvmppc_emul_rfi(struct kvm_vcpu *vcpu)
> kvmppc_set_msr(vcpu, vcpu->arch.shared->srr1); }
>
> +static void kvmppc_emul_rfdi(struct kvm_vcpu *vcpu) {
> + vcpu->arch.pc = vcpu->arch.dsrr0;
> + /* Force MSR_DE when guest does not own debug facilities */
> + if (vcpu->guest_debug)
> + kvmppc_set_msr(vcpu, vcpu->arch.dsrr1 | MSR_DE);
> + else
> + kvmppc_set_msr(vcpu, vcpu->arch.dsrr1); }
> +
> static void kvmppc_emul_rfci(struct kvm_vcpu *vcpu) {
> vcpu->arch.pc = vcpu->arch.csrr0;
> - kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
> + /* Force MSR_DE when guest does not own debug facilities */
> + if (vcpu->guest_debug)
> + kvmppc_set_msr(vcpu, vcpu->arch.csrr1 | MSR_DE);
> + else
> + kvmppc_set_msr(vcpu, vcpu->arch.csrr1);
> }
>
> static void kvmppc_emul_rfmci(struct kvm_vcpu *vcpu) @@ -78,6 +93,12 @@ int
> kvmppc_booke_emulate_op(struct kvm_run *run, struct kvm_vcpu *vcpu,
> *advance = 0;
> break;
>
> + case OP_19_XOP_RFDI:
> + kvmppc_emul_rfdi(vcpu);
> +// kvmppc_set_exit_type(vcpu, EMULATED_RFDI_EXITS);
> + *advance = 0;
> + break;
> +
> default:
> emulated = EMULATE_FAIL;
> break;
> @@ -131,6 +152,7 @@ int kvmppc_booke_emulate_op(struct kvm_run *run, struct
> kvm_vcpu *vcpu, int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int sprn,
> ulong spr_val) {
> int emulated = EMULATE_DONE;
> + bool debug_inst = false;
>
> switch (sprn) {
> case SPRN_DEAR:
> @@ -145,21 +167,74 @@ int kvmppc_booke_emulate_mtspr(struct kvm_vcpu *vcpu, int
> sprn, ulong spr_val)
> case SPRN_CSRR1:
> vcpu->arch.csrr1 = spr_val;
> break;
> - case SPRN_DBCR0:
> - vcpu->arch.dbg_reg.dbcr0 = spr_val;
> + case SPRN_DSRR0:
> + vcpu->arch.dsrr0 = spr_val;
> break;
> - case SPRN_DBCR1:
> - vcpu->arch.dbg_reg.dbcr1 = spr_val;
> + case SPRN_DSRR1:
> + vcpu->arch.dsrr1 = spr_val;
> + break;
> + case SPRN_IAC1:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
> + break;
> + case SPRN_IAC2:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac2 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
> + break;
> +#ifndef CONFIG_PPC_FSL_BOOK3E
> + case SPRN_IAC3:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac3 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
> break;
> + case SPRN_IAC4:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac4 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
> + break;
> +#endif
> + case SPRN_DAC1:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dac1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
> + break;
> + case SPRN_DAC2:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dac2 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
> + break;
> + case SPRN_DBCR0:
> + debug_inst = true;
> + spr_val &= (DBCR0_IDM | DBCR0_IC | DBCR0_BT | DBCR0_TIE |
> + DBCR0_IAC1 | DBCR0_IAC2 | DBCR0_IAC3 | DBCR0_IAC4 |
> + DBCR0_DAC1R | DBCR0_DAC1W | DBCR0_DAC2R | DBCR0_DAC2W);
> +
> + vcpu->arch.dbg_reg.dbcr0 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dbcr0 = spr_val;
> + break;
> + case SPRN_DBCR1:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dbcr1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
> + break;
> + case SPRN_DBCR2:
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dbcr2 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dbcr2 = spr_val;
> + break;
> + case SPRN_DBSR:
> + vcpu->arch.dbsr &= ~spr_val;
> + if (vcpu->arch.dbsr == 0)
> + kvmppc_core_dequeue_debug(vcpu);
> + break;
> case SPRN_MCSRR0:
> vcpu->arch.mcsrr0 = spr_val;
> break;
> case SPRN_MCSRR1:
> vcpu->arch.mcsrr1 = spr_val;
> break;
> - case SPRN_DBSR:
> - vcpu->arch.dbsr &= ~spr_val;
> - break;
> case SPRN_TSR:
> kvmppc_clr_tsr_bits(vcpu, spr_val);
> break;
> @@ -271,6 +346,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;
> + }
> return emulated;
> }
>
> @@ -297,12 +376,41 @@ int kvmppc_booke_emulate_mfspr(struct kvm_vcpu *vcpu, int
> sprn, ulong *spr_val)
> case SPRN_CSRR1:
> *spr_val = vcpu->arch.csrr1;
> break;
> + case SPRN_DSRR0:
> + *spr_val = vcpu->arch.dsrr0;
> + break;
> + case SPRN_DSRR1:
> + *spr_val = vcpu->arch.dsrr1;
> + break;
> + case SPRN_IAC1:
> + *spr_val = vcpu->arch.dbg_reg.iac1;
> + break;
> + case SPRN_IAC2:
> + *spr_val = vcpu->arch.dbg_reg.iac2;
> + break;
> +#ifndef CONFIG_PPC_FSL_BOOK3E
> + case SPRN_IAC3:
> + *spr_val = vcpu->arch.dbg_reg.iac3;
> + break;
> + case SPRN_IAC4:
> + *spr_val = vcpu->arch.dbg_reg.iac4;
> + break;
> +#endif
> + case SPRN_DAC1:
> + *spr_val = vcpu->arch.dbg_reg.dac1;
> + break;
> + case SPRN_DAC2:
> + *spr_val = vcpu->arch.dbg_reg.dac2;
> + break;
> case SPRN_DBCR0:
> *spr_val = vcpu->arch.dbg_reg.dbcr0;
> break;
> case SPRN_DBCR1:
> *spr_val = vcpu->arch.dbg_reg.dbcr1;
> break;
> + case SPRN_DBCR2:
> + *spr_val = vcpu->arch.dbg_reg.dbcr2;
> + break;
> case SPRN_MCSRR0:
> *spr_val = vcpu->arch.mcsrr0;
> break;
> diff --git a/arch/powerpc/kvm/e500mc.c b/arch/powerpc/kvm/e500mc.c index
> bbf7cdd..560b576 100644
> --- a/arch/powerpc/kvm/e500mc.c
> +++ b/arch/powerpc/kvm/e500mc.c
> @@ -249,7 +249,7 @@ int kvmppc_core_vcpu_setup(struct kvm_vcpu *vcpu) #ifdef
> CONFIG_64BIT
> vcpu->arch.shadow_epcr |= SPRN_EPCR_ICM; #endif
> - vcpu->arch.shadow_msrp = MSRP_UCLEP | MSRP_DEP | MSRP_PMMP;
> + vcpu->arch.shadow_msrp = MSRP_UCLEP | MSRP_PMMP;
>
> vcpu->arch.pvr = mfspr(SPRN_PVR);
> vcpu_e500->svr = mfspr(SPRN_SVR);
>
>
> ------------------
>
> Thanks
> -Bharat
>
> >
> > -Scott
> >
��.n��������+%������w��{.n�����o��^n�r������&��z�ޗ�zf���h���~����������_��+v���)ߣ�
