From d2cdfff63b6e2376fad729252a57acfd2b4418ea Mon Sep 17 00:00:00 2001 From: Kyösti Mälkki Date: Tue, 5 Mar 2019 07:56:38 +0200 Subject: device/pci: Rewrite PCI MMCONF with symbol reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The effect of pointer aliasing on writes is that any data on CPU registers that has been resolved from (non-const and non-volatile) memory objects has to be discarded and resolved. In other words, the compiler assumes that a pointer that does not have an absolute value at build-time, and is of type 'void *' or 'char *', may write over any memory object. Using a unique datatype for MMIO writes makes the pointer to _not_ qualify for pointer aliasing with any other objects in memory. This avoid constantly resolving the PCI MMCONF address, which is a derived value from a 'struct device *'. Change-Id: Id112aa5e729ffd8015bb806786bdee38783b7ea9 Signed-off-by: Kyösti Mälkki Reviewed-on: https://review.coreboot.org/c/coreboot/+/31752 Reviewed-by: Aaron Durbin Tested-by: build bot (Jenkins) --- src/arch/x86/include/arch/pci_mmio_cfg_romcc.h | 72 ++++++++++++++++++++++++++ src/arch/x86/include/arch/pci_ops.h | 6 +++ src/device/Makefile.inc | 8 ++- src/device/pci_ops.c | 4 ++ src/include/device/pci_mmio_cfg.h | 55 +++++++++++++------- src/include/device/pci_type.h | 4 ++ 6 files changed, 130 insertions(+), 19 deletions(-) create mode 100644 src/arch/x86/include/arch/pci_mmio_cfg_romcc.h diff --git a/src/arch/x86/include/arch/pci_mmio_cfg_romcc.h b/src/arch/x86/include/arch/pci_mmio_cfg_romcc.h new file mode 100644 index 0000000000..00e8e41088 --- /dev/null +++ b/src/arch/x86/include/arch/pci_mmio_cfg_romcc.h @@ -0,0 +1,72 @@ +/* + * This file is part of the coreboot project. + * + * Copyright (C) 2007-2009 coresystems GmbH + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef _PCI_MMIO_CFG_ROMCC_H +#define _PCI_MMIO_CFG_ROMCC_H + +#include +#include +#include + + +static __always_inline +uint8_t pci_mmio_read_config8(pci_devfn_t dev, uint16_t reg) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | reg); + return read8(addr); +} + +static __always_inline +uint16_t pci_mmio_read_config16(pci_devfn_t dev, uint16_t reg) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~1)); + return read16(addr); +} + +static __always_inline +uint32_t pci_mmio_read_config32(pci_devfn_t dev, uint16_t reg) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~3)); + return read32(addr); +} + +static __always_inline +void pci_mmio_write_config8(pci_devfn_t dev, uint16_t reg, uint8_t value) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | reg); + write8(addr, value); +} + +static __always_inline +void pci_mmio_write_config16(pci_devfn_t dev, uint16_t reg, uint16_t value) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~1)); + write16(addr, value); +} + +static __always_inline +void pci_mmio_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) +{ + void *addr; + addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~3)); + write32(addr, value); +} + +#endif /* _PCI_MMIO_CFG_ROMCC_H */ diff --git a/src/arch/x86/include/arch/pci_ops.h b/src/arch/x86/include/arch/pci_ops.h index e706216586..4278ed0dfd 100644 --- a/src/arch/x86/include/arch/pci_ops.h +++ b/src/arch/x86/include/arch/pci_ops.h @@ -15,6 +15,12 @@ #define ARCH_I386_PCI_OPS_H #include + +#if defined(__ROMCC__) +/* Must come before */ +#include +#endif + #include #endif /* ARCH_I386_PCI_OPS_H */ diff --git a/src/device/Makefile.inc b/src/device/Makefile.inc index 711a403353..baa45bec3e 100644 --- a/src/device/Makefile.inc +++ b/src/device/Makefile.inc @@ -26,9 +26,15 @@ postcar-y += pci_early.c ramstage-y += pci_class.c ramstage-y += pci_device.c -ramstage-y += pci_ops.c ramstage-y += pci_rom.c +bootblock-y += pci_ops.c +verstage-y += pci_ops.c +romstage-y += pci_ops.c +postcar-y += pci_ops.c +ramstage-y += pci_ops.c +smm-y += pci_ops.c + ramstage-$(CONFIG_HYPERTRANSPORT_PLUGIN_SUPPORT) += hypertransport.c ramstage-$(CONFIG_PCIX_PLUGIN_SUPPORT) += pcix_device.c ramstage-$(CONFIG_PCIEXP_PLUGIN_SUPPORT) += pciexp_device.c diff --git a/src/device/pci_ops.c b/src/device/pci_ops.c index bdf8ec4584..34f9d1e5b5 100644 --- a/src/device/pci_ops.c +++ b/src/device/pci_ops.c @@ -10,3 +10,7 @@ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the * GNU General Public License for more details. */ + +#include + +u8 *const pci_mmconf = (void *)(uintptr_t)CONFIG_MMCONF_BASE_ADDRESS; diff --git a/src/include/device/pci_mmio_cfg.h b/src/include/device/pci_mmio_cfg.h index e7019151aa..5567ed86ae 100644 --- a/src/include/device/pci_mmio_cfg.h +++ b/src/include/device/pci_mmio_cfg.h @@ -20,55 +20,74 @@ #include #include +#if !defined(__ROMCC__) + +/* By not assigning this to CONFIG_MMCONF_BASE_ADDRESS here we + * prevent some sub-optimal constant folding. */ +extern u8 *const pci_mmconf; + +/* Using a unique datatype for MMIO writes makes the pointers to _not_ + * qualify for pointer aliasing with any other objects in memory. + * + * MMIO offset is a value originally derived from 'struct device *' + * in ramstage. For the compiler to not discard this MMIO offset value + * from CPU registers after any MMIO writes, -fstrict-aliasing has to + * be also set for the build. + * + * Bottom 12 bits (4 KiB) are reserved to address the registers of a + * single PCI function. Declare the bank as a union to avoid some casting + * in the functions below. + */ +union pci_bank { + uint8_t reg8[4096]; + uint16_t reg16[4096 / sizeof(uint16_t)]; + uint32_t reg32[4096 / sizeof(uint32_t)]; +}; + +static __always_inline +volatile union pci_bank *pcicfg(pci_devfn_t dev) +{ + return (void *)&pci_mmconf[PCI_DEVFN_OFFSET(dev)]; +} static __always_inline uint8_t pci_mmio_read_config8(pci_devfn_t dev, uint16_t reg) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | reg); - return read8(addr); + return pcicfg(dev)->reg8[reg]; } static __always_inline uint16_t pci_mmio_read_config16(pci_devfn_t dev, uint16_t reg) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~1)); - return read16(addr); + return pcicfg(dev)->reg16[reg / sizeof(uint16_t)]; } static __always_inline uint32_t pci_mmio_read_config32(pci_devfn_t dev, uint16_t reg) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~3)); - return read32(addr); + return pcicfg(dev)->reg32[reg / sizeof(uint32_t)]; } static __always_inline void pci_mmio_write_config8(pci_devfn_t dev, uint16_t reg, uint8_t value) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | reg); - write8(addr, value); + pcicfg(dev)->reg8[reg] = value; } static __always_inline void pci_mmio_write_config16(pci_devfn_t dev, uint16_t reg, uint16_t value) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~1)); - write16(addr, value); + pcicfg(dev)->reg16[reg / sizeof(uint16_t)] = value; } static __always_inline void pci_mmio_write_config32(pci_devfn_t dev, uint16_t reg, uint32_t value) { - void *addr; - addr = (void *)(uintptr_t)(CONFIG_MMCONF_BASE_ADDRESS | dev | (reg & ~3)); - write32(addr, value); + pcicfg(dev)->reg32[reg / sizeof(uint32_t)] = value; } +#endif /* !defined(__ROMCC__) */ + #if CONFIG(MMCONF_SUPPORT) /* Avoid name collisions as different stages have different signature diff --git a/src/include/device/pci_type.h b/src/include/device/pci_type.h index 27d35589cc..4d8c2a3d08 100644 --- a/src/include/device/pci_type.h +++ b/src/include/device/pci_type.h @@ -18,6 +18,10 @@ typedef u32 pci_devfn_t; +/* Convert pci_devfn_t to offset in MMCONF space. + * As it is one-to-one, nothing needs to be done. */ +#define PCI_DEVFN_OFFSET(x) ((x)) + #define PCI_DEV(SEGBUS, DEV, FN) ( \ (((SEGBUS) & 0xFFF) << 20) | \ (((DEV) & 0x1F) << 15) | \ -- cgit v1.2.3