[PATCH v2 0/3] RISC-V: KVM: Guest Debug Support - Software Breakpoint Part

[PATCH v2 0/3] RISC-V: KVM: Guest Debug Support - Software Breakpoint Part

[Chao Du]

This series implements the “KVM Guset Debug” feature on RISC-V. This is
an existing feature which is already supported by some other arches.
It allows us to debug a RISC-V KVM guest from GDB in host side.

As the first stage, the software breakpoints (ebreak instruction) is
implemented. HW breakpoints support will come later after a synthetically
consideration with the SBI debug trigger extension.

A selftest case was added in this series. Manual test was done on QEMU
RISC-V hypervisor emulator. (add '-s' to enable the gdbserver in QEMU)

This series is based on Linux 6.8-rc6 and also available at:
https://github.com/Du-Chao/kvm-riscv/tree/guest_debug_sw_v2

The matched QEMU is available at:
https://github.com/Du-Chao/qemu/tree/riscv_gd_sw

Please note that if Paolo's change
https://lore.kernel.org/kvm/20240131233056.10845-8-pbonzini@redhat.com/
is adopted, then we can keep 'arch/riscv/include/uapi/asm/kvm.h'
untouched.

Changes from v1->v2:
- Rebased on Linux 6.8-rc6.
- Maintain a hedeleg in "struct kvm_vcpu_config" for each VCPU.
- Update the HEDELEG csr in kvm_arch_vcpu_load().

Changes from RFC->v1:
- Rebased on Linux 6.8-rc2.
- Merge PATCH1 and PATCH2 into one patch.
- kselftest case added.

v1 link:
https://lore.kernel.org/kvm/20240206074931.22930-1-duchao@eswincomputing.com
RFC link:
https://lore.kernel.org/kvm/20231221095002.7404-1-duchao@eswincomputing.com

Chao Du (3):
  RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()
  RISC-V: KVM: Handle breakpoint exits for VCPU
  RISC-V: KVM: selftests: Add breakpoints test support

 arch/riscv/include/asm/kvm_host.h             | 17 +++++++
 arch/riscv/include/uapi/asm/kvm.h             |  1 +
 arch/riscv/kvm/main.c                         | 18 +------
 arch/riscv/kvm/vcpu.c                         | 15 +++++-
 arch/riscv/kvm/vcpu_exit.c                    |  4 ++
 arch/riscv/kvm/vm.c                           |  1 +
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../testing/selftests/kvm/riscv/breakpoints.c | 49 +++++++++++++++++++
 8 files changed, 88 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c

--
2.17.1

[PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
been checked.

kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
from userspace accordingly. Route the breakpoint exceptions to HS mode
if the VCPU is being debugged by userspace, by clearing the
corresponding bit in hedeleg. Write the actual CSR in
kvm_arch_vcpu_load().

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
 arch/riscv/include/uapi/asm/kvm.h |  1 +
 arch/riscv/kvm/main.c             | 18 ++----------------
 arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
 arch/riscv/kvm/vm.c               |  1 +
 5 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
index 484d04a92fa6..9ee3f03ba5d1 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -43,6 +43,22 @@
 	KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
 #define KVM_REQ_STEAL_UPDATE		KVM_ARCH_REQ(6)
 
+#define KVM_HEDELEG_DEFAULT		((_AC(1, UL) << EXC_INST_MISALIGNED) | \
+					 (_AC(1, UL) << EXC_BREAKPOINT) | \
+					 (_AC(1, UL) << EXC_SYSCALL) | \
+					 (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
+					 (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
+					 (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
+#define KVM_HEDELEG_GUEST_DEBUG		((_AC(1, UL) << EXC_INST_MISALIGNED) | \
+					 (_AC(1, UL) << EXC_SYSCALL) | \
+					 (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
+					 (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
+					 (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
+
+#define KVM_HIDELEG_DEFAULT		((_AC(1, UL) << IRQ_VS_SOFT) | \
+					 (_AC(1, UL) << IRQ_VS_TIMER) | \
+					 (_AC(1, UL) << IRQ_VS_EXT))
+
 enum kvm_riscv_hfence_type {
 	KVM_RISCV_HFENCE_UNKNOWN = 0,
 	KVM_RISCV_HFENCE_GVMA_VMID_GPA,
@@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
 struct kvm_vcpu_config {
 	u64 henvcfg;
 	u64 hstateen0;
+	unsigned long hedeleg;
 };
 
 struct kvm_vcpu_smstateen_csr {
diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index 7499e88a947c..39f4f4b9dede 100644
--- a/arch/riscv/include/uapi/asm/kvm.h
+++ b/arch/riscv/include/uapi/asm/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
 
diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
index 225a435d9c9a..bab2ec34cd87 100644
--- a/arch/riscv/kvm/main.c
+++ b/arch/riscv/kvm/main.c
@@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
 
 int kvm_arch_hardware_enable(void)
 {
-	unsigned long hideleg, hedeleg;
-
-	hedeleg = 0;
-	hedeleg |= (1UL << EXC_INST_MISALIGNED);
-	hedeleg |= (1UL << EXC_BREAKPOINT);
-	hedeleg |= (1UL << EXC_SYSCALL);
-	hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
-	hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
-	hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
-	csr_write(CSR_HEDELEG, hedeleg);
-
-	hideleg = 0;
-	hideleg |= (1UL << IRQ_VS_SOFT);
-	hideleg |= (1UL << IRQ_VS_TIMER);
-	hideleg |= (1UL << IRQ_VS_EXT);
-	csr_write(CSR_HIDELEG, hideleg);
+	csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
+	csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
 
 	/* VS should access only the time counter directly. Everything else should trap */
 	csr_write(CSR_HCOUNTEREN, 0x02);
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..242076c2227f 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
 int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 					struct kvm_guest_debug *dbg)
 {
-	/* TODO; To be implemented later. */
-	return -EINVAL;
+	if (dbg->control & KVM_GUESTDBG_ENABLE) {
+		vcpu->guest_debug = dbg->control;
+		vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
+	} else {
+		vcpu->guest_debug = 0;
+		vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
+	}
+
+	return 0;
 }
 
 static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
@@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
 		if (riscv_isa_extension_available(isa, SMSTATEEN))
 			cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
 	}
+
+	if (!vcpu->guest_debug)
+		cfg->hedeleg = KVM_HEDELEG_DEFAULT;
 }
 
 void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
@@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
 	csr_write(CSR_VSEPC, csr->vsepc);
 	csr_write(CSR_VSCAUSE, csr->vscause);
 	csr_write(CSR_VSTVAL, csr->vstval);
+	csr_write(CSR_HEDELEG, cfg->hedeleg);
 	csr_write(CSR_HVIP, csr->hvip);
 	csr_write(CSR_VSATP, csr->vsatp);
 	csr_write(CSR_HENVCFG, cfg->henvcfg);
diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
index ce58bc48e5b8..7396b8654f45 100644
--- a/arch/riscv/kvm/vm.c
+++ b/arch/riscv/kvm/vm.c
@@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
 	case KVM_CAP_READONLY_MEM:
 	case KVM_CAP_MP_STATE:
 	case KVM_CAP_IMMEDIATE_EXIT:
+	case KVM_CAP_SET_GUEST_DEBUG:
 		r = 1;
 		break;
 	case KVM_CAP_NR_VCPUS:
-- 
2.17.1

[PATCH v2 2/3] RISC-V: KVM: Handle breakpoint exits for VCPU

[Chao Du]

Exit to userspace for breakpoint traps. Set the exit_reason as
KVM_EXIT_DEBUG before exit.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 arch/riscv/kvm/vcpu_exit.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c
index 2415722c01b8..5761f95abb60 100644
--- a/arch/riscv/kvm/vcpu_exit.c
+++ b/arch/riscv/kvm/vcpu_exit.c
@@ -204,6 +204,10 @@ int kvm_riscv_vcpu_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 		if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
 			ret = kvm_riscv_vcpu_sbi_ecall(vcpu, run);
 		break;
+	case EXC_BREAKPOINT:
+		run->exit_reason = KVM_EXIT_DEBUG;
+		ret = 0;
+		break;
 	default:
 		break;
 	}
-- 
2.17.1

[PATCH v2 3/3] RISC-V: KVM: selftests: Add breakpoints test support

[Chao Du]

Initial support for RISC-V KVM breakpoint test. Check the exit reason
and the PC when guest debug is enabled.

Signed-off-by: Chao Du <duchao@eswincomputing.com>
---
 tools/testing/selftests/kvm/Makefile          |  1 +
 .../testing/selftests/kvm/riscv/breakpoints.c | 49 +++++++++++++++++++
 2 files changed, 50 insertions(+)
 create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c

diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
index 492e937fab00..5f9048a740b0 100644
--- a/tools/testing/selftests/kvm/Makefile
+++ b/tools/testing/selftests/kvm/Makefile
@@ -184,6 +184,7 @@ TEST_GEN_PROGS_s390x += rseq_test
 TEST_GEN_PROGS_s390x += set_memory_region_test
 TEST_GEN_PROGS_s390x += kvm_binary_stats_test
 
+TEST_GEN_PROGS_riscv += riscv/breakpoints
 TEST_GEN_PROGS_riscv += demand_paging_test
 TEST_GEN_PROGS_riscv += dirty_log_test
 TEST_GEN_PROGS_riscv += get-reg-list
diff --git a/tools/testing/selftests/kvm/riscv/breakpoints.c b/tools/testing/selftests/kvm/riscv/breakpoints.c
new file mode 100644
index 000000000000..be2d94837c83
--- /dev/null
+++ b/tools/testing/selftests/kvm/riscv/breakpoints.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * RISC-V KVM breakpoint tests.
+ *
+ * Copyright 2024 Beijing ESWIN Computing Technology Co., Ltd.
+ *
+ */
+#include "kvm_util.h"
+
+#define PC(v) ((uint64_t)&(v))
+
+extern unsigned char sw_bp;
+
+static void guest_code(void)
+{
+	asm volatile("sw_bp: ebreak");
+	asm volatile("nop");
+	asm volatile("nop");
+	asm volatile("nop");
+
+	GUEST_DONE();
+}
+
+int main(void)
+{
+	struct kvm_vm *vm;
+	struct kvm_vcpu *vcpu;
+	struct kvm_guest_debug debug;
+	uint64_t pc;
+
+	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SET_GUEST_DEBUG));
+
+	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
+
+	memset(&debug, 0, sizeof(debug));
+	debug.control = KVM_GUESTDBG_ENABLE;
+	vcpu_guest_debug_set(vcpu, &debug);
+	vcpu_run(vcpu);
+
+	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
+
+	vcpu_get_reg(vcpu, RISCV_CORE_REG(regs.pc), &pc);
+
+	TEST_ASSERT_EQ(pc, PC(sw_bp));
+
+	kvm_vm_free(vm);
+
+	return 0;
+}
-- 
2.17.1

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
>
> kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> been checked.
>
> kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> from userspace accordingly. Route the breakpoint exceptions to HS mode
> if the VCPU is being debugged by userspace, by clearing the
> corresponding bit in hedeleg. Write the actual CSR in
> kvm_arch_vcpu_load().
>
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
>  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
>  arch/riscv/include/uapi/asm/kvm.h |  1 +
>  arch/riscv/kvm/main.c             | 18 ++----------------
>  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
>  arch/riscv/kvm/vm.c               |  1 +
>  5 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> index 484d04a92fa6..9ee3f03ba5d1 100644
> --- a/arch/riscv/include/asm/kvm_host.h
> +++ b/arch/riscv/include/asm/kvm_host.h
> @@ -43,6 +43,22 @@
>         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
>  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
>
> +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))

Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)

Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.

