The encoded MUL is 64 bits, so writes %rdx too.  At a minimum, this needs
expressing as a clobber.

Also fix a logical disconnect between 'overflow' being the carry flag not the
overflow flag.  CF and OF are always the same for MUL instructions, so use the
flag which matches the variable name.

Fixes: d5c70a51bfbe ("x86/APIC: handle overflow in TMICT calculation")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <jbeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Teddy Astie <teddy.astie@vates.tech>

Only affects 4.20 (and earlier) where __builtin_umull_overflow() can't be
used.

I've kept this form because it produces best code generation for GCCs which
support flag outputs.

An alternative would be to capture product_hi and check the nonzero-ness, as
that's how OF/CF are produced in hardware, which would be better code
generation on very old GCCs.
---
 xen/arch/x86/apic.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index daf597ed44b7..764ee1e98f77 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1317,9 +1317,10 @@ int reprogram_timer(s_time_t timeout)
 
         apic_tmict = UINT32_MAX;
         asm ( "mul %[expire]\n\t"
-              ASM_FLAG_OUT(, "setc %[cf]")
-              : "=a" (product), [cf] ASM_FLAG_OUT("=@ccc", "=qm") (overflow)
-              : "0" ((unsigned long)bus_scale), [expire] "r" (expire) );
+              ASM_FLAG_OUT(, "seto %[of]")
+              : "=a" (product), [of] ASM_FLAG_OUT("=@cco", "=qm") (overflow)
+              : "0" ((unsigned long)bus_scale), [expire] "r" (expire)
+              : "rdx" );
         if ( !overflow &&
              (product >>= BUS_SCALE_SHIFT) < apic_tmict )
             apic_tmict = product;

base-commit: 5f7054258c6937b74aee411f16db5eb54ce9fda1
-- 
2.39.5

On 20.04.2026 19:46, Andrew Cooper wrote:
> The encoded MUL is 64 bits, so writes %rdx too.  At a minimum, this needs
> expressing as a clobber.

I'm embarrassed of missing this.

> Also fix a logical disconnect between 'overflow' being the carry flag not the
> overflow flag.  CF and OF are always the same for MUL instructions, so use the
> flag which matches the variable name.

I don't mind this too much, but the use of CF was deliberate: Imo OF is
relevant to signed arithmetic only, whereas CF is the flag to use with
unsigned operations.

> Fixes: d5c70a51bfbe ("x86/APIC: handle overflow in TMICT calculation")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> I've kept this form because it produces best code generation for GCCs which
> support flag outputs.
> 
> An alternative would be to capture product_hi and check the nonzero-ness, as
> that's how OF/CF are produced in hardware, which would be better code
> generation on very old GCCs.

We could fit both, by further widening the use of ASM_FLAG_OUT().

> --- a/xen/arch/x86/apic.c
> +++ b/xen/arch/x86/apic.c
> @@ -1317,9 +1317,10 @@ int reprogram_timer(s_time_t timeout)
>  
>          apic_tmict = UINT32_MAX;
>          asm ( "mul %[expire]\n\t"
> -              ASM_FLAG_OUT(, "setc %[cf]")
> -              : "=a" (product), [cf] ASM_FLAG_OUT("=@ccc", "=qm") (overflow)
> -              : "0" ((unsigned long)bus_scale), [expire] "r" (expire) );
> +              ASM_FLAG_OUT(, "seto %[of]")
> +              : "=a" (product), [of] ASM_FLAG_OUT("=@cco", "=qm") (overflow)

Noticing only now - for the non-flag-output case this should be "=&a". With

> +              : "0" ((unsigned long)bus_scale), [expire] "r" (expire)

... %rax also being an input, there's no risk of the compiler using the
register for the other input, but still. Would you mind making that adjustment
as well, while at it?

Jan

On 21/04/2026 7:39 am, Jan Beulich wrote:
> On 20.04.2026 19:46, Andrew Cooper wrote:
>> The encoded MUL is 64 bits, so writes %rdx too.  At a minimum, this needs
>> expressing as a clobber.
> I'm embarrassed of missing this.
>
>> Also fix a logical disconnect between 'overflow' being the carry flag not the
>> overflow flag.  CF and OF are always the same for MUL instructions, so use the
>> flag which matches the variable name.
> I don't mind this too much, but the use of CF was deliberate: Imo OF is
> relevant to signed arithmetic only, whereas CF is the flag to use with
> unsigned operations.
>
>> Fixes: d5c70a51bfbe ("x86/APIC: handle overflow in TMICT calculation")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.  I could rename the variable to carry instead then?  Either works.

>
>> I've kept this form because it produces best code generation for GCCs which
>> support flag outputs.
>>
>> An alternative would be to capture product_hi and check the nonzero-ness, as
>> that's how OF/CF are produced in hardware, which would be better code
>> generation on very old GCCs.
> We could fit both, by further widening the use of ASM_FLAG_OUT().
>
>> --- a/xen/arch/x86/apic.c
>> +++ b/xen/arch/x86/apic.c
>> @@ -1317,9 +1317,10 @@ int reprogram_timer(s_time_t timeout)
>>  
>>          apic_tmict = UINT32_MAX;
>>          asm ( "mul %[expire]\n\t"
>> -              ASM_FLAG_OUT(, "setc %[cf]")
>> -              : "=a" (product), [cf] ASM_FLAG_OUT("=@ccc", "=qm") (overflow)
>> -              : "0" ((unsigned long)bus_scale), [expire] "r" (expire) );
>> +              ASM_FLAG_OUT(, "seto %[of]")
>> +              : "=a" (product), [of] ASM_FLAG_OUT("=@cco", "=qm") (overflow)
> Noticing only now - for the non-flag-output case this should be "=&a". With
>
>> +              : "0" ((unsigned long)bus_scale), [expire] "r" (expire)
> ... %rax also being an input, there's no risk of the compiler using the
> register for the other input, but still. Would you mind making that adjustment
> as well, while at it?

Ok.

~Andrew

On 21.04.2026 12:22, Andrew Cooper wrote:
> On 21/04/2026 7:39 am, Jan Beulich wrote:
>> On 20.04.2026 19:46, Andrew Cooper wrote:
>>> The encoded MUL is 64 bits, so writes %rdx too.  At a minimum, this needs
>>> expressing as a clobber.
>> I'm embarrassed of missing this.
>>
>>> Also fix a logical disconnect between 'overflow' being the carry flag not the
>>> overflow flag.  CF and OF are always the same for MUL instructions, so use the
>>> flag which matches the variable name.
>> I don't mind this too much, but the use of CF was deliberate: Imo OF is
>> relevant to signed arithmetic only, whereas CF is the flag to use with
>> unsigned operations.
>>
>>> Fixes: d5c70a51bfbe ("x86/APIC: handle overflow in TMICT calculation")
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Thanks.  I could rename the variable to carry instead then?  Either works.

I'd slightly prefer that alternative, yes.

Thanks, Jan