Re: [PATCH v3] xen/irq: Delete the pirq_cleanup_check() macro

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Mon, 25 Aug 2025, Dmytro Prokopchuk1 wrote:
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> MISRA Rule 5.5 objects to a macro aliasing a function, which is what
> pirq_cleanup_check() does. The macro was originally intended to ensure
> the condition 'if (!pirq->evtchn)' is always checked before invoking
> the function, avoiding errors across call sites.
> 
> There are only a handful of users, so expand it inline to be plain
> regular C. Doing this shows one path now needing braces, and one path
> in 'evtchn_bind_pirq()' where the expanded form simplies back to no
> delta, as it follows an unconditional clear of 'info->evtchn'.
> 
> While this complies with MISRA, it shifts the responsibility to
> developers to check 'if (!pirq->evtchn)' at call sites.
> 
> No functional changes.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Signed-off-by: Dmytro Prokopchuk <dmytro_prokopchuk1@xxxxxxxx>

Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>

> ---
> Changes in v3:
> - added back wording from v1, originally written by Andrew.
> 
> Link to v2:
> https://patchew.org/Xen/ce37bdf7b5189d314c0f41628dbfb3281358bcf4.1755361782.git.dmytro._5Fprokopchuk1@xxxxxxxx/
> 
> Link to v1:
> https://patchew.org/Xen/20250729223110.3404441-1-andrew.cooper3@xxxxxxxxxx/
> ---
>  xen/arch/x86/irq.c                | 11 +++++++----
>  xen/common/event_channel.c        |  5 ++++-
>  xen/drivers/passthrough/x86/hvm.c |  9 ++++++---
>  xen/include/xen/irq.h             |  3 ---
>  4 files changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 556134f85a..1ed85c0c11 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1325,7 +1325,8 @@ static void clear_domain_irq_pirq(struct domain *d, int 
> irq, struct pirq *pirq)
>  static void cleanup_domain_irq_pirq(struct domain *d, int irq,
>                                      struct pirq *pirq)
>  {
> -    pirq_cleanup_check(pirq, d);
> +    if ( !pirq->evtchn )
> +        pirq_cleanup_check(pirq, d);
>      radix_tree_delete(&d->arch.irq_pirq, irq);
>  }
>  
> @@ -1383,7 +1384,7 @@ struct pirq *alloc_pirq_struct(struct domain *d)
>      return pirq;
>  }
>  
> -void (pirq_cleanup_check)(struct pirq *pirq, struct domain *d)
> +void pirq_cleanup_check(struct pirq *pirq, struct domain *d)
>  {
>      /*
>       * Check whether all fields have their default values, and delete
> @@ -2823,7 +2824,8 @@ int map_domain_emuirq_pirq(struct domain *d, int pirq, 
> int emuirq)
>                  radix_tree_int_to_ptr(pirq));
>              break;
>          default:
> -            pirq_cleanup_check(info, d);
> +            if ( !info->evtchn )
> +                pirq_cleanup_check(info, d);
>              return err;
>          }
>      }
> @@ -2858,7 +2860,8 @@ int unmap_domain_pirq_emuirq(struct domain *d, int pirq)
>      if ( info )
>      {
>          info->arch.hvm.emuirq = IRQ_UNBOUND;
> -        pirq_cleanup_check(info, d);
> +        if ( !info->evtchn )
> +            pirq_cleanup_check(info, d);
>      }
>      if ( emuirq != IRQ_PT )
>          radix_tree_delete(&d->arch.hvm.emuirq_pirq, emuirq);
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index 67700b050a..a3d18bc464 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -741,11 +741,14 @@ int evtchn_close(struct domain *d1, int port1, bool 
> guest)
>              if ( !is_hvm_domain(d1) ||
>                   domain_pirq_to_irq(d1, pirq->pirq) <= 0 ||
>                   unmap_domain_pirq_emuirq(d1, pirq->pirq) < 0 )
> +            {
>                  /*
>                   * The successful path of unmap_domain_pirq_emuirq() will 
> have
>                   * called pirq_cleanup_check() already.
>                   */
> -                pirq_cleanup_check(pirq, d1);
> +                if ( !pirq->evtchn )
> +                    pirq_cleanup_check(pirq, d1);
> +            }
>          }
>          unlink_pirq_port(chn1, d1->vcpu[chn1->notify_vcpu_id]);
>          break;
> diff --git a/xen/drivers/passthrough/x86/hvm.c 
> b/xen/drivers/passthrough/x86/hvm.c
> index a2ca7e0e57..b73bb55055 100644
> --- a/xen/drivers/passthrough/x86/hvm.c
> +++ b/xen/drivers/passthrough/x86/hvm.c
> @@ -329,7 +329,8 @@ int pt_irq_create_bind(
>                  pirq_dpci->gmsi.gvec = 0;
>                  pirq_dpci->dom = NULL;
>                  pirq_dpci->flags = 0;
> -                pirq_cleanup_check(info, d);
> +                if ( !info->evtchn )
> +                    pirq_cleanup_check(info, d);
>                  write_unlock(&d->event_lock);
>                  return rc;
>              }
> @@ -536,7 +537,8 @@ int pt_irq_create_bind(
>                      hvm_irq_dpci->link_cnt[link]--;
>                  }
>                  pirq_dpci->flags = 0;
> -                pirq_cleanup_check(info, d);
> +                if ( !info->evtchn )
> +                    pirq_cleanup_check(info, d);
>                  write_unlock(&d->event_lock);
>                  xfree(girq);
>                  xfree(digl);
> @@ -693,7 +695,8 @@ int pt_irq_destroy_bind(
>           */
>          pt_pirq_softirq_reset(pirq_dpci);
>  
> -        pirq_cleanup_check(pirq, d);
> +        if ( !pirq->evtchn )
> +            pirq_cleanup_check(pirq, d);
>      }
>  
>      write_unlock(&d->event_lock);
> diff --git a/xen/include/xen/irq.h b/xen/include/xen/irq.h
> index 95034c0d6b..6071b00f62 100644
> --- a/xen/include/xen/irq.h
> +++ b/xen/include/xen/irq.h
> @@ -185,9 +185,6 @@ extern struct pirq *pirq_get_info(struct domain *d, int 
> pirq);
>  
>  void pirq_cleanup_check(struct pirq *pirq, struct domain *d);
>  
> -#define pirq_cleanup_check(pirq, d) \
> -    (!(pirq)->evtchn ? pirq_cleanup_check(pirq, d) : (void)0)
> -
>  extern void pirq_guest_eoi(struct pirq *pirq);
>  extern void desc_guest_eoi(struct irq_desc *desc, struct pirq *pirq);
>  extern int pirq_guest_unmask(struct domain *d);
> -- 
> 2.43.0
>