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

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.
---
 xen/arch/x86/include/asm/apic.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/include/asm/apic.h b/xen/arch/x86/include/asm/apic.h
index a254e49dd13b..1133954e5540 100644
--- 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)
 {
 	*((volatile u32 *)(APIC_BASE+reg)) = v;
 }
 
-static inline u32 apic_mem_read(unsigned long reg)
+static inline u32 apic_mem_read(unsigned int reg)
 {
 	return *((volatile u32 *)(APIC_BASE+reg));
 }
@@ -63,7 +63,7 @@ static inline u32 apic_mem_read(unsigned long reg)
  * access the 64-bit ICR register.
  */
 
-static inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
+static inline void apic_wrmsr(unsigned int reg, uint64_t msr_content)
 {
     if (reg == APIC_DFR || reg == APIC_ID || reg == APIC_LDR ||
         reg == APIC_LVR)
@@ -72,7 +72,7 @@ static inline void apic_wrmsr(unsigned long reg, uint64_t msr_content)
     wrmsrl(MSR_X2APIC_FIRST + (reg >> 4), msr_content);
 }
 
-static inline uint64_t apic_rdmsr(unsigned long reg)
+static inline uint64_t apic_rdmsr(unsigned int reg)
 {
     uint64_t msr_content;
 
@@ -83,7 +83,7 @@ static inline uint64_t apic_rdmsr(unsigned long reg)
     return msr_content;
 }
 
-static inline void apic_write(unsigned long reg, u32 v)
+static inline void apic_write(unsigned int reg, u32 v)
 {
 
     if ( x2apic_enabled )
@@ -92,7 +92,7 @@ static inline void apic_write(unsigned long reg, u32 v)
         apic_mem_write(reg, v);
 }
 
-static inline u32 apic_read(unsigned long reg)
+static inline u32 apic_read(unsigned int reg)
 {
     if ( x2apic_enabled )
         return apic_rdmsr(reg);
-- 
2.39.2

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@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> 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