Re: [PATCH] ARM/vgic: Use for_each_set_bit() in vgic_to_sgi()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/08/2024 1:13 pm, Michal Orzel wrote:
>
> On 24/08/2024 01:01, Andrew Cooper wrote:
>>
>> The existing expression is just a very complicated way of expressing a loop
>> over all bits of target->list.  Simplify the expression.
>>
>> While here, fix the two gprintk()'s.  Because of a quotes vs line 
>> continuation
>> issue, there's a long string of spaces in the middle of the format string.
>>
>>   $ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR
>>   <G><1>%pv VGIC: write r=%08x                         target->list=%hx, 
>> wrong CPUTargetList
>>   <G><1>%pv vGICD:unhandled GICD_SGIR write %08x                  with wrong 
>> mode
>>
>> not to mention trailing whitespace too.
>>
>> Rewrite them to be more consise and more useful.  Use 0x prefixes for hex,
> s/consise/concise
>
>> rather than ambigous, and identify the problem target vCPU / mode, rather 
>> than
> s/ambigous/ambiguous
>
>> simply saying somethign was wrong.
> s/somethign/something/
>
>> 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>
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>>
>> In a fun twist, we can't use target->list directly in the expresion, because
>> the typeof() picks up constness from the pointer, and we get:
>>
>>   In file included from arch/arm/vgic.c:11:
>>   arch/arm/vgic.c: In function ‘vgic_to_sgi’:
>>   ./include/xen/bitops.h:305:19: error: assignment of read-only variable 
>> ‘__v’
>>     305 |               __v &= __v - 1 )
>>         |                   ^~
>>   arch/arm/vgic.c:483:9: note: in expansion of macro ‘for_each_set_bit’
>>     483 |         for_each_set_bit ( i, target->list )
>>         |         ^~~~~~~~~~~~~~~~
>>
>> Sadly we need -std=c23 before we can use typeof_unqual() which is what we
>> actually want here.
>> ---
>>  xen/arch/arm/vgic.c | 16 ++++++++--------
>>  1 file changed, 8 insertions(+), 8 deletions(-)
>>
>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>> index 7b54ccc7cbfa..081cbb67fb52 100644
>> --- a/xen/arch/arm/vgic.c
>> +++ b/xen/arch/arm/vgic.c
>> @@ -470,8 +470,7 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
>> gic_sgi_mode irqmode,
>>      struct domain *d = v->domain;
>>      int vcpuid;
>>      int i;
>> -    unsigned int base;
>> -    unsigned long int bitmap;
>> +    unsigned int base, bitmap;
>>
>>      ASSERT( virq < 16 );
>>
>> @@ -481,15 +480,16 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
>> gic_sgi_mode irqmode,
>>          perfc_incr(vgic_sgi_list);
>>          base = target->aff1 << 4;
>>          bitmap = target->list;
>> -        bitmap_for_each ( i, &bitmap, sizeof(target->list) * 8 )
>> +
>> +        for_each_set_bit ( i, bitmap )
>>          {
>>              vcpuid = base + i;
>>              if ( vcpuid >= d->max_vcpus || d->vcpu[vcpuid] == NULL ||
>>                   !is_vcpu_online(d->vcpu[vcpuid]) )
>>              {
>> -                gprintk(XENLOG_WARNING, "VGIC: write r=%"PRIregister" \
>> -                        target->list=%hx, wrong CPUTargetList \n",
>> -                        sgir, target->list);
>> +                gprintk(XENLOG_WARNING,
>> +                        "vGIC: write %#"PRIregister", target->list=%#x, bad 
>> target v%d\n",
> Sth like "bad target v2" where the word vcpu does not occur anywhere in the 
> msg can be ambiguous.
> Can you add the word vcpu e.g. "bad vcpu target v%d" or "bad target vcpu %d"

Hmm yeah, v%d doesn't work quite so well when it's not prefixed with d%d.

Would you be happy with d%dv%d?  It's marginally more informative and
shorter.

>
>> +                        sgir, target->list, vcpuid);
>>                  continue;
>>              }
>>              vgic_inject_irq(d, d->vcpu[vcpuid], virq, true);
>> @@ -510,8 +510,8 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
>> gic_sgi_mode irqmode,
>>          break;
>>      default:
>>          gprintk(XENLOG_WARNING,
>> -                "vGICD:unhandled GICD_SGIR write %"PRIregister" \
>> -                 with wrong mode\n", sgir);
>> +                "vGICD: GICD_SGIR write %#"PRIregister" with unhangled mode 
>> %d\n",
> s/unhangled/unhandled/
>
>> +                sgir, irqmode);
>>          return false;
>>      }
>>
>> --
>> 2.39.2
>>
> Otherwise:
> Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

Bah, I really was asleep when writing this.  I spotted 2 of the typos,
but not all of them.

I'll fix them all up.  Thanks.

~Andrew