From e4a7d9f69347314e7b3110a5f68921da768cb4e8 Mon Sep 17 00:00:00 2001 From: Felix Singer Date: Mon, 15 Jun 2020 15:00:56 +0200 Subject: [PATCH] device: Add method to configure bus mastering based on Kconfig The bus master bit is set at many places in coreboot's code, but the reason for that is not quite clear. We examined not setting the bus master bit whereever possible and tried booting without it, which worked fine for internal PCI devices but not for PCIe. As a PCIe device we used a Samsung M.2 NVMe SSD. For security reasons, we would like to disable bus mastering where possible. Depending on the device, bus mastering might get enabled by the operating system (e.g. for iGPU) and it might be required for some devices to work properly. However, the idea is to leave it disabled and configure the IOMMU first before enabling it. To have some sort of "backwards compatibility", add a method which configures bus mastering based on an additional config option. Since CB:42460 makes usage of this treewide, enable it by default to keep the current behaviour for now. Tested with Siemens/Chili, a Coffee Lake based platform. Change-Id: I876c48ea3fb4f9cf7b6a5c2dcaeda07ea36cbed3 Signed-off-by: Felix Singer Reviewed-on: https://review.coreboot.org/c/coreboot/+/42459 Reviewed-by: Nico Huber Reviewed-by: Subrata Banik Tested-by: build bot (Jenkins) --- src/device/Kconfig | 13 +++++++++++++ src/include/device/pci.h | 12 ++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/device/Kconfig b/src/device/Kconfig index d0d72f9a77..439118f10c 100644 --- a/src/device/Kconfig +++ b/src/device/Kconfig @@ -519,6 +519,19 @@ config PCIEXP_PLUGIN_SUPPORT bool default y +config PCI_ALLOW_BUS_MASTER + bool "Allow PCI bus master bit to be enabled by coreboot" + default y + help + For security reasons, bus mastering should be enabled as late as + possible. In coreboot, it's usually not necessary and payloads + should only enable it for devices they use. Since not all payloads + enable bus mastering properly yet, this option gives some sort of + "backwards compatibility" and is enabled by default to keep the + traditional behaviour for now. This is currently necessary, for + instance, for libpayload based payloads as the drivers don't enable + bus mastering for PCI bridges. + endif # PCI if PCIEXP_PLUGIN_SUPPORT diff --git a/src/include/device/pci.h b/src/include/device/pci.h index 4529074e9b..ec3d45e58b 100644 --- a/src/include/device/pci.h +++ b/src/include/device/pci.h @@ -128,6 +128,18 @@ static inline int pci_base_address_is_memory_space(unsigned int attr) } void pci_dev_disable_bus_master(const struct device *dev); + +static __always_inline +#if ENV_PCI_SIMPLE_DEVICE +void pci_dev_request_bus_master(pci_devfn_t dev) +#else +void pci_dev_request_bus_master(const struct device *dev) +#endif /* ENV_PCI_SIMPLE_DEVICE */ +{ + if (CONFIG(PCI_ALLOW_BUS_MASTER)) + pci_or_config16(dev, PCI_COMMAND, PCI_COMMAND_MASTER); +} + #endif /* CONFIG_PCI */ void pci_early_bridge_init(void);