Message-Id: <D9J9DW53Q2GD.1PB647ISOCXRX@ventanamicro.com>
Date: Tue, 29 Apr 2025 18:21:33 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "Anup Patel" <anup@...infault.org>
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

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.

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.
  3) Userspace prepares VCPU 0 as the boot VCPU.
  4) VCPU 0 executes without going through KVM reset paths.

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.

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.

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

Powered by blists - more mailing lists