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

[Michal Orzel <michal.orzel@xxxxxxx>]

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"

> +                        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>

~Michal