Message-Id: <D9FUIXQ1FIHS.2BLRCEWLXZL45@ventanamicro.com>
Date: Fri, 25 Apr 2025 18:04:52 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "Andrew Jones" <ajones@...tanamicro.com>
Cc: <kvm-riscv@...ts.infradead.org>, <kvm@...r.kernel.org>,
 <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>, "Anup
 Patel" <anup@...infault.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>, "Mayuresh Chitale" <mchitale@...tanamicro.com>
Subject: Re: [PATCH 4/5] KVM: RISC-V: reset VCPU state when becoming
 runnable

2025-04-25T15:26:08+02:00, Andrew Jones <ajones@...tanamicro.com>:
> On Thu, Apr 03, 2025 at 01:25:23PM +0200, Radim Krčmář wrote:
>> Beware, this patch is "breaking" the userspace interface, because it
>> fixes a KVM/QEMU bug where the boot VCPU is not being reset by KVM.
>> 
>> The VCPU reset paths are inconsistent right now.  KVM resets VCPUs that
>> are brought up by KVM-accelerated SBI calls, but does nothing for VCPUs
>> brought up through ioctls.
>
> I guess we currently expect userspace to make a series of set-one-reg
> ioctls in order to prepare ("reset") newly created vcpus,

Userspace should currently get-one-reg a freshly reset VCPU to know what
KVM SBI decides is the correct reset.  Userspace shouldn't set-one-reg
anything other than what KVM decides, hence we can currently just let
KVM do it.

>                                                           and I guess
> the problem is that KVM isn't capturing the resulting configuration
> in order to replay it when SBI HSM reset is invoked by the guest.

That can also be a solution, but it's not possible to capture the
desired reset state with current IOCTLs, because the first run of a VCPU
can just as well be a resume from a mid-execution.

>                                                                   But,
> instead of capture-replay we could just exit to userspace on an SBI
> HSM reset call and let userspace repeat what it did at vcpu-create
> time.

Right, I like the idea.  (It doesn't fix current userspaces, though.)

>> We need to perform a KVM reset even when the VCPU is started through an
>> ioctl.  This patch is one of the ways we can achieve it.
>> 
>> Assume that userspace has no business setting the post-reset state.
>> KVM is de-facto the SBI implementation, as the SBI HSM acceleration
>> cannot be disabled and userspace cannot control the reset state, so KVM
>> should be in full control of the post-reset state.
>> 
>> Do not reset the pc and a1 registers, because SBI reset is expected to
>> provide them and KVM has no idea what these registers should be -- only
>> the userspace knows where it put the data.
>
> s/userspace/guest/

Both are correct...  I should have made the context clearer here.
I meant the initial hart boot, where userspace loads code/dt and sets
pc/a1 to them.

>> An important consideration is resume.  Userspace might want to start
>> with non-reset state.  Check ran_atleast_once to allow this, because
>> KVM-SBI HSM creates some VCPUs as STOPPED.
>> 
>> The drawback is that userspace can still start the boot VCPU with an
>> incorrect reset state, because there is no way to distinguish a freshly
>> reset new VCPU on the KVM side (userspace might set some values by
>> mistake) from a restored VCPU (userspace must set all values).
>
> If there's a correct vs. incorrect reset state that KVM needs to enforce,
> then we'll need a different API than just a bunch of set-one-reg calls,
> or set/get-one-reg should be WARL for userpace.

Incorrect might have been too strong word... while the SBI reset state
is technically UNSPECIFIED, I think it's just asking for bugs if the
harts have different initial states based on their reset method.

>    set/get-one-reg should be WARL for userpace.

WAAAA! :)

>> The advantage of this solution is that it fixes current QEMU and makes
>> some sense with the assumption that KVM implements SBI HSM.
>> I do not like it too much, so I'd be in favor of a different solution if
>> we can still afford to drop support for current userspaces.
>> 
>> For a cleaner solution, we should add interfaces to perform the KVM-SBI
>> reset request on userspace demand.
>
> That's what the change to kvm_arch_vcpu_ioctl_set_mpstate() in this
> patch is providing, right?

It does.  With conditions to be as compatible as possible.

>> I think it would also be much better
>> if userspace was in control of the post-reset state.
>
> Agreed. Can we just exit to userspace on SBI HSM reset?

Yes.  (It needs an userspace toggle if we care about
backward-compatiblity, though.)

How much do we want to fix/break current userspaces?

Thanks.

Powered by blists - more mailing lists