[v2,06/10] KVM: arm64: guest debug, add SW break point support

Commit Message

Alex Bennée March 31, 2015, 3:08 p.m. UTC

This adds support for SW breakpoints inserted by userspace.

We do this by trapping all BKPT exceptions in the
hypervisor (MDCR_EL2_TDE). The kvm_debug_exit_arch carries the address
of the exception. If user-space doesn't know of the breakpoint then we
have a guest inserted breakpoint and the hypervisor needs to start again
and deliver the exception to guest.

Signed-off-by: Alex Bennée <[e-mail]>

---
v2
  - update to use new exit struct
  - tweak for C setup
  - do our setup in debug_setup/clear code
  - fixed up comments

Comments

David Hildenbrand April 2, 2015, 12:52 p.m. UTC | #1

> This adds support for SW breakpoints inserted by userspace.
> 
> We do this by trapping all BKPT exceptions in the
> hypervisor (MDCR_EL2_TDE). The kvm_debug_exit_arch carries the address
> of the exception. If user-space doesn't know of the breakpoint then we
> have a guest inserted breakpoint and the hypervisor needs to start again
> and deliver the exception to guest.
> 
> Signed-off-by: Alex Bennée <[e-mail]>
> 
> ---
> v2
>   - update to use new exit struct
>   - tweak for C setup
>   - do our setup in debug_setup/clear code
>   - fixed up comments
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 06c5064..17d4f9c 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2626,7 +2626,7 @@ when running. Common control bits are:
>  The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
> 
> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7ea8b0e..d3bc8dc 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -304,7 +304,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
> 
> -#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE)
> +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)
> 
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 8a29d0b..cff0475 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -45,11 +45,18 @@ void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR);
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
> 
> +	/* Trap debug register access? */

This should probably go to the earlier patch.

>  	if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  	else
>  		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
> 
> +	/* Trap breakpoints? */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> +	else
> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;

Again, a candidate for clear_debug?

> +
>  }
> 
>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 524fa25..ed1bbb4 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
> 
> +/**
> + * kvm_handle_debug_exception - handle a debug exception instruction

"kvm_handle_guest_debug"

> + *
> + * @vcpu:	the vcpu pointer
> + * @run:	access to the kvm_run structure for results
> + *
> + * We route all debug exceptions through the same handler as we
> + * just need to report the PC and the HSR values to userspace.
> + * Userspace may decide to re-inject the exception and deliver it to
> + * the guest if it wasn't for the host to deal with.
> + */
> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.hsr = hsr;
> +
> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
> +	case ESR_ELx_EC_BKPT32:
> +	case ESR_ELx_EC_BRK64:
> +		run->debug.arch.pc = *vcpu_pc(vcpu);
> +		break;
> +	default:
> +		kvm_err("%s: un-handled case hsr: %#08x\n",
> +			__func__, (unsigned int) hsr);

Don't you want to fail hard in this case? This might result in many messages.
returning 0 feels wrong.

> +		break;
> +	}
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
>  	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -96,6 +127,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
> +	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
> +	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>  };
> 
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)


David

Alex Bennée April 2, 2015, 2:06 p.m. UTC | #2

David Hildenbrand <[e-mail]> writes:

>> This adds support for SW breakpoints inserted by userspace.
>> 
>> We do this by trapping all BKPT exceptions in the
>> hypervisor (MDCR_EL2_TDE). The kvm_debug_exit_arch carries the address
>> of the exception. If user-space doesn't know of the breakpoint then we
>> have a guest inserted breakpoint and the hypervisor needs to start again
>> and deliver the exception to guest.
>> 
>> Signed-off-by: Alex Bennée <[e-mail]>
>> 
>> ---
>> v2
>>   - update to use new exit struct
>>   - tweak for C setup
>>   - do our setup in debug_setup/clear code
>>   - fixed up comments
>> 
>> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
>> index 06c5064..17d4f9c 100644
>> --- a/Documentation/virtual/kvm/api.txt
>> +++ b/Documentation/virtual/kvm/api.txt
>> @@ -2626,7 +2626,7 @@ when running. Common control bits are:
>>  The top 16 bits of the control field are architecture specific control
>>  flags which can include the following:
>> 
>> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
>> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>> index 7ea8b0e..d3bc8dc 100644
>> --- a/arch/arm/kvm/arm.c
>> +++ b/arch/arm/kvm/arm.c
>> @@ -304,7 +304,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>  	kvm_arm_set_running_vcpu(NULL);
>>  }
>> 
>> -#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE)
>> +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)
>> 
>>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>  					struct kvm_guest_debug *dbg)
>> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
>> index 8a29d0b..cff0475 100644
>> --- a/arch/arm64/kvm/debug.c
>> +++ b/arch/arm64/kvm/debug.c
>> @@ -45,11 +45,18 @@ void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
>>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR);
>>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
>> 
>> +	/* Trap debug register access? */
>
> This should probably go to the earlier patch.

Agreed.

>
>>  	if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
>>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>>  	else
>>  		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
>> 
>> +	/* Trap breakpoints? */
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
>> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>> +	else
>> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
>
> Again, a candidate for clear_debug?

I don't follow. Changes to mdcr_el2 will only get applied as we jump in
through the hyp.S code. We need to ensure the guest can use SW BKPTs if
we are not.

