Re: [PATCH v1 1/3] xen/riscv: introduce vSBI extension framework

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.12.2025 18:03, Oleksii Kurochko wrote:
> On 12/8/25 3:25 PM, Jan Beulich wrote:
>> On 01.12.2025 11:24, Oleksii Kurochko wrote:
>>> This commit introduces support for handling virtual SBI extensions in Xen.
>>>
>>> The changes include:
>>> - Added new vsbi.c and vsbi.h files to implement virtual SBI extension
>>>    handling.
>>> - Modified traps.c to handle CAUSE_VIRTUAL_SUPERVISOR_ECALL by calling
>>>    vsbi_handle_ecall() when the trap originates from VS-mode.
>>> - Updated xen.lds.S to include a new .vsbi.exts section for virtual SBI
>>>    extension data.
>>> - Updated Makefile to include the new vsbi/ directory in the build.
>>> - Add hstatus register to struct cpu_user_regs as it is needed for
>>>    a check that CAUSE_VIRTUAL_SUPERVISOR_ECALL happens from VS-mode.
>> I can spot the check, yes, but without the field ever being set how is one
>> to determine whether that check actually makes sense?
> 
> But hstatus is set automatically when a trap occurs and will be copied in
> handle_trap() in entry.S.

Just that entry.S isn't even touched by this series. Did you perhaps omit an
important part of the change?

> If you think it is better to introduce saving and restoring of hstatus in
> handle_trap() now, or instead drop the handling of
> “case CAUSE_VIRTUAL_SUPERVISOR_ECALL:” in do_trap(), please let me know.

I think what I said above is quite clear: When you introduce a field that's
supposed to be filled upon entry to the hypervisor, the entry code wants
updating accordingly.

>>> ---
>>>   xen/arch/riscv/Makefile                |  1 +
>>>   xen/arch/riscv/include/asm/processor.h |  1 +
>>>   xen/arch/riscv/include/asm/vsbi.h      | 31 +++++++++++++++++
>>>   xen/arch/riscv/traps.c                 |  8 +++++
>>>   xen/arch/riscv/vsbi/Makefile           |  1 +
>>>   xen/arch/riscv/vsbi/vsbi.c             | 46 ++++++++++++++++++++++++++
>> A file named identical to the directory it lives in raises the question of
>> why there is such a new sub-directory. Are you expecting moree files to
>> appear there?
> 
> Yes, I'm expecting that and it is done in the next patches of this patch
> series.
> 
>>   How's vsbi.c then be "special" compared to the others? Do
>> you perhaps mean someling like "core.c" or "common.c" here?
> 
> Agree, this is more appropriate for either “core.c” or “common.c”. Both 
> options
> are fine with me. I slightly prefer using the prefix “vsbi-{core/common}.c”, 
> but
> if you think it is better to omit the prefix since the folder name already
> provides that context, I’m fine with dropping it.

Yes, I'm actually quite heavily opposed to such redundant prefixes. They 
obfuscate
things, and they get in the way of name completion features in shells and alike.

>>> +static const struct vsbi_ext vsbi_ext_##ext __used                  \
>>> +__section(".vsbi.exts") = {                                         \
>>> +    .name = #ext,                                                   \
>>> +    .eid_start = extid_start,                                       \
>>> +    .eid_end = extid_end,                                           \
>>> +    .handle = extid_handle,
>>> +
>>> +#define VSBI_EXT_END                                                \
>>> +};
>> There's no use here, and peeking ahead at the other two patches shows
>> no use where this odd split of the macros would be necessary. What is
>> this about?
> 
> I thought this was the common approach, similar to DT_DEVICE, where we have
> DT_DEVICE_START and DT_DEVICE_END. There may be no need for it right now, but
> perhaps we will eventually want similar behavior for VSBI_EXT_START.

For DT_DEVICE_{START,END} there at least is a reason to have a split like
this. (I would very much like for that to be done without such a split, though.)

> If you think it is better to drop VSBI_EXT_END for now, I’m okay with that,
> and can just use VSBI_EXT instead of VSBI_EXT_START.

Yes please. If and when the need arises, it can be introduced, or (as per above)
a better solution be found.

