Re: [PATCH 04/22] x86/idt: Minor improvements to _update_gate_addr_lower()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.08.2025 22:22, Andrew Cooper wrote:
> After some experimentation, using .a/.b makes far better logic than using the
> named fields, as both Clang and GCC spill idte to the stack when named fields
> are used.
> 
> GCC seems to do something very daft for the addr1 field.  It takes addr,
> shifts it by 32, then ANDs with 0xffff0000000000000UL, which requires
> manifesting a MOVABS.
> 
> Clang follows the C, whereby it ANDs with $imm32, then shifts, avoiding the
> MOVABS entirely.
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit I have to admit that I'm not quite happy about ...

> --- a/xen/arch/x86/include/asm/idt.h
> +++ b/xen/arch/x86/include/asm/idt.h
> @@ -92,15 +92,16 @@ static inline void _set_gate_lower(idt_entry_t *gate, 
> unsigned long type,
>   * Update the lower half handler of an IDT entry, without changing any other
>   * configuration.
>   */
> -static inline void _update_gate_addr_lower(idt_entry_t *gate, void *addr)
> +static inline void _update_gate_addr_lower(idt_entry_t *gate, void *_addr)
>  {
> +    unsigned long addr = (unsigned long)_addr;
> +    unsigned int addr1 = addr & 0xffff0000U; /* GCC force better codegen. */
>      idt_entry_t idte;
> -    idte.a = gate->a;
>  
> -    idte.b = ((unsigned long)(addr) >> 32);
> -    idte.a &= 0x0000FFFFFFFF0000ULL;
> -    idte.a |= (((unsigned long)(addr) & 0xFFFF0000UL) << 32) |
> -        ((unsigned long)(addr) & 0xFFFFUL);
> +    idte.b = addr >> 32;
> +    idte.a = gate->a & 0x0000ffffffff0000UL;
> +    idte.a |= (unsigned long)addr1 << 32;

... the cast here. Yet perhaps gcc still generates a MOVABS when you make
addr1 unsigned long?

As to the comment next to the variable declaration: Could I talk you into
adding a colon after "GCC"? Without one, the comment reads somewhat
ambiguously to me.

Jan