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.coop...@citrix.com>
>>> ---
>>> CC: Stefano Stabellini <sstabell...@kernel.org>
>>> CC: Julien Grall <jul...@xen.org>
>>> CC: Volodymyr Babchuk <volodymyr_babc...@epam.com>
>>> CC: Bertrand Marquis <bertrand.marq...@arm.com>
>>> CC: Michal Orzel <michal.or...@amd.com>
>>> CC: Jan Beulich <jbeul...@suse.com>
>>>
>>> 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
