On 22.01.2026 17:47, 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). The consumer must not write to irqs_pending and must not
> act on bits that are not set in the mask. Otherwise, extra synchronization
> should be provided.
> The worst thing which could happen with such approach is that a new pending
> bit will be set to irqs_pending bitmap during update of hvip variable in
> vcpu_flush_interrupt() but it isn't problem as the new pending bit won't
> be lost and just be proceded during the next flush.
>
> It is possible a guest could have pending bit not result in the hardware
> register without to be marked pending in irq_pending bitmap as:
> According to the RISC-V ISA specification:
> Bits hip.VSSIP and hie.VSSIE are the interrupt-pending and
> interrupt-enable bits for VS-level software interrupts. VSSIP in hip
> is an alias (writable) of the same bit in hvip.
> Additionally:
> When bit 2 of hideleg is zero, vsip.SSIP and vsie.SSIE are read-only
> zeros. Else, vsip.SSIP and vsie.SSIE are aliases of hip.VSSIP and
> hie.VSSIE.
> This means the guest may modify vsip.SSIP, which implicitly updates
> hip.VSSIP and the bit being writable with 1 would also trigger an interrupt
> as according to the RISC-V spec:
> These conditions for an interrupt trap to occur must be evaluated in a
> bounded amount of time from when an interrupt becomes, or ceases to be,
> pending in sip, and must also be evaluated immediately following the
> execution of an SRET instruction or an explicit write to a CSR on which
> these interrupt trap conditions expressly depend (including sip, sie and
> sstatus).
> What means that IRQ_VS_SOFT must be synchronized separately, what is done
> in vcpu_sync_interrupts().
And this function is going to be used from where? Exit from guest into the
hypervisor? Whereas vcpu_flush_interrupt() is to be called ahead of re-
entering the guest?
I ask because vcpu_sync_interrupts() very much looks like a producer to me,
yet the patch here supposedly is the consumer side.
> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -171,3 +171,68 @@ int vcpu_unset_interrupt(struct vcpu *v, unsigned int
> irq)
>
> return 0;
> }
> +
> +static void vcpu_update_hvip(struct vcpu *v)
Pointer-to-const?
> +{
> + csr_write(CSR_HVIP, v->arch.hvip);
> +}
> +
> +void vcpu_flush_interrupts(struct vcpu *v)
> +{
> + register_t *hvip = &v->arch.hvip;
> +
> + unsigned long mask, val;
These are used ...
> + if ( ACCESS_ONCE(v->arch.irqs_pending_mask[0]) )
> + {
> + mask = xchg(&v->arch.irqs_pending_mask[0], 0UL);
> + val = ACCESS_ONCE(v->arch.irqs_pending[0]) & mask;
> +
> + *hvip &= ~mask;
> + *hvip |= val;
... solely in this more narrow scope.
> + }
> +
> + /*
> + * 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 hviph"
> +#endif
> +
> + vcpu_update_hvip(v);
Why would bits for which the mask bit wasn't set be written here?
> +void vcpu_sync_interrupts(struct vcpu *v)
> +{
> + unsigned long hvip;
> +
> + /* Read current HVIP and VSIE CSRs */
> + v->arch.vsie = csr_read(CSR_VSIE);
> +
> + /* Sync-up HVIP.VSSIP bit changes does by Guest */
Nit: s/does/done/ ?
> + hvip = csr_read(CSR_HVIP);
> + if ( (v->arch.hvip ^ hvip) & BIT(IRQ_VS_SOFT, UL) )
> + {
> + if ( !test_and_set_bit(IRQ_VS_SOFT,
> + &v->arch.irqs_pending_mask) )
Why two separate, nested if()s?
> + {
> + if ( hvip & BIT(IRQ_VS_SOFT, UL) )
> + set_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
> + else
> + clear_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
> + }
In the previous patch you set forth strict ordering rules, with a barrier in
the middle. All of this is violated here.
> + }
> +
> + /*
> + * Sync-up AIA high interrupts.
> + *
> + * It is necessary to do only for CONFIG_RISCV_32 which isn't supported
> + * now.
> + */
> +#ifdef CONFIG_RISCV_32
> +# error "Update vsieh"
> +#endif
Here you mean the register or the struct vcpu field? It may be helpful to
disambiguate; assuming it's the latter, simply spell out v->arch.vsieh?
(Same then for the similar code in vcpu_flush_interrupts().)
Jan
