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

[Jan Beulich <jbeulich@xxxxxxxx>]

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