Reduce the scope of every variable so they are reinitialised. "iommu",
for instance, isn't being cleared, so the wrong flags may make it to
domains that should not have them.

Fixes: 1d2b4f3049fd("xen/arm, doc: Add a DT property to specify...")
Signed-off-by: Alejandro Vallejo <alejandro.garciavallejo@amd.com>
---
This is implicitly fixed in the next patch, but I'm sending this
standalone so it can be backported where relevant.
---
 xen/common/device-tree/dom0less-build.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/common/device-tree/dom0less-build.c b/xen/common/device-tree/dom0less-build.c
index ef4b095d97..676a3317cf 100644
--- a/xen/common/device-tree/dom0less-build.c
+++ b/xen/common/device-tree/dom0less-build.c
@@ -826,14 +826,14 @@ static int __init construct_domU(struct kernel_info *kinfo,
 void __init create_domUs(void)
 {
     struct dt_device_node *node;
-    const char *dom0less_iommu;
-    bool iommu = false;
-    const struct dt_device_node *cpupool_node,
-                                *chosen = dt_find_node_by_path("/chosen");
+    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
 
     BUG_ON(chosen == NULL);
     dt_for_each_child_node(chosen, node)
     {
+        const char *dom0less_iommu;
+        bool iommu = false;
+        const struct dt_device_node *cpupool_node;
         struct kernel_info ki = KERNEL_INFO_INIT;
         struct xen_domctl_createdomain *d_cfg = &ki.bd.create_cfg;
         unsigned int *flags = &ki.bd.create_flags;
-- 
2.43.0

On 22.07.2025 13:59, Alejandro Vallejo wrote:
> Reduce the scope of every variable so they are reinitialised. "iommu",
> for instance, isn't being cleared, so the wrong flags may make it to
> domains that should not have them.

Yet "for instance" isn't quite right, is it? "iommu" is the only one where
the (re)init was misplaced. The other two ...

> --- a/xen/common/device-tree/dom0less-build.c
> +++ b/xen/common/device-tree/dom0less-build.c
> @@ -826,14 +826,14 @@ static int __init construct_domU(struct kernel_info *kinfo,
>  void __init create_domUs(void)
>  {
>      struct dt_device_node *node;
> -    const char *dom0less_iommu;
> -    bool iommu = false;
> -    const struct dt_device_node *cpupool_node,
> -                                *chosen = dt_find_node_by_path("/chosen");
> +    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>  
>      BUG_ON(chosen == NULL);
>      dt_for_each_child_node(chosen, node)
>      {
> +        const char *dom0less_iommu;
> +        bool iommu = false;
> +        const struct dt_device_node *cpupool_node;

... had no initializer, and also don't gain any. So they must both be
set inside the loop. (Irrespective, the scope reduction is a good thing
imo.)

Jan

On Tue Jul 22, 2025 at 2:18 PM CEST, Jan Beulich wrote:
> On 22.07.2025 13:59, Alejandro Vallejo wrote:
>> Reduce the scope of every variable so they are reinitialised. "iommu",
>> for instance, isn't being cleared, so the wrong flags may make it to
>> domains that should not have them.
>
> Yet "for instance" isn't quite right, is it? "iommu" is the only one where
> the (re)init was misplaced. The other two ...

We do strive for minimal scope where possible. But you're right "for instance"
might be misleading in suggesting there's more bugs than one.

I'm happy to have "for instance" removed, leaving the rest as-is, if that works
for you.

>
>> --- a/xen/common/device-tree/dom0less-build.c
>> +++ b/xen/common/device-tree/dom0less-build.c
>> @@ -826,14 +826,14 @@ static int __init construct_domU(struct kernel_info *kinfo,
>>  void __init create_domUs(void)
>>  {
>>      struct dt_device_node *node;
>> -    const char *dom0less_iommu;
>> -    bool iommu = false;
>> -    const struct dt_device_node *cpupool_node,
>> -                                *chosen = dt_find_node_by_path("/chosen");
>> +    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>>  
>>      BUG_ON(chosen == NULL);
>>      dt_for_each_child_node(chosen, node)
>>      {
>> +        const char *dom0less_iommu;
>> +        bool iommu = false;
>> +        const struct dt_device_node *cpupool_node;
>
> ... had no initializer, and also don't gain any. So they must both be
> set inside the loop. (Irrespective, the scope reduction is a good thing
> imo.)
>
> Jan

Cheers,
Alejandro

On 22.07.2025 14:37, Alejandro Vallejo wrote:
> On Tue Jul 22, 2025 at 2:18 PM CEST, Jan Beulich wrote:
>> On 22.07.2025 13:59, Alejandro Vallejo wrote:
>>> Reduce the scope of every variable so they are reinitialised. "iommu",
>>> for instance, isn't being cleared, so the wrong flags may make it to
>>> domains that should not have them.
>>
>> Yet "for instance" isn't quite right, is it? "iommu" is the only one where
>> the (re)init was misplaced. The other two ...
> 
> We do strive for minimal scope where possible. But you're right "for instance"
> might be misleading in suggesting there's more bugs than one.
> 
> I'm happy to have "for instance" removed, leaving the rest as-is, if that works
> for you.

Except that "every" isn't quite right either. Nor is "they".

Jan

>>> --- a/xen/common/device-tree/dom0less-build.c
>>> +++ b/xen/common/device-tree/dom0less-build.c
>>> @@ -826,14 +826,14 @@ static int __init construct_domU(struct kernel_info *kinfo,
>>>  void __init create_domUs(void)
>>>  {
>>>      struct dt_device_node *node;
>>> -    const char *dom0less_iommu;
>>> -    bool iommu = false;
>>> -    const struct dt_device_node *cpupool_node,
>>> -                                *chosen = dt_find_node_by_path("/chosen");
>>> +    const struct dt_device_node *chosen = dt_find_node_by_path("/chosen");
>>>  
>>>      BUG_ON(chosen == NULL);
>>>      dt_for_each_child_node(chosen, node)
>>>      {
>>> +        const char *dom0less_iommu;
>>> +        bool iommu = false;
>>> +        const struct dt_device_node *cpupool_node;
>>
>> ... had no initializer, and also don't gain any. So they must both be
>> set inside the loop. (Irrespective, the scope reduction is a good thing
>> imo.)
>>
>> Jan
> 
> Cheers,
> Alejandro

On Tue Jul 22, 2025 at 2:57 PM CEST, Jan Beulich wrote:
> On 22.07.2025 14:37, Alejandro Vallejo wrote:
>> On Tue Jul 22, 2025 at 2:18 PM CEST, Jan Beulich wrote:
>>> On 22.07.2025 13:59, Alejandro Vallejo wrote:
>>>> Reduce the scope of every variable so they are reinitialised. "iommu",
>>>> for instance, isn't being cleared, so the wrong flags may make it to
>>>> domains that should not have them.
>>>
>>> Yet "for instance" isn't quite right, is it? "iommu" is the only one where
>>> the (re)init was misplaced. The other two ...
>> 
>> We do strive for minimal scope where possible. But you're right "for instance"
>> might be misleading in suggesting there's more bugs than one.
>> 
>> I'm happy to have "for instance" removed, leaving the rest as-is, if that works
>> for you.
>
> Except that "every" isn't quite right either. Nor is "they".
>
> Jan

Ok, take 3:

	Reduce the scope of dom0less_iommu, iommu and cpupool_node. iommu, in
	particular, wasn't being cleared, so the wrong flags may make it to
	domains that should not have them.

Cheers,
Alejandro

On 22.07.2025 15:31, Alejandro Vallejo wrote:
> On Tue Jul 22, 2025 at 2:57 PM CEST, Jan Beulich wrote:
>> On 22.07.2025 14:37, Alejandro Vallejo wrote:
>>> On Tue Jul 22, 2025 at 2:18 PM CEST, Jan Beulich wrote:
>>>> On 22.07.2025 13:59, Alejandro Vallejo wrote:
>>>>> Reduce the scope of every variable so they are reinitialised. "iommu",
>>>>> for instance, isn't being cleared, so the wrong flags may make it to
>>>>> domains that should not have them.
>>>>
>>>> Yet "for instance" isn't quite right, is it? "iommu" is the only one where
>>>> the (re)init was misplaced. The other two ...
>>>
>>> We do strive for minimal scope where possible. But you're right "for instance"
>>> might be misleading in suggesting there's more bugs than one.
>>>
>>> I'm happy to have "for instance" removed, leaving the rest as-is, if that works
>>> for you.
>>
>> Except that "every" isn't quite right either. Nor is "they".
> 
> Ok, take 3:
> 
> 	Reduce the scope of dom0less_iommu, iommu and cpupool_node. iommu, in
> 	particular, wasn't being cleared, so the wrong flags may make it to
> 	domains that should not have them.

Fine with me, thanks.

Jan

On Tue, 22 Jul 2025, Jan Beulich wrote:
> On 22.07.2025 15:31, Alejandro Vallejo wrote:
> > On Tue Jul 22, 2025 at 2:57 PM CEST, Jan Beulich wrote:
> >> On 22.07.2025 14:37, Alejandro Vallejo wrote:
> >>> On Tue Jul 22, 2025 at 2:18 PM CEST, Jan Beulich wrote:
> >>>> On 22.07.2025 13:59, Alejandro Vallejo wrote:
> >>>>> Reduce the scope of every variable so they are reinitialised. "iommu",
> >>>>> for instance, isn't being cleared, so the wrong flags may make it to
> >>>>> domains that should not have them.
> >>>>
> >>>> Yet "for instance" isn't quite right, is it? "iommu" is the only one where
> >>>> the (re)init was misplaced. The other two ...
> >>>
> >>> We do strive for minimal scope where possible. But you're right "for instance"
> >>> might be misleading in suggesting there's more bugs than one.
> >>>
> >>> I'm happy to have "for instance" removed, leaving the rest as-is, if that works
> >>> for you.
> >>
> >> Except that "every" isn't quite right either. Nor is "they".
> > 
> > Ok, take 3:
> > 
> > 	Reduce the scope of dom0less_iommu, iommu and cpupool_node. iommu, in
> > 	particular, wasn't being cleared, so the wrong flags may make it to
> > 	domains that should not have them.
> 
> Fine with me, thanks.

With Jan's suggestion:

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>