>
>> +
>>  }
>> 
>>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 524fa25..ed1bbb4 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	return 1;
>>  }
>> 
>> +/**
>> + * kvm_handle_debug_exception - handle a debug exception instruction
>
> "kvm_handle_guest_debug"

Sure.

>
>> + *
>> + * @vcpu:	the vcpu pointer
>> + * @run:	access to the kvm_run structure for results
>> + *
>> + * We route all debug exceptions through the same handler as we
>> + * just need to report the PC and the HSR values to userspace.
>> + * Userspace may decide to re-inject the exception and deliver it to
>> + * the guest if it wasn't for the host to deal with.
>> + */
>> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
>> +
>> +	run->exit_reason = KVM_EXIT_DEBUG;
>> +	run->debug.arch.hsr = hsr;
>> +
>> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
>> +	case ESR_ELx_EC_BKPT32:
>> +	case ESR_ELx_EC_BRK64:
>> +		run->debug.arch.pc = *vcpu_pc(vcpu);
>> +		break;
>> +	default:
>> +		kvm_err("%s: un-handled case hsr: %#08x\n",
>> +			__func__, (unsigned int) hsr);
>
> Don't you want to fail hard in this case? This might result in many messages.
> returning 0 feels wrong.

You mean a BUG_ON()? Although it would be a cock up on the hosts part to
have an un-handled exception enabled allowing the guest to trigger a
host panic seems excessive.

>
>> +		break;
>> +	}
>> +	return 0;
>> +}
>> +
>>  static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
>>  	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
>> @@ -96,6 +127,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
>> +	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
>> +	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>>  };
>> 
>>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
>
>
> David

Andrew Jones April 10, 2015, 1:09 p.m. UTC | #3

On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
> This adds support for SW breakpoints inserted by userspace.
> 
> We do this by trapping all BKPT exceptions in the
> hypervisor (MDCR_EL2_TDE). The kvm_debug_exit_arch carries the address
> of the exception. If user-space doesn't know of the breakpoint then we
> have a guest inserted breakpoint and the hypervisor needs to start again
> and deliver the exception to guest.
> 
> Signed-off-by: Alex Bennée <[e-mail]>
> 
> ---
> v2
>   - update to use new exit struct
>   - tweak for C setup
>   - do our setup in debug_setup/clear code
>   - fixed up comments
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 06c5064..17d4f9c 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2626,7 +2626,7 @@ when running. Common control bits are:
>  The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7ea8b0e..d3bc8dc 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -304,7 +304,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
>  
> -#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE)
> +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)
>  
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 8a29d0b..cff0475 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -45,11 +45,18 @@ void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR);
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
>  
> +	/* Trap debug register access? */
>  	if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  	else
>  		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
>  
> +	/* Trap breakpoints? */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> +	else
> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
> +
>  }
>  
>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 524fa25..ed1bbb4 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +/**
> + * kvm_handle_debug_exception - handle a debug exception instruction
> + *
> + * @vcpu:	the vcpu pointer
> + * @run:	access to the kvm_run structure for results
> + *
> + * We route all debug exceptions through the same handler as we
> + * just need to report the PC and the HSR values to userspace.
> + * Userspace may decide to re-inject the exception and deliver it to
> + * the guest if it wasn't for the host to deal with.
> + */
> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.hsr = hsr;
> +
> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
> +	case ESR_ELx_EC_BKPT32:
> +	case ESR_ELx_EC_BRK64:
> +		run->debug.arch.pc = *vcpu_pc(vcpu);
> +		break;
> +	default:
> +		kvm_err("%s: un-handled case hsr: %#08x\n",
> +			__func__, (unsigned int) hsr);
> +		break;
> +	}
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
>  	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -96,6 +127,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
> +	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
> +	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>  };
>  
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> -- 
> 2.3.4
>

I agree with David's name change suggestion, "kvm_handle_guest_debug",
otherwise

Reviewed-by: Andrew Jones <[e-mail]>

Christoffer Dall April 14, 2015, 8:25 a.m. UTC | #4

On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
> This adds support for SW breakpoints inserted by userspace.
> 
> We do this by trapping all BKPT exceptions in the
> hypervisor (MDCR_EL2_TDE).

you mean trapping all exceptions in the guest to the hypervisor?

> The kvm_debug_exit_arch carries the address
> of the exception.

why?  can userspace not simply read out the PC using GET_ONE_REG?

> If user-space doesn't know of the breakpoint then we
> have a guest inserted breakpoint and the hypervisor needs to start again
> and deliver the exception to guest.
> 
> Signed-off-by: Alex Bennée <[e-mail]>
> 
> ---
> v2
>   - update to use new exit struct
>   - tweak for C setup
>   - do our setup in debug_setup/clear code
>   - fixed up comments
> 
> diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
> index 06c5064..17d4f9c 100644
> --- a/Documentation/virtual/kvm/api.txt
> +++ b/Documentation/virtual/kvm/api.txt
> @@ -2626,7 +2626,7 @@ when running. Common control bits are:
>  The top 16 bits of the control field are architecture specific control
>  flags which can include the following:
>  
> -  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
> +  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
>    - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
>    - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
>    - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index 7ea8b0e..d3bc8dc 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -304,7 +304,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	kvm_arm_set_running_vcpu(NULL);
>  }
>  
> -#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE)
> +#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)

nit: spaces around the operator

>  
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>  					struct kvm_guest_debug *dbg)
> diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
> index 8a29d0b..cff0475 100644
> --- a/arch/arm64/kvm/debug.c
> +++ b/arch/arm64/kvm/debug.c
> @@ -45,11 +45,18 @@ void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR);
>  	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
>  
> +	/* Trap debug register access? */

other patch

>  	if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
>  		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
>  	else
>  		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
>  
> +	/* Trap breakpoints? */
> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> +	else
> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;

so now you're trapping all debug exceptions, right?

what happens if the guest is using the hardware to debug debug stuff and
generates other kinds of debug exceptions, like a hardware breakpoint,
will we not see an unhandled exception and the guest being forcefully
killed?

