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

[PATCH v1 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-rc2 and also available at:
https://github.com/Du-Chao/kvm-riscv/tree/guest_debug_sw

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

Changes from RFC->v1:
- Rebased on Linux 6.8-rc2.
- Merge PATCH1 and PATCH2 into one patch. (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)
- kselftest case added.

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/uapi/asm/kvm.h             |  1 +
 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 +++++++++++++++++++
 6 files changed, 69 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/kvm/riscv/breakpoints.c

--
2.17.1

[PATCH v1 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
being 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 VM is being debugged by userspace, by clearing the corresponding
bit in hedeleg CSR.

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

diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
index b5ca9f2e98ac..6cee974592ac 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -475,8 +475,19 @@ 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) {
+		if (vcpu->guest_debug != dbg->control) {
+			vcpu->guest_debug = dbg->control;
+			csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
+		}
+	} else {
+		if (vcpu->guest_debug != 0) {
+			vcpu->guest_debug = 0;
+			csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
+		}
+	}
+
+	return 0;
 }

 static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
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 v1 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 v1 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..8ca09a493460
--- /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 v1 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Anup Patel]

On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
>
> kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> being 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 VM is being debugged by userspace, by clearing the corresponding
> bit in hedeleg CSR.
>
> Signed-off-by: Chao Du <duchao@eswincomputing.com>
> ---
>  arch/riscv/include/uapi/asm/kvm.h |  1 +
>  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
>  arch/riscv/kvm/vm.c               |  1 +
>  3 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> index b5ca9f2e98ac..6cee974592ac 100644
> --- a/arch/riscv/kvm/vcpu.c
> +++ b/arch/riscv/kvm/vcpu.c
> @@ -475,8 +475,19 @@ 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) {
> +               if (vcpu->guest_debug != dbg->control) {
> +                       vcpu->guest_debug = dbg->control;
> +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> +               }
> +       } else {
> +               if (vcpu->guest_debug != 0) {
> +                       vcpu->guest_debug = 0;
> +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> +               }
> +       }

This is broken because directly setting breakpoint exception delegation
in CSR also affects other VCPUs running on the same host CPU.

To address the above, we should do the following:
1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
   is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
   EXC_BREAKPOINT bit.
2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
    EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
     HEDELEG csr before entering the Guest/VM.

Regards,
Anup

> +
> +       return 0;
>  }
>
>  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> 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
>
>
> --
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Anup Patel]

On Tue, Feb 6, 2024 at 1:22 PM 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>

Looks good to me.

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

Regards,
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 v1 1/3] RISC-V: KVM: Implement kvm_arch_vcpu_ioctl_set_guest_debug()

[Chao Du]

On 2024-02-14 21:19, Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > being 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 VM is being debugged by userspace, by clearing the corresponding
> > bit in hedeleg CSR.
> >
> > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > ---
> >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> >  arch/riscv/kvm/vm.c               |  1 +
> >  3 files changed, 15 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> > index b5ca9f2e98ac..6cee974592ac 100644
> > --- a/arch/riscv/kvm/vcpu.c
> > +++ b/arch/riscv/kvm/vcpu.c
> > @@ -475,8 +475,19 @@ 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) {
> > +               if (vcpu->guest_debug != dbg->control) {
> > +                       vcpu->guest_debug = dbg->control;
> > +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > +               }
> > +       } else {
> > +               if (vcpu->guest_debug != 0) {
> > +                       vcpu->guest_debug = 0;
> > +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > +               }
> > +       }
> 
> This is broken because directly setting breakpoint exception delegation
> in CSR also affects other VCPUs running on the same host CPU.
> 
> To address the above, we should do the following:
> 1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
>    is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
>    EXC_BREAKPOINT bit.
> 2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
>     EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
> 3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
>      HEDELEG csr before entering the Guest/VM.
> 
> Regards,
> Anup
> 

Thanks for the review and detailed suggestion.
Maybe we could make it a bit easier:
1) The kvm_arch_vcpu_ioctl_set_guest_debug() only update vcpu->guest_debug
   accordingly.
2) The kvm_riscv_vcpu_swap_in_guest_state() check vcpu->guest_debug and
   set/clear the HEDELEG csr accordingly.

Could you confirm if this is OK?
If yes, I will post another revision.

Regards,
Chao

