for_each_set_bit() allocates its own variable intentionally as loop-scope
only.  Unfortunately, this causes the inner 'i' to shadow the outer 'i'.

Drop the outermost 'i' and 'vcpuid' variables, moving them into a more narrow
scope and correcting them to be unsigned which they should have been all
along.

Fixes: 9429f1a6c475 ("ARM/vgic: Use for_each_set_bit() in vgic_to_sgi()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Bertrand Marquis <bertrand.marquis@arm.com>
CC: Michal Orzel <michal.orzel@amd.com>
---
 xen/arch/arm/vgic.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 8ffe099bcb5f..6ecd9146511c 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -468,8 +468,6 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
                  int virq, const struct sgi_target *target)
 {
     struct domain *d = v->domain;
-    int vcpuid;
-    int i;
     unsigned int base, bitmap;
 
     ASSERT( virq < 16 );
@@ -483,7 +481,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
 
         for_each_set_bit ( i, bitmap )
         {
-            vcpuid = base + i;
+            unsigned int vcpuid = base + i;
+
             if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ||
                  !is_vcpu_online(d->vcpu[vcpuid]) )
             {
@@ -497,7 +496,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
         break;
     case SGI_TARGET_OTHERS:
         perfc_incr(vgic_sgi_others);
-        for ( i = 0; i < d->max_vcpus; i++ )
+        for ( unsigned int i = 0; i < d->max_vcpus; i++ )
         {
             if ( i != current->vcpu_id && d->vcpu[i] != NULL &&
                  is_vcpu_online(d->vcpu[i]) )

base-commit: b8cdfac2be38c98dd3ad0e911a3f7f787f5bcf6b
prerequisite-patch-id: 87415b68ed015b8f36405b2554f2abd6c02f28aa
prerequisite-patch-id: d87fe52c264dc5a33883a04b615043fbefd94f92
prerequisite-patch-id: abb68a851297bbf63c16093c6362c0d4b9c39334
-- 
2.39.2

On 28/08/2024 01:45, Andrew Cooper wrote:
> 
> 
> for_each_set_bit() allocates its own variable intentionally as loop-scope
> only.  Unfortunately, this causes the inner 'i' to shadow the outer 'i'.
NIT: I'd mention it violates MISRA R5.3

> 
> Drop the outermost 'i' and 'vcpuid' variables, moving them into a more narrow
> scope and correcting them to be unsigned which they should have been all
> along.
> 
> Fixes: 9429f1a6c475 ("ARM/vgic: Use for_each_set_bit() in vgic_to_sgi()")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Bertrand Marquis <bertrand.marquis@arm.com>
> CC: Michal Orzel <michal.orzel@amd.com>
> ---
>  xen/arch/arm/vgic.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> index 8ffe099bcb5f..6ecd9146511c 100644
> --- a/xen/arch/arm/vgic.c
> +++ b/xen/arch/arm/vgic.c
> @@ -468,8 +468,6 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
>                   int virq, const struct sgi_target *target)
>  {
>      struct domain *d = v->domain;
> -    int vcpuid;
> -    int i;
>      unsigned int base, bitmap;
> 
>      ASSERT( virq < 16 );
> @@ -483,7 +481,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
> 
>          for_each_set_bit ( i, bitmap )
>          {
> -            vcpuid = base + i;
> +            unsigned int vcpuid = base + i;
With this change you should replace the printk specifier from %d to %u for vcpuid.

Apart from that:
Reviewed-by: Michal Orzel <michal.orzel@amd.com>

~Michal

On 28/08/2024 8:06 am, Michal Orzel wrote:
>
> On 28/08/2024 01:45, Andrew Cooper wrote:
>>
>> for_each_set_bit() allocates its own variable intentionally as loop-scope
>> only.  Unfortunately, this causes the inner 'i' to shadow the outer 'i'.
> NIT: I'd mention it violates MISRA R5.3

Done.

>
>> Drop the outermost 'i' and 'vcpuid' variables, moving them into a more narrow
>> scope and correcting them to be unsigned which they should have been all
>> along.
>>
>> Fixes: 9429f1a6c475 ("ARM/vgic: Use for_each_set_bit() in vgic_to_sgi()")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien@xen.org>
>> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
>> CC: Bertrand Marquis <bertrand.marquis@arm.com>
>> CC: Michal Orzel <michal.orzel@amd.com>
>> ---
>>  xen/arch/arm/vgic.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 8ffe099bcb5f..6ecd9146511c 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -468,8 +468,6 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
>>                   int virq, const struct sgi_target *target)
>>  {
>>      struct domain *d = v->domain;
>> -    int vcpuid;
>> -    int i;
>>      unsigned int base, bitmap;
>>
>>      ASSERT( virq < 16 );
>> @@ -483,7 +481,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum gic_sgi_mode irqmode,
>>
>>          for_each_set_bit ( i, bitmap )
>>          {
>> -            vcpuid = base + i;
>> +            unsigned int vcpuid = base + i;
> With this change you should replace the printk specifier from %d to %u for vcpuid.

So I should, yes.

>
> Apart from that:
> Reviewed-by: Michal Orzel <michal.orzel@amd.com>

Thanks.  Sorry for breaking CI.

I did run Eclair on it, but for the manually triggered run, it only
generates a warning, and does not trigger a "pipeline {failed,fixed}"
event, because the pipeline hasn't changed state from the last time I
ran it.

I'm not sure if we can do any better for something which is
conditionally manually triggered.

~Andrew