Re: [PATCH v2] xen/events: fix global virq handling

[Jan Beulich <jbeulich@xxxxxxxx>]

On 07.03.2025 11:11, Jan Beulich wrote:

Sorry, missed
From: Juergen Gross <jgross@xxxxxxxx>

Jan

> VIRQs are split into "global" and "per vcpu" ones. Unfortunately in
> reality there are "per domain" ones, too.
> 
> send_global_virq() and set_global_virq_handler() make only sense for
> the real "global" ones, so replace virq_is_global() with a new
> function get_virq_type() returning one of the 3 possible types (global,
> domain, vcpu VIRQ).
> 
> To make its intended purpose more clear, also rename
> send_guest_global_virq() to send_guest_domain_virq().
> 
> Fixes: 980822c5edd1 ("xen/events: allow setting of global virq handler only 
> for unbound virqs")
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> ---
> As I don't have much time today (I'm having a day off), only compile
> tested. Please try a CI run with this patch before applying!
> 
> Whether the "domain" infix in send_guest_domain_virq() is really useful
> I don't know. Maybe we should go even father with renaming, to have
> {vcpu,domain}_send_virq()?
> ---
> v2: Also rename send_guest_global_virq().
> 
> --- a/xen/arch/arm/include/asm/event.h
> +++ b/xen/arch/arm/include/asm/event.h
> @@ -47,9 +47,9 @@ static inline void local_event_delivery_
>  }
>  
>  /* No arch specific virq definition now. Default to global. */
> -static inline bool arch_virq_is_global(unsigned int virq)
> +static inline enum virq_type arch_get_virq_type(unsigned int virq)
>  {
> -    return true;
> +    return VIRQ_GLOBAL;
>  }
>  
>  #endif
> --- a/xen/arch/ppc/include/asm/event.h
> +++ b/xen/arch/ppc/include/asm/event.h
> @@ -17,9 +17,9 @@ static inline int vcpu_event_delivery_is
>  }
>  
>  /* No arch specific virq definition now. Default to global. */
> -static inline bool arch_virq_is_global(unsigned int virq)
> +static inline enum virq_type arch_get_virq_type(unsigned int virq)
>  {
> -    return true;
> +    return VIRQ_GLOBAL;
>  }
>  
>  static inline int local_events_need_delivery(void)
> --- a/xen/arch/riscv/include/asm/event.h
> +++ b/xen/arch/riscv/include/asm/event.h
> @@ -24,9 +24,9 @@ static inline void local_event_delivery_
>  }
>  
>  /* No arch specific virq definition now. Default to global. */
> -static inline bool arch_virq_is_global(unsigned int virq)
> +static inline enum virq_type arch_get_virq_type(unsigned int virq)
>  {
> -    return true;
> +    return VIRQ_GLOBAL;
>  }
>  
>  #endif /* ASM__RISCV__EVENT_H */
> --- a/xen/arch/x86/include/asm/event.h
> +++ b/xen/arch/x86/include/asm/event.h
> @@ -41,10 +41,10 @@ static inline void local_event_delivery_
>      vcpu_info(current, evtchn_upcall_mask) = 0;
>  }
>  
> -/* No arch specific virq definition now. Default to global. */
> -static inline bool arch_virq_is_global(unsigned int virq)
> +/* Only global arch specific virq definitions. */
> +static inline enum virq_type arch_get_virq_type(unsigned int virq)
>  {
> -    return true;
> +    return VIRQ_GLOBAL;
>  }
>  
>  #ifdef CONFIG_PV_SHIM
> --- a/xen/common/argo.c
> +++ b/xen/common/argo.c
> @@ -440,7 +440,7 @@ signal_domain(struct domain *d)
>  {
>      argo_dprintk("signalling domid:%u\n", d->domain_id);
>  
> -    send_guest_global_virq(d, VIRQ_ARGO);
> +    send_guest_domain_virq(d, VIRQ_ARGO);
>  }
>  
>  static void
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -127,7 +127,7 @@ static struct domain *get_global_virq_ha
>      return global_virq_handlers[virq] ?: hardware_domain;
>  }
>  
> -static bool virq_is_global(unsigned int virq)
> +static enum virq_type get_virq_type(unsigned int virq)
>  {
>      switch ( virq )
>      {
> @@ -135,14 +135,17 @@ static bool virq_is_global(unsigned int
>      case VIRQ_DEBUG:
>      case VIRQ_XENOPROF:
>      case VIRQ_XENPMU:
> -        return false;
> +        return VIRQ_VCPU;
> +
> +    case VIRQ_ARGO:
> +        return VIRQ_DOMAIN;
>  
>      case VIRQ_ARCH_0 ... VIRQ_ARCH_7:
> -        return arch_virq_is_global(virq);
> +        return arch_get_virq_type(virq);
>      }
>  
>      ASSERT(virq < NR_VIRQS);
> -    return true;
> +    return VIRQ_GLOBAL;
>  }
>  
>  static struct evtchn *_evtchn_from_port(const struct domain *d,
> @@ -476,7 +479,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t
>      struct domain *d = current->domain;
>      int            virq = bind->virq, vcpu = bind->vcpu;
>      int            rc = 0;
> -    bool           is_global;
> +    enum virq_type type;
>  
>      if ( (virq < 0) || (virq >= ARRAY_SIZE(v->virq_to_evtchn)) )
>          return -EINVAL;
> @@ -486,9 +489,9 @@ int evtchn_bind_virq(evtchn_bind_virq_t
>      * speculative execution.
>      */
>      virq = array_index_nospec(virq, ARRAY_SIZE(v->virq_to_evtchn));
> -    is_global = virq_is_global(virq);
> +    type = get_virq_type(virq);
>  
> -    if ( is_global && vcpu != 0 )
> +    if ( type != VIRQ_VCPU && vcpu != 0 )
>          return -EINVAL;
>  
>      if ( (v = domain_vcpu(d, vcpu)) == NULL )
> @@ -496,7 +499,7 @@ int evtchn_bind_virq(evtchn_bind_virq_t
>  
>      write_lock(&d->event_lock);
>  
> -    if ( is_global && get_global_virq_handler(virq) != d )
> +    if ( type == VIRQ_GLOBAL && get_global_virq_handler(virq) != d )
>      {
>          rc = -EBUSY;
>          goto out;
> @@ -756,7 +759,8 @@ int evtchn_close(struct domain *d1, int
>          if ( chn1->u.virq == VIRQ_DOM_EXC )
>              domain_deinit_states(d1);
>  
> -        v = d1->vcpu[virq_is_global(chn1->u.virq) ? 0 : 
> chn1->notify_vcpu_id];
> +        v = d1->vcpu[get_virq_type(chn1->u.virq) != VIRQ_VCPU
> +            ? 0 : chn1->notify_vcpu_id];
>  
>          write_lock_irqsave(&v->virq_lock, flags);
>          ASSERT(read_atomic(&v->virq_to_evtchn[chn1->u.virq]) == port1);
> @@ -900,7 +904,7 @@ bool evtchn_virq_enabled(const struct vc
>      if ( !v )
>          return false;
>  
> -    if ( virq_is_global(virq) && v->vcpu_id )
> +    if ( get_virq_type(virq) != VIRQ_VCPU && v->vcpu_id )
>          v = domain_vcpu(v->domain, 0);
>  
>      return read_atomic(&v->virq_to_evtchn[virq]);
> @@ -913,7 +917,7 @@ void send_guest_vcpu_virq(struct vcpu *v
>      struct domain *d;
>      struct evtchn *chn;
>  
> -    ASSERT(!virq_is_global(virq));
> +    ASSERT(get_virq_type(virq) == VIRQ_VCPU);
>  
>      read_lock_irqsave(&v->virq_lock, flags);
>  
> @@ -933,14 +937,14 @@ void send_guest_vcpu_virq(struct vcpu *v
>      read_unlock_irqrestore(&v->virq_lock, flags);
>  }
>  
> -void send_guest_global_virq(struct domain *d, uint32_t virq)
> +void send_guest_domain_virq(struct domain *d, uint32_t virq)
>  {
>      unsigned long flags;
>      int port;
>      struct vcpu *v;
>      struct evtchn *chn;
>  
> -    ASSERT(virq_is_global(virq));
> +    ASSERT(get_virq_type(virq) != VIRQ_VCPU);
>  
>      if ( unlikely(d == NULL) || unlikely(d->vcpu == NULL) )
>          return;
> @@ -995,9 +999,9 @@ static DEFINE_SPINLOCK(global_virq_handl
>  
>  void send_global_virq(uint32_t virq)
>  {
> -    ASSERT(virq_is_global(virq));
> +    ASSERT(get_virq_type(virq) == VIRQ_GLOBAL);
>  
> -    send_guest_global_virq(get_global_virq_handler(virq), virq);
> +    send_guest_domain_virq(get_global_virq_handler(virq), virq);
>  }
>  
>  int set_global_virq_handler(struct domain *d, uint32_t virq)
> @@ -1008,7 +1012,7 @@ int set_global_virq_handler(struct domai
>  
>      if (virq >= NR_VIRQS)
>          return -EINVAL;
> -    if (!virq_is_global(virq))
> +    if (get_virq_type(virq) != VIRQ_GLOBAL)
>          return -EINVAL;
>  
>      if (global_virq_handlers[virq] == d)
> @@ -1204,7 +1208,7 @@ int evtchn_bind_vcpu(evtchn_port_t port,
>      switch ( chn->state )
>      {
>      case ECS_VIRQ:
> -        if ( virq_is_global(chn->u.virq) )
> +        if ( get_virq_type(chn->u.virq) != VIRQ_VCPU )
>              chn->notify_vcpu_id = v->vcpu_id;
>          else
>              rc = -EINVAL;
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -24,17 +24,17 @@
>  void send_guest_vcpu_virq(struct vcpu *v, uint32_t virq);
>  
>  /*
> - * send_global_virq: Notify the domain handling a global VIRQ.
> - *  @virq:     Virtual IRQ number (VIRQ_*)
> + * send_guest_domain_virq:
> + *  @d:        Domain to which VIRQ should be sent
> + *  @virq:     Virtual IRQ number (VIRQ_*), may not be per-vCPU
>   */
> -void send_global_virq(uint32_t virq);
> +void send_guest_domain_virq(struct domain *d, uint32_t virq);
>  
>  /*
> - * send_guest_global_virq:
> - *  @d:        Domain to which VIRQ should be sent
> - *  @virq:     Virtual IRQ number (VIRQ_*), must be global
> + * send_global_virq: Notify the domain handling a global VIRQ.
> + *  @virq:     Virtual IRQ number (VIRQ_*)
>   */
> -void send_guest_global_virq(struct domain *d, uint32_t virq);
> +void send_global_virq(uint32_t virq);
>  
>  /*
>   * sent_global_virq_handler: Set a global VIRQ handler.
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -84,6 +84,12 @@ extern domid_t hardware_domid;
>  #define XEN_CONSUMER_BITS 3
>  #define NR_XEN_CONSUMERS ((1 << XEN_CONSUMER_BITS) - 1)
>  
> +enum virq_type {
> +    VIRQ_GLOBAL,
> +    VIRQ_DOMAIN,
> +    VIRQ_VCPU
> +};
> +
>  struct evtchn
>  {
>      rwlock_t lock;