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

[Julien Grall <julien@xxxxxxx>]

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.
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).
Cheers,

--
Julien Grall