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.coop...@citrix.com>
Acked-by: Jan Beulich <jbeul...@suse.com>
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
