Re: [PATCH 4/4] x86/hvmloader: Move various helpers to being static inlines

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.08.2022 12:59, Andrew Cooper wrote:
> The IO port, MSR, IO-APIC and LAPIC accessors compile typically to single or
> pairs of instructions, which is less overhead than even the stack manipulation
> to call the helpers.
> 
> Move the implementations from util.c to being static inlines in util.h
> 
> In addition, turn ioapic_base_address into a constant as it is never modified
> from 0xfec00000 (substantially shrinks the IO-APIC logic), and make use of the
> "A" constraint for WRMSR/RDMSR like we already do for RDSTC.

Nit: RDTSC

> Bloat-o-meter reports a net:
>   add/remove: 0/13 grow/shrink: 1/19 up/down: 6/-743 (-737)
> 
> No functional change.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with several further nits/suggestions:

> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -7,6 +7,7 @@
>  #include <stdbool.h>
>  #include <xen/xen.h>
>  #include <xen/hvm/hvm_info_table.h>
> +#include "config.h"
>  #include "e820.h"
>  
>  /* Request un-prefixed values from errno.h. */
> @@ -61,28 +62,91 @@ static inline int test_and_clear_bit(int nr, volatile 
> void *addr)
>  }
>  
>  /* MSR access */
> -void wrmsr(uint32_t idx, uint64_t v);
> -uint64_t rdmsr(uint32_t idx);
> +static inline void wrmsr(uint32_t idx, uint64_t v)
> +{
> +    asm volatile ( "wrmsr" :: "c" (idx), "A" (v) : "memory" );

The addition of the "memory" clobber imo wants mentioning in the
description, so it's clear that it's intentional (and why).

> +}
> +
> +static inline uint64_t rdmsr(uint32_t idx)
> +{
> +    uint64_t res;
> +
> +    asm volatile ( "rdmsr" : "=A" (res) : "c" (idx) );
> +
> +    return res;
> +}
>  
>  /* I/O output */
> -void outb(uint16_t addr, uint8_t  val);
> -void outw(uint16_t addr, uint16_t val);
> -void outl(uint16_t addr, uint32_t val);
> +static inline void outb(uint16_t addr, uint8_t val)
> +{
> +    asm volatile ( "outb %%al, %%dx" :: "d" (addr), "a" (val) );

I'm inclined to ask to use "outb %1, %0" here (and similarly below).
I also wonder whether at least all the OUTs shouldn't also gain a
"memory" clobber.

> +}
> +
> +static inline void outw(uint16_t addr, uint16_t val)
> +{
> +    asm volatile ( "outw %%ax, %%dx" :: "d" (addr), "a" (val) );
> +}
> +
> +static inline void outl(uint16_t addr, uint32_t val)
> +{
> +    asm volatile ( "outl %%eax, %%dx" :: "d" (addr), "a" (val) );
> +}
>  
>  /* I/O input */
> -uint8_t  inb(uint16_t addr);
> -uint16_t inw(uint16_t addr);
> -uint32_t inl(uint16_t addr);
> +static inline uint8_t inb(uint16_t addr)
> +{
> +    uint8_t val;
> +
> +    asm volatile ( "inb %%dx,%%al" : "=a" (val) : "d" (addr) );

Would you mind adding blanks after the comma here and below?

> +
> +    return val;
> +}
> +
> +static inline uint16_t inw(uint16_t addr)
> +{
> +    uint16_t val;
> +
> +    asm volatile ( "inw %%dx,%%ax" : "=a" (val) : "d" (addr) );
> +
> +    return val;
> +}
> +
> +static inline uint32_t inl(uint16_t addr)
> +{
> +    uint32_t val;
> +
> +    asm volatile ( "inl %%dx,%%eax" : "=a" (val) : "d" (addr) );
> +
> +    return val;
> +}
>  
>  /* CMOS access */
>  uint8_t cmos_inb(uint8_t idx);
>  void cmos_outb(uint8_t idx, uint8_t val);
>  
>  /* APIC access */
> -uint32_t ioapic_read(uint32_t reg);
> -void ioapic_write(uint32_t reg, uint32_t val);
> -uint32_t lapic_read(uint32_t reg);
> -void lapic_write(uint32_t reg, uint32_t val);
> +static inline uint32_t ioapic_read(uint32_t reg)
> +{
> +    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
> +    return *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10);
> +}
> +
> +static inline void ioapic_write(uint32_t reg, uint32_t val)
> +{
> +    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
> +    *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10) = val;
> +}
> +
> +#define LAPIC_BASE_ADDRESS  0xfee00000

Seeing this #define here, does there anything stand in the way of
putting IOAPIC_BASE_ADDRESS next to the inline functions as well?

Jan

> +static inline uint32_t lapic_read(uint32_t reg)
> +{
> +    return *(volatile uint32_t *)(LAPIC_BASE_ADDRESS + reg);
> +}
> +
> +static inline void lapic_write(uint32_t reg, uint32_t val)
> +{
> +    *(volatile uint32_t *)(LAPIC_BASE_ADDRESS + reg) = val;
> +}
>  
>  /* PCI access */
>  uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);