xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

[XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Grygorii Strashko 3 months, 2 weeks ago

From: Grygorii Strashko <grygorii_strashko@epam.com>

Enable build time optimization for evtchn_move_pirqs()/send_guest_pirq() on
platforms without PIRQ support by adding compile time check for
!IS_ENABLED(CONFIG_HAS_PIRQ) at the beginning of functions.

This will shrink them to (on Arm64 with -O1):
000000000000264c <send_guest_pirq>:
    264c:	d65f03c0	ret

0000000000004644 <evtchn_move_pirqs>:
    4644:	d65f03c0	ret

Signed-off-by: Grygorii Strashko <grygorii_strashko@epam.com>
---
 xen/common/event_channel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index c8c1bfa615df..ccecdd682dd4 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
     int port;
     struct evtchn *chn;
 
+    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
+        return;
+
     /*
      * PV guests: It should not be possible to race with __evtchn_close(). The
      *     caller of this function must synchronise with pirq_guest_unbind().
@@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
 void evtchn_move_pirqs(struct vcpu *v)
 {
     struct domain *d = v->domain;
-    const cpumask_t *mask = cpumask_of(v->processor);
+    const cpumask_t *mask;
     unsigned int port;
     struct evtchn *chn;
 
+    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
+        return;
+
+    mask = cpumask_of(v->processor);
+
     read_lock(&d->event_lock);
     for ( port = v->pirq_evtchn_head; port; port = chn->u.pirq.next_port )
     {
-- 
2.34.1

Re: [XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Jan Beulich 3 months, 2 weeks ago

On 17.07.2025 15:01, Grygorii Strashko wrote:
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
>      int port;
>      struct evtchn *chn;
>  
> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
> +        return;
> +
>      /*
>       * PV guests: It should not be possible to race with __evtchn_close(). The
>       *     caller of this function must synchronise with pirq_guest_unbind().

Isn't this function unreachable on Arm, and hence a Misra rule 2.1 violation,
requiring #ifdef around the entire function to address?

> @@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
>  void evtchn_move_pirqs(struct vcpu *v)
>  {
>      struct domain *d = v->domain;
> -    const cpumask_t *mask = cpumask_of(v->processor);
> +    const cpumask_t *mask;

This change shouldn't be necessary; compilers ought to be able to DCE the
code.

>      unsigned int port;
>      struct evtchn *chn;
>  
> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))

Nit (style): Missing blanks (see other nearby if()-s).

I wonder though whether we wouldn't better have x86'es arch_move_irqs()
invoke this function, and then #ifdef it out here altogether as well.

Jan

Re: [XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Grygorii Strashko 3 months, 2 weeks ago


On 17.07.25 16:10, Jan Beulich wrote:
> On 17.07.2025 15:01, Grygorii Strashko wrote:
>> --- a/xen/common/event_channel.c
>> +++ b/xen/common/event_channel.c
>> @@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
>>       int port;
>>       struct evtchn *chn;
>>   
>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>> +        return;
>> +
>>       /*
>>        * PV guests: It should not be possible to race with __evtchn_close(). The
>>        *     caller of this function must synchronise with pirq_guest_unbind().
> 
> Isn't this function unreachable on Arm, and hence a Misra rule 2.1 violation,
> requiring #ifdef around the entire function to address?

Yes. It's unused on Arm, only x86 is an user.
I can put it under ifdef.

> 
>> @@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
>>   void evtchn_move_pirqs(struct vcpu *v)
>>   {
>>       struct domain *d = v->domain;
>> -    const cpumask_t *mask = cpumask_of(v->processor);
>> +    const cpumask_t *mask;
> 
> This change shouldn't be necessary; compilers ought to be able to DCE the
> code.

Unfortunately not, with "-O1" more code is generated as cpumask_of() is complicated inside.

> 
>>       unsigned int port;
>>       struct evtchn *chn;
>>   
>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
> 
> Nit (style): Missing blanks (see other nearby if()-s).
> 
> I wonder though whether we wouldn't better have x86'es arch_move_irqs()
> invoke this function, and then #ifdef it out here altogether as well.

Do you mean as in the below diff?

diff --git a/xen/arch/x86/include/asm/irq.h b/xen/arch/x86/include/asm/irq.h
index 3c73073b71b3..6d2bdfc9df1a 100644
--- a/xen/arch/x86/include/asm/irq.h
+++ b/xen/arch/x86/include/asm/irq.h
@@ -224,7 +224,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
  
  bool cpu_has_pending_apic_eoi(void);
  
-static inline void arch_move_irqs(struct vcpu *v) { }
+void arch_move_irqs(struct vcpu *v);
  
  struct msi_info;
  int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p);
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 556134f85aa0..b8d8f202119d 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -1851,6 +1851,10 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
          cleanup_domain_irq_pirq(d, irq, pirq);
  }
  
+void arch_move_irqs(struct vcpu *v) {
+    evtchn_move_pirqs(v);
+}
+
  static bool pirq_guest_force_unbind(struct domain *d, struct pirq *pirq)
  {
      struct irq_desc *desc;
diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 13fdf57e57b9..ad6032fb2865 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -642,7 +642,6 @@ int sched_init_vcpu(struct vcpu *v)
  static void vcpu_move_irqs(struct vcpu *v)
  {
      arch_move_irqs(v);
-    evtchn_move_pirqs(v);
  }


-- 
Best regards,
-grygorii

Re: [XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Jan Beulich 3 months, 2 weeks ago

On 17.07.2025 16:41, Grygorii Strashko wrote:
> 
> 
> On 17.07.25 16:10, Jan Beulich wrote:
>> On 17.07.2025 15:01, Grygorii Strashko wrote:
>>> --- a/xen/common/event_channel.c
>>> +++ b/xen/common/event_channel.c
>>> @@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
>>>       int port;
>>>       struct evtchn *chn;
>>>   
>>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>> +        return;
>>> +
>>>       /*
>>>        * PV guests: It should not be possible to race with __evtchn_close(). The
>>>        *     caller of this function must synchronise with pirq_guest_unbind().
>>
>> Isn't this function unreachable on Arm, and hence a Misra rule 2.1 violation,
>> requiring #ifdef around the entire function to address?
> 
> Yes. It's unused on Arm, only x86 is an user.
> I can put it under ifdef.
> 
>>
>>> @@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
>>>   void evtchn_move_pirqs(struct vcpu *v)
>>>   {
>>>       struct domain *d = v->domain;
>>> -    const cpumask_t *mask = cpumask_of(v->processor);
>>> +    const cpumask_t *mask;
>>
>> This change shouldn't be necessary; compilers ought to be able to DCE the
>> code.
> 
> Unfortunately not, with "-O1" more code is generated as cpumask_of() is complicated inside.
> 
>>
>>>       unsigned int port;
>>>       struct evtchn *chn;
>>>   
>>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>
>> Nit (style): Missing blanks (see other nearby if()-s).
>>
>> I wonder though whether we wouldn't better have x86'es arch_move_irqs()
>> invoke this function, and then #ifdef it out here altogether as well.
> 
> Do you mean as in the below diff?

Along these lines, yes. I guess personally I wouldn't convert to an
out-of-line function. If an inline function fails to compile (and that
isn't easily fixable), use a macro instead.

Jan

> --- a/xen/arch/x86/include/asm/irq.h
> +++ b/xen/arch/x86/include/asm/irq.h
> @@ -224,7 +224,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
>   
>   bool cpu_has_pending_apic_eoi(void);
>   
> -static inline void arch_move_irqs(struct vcpu *v) { }
> +void arch_move_irqs(struct vcpu *v);
>   
>   struct msi_info;
>   int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p);
> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
> index 556134f85aa0..b8d8f202119d 100644
> --- a/xen/arch/x86/irq.c
> +++ b/xen/arch/x86/irq.c
> @@ -1851,6 +1851,10 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
>           cleanup_domain_irq_pirq(d, irq, pirq);
>   }
>   
> +void arch_move_irqs(struct vcpu *v) {
> +    evtchn_move_pirqs(v);
> +}
> +
>   static bool pirq_guest_force_unbind(struct domain *d, struct pirq *pirq)
>   {
>       struct irq_desc *desc;
> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
> index 13fdf57e57b9..ad6032fb2865 100644
> --- a/xen/common/sched/core.c
> +++ b/xen/common/sched/core.c
> @@ -642,7 +642,6 @@ int sched_init_vcpu(struct vcpu *v)
>   static void vcpu_move_irqs(struct vcpu *v)
>   {
>       arch_move_irqs(v);
> -    evtchn_move_pirqs(v);
>   }
> 
>

Re: [XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Grygorii Strashko 3 months, 2 weeks ago


On 17.07.25 18:33, Jan Beulich wrote:
> On 17.07.2025 16:41, Grygorii Strashko wrote:
>>
>>
>> On 17.07.25 16:10, Jan Beulich wrote:
>>> On 17.07.2025 15:01, Grygorii Strashko wrote:
>>>> --- a/xen/common/event_channel.c
>>>> +++ b/xen/common/event_channel.c
>>>> @@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
>>>>        int port;
>>>>        struct evtchn *chn;
>>>>    
>>>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>>> +        return;
>>>> +
>>>>        /*
>>>>         * PV guests: It should not be possible to race with __evtchn_close(). The
>>>>         *     caller of this function must synchronise with pirq_guest_unbind().
>>>
>>> Isn't this function unreachable on Arm, and hence a Misra rule 2.1 violation,
>>> requiring #ifdef around the entire function to address?
>>
>> Yes. It's unused on Arm, only x86 is an user.
>> I can put it under ifdef.
>>
>>>
>>>> @@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
>>>>    void evtchn_move_pirqs(struct vcpu *v)
>>>>    {
>>>>        struct domain *d = v->domain;
>>>> -    const cpumask_t *mask = cpumask_of(v->processor);
>>>> +    const cpumask_t *mask;
>>>
>>> This change shouldn't be necessary; compilers ought to be able to DCE the
>>> code.
>>
>> Unfortunately not, with "-O1" more code is generated as cpumask_of() is complicated inside.
>>
>>>
>>>>        unsigned int port;
>>>>        struct evtchn *chn;
>>>>    
>>>> +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>>
>>> Nit (style): Missing blanks (see other nearby if()-s).
>>>
>>> I wonder though whether we wouldn't better have x86'es arch_move_irqs()
>>> invoke this function, and then #ifdef it out here altogether as well.
>>
>> Do you mean as in the below diff?
> 
> Along these lines, yes. I guess personally I wouldn't convert to an
> out-of-line function. If an inline function fails to compile (and that
> isn't easily fixable), use a macro instead.

I'd prefer stick to out-of-line function, if you don't mind.
Inline implementation causes cascade build failure:

adding
  #include <xen/event.h>
  #include <xen/sched.h>
in irq.h is not enough.

> 
>> --- a/xen/arch/x86/include/asm/irq.h
>> +++ b/xen/arch/x86/include/asm/irq.h
>> @@ -224,7 +224,7 @@ void cleanup_domain_irq_mapping(struct domain *d);
>>    
>>    bool cpu_has_pending_apic_eoi(void);
>>    
>> -static inline void arch_move_irqs(struct vcpu *v) { }
>> +void arch_move_irqs(struct vcpu *v);
>>    
>>    struct msi_info;
>>    int allocate_and_map_gsi_pirq(struct domain *d, int index, int *pirq_p);
>> diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
>> index 556134f85aa0..b8d8f202119d 100644
>> --- a/xen/arch/x86/irq.c
>> +++ b/xen/arch/x86/irq.c
>> @@ -1851,6 +1851,10 @@ void pirq_guest_unbind(struct domain *d, struct pirq *pirq)
>>            cleanup_domain_irq_pirq(d, irq, pirq);
>>    }
>>    
>> +void arch_move_irqs(struct vcpu *v) {
>> +    evtchn_move_pirqs(v);
>> +}
>> +
>>    static bool pirq_guest_force_unbind(struct domain *d, struct pirq *pirq)
>>    {
>>        struct irq_desc *desc;
>> diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
>> index 13fdf57e57b9..ad6032fb2865 100644
>> --- a/xen/common/sched/core.c
>> +++ b/xen/common/sched/core.c
>> @@ -642,7 +642,6 @@ int sched_init_vcpu(struct vcpu *v)
>>    static void vcpu_move_irqs(struct vcpu *v)
>>    {
>>        arch_move_irqs(v);
>> -    evtchn_move_pirqs(v);
>>    }
>>
>>
> 

-- 
Best regards,
-grygorii

Re: [XEN][PATCH] xen/evtchn: enable build optimization for evtchn_move_pirqs()/send_guest_pirq()

Posted by Jan Beulich 3 months, 2 weeks ago

On 17.07.2025 20:55, Grygorii Strashko wrote:
> On 17.07.25 18:33, Jan Beulich wrote:
>> On 17.07.2025 16:41, Grygorii Strashko wrote:
>>> On 17.07.25 16:10, Jan Beulich wrote:
>>>> On 17.07.2025 15:01, Grygorii Strashko wrote:
>>>>> --- a/xen/common/event_channel.c
>>>>> +++ b/xen/common/event_channel.c
>>>>> @@ -975,6 +975,9 @@ void send_guest_pirq(struct domain *d, const struct pirq *pirq)
>>>>>        int port;
>>>>>        struct evtchn *chn;
>>>>>    +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>>>> +        return;
>>>>> +
>>>>>        /*
>>>>>         * PV guests: It should not be possible to race with __evtchn_close(). The
>>>>>         *     caller of this function must synchronise with pirq_guest_unbind().
>>>>
>>>> Isn't this function unreachable on Arm, and hence a Misra rule 2.1 violation,
>>>> requiring #ifdef around the entire function to address?
>>>
>>> Yes. It's unused on Arm, only x86 is an user.
>>> I can put it under ifdef.
>>>
>>>>
>>>>> @@ -1710,10 +1713,15 @@ void evtchn_destroy_final(struct domain *d)
>>>>>    void evtchn_move_pirqs(struct vcpu *v)
>>>>>    {
>>>>>        struct domain *d = v->domain;
>>>>> -    const cpumask_t *mask = cpumask_of(v->processor);
>>>>> +    const cpumask_t *mask;
>>>>
>>>> This change shouldn't be necessary; compilers ought to be able to DCE the
>>>> code.
>>>
>>> Unfortunately not, with "-O1" more code is generated as cpumask_of() is complicated inside.
>>>
>>>>
>>>>>        unsigned int port;
>>>>>        struct evtchn *chn;
>>>>>    +    if (!IS_ENABLED(CONFIG_HAS_PIRQ))
>>>>
>>>> Nit (style): Missing blanks (see other nearby if()-s).
>>>>
>>>> I wonder though whether we wouldn't better have x86'es arch_move_irqs()
>>>> invoke this function, and then #ifdef it out here altogether as well.
>>>
>>> Do you mean as in the below diff?
>>
>> Along these lines, yes. I guess personally I wouldn't convert to an
>> out-of-line function. If an inline function fails to compile (and that
>> isn't easily fixable), use a macro instead.
> 
> I'd prefer stick to out-of-line function, if you don't mind.
> Inline implementation causes cascade build failure:
> 
> adding
>  #include <xen/event.h>
>  #include <xen/sched.h>
> in irq.h is not enough.

Which is why I suggested using a macro; I kind of expected there to be
#include issues.

Jan