> +
>  }
>  
>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 524fa25..ed1bbb4 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	return 1;
>  }
>  
> +/**
> + * kvm_handle_debug_exception - handle a debug exception instruction

handle a software breadkpoint exception

> + *
> + * @vcpu:	the vcpu pointer
> + * @run:	access to the kvm_run structure for results
> + *
> + * We route all debug exceptions through the same handler as we

all debug exceptions?  software breakpoints and all?  then why the above
shot text?

> + * just need to report the PC and the HSR values to userspace.
> + * Userspace may decide to re-inject the exception and deliver it to
> + * the guest if it wasn't for the host to deal with.

now I'm confused - does userspace setup the guest to receive an
exception or does it tell KVM to emulate an exception for the guest or
do we execute the breakpoint without trapping the debug exception?

> + */
> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
> +{
> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> +
> +	run->exit_reason = KVM_EXIT_DEBUG;
> +	run->debug.arch.hsr = hsr;
> +
> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
> +	case ESR_ELx_EC_BKPT32:
> +	case ESR_ELx_EC_BRK64:
> +		run->debug.arch.pc = *vcpu_pc(vcpu);
> +		break;
> +	default:
> +		kvm_err("%s: un-handled case hsr: %#08x\n",
> +			__func__, (unsigned int) hsr);

this should never happen right?

> +		break;
> +	}
> +	return 0;
> +}
> +
>  static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
>  	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
> @@ -96,6 +127,8 @@ static exit_handle_fn arm_exit_handlers[] = {
>  	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
>  	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
>  	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
> +	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
> +	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
>  };
>  
>  static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)
> -- 
> 2.3.4
>

Alex Bennée April 22, 2015, 9:46 a.m. UTC | #5

Zhichao Huang <[e-mail]> writes:

> On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
>> This adds support for SW breakpoints inserted by userspace.
>>
>> We do this by trapping all BKPT exceptions in the
>> hypervisor (MDCR_EL2_TDE).
>
> why should we trap all debug exceptions?
>
> The trap for cp14 register r/w seems enough to record relevant
> informations to context switch the dbg register while neccessary.

Lets think about this case when the SW breakpoint exception occurs:

If KVM doesn't trap it and pass it back to userspace to handle it would
have to deliver it to the guest. The guest not having inserted the
breakpoint in the first place would get very confused.

So what we actually do is re-route the exception to the hypervisor and
stop the VM and return to userspace with the debug information. Once in
QEMU we check to see if the SW breakpoint was one of the ones we
inserted at which point control is passed back to the host GDB (attached
via the GDB stub in QEMU). If it is not a breakpoint which was set-up by
the host then it must be one for the guest at which point we need to
ensure the exception is delivered to the guest for it to process.

Alex Bennée April 23, 2015, 2:26 p.m. UTC | #6

Christoffer Dall <[e-mail]> writes:

> On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
>> This adds support for SW breakpoints inserted by userspace.
>> 
>> We do this by trapping all BKPT exceptions in the
>> hypervisor (MDCR_EL2_TDE).
>
> you mean trapping all exceptions in the guest to the hypervisor?
>
>> The kvm_debug_exit_arch carries the address
>> of the exception.
>
> why?  can userspace not simply read out the PC using GET_ONE_REG?

Yes, I have re-worded and removed PC from the debug information.

<snip>
>>  
>> +	/* Trap breakpoints? */
>> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
>> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
>> +	else
>> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
>
> so now you're trapping all debug exceptions, right?
>
> what happens if the guest is using the hardware to debug debug stuff and
> generates other kinds of debug exceptions, like a hardware breakpoint,
> will we not see an unhandled exception and the guest being forcefully
> killed?

Yes until the later patches which stop the guest using HW debug
registers while we are using them.

