[PATCH] KVM: PPC: Book3S PR: fix software breakpoints

[PATCH] KVM: PPC: Book3S PR: fix software breakpoints

[Mark Cave-Ayland]

QEMU's kvm_handle_debug() function identifies software breakpoints by checking
for a value of 0 in kvm_debug_exit_arch's status field. Since this field isn't
explicitly set to 0 when the software breakpoint instruction is detected, any
previous non-zero value present causes a hang in QEMU as it tries to process
the breakpoint instruction incorrectly as a hardware breakpoint.

Ensure that the kvm_debug_exit_arch status field is set to 0 when the software
breakpoint instruction is detected (similar to the existing logic in booke.c
and e500_emulate.c) to restore software breakpoint functionality under Book3S
PR.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 arch/powerpc/kvm/emulate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
index bb4d09c1ad56..6fca38ca791f 100644
--- a/arch/powerpc/kvm/emulate.c
+++ b/arch/powerpc/kvm/emulate.c
@@ -271,6 +271,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
 		 */
 		if (inst = KVMPPC_INST_SW_BREAKPOINT) {
 			run->exit_reason = KVM_EXIT_DEBUG;
+			run->debug.arch.status = 0;
 			run->debug.arch.address = kvmppc_get_pc(vcpu);
 			emulated = EMULATE_EXIT_USER;
 			advance = 0;
-- 
2.11.0

Re: [PATCH] KVM: PPC: Book3S PR: fix software breakpoints

[Fabiano Rosas]

Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:

> QEMU's kvm_handle_debug() function identifies software breakpoints by checking
> for a value of 0 in kvm_debug_exit_arch's status field. Since this field isn't

LGTM, but let me start a discussion:

In arch/powerpc/include/uapi/asm/kvm.h I see the following:


/*
 * Defines for h/w breakpoint, watchpoint (read, write or both) and
 * software breakpoint.
 * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
 * for KVM_DEBUG_EXIT.
 */
#define KVMPPC_DEBUG_NONE		0x0
#define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
#define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
#define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)


this seems to be biased towards the BookE implementation which uses
KVMPPC_DEBUG_BREAKPOINT to indicate a "hardware breakpoint" (i.e. Instruction
Address Compare - ISA v2 Book III-E, section 10.4.1), and then
KVMPPC_DEBUG_NONE ends up implicitly meaning "software breakpoint" for
Book3s PR/HV.

> explicitly set to 0 when the software breakpoint instruction is detected, any
> previous non-zero value present causes a hang in QEMU as it tries to process
> the breakpoint instruction incorrectly as a hardware breakpoint.

What QEMU does (again biased towards BookE) is to check the 'status'
field and treat both h/w breakpoints and watchpoints as hardware
breakpoints (which is correct in a sense) and then proceed to look for
s/w breakpoints:

    if (arch_info->status) {
        return kvm_handle_hw_breakpoint(cs, arch_info);
    }

    if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
        return kvm_handle_sw_breakpoint(cs, arch_info->address);
    }

So I wonder if it would not be beneficial for future developers if we
drew the line and made a clear distinction between:

 software breakpoints - triggered by a KVMPPC_INST_SW_BREAKPOINT execution
 hardware breakpoints - triggered by some register match

Maybe by turning the first two defines into:

#define KVMPPC_DEBUG_SW_BREAKPOINT 0x0
#define KVMPPC_DEBUG_HW_BREAKPOINT (1UL << 1)

> Ensure that the kvm_debug_exit_arch status field is set to 0 when the software
> breakpoint instruction is detected (similar to the existing logic in booke.c
> and e500_emulate.c) to restore software breakpoint functionality under Book3S
> PR.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---

Anyway, the proposed patch fixes the issue.

Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

