Message-ID: <CAAhSdy0B-pF-jHmTXNYE7NXwdCWJepDtGR__S+P4MhZ1bfUERQ@mail.gmail.com>
Date: Wed, 30 Apr 2025 09:52:43 +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 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.

>
> For design alternatives, it is also possible to reset immediately in an
> IOCTL instead of making the reset request.
>
> >> You also previously mentioned that we need to preserve the pre-reset
> >> state for userspace, which I completely agree with and it is why the
> >> reset happens later.
> >
> > Yes, that was only for debug purposes from user space. At the
> > moment, there is no one using this for debug purposes so we
> > can sacrifice that.
>
> We still can't immediately reset the boot VCPU, because it might already
> be in userspace.  We don't really benefit from immediately resetting the
> initiating VCPU.
> Also, making the reset request for all VCPUs from the initiating VCPU
> has some undesirable race conditions we would have to prevent, so I do
> prefer we go the IOCTL reset way.

All VCPUs are sleeping with mp_state == KVM_MP_STATE_STOPPED
when userspace sees system reset exit on the initiating VCPU so I don't
see any race condition if we also reset the initiating VCPU before exiting
to userspace.

>
> >> >             This way we also force KVM user-space to explicitly
> >> > set the PC, A0, and A1 before running the VCPU again after
> >> > system reset.
> >>
> >> We also want to consider reset from emulation outside of KVM.
> >>
> >> There is a "simple" solution that covers everything (except speed) --
> >> the userspace can tear down the whole VM and re-create it.
> >> Do we want to do this instead and drop all resets from KVM?
> >
> > I think we should keep the VCPU resets in KVM so that handling
> > of system reset handling in user space remains simple. The user
> > space can also re-create the VM upon system reset but that is
> > user space choice.
>
> Ok.

Regards,
Anup

Powered by blists - more mailing lists