>
>> +
>>  }
>>  
>>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
>> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
>> index 524fa25..ed1bbb4 100644
>> --- a/arch/arm64/kvm/handle_exit.c
>> +++ b/arch/arm64/kvm/handle_exit.c
>> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>  	return 1;
>>  }
>>  
>> +/**
>> + * kvm_handle_debug_exception - handle a debug exception instruction
>
> handle a software breadkpoint exception
>
>> + *
>> + * @vcpu:	the vcpu pointer
>> + * @run:	access to the kvm_run structure for results
>> + *
>> + * We route all debug exceptions through the same handler as we
>
> all debug exceptions?  software breakpoints and all?  then why the above
> shot text?
>
>> + * just need to report the PC and the HSR values to userspace.
>> + * Userspace may decide to re-inject the exception and deliver it to
>> + * the guest if it wasn't for the host to deal with.
>
> now I'm confused - does userspace setup the guest to receive an
> exception or does it tell KVM to emulate an exception for the guest or
> do we execute the breakpoint without trapping the debug exception?

I've made it all go through userspace as we may have to translate the
hypervisor visible exception code to what the guest was expecting to see.

>
>> + */
>> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> +{
>> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
>> +
>> +	run->exit_reason = KVM_EXIT_DEBUG;
>> +	run->debug.arch.hsr = hsr;
>> +
>> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
>> +	case ESR_ELx_EC_BKPT32:
>> +	case ESR_ELx_EC_BRK64:
>> +		run->debug.arch.pc = *vcpu_pc(vcpu);
>> +		break;
>> +	default:
>> +		kvm_err("%s: un-handled case hsr: %#08x\n",
>> +			__func__, (unsigned int) hsr);
>
> this should never happen right?

At the moment it could, at the end of the patch series we should cover
all the cases so it would indicate a bug. I've made it return an error
code so it fails hard as suggested by David.

Christoffer Dall April 27, 2015, 8:04 p.m. UTC | #7

On Thu, Apr 23, 2015 at 03:26:53PM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <[e-mail]> writes:
> 
> > On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
> >> This adds support for SW breakpoints inserted by userspace.
> >> 
> >> We do this by trapping all BKPT exceptions in the
> >> hypervisor (MDCR_EL2_TDE).
> >
> > you mean trapping all exceptions in the guest to the hypervisor?
> >
> >> The kvm_debug_exit_arch carries the address
> >> of the exception.
> >
> > why?  can userspace not simply read out the PC using GET_ONE_REG?
> 
> Yes, I have re-worded and removed PC from the debug information.
> 
> <snip>
> >>  
> >> +	/* Trap breakpoints? */
> >> +	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
> >> +		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
> >> +	else
> >> +		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
> >
> > so now you're trapping all debug exceptions, right?
> >
> > what happens if the guest is using the hardware to debug debug stuff and
> > generates other kinds of debug exceptions, like a hardware breakpoint,
> > will we not see an unhandled exception and the guest being forcefully
> > killed?
> 
> Yes until the later patches which stop the guest using HW debug
> registers while we are using them.
> 
> >
> >> +
> >>  }
> >>  
> >>  void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
> >> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> >> index 524fa25..ed1bbb4 100644
> >> --- a/arch/arm64/kvm/handle_exit.c
> >> +++ b/arch/arm64/kvm/handle_exit.c
> >> @@ -82,6 +82,37 @@ static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >>  	return 1;
> >>  }
> >>  
> >> +/**
> >> + * kvm_handle_debug_exception - handle a debug exception instruction
> >
> > handle a software breadkpoint exception
> >
> >> + *
> >> + * @vcpu:	the vcpu pointer
> >> + * @run:	access to the kvm_run structure for results
> >> + *
> >> + * We route all debug exceptions through the same handler as we
> >
> > all debug exceptions?  software breakpoints and all?  then why the above
> > shot text?
> >

I think the issue here was "debug exception instruction" making me think
this is just for software breakpoints...

Not sure what I meant by 'shot text' - probably 'short text'

> >> + * just need to report the PC and the HSR values to userspace.
> >> + * Userspace may decide to re-inject the exception and deliver it to
> >> + * the guest if it wasn't for the host to deal with.
> >
> > now I'm confused - does userspace setup the guest to receive an
> > exception or does it tell KVM to emulate an exception for the guest or
> > do we execute the breakpoint without trapping the debug exception?
> 
> I've made it all go through userspace as we may have to translate the
> hypervisor visible exception code to what the guest was expecting to see.
> 

ok, so I think you should re-phrase something like:

"Userspace may decide that this exception is caused by the guest using
debugging itself, and may in that case emulate the guest debug exception
in userspace before resuming KVM."

But does that really work?  Given that we don't support KVM-TCG
migration, this sounds a little strange.  Did we test it?

> >
> >> + */
> >> +static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
> >> +{
> >> +	u32 hsr = kvm_vcpu_get_hsr(vcpu);
> >> +
> >> +	run->exit_reason = KVM_EXIT_DEBUG;
> >> +	run->debug.arch.hsr = hsr;
> >> +
> >> +	switch (hsr >> ESR_ELx_EC_SHIFT) {
> >> +	case ESR_ELx_EC_BKPT32:
> >> +	case ESR_ELx_EC_BRK64:
> >> +		run->debug.arch.pc = *vcpu_pc(vcpu);
> >> +		break;
> >> +	default:
> >> +		kvm_err("%s: un-handled case hsr: %#08x\n",
> >> +			__func__, (unsigned int) hsr);
> >
> > this should never happen right?
> 
> At the moment it could, at the end of the patch series we should cover
> all the cases so it would indicate a bug. I've made it return an error
> code so it fails hard as suggested by David.
> 
hmm, ok, so I'm not so worried about that kind of bisectability
(although it would be nice to keep that working too), but reading
patches that way is a bit annoying for reviewers, so I recommend you
deal with the patch ordering in some way that makes it more obvious what
happens as reviewers read the patches, one at a time.

Thanks,
-Christoffer

Peter Maydell April 27, 2015, 9:57 p.m. UTC | #8

On 27 April 2015 at 21:04, Christoffer Dall <[e-mail]> wrote:
> On Thu, Apr 23, 2015 at 03:26:53PM +0100, Alex Bennée wrote:
>>
>> Christoffer Dall <[e-mail]> writes:
>>
>> > On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
>> >> + * just need to report the PC and the HSR values to userspace.
>> >> + * Userspace may decide to re-inject the exception and deliver it to
>> >> + * the guest if it wasn't for the host to deal with.
>> >
>> > now I'm confused - does userspace setup the guest to receive an
>> > exception or does it tell KVM to emulate an exception for the guest or
>> > do we execute the breakpoint without trapping the debug exception?
>>
>> I've made it all go through userspace as we may have to translate the
>> hypervisor visible exception code to what the guest was expecting to see.
>>
>
> ok, so I think you should re-phrase something like:
>
> "Userspace may decide that this exception is caused by the guest using
> debugging itself, and may in that case emulate the guest debug exception
> in userspace before resuming KVM."
>
> But does that really work?  Given that we don't support KVM-TCG
> migration, this sounds a little strange.  Did we test it?

The QEMU patches have a TODO note at the point where you'd want
to do this... Design-wise you can do the reinjection in the
kernel or in userspace (ppc QEMU does this in userspace, for
instance) because it's pretty much just setting registers to fake
up the exception-entry into EL1. Code-wise QEMU's ARM code isn't
set up to do it right now, but it shouldn't be too difficult to
persuade the TCG exception-entry code to work for this case too.

Does the kernel already have a conveniently implemented "inject
exception into guest" lump of code? If so it might be less effort
to do it that way round, maybe.

-- PMM

Alex Bennée April 28, 2015, 8:42 a.m. UTC | #9

Peter Maydell <[e-mail]> writes:

> On 27 April 2015 at 21:04, Christoffer Dall <[e-mail]> wrote:
>> On Thu, Apr 23, 2015 at 03:26:53PM +0100, Alex Bennée wrote:
>>>
>>> Christoffer Dall <[e-mail]> writes:
>>>
>>> > On Tue, Mar 31, 2015 at 04:08:04PM +0100, Alex Bennée wrote:
>>> >> + * just need to report the PC and the HSR values to userspace.
>>> >> + * Userspace may decide to re-inject the exception and deliver it to
>>> >> + * the guest if it wasn't for the host to deal with.
>>> >
>>> > now I'm confused - does userspace setup the guest to receive an
>>> > exception or does it tell KVM to emulate an exception for the guest or
>>> > do we execute the breakpoint without trapping the debug exception?
>>>
>>> I've made it all go through userspace as we may have to translate the
>>> hypervisor visible exception code to what the guest was expecting to see.
>>>
>>
>> ok, so I think you should re-phrase something like:
>>
>> "Userspace may decide that this exception is caused by the guest using
>> debugging itself, and may in that case emulate the guest debug exception
>> in userspace before resuming KVM."
>>
>> But does that really work?  Given that we don't support KVM-TCG
>> migration, this sounds a little strange.  Did we test it?
>
> The QEMU patches have a TODO note at the point where you'd want
> to do this... Design-wise you can do the reinjection in the
> kernel or in userspace (ppc QEMU does this in userspace, for
> instance) because it's pretty much just setting registers to fake
> up the exception-entry into EL1. Code-wise QEMU's ARM code isn't
> set up to do it right now, but it shouldn't be too difficult to
> persuade the TCG exception-entry code to work for this case too.
>
> Does the kernel already have a conveniently implemented "inject
> exception into guest" lump of code? If so it might be less effort
> to do it that way round, maybe.

So you pointed out we can't just re-inject the exceptions we get as we
need to map from things like ESR_ELx_EC_WATCHPT_LOW to
ESR_ELx_EC_WATCHPT_CUR before re-injection.

Of course if it is as simple as modifying the ESR_EL1 register and
returning +ve in the handle_exit path then I can do that but I assumed
if any other wrangling needs doing it should be done in userspace.

>
> -- PMM

Peter Maydell April 28, 2015, 9:34 a.m. UTC | #10

On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
> Peter Maydell <[e-mail]> writes:
>> Does the kernel already have a conveniently implemented "inject
>> exception into guest" lump of code? If so it might be less effort
>> to do it that way round, maybe.
>
> So you pointed out we can't just re-inject the exceptions we get as we
> need to map from things like ESR_ELx_EC_WATCHPT_LOW to
> ESR_ELx_EC_WATCHPT_CUR before re-injection.
>
> Of course if it is as simple as modifying the ESR_EL1 register and
> returning +ve in the handle_exit path then I can do that but I assumed
> if any other wrangling needs doing it should be done in userspace.

Well, somebody's got to do it, and it's the same amount of work
either way (fiddling with ESR, making sure we direct the guest
to the right exception vector entry point, maybe a few other
things).

-- PMM

Christoffer Dall April 28, 2015, 12:56 p.m. UTC | #11

On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
> > Peter Maydell <[e-mail]> writes:
> >> Does the kernel already have a conveniently implemented "inject
> >> exception into guest" lump of code? If so it might be less effort
> >> to do it that way round, maybe.
> >
> > So you pointed out we can't just re-inject the exceptions we get as we
> > need to map from things like ESR_ELx_EC_WATCHPT_LOW to
> > ESR_ELx_EC_WATCHPT_CUR before re-injection.
> >
> > Of course if it is as simple as modifying the ESR_EL1 register and
> > returning +ve in the handle_exit path then I can do that but I assumed
> > if any other wrangling needs doing it should be done in userspace.
> 
> Well, somebody's got to do it, and it's the same amount of work
> either way (fiddling with ESR, making sure we direct the guest
> to the right exception vector entry point, maybe a few other
> things).
> 
We already have code in the kernel to inject data/instruction aborts,
but not sure how much benefit there is in re-using that.  It's up to you
really, but I think the kernel code should be clear about what the
intention is so that we don't end up in a situation where: (1) The
intended behavior is unclear/vague, and (2) it doesn't actually work in
practice so nobody can follow the code.

Thanks,
-Christoffer

Alex Bennée April 28, 2015, 2:37 p.m. UTC | #12

Christoffer Dall <[e-mail]> writes:

> On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
>> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
>> > Peter Maydell <[e-mail]> writes:
>> >> Does the kernel already have a conveniently implemented "inject
>> >> exception into guest" lump of code? If so it might be less effort
>> >> to do it that way round, maybe.
>> >
>> > So you pointed out we can't just re-inject the exceptions we get as we
>> > need to map from things like ESR_ELx_EC_WATCHPT_LOW to
>> > ESR_ELx_EC_WATCHPT_CUR before re-injection.
>> >
>> > Of course if it is as simple as modifying the ESR_EL1 register and
>> > returning +ve in the handle_exit path then I can do that but I assumed
>> > if any other wrangling needs doing it should be done in userspace.
>> 
>> Well, somebody's got to do it, and it's the same amount of work
>> either way (fiddling with ESR, making sure we direct the guest
>> to the right exception vector entry point, maybe a few other
>> things).
>> 
> We already have code in the kernel to inject data/instruction aborts,
> but not sure how much benefit there is in re-using that.  It's up to you
> really, but I think the kernel code should be clear about what the
> intention is so that we don't end up in a situation where: (1) The
> intended behavior is unclear/vague, and (2) it doesn't actually work in
> practice so nobody can follow the code.

Certainly there are some cases where the kernel doesn't have all the
information. For example it doesn't know if the soft break was inserted
by the guest or the host. That to me favours the "let userspace deal
with the ugly" approach.

Christoffer Dall April 29, 2015, 8:10 a.m. UTC | #13

On Tue, Apr 28, 2015 at 03:37:01PM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <[e-mail]> writes:
> 
> > On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
> >> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
> >> > Peter Maydell <[e-mail]> writes:
> >> >> Does the kernel already have a conveniently implemented "inject
> >> >> exception into guest" lump of code? If so it might be less effort
> >> >> to do it that way round, maybe.
> >> >
> >> > So you pointed out we can't just re-inject the exceptions we get as we
> >> > need to map from things like ESR_ELx_EC_WATCHPT_LOW to
> >> > ESR_ELx_EC_WATCHPT_CUR before re-injection.
> >> >
> >> > Of course if it is as simple as modifying the ESR_EL1 register and
> >> > returning +ve in the handle_exit path then I can do that but I assumed
> >> > if any other wrangling needs doing it should be done in userspace.
> >> 
> >> Well, somebody's got to do it, and it's the same amount of work
> >> either way (fiddling with ESR, making sure we direct the guest
> >> to the right exception vector entry point, maybe a few other
> >> things).
> >> 
> > We already have code in the kernel to inject data/instruction aborts,
> > but not sure how much benefit there is in re-using that.  It's up to you
> > really, but I think the kernel code should be clear about what the
> > intention is so that we don't end up in a situation where: (1) The
> > intended behavior is unclear/vague, and (2) it doesn't actually work in
> > practice so nobody can follow the code.
> 
> Certainly there are some cases where the kernel doesn't have all the
> information. For example it doesn't know if the soft break was inserted
> by the guest or the host. That to me favours the "let userspace deal
> with the ugly" approach.
> 
Not sure I follow.

If it's an exception for the guest, then that must be because the guest
put in the breakpoint instruction, right?

However, that's a separate discussion from that of *how* userspace or
the kernel then injects an exception to the guest.

By using some QEMU TCG functionality or by QEMU calling back into KVM
and asking it to inject an exception for it.

What am I missing?

-Christoffer

Alex Bennée April 29, 2015, 9:18 a.m. UTC | #14

Christoffer Dall <[e-mail]> writes:

> On Tue, Apr 28, 2015 at 03:37:01PM +0100, Alex Bennée wrote:
>> 
>> Christoffer Dall <[e-mail]> writes:
>> 
>> > On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
>> >> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
>> >> > Peter Maydell <[e-mail]> writes:
>> >> >> Does the kernel already have a conveniently implemented "inject
>> >> >> exception into guest" lump of code? If so it might be less effort
>> >> >> to do it that way round, maybe.
>> >> >
>> >> > So you pointed out we can't just re-inject the exceptions we get as we
>> >> > need to map from things like ESR_ELx_EC_WATCHPT_LOW to
>> >> > ESR_ELx_EC_WATCHPT_CUR before re-injection.
>> >> >
>> >> > Of course if it is as simple as modifying the ESR_EL1 register and
>> >> > returning +ve in the handle_exit path then I can do that but I assumed
>> >> > if any other wrangling needs doing it should be done in userspace.
>> >> 
>> >> Well, somebody's got to do it, and it's the same amount of work
>> >> either way (fiddling with ESR, making sure we direct the guest
>> >> to the right exception vector entry point, maybe a few other
>> >> things).
>> >> 
>> > We already have code in the kernel to inject data/instruction aborts,
>> > but not sure how much benefit there is in re-using that.  It's up to you
>> > really, but I think the kernel code should be clear about what the
>> > intention is so that we don't end up in a situation where: (1) The
>> > intended behavior is unclear/vague, and (2) it doesn't actually work in
>> > practice so nobody can follow the code.
>> 
>> Certainly there are some cases where the kernel doesn't have all the
>> information. For example it doesn't know if the soft break was inserted
>> by the guest or the host. That to me favours the "let userspace deal
>> with the ugly" approach.
>> 
> Not sure I follow.
>
> If it's an exception for the guest, then that must be because the guest
> put in the breakpoint instruction, right?

No the host can add breakpoint instructions as well. They both generate
the same (redirected) exception to the hypervisor which then has to
figure out who planted the breakpoint and where the eventual exception
will be handled.

> However, that's a separate discussion from that of *how* userspace or
> the kernel then injects an exception to the guest.
>
> By using some QEMU TCG functionality or by QEMU calling back into KVM
> and asking it to inject an exception for it.

I don't know if there is explicit TCG functionality to use but QEMU can
set the registers and PC up for exception entry and re-enter KVM.

>
> What am I missing?
>
> -Christoffer

Christoffer Dall April 29, 2015, 10:38 a.m. UTC | #15

On Wed, Apr 29, 2015 at 10:18:18AM +0100, Alex Bennée wrote:
> 
> Christoffer Dall <[e-mail]> writes:
> 
> > On Tue, Apr 28, 2015 at 03:37:01PM +0100, Alex Bennée wrote:
> >> 
> >> Christoffer Dall <[e-mail]> writes:
> >> 
> >> > On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
> >> >> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
> >> >> > Peter Maydell <[e-mail]> writes:
> >> >> >> Does the kernel already have a conveniently implemented "inject
> >> >> >> exception into guest" lump of code? If so it might be less effort
> >> >> >> to do it that way round, maybe.
> >> >> >
> >> >> > So you pointed out we can't just re-inject the exceptions we get as we
> >> >> > need to map from things like ESR_ELx_EC_WATCHPT_LOW to
> >> >> > ESR_ELx_EC_WATCHPT_CUR before re-injection.
> >> >> >
> >> >> > Of course if it is as simple as modifying the ESR_EL1 register and
> >> >> > returning +ve in the handle_exit path then I can do that but I assumed
> >> >> > if any other wrangling needs doing it should be done in userspace.
> >> >> 
> >> >> Well, somebody's got to do it, and it's the same amount of work
> >> >> either way (fiddling with ESR, making sure we direct the guest
> >> >> to the right exception vector entry point, maybe a few other
> >> >> things).
> >> >> 
> >> > We already have code in the kernel to inject data/instruction aborts,
> >> > but not sure how much benefit there is in re-using that.  It's up to you
> >> > really, but I think the kernel code should be clear about what the
> >> > intention is so that we don't end up in a situation where: (1) The
> >> > intended behavior is unclear/vague, and (2) it doesn't actually work in
> >> > practice so nobody can follow the code.
> >> 
> >> Certainly there are some cases where the kernel doesn't have all the
> >> information. For example it doesn't know if the soft break was inserted
> >> by the guest or the host. That to me favours the "let userspace deal
> >> with the ugly" approach.
> >> 
> > Not sure I follow.
> >
> > If it's an exception for the guest, then that must be because the guest
> > put in the breakpoint instruction, right?
> 
> No the host can add breakpoint instructions as well. They both generate
> the same (redirected) exception to the hypervisor which then has to
> figure out who planted the breakpoint and where the eventual exception
> will be handled.

I understand this; let's just rewind here.

If you've concluded that the exception is for the guest, then the guest
must have placed the breakpoint instruction there, correct?  Otherwise,
the exception is for the hypervisor and the discussion about how to
inject an exception for the guest is invalid.

Or are you talking about the corner case where the host uses a soft
breakpoint to get a breakpoint on an instruction which is also a
breakpoint in the guest?

> 
> > However, that's a separate discussion from that of *how* userspace or
> > the kernel then injects an exception to the guest.
> >
> > By using some QEMU TCG functionality or by QEMU calling back into KVM
> > and asking it to inject an exception for it.
> 
> I don't know if there is explicit TCG functionality to use but QEMU can
> set the registers and PC up for exception entry and re-enter KVM.
> 

I also understand this.  I think Peter's point was exactly that if we
have existing code somewhere which we can reuse, then we should consider
reusing it.

Again, I don't care particularly which way, I just want the expected
working behavior to be clearly defined.

-Christoffer

Alex Bennée April 29, 2015, 3:08 p.m. UTC | #16

Christoffer Dall <[e-mail]> writes:

> On Wed, Apr 29, 2015 at 10:18:18AM +0100, Alex Bennée wrote:
>> 
>> Christoffer Dall <[e-mail]> writes:
>> 
>> > On Tue, Apr 28, 2015 at 03:37:01PM +0100, Alex Bennée wrote:
>> >> 
>> >> Christoffer Dall <[e-mail]> writes:
>> >> 
>> >> > On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
>> >> >> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
>> >> >> > Peter Maydell <[e-mail]> writes:
>> >> >> >> Does the kernel already have a conveniently implemented "inject
>> >> >> >> exception into guest" lump of code? If so it might be less effort
>> >> >> >> to do it that way round, maybe.
>> >> >> >
<snip>
>> >> 
>> >> Certainly there are some cases where the kernel doesn't have all the
>> >> information. For example it doesn't know if the soft break was inserted
>> >> by the guest or the host. That to me favours the "let userspace deal
>> >> with the ugly" approach.
>> >> 
>> > Not sure I follow.
>> >
>> > If it's an exception for the guest, then that must be because the guest
>> > put in the breakpoint instruction, right?
>> 
>> No the host can add breakpoint instructions as well. They both generate
>> the same (redirected) exception to the hypervisor which then has to
>> figure out who planted the breakpoint and where the eventual exception
>> will be handled.
>
> I understand this; let's just rewind here.
>
> If you've concluded that the exception is for the guest, then the guest
> must have placed the breakpoint instruction there, correct?  Otherwise,
> the exception is for the hypervisor and the discussion about how to
> inject an exception for the guest is invalid.

But only userspace has enough information to make that conclusion (after
searching the list of breakpoints it added to the code). So from
userspace we can:

  - re-enter KVM telling it to re-route the exception it just delivered
    to userspace somehow

  or

  - make the changes to deliver the exception in userspace and re-enter
    KVM as normal.

It seems to me if we have already exited into userspace it may as well
clean up if it has all the information it needs?

> Or are you talking about the corner case where the host uses a soft
> breakpoint to get a breakpoint on an instruction which is also a
> breakpoint in the guest?

I think in this case host debugging just wins.

>
>> 
>> > However, that's a separate discussion from that of *how* userspace or
>> > the kernel then injects an exception to the guest.
>> >
>> > By using some QEMU TCG functionality or by QEMU calling back into KVM
>> > and asking it to inject an exception for it.
>> 
>> I don't know if there is explicit TCG functionality to use but QEMU can
>> set the registers and PC up for exception entry and re-enter KVM.
>> 
>
> I also understand this.  I think Peter's point was exactly that if we
> have existing code somewhere which we can reuse, then we should consider
> reusing it.

I'm not sure such code exists. The only injection code I know of in KVMs
handle_exit code where a +ve return value signals KVM to deliver the
exception to the guest. This is used by the hvc and svc handlers after
calling kvm_inject_undefined() and the wfx handler which advances the PC
first.

> Again, I don't care particularly which way, I just want the expected
> working behavior to be clearly defined.

I think it makes sense to do it in userspace. I have the kernels
inject_fault code for reference for what needs setting up but I'll see
if I can find anything in QEMU that already handles this for some other
thing (although I don't think it does at first glance).

Christoffer Dall April 29, 2015, 7:20 p.m. UTC | #17

On Wed, Apr 29, 2015 at 5:08 PM, Alex Bennée <[e-mail]> wrote:
>
> Christoffer Dall <[e-mail]> writes:
>
>> On Wed, Apr 29, 2015 at 10:18:18AM +0100, Alex Bennée wrote:
>>>
>>> Christoffer Dall <[e-mail]> writes:
>>>
>>> > On Tue, Apr 28, 2015 at 03:37:01PM +0100, Alex Bennée wrote:
>>> >>
>>> >> Christoffer Dall <[e-mail]> writes:
>>> >>
>>> >> > On Tue, Apr 28, 2015 at 10:34:12AM +0100, Peter Maydell wrote:
>>> >> >> On 28 April 2015 at 09:42, Alex Bennée <[e-mail]> wrote:
>>> >> >> > Peter Maydell <[e-mail]> writes:
>>> >> >> >> Does the kernel already have a conveniently implemented "inject
>>> >> >> >> exception into guest" lump of code? If so it might be less effort
>>> >> >> >> to do it that way round, maybe.
>>> >> >> >
> <snip>
>>> >>
>>> >> Certainly there are some cases where the kernel doesn't have all the
>>> >> information. For example it doesn't know if the soft break was inserted
>>> >> by the guest or the host. That to me favours the "let userspace deal
>>> >> with the ugly" approach.
>>> >>
>>> > Not sure I follow.
>>> >
>>> > If it's an exception for the guest, then that must be because the guest
>>> > put in the breakpoint instruction, right?
>>>
>>> No the host can add breakpoint instructions as well. They both generate
>>> the same (redirected) exception to the hypervisor which then has to
>>> figure out who planted the breakpoint and where the eventual exception
>>> will be handled.
>>
>> I understand this; let's just rewind here.
>>
>> If you've concluded that the exception is for the guest, then the guest
>> must have placed the breakpoint instruction there, correct?  Otherwise,
>> the exception is for the hypervisor and the discussion about how to
>> inject an exception for the guest is invalid.
>
> But only userspace has enough information to make that conclusion (after
> searching the list of breakpoints it added to the code). So from
> userspace we can:
>
>   - re-enter KVM telling it to re-route the exception it just delivered
>     to userspace somehow
>
>   or
>
>   - make the changes to deliver the exception in userspace and re-enter
>     KVM as normal.
>

ok, we agree and are talking about the same thing.  good.

> It seems to me if we have already exited into userspace it may as well
> clean up if it has all the information it needs?
>

depends on the complexity and size of the code really, imho.

>> Or are you talking about the corner case where the host uses a soft
>> breakpoint to get a breakpoint on an instruction which is also a
>> breakpoint in the guest?
>
> I think in this case host debugging just wins.
>
ok

Patch

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 06c5064..17d4f9c 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -2626,7 +2626,7 @@  when running. Common control bits are:
 The top 16 bits of the control field are architecture specific control
 flags which can include the following:
 
-  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86]
+  - KVM_GUESTDBG_USE_SW_BP:     using software breakpoints [x86, arm64]
   - KVM_GUESTDBG_USE_HW_BP:     using hardware breakpoints [x86, s390]
   - KVM_GUESTDBG_INJECT_DB:     inject DB type exception [x86]
   - KVM_GUESTDBG_INJECT_BP:     inject BP type exception [x86]
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 7ea8b0e..d3bc8dc 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -304,7 +304,7 @@  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	kvm_arm_set_running_vcpu(NULL);
 }
 
-#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE)
+#define KVM_GUESTDBG_VALID (KVM_GUESTDBG_ENABLE|KVM_GUESTDBG_USE_SW_BP)
 
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
diff --git a/arch/arm64/kvm/debug.c b/arch/arm64/kvm/debug.c
index 8a29d0b..cff0475 100644
--- a/arch/arm64/kvm/debug.c
+++ b/arch/arm64/kvm/debug.c
@@ -45,11 +45,18 @@  void kvm_arch_setup_debug(struct kvm_vcpu *vcpu)
 	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TPM | MDCR_EL2_TPMCR);
 	vcpu->arch.mdcr_el2 |= (MDCR_EL2_TDRA | MDCR_EL2_TDOSA);
 
