Re: [PATCH] x86/APIC: Change APIC reg types to unsigned int

[Jan Beulich <jbeulich@xxxxxxxx>]

On 23.07.2024 23:05, Andrew Cooper wrote:
> They're all within a 12 bit range of their respective bases, and this prevents
> all the MSR coordinates being calculated in %rcx.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> There's one unpleasant surprise hidden:
> 
>   add/remove: 0/0 grow/shrink: 2/6 up/down: 18/-99 (-81)
>   Function                                     old     new   delta
>   trace_exit_reason.part                       229     245     +16
>   check_for_unexpected_msi                      73      75      +2
>   fixup_irqs                                  1239    1237      -2
>   lapic_resume                                 860     844     -16
>   irq_move_cleanup_interrupt                   683     667     -16
>   intel_mcheck_init                           1840    1824     -16
>   setup_local_APIC                             985     968     -17
>   clear_local_APIC                            1141    1109     -32
> 
> This causes check_for_unexpected_msi() to gain a CLTQ sign extending reg
> before adding it to APIC_BASE.  Furthermore it retains it's SAR from the start
> of apic_isr_read().
> 
> If the vector parameter changes from uint8_t to unsigned int, both the CLTQ
> and SAR go away and and are replaced with regular unsigned logic.
> 
> (uint8_t) & ~0x1f undergoes promotion to int, but can't be negative due to
> it's range, and I'm quite surprised that GCC-12 can't figure this out.

Odd indeed. But certainly no the only odd thing in code generation.

> --- a/xen/arch/x86/include/asm/apic.h
> +++ b/xen/arch/x86/include/asm/apic.h
> @@ -49,12 +49,12 @@ const struct genapic *apic_x2apic_probe(void);
>   * Basic functions accessing APICs.
>   */
>  
> -static inline void apic_mem_write(unsigned long reg, u32 v)
> +static inline void apic_mem_write(unsigned int reg, u32 v)

Mind me asking that on lines you touch anyway you also change u32 to
uint32_t?

Jan