Re: [PATCH v1 07/15] xen/riscv: introduce tracking of pending vCPU interrupts, part 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.01.2026 10:14, Oleksii Kurochko wrote:
> On 1/14/26 4:56 PM, Jan Beulich wrote:
>> On 14.01.2026 16:39, Oleksii Kurochko wrote:
>>> On 1/13/26 2:54 PM, Jan Beulich wrote:
>>>> On 13.01.2026 13:51, Oleksii Kurochko wrote:
>>>>> On 1/7/26 5:28 PM, Jan Beulich wrote:
>>>>>> On 24.12.2025 18:03, Oleksii Kurochko wrote:
>>>>> By maintaining irqs_pending_mask, you can detect “this bit changed
>>>>> recently,” even if the final state is 0.
>>>>>
>>>>> Also, having irqs_pending_mask allows to flush interrupts without lock:
>>>>> 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;
>>>>> }
>>>>> Without it I assume that we should have spinlcok around access to 
>>>>> irqs_pending.
>>>> Ah yes, this would indeed be a benefit. Just that it's not quite clear to
>>>> me:
>>>>
>>>>       *hvip |= xchg(&v->arch.irqs_pending[0], 0UL);
>>>>
>>>> wouldn't require a lock either
>>> Because vCPU's hvip (which is stored on the stack) can't be changed 
>>> concurrently
>>> and it's almost the one place in the code where vCPU->hvip is changed. 
>>> Another
>>> place it is save_csrs() during context switch but it can't be called in 
>>> parallel
>>> with the vcpu_sync_interrupts() (look below).
>>>
>>>> . What may be confusing me is that you put
>>>> things as if it was normal to see 1 -> 0 transitions from (virtual)
>>>> hardware, when I (with my x86 background) would expect 1 -> 0 transitions
>>>> to only occur due to software actions (End Of Interrupt), unless - see
>>>> above - something malfunctioned and an interrupt was lost. That (the 1 ->
>>>> 0 transitions) could be (guest) writes to SVIP, for example.
>>>>
>>>> Talking of which - do you really mean HVIP in the code you provided, not
>>>> VSVIP? So far I my understanding was that HVIP would be recording the
>>>> interrupts the hypervisor itself has pending (and needs to service).
>>> HVIP is correct to use here, HVIP is used to indicate virtual interrupts
>>> intended for VS-mode. And I think you confused HVIP with the HIP register
>>> which supplements the standard supervisor-level SIP register to indicate
>>> pending virtual supervisor (VS-level) interrupts and hypervisor-specific
>>> interrupts.
>>>
>>> If a guest will do "That (the 1 -> 0 transitions) could be (guest) writes
>>> to SVIP, for example." then the correspondent HVIP (and HIP as usually
>>> they are aliasis of HVIP) bits will be updated. And that is why we need
>>> vcpu_sync_interrupts() I've mentioned in one of replies and sync VSSIP:
>>> +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 */
>>> +    hvip = csr_read(CSR_HVIP);
>>> +    if ( (v->arch.hvip ^ hvip) & BIT(IRQ_VS_SOFT, UL) )
>>> +    {
>>> +        if ( hvip & BIT(IRQ_VS_SOFT, UL) )
>>> +        {
>>> +            if ( !test_and_set_bit(IRQ_VS_SOFT,
>>> +                                   &v->arch.irqs_pending_mask) )
>>> +                set_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
>>> +        }
>>> +        else
>>> +        {
>>> +            if ( !test_and_set_bit(IRQ_VS_SOFT,
>>> +                                   &v->arch.irqs_pending_mask) )
>>> +                clear_bit(IRQ_VS_SOFT, &v->arch.irqs_pending);
>>> +        }
>>> +    }
>> I fear I don't understand this at all. Why would the guest having set a
>> pending bit not result in the IRQ to be marked pending?
> 
> Maybe it is wrong assumption but based on the spec:
>    Bits sip.SSIP and sie.SSIE are the interrupt-pending and interrupt-enable
>    bits  for supervisor-level software interrupts. If implemented, SSIP is
>    writable in sip and may also be set to 1 by a platform-specific interrupt
>    controller.
> and:
>    Interprocessor interrupts are sent to other harts by 
> implementation-specific
>    means, which will ultimately cause the SSIP bit to be set in the recipient
>    hart’s sip register.
> 
> Meaning that sending an IPI to self by writing 1 to sip.SSIP is
> well-defined. The same should be true of vsip.SSIP while in VS mode.

I can't read that out of the text above. To the contrary, "will ultimately cause
the SSIP bit to be set" suggests to me that the bit is not to be set by writing
the CSR. Things still may work like this for self-IPI, but that wouldn't follow
from the quotation above.

>>   You can't know
>> whether that guest write happened before or after you last touched
>> .irqs_pending{,mask}[]?
> 
> Yes, I think you are right.
> 
> On the other hand, if we are in hypervisor when vcpu_sync_interrupts() is
> called it means that pCPU on which vCPU is ran and for which
> vcpu_sync_interrupts() is called now executes some hypervisor things, so
> guest won't able to update VSIP.SSIP for this pCPU. So nothing else will
> change VSIP.SSIP and so h/w HVIP won't be changed by something and it is
> okay to sync .irqs_pending{,mask} with what h/w in its HVIP.

That is, vcpu_sync_interrupts() is called on every entry to the hypervisor?
Not just during context switch?

Jan