+	/* Trap debug register access? */
 	if (!vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
 		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDA;
 	else
 		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDA;
 
+	/* Trap breakpoints? */
+	if (vcpu->guest_debug & KVM_GUESTDBG_USE_SW_BP)
+		vcpu->arch.mdcr_el2 |= MDCR_EL2_TDE;
+	else
+		vcpu->arch.mdcr_el2 &= ~MDCR_EL2_TDE;
+
 }
 
 void kvm_arch_clear_debug(struct kvm_vcpu *vcpu)
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 524fa25..ed1bbb4 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -82,6 +82,37 @@  static int kvm_handle_wfx(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	return 1;
 }
 
+/**
+ * kvm_handle_debug_exception - handle a debug exception instruction
+ *
+ * @vcpu:	the vcpu pointer
+ * @run:	access to the kvm_run structure for results
+ *
+ * We route all debug exceptions through the same handler as we
+ * just need to report the PC and the HSR values to userspace.
+ * Userspace may decide to re-inject the exception and deliver it to
+ * the guest if it wasn't for the host to deal with.
+ */
+static int kvm_handle_guest_debug(struct kvm_vcpu *vcpu, struct kvm_run *run)
+{
+	u32 hsr = kvm_vcpu_get_hsr(vcpu);
+
+	run->exit_reason = KVM_EXIT_DEBUG;
+	run->debug.arch.hsr = hsr;
+
+	switch (hsr >> ESR_ELx_EC_SHIFT) {
+	case ESR_ELx_EC_BKPT32:
+	case ESR_ELx_EC_BRK64:
+		run->debug.arch.pc = *vcpu_pc(vcpu);
+		break;
+	default:
+		kvm_err("%s: un-handled case hsr: %#08x\n",
+			__func__, (unsigned int) hsr);
+		break;
+	}
+	return 0;
+}
+
 static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_WFx]	= kvm_handle_wfx,
 	[ESR_ELx_EC_CP15_32]	= kvm_handle_cp15_32,
@@ -96,6 +127,8 @@  static exit_handle_fn arm_exit_handlers[] = {
 	[ESR_ELx_EC_SYS64]	= kvm_handle_sys_reg,
 	[ESR_ELx_EC_IABT_LOW]	= kvm_handle_guest_abort,
 	[ESR_ELx_EC_DABT_LOW]	= kvm_handle_guest_abort,
+	[ESR_ELx_EC_BKPT32]	= kvm_handle_guest_debug,
+	[ESR_ELx_EC_BRK64]	= kvm_handle_guest_debug,
 };
 
 static exit_handle_fn kvm_get_exit_handler(struct kvm_vcpu *vcpu)