Re: [PATCH] ARM/vgic: Fix variable shadowing in vgic_to_sgi()

[Michal Orzel <michal.orzel@xxxxxxx>]

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@xxxxxxxxxx>
> ---
> CC: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> CC: Julien Grall <julien@xxxxxxx>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>
> CC: Bertrand Marquis <bertrand.marquis@xxxxxxx>
> CC: Michal Orzel <michal.orzel@xxxxxxx>
> ---
>  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@xxxxxxx>

~Michal