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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@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.

So I should, yes.

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

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