On 12/08/2025 9:16 am, Jan Beulich wrote:
> 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>
Thanks.
> 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?
Correct. Forcing the mask operation to be 32bit is the only way I found
of avoiding the MOVABS.
>
> 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.
Ok.
~Andrew
