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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/08/2024 14:20, Andrew Cooper wrote:
> 
> 
> 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.
I don't think we can target a different domain with SGIs, so it does not make 
much sense to print domain id if
it always stays the same as the leading %pv from gprintk.

~Michal