Message-ID: <CAAhSdy20pq3KvbCeST=h+O5PWfs2E4uXpX9BbbzE7GJzn+pzkA@mail.gmail.com>
Date: Wed, 30 Apr 2025 10:56:35 +0530
From: Anup Patel <anup@...infault.org>
To: Radim Krčmář <rkrcmar@...tanamicro.com>
Cc: Anup Patel <apatel@...tanamicro.com>, kvm-riscv@...ts.infradead.org, 
	kvm@...r.kernel.org, linux-riscv@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, Atish Patra <atishp@...shpatra.org>, 
	Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt <palmer@...belt.com>, 
	Albert Ou <aou@...s.berkeley.edu>, Alexandre Ghiti <alex@...ti.fr>, 
	Andrew Jones <ajones@...tanamicro.com>, Mayuresh Chitale <mchitale@...tanamicro.com>
Subject: Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@...infault.org> wrote:
>
> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
> >
> > 2025-04-29T20:31:18+05:30, Anup Patel <anup@...infault.org>:
> > > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
> > >>
> > >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@...tanamicro.com>:
> > >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
> > >> >>
> > >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@...infault.org>:
> > >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
> > >> >> >> For a cleaner solution, we should add interfaces to perform the KVM-SBI
> > >> >> >> reset request on userspace demand.  I think it would also be much better
> > >> >> >> if userspace was in control of the post-reset state.
> > >> >> >
> > >> >> > Apart from breaking KVM user-space, this patch is incorrect and
> > >> >> > does not align with the:
> > >> >> > 1) SBI spec
> > >> >> > 2) OS boot protocol.
> > >> >> >
> > >> >> > The SBI spec only defines the entry state of certain CPU registers
> > >> >> > (namely, PC, A0, and A1) when CPU enters S-mode:
> > >> >> > 1) Upon SBI HSM start call from some other CPU
> > >> >> > 2) Upon resuming from non-retentive SBI HSM suspend or
> > >> >> >     SBI system suspend
> > >> >> >
> > >> >> > The S-mode entry state of the boot CPU is defined by the
> > >> >> > OS boot protocol and not by the SBI spec. Due to this, reason
> > >> >> > KVM RISC-V expects user-space to set up the S-mode entry
> > >> >> > state of the boot CPU upon system reset.
> > >> >>
> > >> >> We can handle the initial state consistency in other patches.
> > >> >> What needs addressing is a way to trigger the KVM reset from userspace,
> > >> >> even if only to clear the internal KVM state.
> > >> >>
> > >> >> I think mp_state is currently the best signalization that KVM should
> > >> >> reset, so I added it there.
> > >> >>
> > >> >> What would be your preferred interface for that?
> > >> >>
> > >> >
> > >> > Instead of creating a new interface, I would prefer that VCPU
> > >> > which initiates SBI System Reset should be resetted immediately
> > >> > in-kernel space before forwarding the system reset request to
> > >> > user space.
> > >>
> > >> The initiating VCPU might not be the boot VCPU.
> > >> It would be safer to reset all of them.
> > >
> > > I meant initiating VCPU and not the boot VCPU. Currently, the
> > > non-initiating VCPUs are already resetted by VCPU requests
> > > so nothing special needs to be done.
>
> There is no designated boot VCPU for KVM so let us only use the
> term "initiating" or "non-initiating" VCPUs in context of system reset.
>
> >
> > Currently, we make the request only for VCPUs brought up by HSM -- the
> > non-boot VCPUs.  There is a single VCPU not being reset and resetting
> > the reset initiating VCPU changes nothing. e.g.
> >
> >   1) VCPU 1 initiates the reset through an ecall.
> >   2) All VCPUs are stopped and return to userspace.
>
> When all VCPUs are stopped, all VCPUs except VCPU1
> (in this example) will SLEEP because we do
> "kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP)"
> so none of the VCPUs except VCPU1 (in this case) will
> return to userspace.
>
> >   3) Userspace prepares VCPU 0 as the boot VCPU.
> >   4) VCPU 0 executes without going through KVM reset paths.
>
> Userspace will see a system reset event exit for the
> initiating VCPU by that time all other VCPUs are already
> sleeping with mp_state == KVM_MP_STATE_STOPPED.
>
> >
> > The point of this patch is to reset the boot VCPU, so we reset the VCPU
> > that is made runnable by the KVM_SET_MP_STATE IOCTL.
>
> Like I said before, we don't need to do this. The initiating VCPU
> can be resetted just before exiting to user space for system reset
> event exit.
>

Below is what I am suggesting. This change completely removes
dependency of kvm_sbi_hsm_vcpu_start() on "reset" structures.

diff --git a/arch/riscv/include/asm/kvm_host.h
b/arch/riscv/include/asm/kvm_host.h
index 0e9c2fab6378..6bd12469852d 100644
--- a/arch/riscv/include/asm/kvm_host.h
+++ b/arch/riscv/include/asm/kvm_host.h
@@ -396,6 +396,7 @@ int kvm_riscv_vcpu_get_reg(struct kvm_vcpu *vcpu,
 int kvm_riscv_vcpu_set_reg(struct kvm_vcpu *vcpu,
                const struct kvm_one_reg *reg);

+void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu);
 int kvm_riscv_vcpu_set_interrupt(struct kvm_vcpu *vcpu, unsigned int irq);
 int kvm_riscv_vcpu_unset_interrupt(struct kvm_vcpu *vcpu, unsigned int irq);
 void kvm_riscv_vcpu_flush_interrupts(struct kvm_vcpu *vcpu);