>>> --- a/xen/arch/riscv/traps.c
>>> +++ b/xen/arch/riscv/traps.c
>>> @@ -15,6 +15,7 @@
>>>   #include <asm/processor.h>
>>>   #include <asm/riscv_encoding.h>
>>>   #include <asm/traps.h>
>>> +#include <asm/vsbi.h>
>>>   
>>>   /*
>>>    * Initialize the trap handling.
>>> @@ -114,6 +115,13 @@ void do_trap(struct cpu_user_regs *cpu_regs)
>>>   
>>>       switch ( cause )
>>>       {
>>> +    case CAUSE_VIRTUAL_SUPERVISOR_ECALL:
>>> +        if ( !(cpu_regs->hstatus & HSTATUS_SPV) )
>>> +            panic("CAUSE_VIRTUAL_SUPERVISOR_ECALL came not from 
>>> VS-mode\n");
>> This might more naturally want to be BUG_ON()? Assuming of course the value
>> in question is exclusively under hypervisor control. Otherwise panic() would
>> also be wrong to use here.
> 
> Only hypervisor can access ->hstatus (of course, hart is changing it when a 
> trap
> happens, for example).
> BUG_ON() is a good option for me.

Just to clarify: "can access" != "under control". There's also the possibility
that a guest could do something causing the hardware to raise a
CAUSE_VIRTUAL_SUPERVISOR_ECALL trap without setting HSTATUS_SPV. That was the
underlying question here.

>>> --- /dev/null
>>> +++ b/xen/arch/riscv/vsbi/vsbi.c
>>> @@ -0,0 +1,46 @@
>>> +/* SPDX-License-Identifier: GPL-2.0-only */
>>> +
>>> +#include <xen/sched.h>
>>> +
>>> +#include <asm/processor.h>
>>> +#include <asm/sbi.h>
>>> +#include <asm/vsbi.h>
>>> +
>>> +extern const struct vsbi_ext _svsbi_exts[], _evsbi_exts[];
>>> +
>>> +const struct vsbi_ext *vsbi_find_extension(unsigned long ext_id)
>> static?
> 
> It could be use not in vsbi.c (for example, in the next patches it is used for
> SBI_EXT_BASE_PROBE_EXT), so it shouldn't be static.

Okay. In RISC-V that's okay as long as it's not subject to Misra scanning. Yet
still introducing a non-static function without callers from other CUs may
warrant a remark in the description. Once RISC-V becomes subject to Misra scans,
such will be problematic, after all.

>> Also, again - is the ext_ prefix adding any value here?
> 
> Not really, I guess.

Maybe, to still distinguish from "fid", use "eid" here then?

>>> +{
>>> +    const struct vsbi_ext *vsbi_ext;
>>> +
>>> +    for ( vsbi_ext = _svsbi_exts; vsbi_ext != _evsbi_exts; vsbi_ext++ )
>>> +        if ( ext_id >= vsbi_ext->eid_start &&
>>> +             ext_id <= vsbi_ext->eid_end )
>>> +            return vsbi_ext;
>> What if multiple entries have overlapping EID ranges?
> 
> Good question, I wasn't able to find that EID is always unique in SBI spec,
> but, at the same time, if to look at all available extensions and their id(s),
> they are always unique, so I expect that they will be always unique, 
> otherwise,
> it won't be possible which extension should be used.

Then should there be a build-time (or if that's not easily possible, boot-
time) check?

>>> +void vsbi_handle_ecall(struct vcpu *vcpu, struct cpu_user_regs *regs)
>>> +{
>>> +    const unsigned long eid = regs->a7;
>>> +    const unsigned long fid = regs->a6;
>>> +    const struct vsbi_ext *ext = vsbi_find_extension(eid);
>>> +    int ret;
>>> +
>>> +    if ( ext && ext->handle )
>>> +        ret = ext->handle(vcpu, eid, fid, regs);
>> Is a registration record NULL handler pointer actually legitimate / useful?
>> (If there was range overlap checking I could see a reason to permit such.)
> 
> it is a good question, I think ext->handle = NULL should be impossible. At
> least, at the moment I can't come up with the case where it is possible and
> what will be a use case. I will drop ext->handle check.
> 
>>
>>> +    else
>>> +    {
>>> +        printk("Unsupported Guest SBI EID #%#lx, FID #%lu\n", eid, 
>>> regs->a1);
>> Are the #-es ahead of the %-s adding value here?
> 
> It is how SBI spec writes them. For example,
>   9. Hart State Management Extension (EID #0x48534D "HSM") . . . . . . . . . 
> . . . . . . . . . . . . . . . . . . . . . . . . . 26
>   9.1. Function: Hart start (FID #0) . . . . . . . . . . . . . . . . . . . . 
> . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . 27
> 
> So I thought that it would help to find stuff faster.

Okay. Maybe mention such in the description?

Jan