Re: [PATCH v2 19/39] xen/riscv: implement trap redirection to a guest

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/7/26 5:57 PM, Baptiste Le Duc wrote:
>> Some traps taken by Xen on behalf of a guest can't or shouldn't be handled
>> by the hypervisor and have to be reflected to the guest's own S-mode trap
>> handler instead: the access faults which handle_guest_page_fault() injects
>> for a fault that can never become an emulated access, and, later on, a
>> fault taken by the hlv/hlvx sequences of riscv_read_guest() while
>> accessing guest memory on a vCPU's behalf.
>>
> Access faults from handle_guest_page_fault() aren't "taken by Xen on
> behalf of a guest" as those are guest-page faults taken directly from the
> guest's own execution. Xen just decides they can't be emulated and
> reflects them back as access faults. Only the hlv/hlvx case is Xen
> trapping on the guest's behalf (Xen itself executes the faulting access).> Conflating the two under one description makes the paragraph confusing.
> 
> Suggest splitting into two:
> 
>      Two kinds of traps can't or shouldn't be handled by the hypervisor and
>      have to be reflected to the guest's own S-mode trap handler instead:
> 
>      - Traps Xen takes on the guest's behalf: the hlv/hlvx sequences
>      riscv_read_guest() uses to access guest memory.
>      - Access faults handle_guest_page_fault() injects for a guest-page
>      fault that can never become an emulated access.

Thanks, I'll update original paragraph with what you suggested.>
>>
>> Implement trap_redirect(), until now a BUG_ON() placeholder, for that
>> purpose. It makes the trap appear to the guest as if it had been taken
>> directly in VS-mode: the trap information is transferred to the guest's
>> virtual supervisor CSRs and the vCPU is resumed at its exception vector in
>> supervisor mode, following the trap entry rules of the RISC-V privileged
>> specification.
>>
>> Add the STVEC_* definitions needed to tell the BASE and MODE fields of
>> vstvec apart.
>>
>> The implementation is based on kvm_riscv_vcpu_trap_redirect() from Linux,
>> with a few deviations:
>>   - The function reads and writes physical VS-mode CSRs, so it is only
>>     meaningful for the currently running vCPU. Instead of taking a
>>     struct vcpu argument, it always operates on current.
>>   - The MODE field of vstvec is masked off explicitly when computing the
>>     exception target PC (exceptions always vector to BASE), rather than
>>     relying on the hardwired zero bit of sepc to drop it on VM entry.
>>   - Assertions document the preconditions: the trap must have been taken
>>     from virtualized mode (hstatus.SPV set), and only synchronous
>>     exceptions may be redirected - interrupts must be injected via hvip
>>     instead, so that the hardware performs VS-mode trap entry itself,
>>     respecting vsstatus.SIE and vectored vstvec dispatch.
>>
>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
>>
>> diff --git a/xen/arch/riscv/include/asm/riscv_encoding.h b/xen/arch/riscv/include/asm/riscv_encoding.h
>> index 2d2e7e11b3..b2071f4758 100644
>> --- a/xen/arch/riscv/include/asm/riscv_encoding.h
>> +++ b/xen/arch/riscv/include/asm/riscv_encoding.h
>> @@ -109,6 +109,12 @@
>>   #define SIP_SSIP			MIP_SSIP
>>   #define SIP_STIP			MIP_STIP
>>   
>> +/* stvec/vstvec: MODE is bits [1:0], BASE is bits [XLEN-1:2] */
>> +#define STVEC_MODE_MASK			_UL(0x3)
>> +#define STVEC_MODE_DIRECT		_UL(0x0)
>> +#define STVEC_MODE_VECTORED		_UL(0x1)
>> +#define STVEC_BASE_MASK			(~STVEC_MODE_MASK)
> Nit: STVEC_MODE_DIRECT and STVEC_MODE_VECTORED aren't used anywhere in
> this patch (only STVEC_BASE_MASK is). Either use them where you decide
> exceptions always target BASE regardless of MODE, or drop them until a
> patch that needs them.

IMO it is fine to introduce *_DIRECT/VECORED here as they are used 
implicitly through STVEC_BASE_MASK and thereby it will be better to 
introduce them here now instead of open-code them and then just update 
STVEC_BASE_MASK again when *_DIRECT/VECORED will be re-introduced.

Thanks for review.

~ Oleksii