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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.02.2026 11:50, Oleksii Kurochko wrote:
> On 1/29/26 6:01 PM, Jan Beulich wrote:
>> On 22.01.2026 17:47, Oleksii Kurochko wrote:
>>> This patch is based on Linux kernel 6.16.0.
>>> +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?
> 
> Do you mean that it could be:
>    if ( !test_and_set_bit(IRQ_VS_SOFT, &v->arch.irqs_pending_mask) && (hvip & 
> BIT(IRQ_VS_SOFT, UL))
> ?

That's combining with the if() ...

>>> +        {
>>> +            if ( hvip & BIT(IRQ_VS_SOFT, UL) )

... down here, which - ...

>>> +                set_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
>>> +            else

... having an "else" - can't be folded like this, I think. I meant the two
if()s immediately ahead of my remark.

>>> +                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.
> 
> It still respects the rule that the producer (|vcpu_sync_interrupts()| which
> should be in the producer path) never clears the mask and only writes to
> |irqs_pending| if it is the one that flipped the corresponding mask bit from 0
> to 1.
> 
> Considering that the consumer cannot be called concurrently in this case
> (since|vcpu_flush_interrupts()| and|vcpu_sync_interrupts()| are only invoked
> sequentially in|check_for_pcpu_work()|, as mentioned above), nothing can
> clear a bit in the mask in between. Therefore, I think it is acceptable to
> slightly bend (and it should be explained in the comment above the
> function or in the commit message) the rule that the|irqs_pending| bit must
> be written first, followed by updating the corresponding bit in
> |irqs_pending_mask() specifically for |vcpu_sync_interrupts().

With suitable commenting - yes.

Jan