* [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
@ 2023-12-21 9:49 Chao Du
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Chao Du @ 2023-12-21 9:49 UTC (permalink / raw)
To: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
dbarboza, zhiwei_liu, palmer, anup, atishp
This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
debug RISC-V KVM guest from the host side, with software breakpoints.
A brief test was done on QEMU RISC-V hypervisor emulator.
A TODO list which will be added later:
1. HW breakpoints support
2. Test cases
This series is based on QEMU 8.2.0-rc4 and is also available at:
https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
This is dependent on KVM side changes:
https://github.com/Du-Chao/linux/tree/riscv_gd_sw
Chao Du (4):
target/riscv/kvm: add software breakpoints support
target/riscv/kvm: implement kvm_arch_update_guest_debug()
target/riscv/kvm: handle the exit with debug reason
linux-headers: enable KVM GUEST DEBUG for RISC-V
accel/kvm/kvm-all.c | 8 +--
include/sysemu/kvm.h | 6 +-
linux-headers/asm-riscv/kvm.h | 1 +
target/arm/kvm64.c | 6 +-
target/i386/kvm/kvm.c | 6 +-
target/mips/kvm.c | 6 +-
target/ppc/kvm.c | 6 +-
target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
target/s390x/kvm/kvm.c | 6 +-
9 files changed, 130 insertions(+), 16 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 12+ messages in thread
* [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
@ 2023-12-21 9:49 ` Chao Du
2024-04-16 9:23 ` Daniel Henrique Barboza
2023-12-21 9:49 ` [RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug() Chao Du
` (3 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Chao Du @ 2023-12-21 9:49 UTC (permalink / raw)
To: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
dbarboza, zhiwei_liu, palmer, anup, atishp
This patch implements insert/remove software breakpoint process:
Add an input parameter for kvm_arch_insert_sw_breakpoint() and
kvm_arch_remove_sw_breakpoint() to pass the length information,
which helps us to know whether it is a compressed instruction.
For some remove cases, we do not have the length info, so we need
to judge by ourselves.
For RISC-V, GDB treats single-step similarly to breakpoint: add a
breakpoint at the next step address, then continue. So this also
works for single-step debugging.
Add some stubs which are necessary for building, and will be
implemented later.
Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
accel/kvm/kvm-all.c | 8 ++--
include/sysemu/kvm.h | 6 ++-
target/arm/kvm64.c | 6 ++-
target/i386/kvm/kvm.c | 6 ++-
target/mips/kvm.c | 6 ++-
target/ppc/kvm.c | 6 ++-
target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
target/s390x/kvm/kvm.c | 6 ++-
8 files changed, 107 insertions(+), 16 deletions(-)
diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index e39a810a4e..ccc505d0c2 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -3231,7 +3231,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
bp = g_new(struct kvm_sw_breakpoint, 1);
bp->pc = addr;
bp->use_count = 1;
- err = kvm_arch_insert_sw_breakpoint(cpu, bp);
+ err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
if (err) {
g_free(bp);
return err;
@@ -3270,7 +3270,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
return 0;
}
- err = kvm_arch_remove_sw_breakpoint(cpu, bp);
+ err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
if (err) {
return err;
}
@@ -3300,10 +3300,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
CPUState *tmpcpu;
QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
- if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
+ if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
/* Try harder to find a CPU that currently sees the breakpoint. */
CPU_FOREACH(tmpcpu) {
- if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
+ if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
break;
}
}
diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
index d614878164..ab38c23def 100644
--- a/include/sysemu/kvm.h
+++ b/include/sysemu/kvm.h
@@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
int kvm_sw_breakpoints_active(CPUState *cpu);
int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
- struct kvm_sw_breakpoint *bp);
+ struct kvm_sw_breakpoint *bp,
+ vaddr len);
int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
- struct kvm_sw_breakpoint *bp);
+ struct kvm_sw_breakpoint *bp,
+ vaddr len);
int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
void kvm_arch_remove_all_hw_breakpoints(void);
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 3c175c93a7..023e92b577 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -1139,7 +1139,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
/* C6.6.29 BRK instruction */
static const uint32_t brk_insn = 0xd4200000;
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
if (have_guest_debug) {
if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
@@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
}
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
static uint32_t brk;
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 4ce80555b4..742b7c8296 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -4935,7 +4935,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
return 1;
}
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
static const uint8_t int3 = 0xcc;
@@ -4946,7 +4947,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
return 0;
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
uint8_t int3;
diff --git a/target/mips/kvm.c b/target/mips/kvm.c
index e22e24ed97..2f68938cdf 100644
--- a/target/mips/kvm.c
+++ b/target/mips/kvm.c
@@ -112,13 +112,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
DPRINTF("%s\n", __func__);
}
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
DPRINTF("%s\n", __func__);
return 0;
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
DPRINTF("%s\n", __func__);
return 0;
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 9b1abe2fc4..a99c85b2f3 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -1375,7 +1375,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
return 0;
}
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
/* Mixed endian case is not handled */
uint32_t sc = debug_inst_opcode;
@@ -1389,7 +1390,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
return 0;
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
uint32_t sc;
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 45b6cf1cfa..e9110006b0 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1521,3 +1521,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
};
DEFINE_TYPES(riscv_kvm_cpu_type_infos)
+
+static const uint32_t ebreak_insn = 0x00100073;
+static const uint16_t c_ebreak_insn = 0x9002;
+
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
+{
+ if (len != 4 && len != 2) {
+ return -EINVAL;
+ }
+
+ uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
+ (uint8_t *)&c_ebreak_insn;
+
+ if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
+ cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
+{
+ uint8_t length;
+
+ if (len == 4 || len == 2) {
+ length = (uint8_t)len;
+ } else if (len == 0) {
+ /* Need to decide the instruction length in this case. */
+ uint32_t read_4_bytes;
+ uint16_t read_2_bytes;
+
+ if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
+ cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
+ return -EINVAL;
+ }
+
+ if (read_4_bytes == ebreak_insn) {
+ length = 4;
+ } else if (read_2_bytes == c_ebreak_insn) {
+ length = 2;
+ } else {
+ return -EINVAL;
+ }
+ } else {
+ return -EINVAL;
+ }
+
+ if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
+ length, 1)) {
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
+{
+ /* TODO; To be implemented later. */
+ return -EINVAL;
+}
+
+int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
+{
+ /* TODO; To be implemented later. */
+ return -EINVAL;
+}
+
+void kvm_arch_remove_all_hw_breakpoints(void)
+{
+ /* TODO; To be implemented later. */
+}
+
+void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
+{
+ /* TODO; To be implemented later. */
+}
diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
index 33ab3551f4..fafacedd6a 100644
--- a/target/s390x/kvm/kvm.c
+++ b/target/s390x/kvm/kvm.c
@@ -867,7 +867,8 @@ static void determine_sw_breakpoint_instr(void)
}
}
-int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
determine_sw_breakpoint_instr();
@@ -879,7 +880,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
return 0;
}
-int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
+int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
+ vaddr len)
{
uint8_t t[MAX_ILEN];
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug()
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
@ 2023-12-21 9:49 ` Chao Du
2023-12-21 9:49 ` [RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason Chao Du
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Chao Du @ 2023-12-21 9:49 UTC (permalink / raw)
To: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
dbarboza, zhiwei_liu, palmer, anup, atishp
Set the control flag when there are active breakpoints. This will
help KVM to know the status in the userspace.
Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
target/riscv/kvm/kvm-cpu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index e9110006b0..94697b09bb 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1598,5 +1598,7 @@ void kvm_arch_remove_all_hw_breakpoints(void)
void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
{
- /* TODO; To be implemented later. */
+ if (kvm_sw_breakpoints_active(cs)) {
+ dbg->control |= KVM_GUESTDBG_ENABLE;
+ }
}
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
2023-12-21 9:49 ` [RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug() Chao Du
@ 2023-12-21 9:49 ` Chao Du
2023-12-21 9:49 ` [RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V Chao Du
2023-12-22 14:16 ` [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Daniel Henrique Barboza
4 siblings, 0 replies; 12+ messages in thread
From: Chao Du @ 2023-12-21 9:49 UTC (permalink / raw)
To: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
dbarboza, zhiwei_liu, palmer, anup, atishp
If the breakpoint belongs to the userspace then set the ret value.
Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
target/riscv/kvm/kvm-cpu.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
index 94697b09bb..b1aed78780 100644
--- a/target/riscv/kvm/kvm-cpu.c
+++ b/target/riscv/kvm/kvm-cpu.c
@@ -1213,6 +1213,21 @@ static int kvm_riscv_handle_sbi(CPUState *cs, struct kvm_run *run)
return ret;
}
+static bool kvm_riscv_handle_debug(CPUState *cs)
+{
+ RISCVCPU *cpu = RISCV_CPU(cs);
+ CPURISCVState *env = &cpu->env;
+
+ /* Ensure PC is synchronised */
+ kvm_cpu_synchronize_state(cs);
+
+ if (kvm_find_sw_breakpoint(cs, env->pc)) {
+ return true;
+ }
+
+ return false;
+}
+
int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
{
int ret = 0;
@@ -1220,6 +1235,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
case KVM_EXIT_RISCV_SBI:
ret = kvm_riscv_handle_sbi(cs, run);
break;
+ case KVM_EXIT_DEBUG:
+ if (kvm_riscv_handle_debug(cs)) {
+ ret = EXCP_DEBUG;
+ }
+ break;
default:
qemu_log_mask(LOG_UNIMP, "%s: un-handled exit reason %d\n",
__func__, run->exit_reason);
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
` (2 preceding siblings ...)
2023-12-21 9:49 ` [RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason Chao Du
@ 2023-12-21 9:49 ` Chao Du
2023-12-22 14:16 ` [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Daniel Henrique Barboza
4 siblings, 0 replies; 12+ messages in thread
From: Chao Du @ 2023-12-21 9:49 UTC (permalink / raw)
To: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
dbarboza, zhiwei_liu, palmer, anup, atishp
Synchronize the kvm.h file which enables KVM GUEST DEBUG at QEMU side.
Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
| 1 +
1 file changed, 1 insertion(+)
--git a/linux-headers/asm-riscv/kvm.h b/linux-headers/asm-riscv/kvm.h
index 992c5e4071..72942a7aaf 100644
--- a/linux-headers/asm-riscv/kvm.h
+++ b/linux-headers/asm-riscv/kvm.h
@@ -17,6 +17,7 @@
#define __KVM_HAVE_IRQ_LINE
#define __KVM_HAVE_READONLY_MEM
+#define __KVM_HAVE_GUEST_DEBUG
#define KVM_COALESCED_MMIO_PAGE_OFFSET 1
--
2.17.1
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
` (3 preceding siblings ...)
2023-12-21 9:49 ` [RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V Chao Du
@ 2023-12-22 14:16 ` Daniel Henrique Barboza
2024-04-09 9:43 ` Chao Du
4 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2023-12-22 14:16 UTC (permalink / raw)
To: Chao Du, qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng,
liweiwei, zhiwei_liu, palmer, anup, atishp
Hi,
It seems that we still need the kernel KVM side to be sorted out first [1],
so I believe we should wait a bit until we can review this RFC. Otherwise we
might risk reviewing something that has to be changed later.
[1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
Thanks,
Daniel
On 12/21/23 06:49, Chao Du wrote:
> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> debug RISC-V KVM guest from the host side, with software breakpoints.
>
> A brief test was done on QEMU RISC-V hypervisor emulator.
>
> A TODO list which will be added later:
> 1. HW breakpoints support
> 2. Test cases
>
> This series is based on QEMU 8.2.0-rc4 and is also available at:
> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
>
> This is dependent on KVM side changes:
> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
>
> Chao Du (4):
> target/riscv/kvm: add software breakpoints support
> target/riscv/kvm: implement kvm_arch_update_guest_debug()
> target/riscv/kvm: handle the exit with debug reason
> linux-headers: enable KVM GUEST DEBUG for RISC-V
>
> accel/kvm/kvm-all.c | 8 +--
> include/sysemu/kvm.h | 6 +-
> linux-headers/asm-riscv/kvm.h | 1 +
> target/arm/kvm64.c | 6 +-
> target/i386/kvm/kvm.c | 6 +-
> target/mips/kvm.c | 6 +-
> target/ppc/kvm.c | 6 +-
> target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
> target/s390x/kvm/kvm.c | 6 +-
> 9 files changed, 130 insertions(+), 16 deletions(-)
>
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
2023-12-22 14:16 ` [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Daniel Henrique Barboza
@ 2024-04-09 9:43 ` Chao Du
2024-04-15 1:47 ` Chao Du
2024-04-16 9:25 ` Daniel Henrique Barboza
0 siblings, 2 replies; 12+ messages in thread
From: Chao Du @ 2024-04-09 9:43 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
zhiwei_liu, palmer, anup, atishp
Hi Daniel and all,
The KVM patches have been reviewd and are in the queue.
https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
Could you please review in the QEMU side ?
Then I will rebase this series with your comments.
Some Notes:
1. As the first stage, only the software breakpoints is implemented.
2. A 'corner case' in which the debug exception is not inserted by the
debugger, need to be re-injected to the guest. This is not handled yet
in this series.
Thanks,
Chao
On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>
> Hi,
>
> It seems that we still need the kernel KVM side to be sorted out first [1],
> so I believe we should wait a bit until we can review this RFC. Otherwise we
> might risk reviewing something that has to be changed later.
>
>
> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
>
>
> Thanks,
>
> Daniel
>
> On 12/21/23 06:49, Chao Du wrote:
> > This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> > debug RISC-V KVM guest from the host side, with software breakpoints.
> >
> > A brief test was done on QEMU RISC-V hypervisor emulator.
> >
> > A TODO list which will be added later:
> > 1. HW breakpoints support
> > 2. Test cases
> >
> > This series is based on QEMU 8.2.0-rc4 and is also available at:
> > https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> >
> > This is dependent on KVM side changes:
> > https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> >
> > Chao Du (4):
> > target/riscv/kvm: add software breakpoints support
> > target/riscv/kvm: implement kvm_arch_update_guest_debug()
> > target/riscv/kvm: handle the exit with debug reason
> > linux-headers: enable KVM GUEST DEBUG for RISC-V
> >
> > accel/kvm/kvm-all.c | 8 +--
> > include/sysemu/kvm.h | 6 +-
> > linux-headers/asm-riscv/kvm.h | 1 +
> > target/arm/kvm64.c | 6 +-
> > target/i386/kvm/kvm.c | 6 +-
> > target/mips/kvm.c | 6 +-
> > target/ppc/kvm.c | 6 +-
> > target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
> > target/s390x/kvm/kvm.c | 6 +-
> > 9 files changed, 130 insertions(+), 16 deletions(-)
> >
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
2024-04-09 9:43 ` Chao Du
@ 2024-04-15 1:47 ` Chao Du
2024-04-16 9:25 ` Daniel Henrique Barboza
1 sibling, 0 replies; 12+ messages in thread
From: Chao Du @ 2024-04-15 1:47 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
zhiwei_liu, palmer, anup, atishp
Gentle ping...
On 2024-04-09 17:43, Chao Du <duchao@eswincomputing.com> wrote:
>
> Hi Daniel and all,
>
> The KVM patches have been reviewd and are in the queue.
> https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
>
> Could you please review in the QEMU side ?
> Then I will rebase this series with your comments.
>
> Some Notes:
> 1. As the first stage, only the software breakpoints is implemented.
> 2. A 'corner case' in which the debug exception is not inserted by the
> debugger, need to be re-injected to the guest. This is not handled yet
> in this series.
>
> Thanks,
> Chao
>
>
> On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> >
> > Hi,
> >
> > It seems that we still need the kernel KVM side to be sorted out first [1],
> > so I believe we should wait a bit until we can review this RFC. Otherwise we
> > might risk reviewing something that has to be changed later.
> >
> >
> > [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
> >
> >
> > Thanks,
> >
> > Daniel
> >
> > On 12/21/23 06:49, Chao Du wrote:
> > > This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> > > debug RISC-V KVM guest from the host side, with software breakpoints.
> > >
> > > A brief test was done on QEMU RISC-V hypervisor emulator.
> > >
> > > A TODO list which will be added later:
> > > 1. HW breakpoints support
> > > 2. Test cases
> > >
> > > This series is based on QEMU 8.2.0-rc4 and is also available at:
> > > https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> > >
> > > This is dependent on KVM side changes:
> > > https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> > >
> > > Chao Du (4):
> > > target/riscv/kvm: add software breakpoints support
> > > target/riscv/kvm: implement kvm_arch_update_guest_debug()
> > > target/riscv/kvm: handle the exit with debug reason
> > > linux-headers: enable KVM GUEST DEBUG for RISC-V
> > >
> > > accel/kvm/kvm-all.c | 8 +--
> > > include/sysemu/kvm.h | 6 +-
> > > linux-headers/asm-riscv/kvm.h | 1 +
> > > target/arm/kvm64.c | 6 +-
> > > target/i386/kvm/kvm.c | 6 +-
> > > target/mips/kvm.c | 6 +-
> > > target/ppc/kvm.c | 6 +-
> > > target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
> > > target/s390x/kvm/kvm.c | 6 +-
> > > 9 files changed, 130 insertions(+), 16 deletions(-)
> > >
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
@ 2024-04-16 9:23 ` Daniel Henrique Barboza
2024-04-18 9:46 ` Chao Du
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-04-16 9:23 UTC (permalink / raw)
To: Chao Du, qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng,
liweiwei, zhiwei_liu, palmer, anup, atishp
On 12/21/23 06:49, Chao Du wrote:
> This patch implements insert/remove software breakpoint process:
>
> Add an input parameter for kvm_arch_insert_sw_breakpoint() and
> kvm_arch_remove_sw_breakpoint() to pass the length information,
> which helps us to know whether it is a compressed instruction.
> For some remove cases, we do not have the length info, so we need
> to judge by ourselves.
>
> For RISC-V, GDB treats single-step similarly to breakpoint: add a
> breakpoint at the next step address, then continue. So this also
> works for single-step debugging.
>
> Add some stubs which are necessary for building, and will be
> implemented later.
>
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
> accel/kvm/kvm-all.c | 8 ++--
> include/sysemu/kvm.h | 6 ++-
> target/arm/kvm64.c | 6 ++-
> target/i386/kvm/kvm.c | 6 ++-
> target/mips/kvm.c | 6 ++-
> target/ppc/kvm.c | 6 ++-
> target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
> target/s390x/kvm/kvm.c | 6 ++-
> 8 files changed, 107 insertions(+), 16 deletions(-)
>
> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> index e39a810a4e..ccc505d0c2 100644
> --- a/accel/kvm/kvm-all.c
> +++ b/accel/kvm/kvm-all.c
> @@ -3231,7 +3231,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> bp = g_new(struct kvm_sw_breakpoint, 1);
> bp->pc = addr;
> bp->use_count = 1;
> - err = kvm_arch_insert_sw_breakpoint(cpu, bp);
> + err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
> if (err) {
> g_free(bp);
> return err;
> @@ -3270,7 +3270,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> return 0;
> }
>
> - err = kvm_arch_remove_sw_breakpoint(cpu, bp);
> + err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
> if (err) {
> return err;
> }
> @@ -3300,10 +3300,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
> CPUState *tmpcpu;
>
> QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
> - if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
> + if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
> /* Try harder to find a CPU that currently sees the breakpoint. */
> CPU_FOREACH(tmpcpu) {
> - if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
> + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
> break;
> }
> }
> diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> index d614878164..ab38c23def 100644
> --- a/include/sysemu/kvm.h
> +++ b/include/sysemu/kvm.h
> @@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
> int kvm_sw_breakpoints_active(CPUState *cpu);
>
> int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
> - struct kvm_sw_breakpoint *bp);
> + struct kvm_sw_breakpoint *bp,
> + vaddr len);
> int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
> - struct kvm_sw_breakpoint *bp);
> + struct kvm_sw_breakpoint *bp,
> + vaddr len);
> int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
> int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
> void kvm_arch_remove_all_hw_breakpoints(void);
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 3c175c93a7..023e92b577 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -1139,7 +1139,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> /* C6.6.29 BRK instruction */
> static const uint32_t brk_insn = 0xd4200000;
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> if (have_guest_debug) {
> if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> @@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> }
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> static uint32_t brk;
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 4ce80555b4..742b7c8296 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -4935,7 +4935,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
> return 1;
> }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> static const uint8_t int3 = 0xcc;
>
> @@ -4946,7 +4947,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> return 0;
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> uint8_t int3;
>
> diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> index e22e24ed97..2f68938cdf 100644
> --- a/target/mips/kvm.c
> +++ b/target/mips/kvm.c
> @@ -112,13 +112,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
> DPRINTF("%s\n", __func__);
> }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> DPRINTF("%s\n", __func__);
> return 0;
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> DPRINTF("%s\n", __func__);
> return 0;
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 9b1abe2fc4..a99c85b2f3 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -1375,7 +1375,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> return 0;
> }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> /* Mixed endian case is not handled */
> uint32_t sc = debug_inst_opcode;
> @@ -1389,7 +1390,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> return 0;
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> uint32_t sc;
>
> diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> index 45b6cf1cfa..e9110006b0 100644
> --- a/target/riscv/kvm/kvm-cpu.c
> +++ b/target/riscv/kvm/kvm-cpu.c
> @@ -1521,3 +1521,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
> };
>
> DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> +
> +static const uint32_t ebreak_insn = 0x00100073;
> +static const uint16_t c_ebreak_insn = 0x9002;
> +
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> +{
> + if (len != 4 && len != 2) {
> + return -EINVAL;
> + }
I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
there any known reason why other archs would use 'len' other than 2 or 4? The
parent function can throw the EINVAL in this case. Otherwise all callers from
all archs will need a similar EINVAL check.
> +
> + uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
> + (uint8_t *)&c_ebreak_insn;
> +
> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
> + cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> +{
> + uint8_t length;
> +
> + if (len == 4 || len == 2) {
> + length = (uint8_t)len;
Same question as above - perhaps the len = 0 | 2 | 4 conditional can be moved to
kvm_remove_breakpoint().
Thanks,
Daniel
> + } else if (len == 0) {
> + /* Need to decide the instruction length in this case. */
> + uint32_t read_4_bytes;
> + uint16_t read_2_bytes;
> +
> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
> + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
> + return -EINVAL;
> + }
> +
> + if (read_4_bytes == ebreak_insn) {
> + length = 4;
> + } else if (read_2_bytes == c_ebreak_insn) {
> + length = 2;
> + } else {
> + return -EINVAL;
> + }
> + } else {
> + return -EINVAL;
> + }
> +
> + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> + length, 1)) {
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> + /* TODO; To be implemented later. */
> + return -EINVAL;
> +}
> +
> +int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
> +{
> + /* TODO; To be implemented later. */
> + return -EINVAL;
> +}
> +
> +void kvm_arch_remove_all_hw_breakpoints(void)
> +{
> + /* TODO; To be implemented later. */
> +}
> +
> +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> +{
> + /* TODO; To be implemented later. */
> +}
> diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> index 33ab3551f4..fafacedd6a 100644
> --- a/target/s390x/kvm/kvm.c
> +++ b/target/s390x/kvm/kvm.c
> @@ -867,7 +867,8 @@ static void determine_sw_breakpoint_instr(void)
> }
> }
>
> -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> determine_sw_breakpoint_instr();
>
> @@ -879,7 +880,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> return 0;
> }
>
> -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> + vaddr len)
> {
> uint8_t t[MAX_ILEN];
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
2024-04-09 9:43 ` Chao Du
2024-04-15 1:47 ` Chao Du
@ 2024-04-16 9:25 ` Daniel Henrique Barboza
2024-05-20 10:22 ` Chao Du
1 sibling, 1 reply; 12+ messages in thread
From: Daniel Henrique Barboza @ 2024-04-16 9:25 UTC (permalink / raw)
To: Chao Du
Cc: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
zhiwei_liu, palmer, anup, atishp
On 4/9/24 06:43, Chao Du wrote:
> Hi Daniel and all,
>
> The KVM patches have been reviewd and are in the queue.
> https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
>
> Could you please review in the QEMU side ?
> Then I will rebase this series with your comments.
>
> Some Notes:
> 1. As the first stage, only the software breakpoints is implemented.
> 2. A 'corner case' in which the debug exception is not inserted by the
> debugger, need to be re-injected to the guest. This is not handled yet
> in this series.
Aside from the comments I made in patch 1 w.r.t checks that (perhaps) can be moved
to kvm-all.c, it looks good to me.
Since you're changing kvm-all.c we'll need Paolo to ack the changes in patch 1, so
feel free to wait for him to take a look before sending v2.
Thanks,
Daniel
>
> Thanks,
> Chao
>
>
> On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>>
>> Hi,
>>
>> It seems that we still need the kernel KVM side to be sorted out first [1],
>> so I believe we should wait a bit until we can review this RFC. Otherwise we
>> might risk reviewing something that has to be changed later.
>>
>>
>> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
>>
>>
>> Thanks,
>>
>> Daniel
>>
>> On 12/21/23 06:49, Chao Du wrote:
>>> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
>>> debug RISC-V KVM guest from the host side, with software breakpoints.
>>>
>>> A brief test was done on QEMU RISC-V hypervisor emulator.
>>>
>>> A TODO list which will be added later:
>>> 1. HW breakpoints support
>>> 2. Test cases
>>>
>>> This series is based on QEMU 8.2.0-rc4 and is also available at:
>>> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
>>>
>>> This is dependent on KVM side changes:
>>> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
>>>
>>> Chao Du (4):
>>> target/riscv/kvm: add software breakpoints support
>>> target/riscv/kvm: implement kvm_arch_update_guest_debug()
>>> target/riscv/kvm: handle the exit with debug reason
>>> linux-headers: enable KVM GUEST DEBUG for RISC-V
>>>
>>> accel/kvm/kvm-all.c | 8 +--
>>> include/sysemu/kvm.h | 6 +-
>>> linux-headers/asm-riscv/kvm.h | 1 +
>>> target/arm/kvm64.c | 6 +-
>>> target/i386/kvm/kvm.c | 6 +-
>>> target/mips/kvm.c | 6 +-
>>> target/ppc/kvm.c | 6 +-
>>> target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
>>> target/s390x/kvm/kvm.c | 6 +-
>>> 9 files changed, 130 insertions(+), 16 deletions(-)
>>>
>>> --
>>> 2.17.1
>>>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support
2024-04-16 9:23 ` Daniel Henrique Barboza
@ 2024-04-18 9:46 ` Chao Du
0 siblings, 0 replies; 12+ messages in thread
From: Chao Du @ 2024-04-18 9:46 UTC (permalink / raw)
To: Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, pbonzini, alistair23, bin.meng, liweiwei,
zhiwei_liu, palmer, anup, atishp
On 2024-04-16 17:23, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>
> On 12/21/23 06:49, Chao Du wrote:
> > This patch implements insert/remove software breakpoint process:
> >
> > Add an input parameter for kvm_arch_insert_sw_breakpoint() and
> > kvm_arch_remove_sw_breakpoint() to pass the length information,
> > which helps us to know whether it is a compressed instruction.
> > For some remove cases, we do not have the length info, so we need
> > to judge by ourselves.
> >
> > For RISC-V, GDB treats single-step similarly to breakpoint: add a
> > breakpoint at the next step address, then continue. So this also
> > works for single-step debugging.
> >
> > Add some stubs which are necessary for building, and will be
> > implemented later.
> >
> > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > ---
> > accel/kvm/kvm-all.c | 8 ++--
> > include/sysemu/kvm.h | 6 ++-
> > target/arm/kvm64.c | 6 ++-
> > target/i386/kvm/kvm.c | 6 ++-
> > target/mips/kvm.c | 6 ++-
> > target/ppc/kvm.c | 6 ++-
> > target/riscv/kvm/kvm-cpu.c | 79 ++++++++++++++++++++++++++++++++++++++
> > target/s390x/kvm/kvm.c | 6 ++-
> > 8 files changed, 107 insertions(+), 16 deletions(-)
> >
> > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
> > index e39a810a4e..ccc505d0c2 100644
> > --- a/accel/kvm/kvm-all.c
> > +++ b/accel/kvm/kvm-all.c
> > @@ -3231,7 +3231,7 @@ int kvm_insert_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> > bp = g_new(struct kvm_sw_breakpoint, 1);
> > bp->pc = addr;
> > bp->use_count = 1;
> > - err = kvm_arch_insert_sw_breakpoint(cpu, bp);
> > + err = kvm_arch_insert_sw_breakpoint(cpu, bp, len);
> > if (err) {
> > g_free(bp);
> > return err;
> > @@ -3270,7 +3270,7 @@ int kvm_remove_breakpoint(CPUState *cpu, int type, vaddr addr, vaddr len)
> > return 0;
> > }
> >
> > - err = kvm_arch_remove_sw_breakpoint(cpu, bp);
> > + err = kvm_arch_remove_sw_breakpoint(cpu, bp, len);
> > if (err) {
> > return err;
> > }
> > @@ -3300,10 +3300,10 @@ void kvm_remove_all_breakpoints(CPUState *cpu)
> > CPUState *tmpcpu;
> >
> > QTAILQ_FOREACH_SAFE(bp, &s->kvm_sw_breakpoints, entry, next) {
> > - if (kvm_arch_remove_sw_breakpoint(cpu, bp) != 0) {
> > + if (kvm_arch_remove_sw_breakpoint(cpu, bp, 0) != 0) {
> > /* Try harder to find a CPU that currently sees the breakpoint. */
> > CPU_FOREACH(tmpcpu) {
> > - if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp) == 0) {
> > + if (kvm_arch_remove_sw_breakpoint(tmpcpu, bp, 0) == 0) {
> > break;
> > }
> > }
> > diff --git a/include/sysemu/kvm.h b/include/sysemu/kvm.h
> > index d614878164..ab38c23def 100644
> > --- a/include/sysemu/kvm.h
> > +++ b/include/sysemu/kvm.h
> > @@ -391,9 +391,11 @@ struct kvm_sw_breakpoint *kvm_find_sw_breakpoint(CPUState *cpu,
> > int kvm_sw_breakpoints_active(CPUState *cpu);
> >
> > int kvm_arch_insert_sw_breakpoint(CPUState *cpu,
> > - struct kvm_sw_breakpoint *bp);
> > + struct kvm_sw_breakpoint *bp,
> > + vaddr len);
> > int kvm_arch_remove_sw_breakpoint(CPUState *cpu,
> > - struct kvm_sw_breakpoint *bp);
> > + struct kvm_sw_breakpoint *bp,
> > + vaddr len);
> > int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type);
> > int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type);
> > void kvm_arch_remove_all_hw_breakpoints(void);
> > diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> > index 3c175c93a7..023e92b577 100644
> > --- a/target/arm/kvm64.c
> > +++ b/target/arm/kvm64.c
> > @@ -1139,7 +1139,8 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> > /* C6.6.29 BRK instruction */
> > static const uint32_t brk_insn = 0xd4200000;
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > if (have_guest_debug) {
> > if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, 4, 0) ||
> > @@ -1153,7 +1154,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > }
> > }
> >
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > static uint32_t brk;
> >
> > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> > index 4ce80555b4..742b7c8296 100644
> > --- a/target/i386/kvm/kvm.c
> > +++ b/target/i386/kvm/kvm.c
> > @@ -4935,7 +4935,8 @@ static int kvm_handle_tpr_access(X86CPU *cpu)
> > return 1;
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > static const uint8_t int3 = 0xcc;
> >
> > @@ -4946,7 +4947,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > return 0;
> > }
> >
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > uint8_t int3;
> >
> > diff --git a/target/mips/kvm.c b/target/mips/kvm.c
> > index e22e24ed97..2f68938cdf 100644
> > --- a/target/mips/kvm.c
> > +++ b/target/mips/kvm.c
> > @@ -112,13 +112,15 @@ void kvm_mips_reset_vcpu(MIPSCPU *cpu)
> > DPRINTF("%s\n", __func__);
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > DPRINTF("%s\n", __func__);
> > return 0;
> > }
> >
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > DPRINTF("%s\n", __func__);
> > return 0;
> > diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> > index 9b1abe2fc4..a99c85b2f3 100644
> > --- a/target/ppc/kvm.c
> > +++ b/target/ppc/kvm.c
> > @@ -1375,7 +1375,8 @@ static int kvmppc_handle_dcr_write(CPUPPCState *env,
> > return 0;
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > /* Mixed endian case is not handled */
> > uint32_t sc = debug_inst_opcode;
> > @@ -1389,7 +1390,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > return 0;
> > }
> >
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > uint32_t sc;
> >
> > diff --git a/target/riscv/kvm/kvm-cpu.c b/target/riscv/kvm/kvm-cpu.c
> > index 45b6cf1cfa..e9110006b0 100644
> > --- a/target/riscv/kvm/kvm-cpu.c
> > +++ b/target/riscv/kvm/kvm-cpu.c
> > @@ -1521,3 +1521,82 @@ static const TypeInfo riscv_kvm_cpu_type_infos[] = {
> > };
> >
> > DEFINE_TYPES(riscv_kvm_cpu_type_infos)
> > +
> > +static const uint32_t ebreak_insn = 0x00100073;
> > +static const uint16_t c_ebreak_insn = 0x9002;
> > +
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > +{
> > + if (len != 4 && len != 2) {
> > + return -EINVAL;
> > + }
>
> I wonder if this verification should be moved to kvm_insert_breakpoint(). Is
> there any known reason why other archs would use 'len' other than 2 or 4? The
> parent function can throw the EINVAL in this case. Otherwise all callers from
> all archs will need a similar EINVAL check.
>
Thanks for the review. And you are right, we can move this check to the parent
function, to avoid the duplicated codes.
But since I'm not pretty sure whether it will break other archs, so I chose to
do this only for RISC-V. Keep other archs as they are.
As you mentioned, maybe Paolo could comment on this also.
> > +
> > + uint8_t * insn = (len == 4) ? (uint8_t *)&ebreak_insn :
> > + (uint8_t *)&c_ebreak_insn;
> > +
> > + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn, len, 0) ||
> > + cpu_memory_rw_debug(cs, bp->pc, insn, len, 1)) {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > +{
> > + uint8_t length;
> > +
> > + if (len == 4 || len == 2) {
> > + length = (uint8_t)len;
>
> Same question as above - perhaps the len = 0 | 2 | 4 conditional can be moved to
> kvm_remove_breakpoint().
>
Same as above.
Thanks,
Chao
>
> Thanks,
>
>
> Daniel
>
>
> > + } else if (len == 0) {
> > + /* Need to decide the instruction length in this case. */
> > + uint32_t read_4_bytes;
> > + uint16_t read_2_bytes;
> > +
> > + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_4_bytes, 4, 0) ||
> > + cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&read_2_bytes, 2, 0)) {
> > + return -EINVAL;
> > + }
> > +
> > + if (read_4_bytes == ebreak_insn) {
> > + length = 4;
> > + } else if (read_2_bytes == c_ebreak_insn) {
> > + length = 2;
> > + } else {
> > + return -EINVAL;
> > + }
> > + } else {
> > + return -EINVAL;
> > + }
> > +
> > + if (cpu_memory_rw_debug(cs, bp->pc, (uint8_t *)&bp->saved_insn,
> > + length, 1)) {
> > + return -EINVAL;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int kvm_arch_insert_hw_breakpoint(vaddr addr, vaddr len, int type)
> > +{
> > + /* TODO; To be implemented later. */
> > + return -EINVAL;
> > +}
> > +
> > +int kvm_arch_remove_hw_breakpoint(vaddr addr, vaddr len, int type)
> > +{
> > + /* TODO; To be implemented later. */
> > + return -EINVAL;
> > +}
> > +
> > +void kvm_arch_remove_all_hw_breakpoints(void)
> > +{
> > + /* TODO; To be implemented later. */
> > +}
> > +
> > +void kvm_arch_update_guest_debug(CPUState *cs, struct kvm_guest_debug *dbg)
> > +{
> > + /* TODO; To be implemented later. */
> > +}
> > diff --git a/target/s390x/kvm/kvm.c b/target/s390x/kvm/kvm.c
> > index 33ab3551f4..fafacedd6a 100644
> > --- a/target/s390x/kvm/kvm.c
> > +++ b/target/s390x/kvm/kvm.c
> > @@ -867,7 +867,8 @@ static void determine_sw_breakpoint_instr(void)
> > }
> > }
> >
> > -int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > determine_sw_breakpoint_instr();
> >
> > @@ -879,7 +880,8 @@ int kvm_arch_insert_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > return 0;
> > }
> >
> > -int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp)
> > +int kvm_arch_remove_sw_breakpoint(CPUState *cs, struct kvm_sw_breakpoint *bp,
> > + vaddr len)
> > {
> > uint8_t t[MAX_ILEN];
> >
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V
2024-04-16 9:25 ` Daniel Henrique Barboza
@ 2024-05-20 10:22 ` Chao Du
0 siblings, 0 replies; 12+ messages in thread
From: Chao Du @ 2024-05-20 10:22 UTC (permalink / raw)
To: pbonzini, Daniel Henrique Barboza
Cc: qemu-devel, qemu-riscv, alistair23, bin.meng, liweiwei,
zhiwei_liu, palmer, anup, atishp
Hi Paolo,
As Daniel suggested, could you please take a look at this series ?
Thanks,
Chao
On 2024-04-16 17:25, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
>
> On 4/9/24 06:43, Chao Du wrote:
> > Hi Daniel and all,
> >
> > The KVM patches have been reviewd and are in the queue.
> > https://lore.kernel.org/all/20240402062628.5425-1-duchao@eswincomputing.com/
> >
> > Could you please review in the QEMU side ?
> > Then I will rebase this series with your comments.
> >
> > Some Notes:
> > 1. As the first stage, only the software breakpoints is implemented.
> > 2. A 'corner case' in which the debug exception is not inserted by the
> > debugger, need to be re-injected to the guest. This is not handled yet
> > in this series.
>
> Aside from the comments I made in patch 1 w.r.t checks that (perhaps) can be moved
> to kvm-all.c, it looks good to me.
>
> Since you're changing kvm-all.c we'll need Paolo to ack the changes in patch 1, so
> feel free to wait for him to take a look before sending v2.
>
>
> Thanks,
>
>
> Daniel
>
> >
> > Thanks,
> > Chao
> >
> >
> > On 2023-12-22 22:16, Daniel Henrique Barboza <dbarboza@ventanamicro.com> wrote:
> >>
> >> Hi,
> >>
> >> It seems that we still need the kernel KVM side to be sorted out first [1],
> >> so I believe we should wait a bit until we can review this RFC. Otherwise we
> >> might risk reviewing something that has to be changed later.
> >>
> >>
> >> [1] https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com/
> >>
> >>
> >> Thanks,
> >>
> >> Daniel
> >>
> >> On 12/21/23 06:49, Chao Du wrote:
> >>> This series implements QEMU KVM Guest Debug on RISC-V. Currently, we can
> >>> debug RISC-V KVM guest from the host side, with software breakpoints.
> >>>
> >>> A brief test was done on QEMU RISC-V hypervisor emulator.
> >>>
> >>> A TODO list which will be added later:
> >>> 1. HW breakpoints support
> >>> 2. Test cases
> >>>
> >>> This series is based on QEMU 8.2.0-rc4 and is also available at:
> >>> https://github.com/Du-Chao/qemu/tree/riscv_gd_sw
> >>>
> >>> This is dependent on KVM side changes:
> >>> https://github.com/Du-Chao/linux/tree/riscv_gd_sw
> >>>
> >>> Chao Du (4):
> >>> target/riscv/kvm: add software breakpoints support
> >>> target/riscv/kvm: implement kvm_arch_update_guest_debug()
> >>> target/riscv/kvm: handle the exit with debug reason
> >>> linux-headers: enable KVM GUEST DEBUG for RISC-V
> >>>
> >>> accel/kvm/kvm-all.c | 8 +--
> >>> include/sysemu/kvm.h | 6 +-
> >>> linux-headers/asm-riscv/kvm.h | 1 +
> >>> target/arm/kvm64.c | 6 +-
> >>> target/i386/kvm/kvm.c | 6 +-
> >>> target/mips/kvm.c | 6 +-
> >>> target/ppc/kvm.c | 6 +-
> >>> target/riscv/kvm/kvm-cpu.c | 101 ++++++++++++++++++++++++++++++++++
> >>> target/s390x/kvm/kvm.c | 6 +-
> >>> 9 files changed, 130 insertions(+), 16 deletions(-)
> >>>
> >>> --
> >>> 2.17.1
> >>>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2024-05-20 10:25 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-21 9:49 [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Chao Du
2023-12-21 9:49 ` [RFC PATCH 1/4] target/riscv/kvm: add software breakpoints support Chao Du
2024-04-16 9:23 ` Daniel Henrique Barboza
2024-04-18 9:46 ` Chao Du
2023-12-21 9:49 ` [RFC PATCH 2/4] target/riscv/kvm: implement kvm_arch_update_guest_debug() Chao Du
2023-12-21 9:49 ` [RFC PATCH 3/4] target/riscv/kvm: handle the exit with debug reason Chao Du
2023-12-21 9:49 ` [RFC PATCH 4/4] linux-headers: enable KVM GUEST DEBUG for RISC-V Chao Du
2023-12-22 14:16 ` [RFC PATCH 0/4] target/riscv/kvm: QEMU support for KVM Guest Debug on RISC-V Daniel Henrique Barboza
2024-04-09 9:43 ` Chao Du
2024-04-15 1:47 ` Chao Du
2024-04-16 9:25 ` Daniel Henrique Barboza
2024-05-20 10:22 ` Chao Du
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.