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

[Jan Beulich <jbeulich@xxxxxxxx>]

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);
>   }
> 
>