>  arch/powerpc/kvm/emulate.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index bb4d09c1ad56..6fca38ca791f 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -271,6 +271,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>  		 */
>  		if (inst = KVMPPC_INST_SW_BREAKPOINT) {
>  			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.status = 0;
>  			run->debug.arch.address = kvmppc_get_pc(vcpu);
>  			emulated = EMULATE_EXIT_USER;
>  			advance = 0;

Re: [PATCH] KVM: PPC: Book3S PR: fix software breakpoints

[Mark Cave-Ayland]

On 14/06/2019 23:14, Fabiano Rosas wrote:

> Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> writes:
> 
>> QEMU's kvm_handle_debug() function identifies software breakpoints by checking
>> for a value of 0 in kvm_debug_exit_arch's status field. Since this field isn't
> 
> LGTM, but let me start a discussion:
> 
> In arch/powerpc/include/uapi/asm/kvm.h I see the following:
> 
> 
> /*
>  * Defines for h/w breakpoint, watchpoint (read, write or both) and
>  * software breakpoint.
>  * These are used as "type" in KVM_SET_GUEST_DEBUG ioctl and "status"
>  * for KVM_DEBUG_EXIT.
>  */
> #define KVMPPC_DEBUG_NONE		0x0
> #define KVMPPC_DEBUG_BREAKPOINT		(1UL << 1)
> #define KVMPPC_DEBUG_WATCH_WRITE	(1UL << 2)
> #define KVMPPC_DEBUG_WATCH_READ		(1UL << 3)
> 
> 
> this seems to be biased towards the BookE implementation which uses
> KVMPPC_DEBUG_BREAKPOINT to indicate a "hardware breakpoint" (i.e. Instruction
> Address Compare - ISA v2 Book III-E, section 10.4.1), and then
> KVMPPC_DEBUG_NONE ends up implicitly meaning "software breakpoint" for
> Book3s PR/HV.
> 
>> explicitly set to 0 when the software breakpoint instruction is detected, any
>> previous non-zero value present causes a hang in QEMU as it tries to process
>> the breakpoint instruction incorrectly as a hardware breakpoint.
> 
> What QEMU does (again biased towards BookE) is to check the 'status'
> field and treat both h/w breakpoints and watchpoints as hardware
> breakpoints (which is correct in a sense) and then proceed to look for
> s/w breakpoints:
> 
>     if (arch_info->status) {
>         return kvm_handle_hw_breakpoint(cs, arch_info);
>     }
> 
>     if (kvm_find_sw_breakpoint(cs, arch_info->address)) {
>         return kvm_handle_sw_breakpoint(cs, arch_info->address);
>     }

Right so this feels a bit like BookE was used for the original implementation and
then the Book3S functionality followed on from that.

> So I wonder if it would not be beneficial for future developers if we
> drew the line and made a clear distinction between:
> 
>  software breakpoints - triggered by a KVMPPC_INST_SW_BREAKPOINT execution
>  hardware breakpoints - triggered by some register match
> 
> Maybe by turning the first two defines into:
> 
> #define KVMPPC_DEBUG_SW_BREAKPOINT 0x0
> #define KVMPPC_DEBUG_HW_BREAKPOINT (1UL << 1)

No objection from me, however I don't really have enough history with KVM PPC to be
able to opine as to whether this fits with the general design of
KVM/PPC/BookE/Book3S. I'm really coming at this from the perspective of toying with a
hobby project on my iMac G4 and fixing up the things I find are broken.

>> Ensure that the kvm_debug_exit_arch status field is set to 0 when the software
>> breakpoint instruction is detected (similar to the existing logic in booke.c
>> and e500_emulate.c) to restore software breakpoint functionality under Book3S
>> PR.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
> 
> Anyway, the proposed patch fixes the issue.
> 
> Reviewed-by: Fabiano Rosas <farosas@linux.ibm.com>

Great, thank you! Is this enough for the patch to be queued?

>>  arch/powerpc/kvm/emulate.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
>> index bb4d09c1ad56..6fca38ca791f 100644
>> --- a/arch/powerpc/kvm/emulate.c
>> +++ b/arch/powerpc/kvm/emulate.c
>> @@ -271,6 +271,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>>  		 */
>>  		if (inst = KVMPPC_INST_SW_BREAKPOINT) {
>>  			run->exit_reason = KVM_EXIT_DEBUG;
>> +			run->debug.arch.status = 0;
>>  			run->debug.arch.address = kvmppc_get_pc(vcpu);
>>  			emulated = EMULATE_EXIT_USER;
>>  			advance = 0;


ATB,

Mark.

Re: [PATCH] KVM: PPC: Book3S PR: fix software breakpoints

[Paul Mackerras]

On Fri, Jun 14, 2019 at 07:57:45PM +0100, Mark Cave-Ayland wrote:
> QEMU's kvm_handle_debug() function identifies software breakpoints by checking
> for a value of 0 in kvm_debug_exit_arch's status field. Since this field isn't
> explicitly set to 0 when the software breakpoint instruction is detected, any
> previous non-zero value present causes a hang in QEMU as it tries to process
> the breakpoint instruction incorrectly as a hardware breakpoint.
> 
> Ensure that the kvm_debug_exit_arch status field is set to 0 when the software
> breakpoint instruction is detected (similar to the existing logic in booke.c
> and e500_emulate.c) to restore software breakpoint functionality under Book3S
> PR.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  arch/powerpc/kvm/emulate.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/kvm/emulate.c b/arch/powerpc/kvm/emulate.c
> index bb4d09c1ad56..6fca38ca791f 100644
> --- a/arch/powerpc/kvm/emulate.c
> +++ b/arch/powerpc/kvm/emulate.c
> @@ -271,6 +271,7 @@ int kvmppc_emulate_instruction(struct kvm_run *run, struct kvm_vcpu *vcpu)
>  		 */
>  		if (inst = KVMPPC_INST_SW_BREAKPOINT) {
>  			run->exit_reason = KVM_EXIT_DEBUG;
> +			run->debug.arch.status = 0;
>  			run->debug.arch.address = kvmppc_get_pc(vcpu);
>  			emulated = EMULATE_EXIT_USER;
>  			advance = 0;

Thanks, applied to my kvm-ppc-next branch.

Paul.