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

["Orzel, Michal" <michal.orzel@xxxxxxx>]

On 14/03/2025 22:21, Andrew Cooper wrote:
> 
> 
> The bitmap_for_each() expression only inspects the bottom 8 bits of targets.
> Change it's type to uint8_t and use for_each_set_bit() which is more efficient
> over scalars.
> 
> GICD_SGI_TARGET_LIST_MASK is 2 bits wide.  Two cases discard the prior
> calculation, and one case exits early.
NIT: I'd add ... calculation of targets. I had to read it couple times to
understand.

> 
> Therefore, move the GICD_SGI_TARGET_MASK calculation into the only case which
> wants it, and use MASK_EXTR() to simplify the expression.
> 
> No functional change.
> 
> 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>
> 
> v2:
>  * Split out of prior VGIC work as it's somewhat standalone.
>  * Leave the case labels as they were.
> ---
>  xen/arch/arm/vgic/vgic-mmio-v2.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c 
> b/xen/arch/arm/vgic/vgic-mmio-v2.c
> index 670b335db2c3..da62a8078b5f 100644
> --- a/xen/arch/arm/vgic/vgic-mmio-v2.c
> +++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
> @@ -88,13 +88,12 @@ static void vgic_mmio_write_sgir(struct vcpu *source_vcpu,
>      struct domain *d = source_vcpu->domain;
>      unsigned int nr_vcpus = d->max_vcpus;
>      unsigned int intid = val & GICD_SGI_INTID_MASK;
> -    unsigned long targets = (val & GICD_SGI_TARGET_MASK) >>
> -                            GICD_SGI_TARGET_SHIFT;
> -    unsigned int vcpu_id;
> +    uint8_t targets = 0;
> 
>      switch ( val & GICD_SGI_TARGET_LIST_MASK )
>      {
>      case GICD_SGI_TARGET_LIST:                    /* as specified by targets 
> */
> +        targets = MASK_EXTR(val, GICD_SGI_TARGET_MASK);
>          targets &= GENMASK(nr_vcpus - 1, 0);      /* limit to existing VCPUs 
> */
>          break;
>      case GICD_SGI_TARGET_OTHERS:
> @@ -104,11 +103,12 @@ static void vgic_mmio_write_sgir(struct vcpu 
> *source_vcpu,
>      case GICD_SGI_TARGET_SELF:                    /* this very vCPU only */
>          targets = (1U << source_vcpu->vcpu_id);
>          break;
> -    case 0x3:                                     /* reserved */
AFAICT 0x3 would not be even possible to trigger. It should have been 0x3<<24.

Reviewed-by: Michal Orzel <michal.orzel@xxxxxxx>

~Michal