arch/riscv/include/asm/kvm_aia.h | 14 ++++++- arch/riscv/include/asm/kvm_vcpu_insn.h | 4 ++ arch/riscv/kvm/aia.c | 77 ++++++++++++++++++++++++++++++++++ arch/riscv/kvm/aia_imsic.c | 8 ++++ arch/riscv/kvm/vcpu.c | 10 ----- arch/riscv/kvm/vcpu_insn.c | 52 +++++++++++++++++++++++ 6 files changed, 154 insertions(+), 11 deletions(-)
This series adds support for enabling hstateen bits lazily at runtime
instead of statically at bootime. The boot time enabling happens for
all the guests if the required extensions are present in the host and/or
guest. That may not be necessary if the guest never exercise that
feature. We can enable the hstateen bits that controls the access lazily
upon first access. This providers KVM more granular control of which
feature is enabled in the guest at runtime.
Currently, the following hstateen bits are supported to control the access
from VS mode.
1. BIT(58): IMSIC : STOPEI and IMSIC guest interrupt file
2. BIT(59): AIA : SIPH/SIEH/STOPI
3. BIT(60): AIA_ISEL : Indirect csr access via siselect/sireg
4. BIT(62): HSENVCFG : SENVCFG access
5. BIT(63): SSTATEEN0 : SSTATEEN0 access
KVM already support trap/enabling of BIT(58) and BIT(60) in order
to support sw version of the guest interrupt file. This series extends
those to enable to correpsonding hstateen bits in PATCH1. The remaining
patches adds lazy enabling support of the other bits.
I am working on a followup series to add indirect CSR extension and move the
siselect/sireg handlers out of AIA so that other features(e.g CTR) can leverage
it.
Note: This series just updates the hstateen bit in cfg so that any update
would reflect in the correct VM state during the next vcpu load.
Alternatively, we can save the hstateen state in vcpu_put to achieve this.
However, it will incur additional cost on every VM exit while the current
approach just updates the configuration once per VM life time upon first
access.
To: Anup Patel <anup@brainfault.org>
To: Atish Patra <atishp@atishpatra.org>
To: Paul Walmsley <paul.walmsley@sifive.com>
To: Palmer Dabbelt <palmer@dabbelt.com>
To: Alexandre Ghiti <alex@ghiti.fr>
Cc: kvm@vger.kernel.org
Cc: kvm-riscv@lists.infradead.org
Cc: linux-riscv@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Atish Patra <atishp@rivosinc.com>
---
Atish Patra (5):
RISC-V: KVM: Lazy enable hstateen IMSIC & ISEL bit
RISC-V: KVM: Add a hstateen lazy enabler helper function
RISC-V: KVM: Support lazy enabling of siselect and aia bits
RISC-V: KVM: Enable envcfg and sstateen bits lazily
RISC-V: KVM: Remove the boot time enabling of hstateen bits
arch/riscv/include/asm/kvm_aia.h | 14 ++++++-
arch/riscv/include/asm/kvm_vcpu_insn.h | 4 ++
arch/riscv/kvm/aia.c | 77 ++++++++++++++++++++++++++++++++++
arch/riscv/kvm/aia_imsic.c | 8 ++++
arch/riscv/kvm/vcpu.c | 10 -----
arch/riscv/kvm/vcpu_insn.c | 52 +++++++++++++++++++++++
6 files changed, 154 insertions(+), 11 deletions(-)
---
base-commit: 01f95500a162fca88cefab9ed64ceded5afabc12
change-id: 20250430-kvm_lazy_enable_stateen-33c8aa9a3071
--
Regards,
Atish patra
2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: > This series adds support for enabling hstateen bits lazily at runtime > instead of statically at bootime. The boot time enabling happens for > all the guests if the required extensions are present in the host and/or > guest. That may not be necessary if the guest never exercise that > feature. We can enable the hstateen bits that controls the access lazily > upon first access. This providers KVM more granular control of which > feature is enabled in the guest at runtime. > > Currently, the following hstateen bits are supported to control the access > from VS mode. > > 1. BIT(58): IMSIC : STOPEI and IMSIC guest interrupt file > 2. BIT(59): AIA : SIPH/SIEH/STOPI > 3. BIT(60): AIA_ISEL : Indirect csr access via siselect/sireg > 4. BIT(62): HSENVCFG : SENVCFG access > 5. BIT(63): SSTATEEN0 : SSTATEEN0 access > > KVM already support trap/enabling of BIT(58) and BIT(60) in order > to support sw version of the guest interrupt file. I don't think KVM toggles the hstateen bits at runtime, because that would mean there is a bug even in current KVM. > This series extends > those to enable to correpsonding hstateen bits in PATCH1. The remaining > patches adds lazy enabling support of the other bits. The ISA has a peculiar design for hstateen/sstateen interaction: For every bit in an hstateen CSR that is zero (whether read-only zero or set to zero), the same bit appears as read-only zero in sstateen when accessed in VS-mode. This means we must clear bit 63 in hstateen and trap on sstateen accesses if any of the sstateen bits are not supposed to be read-only 0 to the guest while the hypervisor wants to have them as 0. Thanks.
On 5/6/25 2:24 AM, Radim Krčmář wrote: > 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: >> This series adds support for enabling hstateen bits lazily at runtime >> instead of statically at bootime. The boot time enabling happens for >> all the guests if the required extensions are present in the host and/or >> guest. That may not be necessary if the guest never exercise that >> feature. We can enable the hstateen bits that controls the access lazily >> upon first access. This providers KVM more granular control of which >> feature is enabled in the guest at runtime. >> >> Currently, the following hstateen bits are supported to control the access >> from VS mode. >> >> 1. BIT(58): IMSIC : STOPEI and IMSIC guest interrupt file >> 2. BIT(59): AIA : SIPH/SIEH/STOPI >> 3. BIT(60): AIA_ISEL : Indirect csr access via siselect/sireg >> 4. BIT(62): HSENVCFG : SENVCFG access >> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access >> >> KVM already support trap/enabling of BIT(58) and BIT(60) in order >> to support sw version of the guest interrupt file. > I don't think KVM toggles the hstateen bits at runtime, because that > would mean there is a bug even in current KVM. This was a typo. I meant to say trap/emulate BIT(58) and BIT(60). This patch series is trying to enable the toggling of the hstateen bits upon first access. Sorry for the confusion. >> This series extends >> those to enable to correpsonding hstateen bits in PATCH1. The remaining >> patches adds lazy enabling support of the other bits. > The ISA has a peculiar design for hstateen/sstateen interaction: > > For every bit in an hstateen CSR that is zero (whether read-only zero > or set to zero), the same bit appears as read-only zero in sstateen > when accessed in VS-mode. Correct. > This means we must clear bit 63 in hstateen and trap on sstateen > accesses if any of the sstateen bits are not supposed to be read-only 0 > to the guest while the hypervisor wants to have them as 0. Currently, there are two bits in sstateen. FCSR and ZVT which are not used anywhere in opensbi/Linux/KVM stack. In case, we need to enable one of the bits in the future, does hypevisor need to trap every sstateen access ? As per my understanding, it should be handled in the hardware and any write access to to those bits should be masked with hstateen bit value so that it matches. That's what we do in Qemu as well. > Thanks.
2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>: > On 5/6/25 2:24 AM, Radim Krčmář wrote: >> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: >>> This series adds support for enabling hstateen bits lazily at runtime >>> instead of statically at bootime. The boot time enabling happens for >>> all the guests if the required extensions are present in the host and/or >>> guest. That may not be necessary if the guest never exercise that >>> feature. We can enable the hstateen bits that controls the access lazily >>> upon first access. This providers KVM more granular control of which >>> feature is enabled in the guest at runtime. >>> >>> Currently, the following hstateen bits are supported to control the access >>> from VS mode. >>> >>> 1. BIT(58): IMSIC : STOPEI and IMSIC guest interrupt file >>> 2. BIT(59): AIA : SIPH/SIEH/STOPI >>> 3. BIT(60): AIA_ISEL : Indirect csr access via siselect/sireg >>> 4. BIT(62): HSENVCFG : SENVCFG access >>> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access >>> >>> KVM already support trap/enabling of BIT(58) and BIT(60) in order >>> to support sw version of the guest interrupt file. >> I don't think KVM toggles the hstateen bits at runtime, because that >> would mean there is a bug even in current KVM. > > This was a typo. I meant to say trap/emulate BIT(58) and BIT(60). > This patch series is trying to enable the toggling of the hstateen bits > upon first access. > > Sorry for the confusion. No worries, it's my fault for misreading. I got confused, because the code looked like generic lazy enablement, while it's really only for the upper 32 bits and this series is not lazy toggling any VS-mode visible bits. >>> This series extends >>> those to enable to correpsonding hstateen bits in PATCH1. The remaining >>> patches adds lazy enabling support of the other bits. >> The ISA has a peculiar design for hstateen/sstateen interaction: >> >> For every bit in an hstateen CSR that is zero (whether read-only zero >> or set to zero), the same bit appears as read-only zero in sstateen >> when accessed in VS-mode. > > Correct. > >> This means we must clear bit 63 in hstateen and trap on sstateen >> accesses if any of the sstateen bits are not supposed to be read-only 0 >> to the guest while the hypervisor wants to have them as 0. > > Currently, there are two bits in sstateen. FCSR and ZVT which are not > used anywhere in opensbi/Linux/KVM stack. True, I guess we can just make sure the current code can't by mistake lazily enable any of the bottom 32 hstateen bits and handle the case properly later. > In case, we need to enable one of the bits in the future, does hypevisor > need to trap every sstateen access ? We need to trap sstateen accesses if the guest is supposed to be able to control a bit in sstateen, but the hypervisor wants to lazily enable that feature and sets 0 in hstateen until the first trap. If hstateen is 1 for all features that the guest could control through sstateen, we can and should just set the SE bit (63) to 1 as well. > As per my understanding, it should be handled in the hardware and any > write access to to those bits should be masked > with hstateen bit value so that it matches. That's what we do in Qemu as > well. Right, hardware will do the job most of the time. It's really only for the lazy masking, beause if we don't trap the stateen accesses, they would differ from what the guest should see.
On 5/7/25 7:36 AM, Radim Krčmář wrote: > 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>: >> On 5/6/25 2:24 AM, Radim Krčmář wrote: >>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: >>>> This series adds support for enabling hstateen bits lazily at runtime >>>> instead of statically at bootime. The boot time enabling happens for >>>> all the guests if the required extensions are present in the host and/or >>>> guest. That may not be necessary if the guest never exercise that >>>> feature. We can enable the hstateen bits that controls the access lazily >>>> upon first access. This providers KVM more granular control of which >>>> feature is enabled in the guest at runtime. >>>> >>>> Currently, the following hstateen bits are supported to control the access >>>> from VS mode. >>>> >>>> 1. BIT(58): IMSIC : STOPEI and IMSIC guest interrupt file >>>> 2. BIT(59): AIA : SIPH/SIEH/STOPI >>>> 3. BIT(60): AIA_ISEL : Indirect csr access via siselect/sireg >>>> 4. BIT(62): HSENVCFG : SENVCFG access >>>> 5. BIT(63): SSTATEEN0 : SSTATEEN0 access >>>> >>>> KVM already support trap/enabling of BIT(58) and BIT(60) in order >>>> to support sw version of the guest interrupt file. >>> I don't think KVM toggles the hstateen bits at runtime, because that >>> would mean there is a bug even in current KVM. >> This was a typo. I meant to say trap/emulate BIT(58) and BIT(60). >> This patch series is trying to enable the toggling of the hstateen bits >> upon first access. >> >> Sorry for the confusion. > No worries, it's my fault for misreading. > I got confused, because the code looked like generic lazy enablement, > while it's really only for the upper 32 bits and this series is not lazy > toggling any VS-mode visible bits. > >>>> This series extends >>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining >>>> patches adds lazy enabling support of the other bits. >>> The ISA has a peculiar design for hstateen/sstateen interaction: >>> >>> For every bit in an hstateen CSR that is zero (whether read-only zero >>> or set to zero), the same bit appears as read-only zero in sstateen >>> when accessed in VS-mode. >> Correct. >> >>> This means we must clear bit 63 in hstateen and trap on sstateen >>> accesses if any of the sstateen bits are not supposed to be read-only 0 >>> to the guest while the hypervisor wants to have them as 0. >> Currently, there are two bits in sstateen. FCSR and ZVT which are not >> used anywhere in opensbi/Linux/KVM stack. > True, I guess we can just make sure the current code can't by mistake > lazily enable any of the bottom 32 hstateen bits and handle the case > properly later. I can update the cover letter and leave a comment about that. Do you want a additional check in sstateen trap(kvm_riscv_vcpu_hstateen_enable_stateen) to make sure that the new value doesn't have any bits set that is not permitted by the hypervisor ? >> In case, we need to enable one of the bits in the future, does hypevisor >> need to trap every sstateen access ? > We need to trap sstateen accesses if the guest is supposed to be able to > control a bit in sstateen, but the hypervisor wants to lazily enable > that feature and sets 0 in hstateen until the first trap. Yes. That's what PATCH 4 in this series does. > If hstateen is 1 for all features that the guest could control through > sstateen, we can and should just set the SE bit (63) to 1 as well. > >> As per my understanding, it should be handled in the hardware and any >> write access to to those bits should be masked >> with hstateen bit value so that it matches. That's what we do in Qemu as >> well. > Right, hardware will do the job most of the time. It's really only for > the lazy masking, beause if we don't trap the stateen accesses, they > would differ from what the guest should see.
2025-05-07T17:34:38-07:00, Atish Patra <atish.patra@linux.dev>: > On 5/7/25 7:36 AM, Radim Krčmář wrote: >> 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>: >>> On 5/6/25 2:24 AM, Radim Krčmář wrote: >>>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: >>>>> This series extends >>>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining >>>>> patches adds lazy enabling support of the other bits. >>>> The ISA has a peculiar design for hstateen/sstateen interaction: >>>> >>>> For every bit in an hstateen CSR that is zero (whether read-only zero >>>> or set to zero), the same bit appears as read-only zero in sstateen >>>> when accessed in VS-mode. >>> Correct. >>> >>>> This means we must clear bit 63 in hstateen and trap on sstateen >>>> accesses if any of the sstateen bits are not supposed to be read-only 0 >>>> to the guest while the hypervisor wants to have them as 0. >>> Currently, there are two bits in sstateen. FCSR and ZVT which are not >>> used anywhere in opensbi/Linux/KVM stack. >> True, I guess we can just make sure the current code can't by mistake >> lazily enable any of the bottom 32 hstateen bits and handle the case >> properly later. > > I can update the cover letter and leave a comment about that. > > Do you want a additional check in sstateen > trap(kvm_riscv_vcpu_hstateen_enable_stateen) > to make sure that the new value doesn't have any bits set that is not > permitted by the hypervisor ? I wanted to prevent kvm_riscv_vcpu_hstateen_lazy_enable() from being able to modify the bottom 32 bits, because they are guest-visible and KVM does not handle them correctly -- it's an internal KVM error that should be made obvious to future programmers. >>> In case, we need to enable one of the bits in the future, does hypevisor >>> need to trap every sstateen access ? >> We need to trap sstateen accesses if the guest is supposed to be able to >> control a bit in sstateen, but the hypervisor wants to lazily enable >> that feature and sets 0 in hstateen until the first trap. > Yes. That's what PATCH 4 in this series does. I was thinking about the correct emulation. e.g. guest sets sstateen bit X to 1, but KVM wants to handle the feature X lazily, which means that hstateen bit X is 0. hstateen bit SE0 must be 0 in that case, because KVM must trap the guest access to bit X and properly emulate it. When the guest accesses a feature controlled by sstateen bit X, KVM will lazily enable the feature and then set sstateen and hstateen bit X.
On 5/8/25 6:45 AM, Radim Krčmář wrote: > 2025-05-07T17:34:38-07:00, Atish Patra <atish.patra@linux.dev>: >> On 5/7/25 7:36 AM, Radim Krčmář wrote: >>> 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@linux.dev>: >>>> On 5/6/25 2:24 AM, Radim Krčmář wrote: >>>>> 2025-05-05T14:39:25-07:00, Atish Patra <atishp@rivosinc.com>: >>>>>> This series extends >>>>>> those to enable to correpsonding hstateen bits in PATCH1. The remaining >>>>>> patches adds lazy enabling support of the other bits. >>>>> The ISA has a peculiar design for hstateen/sstateen interaction: >>>>> >>>>> For every bit in an hstateen CSR that is zero (whether read-only zero >>>>> or set to zero), the same bit appears as read-only zero in sstateen >>>>> when accessed in VS-mode. >>>> Correct. >>>> >>>>> This means we must clear bit 63 in hstateen and trap on sstateen >>>>> accesses if any of the sstateen bits are not supposed to be read-only 0 >>>>> to the guest while the hypervisor wants to have them as 0. >>>> Currently, there are two bits in sstateen. FCSR and ZVT which are not >>>> used anywhere in opensbi/Linux/KVM stack. >>> True, I guess we can just make sure the current code can't by mistake >>> lazily enable any of the bottom 32 hstateen bits and handle the case >>> properly later. >> I can update the cover letter and leave a comment about that. >> >> Do you want a additional check in sstateen >> trap(kvm_riscv_vcpu_hstateen_enable_stateen) >> to make sure that the new value doesn't have any bits set that is not >> permitted by the hypervisor ? > I wanted to prevent kvm_riscv_vcpu_hstateen_lazy_enable() from being > able to modify the bottom 32 bits, because they are guest-visible and > KVM does not handle them correctly -- it's an internal KVM error that > should be made obvious to future programmers. Sure. I will add something along those lines. >>>> In case, we need to enable one of the bits in the future, does hypevisor >>>> need to trap every sstateen access ? >>> We need to trap sstateen accesses if the guest is supposed to be able to >>> control a bit in sstateen, but the hypervisor wants to lazily enable >>> that feature and sets 0 in hstateen until the first trap. >> Yes. That's what PATCH 4 in this series does. > I was thinking about the correct emulation. > > e.g. guest sets sstateen bit X to 1, but KVM wants to handle the feature > X lazily, which means that hstateen bit X is 0. > hstateen bit SE0 must be 0 in that case, because KVM must trap the guest > access to bit X and properly emulate it. > When the guest accesses a feature controlled by sstateen bit X, KVM will > lazily enable the feature and then set sstateen and hstateen bit X. Yeah. That's possible. The current series is just trying to trap & enable rather than trap & emulate except for few AIA related bits which trap even with hstateen bit set due to sw file instead of vsfile. Once we have such requirement any other feature bit, we can extend the generic trap & enable framework to trap & emulate.
© 2016 - 2025 Red Hat, Inc.