diff --git a/arch/riscv/kvm/vcpu.c b/arch/riscv/kvm/vcpu.c
index 02635bac91f1..801c6a1a1aef 100644
--- a/arch/riscv/kvm/vcpu.c
+++ b/arch/riscv/kvm/vcpu.c
@@ -51,7 +51,7 @@ const struct kvm_stats_header kvm_vcpu_stats_header = {
                sizeof(kvm_vcpu_stats_desc),
 };

-static void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
+void kvm_riscv_reset_vcpu(struct kvm_vcpu *vcpu)
 {
     struct kvm_vcpu_csr *csr = &vcpu->arch.guest_csr;
     struct kvm_vcpu_csr *reset_csr = &vcpu->arch.guest_reset_csr;
@@ -689,6 +689,9 @@ static void kvm_riscv_check_vcpu_requests(struct
kvm_vcpu *vcpu)
     struct rcuwait *wait = kvm_arch_vcpu_get_wait(vcpu);

     if (kvm_request_pending(vcpu)) {
+        if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
+            kvm_riscv_reset_vcpu(vcpu);
+
         if (kvm_check_request(KVM_REQ_SLEEP, vcpu)) {
             kvm_vcpu_srcu_read_unlock(vcpu);
             rcuwait_wait_event(wait,
@@ -705,9 +708,6 @@ static void kvm_riscv_check_vcpu_requests(struct
kvm_vcpu *vcpu)
             }
         }

-        if (kvm_check_request(KVM_REQ_VCPU_RESET, vcpu))
-            kvm_riscv_reset_vcpu(vcpu);
-
         if (kvm_check_request(KVM_REQ_UPDATE_HGATP, vcpu))
             kvm_riscv_gstage_update_hgatp(vcpu);

diff --git a/arch/riscv/kvm/vcpu_sbi.c b/arch/riscv/kvm/vcpu_sbi.c
index d1c83a77735e..79477e7f240a 100644
--- a/arch/riscv/kvm/vcpu_sbi.c
+++ b/arch/riscv/kvm/vcpu_sbi.c
@@ -146,9 +146,15 @@ void kvm_riscv_vcpu_sbi_system_reset(struct kvm_vcpu *vcpu,
         spin_lock(&vcpu->arch.mp_state_lock);
         WRITE_ONCE(tmp->arch.mp_state.mp_state, KVM_MP_STATE_STOPPED);
         spin_unlock(&vcpu->arch.mp_state_lock);
+        if (tmp != vcpu) {
+            kvm_make_request(KVM_REQ_SLEEP | KVM_REQ_VCPU_RESET, vcpu);
+            kvm_vcpu_kick(vcpu);
+        } else {
+            kvm_make_request(KVM_REQ_SLEEP, vcpu);
+        }
     }
-    kvm_make_all_cpus_request(vcpu->kvm, KVM_REQ_SLEEP);

+    kvm_riscv_reset_vcpu(vcpu);
     memset(&run->system_event, 0, sizeof(run->system_event));
     run->system_event.type = type;
     run->system_event.ndata = 1;
diff --git a/arch/riscv/kvm/vcpu_sbi_hsm.c b/arch/riscv/kvm/vcpu_sbi_hsm.c
index 3070bb31745d..30d7d59db5a5 100644
--- a/arch/riscv/kvm/vcpu_sbi_hsm.c
+++ b/arch/riscv/kvm/vcpu_sbi_hsm.c
@@ -15,15 +15,15 @@

 static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
 {
-    struct kvm_cpu_context *reset_cntx;
-    struct kvm_cpu_context *cp = &vcpu->arch.guest_context;
-    struct kvm_vcpu *target_vcpu;
+    struct kvm_cpu_context *target_cp, *cp = &vcpu->arch.guest_context;
     unsigned long target_vcpuid = cp->a0;
+    struct kvm_vcpu *target_vcpu;
     int ret = 0;

     target_vcpu = kvm_get_vcpu_by_id(vcpu->kvm, target_vcpuid);
     if (!target_vcpu)
         return SBI_ERR_INVALID_PARAM;
+    target_cp = &target_vcpu->arch.guest_context;

     spin_lock(&target_vcpu->arch.mp_state_lock);

@@ -32,17 +32,12 @@ static int kvm_sbi_hsm_vcpu_start(struct kvm_vcpu *vcpu)
         goto out;
     }

-    spin_lock(&target_vcpu->arch.reset_cntx_lock);
-    reset_cntx = &target_vcpu->arch.guest_reset_context;
     /* start address */
-    reset_cntx->sepc = cp->a1;
+    target_cp->sepc = cp->a1;
     /* target vcpu id to start */
-    reset_cntx->a0 = target_vcpuid;
+    target_cp->a0 = target_vcpuid;
     /* private data passed from kernel */
-    reset_cntx->a1 = cp->a2;
-    spin_unlock(&target_vcpu->arch.reset_cntx_lock);
-
-    kvm_make_request(KVM_REQ_VCPU_RESET, target_vcpu);
+    target_cp->a1 = cp->a2;

     __kvm_riscv_vcpu_power_on(target_vcpu);

@@ -63,6 +58,7 @@ static int kvm_sbi_hsm_vcpu_stop(struct kvm_vcpu *vcpu)
         goto out;
     }

+    kvm_make_request(KVM_REQ_VCPU_RESET, vcpu);
     __kvm_riscv_vcpu_power_off(vcpu);

 out:

Regards,
Anup

Powered by blists - more mailing lists