> +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))

No need for KVM_HEDELEG_GUEST_DEBUG, see below.

> +
> +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> +                                        (_AC(1, UL) << IRQ_VS_EXT))
> +

Same as above, use BIT(xyz) here.

>  enum kvm_riscv_hfence_type {
>         KVM_RISCV_HFENCE_UNKNOWN = 0,
>         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
>  struct kvm_vcpu_config {
>         u64 henvcfg;
>         u64 hstateen0;
> +       unsigned long hedeleg;
>  };
>
>  struct kvm_vcpu_smstateen_csr {
> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index 7499e88a947c..39f4f4b9dede 100644
> --- a/arch/riscv/include/uapi/asm/kvm.h
> +++ b/arch/riscv/include/uapi/asm/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
>
> diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> index 225a435d9c9a..bab2ec34cd87 100644
> --- a/arch/riscv/kvm/main.c
> +++ b/arch/riscv/kvm/main.c
> @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
>
>  int kvm_arch_hardware_enable(void)
>  {
> -       unsigned long hideleg, hedeleg;
> -
> -       hedeleg = 0;
> -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> -       hedeleg |= (1UL << EXC_BREAKPOINT);
> -       hedeleg |= (1UL << EXC_SYSCALL);
> -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> -       csr_write(CSR_HEDELEG, hedeleg);
> -
> -       hideleg = 0;
> -       hideleg |= (1UL << IRQ_VS_SOFT);
> -       hideleg |= (1UL << IRQ_VS_TIMER);
> -       hideleg |= (1UL << IRQ_VS_EXT);
> -       csr_write(CSR_HIDELEG, hideleg);
> +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
>
>         /* VS should access only the time counter directly. Everything else should trap */
>         csr_write(CSR_HCOUNTEREN, 0x02);
> diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> index b5ca9f2e98ac..242076c2227f 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
>  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>                                         struct kvm_guest_debug *dbg)
>  {
> -       /* TODO; To be implemented later. */
> -       return -EINVAL;

if (vcpu->arch.ran_atleast_once)
        return -EBUSY;


> +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> +               vcpu->guest_debug = dbg->control;
> +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> +       } else {
> +               vcpu->guest_debug = 0;
> +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> +       }

Don't update vcpu->arch.cfg.hedeleg here since it should be only done
in kvm_riscv_vcpu_setup_config().

> +
> +       return 0;
>  }
>
>  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
>                 if (riscv_isa_extension_available(isa, SMSTATEEN))
>                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
>         }
> +
> +       if (!vcpu->guest_debug)
> +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;

This should be:

cfg->hedeleg = KVM_HEDELEG_DEFAULT;
if (vcpu->guest_debug)
        cfg->hedeleg |= BIT(EXC_BREAKPOINT);

>  }
>
>  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>         csr_write(CSR_VSEPC, csr->vsepc);
>         csr_write(CSR_VSCAUSE, csr->vscause);
>         csr_write(CSR_VSTVAL, csr->vstval);
> +       csr_write(CSR_HEDELEG, cfg->hedeleg);
>         csr_write(CSR_HVIP, csr->hvip);
>         csr_write(CSR_VSATP, csr->vsatp);
>         csr_write(CSR_HENVCFG, cfg->henvcfg);
> diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> index ce58bc48e5b8..7396b8654f45 100644
> --- a/arch/riscv/kvm/vm.c
> +++ b/arch/riscv/kvm/vm.c
> @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
>         case KVM_CAP_READONLY_MEM:
>         case KVM_CAP_MP_STATE:
>         case KVM_CAP_IMMEDIATE_EXIT:
> +       case KVM_CAP_SET_GUEST_DEBUG:
>                 r = 1;
>                 break;
>         case KVM_CAP_NR_VCPUS:
> --
> 2.17.1
>

Regards,
Anup

Re: [PATCH v2 2/3] RISC-V: KVM: Handle breakpoint exits for VCPU

[Anup Patel]

On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
>
> Exit to userspace for breakpoint traps. Set the exit_reason as
> KVM_EXIT_DEBUG before exit.
>
> Signed-off-by: Chao Du <duchao@eswincomputing.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Thanks,
Anup

> ---
>  arch/riscv/kvm/vcpu_exit.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/riscv/kvm/vcpu_exit.c b/arch/riscv/kvm/vcpu_exit.c
> index 2415722c01b8..5761f95abb60 100644
> --- a/arch/riscv/kvm/vcpu_exit.c
> +++ b/arch/riscv/kvm/vcpu_exit.c
> @@ -204,6 +204,10 @@ int kvm_riscv_vcpu_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
>                 if (vcpu->arch.guest_context.hstatus & HSTATUS_SPV)
>                         ret = kvm_riscv_vcpu_sbi_ecall(vcpu, run);
>                 break;
> +       case EXC_BREAKPOINT:
> +               run->exit_reason = KVM_EXIT_DEBUG;
> +               ret = 0;
> +               break;
>         default:
>                 break;
>         }
> --
> 2.17.1
>

Re: [PATCH v2 3/3] RISC-V: KVM: selftests: Add breakpoints test support

[Anup Patel]

This is actually a ebreak test so subject should be:
"RISC-V: KVM: selftests: Add ebreak test support"

On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
>
> Initial support for RISC-V KVM breakpoint test. Check the exit reason
> and the PC when guest debug is enabled.

s/breakpoint/ebreak/

>
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../testing/selftests/kvm/riscv/breakpoints.c | 49 +++++++++++++++++++

Rename riscv/breakpoints.c to riscv/ebreak_test.c

>  2 files changed, 50 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c
>
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 492e937fab00..5f9048a740b0 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -184,6 +184,7 @@ TEST_GEN_PROGS_s390x += rseq_test
>  TEST_GEN_PROGS_s390x += set_memory_region_test
>  TEST_GEN_PROGS_s390x += kvm_binary_stats_test
>
> +TEST_GEN_PROGS_riscv += riscv/breakpoints
>  TEST_GEN_PROGS_riscv += demand_paging_test
>  TEST_GEN_PROGS_riscv += dirty_log_test
>  TEST_GEN_PROGS_riscv += get-reg-list
> diff --git a/tools/testing/selftests/kvm/riscv/breakpoints.c b/tools/testing/selftests/kvm/riscv/breakpoints.c
> new file mode 100644
> index 000000000000..be2d94837c83
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/riscv/breakpoints.c
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RISC-V KVM breakpoint tests.

s/breakpoint tests/ebreak test/

> + *
> + * Copyright 2024 Beijing ESWIN Computing Technology Co., Ltd.
> + *
> + */
> +#include "kvm_util.h"
> +
> +#define PC(v) ((uint64_t)&(v))
> +
> +extern unsigned char sw_bp;
> +
> +static void guest_code(void)
> +{
> +       asm volatile("sw_bp: ebreak");
> +       asm volatile("nop");
> +       asm volatile("nop");
> +       asm volatile("nop");
> +
> +       GUEST_DONE();
> +}
> +
> +int main(void)
> +{
> +       struct kvm_vm *vm;
> +       struct kvm_vcpu *vcpu;
> +       struct kvm_guest_debug debug;
> +       uint64_t pc;
> +
> +       TEST_REQUIRE(kvm_has_cap(KVM_CAP_SET_GUEST_DEBUG));
> +
> +       vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +
> +       memset(&debug, 0, sizeof(debug));
> +       debug.control = KVM_GUESTDBG_ENABLE;
> +       vcpu_guest_debug_set(vcpu, &debug);
> +       vcpu_run(vcpu);
> +
> +       TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);

This is only testing the case where KVM_GUESTDBG_ENABLE is set.

This test should be enhanced to:
1) First test "ebreak" executed by Guest when KVM_GUESTDBG_ENABLE
    is not set. Here, we would expect main() to receive KVM_EXIT_DEBUG.
2) Second test "ebreak" executed by Guest when KVM_GUESTDBG_ENABLE
   is set. Here, we would expect main() will not receive KVM_EXIT_DEBUG.

> +
> +       vcpu_get_reg(vcpu, RISCV_CORE_REG(regs.pc), &pc);
> +
> +       TEST_ASSERT_EQ(pc, PC(sw_bp));
> +
> +       kvm_vm_free(vm);
> +
> +       return 0;
> +}
> --
> 2.17.1
>

Regards,
Anup

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> 
> On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > been checked.
> >
> > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > if the VCPU is being debugged by userspace, by clearing the
> > corresponding bit in hedeleg. Write the actual CSR in
> > kvm_arch_vcpu_load().
> >
> > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > ---
> >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> >  arch/riscv/kvm/main.c             | 18 ++----------------
> >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> >  arch/riscv/kvm/vm.c               |  1 +
> >  5 files changed, 34 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > index 484d04a92fa6..9ee3f03ba5d1 100644
> > --- a/arch/riscv/include/asm/kvm_host.h
> > +++ b/arch/riscv/include/asm/kvm_host.h
> > @@ -43,6 +43,22 @@
> >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> >
> > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> 
> Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)

Thanks, I will use BIT() instead in next revision.

> 

> Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.

I think the bit EXC_BREAKPOINT should be set by default, like what you
already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
and handle it accordingly.

If the guest_debug is enabled, ebreak instructions are inserted by the
userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
Bit EXC_BREAKPOINT should be cleared in this case.