> > +
> > +       return 0;
> >  }
> >
> >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > 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
> >
> >
> > --
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Anup Patel]

On Tue, Feb 20, 2024 at 8:31 AM Chao Du <duchao@eswincomputing.com> wrote:
>
> On 2024-02-14 21:19, Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > being 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 VM is being debugged by userspace, by clearing the corresponding
> > > bit in hedeleg CSR.
> > >
> > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > ---
> > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > >  arch/riscv/kvm/vm.c               |  1 +
> > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > index b5ca9f2e98ac..6cee974592ac 100644
> > > --- a/arch/riscv/kvm/vcpu.c
> > > +++ b/arch/riscv/kvm/vcpu.c
> > > @@ -475,8 +475,19 @@ 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) {
> > > +               if (vcpu->guest_debug != dbg->control) {
> > > +                       vcpu->guest_debug = dbg->control;
> > > +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > +               }
> > > +       } else {
> > > +               if (vcpu->guest_debug != 0) {
> > > +                       vcpu->guest_debug = 0;
> > > +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > +               }
> > > +       }
> >
> > This is broken because directly setting breakpoint exception delegation
> > in CSR also affects other VCPUs running on the same host CPU.
> >
> > To address the above, we should do the following:
> > 1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
> >    is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
> >    EXC_BREAKPOINT bit.
> > 2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
> >     EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
> > 3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
> >      HEDELEG csr before entering the Guest/VM.
> >
> > Regards,
> > Anup
> >
>
> Thanks for the review and detailed suggestion.
> Maybe we could make it a bit easier:
> 1) The kvm_arch_vcpu_ioctl_set_guest_debug() only update vcpu->guest_debug
>    accordingly.
> 2) The kvm_riscv_vcpu_swap_in_guest_state() check vcpu->guest_debug and
>    set/clear the HEDELEG csr accordingly.
>
> Could you confirm if this is OK?

Your suggestion will work but it adds an additional "if ()" check in
kvm_riscv_vcpu_swap_in_guest_state() which is in the hot path.

I am still leaning towards what I suggested.

Regards,
Anup

> If yes, I will post another revision.
>
> Regards,
> Chao
>
> > > +
> > > +       return 0;
> > >  }
> > >
> > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > 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
> > >
> > >
> > > --
> > > kvm-riscv mailing list
> > > kvm-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> --
> kvm-riscv mailing list
> kvm-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Chao Du]

On 2024-02-20 12:54, Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Tue, Feb 20, 2024 at 8:31 AM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > On 2024-02-14 21:19, Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > >
> > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > being 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 VM is being debugged by userspace, by clearing the corresponding
> > > > bit in hedeleg CSR.
> > > >
> > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > ---
> > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > >  arch/riscv/kvm/vm.c               |  1 +
> > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > index b5ca9f2e98ac..6cee974592ac 100644
> > > > --- a/arch/riscv/kvm/vcpu.c
> > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > @@ -475,8 +475,19 @@ 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) {
> > > > +               if (vcpu->guest_debug != dbg->control) {
> > > > +                       vcpu->guest_debug = dbg->control;
> > > > +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > +               }
> > > > +       } else {
> > > > +               if (vcpu->guest_debug != 0) {
> > > > +                       vcpu->guest_debug = 0;
> > > > +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > +               }
> > > > +       }
> > >
> > > This is broken because directly setting breakpoint exception delegation
> > > in CSR also affects other VCPUs running on the same host CPU.
> > >
> > > To address the above, we should do the following:
> > > 1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
> > >    is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
> > >    EXC_BREAKPOINT bit.
> > > 2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
> > >     EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
> > > 3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
> > >      HEDELEG csr before entering the Guest/VM.
> > >
> > > Regards,
> > > Anup
> > >
> >
> > Thanks for the review and detailed suggestion.
> > Maybe we could make it a bit easier:
> > 1) The kvm_arch_vcpu_ioctl_set_guest_debug() only update vcpu->guest_debug
> >    accordingly.
> > 2) The kvm_riscv_vcpu_swap_in_guest_state() check vcpu->guest_debug and
> >    set/clear the HEDELEG csr accordingly.
> >
> > Could you confirm if this is OK?
> 
> Your suggestion will work but it adds an additional "if ()" check in
> kvm_riscv_vcpu_swap_in_guest_state() which is in the hot path.
> 

Yes, it makes sense.
I will prepare a V2 patch.

