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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

I'm disappointed about how poor the code generation is when assigning to named
fields, but I suppose it is a harder problem for the compiler to figure out.

add/remove: 0/0 grow/shrink: 0/2 up/down: 0/-24 (-24)
Function                                     old     new   delta
machine_kexec                                356     348      -8
traps_init                                   434     418     -16
---
 xen/arch/x86/include/asm/idt.h | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/idt.h b/xen/arch/x86/include/asm/idt.h
index f613d5693e0e..b5e570a77fae 100644
--- 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;
+    idte.a |= addr & 0xffff;
 
     _write_gate_lower(gate, &idte);
 }
-- 
2.39.5

On 12.05.2025 13:58, Andrew Cooper wrote:
> --- 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)

Considering comment and name of the function, ...

>  {
> +    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;

... doesn't this line want dropping altogether? Or at best be an assertion?

Jan

> +    idte.a = gate->a & 0x0000ffffffff0000UL;
> +    idte.a |= (unsigned long)addr1 << 32;
> +    idte.a |= addr & 0xffff;
>  
>      _write_gate_lower(gate, &idte);
>  }

On 12/05/2025 2:21 pm, Jan Beulich wrote:
> On 12.05.2025 13:58, Andrew Cooper wrote:
>> --- 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)
> Considering comment and name of the function, ...
>
>>  {
>> +    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;
> ... doesn't this line want dropping altogether? Or at best be an assertion?

That's what _write_gate_lower() does, hence why
_update_gate_addr_lower() needs to calculate .b.

~Andrew

On 12.05.2025 15:48, Andrew Cooper wrote:
> On 12/05/2025 2:21 pm, Jan Beulich wrote:
>> On 12.05.2025 13:58, Andrew Cooper wrote:
>>> --- 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)
>> Considering comment and name of the function, ...
>>
>>>  {
>>> +    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;
>> ... doesn't this line want dropping altogether? Or at best be an assertion?
> 
> That's what _write_gate_lower() does, hence why
> _update_gate_addr_lower() needs to calculate .b.

To satisfy that, idte.b = gate.b would be all we need (i.e. just like
_set_gate_lower() does). Or else I think the "lower" would need dropping from
comment and name here.

Jan