Message-Id: <DC6DLP13J0LA.XW9J3XFBCM1Y@ventanamicro.com>
Date: Tue, 19 Aug 2025 13:43:05 +0200
From: Radim Krčmář <rkrcmar@...tanamicro.com>
To: "Anup Patel" <apatel@...tanamicro.com>
Cc: "Atish Patra" <atish.patra@...ux.dev>, "Palmer Dabbelt"
 <palmer@...belt.com>, "Paul Walmsley" <paul.walmsley@...ive.com>,
 "Alexandre Ghiti" <alex@...ti.fr>, "Andrew Jones"
 <ajones@...tanamicro.com>, "Anup Patel" <anup@...infault.org>, "Paolo
 Bonzini" <pbonzini@...hat.com>, "Shuah Khan" <shuah@...nel.org>,
 <kvm@...r.kernel.org>, <kvm-riscv@...ts.infradead.org>,
 <linux-riscv@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
 <linux-kselftest@...r.kernel.org>, "linux-riscv"
 <linux-riscv-bounces@...ts.infradead.org>
Subject: Re: [PATCH 0/6] ONE_REG interface for SBI FWFT extension

2025-08-19T12:00:43+05:30, Anup Patel <apatel@...tanamicro.com>:
> On Mon, Aug 18, 2025 at 3:59 PM Radim Krčmář <rkrcmar@...tanamicro.com> wrote:
>>
>> 2025-08-14T21:25:42+05:30, Anup Patel <apatel@...tanamicro.com>:
>> > This series adds ONE_REG interface for SBI FWFT extension implemented
>> > by KVM RISC-V.
>>
>> I think it would be better to ONE_REG the CSRs (medeleg/menvcfg), or at
>> least expose their CSR fields (each sensible medeleg bit, PMM, ...)
>> through kvm_riscv_config, than to couple this with SBI/FWFT.
>>
>> The controlled behavior is defined by the ISA, and userspace might want
>> to configure the S-mode execution environment even when SBI/FWFT is not
>> present, which is not possible with the current design.
>>
>> Is there a benefit in expressing the ISA model through SBI/FWFT?
>>
>
> Exposing medeleg/menvcfg is not the right approach because a
> Guest/VM does not have M-mode hence it is not appropriate to
> expose m<xyz> CSRs via ONE_REG interface. This also aligns
> with H-extension architecture which does not virtualize M-mode.

We already have mvendorid, marchid, and mipid in kvm_riscv_config.

The virtualized M-mode is userspace+KVM.  (KVM doesn't allow userspace
to configure most things now, but I think we'll have to change that when
getting ready for production.)

> We already had discussions about this in the past.
>
> As such, we have two options. One option is to expose
> hedeleg/henvcfg via kvm_riscv_config and another option
> is to have a separate ONE_REG for each FWFT feature.
>
> Separate ONE_REG registers for each FWFT feature is better
> than directly exposing hedeleg/henvcfg via ONE_REG because:
>
> 1) Once nested virtualization lands, we will be having separate
> hedeleg/henvcfg as part of nested virtualization state of Guest
> which is trap-n-emulated by KVM. The existence of hedeleg/henvcfg
> in kvm_riscv_config and nested virtualization state will only create
> more confusion.

Right, the userspace registers for this can't be called h*.

> 2) Not all bits in hedeleg/henvcfg are used for FWFT since quite
> a few bits are programmed with fixed value based on KVM
> implementation choices (which may change in future).

Yes, we'll want to expose some to userspace.

>                                                      Also,
> things like set_debug_ioctl() change hedeleg at runtime
> which allow KVM user space to decide who takes breakpoint
> traps from Guest/VM.

This is still doable.  The clear hedeleg bit does not have to change the
virtualized behavior -- if the guest is expecting to see breakpoint
traps, then even if userspace+KVM configure the architecture to direct
the traps to the hypervisor, they must then forward the breakpoints that
were supposed to be delivered to the guest.

>                      This means value saved/restored
> through hedeleg/henvcfg in kvm_riscv_config becomes
> specific to the kernel version and specific to host ISA features.

Hedeleg/henvcfg bits do not have to be the same as userspace interface
bits -- KVM always has to distinguish what the userspace wants to
virtualize, and what the KVM changed for its own reasons.

> 3) We anyway need to provide ONE_REG interface to
> save/restore FWFT feature flags so it's better to keep the
> FWFT feature value as part of the same ONE_REG interface.

I think we want to have SBI in userspace (especially for single-shot
ecalls like FWFT).  The userspace implementation will want an interface
to set the ISA bits, and it's very awkward with the proposed design.

Flags can to stay, in case the userpace wants to accelerate FWFT.

> 4) The availability of quite a few FWFT features is dependent
> on corresponding ISA extensions so having separate ONE_REG
> registers of each FWFT feature allows get_reg_list_ioctl() to
> provide KVM user-space only available FWFT feature registers.

Yes, but similarly the userspace would be forbidden from setting bits
that cannot be expressed in henvcfg/hededeg.

There are also behaviors we want to configure that do not have a FWFT
toggle.  e.g. the recent patches for delegation of illegal-instruction
exceptions that changed the guest behavior -- someone might want to
keep incrementing the SBI PMU counter, and someone will want to forward
them to be implemented in userspace (when developing a new extension,
because most of the existing ISA can still be accelerated by KVM).

For general virtualization, we want to be able to configure the
following behavior for each exception that would go to the virtualized
M-mode:
  0) delegated to the guest
  1) implemented by userspace
  2-N) implementations by KVM (ideally zero or one)

We can have medeleg, and another method to decide how to handle trapped
exceptions, but it probably makes more sense to have a per-exception
ONE_REG that sets how each exception behaves.

The FWFT value could be a part of a more general interface.

Thanks.

Powered by blists - more mailing lists