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

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