Re: [PATCH 0/5] Enable hstateen bits lazily for the KVM RISC-V Guests

[Atish Patra]

On 5/8/25 6:45 AM, Radim Krčmář wrote:
> 2025-05-07T17:34:38-07:00, Atish Patra <atish.patra@xxxxxxxxx>:
> > On 5/7/25 7:36 AM, Radim Krčmář wrote:
> > > 2025-05-06T11:24:41-07:00, Atish Patra <atish.patra@xxxxxxxxx>:
> > > > On 5/6/25 2:24 AM, Radim Krčmář wrote:
> > > > > 2025-05-05T14:39:25-07:00, Atish Patra <atishp@xxxxxxxxxxxx>:
> > > > > >                                                       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.