Re: [PATCH v3 29/39] xen/riscv: prepare new IMSIC VS-file

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

On 10/23/26 2:55 PM, Baptiste Le Duc wrote:
>> [ ... 64 lines skipped ... ]
>> diff --git a/xen/arch/riscv/aia.c b/xen/arch/riscv/aia.c
>> index e31c9c2d24..229ab4b867 100644
>> --- a/xen/arch/riscv/aia.c
>> +++ b/xen/arch/riscv/aia.c
>> @@ -1,8 +1,10 @@
>>   /* SPDX-License-Identifier: GPL-2.0-only */
>>   
>> +#include <xen/bug.h>
>>   #include <xen/errno.h>
>>   #include <xen/init.h>
>>   #include <xen/sections.h>
>> +#include <xen/sched.h>
> Nit: Why xen/sched.h? The stub doesn't dereference v, and asm/aia.h already
> forward-declares struct vcpu. If it stays: sched < sections, so it goes one
> line up.

Indeed, nothing needs it here. I'll include <asm/aia.h> instead, which
aia.c doesn't include yet: without that the stub's struct vcpu would 
only be declared inside the parameter list. <xen/sched.h> then moves to 
the patch where v gets dereferenced first.

>> [ ... 30 lines skipped ... ]
>> +#define imsic_vs_csr_write(c, v)    \
>> +do {                                \
>> +    csr_write(CSR_VSISELECT, c);    \
>> +    csr_write(CSR_VSIREG, v);       \
>> +} while (0)
>> +
>> +/*
>> + * Leaf macros for the switchcase expansion pyramid below:
>> + * imsic_switchcase_break(ireg, op, v) - emit "case ireg: op(ireg, v); break"
>> + */
>> +#define imsic_switchcase_break(ireg, op, v) \
>> +    case ireg:                              \
>> +        op(ireg, v);                        \
>> +        break
>> +
>> +/*
>> + * Generic switchcase expansion pyramid.
>> + * F is the per-operation leaf macro, ireg is the base register index.
>> + * Extra args (e.g. an operation and/or a value) are forwarded to F via
>> + * __VA_ARGS__.
>> + */
>> +#define imsic_switchcase_2(F, ireg, ...)             \
>> +    F((ireg) + 0, __VA_ARGS__);                      \
>> +    F((ireg) + 1, __VA_ARGS__)
>> +#define imsic_switchcase_4(F, ireg, ...)             \
>> +    imsic_switchcase_2(F, (ireg) + 0, __VA_ARGS__);  \
>> +    imsic_switchcase_2(F, (ireg) + 2, __VA_ARGS__)
>> +#define imsic_switchcase_8(F, ireg, ...)             \
>> +    imsic_switchcase_4(F, (ireg) + 0, __VA_ARGS__);  \
>> +    imsic_switchcase_4(F, (ireg) + 4, __VA_ARGS__)
>> +#define imsic_switchcase_16(F, ireg, ...)            \
>> +    imsic_switchcase_8(F, (ireg) + 0, __VA_ARGS__);  \
>> +    imsic_switchcase_8(F, (ireg) + 8, __VA_ARGS__)
>> +#define imsic_switchcase_32(F, ireg, ...)            \
>> +    imsic_switchcase_16(F, (ireg) + 0, __VA_ARGS__); \
>> +    imsic_switchcase_16(F, (ireg) + 16, __VA_ARGS__)
>> +#define imsic_switchcase_64(F, ireg, ...)            \
>> +    imsic_switchcase_32(F, (ireg) + 0, __VA_ARGS__); \
>> +    imsic_switchcase_32(F, (ireg) + 32, __VA_ARGS__)
>> +
>> +static void imsic_eix_write(unsigned int ireg, unsigned long val)
>> +{
>> +    switch ( ireg )
>> +    {
>> +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIP0,
>> +                        imsic_vs_csr_write, val);
>> +    imsic_switchcase_64(imsic_switchcase_break, IMSIC_EIE0,
>> +                        imsic_vs_csr_write, val);
>> +    default:
>> +        ASSERT_UNREACHABLE();
>> +        break;
>> +    }
>> +}
>> +
> What does the switch buy? ireg only ends up as the value written to
> vsiselect, not as a CSR number, so it doesn't need to be a compile-time
> constant. imsic_vs_csr_write(ireg, val) does the same, without the 128 case
> labels and the macro pyramid. If there's a reason, it wants stating in a
> comment; otherwise please drop the pyramid (an ASSERT() on the range can
> stay if you want one).

