Re: [PATCH] x86/APIC: handle overflow in TMICT calculation

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/04/2026 10:21 am, Jan Beulich wrote:
> With an expiry value on the order of 20 hours, and with a bus scale value
> of 256k (as supplied by qemu), the (signed) multiplication will be UB. As
> we've checked that the value is positive, we mean unsigned multiplication
> anyway. Yet let's play safe against even larger expiry and bus scale
> values, leveraging the compiler builtin that there is for this purpose.
>
> While there also drop the stray cast from the actual TMICT write.
>
> Fixes: 9062553a0dc1 ("added time and accurate timer support")
> Fixes: b95beb185810 ("x86: Clean up APIC local timer handling")
> Reported-by: Stewart Hildebrand <stewart.hildebrand@xxxxxxx>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

> ---
> Sadly neither gcc5 nor gcc15 properly optimize the (effectively) two uses
> of the 0xffffffffU constant: Both use a 2nd register to load the constant
> (really 0xfffffffeU unless <= is used) a 2nd time.
>
> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1224,10 +1224,16 @@ int reprogram_timer(s_time_t timeout)
>      }
>  
>      if ( timeout && ((expire = timeout - NOW()) > 0) )
> -        apic_tmict = min_t(uint64_t, (bus_scale * expire) >> BUS_SCALE_SHIFT,
> -                           UINT32_MAX);
> +    {
> +        unsigned long product;
>  
> -    apic_write(APIC_TMICT, (unsigned long)apic_tmict);
> +        apic_tmict = UINT32_MAX;
> +        if ( !__builtin_umull_overflow(bus_scale, expire, &product) &&
> +             (product >>= BUS_SCALE_SHIFT) < apic_tmict )
> +            apic_tmict = product;
> +    }
> +
> +    apic_write(APIC_TMICT, apic_tmict);
>  
>      return apic_tmict || !timeout;
>  }

This is fine for staging, but be aware it cannot be backported before
4.21 due to the toolchain baseline (and nothing in CI will notice, I
don't think.)

~Andrew