Re: [PATCH v3 05/16] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/12/26 11:24 AM, Jan Beulich wrote:
> On 12.02.2026 10:37, Oleksii Kurochko wrote:
> > On 2/11/26 3:16 PM, Jan Beulich wrote:
> > > On 09.02.2026 17:52, Oleksii Kurochko wrote:
> > > > Based on Linux kernel v6.16.0.
> > > > Note that smp_wmb() is used instead of smp_mb__before_atomic() as what
> > > > we want to guarantee that if a bit in irqs_pending_mask is obversable
> > > > that the correspondent bit in irqs_pending is observable too.
> > > And the counterpart of this barrier is the one encoded implicitly in
> > > xchg()? Could do with making more explicit, e.g. by way of adding a
> > > code comment there.
> > I thought it would be clear from the paragraph below where xchng_acquire()
> > is mentioned. I'll update the comment to make it more explicit.
> I'm confused. The (bogus) mentioning of xchg_acquire() is in the patch
> description, whereas I suggested a code comment.
Oh, I see. I'll add the a code comment.


> > > > @@ -124,3 +125,72 @@ void arch_vcpu_destroy(struct vcpu *v)
> > > >    {
> > > >        vfree((char *)v->arch.cpu_info + sizeof(struct cpu_info) - STACK_SIZE);
> > > >    }
> > > > +
> > > > +int vcpu_set_interrupt(struct vcpu *v, unsigned int irq)
> > > > +{
> > > > +    /*
> > > > +     * We only allow VS-mode software, timer, and external
> > > > +     * interrupts when irq is one of the local interrupts
> > > > +     * defined by RISC-V privilege specification.
> > > > +     */
> > > What is "when irq is one ..." intended to be telling me? There's no ...
> > >
> > > > +    if ( irq != IRQ_VS_SOFT &&
> > > > +         irq != IRQ_VS_TIMER &&
> > > > +         irq != IRQ_VS_EXT )
> > > > +        return -EINVAL;
> > > ... corresponding code (anymore), afaict.
> > That part should be removed, there is no any sense for it anymore.
> >
> > > Further, who are the prospected callers of this function and its sibling
> > > below? If they're all internal (i.e. not reachable via hypercalls or
> > > emulation on behalf of the guest), this may want to be assertions.
> > Considering your further reply:
> >     Having seen a use in patch 08, I should clarify the "reachable" part here:
> >     It's not the "reachable" alone, but whether the guest has control over the
> >     "irq" value passed. For the example in patch 08 this isn't the case.
> >
> > I think I did not fully understand the part about “the guest has control over
> > the ‘irq’ value passed”, but at the moment I do not have any plans for the guest
> > to pass the irq value directly. (Do you have any examples where it should be
> > needed?).
> No, I don't. This is all for you to sort out.
>
> > > > --- a/xen/arch/riscv/include/asm/domain.h
> > > > +++ b/xen/arch/riscv/include/asm/domain.h
> > > > @@ -54,6 +54,25 @@ struct arch_vcpu {
> > > >        register_t henvcfg;
> > > >        register_t hideleg;
> > > >        register_t hstateen0;
> > > > +    register_t hvip;
> > > > +
> > > > +    register_t vsie;
> > > > +
> > > > +    /*
> > > > +     * VCPU interrupts
> > > > +     *
> > > > +     * We have a lockless approach for tracking pending VCPU interrupts
> > > > +     * implemented using atomic bitops. The irqs_pending bitmap represent
> > > > +     * pending interrupts whereas irqs_pending_mask represent bits changed
> > > > +     * in irqs_pending. Our approach is modeled around multiple producer
> > > > +     * and single consumer problem where the consumer is the VCPU itself.
> > > > +     *
> > > > +     * DECLARE_BITMAP() is needed here to support 64 vCPU local interrupts
> > > > +     * on RV32 host.
> > > > +     */
> > > > +#define RISCV_VCPU_NR_IRQS 64
> > > What is this 64 deriving from? IOW - can it be some kind of expression,
> > > helping the reader?
> > Originally it derives from the width of mideleg, mie, mvien, mvip, and mip (and
> > their counterpars for other modes) what means that RV32 will have no more then
> > 32 local interrutps, but considering that RISC-V AIA spec tells ...:
> >
> >     Table 2.1 lists both the CSRs added for machine level and existing 
> > machine-level
> >     CSRs whose size is changed by the Advanced Interrupt Architecture. Existing 
> > CSRs
> >     mie, mip, and mideleg are widended to 64 bits to support a total of 64 
> > interrupt
> >     causes.
> >     For RV32, the high-half CSRs listed in the table allow access to the upper 
> > 32
> >     bits of registers mideleg, mie, mvien, mvip, and mip. The Advanced Interrupt
> >     Architecture requires that these high-half CSRs exist for RV32, but the 
> > bits they
> >     access may all be merely read-only zeros.
> >
> > ... that for RV32 it was widened to 64, so 64 appears here. I haven't used some 
> > AIA
> > specific name for constant 64 as in case if AIA isn't used it is more then 
> > enough
> > to cover PLIC case, for example.
> Thing is that with RV128 in mind I wonder whether 64 is correct, or whether it
> e.g. wants to be max(BITS_PER_LONG, 64).
If to look at registers which are used now in hypervisor [1][Table 3] and are 
connected
to the bitmask (or their counterparts mentioned in the quote [2][Table 1]) are
hard-coded to 64 and it doesn't dependent on architecture bit-ness. And this is 
true
for AIA.

But on the other hand in RISC-V privilege spec the length of hvip is defined as
HSXLEN which could be 128 in the case of RV128 and to not depend only on AIA 
spec
likely it would be better to use suggested by you way to define 
RISCV_VCPU_NR_IRQS.

[1] 
https://github.com/riscv/riscv-aia/blob/main/src/CSRs.adoc#hypervisor-and-vs-csrs
[2] 
https://github.com/riscv/riscv-aia/blob/main/src/CSRs.adoc#machine-level-csrs

Thanks.

~ Oleksii


> Jan