Thanks,
Chao

> I am still leaning towards what I suggested.
> 
> Regards,
> Anup
> 
> > If yes, I will post another revision.
> >
> > Regards,
> > Chao
> >
> > > > +
> > > > +       return 0;
> > > >  }
> > > >
> > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > 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
> > > >
> > > >
> > > > --
> > > > kvm-riscv mailing list
> > > > kvm-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > --
> > kvm-riscv mailing list
> > kvm-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Anup Patel]

On Wed, Feb 21, 2024 at 7:28 AM Chao Du <duchao@eswincomputing.com> wrote:
>
> On 2024-02-20 12:54, Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Tue, Feb 20, 2024 at 8:31 AM Chao Du <duchao@eswincomputing.com> wrote:
> > >
> > > On 2024-02-14 21:19, Anup Patel <apatel@ventanamicro.com> wrote:
> > > >
> > > > On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > >
> > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > being 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 VM is being debugged by userspace, by clearing the corresponding
> > > > > bit in hedeleg CSR.
> > > > >
> > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > ---
> > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > index b5ca9f2e98ac..6cee974592ac 100644
> > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > @@ -475,8 +475,19 @@ 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) {
> > > > > +               if (vcpu->guest_debug != dbg->control) {
> > > > > +                       vcpu->guest_debug = dbg->control;
> > > > > +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > > +               }
> > > > > +       } else {
> > > > > +               if (vcpu->guest_debug != 0) {
> > > > > +                       vcpu->guest_debug = 0;
> > > > > +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > > +               }
> > > > > +       }
> > > >
> > > > This is broken because directly setting breakpoint exception delegation
> > > > in CSR also affects other VCPUs running on the same host CPU.
> > > >
> > > > To address the above, we should do the following:
> > > > 1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
> > > >    is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
> > > >    EXC_BREAKPOINT bit.
> > > > 2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
> > > >     EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
> > > > 3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
> > > >      HEDELEG csr before entering the Guest/VM.
> > > >
> > > > Regards,
> > > > Anup
> > > >
> > >
> > > Thanks for the review and detailed suggestion.
> > > Maybe we could make it a bit easier:
> > > 1) The kvm_arch_vcpu_ioctl_set_guest_debug() only update vcpu->guest_debug
> > >    accordingly.
> > > 2) The kvm_riscv_vcpu_swap_in_guest_state() check vcpu->guest_debug and
> > >    set/clear the HEDELEG csr accordingly.
> > >
> > > Could you confirm if this is OK?
> >
> > Your suggestion will work but it adds an additional "if ()" check in
> > kvm_riscv_vcpu_swap_in_guest_state() which is in the hot path.
> >
>
> Yes, it makes sense.
> I will prepare a V2 patch.

I just realized that kvm_arch_vcpu_load() will be a much
better place to add hedeleg CSR write instead of
kvm_riscv_vcpu_swap_in_guest_state(). This way
it will be only at the start hot-path (aka run-loop).

Regards,
Anup

>
> Thanks,
> Chao
>
> > I am still leaning towards what I suggested.
> >
> > Regards,
> > Anup
> >
> > > If yes, I will post another revision.
> > >
> > > Regards,
> > > Chao
> > >
> > > > > +
> > > > > +       return 0;
> > > > >  }
> > > > >
> > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > 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
> > > > >
> > > > >
> > > > > --
> > > > > kvm-riscv mailing list
> > > > > kvm-riscv@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > > --
> > > kvm-riscv mailing list
> > > kvm-riscv@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/kvm-riscv

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

[Chao Du]

