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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

> --- 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?

> +    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?

> +        *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?

> --- 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).

Jan