> 
> > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> 
> No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> 
> > +
> > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > +
> 
> Same as above, use BIT(xyz) here.
> 
> >  enum kvm_riscv_hfence_type {
> >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> >  struct kvm_vcpu_config {
> >         u64 henvcfg;
> >         u64 hstateen0;
> > +       unsigned long hedeleg;
> >  };
> >
> >  struct kvm_vcpu_smstateen_csr {
> > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > index 7499e88a947c..39f4f4b9dede 100644
> > --- a/arch/riscv/include/uapi/asm/kvm.h
> > +++ b/arch/riscv/include/uapi/asm/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
> >
> > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > index 225a435d9c9a..bab2ec34cd87 100644
> > --- a/arch/riscv/kvm/main.c
> > +++ b/arch/riscv/kvm/main.c
> > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> >
> >  int kvm_arch_hardware_enable(void)
> >  {
> > -       unsigned long hideleg, hedeleg;
> > -
> > -       hedeleg = 0;
> > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > -       hedeleg |= (1UL << EXC_SYSCALL);
> > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > -       csr_write(CSR_HEDELEG, hedeleg);
> > -
> > -       hideleg = 0;
> > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > -       hideleg |= (1UL << IRQ_VS_EXT);
> > -       csr_write(CSR_HIDELEG, hideleg);
> > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> >
> >         /* VS should access only the time counter directly. Everything else should trap */
> >         csr_write(CSR_HCOUNTEREN, 0x02);
> > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > index b5ca9f2e98ac..242076c2227f 100644
> > --- a/arch/riscv/kvm/vcpu.c
> > +++ b/arch/riscv/kvm/vcpu.c
> > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >                                         struct kvm_guest_debug *dbg)
> >  {
> > -       /* TODO; To be implemented later. */
> > -       return -EINVAL;
> 
> if (vcpu->arch.ran_atleast_once)
>         return -EBUSY;

If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
will come before the first KVM_RUN. This will always cause an ERROR.

> 
> 
> > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > +               vcpu->guest_debug = dbg->control;
> > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > +       } else {
> > +               vcpu->guest_debug = 0;
> > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > +       }
> 
> Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> in kvm_riscv_vcpu_setup_config().
> 
> > +
> > +       return 0;
> >  }
> >
> >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> >         }
> > +
> > +       if (!vcpu->guest_debug)
> > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> 
> This should be:
> 
> cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> if (vcpu->guest_debug)
>         cfg->hedeleg |= BIT(EXC_BREAKPOINT);

Like above, here the logic should be:

cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
if (vcpu->guest_debug)
        cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);

Another approach is:
initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().

> 
> >  }
> >
> >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> >         csr_write(CSR_VSEPC, csr->vsepc);
> >         csr_write(CSR_VSCAUSE, csr->vscause);
> >         csr_write(CSR_VSTVAL, csr->vstval);
> > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> >         csr_write(CSR_HVIP, csr->hvip);
> >         csr_write(CSR_VSATP, csr->vsatp);
> >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > index ce58bc48e5b8..7396b8654f45 100644
> > --- a/arch/riscv/kvm/vm.c
> > +++ b/arch/riscv/kvm/vm.c
> > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> >         case KVM_CAP_READONLY_MEM:
> >         case KVM_CAP_MP_STATE:
> >         case KVM_CAP_IMMEDIATE_EXIT:
> > +       case KVM_CAP_SET_GUEST_DEBUG:
> >                 r = 1;
> >                 break;
> >         case KVM_CAP_NR_VCPUS:
> > --
> > 2.17.1
> >
> 
> Regards,
> Anup

Thanks,
Chao

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
>
> On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > been checked.
> > >
> > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > if the VCPU is being debugged by userspace, by clearing the
> > > corresponding bit in hedeleg. Write the actual CSR in
> > > kvm_arch_vcpu_load().
> > >
> > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > ---
> > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > >  arch/riscv/kvm/vm.c               |  1 +
> > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > --- a/arch/riscv/include/asm/kvm_host.h
> > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > @@ -43,6 +43,22 @@
> > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > >
> > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> >
> > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
>
> Thanks, I will use BIT() instead in next revision.
>
> >
>
> > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
>
> I think the bit EXC_BREAKPOINT should be set by default, like what you
> already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> and handle it accordingly.
>
> If the guest_debug is enabled, ebreak instructions are inserted by the
> userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> Bit EXC_BREAKPOINT should be cleared in this case.

If EXC_BREAKPOINT is delegated by default then it is not consistent with
vcpu->guest_debug which is not enabled by default.

>
> >
> > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> >
> > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> >
> > > +
> > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > +
> >
> > Same as above, use BIT(xyz) here.
> >
> > >  enum kvm_riscv_hfence_type {
> > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > >  struct kvm_vcpu_config {
> > >         u64 henvcfg;
> > >         u64 hstateen0;
> > > +       unsigned long hedeleg;
> > >  };
> > >
> > >  struct kvm_vcpu_smstateen_csr {
> > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > index 7499e88a947c..39f4f4b9dede 100644
> > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > +++ b/arch/riscv/include/uapi/asm/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
> > >
> > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > index 225a435d9c9a..bab2ec34cd87 100644
> > > --- a/arch/riscv/kvm/main.c
> > > +++ b/arch/riscv/kvm/main.c
> > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > >
> > >  int kvm_arch_hardware_enable(void)
> > >  {
> > > -       unsigned long hideleg, hedeleg;
> > > -
> > > -       hedeleg = 0;
> > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > -
> > > -       hideleg = 0;
> > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > -       csr_write(CSR_HIDELEG, hideleg);
> > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > >
> > >         /* VS should access only the time counter directly. Everything else should trap */
> > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > index b5ca9f2e98ac..242076c2227f 100644
> > > --- a/arch/riscv/kvm/vcpu.c
> > > +++ b/arch/riscv/kvm/vcpu.c
> > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > >                                         struct kvm_guest_debug *dbg)
> > >  {
> > > -       /* TODO; To be implemented later. */
> > > -       return -EINVAL;
> >
> > if (vcpu->arch.ran_atleast_once)
> >         return -EBUSY;
>
> If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> will come before the first KVM_RUN. This will always cause an ERROR.

The check ensures that KVM user space can only enable/disable
guest debug before the VCPU is run. I don't see why this would
fail for QEMU.

>
> >
> >
> > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > +               vcpu->guest_debug = dbg->control;
> > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > +       } else {
> > > +               vcpu->guest_debug = 0;
> > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > +       }
> >
> > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > in kvm_riscv_vcpu_setup_config().
> >
> > > +
> > > +       return 0;
> > >  }
> > >
> > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > >         }
> > > +
> > > +       if (!vcpu->guest_debug)
> > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> >
> > This should be:
> >
> > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > if (vcpu->guest_debug)
> >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
>
> Like above, here the logic should be:
>
> cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> if (vcpu->guest_debug)
>         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
>
> Another approach is:
> initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().

I disagree. We should handle hedeleg just like we handle henvcfg.

>
> >
> > >  }
> > >
> > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > >         csr_write(CSR_VSEPC, csr->vsepc);
> > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > >         csr_write(CSR_HVIP, csr->hvip);
> > >         csr_write(CSR_VSATP, csr->vsatp);
> > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > index ce58bc48e5b8..7396b8654f45 100644
> > > --- a/arch/riscv/kvm/vm.c
> > > +++ b/arch/riscv/kvm/vm.c
> > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > >         case KVM_CAP_READONLY_MEM:
> > >         case KVM_CAP_MP_STATE:
> > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > >                 r = 1;
> > >                 break;
> > >         case KVM_CAP_NR_VCPUS:
> > > --
> > > 2.17.1
> > >
> >
> > Regards,
> > Anup
>
> Thanks,
> Chao

Regards,
Anup

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> 
> On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > >
> > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > been checked.
> > > >
> > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > if the VCPU is being debugged by userspace, by clearing the
> > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > kvm_arch_vcpu_load().
> > > >
> > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > ---
> > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > >  arch/riscv/kvm/vm.c               |  1 +
> > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > @@ -43,6 +43,22 @@
> > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > >
> > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > >
> > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> >
> > Thanks, I will use BIT() instead in next revision.
> >
> > >
> >
> > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> >
> > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > and handle it accordingly.
> >
> > If the guest_debug is enabled, ebreak instructions are inserted by the
> > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > Bit EXC_BREAKPOINT should be cleared in this case.
> 
> If EXC_BREAKPOINT is delegated by default then it is not consistent with
> vcpu->guest_debug which is not enabled by default.

To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
They are somehow 'opposite'.

This 'kvm_guest_debug' feature is different from "debug in the guest".
The later requires the delegation of EXC_BREAKPOINT.
The former does not.

> 
> >
> > >
> > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > >
> > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > >
> > > > +
> > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > +
> > >
> > > Same as above, use BIT(xyz) here.
> > >
> > > >  enum kvm_riscv_hfence_type {
> > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > >  struct kvm_vcpu_config {
> > > >         u64 henvcfg;
> > > >         u64 hstateen0;
> > > > +       unsigned long hedeleg;
> > > >  };
> > > >
> > > >  struct kvm_vcpu_smstateen_csr {
> > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > +++ b/arch/riscv/include/uapi/asm/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
> > > >
> > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > --- a/arch/riscv/kvm/main.c
> > > > +++ b/arch/riscv/kvm/main.c
> > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > >
> > > >  int kvm_arch_hardware_enable(void)
> > > >  {
> > > > -       unsigned long hideleg, hedeleg;
> > > > -
> > > > -       hedeleg = 0;
> > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > -
> > > > -       hideleg = 0;
> > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > >
> > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > --- a/arch/riscv/kvm/vcpu.c
> > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > >                                         struct kvm_guest_debug *dbg)
> > > >  {
> > > > -       /* TODO; To be implemented later. */
> > > > -       return -EINVAL;
> > >
> > > if (vcpu->arch.ran_atleast_once)
> > >         return -EBUSY;
> >
> > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > will come before the first KVM_RUN. This will always cause an ERROR.
> 
> The check ensures that KVM user space can only enable/disable
> guest debug before the VCPU is run. I don't see why this would
> fail for QEMU.

In the current implementation of GDB and QEMU, the userspace will enable/disable
guest_debug frequently during the debugging (almost every step). 
The sequence should like:

KVM_SET_GUEST_DEBUG enable
KVM_RUN
KVM_SET_GUEST_DEBUG disable
KVM_SET_GUEST_DEBUG enable
KVM_RUN
KVM_SET_GUEST_DEBUG disable
KVM_SET_GUEST_DEBUG enable
KVM_RUN
KVM_SET_GUEST_DEBUG disable
...

> 
> >
> > >
> > >
> > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > +               vcpu->guest_debug = dbg->control;
> > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > +       } else {
> > > > +               vcpu->guest_debug = 0;
> > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > +       }
> > >
> > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > in kvm_riscv_vcpu_setup_config().
> > >
> > > > +
> > > > +       return 0;
> > > >  }
> > > >
> > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > >         }
> > > > +
> > > > +       if (!vcpu->guest_debug)
> > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > >
> > > This should be:
> > >
> > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > if (vcpu->guest_debug)
> > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> >
> > Like above, here the logic should be:
> >
> > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > if (vcpu->guest_debug)
> >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> >
> > Another approach is:
> > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> 
> I disagree. We should handle hedeleg just like we handle henvcfg.

OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().

> 
> >
> > >
> > > >  }
> > > >
> > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > >         csr_write(CSR_HVIP, csr->hvip);
> > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > --- a/arch/riscv/kvm/vm.c
> > > > +++ b/arch/riscv/kvm/vm.c
> > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > >         case KVM_CAP_READONLY_MEM:
> > > >         case KVM_CAP_MP_STATE:
> > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > >                 r = 1;
> > > >                 break;
> > > >         case KVM_CAP_NR_VCPUS:
> > > > --
> > > > 2.17.1
> > > >
> > >
> > > Regards,
> > > Anup
> >
> > Thanks,
> > Chao
> 
> Regards,
> Anup

Thanks,
Chao

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Fri, Mar 1, 2024 at 12:57 PM Chao Du <duchao@eswincomputing.com> wrote:
>
> On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > > >
> > > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > > >
> > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > been checked.
> > > > >
> > > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > > if the VCPU is being debugged by userspace, by clearing the
> > > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > > kvm_arch_vcpu_load().
> > > > >
> > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > ---
> > > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > > @@ -43,6 +43,22 @@
> > > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > > >
> > > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > >
> > > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> > >
> > > Thanks, I will use BIT() instead in next revision.
> > >
> > > >
> > >
> > > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> > >
> > > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > > and handle it accordingly.
> > >
> > > If the guest_debug is enabled, ebreak instructions are inserted by the
> > > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > > Bit EXC_BREAKPOINT should be cleared in this case.
> >
> > If EXC_BREAKPOINT is delegated by default then it is not consistent with
> > vcpu->guest_debug which is not enabled by default.
>
> To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
> They are somehow 'opposite'.
>
> This 'kvm_guest_debug' feature is different from "debug in the guest".
> The later requires the delegation of EXC_BREAKPOINT.
> The former does not.

In which case your below code is totally misleading.

+       if (dbg->control & KVM_GUESTDBG_ENABLE) {
+               vcpu->guest_debug = dbg->control;
+               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
+       } else {
+               vcpu->guest_debug = 0;
+               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
+       }

This should have been:

+       if (dbg->control & KVM_GUESTDBG_ENABLE) {
+               vcpu->guest_debug = dbg->control;
+               vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
+       } else {
+               vcpu->guest_debug = 0;
+               vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
+       }

>
> >
> > >
> > > >
> > > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > >
> > > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > > >
> > > > > +
> > > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > > +
> > > >
> > > > Same as above, use BIT(xyz) here.
> > > >
> > > > >  enum kvm_riscv_hfence_type {
> > > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > > >  struct kvm_vcpu_config {
> > > > >         u64 henvcfg;
> > > > >         u64 hstateen0;
> > > > > +       unsigned long hedeleg;
> > > > >  };
> > > > >
> > > > >  struct kvm_vcpu_smstateen_csr {
> > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > > +++ b/arch/riscv/include/uapi/asm/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
> > > > >
> > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > > --- a/arch/riscv/kvm/main.c
> > > > > +++ b/arch/riscv/kvm/main.c
> > > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > > >
> > > > >  int kvm_arch_hardware_enable(void)
> > > > >  {
> > > > > -       unsigned long hideleg, hedeleg;
> > > > > -
> > > > > -       hedeleg = 0;
> > > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > > -
> > > > > -       hideleg = 0;
> > > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > > >
> > > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > > >                                         struct kvm_guest_debug *dbg)
> > > > >  {
> > > > > -       /* TODO; To be implemented later. */
> > > > > -       return -EINVAL;
> > > >
> > > > if (vcpu->arch.ran_atleast_once)
> > > >         return -EBUSY;
> > >
> > > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > > will come before the first KVM_RUN. This will always cause an ERROR.
> >
> > The check ensures that KVM user space can only enable/disable
> > guest debug before the VCPU is run. I don't see why this would
> > fail for QEMU.
>
> In the current implementation of GDB and QEMU, the userspace will enable/disable
> guest_debug frequently during the debugging (almost every step).
> The sequence should like:
>
> KVM_SET_GUEST_DEBUG enable
> KVM_RUN
> KVM_SET_GUEST_DEBUG disable
> KVM_SET_GUEST_DEBUG enable
> KVM_RUN
> KVM_SET_GUEST_DEBUG disable
> KVM_SET_GUEST_DEBUG enable
> KVM_RUN
> KVM_SET_GUEST_DEBUG disable
> ...

Fair enough, no need to check "ran_atleast_once"

>
> >
> > >
> > > >
> > > >
> > > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > > +               vcpu->guest_debug = dbg->control;
> > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > > +       } else {
> > > > > +               vcpu->guest_debug = 0;
> > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > +       }
> > > >
> > > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > > in kvm_riscv_vcpu_setup_config().
> > > >
> > > > > +
> > > > > +       return 0;
> > > > >  }
> > > > >
> > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > > >         }
> > > > > +
> > > > > +       if (!vcpu->guest_debug)
> > > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > >
> > > > This should be:
> > > >
> > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > if (vcpu->guest_debug)
> > > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> > >
> > > Like above, here the logic should be:
> > >
> > > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > > if (vcpu->guest_debug)
> > >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > >
> > > Another approach is:
> > > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> >
> > I disagree. We should handle hedeleg just like we handle henvcfg.
>
> OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().
>
> >
> > >
> > > >
> > > > >  }
> > > > >
> > > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > > >         csr_write(CSR_HVIP, csr->hvip);
> > > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > > --- a/arch/riscv/kvm/vm.c
> > > > > +++ b/arch/riscv/kvm/vm.c
> > > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > > >         case KVM_CAP_READONLY_MEM:
> > > > >         case KVM_CAP_MP_STATE:
> > > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > > >                 r = 1;
> > > > >                 break;
> > > > >         case KVM_CAP_NR_VCPUS:
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > > Regards,
> > > > Anup
> > >
> > > Thanks,
> > > Chao
> >
> > Regards,
> > Anup
>
> Thanks,
> Chao
> --
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

Regards,
Anup

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

Thanks Anup.
Let me try to summarize the changes in next revision:

1. Use BIT().
2. In kvm_arch_vcpu_ioctl_set_guest_debug(), do below things:
       if (dbg->control & KVM_GUESTDBG_ENABLE) {
               vcpu->guest_debug = dbg->control;
       } else {
               vcpu->guest_debug = 0;
       }
3. In kvm_riscv_vcpu_setup_config(), do below things:
       cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
       if (vcpu->guest_debug)
               cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);

Will prepare a PATCH v3 accordingly.

Regards,
Chao

On 2024-03-01 16:11, Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Fri, Mar 1, 2024 at 12:57 PM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> > >
> > > On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > >
> > > > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > > > >
> > > > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > >
> > > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > > been checked.
> > > > > >
> > > > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > > > if the VCPU is being debugged by userspace, by clearing the
> > > > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > > > kvm_arch_vcpu_load().
> > > > > >
> > > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > > ---
> > > > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > > > @@ -43,6 +43,22 @@
> > > > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > > > >
> > > > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > >
> > > > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> > > >
> > > > Thanks, I will use BIT() instead in next revision.
> > > >
> > > > >
> > > >
> > > > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> > > >
> > > > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > > > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > > > and handle it accordingly.
> > > >
> > > > If the guest_debug is enabled, ebreak instructions are inserted by the
> > > > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > > > Bit EXC_BREAKPOINT should be cleared in this case.
> > >
> > > If EXC_BREAKPOINT is delegated by default then it is not consistent with
> > > vcpu->guest_debug which is not enabled by default.
> >
> > To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
> > They are somehow 'opposite'.
> >
> > This 'kvm_guest_debug' feature is different from "debug in the guest".
> > The later requires the delegation of EXC_BREAKPOINT.
> > The former does not.
> 
> In which case your below code is totally misleading.
> 
> +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> +               vcpu->guest_debug = dbg->control;
> +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> +       } else {
> +               vcpu->guest_debug = 0;
> +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> +       }
> 
> This should have been:
> 
> +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> +               vcpu->guest_debug = dbg->control;
> +               vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
> +       } else {
> +               vcpu->guest_debug = 0;
> +               vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
> +       }
> 
> >
> > >
> > > >
> > > > >
> > > > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > >
> > > > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > > > >
> > > > > > +
> > > > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > > > +
> > > > >
> > > > > Same as above, use BIT(xyz) here.
> > > > >
> > > > > >  enum kvm_riscv_hfence_type {
> > > > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > > > >  struct kvm_vcpu_config {
> > > > > >         u64 henvcfg;
> > > > > >         u64 hstateen0;
> > > > > > +       unsigned long hedeleg;
> > > > > >  };
> > > > > >
> > > > > >  struct kvm_vcpu_smstateen_csr {
> > > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > > > +++ b/arch/riscv/include/uapi/asm/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
> > > > > >
> > > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > > > --- a/arch/riscv/kvm/main.c
> > > > > > +++ b/arch/riscv/kvm/main.c
> > > > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > > > >
> > > > > >  int kvm_arch_hardware_enable(void)
> > > > > >  {
> > > > > > -       unsigned long hideleg, hedeleg;
> > > > > > -
> > > > > > -       hedeleg = 0;
> > > > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > > > -
> > > > > > -       hideleg = 0;
> > > > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > > > >
> > > > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > > > >                                         struct kvm_guest_debug *dbg)
> > > > > >  {
> > > > > > -       /* TODO; To be implemented later. */
> > > > > > -       return -EINVAL;
> > > > >
> > > > > if (vcpu->arch.ran_atleast_once)
> > > > >         return -EBUSY;
> > > >
> > > > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > > > will come before the first KVM_RUN. This will always cause an ERROR.
> > >
> > > The check ensures that KVM user space can only enable/disable
> > > guest debug before the VCPU is run. I don't see why this would
> > > fail for QEMU.
> >
> > In the current implementation of GDB and QEMU, the userspace will enable/disable
> > guest_debug frequently during the debugging (almost every step).
> > The sequence should like:
> >
> > KVM_SET_GUEST_DEBUG enable
> > KVM_RUN
> > KVM_SET_GUEST_DEBUG disable
> > KVM_SET_GUEST_DEBUG enable
> > KVM_RUN
> > KVM_SET_GUEST_DEBUG disable
> > KVM_SET_GUEST_DEBUG enable
> > KVM_RUN
> > KVM_SET_GUEST_DEBUG disable
> > ...
> 
> Fair enough, no need to check "ran_atleast_once"
> 
> >
> > >
> > > >
> > > > >
> > > > >
> > > > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > > > +               vcpu->guest_debug = dbg->control;
> > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > > > +       } else {
> > > > > > +               vcpu->guest_debug = 0;
> > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > +       }
> > > > >
> > > > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > > > in kvm_riscv_vcpu_setup_config().
> > > > >
> > > > > > +
> > > > > > +       return 0;
> > > > > >  }
> > > > > >
> > > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > > > >         }
> > > > > > +
> > > > > > +       if (!vcpu->guest_debug)
> > > > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > >
> > > > > This should be:
> > > > >
> > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > if (vcpu->guest_debug)
> > > > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> > > >
> > > > Like above, here the logic should be:
> > > >
> > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > > > if (vcpu->guest_debug)
> > > >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > >
> > > > Another approach is:
> > > > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > > > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> > >
> > > I disagree. We should handle hedeleg just like we handle henvcfg.
> >
> > OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().
> >
> > >
> > > >
> > > > >
> > > > > >  }
> > > > > >
> > > > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > > > >         csr_write(CSR_HVIP, csr->hvip);
> > > > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > > > --- a/arch/riscv/kvm/vm.c
> > > > > > +++ b/arch/riscv/kvm/vm.c
> > > > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > > > >         case KVM_CAP_READONLY_MEM:
> > > > > >         case KVM_CAP_MP_STATE:
> > > > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > > > >                 r = 1;
> > > > > >                 break;
> > > > > >         case KVM_CAP_NR_VCPUS:
> > > > > > --
> > > > > > 2.17.1
> > > > > >
> > > > >
> > > > > Regards,
> > > > > Anup
> > > >
> > > > Thanks,
> > > > Chao
> > >
> > > Regards,
> > > Anup
> >
> > Thanks,
> > Chao
> > --
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> 
> Regards,
> Anup

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Fri, Mar 1, 2024 at 1:46 PM Chao Du <duchao@eswincomputing.com> wrote:
>
> Thanks Anup.
> Let me try to summarize the changes in next revision:
>
> 1. Use BIT().
> 2. In kvm_arch_vcpu_ioctl_set_guest_debug(), do below things:
>        if (dbg->control & KVM_GUESTDBG_ENABLE) {
>                vcpu->guest_debug = dbg->control;
>        } else {
>                vcpu->guest_debug = 0;
>        }

