Re: [PATCH] xen/arm: Create GIC node using the node name from host dt

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2965 bytes --]


On 2/20/25 9:00 AM, Orzel, Michal wrote:
>
> On 20/02/2025 03:26, Stefano Stabellini wrote:
>>
>> On Wed, 19 Feb 2025, Michal Orzel wrote:
>>> At the moment the GIC node we create for hwdom has a name
>>> "interrupt-controller". Change it so that we use the same name as the
>>> GIC node from host device tree. This is done for at least 2 purposes:
>>> 1) The convention in DT spec is that a node name with "reg" property
>>> is formed "node-name@unit-address".
>>> 2) With DT overlay feature, many overlays refer to the GIC node using
>>> the symbol under __symbols__ that we copy to hwdom 1:1. With the name
>>> changed, the symbol is no longer valid and requires error prone manual
>>> change by the user.
>>>
>>> The unit-address part of the node name always refers to the first
>>> address in the "reg" property which in case of GIC, always refers to
>>> GICD and hwdom uses host memory layout.
>>>
>>> Signed-off-by: Michal Orzel<michal.orzel@amd.com>
>> Reviewed-by: Stefano Stabellini<sstabellini@kernel.org>
>>
>> While this fix changes behavior for everyone, so it is risky at RC5, it
>> also fixes bugs with DT overlays, but that is an experimental feature. I
>> am in two minds whether it should go in right now or not. Maybe I would
>> wait until 4.20 is out and commit when the tree reopens.
> Technically this is not a bug, hence no Fixes tag. I'm fine with this patch not
> landing in 4.20. That said, I don't agree with what you wrote about a change in
> behavior. There is no functional change at all. Only the node name change. It
> could impact only those OSes that parse by the exact name which would be super
> irrational and wrong. The only way one should parse intc is by searching for
> "interrupt-controller" property as written in DT spec.

I would prefer to have this changes when the tree reopens.

~ Oleksii

>>
>>> ---
>>>   xen/arch/arm/domain_build.c | 7 ++++++-
>>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
>>> index 7b47abade196..e760198d8609 100644
>>> --- a/xen/arch/arm/domain_build.c
>>> +++ b/xen/arch/arm/domain_build.c
>>> @@ -1615,6 +1615,7 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>>>       int res = 0;
>>>       const void *addrcells, *sizecells;
>>>       u32 addrcells_len, sizecells_len;
>>> +    const char *name;
>>>
>>>       /*
>>>        * Xen currently supports only a single GIC. Discard any secondary
>>> @@ -1628,7 +1629,11 @@ static int __init make_gic_node(const struct domain *d, void *fdt,
>>>
>>>       dt_dprintk("Create gic node\n");
>>>
>>> -    res = fdt_begin_node(fdt, "interrupt-controller");
>>> +    /* Use the same name as the GIC node in host device tree */
>>> +    name = strrchr(gic->full_name, '/');
>>> +    name = name ? name + 1 : gic->full_name;
>>> +
>>> +    res = fdt_begin_node(fdt, name);
>>>       if ( res )
>>>           return res;
>>>
>>> --
>>> 2.25.1
>>>

[-- Attachment #2: Type: text/html, Size: 3977 bytes --]