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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 2/11/26 3:26 PM, Jan Beulich wrote:
> On 09.02.2026 17:52, Oleksii Kurochko wrote:
> > This patch is based on Linux kernel 6.16.0.
> >
> > Add the consumer side (vcpu_flush_interrupts()) of the lockless pending
> > interrupt tracking introduced in part 1 (for producers). According, to the
> > design only one consumer is possible, and it is vCPU itself.
> > vcpu_flush_interrupts() is expected to be ran (as guests aren't ran now due
> > to the lack of functionality) before the hypervisor returns control to the
> > guest.
> >
> > Producers may set bits in irqs_pending_mask without a lock. Clearing bits in
> > irqs_pending_mask is performed only by the consumer via xchg() (with aquire &
> > release semantics).
> Where the release part isn't relevant here, aiui.
Yes, only acquire part is relevant here. release mentioned here as xchg use 
.aqrl
prefix.


> > --- a/xen/arch/riscv/domain.c
> > +++ b/xen/arch/riscv/domain.c
> > @@ -194,3 +194,36 @@ void vcpu_sync_interrupts(struct vcpu *v)
> >   #   error "Update v->arch.vsieh"
> >   #endif
> >   }
> > +
> > +static void vcpu_update_hvip(const struct vcpu *v)
> > +{
> > +    csr_write(CSR_HVIP, v->arch.hvip);
> > +}
> > +
> > +void vcpu_flush_interrupts(struct vcpu *v)
> > +{
> > +    register_t *hvip = &v->arch.hvip;
> Why not in the if()'s scope, when it's used only there?
Missed that during vcpu_update_hvip() inside if().

> > +    if ( ACCESS_ONCE(v->arch.irqs_pending_mask[0]) )
> > +    {
> > +        unsigned long mask, val;
> > +
> > +        mask = xchg(&v->arch.irqs_pending_mask[0], 0UL);
> > +        val = ACCESS_ONCE(v->arch.irqs_pending[0]) & mask;
> Make these the initializers of the variables?
Good point. I will do that.

> > +        *hvip &= ~mask;
> > +        *hvip |= val;
> > +
> > +        /*
> > +         * Flush AIA high interrupts.
> > +         *
> > +         * It is necessary to do only for CONFIG_RISCV_32 which isn't
> > +         * supported now.
> > +         */
> > +#ifdef CONFIG_RISCV_32
> > +        #   error "Update v->arch.hviph"
> Ehem. I really dislike having to give the same comment over and over again.
> Please have the # of a pre-processor directive always in the first column.
>
> Also, isn't this misplaced? The containing if() checks irqs_pending_mask[0],
> yet aiui irqs_pending_mask[1] would be of interest for hviph?
Agree, it would be more correct to have outside if().

> > --- a/xen/arch/riscv/traps.c
> > +++ b/xen/arch/riscv/traps.c
> > @@ -172,6 +172,8 @@ static void do_unexpected_trap(const struct cpu_user_regs 
> > *regs)
> >   static void check_for_pcpu_work(void)
> >   {
> >       vcpu_sync_interrupts(current);
> > +
> > +    vcpu_flush_interrupts(current);
> >   }
> Ah, okay - here comes a 2nd call from this function. However, please latch
> current into a local variable (already in the earlier patch perhaps); no
> need to fetch it from per-CPU data twice (or yet more times, if further
> stuff was going to appear here).
It makes sense. Ill do that.

Thanks.

~ Oleksii