Since, kvm_arch_vcpu_ioctl_set_guest_debug() is called multiple times
at runtime, this should be:

        if (dbg->control & KVM_GUESTDBG_ENABLE) {
                vcpu->guest_debug = dbg->control;
                vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
        } else {
                vcpu->guest_debug = 0;
                vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
        }

> 3. In kvm_riscv_vcpu_setup_config(), do below things:
>        cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
>        if (vcpu->guest_debug)
>                cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
>
> Will prepare a PATCH v3 accordingly.

Yes, please send v3.

Thanks,
Anup

> On 2024-03-01 16:11, Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Fri, Mar 1, 2024 at 12:57 PM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> > > >
> > > > On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > >
> > > > > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > > > > >
> > > > > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > > >
> > > > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > > > been checked.
> > > > > > >
> > > > > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > > > > if the VCPU is being debugged by userspace, by clearing the
> > > > > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > > > > kvm_arch_vcpu_load().
> > > > > > >
> > > > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > > > ---
> > > > > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > > > > @@ -43,6 +43,22 @@
> > > > > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > > > > >
> > > > > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > >
> > > > > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> > > > >
> > > > > Thanks, I will use BIT() instead in next revision.
> > > > >
> > > > > >
> > > > >
> > > > > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> > > > >
> > > > > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > > > > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > > > > and handle it accordingly.
> > > > >
> > > > > If the guest_debug is enabled, ebreak instructions are inserted by the
> > > > > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > > > > Bit EXC_BREAKPOINT should be cleared in this case.
> > > >
> > > > If EXC_BREAKPOINT is delegated by default then it is not consistent with
> > > > vcpu->guest_debug which is not enabled by default.
> > >
> > > To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
> > > They are somehow 'opposite'.
> > >
> > > This 'kvm_guest_debug' feature is different from "debug in the guest".
> > > The later requires the delegation of EXC_BREAKPOINT.
> > > The former does not.
> >
> > In which case your below code is totally misleading.
> >
> > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > +               vcpu->guest_debug = dbg->control;
> > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > +       } else {
> > +               vcpu->guest_debug = 0;
> > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > +       }
> >
> > This should have been:
> >
> > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > +               vcpu->guest_debug = dbg->control;
> > +               vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
> > +       } else {
> > +               vcpu->guest_debug = 0;
> > +               vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
> > +       }
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > >
> > > > > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > > > > >
> > > > > > > +
> > > > > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > > > > +
> > > > > >
> > > > > > Same as above, use BIT(xyz) here.
> > > > > >
> > > > > > >  enum kvm_riscv_hfence_type {
> > > > > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > > > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > > > > >  struct kvm_vcpu_config {
> > > > > > >         u64 henvcfg;
> > > > > > >         u64 hstateen0;
> > > > > > > +       unsigned long hedeleg;
> > > > > > >  };
> > > > > > >
> > > > > > >  struct kvm_vcpu_smstateen_csr {
> > > > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > +++ b/arch/riscv/include/uapi/asm/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
> > > > > > >
> > > > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > > > > --- a/arch/riscv/kvm/main.c
> > > > > > > +++ b/arch/riscv/kvm/main.c
> > > > > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > > > > >
> > > > > > >  int kvm_arch_hardware_enable(void)
> > > > > > >  {
> > > > > > > -       unsigned long hideleg, hedeleg;
> > > > > > > -
> > > > > > > -       hedeleg = 0;
> > > > > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > > > > -
> > > > > > > -       hideleg = 0;
> > > > > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > > > > >
> > > > > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > > > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > > > > >                                         struct kvm_guest_debug *dbg)
> > > > > > >  {
> > > > > > > -       /* TODO; To be implemented later. */
> > > > > > > -       return -EINVAL;
> > > > > >
> > > > > > if (vcpu->arch.ran_atleast_once)
> > > > > >         return -EBUSY;
> > > > >
> > > > > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > > > > will come before the first KVM_RUN. This will always cause an ERROR.
> > > >
> > > > The check ensures that KVM user space can only enable/disable
> > > > guest debug before the VCPU is run. I don't see why this would
> > > > fail for QEMU.
> > >
> > > In the current implementation of GDB and QEMU, the userspace will enable/disable
> > > guest_debug frequently during the debugging (almost every step).
> > > The sequence should like:
> > >
> > > KVM_SET_GUEST_DEBUG enable
> > > KVM_RUN
> > > KVM_SET_GUEST_DEBUG disable
> > > KVM_SET_GUEST_DEBUG enable
> > > KVM_RUN
> > > KVM_SET_GUEST_DEBUG disable
> > > KVM_SET_GUEST_DEBUG enable
> > > KVM_RUN
> > > KVM_SET_GUEST_DEBUG disable
> > > ...
> >
> > Fair enough, no need to check "ran_atleast_once"
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > > > > +               vcpu->guest_debug = dbg->control;
> > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > > > > +       } else {
> > > > > > > +               vcpu->guest_debug = 0;
> > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > +       }
> > > > > >
> > > > > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > > > > in kvm_riscv_vcpu_setup_config().
> > > > > >
> > > > > > > +
> > > > > > > +       return 0;
> > > > > > >  }
> > > > > > >
> > > > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > > > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > > > > >         }
> > > > > > > +
> > > > > > > +       if (!vcpu->guest_debug)
> > > > > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > >
> > > > > > This should be:
> > > > > >
> > > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > if (vcpu->guest_debug)
> > > > > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> > > > >
> > > > > Like above, here the logic should be:
> > > > >
> > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > > > > if (vcpu->guest_debug)
> > > > >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > > >
> > > > > Another approach is:
> > > > > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > > > > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> > > >
> > > > I disagree. We should handle hedeleg just like we handle henvcfg.
> > >
> > > OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >  }
> > > > > > >
> > > > > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > > > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > > > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > > > > >         csr_write(CSR_HVIP, csr->hvip);
> > > > > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > > > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > > > > --- a/arch/riscv/kvm/vm.c
> > > > > > > +++ b/arch/riscv/kvm/vm.c
> > > > > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > > > > >         case KVM_CAP_READONLY_MEM:
> > > > > > >         case KVM_CAP_MP_STATE:
> > > > > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > > > > >                 r = 1;
> > > > > > >                 break;
> > > > > > >         case KVM_CAP_NR_VCPUS:
> > > > > > > --
> > > > > > > 2.17.1
> > > > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Anup
> > > > >
> > > > > Thanks,
> > > > > Chao
> > > >
> > > > Regards,
> > > > Anup
> > >
> > > Thanks,
> > > Chao
> > > --
> > > kvm-riscv mailing list
> > > kvm-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> >
> > Regards,
> > Anup

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

On 2024-03-01 17:22, Anup Patel <anup@brainfault.org> wrote:
> 
> On Fri, Mar 1, 2024 at 1:46 PM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > Thanks Anup.
> > Let me try to summarize the changes in next revision:
> >
> > 1. Use BIT().
> > 2. In kvm_arch_vcpu_ioctl_set_guest_debug(), do below things:
> >        if (dbg->control & KVM_GUESTDBG_ENABLE) {
> >                vcpu->guest_debug = dbg->control;
> >        } else {
> >                vcpu->guest_debug = 0;
> >        }
> 
> Since, kvm_arch_vcpu_ioctl_set_guest_debug() is called multiple times
> at runtime, this should be:
> 
>         if (dbg->control & KVM_GUESTDBG_ENABLE) {
>                 vcpu->guest_debug = dbg->control;
>                 vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
>         } else {
>                 vcpu->guest_debug = 0;
>                 vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
>         }
> 
> > 3. In kvm_riscv_vcpu_setup_config(), do below things:
> >        cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> >        if (vcpu->guest_debug)
> >                cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> >
> > Will prepare a PATCH v3 accordingly.

I thought it over, maybe we should do this:

- In kvm_arch_vcpu_ioctl_set_guest_debug():
       if (dbg->control & KVM_GUESTDBG_ENABLE) {
               vcpu->guest_debug = dbg->control;
               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT & (~BIT(EXC_BREAKPOINT));
       } else {
               vcpu->guest_debug = 0;
               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
       }
- In kvm_riscv_vcpu_setup_config():
       if (!vcpu->guest_debug)
               cfg->hedeleg = KVM_HEDELEG_DEFAULT;

which is similar with my original changes, but clear the bit EXC_BREAKPOINT explicitly.

Could you please confirm ?

> 
> Yes, please send v3.
> 
> Thanks,
> Anup
> 
> > On 2024-03-01 16:11, Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Fri, Mar 1, 2024 at 12:57 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > >
> > > > On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> > > > >
> > > > > On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > >
> > > > > > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > > > > > >
> > > > > > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > > > >
> > > > > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > > > > been checked.
> > > > > > > >
> > > > > > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > > > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > > > > > if the VCPU is being debugged by userspace, by clearing the
> > > > > > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > > > > > kvm_arch_vcpu_load().
> > > > > > > >
> > > > > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > > > > ---
> > > > > > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > > > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > > > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > > > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > > > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > > > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > > > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > > > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > > > > > @@ -43,6 +43,22 @@
> > > > > > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > > > > > >
> > > > > > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > > >
> > > > > > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> > > > > >
> > > > > > Thanks, I will use BIT() instead in next revision.
> > > > > >
> > > > > > >
> > > > > >
> > > > > > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> > > > > >
> > > > > > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > > > > > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > > > > > and handle it accordingly.
> > > > > >
> > > > > > If the guest_debug is enabled, ebreak instructions are inserted by the
> > > > > > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > > > > > Bit EXC_BREAKPOINT should be cleared in this case.
> > > > >
> > > > > If EXC_BREAKPOINT is delegated by default then it is not consistent with
> > > > > vcpu->guest_debug which is not enabled by default.
> > > >
> > > > To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
> > > > They are somehow 'opposite'.
> > > >
> > > > This 'kvm_guest_debug' feature is different from "debug in the guest".
> > > > The later requires the delegation of EXC_BREAKPOINT.
> > > > The former does not.
> > >
> > > In which case your below code is totally misleading.
> > >
> > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > +               vcpu->guest_debug = dbg->control;
> > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > +       } else {
> > > +               vcpu->guest_debug = 0;
> > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > +       }
> > >
> > > This should have been:
> > >
> > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > +               vcpu->guest_debug = dbg->control;
> > > +               vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > +       } else {
> > > +               vcpu->guest_debug = 0;
> > > +               vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
> > > +       }
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > > >
> > > > > > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > > > > > >
> > > > > > > > +
> > > > > > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > > > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > > > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > > > > > +
> > > > > > >
> > > > > > > Same as above, use BIT(xyz) here.
> > > > > > >
> > > > > > > >  enum kvm_riscv_hfence_type {
> > > > > > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > > > > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > > > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > > > > > >  struct kvm_vcpu_config {
> > > > > > > >         u64 henvcfg;
> > > > > > > >         u64 hstateen0;
> > > > > > > > +       unsigned long hedeleg;
> > > > > > > >  };
> > > > > > > >
> > > > > > > >  struct kvm_vcpu_smstateen_csr {
> > > > > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > > > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > > +++ b/arch/riscv/include/uapi/asm/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
> > > > > > > >
> > > > > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > > > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > > > > > --- a/arch/riscv/kvm/main.c
> > > > > > > > +++ b/arch/riscv/kvm/main.c
> > > > > > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > > > > > >
> > > > > > > >  int kvm_arch_hardware_enable(void)
> > > > > > > >  {
> > > > > > > > -       unsigned long hideleg, hedeleg;
> > > > > > > > -
> > > > > > > > -       hedeleg = 0;
> > > > > > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > > > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > > > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > > > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > > > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > > > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > > > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > > > > > -
> > > > > > > > -       hideleg = 0;
> > > > > > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > > > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > > > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > > > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > > > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > > > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > > > > > >
> > > > > > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > > > > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > > > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > > > > > >                                         struct kvm_guest_debug *dbg)
> > > > > > > >  {
> > > > > > > > -       /* TODO; To be implemented later. */
> > > > > > > > -       return -EINVAL;
> > > > > > >
> > > > > > > if (vcpu->arch.ran_atleast_once)
> > > > > > >         return -EBUSY;
> > > > > >
> > > > > > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > > > > > will come before the first KVM_RUN. This will always cause an ERROR.
> > > > >
> > > > > The check ensures that KVM user space can only enable/disable
> > > > > guest debug before the VCPU is run. I don't see why this would
> > > > > fail for QEMU.
> > > >
> > > > In the current implementation of GDB and QEMU, the userspace will enable/disable
> > > > guest_debug frequently during the debugging (almost every step).
> > > > The sequence should like:
> > > >
> > > > KVM_SET_GUEST_DEBUG enable
> > > > KVM_RUN
> > > > KVM_SET_GUEST_DEBUG disable
> > > > KVM_SET_GUEST_DEBUG enable
> > > > KVM_RUN
> > > > KVM_SET_GUEST_DEBUG disable
> > > > KVM_SET_GUEST_DEBUG enable
> > > > KVM_RUN
> > > > KVM_SET_GUEST_DEBUG disable
> > > > ...
> > >
> > > Fair enough, no need to check "ran_atleast_once"
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > > > > > +               vcpu->guest_debug = dbg->control;
> > > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > > > > > +       } else {
> > > > > > > > +               vcpu->guest_debug = 0;
> > > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > > +       }
> > > > > > >
> > > > > > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > > > > > in kvm_riscv_vcpu_setup_config().
> > > > > > >
> > > > > > > > +
> > > > > > > > +       return 0;
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > > > > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > > > > > >         }
> > > > > > > > +
> > > > > > > > +       if (!vcpu->guest_debug)
> > > > > > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > >
> > > > > > > This should be:
> > > > > > >
> > > > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > if (vcpu->guest_debug)
> > > > > > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> > > > > >
> > > > > > Like above, here the logic should be:
> > > > > >
> > > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > > > > > if (vcpu->guest_debug)
> > > > > >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > > > >
> > > > > > Another approach is:
> > > > > > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > > > > > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> > > > >
> > > > > I disagree. We should handle hedeleg just like we handle henvcfg.
> > > >
> > > > OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >  }
> > > > > > > >
> > > > > > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > > > > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > > > > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > > > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > > > > > >         csr_write(CSR_HVIP, csr->hvip);
> > > > > > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > > > > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > > > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > > > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > > > > > --- a/arch/riscv/kvm/vm.c
> > > > > > > > +++ b/arch/riscv/kvm/vm.c
> > > > > > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > > > > > >         case KVM_CAP_READONLY_MEM:
> > > > > > > >         case KVM_CAP_MP_STATE:
> > > > > > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > > > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > > > > > >                 r = 1;
> > > > > > > >                 break;
> > > > > > > >         case KVM_CAP_NR_VCPUS:
> > > > > > > > --
> > > > > > > > 2.17.1
> > > > > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > > Anup
> > > > > >
> > > > > > Thanks,
> > > > > > Chao
> > > > >
> > > > > Regards,
> > > > > Anup
> > > >
> > > > Thanks,
> > > > Chao
> > > > --
> > > > kvm-riscv mailing list
> > > > kvm-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > >
> > > Regards,
> > > Anup

Re: [PATCH v2 3/3] RISC-V: KVM: selftests: Add breakpoints test support

[Andrew Jones]

On Fri, Mar 01, 2024 at 01:35:45AM +0000, Chao Du wrote:
> Initial support for RISC-V KVM breakpoint test. Check the exit reason
> and the PC when guest debug is enabled.
> 
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
>  tools/testing/selftests/kvm/Makefile          |  1 +
>  .../testing/selftests/kvm/riscv/breakpoints.c | 49 +++++++++++++++++++
>  2 files changed, 50 insertions(+)
>  create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c
> 
> diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> index 492e937fab00..5f9048a740b0 100644
> --- a/tools/testing/selftests/kvm/Makefile
> +++ b/tools/testing/selftests/kvm/Makefile
> @@ -184,6 +184,7 @@ TEST_GEN_PROGS_s390x += rseq_test
>  TEST_GEN_PROGS_s390x += set_memory_region_test
>  TEST_GEN_PROGS_s390x += kvm_binary_stats_test
>  
> +TEST_GEN_PROGS_riscv += riscv/breakpoints
>  TEST_GEN_PROGS_riscv += demand_paging_test
>  TEST_GEN_PROGS_riscv += dirty_log_test
>  TEST_GEN_PROGS_riscv += get-reg-list
> diff --git a/tools/testing/selftests/kvm/riscv/breakpoints.c b/tools/testing/selftests/kvm/riscv/breakpoints.c
> new file mode 100644
> index 000000000000..be2d94837c83
> --- /dev/null
> +++ b/tools/testing/selftests/kvm/riscv/breakpoints.c
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * RISC-V KVM breakpoint tests.
> + *
> + * Copyright 2024 Beijing ESWIN Computing Technology Co., Ltd.
> + *
> + */
> +#include "kvm_util.h"
> +
> +#define PC(v) ((uint64_t)&(v))
> +
> +extern unsigned char sw_bp;
> +
> +static void guest_code(void)
> +{
> +	asm volatile("sw_bp: ebreak");
> +	asm volatile("nop");
> +	asm volatile("nop");
> +	asm volatile("nop");

What are the nops for? And, since they're all in their own asm()'s the
compiler could be inserting instructions between them and also the ebreak
above. If we need three nops immediately following the ebreak then we
need to put everything in one asm()

  asm volatile(
  "sw_bp:	ebreak\n"
  "		nop\n"
  "		nop\n"
  "		nop\n");

> +
> +	GUEST_DONE();
> +}
> +
> +int main(void)
> +{
> +	struct kvm_vm *vm;
> +	struct kvm_vcpu *vcpu;
> +	struct kvm_guest_debug debug;
> +	uint64_t pc;
> +
> +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SET_GUEST_DEBUG));
> +
> +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> +
> +	memset(&debug, 0, sizeof(debug));
> +	debug.control = KVM_GUESTDBG_ENABLE;

nit: The above two lines can be removed if we initialize debug as

  struct kvm_guest_debug debug = {
     .control = KVM_GUESTDBG_ENABLE,
  };

> +	vcpu_guest_debug_set(vcpu, &debug);
> +	vcpu_run(vcpu);
> +
> +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);

As Anup pointed out, we need to also ensure that without making the
KVM_SET_GUEST_DEBUG ioctl call we get the expected behavior. You
can use GUEST_SYNC() in the guest code to prove that it was able to
issue an ebreak without exiting to the VMM.