Good point.

I will drop it and use the following instead:

/*
  * On RV64 a 64-bit EIx group is the even-numbered register alone, 
whereas on
  * RV32 it is split into the even-numbered (low half) and the following
  * odd-numbered (high half) register. ireg names the first, even-numbered
  * register of such a group, among eip0..eip63 or eie0..eie63.
  */
static void imsic_eix_check(unsigned int ireg)
{
     ASSERT(!(ireg & 1));
     ASSERT((ireg >= IMSIC_EIP0 && ireg < IMSIC_EIP0 + 64) ||
            (ireg >= IMSIC_EIE0 && ireg < IMSIC_EIE0 + 64));
}

/* Write a whole 64-bit EIx group, laid out as described above. */
static void imsic_eix_write64(unsigned int ireg, uint64_t val)
{
     imsic_eix_check(ireg);
     imsic_vs_csr_write(ireg, val);
     if ( IS_ENABLED(CONFIG_RISCV_32) )
         imsic_vs_csr_write(ireg + 1, val >> 32);
}

I will applt the similar patter to other patches where the pyramid was used.


> 
> Typo (only if the macros stay): "Leaf macros" - there's only one here. And
> `case ireg:` / `op(ireg, v)` as well as c and v in imsic_vs_csr_write()
> want parenthesizing (MISRA 20.7).

As to c and v in imsic_vs_csr_write(): they're passed as whole arguments
to csr_write() and the other csr_*() macros, which is covered by the
R20.7 deviation for macro parameters used as macro arguments. That's 
also why the parentheses were dropped in v3.

>> [ ... 67 lines skipped ... ]
>> +static void cf_check imsic_vsfile_local_clear(void *data)
>> +{
>> +    unsigned int i;
>> +    const struct imsic_vsfile_data *idata = data;
>> +    unsigned long new_hstatus, old_hstatus, old_vsiselect;
>> +
>> +    /* We can only zero-out if we have a IMSIC VS-file */
> Typo: s/a IMSIC/an IMSIC/
>> [ ... 31 lines skipped ... ]
>>   void imsic_migrate_vcpu(struct vcpu *v)
>>   {
>> +    struct imsic_vsfile_data vsfile_data = {
>> +        .nr_eix = imsic_cfg.nr_eix,
>> +    };
>> +
>>       /*
>>        * The scheduler can mark a freshly created vCPU's unit as migrated and
>>        * invoke this before the vCPU has ever run (see the migrated branch in
>> @@ -703,5 +849,18 @@ void imsic_migrate_vcpu(struct vcpu *v)
>>       if ( v->arch.last_cpu == CPU_NONE )
>>           return;
>>   
>> +    /*
>> +     * At this point, all interrupt producers are still using the old IMSIC
>> +     * VS-file.
>> +     */
>> +
>> +    vsfile_data.hgei = vgein_assign(v);
>> +
>> +    /* We don't support SW interrupt files at the moment. */
>> +    BUG_ON(!vsfile_data.hgei);
>> +
>> +    /* Zero-out new IMSIC VS-file */
>> +    imsic_call_on_cpu(v->processor, imsic_vsfile_local_clear, &vsfile_data);
> Nit: The comment restates the call; drop it.
> 

Agreed, I will drop.

Thanks!

~ Oleksii