| Message ID | 20190614185745.6863-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | Accepted |
| Headers | show |
| Series | KVM: PPC: Book3S PR: fix software breakpoints | expand |
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;
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.
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.
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;
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(+)