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.coop...@citrix.com>
Reviewed-by: Jan Beulich <jbeul...@suse.com>
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);
