Re: [PATCH v8 16/20] xen/riscv: implement IRQ routing for device passthrough

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 9/3/26 4:39 PM, Oleksii Kurochko wrote:
>>> +    do { smp_rmb(); } while ( test_bit(_IRQ_INPROGRESS, &desc- 
>>> >status) );
> 
> I am thinking if a barrier is in correct place or needed at all.
> 
> Considering that desc->status is updated under spinlock() which uses 
> full barrier the result here should be already observable without 
> smp_rmb() inside do {} while ().
> 
> Probably we want to have load->load between test_bit() and a read of 
> action->free_on_release in if () below but I don't see what could go 
> wrong if this read will happen before do {} while ().
> 
> xvfree() (stores inside it) can't be executed ealier because of control 
> dependency [Rule 11: b (xfree) is a (action->free_on_release) store, and 
> b has a syntactic control dependency on a] so again it looks like a 
> barrier isn't needed here.
> 
> So considering what kind of barrier is used inside spinlock + Rule 11 we 
> can just move smp_rmb() after the cycle (just in case) and it looks like 
> smp_rmb() is only here just to force compiler not to order the things 
> considering how action->free_on_release is used:
> 
> static void irq_release_action(const struct irq_desc *desc,
>                                 struct irqaction *action)
> {
>      /*
>       * Wait to make sure it's not being used on another CPU.
>       *
>       * desc->status is cleared in do_IRQ() under desc->lock, whose
>       * acquire/release barriers are a full smp_mb() on this arch, so the
>       * handler's writes are already ordered before the clear is visible.
>       * On this side, xvfree() is control-dependent on the final test_bit()
>       * load, so Rule 11 (RVWMO) already orders it after the wait with no
>       * barrier. smp_rmb() below adds real read->read ordering, but nothing
>       * after the loop depends on it (action->free_on_release isn't racy);
>       * it's kept as a guard against the compiler breaking the control
>       * dependency the ordering actually relies on.
>       */
>      while ( test_bit(_IRQ_INPROGRESS, &desc->status) )
>          cpu_relax();
>      smp_rmb();
> 
>      if ( action->free_on_release )
>          xvfree(action);
> 
> Am I missing something?

It could be option just to skip smp_rmb() here at all:

     /*
      * Wait for a handler still running on another CPU to complete: 
do_IRQ()
      * clears IRQ_INPROGRESS only after the handler has returned.
      *
      * No barrier is needed here: nothing below reads data written by the
      * handler (action->free_on_release is set up once, before the 
action is
      * ever registered), and the stores done by xvfree() are ordered after
      * the loop's load of desc->status by the control dependency alone
      * (RVWMO ppo rule 11).
      */
     while ( test_bit(_IRQ_INPROGRESS, &desc->status) )
         cpu_relax();

     if ( action->free_on_release )
         xvfree(action);

But probably just to be sure that if ->free_on_release will one day 
somewhere else set except the mentioned case it makes sense to have 
smp_rmb() or even smp_mb() (depsite of the fact smp_rmb() looks more 
then enough).

Does it make sense?

> 
>>
>> Please split this across three lines, to conform to style. (Also same nit
>> as above.)
>>
>>> +    if ( action->free_on_release )
>>> +        xvfree(action);