| Message ID | 20250219172946.359234-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Create GIC node using the node name from host dt | expand |
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. > --- > 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 >
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. ~Michal > > >> --- >> 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 >>
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 >>>
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;
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> --- xen/arch/arm/domain_build.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)