On Fri, 2014-07-11 at 14:09 +0530, Bharat Bhushan wrote:
> This patch emulates debug registers and debug exception
> to support guest using debug resource. This enables running
> gdb/kgdb etc in guest.
>
> On BOOKE architecture we cannot share debug resources between QEMU and
> guest because:
> When QEMU is using debug resources then debug exception must
> be always enabled. To achieve this we set MSR_DE and also set
> MSRP_DEP so guest cannot change MSR_DE.
>
> When emulating debug resource for guest we want guest
> to control MSR_DE (enable/disable debug interrupt on need).
>
> So above mentioned two configuration cannot be supported
> at the same time. So the result is that we cannot share
> debug resources between QEMU and Guest on BOOKE architecture.
>
> In the current design QEMU gets priority over guest, this means that if
> QEMU is using debug resources then guest cannot use them and if guest is
> using debug resource then QEMU can overwrite them.
>
> Signed-off-by: Bharat Bhushan <bharat.bhus...@freescale.com>
> ---
> Hi Alex,
>
> I thought of having some print in register emulation if QEMU
> is using debug resource, Also when QEMU overwrites guest written
> values but that looks excessive. If I uses some variable which
> get set when guest starts using debug registers and check in
> debug set ioctl then that look ugly. Looking for suggestions
>
> arch/powerpc/include/asm/kvm_ppc.h | 3 +
> arch/powerpc/kvm/booke.c | 27 +++++++
> arch/powerpc/kvm/booke_emulate.c | 157
> +++++++++++++++++++++++++++++++++++++
> 3 files changed, 187 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/kvm_ppc.h
> b/arch/powerpc/include/asm/kvm_ppc.h
> index e2fd5a1..f3f7611 100644
> --- a/arch/powerpc/include/asm/kvm_ppc.h
> +++ b/arch/powerpc/include/asm/kvm_ppc.h
> @@ -173,6 +173,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);
> +
> union kvmppc_one_reg {
> u32 wval;
> u64 dval;
> diff --git a/arch/powerpc/kvm/booke.c b/arch/powerpc/kvm/booke.c
> index fadfe76..c2471ed 100644
> --- a/arch/powerpc/kvm/booke.c
> +++ b/arch/powerpc/kvm/booke.c
> @@ -264,6 +264,16 @@ static void kvmppc_core_dequeue_watchdog(struct kvm_vcpu
> *vcpu)
> clear_bit(BOOKE_IRQPRIO_WATCHDOG, &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);
> +}
> +
> static void set_guest_srr(struct kvm_vcpu *vcpu, unsigned long srr0, u32
> srr1)
> {
> #ifdef CONFIG_KVM_BOOKE_HV
> @@ -783,6 +793,23 @@ 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;
>
> + if (vcpu->guest_debug == 0) {
> + /* Debug resources belong to Guest */
> + if (dbsr && (vcpu->arch.shared->msr & MSR_DE))
> + kvmppc_core_queue_debug(vcpu);
Should also check for DBCR0[IDM].
> +
> + /* 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;
> + }
> +
> + /*
> + * Prepare for userspace exit as debug resources
> + * are owned by userspace.
> + */
> + vcpu->arch.dbsr = 0;
> run->debug.arch.status = 0;
> run->debug.arch.address = vcpu->arch.pc;
Why are you clearing vcpu->arch.dbsr? Userspace might be interested in
the raw value, plus it's a change from the current API semantics.
Where is this run->debug.arch.<foo> stuff and KVM_EXIT_DEBUG documented?
> diff --git a/arch/powerpc/kvm/booke_emulate.c
> b/arch/powerpc/kvm/booke_emulate.c
> index 2a20194..3d143fe 100644
> --- a/arch/powerpc/kvm/booke_emulate.c
> +++ b/arch/powerpc/kvm/booke_emulate.c
> @@ -139,6 +139,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:
> @@ -153,14 +154,137 @@ 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_DSRR0:
> + vcpu->arch.dsrr0 = spr_val;
> + break;
> + case SPRN_DSRR1:
> + vcpu->arch.dsrr1 = spr_val;
> + break;
> + case SPRN_IAC1:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac1 = spr_val;
> + break;
> + case SPRN_IAC2:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac2 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac2 = spr_val;
> + break;
> +#if CONFIG_PPC_ADV_DEBUG_IACS > 2
> + case SPRN_IAC3:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac3 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac3 = spr_val;
> + break;
> + case SPRN_IAC4:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.iac4 = spr_val;
> + vcpu->arch.shadow_dbg_reg.iac4 = spr_val;
> + break;
> +#endif
> + case SPRN_DAC1:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dac1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dac1 = spr_val;
> + break;
> + case SPRN_DAC2:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> + vcpu->arch.dbg_reg.dac2 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dac2 = spr_val;
> + break;
> case SPRN_DBCR0:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + 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:
> + /*
> + * If userspace is debugging guest then guest
> + * can not access debug registers.
> + */
> + if (vcpu->guest_debug)
> + break;
> +
> + debug_inst = true;
> vcpu->arch.dbg_reg.dbcr1 = spr_val;
> + vcpu->arch.shadow_dbg_reg.dbcr1 = spr_val;
> + break;
> + 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?
> 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.
> case SPRN_TSR:
> kvmppc_clr_tsr_bits(vcpu, spr_val);
> @@ -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)?
-Scott
;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
