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"
> + 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.or...@amd.com>
~Michal
