Re: [PATCH] xen/arm: Move static event channel feature to a separate module

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien,

On 29/11/2023 18:17, Julien Grall wrote:
> 
> 
> Hi Michal
> 
> On 29/11/2023 16:34, Michal Orzel wrote:
>> Move static event channel feature related code to a separate module
>> (static-evtchn.{c,h}) in the spirit of fine granular configuration, so
>> that the feature can be disabled if not needed.
>>
>> Introduce Kconfig option CONFIG_STATIC_EVTCHN, enabled by default (to
>> keep the current behavior) dependent on CONFIG_DOM0LESS. While it could
>> be possible to create a loopback connection for dom0 only, this use case
>> does not really need this feature and all the docs and commit messages
>> refer explicitly to the use in dom0less system.
>>
>> The only function visible externally is alloc_static_evtchn(), so move
>> the prototype to static-evtchn.h and provide a stub in case a feature
>> is disabled. Guard static_evtchn_created in struct dt_device_node and
>> move its helpers to static-evtchn.h.
> 
> I guess this is a matter of taste, but I think
> dt_device_set_static_evtchn_created() and
> dt_device_static_evtchn_created() are better suited in device_tree.h
> because they are touching a field in the device tree structure.
> 
> This would stay consistent with the helper dt_device_set_protected()
> which is only used by the IOMMU code yet is defined in device_tree.h.
Good point about consistency. I will keep the helpers in device_tree.h + add 
guards.

> 
> That said, I could settle on defining the two helpers in the *.c
> directly because they are not meant to be used outside of a single *.c.
> 
> Simarly...
> 
>> diff --git a/xen/arch/arm/include/asm/static-evtchn.h 
>> b/xen/arch/arm/include/asm/static-evtchn.h
>> new file mode 100644
>> index 000000000000..472673fae345
>> --- /dev/null
>> +++ b/xen/arch/arm/include/asm/static-evtchn.h
>> @@ -0,0 +1,41 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef __ASM_STATIC_EVTCHN_H_
>> +#define __ASM_STATIC_EVTCHN_H_
>> +
>> +#ifdef CONFIG_STATIC_EVTCHN
>> +
>> +#include <xen/device_tree.h>
>> +
>> +#define STATIC_EVTCHN_NODE_SIZE_CELLS 2
> 
> ... this used to be defined in domain_build.c. AFAICT the only use is
> now in static-evtchn.c. So why is it defined in the header?
> 
> If this is moved in the *.c, then the header static-evtchn.h would only
> contain alloc_static_evtchn(). This could be moved in domain_build.h or
> setup.h (I don't have any preference).
Apart from a prototype, we still need a stub. Therefore I would prefer to still 
have a header (will
be needed for future upgrades e.g. port exposure in fdt) and move the prototype 
and a stub there (the macro
I can move to *.c). It just looks better for me + we reduce ifdefery in 
domain_build/setup.h.
Would you be ok with that?

~Michal