On 9/14/26 3:13 PM, Jan Beulich wrote:
On 27.08.2026 17:21, Oleksii Kurochko wrote:
@@ -77,6 +78,64 @@ do { \
csr_clear(CSR_SIREG, v); \
} while (0)
+#define imsic_vs_csr_write(c, v) \
+do { \
+ csr_write(CSR_VSISELECT, (c)); \
+ csr_write(CSR_VSIREG, (v)); \
As patch context also tells: Excess parentheses.
I will drop them then.
+} while ( 0 )
+
+/*
+ * Generic switchcase expansion pyramid.
+ * F is the per-operation leaf macro, ireg is the base register index.
+ * Optional extra args (e.g. an operation and/or a value) are forwarded to F
+ * via __VA_ARGS__.
Just that there's no F below.
Oh, right. I will move this comment a little bit down.
+ * imsic_switchcase_break(ireg, op, v) - emit "case ireg: op(ireg,v); break;"
+ * imsic_switchcase_ret(ireg, op, ...) - emit "case ireg: return op(ireg[,v]);"
+ * The variadic tail is optional so the same leaf works for both read (no v)
+ * and swap (with v).
+ */
+#define imsic_switchcase_break(ireg, op, v) \
+ case ireg: \
+ op(ireg, v); \
+ break;
+
+#define imsic_switchcase_ret(ireg, op, ...) \
+ case ireg: \
+ return op(ireg, ##__VA_ARGS__);
+
+#define imsic_switchcase_2(F, ireg, ...) \
+ F(ireg + 0, ##__VA_ARGS__) \
+ F(ireg + 1, ##__VA_ARGS__)
Ah, there is an F here.
This (recurring below) shows another problem: The two F invocations
look syntacticlly incorrect, due to the missing semicolon. Semicolon
use wants redoing everywhere here.
I will drop then ';' from imsic_switchcase_{break,ret}.
Further (and again throughout) I think we'd be better off using either
standard C constructs (e.g. __VA_ARGS__) or the gcc extension
permitting use of ## after a comma. A mix of both always looks odd
(to me at least).
I will follow C standard here.
Finally, unlike further up, here (and below) ireg wants parenthesizing.
I will add some.
@@ -389,6 +448,76 @@ int cf_check vcpu_imsic_init(struct vcpu *v)
return 0;
}
+/*
+ * Arguments of the imsic_vsfile_local_*() helpers, which are executed by the
+ * pCPU owning the interrupt file, thereby through imsic_call_on_cpu().
+ */
+struct imsic_vsfile_data {
+ unsigned int hgei;
+ unsigned int nr_eix;
+ struct imsic_mrif *mrif;
I can't spot any use of this field (and hence I also can't judge
whether const wants adding).
It should be introduced later in the another patch.
+};
+
+/*
+ * Execute func() on the pCPU which owns the IMSIC interrupt file func() is
+ * going to work with.
+ *
+ * An IMSIC VS-file is reachable only through hstatus.VGEIN of the hart the
+ * file belongs to, and a guest interrupt file index is meaningless on any
+ * other hart, so such work always has to be done by that very hart.
+ *
+ * The local case runs with IRQs disabled to provide func() with the same
+ * environment it is given when it is called from the function call IPI
+ * handler.
+ */
+static void imsic_call_on_cpu(unsigned int cpu, void (*func)(void *),
+ void *data)
If this is supposed to be passed struct imsic_vsfile_data *, why not say
so here? Be as type-safe as possible. Of course the callback function
has to use void *.
At the moment, I don't see where I am using void * so I will use struct
imsic_vsfile_data * instead.
+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 */
+ if ( !idata->hgei )
+ return;
Wouldn't it make sense to avoid the call here altogether then?
I think it is better to have this if () here instead of the caller side
as this function one day could be just directly (w/ imsic_call_on_cpu)
and even the way how it is called now and in the case of
imsic_call_on_cpu() is executed on local cpu then it will be basically
just direct call of imsic_vsfile_local_clear(). So in the case I am not
missing something I prefer to have a check here.
+ old_vsiselect = csr_read(CSR_VSISELECT);
Likely obvious to you, but I can't spot why vsiselect would need saving
here. If you want me to ack such code, please add at least brief comments.
I think then it will be better to put the comment once above struct
imsic_vsfile_data and then just point here to that comment as basically
it will be needed for all imsic_vsfile_local_* helpers.
So basically I am suggesting:
--- a/xen/arch/riscv/imsic.c
+++ b/xen/arch/riscv/imsic.c
@@ ... @@
/*
* Arguments of the imsic_vsfile_local_*() helpers, which are executed
by the
* pCPU owning the interrupt file, thereby through imsic_call_on_cpu().
+ *
+ * The helpers interrupt whatever vCPU context is loaded on that pCPU,
which
+ * generally isn't the vCPU the interrupt file belongs to. To reach the
file
+ * they retarget hstatus.VGEIN and select the file's registers through
+ * vsiselect. Both CSRs are live state of the interrupted vCPU
(vsiselect is
+ * saved to struct arch_vcpu only on context switch, but the
interrupted vCPU
+ * may resume guest execution without one), hence the helpers have to
restore
+ * them before returning.
*/
struct imsic_vsfile_data {
unsigned int hgei;
unsigned int nr_eix;
struct imsic_mrif *mrif;
};
@@ ... @@ static void cf_check imsic_vsfile_local_clear(void *data)
/* We can only zero-out if we have a IMSIC VS-file */
if ( !idata->hgei )
return;
+ /* See the comment ahead of struct imsic_vsfile_data. */
old_vsiselect = csr_read(CSR_VSISELECT);
old_hstatus = csr_read(CSR_HSTATUS);
@@ ... @@ static void cf_check imsic_vsfile_local_read_clear(void *data)
csr_clear(CSR_HGEIE, BIT(idata->hgei, UL));
+ /* See the comment ahead of struct imsic_vsfile_data. */
old_vsiselect = csr_read(CSR_VSISELECT);
old_hstatus = csr_read(CSR_HSTATUS);
@@ ... @@ static void cf_check imsic_vsfile_local_update(void *data)
* stack.
*/
+ /* See the comment ahead of struct imsic_vsfile_data. */
old_vsiselect = csr_read(CSR_VSISELECT);
old_hstatus = csr_read(CSR_HSTATUS);
Does it look clear now?
+ old_hstatus = csr_read(CSR_HSTATUS);
+ new_hstatus = old_hstatus & ~HSTATUS_VGEIN;
+ new_hstatus |= MASK_INSR(idata->hgei, HSTATUS_VGEIN);
+ csr_write(CSR_HSTATUS, new_hstatus);
+
+ imsic_vs_csr_write(IMSIC_EIDELIVERY, 0);
+ imsic_vs_csr_write(IMSIC_EITHRESHOLD, 0);
+
+ for ( i = 0; i < idata->nr_eix; i++ )
+ {
+ imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
+ imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
+#ifdef CONFIG_RISCV_32
+ imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
+ imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
+#endif
In asm/imsic.h I see
#define IMSIC_EIPx_BITS 32
Why is the number of CSR writes different here for RV32 vs RV64?
IMSIC_EIPx_BITS is the unit the AIA spec numbers the eip<k>/eie<k>
registers by. On RV32 all of eip0..eip63 exist and are 32 bits wide. On
RV64 only the even-numbered ones exist, each being 64 bits wide and
covering what eip<k> and eip<k+1> cover on RV32; accessing an
odd-numbered one is an illegal instruction. Hence one 64-bit group of
interrupt identities takes one register on RV64, but two on RV32.
I will add some small comments:
for ( i = 0; i < idata->nr_eix; i++ )
{
/* On RV64 a 64-bit EIx group is the even-numbered register
alone. */
imsic_eix_write(IMSIC_EIP0 + i * 2, 0);
imsic_eix_write(IMSIC_EIE0 + i * 2, 0);
#ifdef CONFIG_RISCV_32
/*
* On RV32 it is split into the even-numbered (low half) and the
* following odd-numbered (high half) register.
*/
imsic_eix_write(IMSIC_EIP0 + i * 2 + 1, 0);
imsic_eix_write(IMSIC_EIE0 + i * 2 + 1, 0);
#endif
}
And
if so, why would you not use the 64-bit write function, allowing the
#ifdef to be omitted?
I can introduce something like:
/*
* 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.
*/
static void imsic_eix_write64(unsigned int ireg, uint64_t val)
{
imsic_eix_write(ireg, val);
if ( IS_ENABLED(CONFIG_RISCV_32) )
imsic_eix_write(ireg + 1, val >> 32);
}
And then:
for ( i = 0; i < idata->nr_eix; i++ )
{
imsic_eix_write64(IMSIC_EIP0 + i * 2, 0);
imsic_eix_write64(IMSIC_EIE0 + i * 2, 0);
}
Would it be better?
Or instead of imsic_eix_write64() I could just have a combination of
imisc_vsfile_local_clear():
imsic_eix_write(ireg, 0);
if ( IS_ENABLED(CONFIG_RISCV_32) )
imsic_eix_write(ireg + 1, 0);
@@ -689,6 +818,14 @@ int __init vimsic_make_domu_dt_node(struct kernel_info
*kinfo,
void imsic_migrate_vcpu(struct vcpu *v)
{
+ unsigned int new_vsfile_hgei;
+ unsigned int new_vsfile_cpu;
+ unsigned int nr_hw_eix = DIV_ROUND_UP(imsic_cfg.nr_ids + 1,
+ BITS_PER_TYPE(uint64_t));
As written, this could also be 64. uint64_t is a fixed-width type after
all. The question here is: Which variable's type do you really mean
here?
The minimum number of hardware EIx groups is 1 (for the minimum 63
supported interrupt identities, i.e. DIV_ROUND_UP(63 + 1, 64) = 1), and
the maximum is 32 (for 2047 identities), on both RV32 and RV64.You are
right regarding BITS_PER_TYPE(uint64_t). Since an architectural EIx
group always covers 64 interrupt identities regardless of XLEN, using
BITS_PER_TYPE(uint64_t) is unnecessarily indirect when we mean a
constant 64-bit group size.I will simplify this in v3 to use 64.
+ struct imsic_vsfile_data vsfile_data = {
+ .nr_eix = nr_hw_eix,
The local variable looks to be used only here. Is there really a need
for such a local variable?
No, I will drop nr_hw_eix.
@@ -699,5 +836,27 @@ void imsic_migrate_vcpu(struct vcpu *v)
if ( v->arch.last_cpu == NR_CPUS )
return;
+ /*
+ * At this point, all interrupt producers are still using the old IMSIC
+ * VS-file.
+ */
+
+ /*
+ * Latch the pCPU the new interrupt file is taken from: vgein_assign()
+ * allocates it from v->processor's pool of guest interrupt files, and
+ * only that hart can access the file afterwards.
+ */
+ new_vsfile_cpu = v->processor;
Same here: Is this variable really needed?
Technically no, it could be used v->processor everywhere but just for
readability (to how spec is wording migration process) I think I will
prefer to have new_vsfile_cpu here. But if it doesn't make sense I can
agree to drop it.
And what exactly is the comment
telling me?
I am re-reading it now and it looks just useless.
I think that initially I thought that for some reason v->processor could
change during the end of migration and so by that I wanted to fix new
vsfile cpu so all the interrupts will go there before migration functon
for that vcpu will be called again and reschedule all the interrupt to
new v->processor.
I think it isn't real case so the comment could be dropped.
+ new_vsfile_hgei = vgein_assign(v);
+
+ /* We don't support SW interrupt files at the moment. */
+ BUG_ON(!new_vsfile_hgei);
+
+ vsfile_data.hgei = new_vsfile_hgei;
And again - any real need for the separate local variable?
Here I agree, we could have only vsfile_data.hgei.
Thanks.
~ Oleksii
