Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming runnable

["Radim Krčmář" <rkrcmar@ventanamicro.com>]

2025-04-30T10:56:35+05:30, Anup Patel <anup@brainfault.org>:
> On Wed, Apr 30, 2025 at 9:52 AM Anup Patel <anup@brainfault.org> wrote:
>>
>> On Tue, Apr 29, 2025 at 9:51 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> >
>> > 2025-04-29T20:31:18+05:30, Anup Patel <anup@brainfault.org>:
>> > > On Tue, Apr 29, 2025 at 3:55 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> > >>
>> > >> 2025-04-29T11:25:35+05:30, Anup Patel <apatel@ventanamicro.com>:
>> > >> > On Mon, Apr 28, 2025 at 11:15 PM Radim Krčmář <rkrcmar@ventanamicro.com> wrote:
>> > >> >>
>> > >> >> 2025-04-28T17:52:25+05:30, Anup Patel <anup@brainfault.org>:
>> > >> >> > On Thu, Apr 3, 2025 at 5:02 PM Radim Krčmář <rkrcmar@ventanamicro.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.

That is exactly how I use it.  Some VCPU will be the boot VCPU (the VCPU
made runnable by KVM_SET_MP_STATE) and loaded with state from userspace.

RISC-V doesn't guarantee that the boot VCPU is the reset initiating
VCPU, so I think KVM should allow it.

>> > 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.

Userspace should be able to do whatever it likes -- in my example, all
the VCPUs are brought to userspace and a different boot VCPU is
selected.

(Perhaps userspace wanted to record their reset pre-reset state, or
 maybe it really wants to boot with a designated VCPU.)

>> >   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.

You assume initiating VCPU == boot VCPU.

We should prevent KVM_SET_MP_STATE IOCTL for all non-initiating VCPUs if
we decide to accept the assumption.

I'd rather choose a different design, though.

How about a new userspace interface for IOCTL reset?
(Can be capability toggle for KVM_SET_MP_STATE or a straight new IOCTL.)

That wouldn't "fix" current userspaces, but would significantly improve
the sanity of the KVM interface.

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

I'd keep the reset structure in this series -- it's small enough and
locklessly accessing the state of another VCPU needs a lot of
consideration to prevent all possible race conditions.

Thanks.