> +
> +	vcpu_get_reg(vcpu, RISCV_CORE_REG(regs.pc), &pc);
> +
> +	TEST_ASSERT_EQ(pc, PC(sw_bp));
> +
> +	kvm_vm_free(vm);
> +
> +	return 0;
> +}
> -- 
> 2.17.1
>

Thanks,
drew

Re: [PATCH v2 3/3] RISC-V: KVM: selftests: Add breakpoints test support

[Chao Du]

On 2024-03-01 17:24, Andrew Jones <ajones@ventanamicro.com> wrote:
> 
> On Fri, Mar 01, 2024 at 01:35:45AM +0000, Chao Du wrote:
> > Initial support for RISC-V KVM breakpoint test. Check the exit reason
> > and the PC when guest debug is enabled.
> > 
> > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > ---
> >  tools/testing/selftests/kvm/Makefile          |  1 +
> >  .../testing/selftests/kvm/riscv/breakpoints.c | 49 +++++++++++++++++++
> >  2 files changed, 50 insertions(+)
> >  create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c
> > 
> > diff --git a/tools/testing/selftests/kvm/Makefile b/tools/testing/selftests/kvm/Makefile
> > index 492e937fab00..5f9048a740b0 100644
> > --- a/tools/testing/selftests/kvm/Makefile
> > +++ b/tools/testing/selftests/kvm/Makefile
> > @@ -184,6 +184,7 @@ TEST_GEN_PROGS_s390x += rseq_test
> >  TEST_GEN_PROGS_s390x += set_memory_region_test
> >  TEST_GEN_PROGS_s390x += kvm_binary_stats_test
> >  
> > +TEST_GEN_PROGS_riscv += riscv/breakpoints
> >  TEST_GEN_PROGS_riscv += demand_paging_test
> >  TEST_GEN_PROGS_riscv += dirty_log_test
> >  TEST_GEN_PROGS_riscv += get-reg-list
> > diff --git a/tools/testing/selftests/kvm/riscv/breakpoints.c b/tools/testing/selftests/kvm/riscv/breakpoints.c
> > new file mode 100644
> > index 000000000000..be2d94837c83
> > --- /dev/null
> > +++ b/tools/testing/selftests/kvm/riscv/breakpoints.c
> > @@ -0,0 +1,49 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * RISC-V KVM breakpoint tests.
> > + *
> > + * Copyright 2024 Beijing ESWIN Computing Technology Co., Ltd.
> > + *
> > + */
> > +#include "kvm_util.h"
> > +
> > +#define PC(v) ((uint64_t)&(v))
> > +
> > +extern unsigned char sw_bp;
> > +
> > +static void guest_code(void)
> > +{
> > +	asm volatile("sw_bp: ebreak");
> > +	asm volatile("nop");
> > +	asm volatile("nop");
> > +	asm volatile("nop");
> 
> What are the nops for? And, since they're all in their own asm()'s the
> compiler could be inserting instructions between them and also the ebreak
> above. If we need three nops immediately following the ebreak then we
> need to put everything in one asm()
> 
>   asm volatile(
>   "sw_bp:	ebreak\n"
>   "		nop\n"
>   "		nop\n"
>   "		nop\n");
> 

Actually the nops have no special purpose, I will remove and just keep the
ebreak here.

> > +
> > +	GUEST_DONE();
> > +}
> > +
> > +int main(void)
> > +{
> > +	struct kvm_vm *vm;
> > +	struct kvm_vcpu *vcpu;
> > +	struct kvm_guest_debug debug;
> > +	uint64_t pc;
> > +
> > +	TEST_REQUIRE(kvm_has_cap(KVM_CAP_SET_GUEST_DEBUG));
> > +
> > +	vm = vm_create_with_one_vcpu(&vcpu, guest_code);
> > +
> > +	memset(&debug, 0, sizeof(debug));
> > +	debug.control = KVM_GUESTDBG_ENABLE;
> 
> nit: The above two lines can be removed if we initialize debug as
> 
>   struct kvm_guest_debug debug = {
>      .control = KVM_GUESTDBG_ENABLE,
>   };
> 

Yeah, I will do so in the next patch.

> > +	vcpu_guest_debug_set(vcpu, &debug);
> > +	vcpu_run(vcpu);
> > +
> > +	TEST_ASSERT_KVM_EXIT_REASON(vcpu, KVM_EXIT_DEBUG);
> 
> As Anup pointed out, we need to also ensure that without making the
> KVM_SET_GUEST_DEBUG ioctl call we get the expected behavior. You
> can use GUEST_SYNC() in the guest code to prove that it was able to
> issue an ebreak without exiting to the VMM.
> 

Sure, will cover that case also.

> > +
> > +	vcpu_get_reg(vcpu, RISCV_CORE_REG(regs.pc), &pc);
> > +
> > +	TEST_ASSERT_EQ(pc, PC(sw_bp));
> > +
> > +	kvm_vm_free(vm);
> > +
> > +	return 0;
> > +}
> > -- 
> > 2.17.1
> >
> 
> Thanks,
> drew

Re: [PATCH v2 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Fri, Mar 1, 2024 at 2:53 PM Chao Du <duchao@eswincomputing.com> wrote:
>
> On 2024-03-01 17:22, Anup Patel <anup@brainfault.org> wrote:
> >
> > On Fri, Mar 1, 2024 at 1:46 PM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > Thanks Anup.
> > > Let me try to summarize the changes in next revision:
> > >
> > > 1. Use BIT().
> > > 2. In kvm_arch_vcpu_ioctl_set_guest_debug(), do below things:
> > >        if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > >                vcpu->guest_debug = dbg->control;
> > >        } else {
> > >                vcpu->guest_debug = 0;
> > >        }
> >
> > Since, kvm_arch_vcpu_ioctl_set_guest_debug() is called multiple times
> > at runtime, this should be:
> >
> >         if (dbg->control & KVM_GUESTDBG_ENABLE) {
> >                 vcpu->guest_debug = dbg->control;
> >                 vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
> >         } else {
> >                 vcpu->guest_debug = 0;
> >                 vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
> >         }
> >
> > > 3. In kvm_riscv_vcpu_setup_config(), do below things:
> > >        cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > >        if (vcpu->guest_debug)
> > >                cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > >
> > > Will prepare a PATCH v3 accordingly.
>
> I thought it over, maybe we should do this:
>
> - In kvm_arch_vcpu_ioctl_set_guest_debug():
>        if (dbg->control & KVM_GUESTDBG_ENABLE) {
>                vcpu->guest_debug = dbg->control;
>                vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT & (~BIT(EXC_BREAKPOINT));
>        } else {
>                vcpu->guest_debug = 0;
>                vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
>        }
> - In kvm_riscv_vcpu_setup_config():
>        if (!vcpu->guest_debug)
>                cfg->hedeleg = KVM_HEDELEG_DEFAULT;
>
> which is similar with my original changes, but clear the bit EXC_BREAKPOINT explicitly.
>
> Could you please confirm ?

I suggest going with my previous suggestion.

Regards,
Anup