On 2024-02-21 12:19, Anup Patel <apatel@ventanamicro.com> wrote:
> 
> On Wed, Feb 21, 2024 at 7:28 AM Chao Du <duchao@eswincomputing.com> wrote:
> >
> > On 2024-02-20 12:54, Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Tue, Feb 20, 2024 at 8:31 AM Chao Du <duchao@eswincomputing.com> wrote:
> > > >
> > > > On 2024-02-14 21:19, Anup Patel <apatel@ventanamicro.com> wrote:
> > > > >
> > > > > On Tue, Feb 6, 2024 at 1:22 PM Chao Du <duchao@eswincomputing.com> wrote:
> > > > > >
> > > > > > kvm_vm_ioctl_check_extension(): Return 1 if KVM_CAP_SET_GUEST_DEBUG is
> > > > > > being 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 VM is being debugged by userspace, by clearing the corresponding
> > > > > > bit in hedeleg CSR.
> > > > > >
> > > > > > Signed-off-by: Chao Du <duchao@eswincomputing.com>
> > > > > > ---
> > > > > >  arch/riscv/include/uapi/asm/kvm.h |  1 +
> > > > > >  arch/riscv/kvm/vcpu.c             | 15 +++++++++++++--
> > > > > >  arch/riscv/kvm/vm.c               |  1 +
> > > > > >  3 files changed, 15 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/riscv/include/uapi/asm/kvm.h b/arch/riscv/include/uapi/asm/kvm.h
> > > > > > index d6b7a5b95874..8890977836f0 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/vcpu.c b/arch/riscv/kvm/vcpu.c
> > > > > > index b5ca9f2e98ac..6cee974592ac 100644
> > > > > > --- a/arch/riscv/kvm/vcpu.c
> > > > > > +++ b/arch/riscv/kvm/vcpu.c
> > > > > > @@ -475,8 +475,19 @@ 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) {
> > > > > > +               if (vcpu->guest_debug != dbg->control) {
> > > > > > +                       vcpu->guest_debug = dbg->control;
> > > > > > +                       csr_clear(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > > > +               }
> > > > > > +       } else {
> > > > > > +               if (vcpu->guest_debug != 0) {
> > > > > > +                       vcpu->guest_debug = 0;
> > > > > > +                       csr_set(CSR_HEDELEG, BIT(EXC_BREAKPOINT));
> > > > > > +               }
> > > > > > +       }
> > > > >
> > > > > This is broken because directly setting breakpoint exception delegation
> > > > > in CSR also affects other VCPUs running on the same host CPU.
> > > > >
> > > > > To address the above, we should do the following:
> > > > > 1) Add "unsigned long hedeleg" in "struct kvm_vcpu_config" which
> > > > >    is pre-initialized in kvm_riscv_vcpu_setup_config() without setting
> > > > >    EXC_BREAKPOINT bit.
> > > > > 2) The kvm_arch_vcpu_ioctl_set_guest_debug() should only set/clear
> > > > >     EXC_BREAKPOINT bit in "hedeleg" of "struct kvm_vcpu_config".
> > > > > 3) The kvm_riscv_vcpu_swap_in_guest_state() must write the
> > > > >      HEDELEG csr before entering the Guest/VM.
> > > > >
> > > > > Regards,
> > > > > Anup
> > > > >
> > > >
> > > > Thanks for the review and detailed suggestion.
> > > > Maybe we could make it a bit easier:
> > > > 1) The kvm_arch_vcpu_ioctl_set_guest_debug() only update vcpu->guest_debug
> > > >    accordingly.
> > > > 2) The kvm_riscv_vcpu_swap_in_guest_state() check vcpu->guest_debug and
> > > >    set/clear the HEDELEG csr accordingly.
> > > >
> > > > Could you confirm if this is OK?
> > >
> > > Your suggestion will work but it adds an additional "if ()" check in
> > > kvm_riscv_vcpu_swap_in_guest_state() which is in the hot path.
> > >
> >
> > Yes, it makes sense.
> > I will prepare a V2 patch.
> 
> I just realized that kvm_arch_vcpu_load() will be a much
> better place to add hedeleg CSR write instead of
> kvm_riscv_vcpu_swap_in_guest_state(). This way
> it will be only at the start hot-path (aka run-loop).

OK, I'm also considering doing so.

> 
> Regards,
> Anup
> 
> >
> > Thanks,
> > Chao
> >
> > > I am still leaning towards what I suggested.
> > >
> > > Regards,
> > > Anup
> > >
> > > > If yes, I will post another revision.
> > > >
> > > > Regards,
> > > > Chao
> > > >
> > > > > > +
> > > > > > +       return 0;
> > > > > >  }
> > > > > >
> > > > > >  static void kvm_riscv_vcpu_setup_config(struct kvm_vcpu *vcpu)
> > > > > > 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
> > > > > >
> > > > > >
> > > > > > --
> > > > > > kvm-riscv mailing list
> > > > > > kvm-riscv@lists.infradead.org
> > > > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv
> > > > --
> > > > kvm-riscv mailing list
> > > > kvm-riscv@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/kvm-riscv