Re: [PATCH v7 14/14] xen/riscv: Disable SSTC extension and add trap-based CSR probing

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.03.2026 12:38, Oleksii Kurochko wrote:
> On 3/11/26 11:58 AM, Jan Beulich wrote:
>> On 11.03.2026 10:54, Oleksii Kurochko wrote:
>>> On 3/10/26 10:15 AM, Jan Beulich wrote:
>>>> On 06.03.2026 17:33, Oleksii Kurochko wrote:
>>>>> --- a/automation/scripts/qemu-smoke-riscv64.sh
>>>>> +++ b/automation/scripts/qemu-smoke-riscv64.sh
>>>>> @@ -7,7 +7,7 @@ rm -f smoke.serial
>>>>>    
>>>>>    export TEST_CMD="qemu-system-riscv64 \
>>>>>        -M virt,aia=aplic-imsic \
>>>>> -    -cpu rv64,svpbmt=on \
>>>>> +    -cpu rv64,svpbmt=on,sstc=off \
>>>>>        -smp 1 \
>>>>>        -nographic \
>>>>>        -m 2g \
>>>> How does this fit with you panic()ing when SSTC isn't available (i.e. the
>>>> register cannot be read)? I must be missing something, likely a result of
>>>> me not being able to really understand the description.
>>> When SSTC isn't available my panic() won't occur and then will continue to
>>> be executed. Otherwise, when SSTC is enabled (it is enabled by QEMU by 
>>> default)
>>> my panic will occur.
>> Oh, I notice I misread the condition around the panic(), mainly because of
>> the misleading / ambiguous message passed to it: "SSTC isn't supported\n"
>> can mean unsupported by Xen or unsupported by the platform.
>>
>> Anyway, to me this is entirely bogus: Why would we panic() because there is
>> a certain extension available?
> 
> It is bogus because then we need also add support of SSTC for a guest what 
> isn't
> done now thereby if it is detected that SSTC is available that it is dangerous
> to continue about full support (guest part) of it.
> 
> I thought about the case to let Xen use SSTC and just don't tell Linux that 
> SSTC
> is available by dropping from riscv,isa property the mentioning of SSTC, so 
> then
> Linux will still continue to use SBI set timer call to Xen and the will just
> safely reprogram (if it is needed) a timer using SSTC instructions. But if to 
> do
> in this way still nothing will prevent a guest to test if SSTC is available by
> reading CSR_STIMECMP and nothing will prevent to access CSR_VSTIMECMP by guest
> what could also lead to some misleading behavior.
> Likely we could set henvcfg.STCE to zero and it will forbid guest to access 
> SSTC
> registers but I am not sure that we really want such behavior when Xen is 
> using
> SSTC to setup a timer, but guest isn't allowed.

I don't see what's wrong with Xen using an extension that isn't made available
to guests.

> It seems it will be better just
> support SSTC extension fully and not to support it only for now.

If at all, an ack for such from me would be pretty reluctantly given.

>>>>> +    register unsigned long ret = 0; \
>>>>> +    unsigned long flags; \
>>>>> +    ((struct trap_info *)(trap))->scause = 0; \
>>>> "trap" would better be of the correct type. Don't use casts like this, 
>>>> please.
>>>>
>>>> Further, wouldn't you better set the field to a guaranteed invalid value? 
>>>> 0 is
>>>> CAUSE_MISALIGNED_FETCH, after all.
>>> I don't see that such an invalid value exist for scause. I think we have to 
>>> reserved
>>> a value from region 24-31 or 48-63 as they are designated for custom use.
>> Not sure that's possible. "Custom use" may mean "custom" from hw perspective.
>> I was rather thinking of picking something pretty high in the reserved range,
>> like (1 << (MXLEN-1)) - 1 or 1 << (MXLEN-2).
> 
> Agree, it could be an option.
> 
>>
>>>>> +    local_irq_save(flags); \
>>>>> +    asm volatile ( \
>>>>> +        ".option push\n" \
>>>>> +        ".option norvc\n" \
>>>> Shouldn't this come later?
>>> Do you mean before where SSTC csr is really tried to be read ("csrr %[ret], 
>>> %[csr]\n")?
>> Yes.
>>
>>> Does it really matter in such small inline assembler?
>> Yes, if nothing else then to not raise questions. Plus (depending on the
>> specific operands used), the ADD (MV) could e.g. be representable by a C 
>> insn.
>>
>>>> And why set ttmp in the first place, when
>>>> that's what do_expected_trap() writes to?
>>> To force the compiler to materialize tinfo in register a4 (ttmp) before the
>>> trap handler runs as handler will use a4 as temporary register.
>> ??? I don't understand what you mean with "materialize".
> 
> Mean forcing the compiler to load the variable into the specific hardware
> register (a4) before the potentially trapping instruction executes, so the
> trap handler can safely use that register.

I fear there's some misunderstanding on inline assembly here. An output-only
variable doesn't need "pre-loading". But anyway, all of this is gong to be
moot here (but potentially relevant elsewhere in the future) when this
becomes a mere clobber.

>>>>> +        "csrr %[ret], %[csr]\n" \
>>>>> +        "csrw " STR(CSR_STVEC) ", %[stvec]\n" \
>>>>> +        ".option pop" \
>>>>> +        : [stvec] "+&r" (stvec), [tinfo] "+&r" (tinfo), \
>>>> tinfo isn't modified, is it?
>>> It is modified by handler.
>> Where? It's only used as the address of the two stores.
> 
> There are to updates of tinfo in the do_expected_trap():
> 
> FUNC(do_expected_trap)
>          ...
>          REG_S   a4, RISCV_TRAP_SEPC(a3)
>          ...
>          REG_S   a4, RISCV_TRAP_SCAUSE(a3)
>          ...
> END(do_expected_trap)

a3 is used there twice, yes, but neither use modifies the register. If the
first use modified it, the 2nd use would be broken.

Jan