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
