Re: [PATCH v2 09/13] xen/bitops: Replace find_first_set_bit() with ffsl() - 1

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.05.2024 22:03, Andrew Cooper wrote:
> --- a/xen/arch/x86/hvm/hpet.c
> +++ b/xen/arch/x86/hvm/hpet.c
> @@ -335,7 +335,7 @@ static void timer_sanitize_int_route(HPETState *h, 
> unsigned int tn)
>       * enabled pick the first irq.
>       */
>      timer_config(h, tn) |=
> -        MASK_INSR(find_first_set_bit(timer_int_route_cap(h, tn)),
> +        MASK_INSR(ffsl(timer_int_route_cap(h, tn)) - 1,
>                    HPET_TN_ROUTE);
>  }

This can be just ffs().

> @@ -409,7 +409,7 @@ static int cf_check hpet_write(
>          {
>              bool active;
>  
> -            i = find_first_set_bit(new_val);
> +            i = ffsl(new_val) - 1;
>              if ( i >= HPET_TIMER_NUM )
>                  break;

This in principle can be, too, but would require a little further care.

> @@ -535,14 +535,14 @@ static int cf_check hpet_write(
>      /* stop/start timers whos state was changed by this write. */
>      while (stop_timers)
>      {
> -        i = find_first_set_bit(stop_timers);
> +        i = ffsl(stop_timers) - 1;
>          __clear_bit(i, &stop_timers);
>          hpet_stop_timer(h, i, guest_time);
>      }
>  
>      while (start_timers)
>      {
> -        i = find_first_set_bit(start_timers);
> +        i = ffsl(start_timers) - 1;
>          __clear_bit(i, &start_timers);
>          hpet_set_timer(h, i, guest_time);
>      }

Same here; in fact {start,stop}_timers are needlessly unsigned long in
the first place.

> --- a/xen/arch/x86/include/asm/pt-contig-markers.h
> +++ b/xen/arch/x86/include/asm/pt-contig-markers.h
> @@ -60,7 +60,7 @@ static bool pt_update_contig_markers(uint64_t *pt, unsigned 
> int idx,
>      /* Step 1: Reduce markers in lower numbered entries. */
>      while ( i )
>      {
> -        b = find_first_set_bit(i);
> +        b = ffsl(i) - 1;
>          i &= ~(1U << b);

Considering i's type and the immediately following expression, this again
can easily be just ffs().

> --- a/xen/drivers/passthrough/amd/iommu_map.c
> +++ b/xen/drivers/passthrough/amd/iommu_map.c
> @@ -137,7 +137,7 @@ static void set_iommu_ptes_present(unsigned long pt_mfn,
>          ASSERT(!pde->u);
>  
>          if ( pde > table )
> -            ASSERT(pde->ign0 == find_first_set_bit(pde - table));
> +            ASSERT(pde->ign0 == ffsl(pde - table) - 1);

pde pointing into the page starting at table, this can be ffs(), too.

Preferably with at least the easy adjustments done:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan