Re: [PATCH v2] xen/common: Move gic_preinit() to common code

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.10.2024 16:13, oleksii.kurochko@xxxxxxxxx wrote:
> On Thu, 2024-10-31 at 11:29 +0100, Michal Orzel wrote:
>>
>>
>> On 31/10/2024 10:34, Jan Beulich wrote:
>>>
>>>
>>> On 30.10.2024 14:14, Oleksii Kurochko wrote:
>>>> Introduce ic_preinit() in the common codebase, as it is not
>>>> architecture-specific and can be reused by both PPC and RISC-V.
>>>> This function identifies the node with the interrupt-controller
>>>> property
>>>> in the device tree and calls device_init() to handle
>>>> architecture-specific
>>>> initialization of the interrupt controller.
>>>>
>>>> Additionally, rename gic_acpi_preinit() to ic_acpi_preinit() as
>>>> it is used
>>>> by ic_preinit(), while keeping it defined in architecture-
>>>> specific as this
>>>> part is architecture-specific. In case if CONFIG_ACPI=n a stub
>>>> for
>>>> ic_acpi_preinit() is provided. To declaration/defintion of
>>>> ic_acpi_preint()
>>>> is added `inline` to deal with the compilation issue:
>>>>   error: 'ic_acpi_preinit' defined but not used [-Werror=unused-
>>>> function]
>>>>
>>>> Make minor adjustments compared to the original ARM
>>>> implementation of
>>>> gic_dt_preinit():
>>>>  - Remove the local rc variable in gic_dt_preinit() since it is
>>>> only used once.
>>>>  - Change the prefix from gic to ic to clarify that the function
>>>> is not
>>>>    specific to ARM’s GIC, making it suitable for other
>>>> architectures as well.
>>>>
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>> ---
>>>> Changes in v2:
>>>>  - Revert changes connected to moving of gic_acpi_preinit() to
>>>> common code as
>>>>    it isn't really architecture indepent part.
>>>>  - Update the commit message.
>>>>  - Move stub of ic_acpi_preinit() to <asm-generic/device.h> for
>>>> the case when
>>>>    CONFIG_ACPI=n.
>>>> ---
>>>>  xen/arch/arm/gic.c               | 45 +-------------------------
>>>> -----
>>>>  xen/arch/arm/setup.c             |  3 ++-
>>>>  xen/common/device.c              | 46
>>>> ++++++++++++++++++++++++++++++++
>>>>  xen/include/asm-generic/device.h | 10 +++++++
>>>>  4 files changed, 59 insertions(+), 45 deletions(-)
>>>
>>> Looks largely okay to me now, with a question and a nit at the
>>> bottom. The
>>> question is mainly to Arm folks, where the code is coming from, and
>>> DT
>>> maintainers: Is a file named device.c really an appropriate "home"
>>> for
>>> IC-related code? If IC is a common thing in the DT world, would
>>> such code
>>> maybe better live under common/device-tree/?
>> I think we will have more interrupt related dt stuff common, so it
>> would make sense to create
>> device-tree/irq.c or intc.c.
> What about the part of the code in common/device.c:
>    void __init ic_preinit(void)
>    {
>        if ( acpi_disabled )
>            ic_dt_preinit();
>        else
>            ic_acpi_preinit();
>    }
>    
> Should it be also moved to device-tree/intc.c even ic_acpi_preinit() is
> used?

Maybe best left to each arch? device.[ch] aren't a good home for this,
just like they aren't for the actual DT implementation.

Jan