>
> >
> > Yes, please send v3.
> >
> > Thanks,
> > Anup
> >
> > > On 2024-03-01 16:11, Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Fri, Mar 1, 2024 at 12:57 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > >
> > > > > On 2024-03-01 15:29, Anup Patel <anup@brainfault.org> wrote:
> > > > > >
> > > > > > On Fri, Mar 1, 2024 at 12:05 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > > >
> > > > > > > On 2024-03-01 13:00, Anup Patel <anup@brainfault.org> wrote:
> > > > > > > >
> > > > > > > > On Fri, Mar 1, 2024 at 7:08 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > > > > >
> > > > > > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > > > > > been checked.
> > > > > > > > >
> > > > > > > > > kvm_arch_vcpu_ioctl_set_guest_debug(): Update the guest_debug flags
> > > > > > > > > from userspace accordingly. Route the breakpoint exceptions to HS mode
> > > > > > > > > if the VCPU is being debugged by userspace, by clearing the
> > > > > > > > > corresponding bit in hedeleg. Write the actual CSR in
> > > > > > > > > kvm_arch_vcpu_load().
> > > > > > > > >
> > > > > > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > > > > > ---
> > > > > > > > >  arch/riscv/include/asm/kvm_host.h | 17 +++++++++++++++++
> > > > > > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > > > > > >  arch/riscv/kvm/main.c             | 18 ++----------------
> > > > > > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > > > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > > > > > >  5 files changed, 34 insertions(+), 18 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/arch/riscv/include/asm/kvm_host.h b/arch/riscv/include/asm/kvm_host.h
> > > > > > > > > index 484d04a92fa6..9ee3f03ba5d1 100644
> > > > > > > > > --- a/arch/riscv/include/asm/kvm_host.h
> > > > > > > > > +++ b/arch/riscv/include/asm/kvm_host.h
> > > > > > > > > @@ -43,6 +43,22 @@
> > > > > > > > >         KVM_ARCH_REQ_FLAGS(5, KVM_REQUEST_WAIT | KVM_REQUEST_NO_WAKEUP)
> > > > > > > > >  #define KVM_REQ_STEAL_UPDATE           KVM_ARCH_REQ(6)
> > > > > > > > >
> > > > > > > > > +#define KVM_HEDELEG_DEFAULT            ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_BREAKPOINT) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > > > >
> > > > > > > > Use BIT(xyz) here. For example: BIT(EXC_INST_MISALIGNED)
> > > > > > >
> > > > > > > Thanks, I will use BIT() instead in next revision.
> > > > > > >
> > > > > > > >
> > > > > > >
> > > > > > > > Also, BIT(EXC_BREAKPOINT) should not be part of KVM_HEDELEG_DEFAULT.
> > > > > > >
> > > > > > > I think the bit EXC_BREAKPOINT should be set by default, like what you
> > > > > > > already did in kvm_arch_hardware_enable(). Then the VS could get the ebreak
> > > > > > > and handle it accordingly.
> > > > > > >
> > > > > > > If the guest_debug is enabled, ebreak instructions are inserted by the
> > > > > > > userspace(QEMU). So KVM should 'intercept' the ebreak and exit to QEMU.
> > > > > > > Bit EXC_BREAKPOINT should be cleared in this case.
> > > > > >
> > > > > > If EXC_BREAKPOINT is delegated by default then it is not consistent with
> > > > > > vcpu->guest_debug which is not enabled by default.
> > > > >
> > > > > To enable the guest_debug corresponding to NOT delegate the EXC_BREAKPOINT.
> > > > > They are somehow 'opposite'.
> > > > >
> > > > > This 'kvm_guest_debug' feature is different from "debug in the guest".
> > > > > The later requires the delegation of EXC_BREAKPOINT.
> > > > > The former does not.
> > > >
> > > > In which case your below code is totally misleading.
> > > >
> > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > +               vcpu->guest_debug = dbg->control;
> > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > +       } else {
> > > > +               vcpu->guest_debug = 0;
> > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > +       }
> > > >
> > > > This should have been:
> > > >
> > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > +               vcpu->guest_debug = dbg->control;
> > > > +               vcpu->arch.cfg.hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > > +       } else {
> > > > +               vcpu->guest_debug = 0;
> > > > +               vcpu->arch.cfg.hedeleg |= BIT(EXC_BREAKPOINT);
> > > > +       }
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > > +#define KVM_HEDELEG_GUEST_DEBUG                ((_AC(1, UL) << EXC_INST_MISALIGNED) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_SYSCALL) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_INST_PAGE_FAULT) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_LOAD_PAGE_FAULT) | \
> > > > > > > > > +                                        (_AC(1, UL) << EXC_STORE_PAGE_FAULT))
> > > > > > > >
> > > > > > > > No need for KVM_HEDELEG_GUEST_DEBUG, see below.
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > +#define KVM_HIDELEG_DEFAULT            ((_AC(1, UL) << IRQ_VS_SOFT) | \
> > > > > > > > > +                                        (_AC(1, UL) << IRQ_VS_TIMER) | \
> > > > > > > > > +                                        (_AC(1, UL) << IRQ_VS_EXT))
> > > > > > > > > +
> > > > > > > >
> > > > > > > > Same as above, use BIT(xyz) here.
> > > > > > > >
> > > > > > > > >  enum kvm_riscv_hfence_type {
> > > > > > > > >         KVM_RISCV_HFENCE_UNKNOWN = 0,
> > > > > > > > >         KVM_RISCV_HFENCE_GVMA_VMID_GPA,
> > > > > > > > > @@ -169,6 +185,7 @@ struct kvm_vcpu_csr {
> > > > > > > > >  struct kvm_vcpu_config {
> > > > > > > > >         u64 henvcfg;
> > > > > > > > >         u64 hstateen0;
> > > > > > > > > +       unsigned long hedeleg;
> > > > > > > > >  };
> > > > > > > > >
> > > > > > > > >  struct kvm_vcpu_smstateen_csr {
> > > > > > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > > > index 7499e88a947c..39f4f4b9dede 100644
> > > > > > > > > --- a/arch/riscv/include/uapi/asm/kvm.h
> > > > > > > > > +++ b/arch/riscv/include/uapi/asm/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
> > > > > > > > >
> > > > > > > > > diff --git a/arch/riscv/kvm/main.c b/arch/riscv/kvm/main.c
> > > > > > > > > index 225a435d9c9a..bab2ec34cd87 100644
> > > > > > > > > --- a/arch/riscv/kvm/main.c
> > > > > > > > > +++ b/arch/riscv/kvm/main.c
> > > > > > > > > @@ -22,22 +22,8 @@ long kvm_arch_dev_ioctl(struct file *filp,
> > > > > > > > >
> > > > > > > > >  int kvm_arch_hardware_enable(void)
> > > > > > > > >  {
> > > > > > > > > -       unsigned long hideleg, hedeleg;
> > > > > > > > > -
> > > > > > > > > -       hedeleg = 0;
> > > > > > > > > -       hedeleg |= (1UL << EXC_INST_MISALIGNED);
> > > > > > > > > -       hedeleg |= (1UL << EXC_BREAKPOINT);
> > > > > > > > > -       hedeleg |= (1UL << EXC_SYSCALL);
> > > > > > > > > -       hedeleg |= (1UL << EXC_INST_PAGE_FAULT);
> > > > > > > > > -       hedeleg |= (1UL << EXC_LOAD_PAGE_FAULT);
> > > > > > > > > -       hedeleg |= (1UL << EXC_STORE_PAGE_FAULT);
> > > > > > > > > -       csr_write(CSR_HEDELEG, hedeleg);
> > > > > > > > > -
> > > > > > > > > -       hideleg = 0;
> > > > > > > > > -       hideleg |= (1UL << IRQ_VS_SOFT);
> > > > > > > > > -       hideleg |= (1UL << IRQ_VS_TIMER);
> > > > > > > > > -       hideleg |= (1UL << IRQ_VS_EXT);
> > > > > > > > > -       csr_write(CSR_HIDELEG, hideleg);
> > > > > > > > > +       csr_write(CSR_HEDELEG, KVM_HEDELEG_DEFAULT);
> > > > > > > > > +       csr_write(CSR_HIDELEG, KVM_HIDELEG_DEFAULT);
> > > > > > > > >
> > > > > > > > >         /* VS should access only the time counter directly. Everything else should trap */
> > > > > > > > >         csr_write(CSR_HCOUNTEREN, 0x02);
> > > > > > > > > diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > > > > > index b5ca9f2e98ac..242076c2227f 100644
> > > > > > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > > > > > @@ -475,8 +475,15 @@ int kvm_arch_vcpu_ioctl_set_mpstate(struct kvm_vcpu *vcpu,
> > > > > > > > >  int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> > > > > > > > >                                         struct kvm_guest_debug *dbg)
> > > > > > > > >  {
> > > > > > > > > -       /* TODO; To be implemented later. */
> > > > > > > > > -       return -EINVAL;
> > > > > > > >
> > > > > > > > if (vcpu->arch.ran_atleast_once)
> > > > > > > >         return -EBUSY;
> > > > > > >
> > > > > > > If we enabled the guest_debug in QEMU side, then the KVM_SET_GUEST_DEBUG ioctl
> > > > > > > will come before the first KVM_RUN. This will always cause an ERROR.
> > > > > >
> > > > > > The check ensures that KVM user space can only enable/disable
> > > > > > guest debug before the VCPU is run. I don't see why this would
> > > > > > fail for QEMU.
> > > > >
> > > > > In the current implementation of GDB and QEMU, the userspace will enable/disable
> > > > > guest_debug frequently during the debugging (almost every step).
> > > > > The sequence should like:
> > > > >
> > > > > KVM_SET_GUEST_DEBUG enable
> > > > > KVM_RUN
> > > > > KVM_SET_GUEST_DEBUG disable
> > > > > KVM_SET_GUEST_DEBUG enable
> > > > > KVM_RUN
> > > > > KVM_SET_GUEST_DEBUG disable
> > > > > KVM_SET_GUEST_DEBUG enable
> > > > > KVM_RUN
> > > > > KVM_SET_GUEST_DEBUG disable
> > > > > ...
> > > >
> > > > Fair enough, no need to check "ran_atleast_once"
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > > +       if (dbg->control & KVM_GUESTDBG_ENABLE) {
> > > > > > > > > +               vcpu->guest_debug = dbg->control;
> > > > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_GUEST_DEBUG;
> > > > > > > > > +       } else {
> > > > > > > > > +               vcpu->guest_debug = 0;
> > > > > > > > > +               vcpu->arch.cfg.hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > > > +       }
> > > > > > > >
> > > > > > > > Don't update vcpu->arch.cfg.hedeleg here since it should be only done
> > > > > > > > in kvm_riscv_vcpu_setup_config().
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > +       return 0;
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > > > > @@ -505,6 +512,9 @@ static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > > > >                 if (riscv_isa_extension_available(isa, SMSTATEEN))
> > > > > > > > >                         cfg->hstateen0 |= SMSTATEEN0_SSTATEEN0;
> > > > > > > > >         }
> > > > > > > > > +
> > > > > > > > > +       if (!vcpu->guest_debug)
> > > > > > > > > +               cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > >
> > > > > > > > This should be:
> > > > > > > >
> > > > > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT;
> > > > > > > > if (vcpu->guest_debug)
> > > > > > > >         cfg->hedeleg |= BIT(EXC_BREAKPOINT);
> > > > > > >
> > > > > > > Like above, here the logic should be:
> > > > > > >
> > > > > > > cfg->hedeleg = KVM_HEDELEG_DEFAULT; // with BIT(EXC_BREAKPOINT)
> > > > > > > if (vcpu->guest_debug)
> > > > > > >         cfg->hedeleg &= ~BIT(EXC_BREAKPOINT);
> > > > > > >
> > > > > > > Another approach is:
> > > > > > > initialize the cfg->hedeleg as KVM_HEDELEG_DEFAULT during kvm_arch_vcpu_create().
> > > > > > > Besides that, only update the cfg->hedeleg in kvm_arch_vcpu_ioctl_set_guest_debug().
> > > > > >
> > > > > > I disagree. We should handle hedeleg just like we handle henvcfg.
> > > > >
> > > > > OK, let's only update the cfg->hedeleg in kvm_riscv_vcpu_setup_config().
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > >  void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > > > > @@ -519,6 +529,7 @@ void kvm_arch_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
> > > > > > > > >         csr_write(CSR_VSEPC, csr->vsepc);
> > > > > > > > >         csr_write(CSR_VSCAUSE, csr->vscause);
> > > > > > > > >         csr_write(CSR_VSTVAL, csr->vstval);
> > > > > > > > > +       csr_write(CSR_HEDELEG, cfg->hedeleg);
> > > > > > > > >         csr_write(CSR_HVIP, csr->hvip);
> > > > > > > > >         csr_write(CSR_VSATP, csr->vsatp);
> > > > > > > > >         csr_write(CSR_HENVCFG, cfg->henvcfg);
> > > > > > > > > diff --git a/arch/riscv/kvm/vm.c b/arch/riscv/kvm/vm.c
> > > > > > > > > index ce58bc48e5b8..7396b8654f45 100644
> > > > > > > > > --- a/arch/riscv/kvm/vm.c
> > > > > > > > > +++ b/arch/riscv/kvm/vm.c
> > > > > > > > > @@ -186,6 +186,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> > > > > > > > >         case KVM_CAP_READONLY_MEM:
> > > > > > > > >         case KVM_CAP_MP_STATE:
> > > > > > > > >         case KVM_CAP_IMMEDIATE_EXIT:
> > > > > > > > > +       case KVM_CAP_SET_GUEST_DEBUG:
> > > > > > > > >                 r = 1;
> > > > > > > > >                 break;
> > > > > > > > >         case KVM_CAP_NR_VCPUS:
> > > > > > > > > --
> > > > > > > > > 2.17.1
> > > > > > > > >
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Anup
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Chao
> > > > > >
> > > > > > Regards,
> > > > > > Anup
> > > > >
> > > > > Thanks,
> > > > > Chao
> > > > > --
> > > > > kvm-riscv mailing list
> > > > > kvm-riscv@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > > >
> > > > Regards,
> > > > Anup