From ebf7f8f599ed13bb949194ed6023a72e9fa5bd2c Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Wed, 2 Dec 2020 18:06:51 +0100 Subject: [PATCH] platform_drv: introduce structured PCI BDF type Replace explicit usage of bus, device, function arguments to methods or variables all over the code by a single data type. It eases the reading of and shorten the code. Issue #3963 --- repos/os/src/drivers/platform/spec/x86/irq.cc | 7 +- repos/os/src/drivers/platform/spec/x86/irq.h | 4 +- .../platform/spec/x86/pci_config_access.h | 66 ++++++++++------ .../platform/spec/x86/pci_device_component.h | 11 +-- .../platform/spec/x86/pci_device_config.h | 78 ++++++++----------- .../platform/spec/x86/pci_session_component.h | 78 ++++++++----------- .../src/drivers/platform/spec/x86/session.cc | 2 +- 7 files changed, 119 insertions(+), 127 deletions(-) diff --git a/repos/os/src/drivers/platform/spec/x86/irq.cc b/repos/os/src/drivers/platform/spec/x86/irq.cc index f3fff5f806..b82affee3e 100644 --- a/repos/os/src/drivers/platform/spec/x86/irq.cc +++ b/repos/os/src/drivers/platform/spec/x86/irq.cc @@ -286,13 +286,12 @@ void Platform::Irq_session_component::sigh(Genode::Signal_context_capability sig } -unsigned short Platform::Irq_routing::rewrite(unsigned char bus, unsigned char dev, - unsigned char, unsigned char pin) +unsigned short Platform::Irq_routing::rewrite(Pci::Bdf const bdf, unsigned char pin) { - unsigned const bridge_bdf_bus = Platform::bridge_bdf(bus); + unsigned const bridge_bdf_bus = Platform::bridge_bdf(bdf.bus()); for (Irq_routing *i = list()->first(); i; i = i->next()) { - if ((dev == i->_device) && (pin - 1 == i->_device_pin) && + if ((bdf.device() == i->_device) && (pin - 1 == i->_device_pin) && (i->_bridge_bdf == bridge_bdf_bus)) return i->_gsi; } diff --git a/repos/os/src/drivers/platform/spec/x86/irq.h b/repos/os/src/drivers/platform/spec/x86/irq.h index dc9ade3e96..7cb1add2c6 100644 --- a/repos/os/src/drivers/platform/spec/x86/irq.h +++ b/repos/os/src/drivers/platform/spec/x86/irq.h @@ -20,6 +20,7 @@ #include /* platform local includes */ +#include #include @@ -181,8 +182,7 @@ class Platform::Irq_routing : public Genode::List::Elemen _device_pin(device_pin) { } - static unsigned short rewrite(unsigned char bus, unsigned char dev, - unsigned char func, unsigned char pin); + static unsigned short rewrite(Pci::Bdf, unsigned char pin); }; #endif /* _X86__IRQ_H_ */ diff --git a/repos/os/src/drivers/platform/spec/x86/pci_config_access.h b/repos/os/src/drivers/platform/spec/x86/pci_config_access.h index ac282ee6b1..4bdbf04b6f 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_config_access.h +++ b/repos/os/src/drivers/platform/spec/x86/pci_config_access.h @@ -15,13 +15,44 @@ #ifndef _X86_PCI_CONFIG_ACCESS_H_ #define _X86_PCI_CONFIG_ACCESS_H_ -#include #include #include #include +#include +#include using namespace Genode; +namespace Platform { namespace Pci { struct Bdf; } } + + +struct Platform::Pci::Bdf +{ + struct Bdf_register : Register<16> + { + struct Function : Bitfield< 0, 3> { }; + struct Device : Bitfield< 3, 5> { }; + struct Bus : Bitfield< 8, 8> { }; + }; + + uint16_t bdf; + + Bdf(uint16_t bdf) : bdf(bdf) { } + + Bdf(unsigned bus, unsigned device, unsigned function) : bdf(0) + { + Bdf_register::Bus::set(bdf, bus); + Bdf_register::Device::set(bdf, device); + Bdf_register::Function::set(bdf, function); + } + + uint8_t bus() const { return Bdf_register::Bus::get(bdf); } + uint8_t device() const { return Bdf_register::Device::get(bdf); } + uint8_t function() const { return Bdf_register::Function::get(bdf); } + + bool operator == (Bdf const &other) const { return bdf == other.bdf; } +}; + namespace Platform { class Config_access @@ -34,17 +65,11 @@ namespace Platform { /** * Calculate device offset from BDF * - * \param bus target PCI bus ID (0..255) - * \param device target device ID (0..31) - * \param function target function ID (0..7) - * * \return device base address */ - unsigned _dev_base(int bus, int device, int function) + unsigned _dev_base(Pci::Bdf const bdf) { - return ((bus << 20) | - (device << 15) | - (function << 12)); + return unsigned(bdf.bdf) << 12; } Genode::Bit_array<256> _used { }; @@ -72,9 +97,7 @@ namespace Platform { /** * Read value from config space of specified device/function * - * \param bus target PCI bus ID - * \param device target device ID - * \param function target function ID + * \param bdf target PCI bus, device & function ID * \param addr target byte within targeted PCI config space * \param size bit width of read access * @@ -82,12 +105,11 @@ namespace Platform { * * There is no range check for the input values. */ - unsigned read(int bus, int device, int function, - unsigned char addr, Device::Access_size size, - bool track = true) + unsigned read(Pci::Bdf const bdf, unsigned char const addr, + Device::Access_size const size, bool const track = true) { unsigned ret; - unsigned const offset = _dev_base(bus, device, function) + addr; + unsigned const offset = _dev_base(bdf) + addr; char const * const field = _pciconf.local_addr() + offset; if (offset >= _pciconf_size) @@ -129,20 +151,18 @@ namespace Platform { /** * Write to config space of specified device/function * - * \param bus target PCI bus ID - * \param device target device ID - * \param function target function ID + * \param bdf target PCI bus, device & function ID * \param addr target byte within targeted PCI config space * \param value value to be written * \param size bit width of write access * * There is no range check for the input values. */ - void write(int bus, int device, int function, unsigned char addr, - unsigned value, Device::Access_size size, - bool track = true) + void write(Pci::Bdf const bdf, unsigned char const addr, + unsigned const value, Device::Access_size const size, + bool const track = true) { - unsigned const offset = _dev_base(bus, device, function) + addr; + unsigned const offset = _dev_base(bdf) + addr; char const * const field = _pciconf.local_addr() + offset; if (offset >= _pciconf_size) diff --git a/repos/os/src/drivers/platform/spec/x86/pci_device_component.h b/repos/os/src/drivers/platform/spec/x86/pci_device_component.h index c5766d74a5..f9456c9478 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_device_component.h +++ b/repos/os/src/drivers/platform/spec/x86/pci_device_component.h @@ -165,10 +165,7 @@ class Platform::Device_component : public Genode::Rpc_object, return Irq_session_component::INVALID_IRQ; /* lookup rewrite information as provided by acpi table */ - uint16_t irq_r = Irq_routing::rewrite(_device_config.bus_number(), - _device_config.device_number(), - _device_config.function_number(), - pin); + uint16_t irq_r = Irq_routing::rewrite(_device_config.bdf(), pin); if (irq_r) { Genode::log(_device_config, " adjust IRQ as reported by ACPI: ", irq, " -> ", irq_r); @@ -353,9 +350,9 @@ class Platform::Device_component : public Genode::Rpc_object, void bus_address(unsigned char *bus, unsigned char *dev, unsigned char *fn) override { - *bus = _device_config.bus_number(); - *dev = _device_config.device_number(); - *fn = _device_config.function_number(); + *bus = _device_config.bdf().bus(); + *dev = _device_config.bdf().device(); + *fn = _device_config.bdf().function(); } unsigned short vendor_id() override { return _device_config.vendor_id(); } diff --git a/repos/os/src/drivers/platform/spec/x86/pci_device_config.h b/repos/os/src/drivers/platform/spec/x86/pci_device_config.h index 12f55e2bb0..09e5bdb022 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_device_config.h +++ b/repos/os/src/drivers/platform/spec/x86/pci_device_config.h @@ -97,9 +97,7 @@ namespace Platform { { private: - uint8_t _bus = 0; - uint8_t _device = 0; - uint8_t _function = 0; /* location at PCI bus */ + Pci::Bdf _bdf; /* * Information provided by the PCI config space @@ -147,29 +145,22 @@ namespace Platform { /** * Constructor */ - Device_config() : _vendor_id(INVALID_VENDOR) { } + Device_config() : _bdf(0, 0, 0), _vendor_id(INVALID_VENDOR) { } - Device_config(unsigned bdf) - : - _bus((bdf >> 8) & 0xff), - _device((bdf >> 3) & 0x1f), - _function(bdf & 0x7) - { } + Device_config(Pci::Bdf bdf) : _bdf(bdf) { } - Device_config(int bus, int device, int function, - Config_access *pci_config): - _bus(bus), _device(device), _function(function) + Device_config(Pci::Bdf bdf, Config_access *pci_config) : _bdf(bdf) { - _vendor_id = pci_config->read(bus, device, function, 0, Device::ACCESS_16BIT); + _vendor_id = pci_config->read(bdf, 0, Device::ACCESS_16BIT); /* break here if device is invalid */ if (_vendor_id == INVALID_VENDOR) return; - _device_id = pci_config->read(bus, device, function, 2, Device::ACCESS_16BIT); - _class_code = pci_config->read(bus, device, function, 8, Device::ACCESS_32BIT) >> 8; + _device_id = pci_config->read(bdf, 2, Device::ACCESS_16BIT); + _class_code = pci_config->read(bdf, 8, Device::ACCESS_32BIT) >> 8; _class_code &= 0xffffff; - _header_type = pci_config->read(bus, device, function, 0xe, Device::ACCESS_8BIT); + _header_type = pci_config->read(bdf, 0xe, Device::ACCESS_8BIT); _header_type &= 0x7f; /* @@ -178,10 +169,12 @@ namespace Platform { * device. Note, the mf bit of function 1-7 is not significant * and may be set or unset. */ - if (function != 0 - && !(pci_config->read(bus, device, 0, 0xe, Device::ACCESS_8BIT) & 0x80)) { - _vendor_id = INVALID_VENDOR; - return; + if (bdf.function() != 0) { + Pci::Bdf const dev(bdf.bus(), bdf.device(), 0); + if (!(pci_config->read(dev, 0xe, Device::ACCESS_8BIT) & 0x80)) { + _vendor_id = INVALID_VENDOR; + return; + } } /* @@ -200,7 +193,7 @@ namespace Platform { /* read base-address register value */ unsigned const bar_value = - pci_config->read(bus, device, function, bar_idx, Device::ACCESS_32BIT); + pci_config->read(bdf, bar_idx, Device::ACCESS_32BIT); /* skip invalid resource BARs */ if (bar_value == ~0U || bar_value == 0U) { @@ -216,9 +209,9 @@ namespace Platform { * corresponding to the resource size. Finally, we write * back the bar-address value as assigned by the BIOS. */ - pci_config->write(bus, device, function, bar_idx, ~0, Device::ACCESS_32BIT); - unsigned const bar_size = pci_config->read(bus, device, function, bar_idx, Device::ACCESS_32BIT); - pci_config->write(bus, device, function, bar_idx, bar_value, Device::ACCESS_32BIT); + pci_config->write(bdf, bar_idx, ~0, Device::ACCESS_32BIT); + unsigned const bar_size = pci_config->read(bdf, bar_idx, Device::ACCESS_32BIT); + pci_config->write(bdf, bar_idx, bar_value, Device::ACCESS_32BIT); if (!Resource::Bar::mem64(bar_value)) { _resource[i] = Resource(bar_value, bar_size); @@ -227,11 +220,11 @@ namespace Platform { /* also consume next BAR for MEM64 */ unsigned const bar2_idx = bar_idx + 4; unsigned const bar2_value = - pci_config->read(bus, device, function, bar2_idx, Device::ACCESS_32BIT); - pci_config->write(bus, device, function, bar2_idx, ~0, Device::ACCESS_32BIT); + pci_config->read(bdf, bar2_idx, Device::ACCESS_32BIT); + pci_config->write(bdf, bar2_idx, ~0, Device::ACCESS_32BIT); unsigned const bar2_size = - pci_config->read(bus, device, function, bar2_idx, Device::ACCESS_32BIT); - pci_config->write(bus, device, function, bar2_idx, bar2_value, Device::ACCESS_32BIT); + pci_config->read(bdf, bar2_idx, Device::ACCESS_32BIT); + pci_config->write(bdf, bar2_idx, bar2_value, Device::ACCESS_32BIT); /* combine into first resource and mark second as invalid */ _resource[i] = Resource(bar_value, bar_size, @@ -244,24 +237,19 @@ namespace Platform { } /** - * Accessor functions for device location + * Accessor function for device location */ - int bus_number() { return _bus; } - int device_number() { return _device; } - int function_number() { return _function; } + Pci::Bdf bdf() const { return _bdf; } void print(Genode::Output &out) const { using Genode::print; using Genode::Hex; - print(out, Hex(_bus, Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), - ":", Hex(_device, Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), - ".", Hex(_function, Hex::Prefix::OMIT_PREFIX)); + print(out, Hex(_bdf.bus(), Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), + ":", Hex(_bdf.device(), Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), + ".", Hex(_bdf.function(), Hex::Prefix::OMIT_PREFIX)); } - Genode::uint16_t bdf () { - return (_bus << 8) | (_device << 3) | (_function & 0x7); } - /** * Accessor functions for device information */ @@ -298,8 +286,7 @@ namespace Platform { unsigned read(Config_access &pci_config, unsigned char address, Device::Access_size size, bool track = true) { - return pci_config.read(_bus, _device, _function, address, - size, track); + return pci_config.read(_bdf, address, size, track); } /** @@ -309,8 +296,7 @@ namespace Platform { unsigned long value, Device::Access_size size, bool track = true) { - pci_config.write(_bus, _device, _function, address, value, - size, track); + pci_config.write(_bdf, address, value, size, track); } bool reg_in_use(Config_access &pci_config, unsigned char address, @@ -346,10 +332,10 @@ namespace Platform { : _bdf_start(bdf_start), _func_count(func_count), _base(base) {} - Genode::addr_t lookup_config_space(Genode::uint32_t bdf) + Genode::addr_t lookup_config_space(Pci::Bdf const bdf) { - if ((_bdf_start <= bdf) && (bdf <= _bdf_start + _func_count - 1)) - return _base + (bdf << 12); + if ((_bdf_start <= bdf.bdf) && (bdf.bdf <= _bdf_start + _func_count - 1)) + return _base + (unsigned(bdf.bdf) << 12); return 0; } }; diff --git a/repos/os/src/drivers/platform/spec/x86/pci_session_component.h b/repos/os/src/drivers/platform/spec/x86/pci_session_component.h index e9b47fece5..127e5f8541 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_session_component.h +++ b/repos/os/src/drivers/platform/spec/x86/pci_session_component.h @@ -79,9 +79,11 @@ class Platform::Rmrr : public Genode::List::Element Genode::uint8_t func) : _bus(bus), _dev(dev), _func(func) { } - bool match(Genode::uint8_t bus, Genode::uint8_t dev, - Genode::uint8_t func) { - return bus == _bus && dev == _dev && func == _func; } + bool match(Pci::Bdf const bdf) + { + return bdf.bus() == _bus && bdf.device() == _dev && + bdf.function() == _func; + } }; private: @@ -103,12 +105,8 @@ class Platform::Rmrr : public Genode::List::Element Genode::Io_mem_dataspace_capability match(Genode::Env &env, Device_config config) { - Genode::uint8_t bus = config.bus_number(); - Genode::uint8_t device = config.device_number(); - Genode::uint8_t function = config.function_number(); - for (Bdf *bdf = _bdf_list.first(); bdf; bdf = bdf->next()) { - if (!bdf->match(bus, device, function)) + if (!bdf->match(config.bdf())) continue; if (_cap.valid()) @@ -183,7 +181,8 @@ class Platform::Pci_buses for (; function < Device_config::MAX_FUNCTIONS; function++) { /* read config space */ - Device_config config(bus, device, function, config_access); + Pci::Bdf const bdf(bus, device, function); + Device_config config(bdf, config_access); if (config.valid()) { *out_device_config = config; @@ -263,7 +262,7 @@ class Platform::Session_component : public Genode::Rpc_object * the corresponding extended 4K PCI config space address. * A io mem dataspace is created and returned. */ - Genode::addr_t lookup_config_space(Genode::uint16_t const bdf) + Genode::addr_t lookup_config_space(Pci::Bdf const bdf) { using namespace Genode; @@ -652,9 +651,9 @@ class Platform::Session_component : public Genode::Rpc_object if (prev) { Device_config config = prev->device_config(); - bus = config.bus_number(); - device = config.device_number(); - function = config.function_number(); + bus = config.bdf().bus(); + device = config.bdf().device(); + function = config.bdf().function(); } /* @@ -670,9 +669,9 @@ class Platform::Session_component : public Genode::Rpc_object return Device_capability(); /* get new bdf values */ - bus = config.bus_number(); - device = config.device_number(); - function = config.function_number(); + bus = config.bdf().bus(); + device = config.bdf().device(); + function = config.bdf().function(); /* if filter of driver don't match skip and continue */ if ((config.class_code() ^ device_class) & class_mask) @@ -702,16 +701,12 @@ class Platform::Session_component : public Genode::Rpc_object try { /* if more than one driver uses the device - warn about */ - if (bdf_in_use.get(Device_config::MAX_BUSES * bus + - Device_config::MAX_DEVICES * device + - function, 1)) + if (bdf_in_use.get(config.bdf().bdf, 1)) Genode::error("Device ", config, " is used by more than one driver - " "session '", _label, "'."); else - bdf_in_use.set(Device_config::MAX_BUSES * bus + - Device_config::MAX_DEVICES * device + - function, 1); + bdf_in_use.set(config.bdf().bdf, 1); return _env.ep().rpc_ep().manage(dev); } catch (...) { @@ -733,15 +728,8 @@ class Platform::Session_component : public Genode::Rpc_object return; if (device->device_config().valid()) { - unsigned const bus = device->device_config().bus_number(); - unsigned const dev = device->device_config().device_number(); - unsigned const func = device->device_config().function_number(); - - if (bdf_in_use.get(Device_config::MAX_BUSES * bus + - Device_config::MAX_DEVICES * dev + - func, 1)) - bdf_in_use.clear(Device_config::MAX_BUSES * bus + - Device_config::MAX_DEVICES * dev + func, 1); + if (bdf_in_use.get(device->device_config().bdf().bdf, 1)) + bdf_in_use.clear(device->device_config().bdf().bdf, 1); } _device_list.remove(device); @@ -763,11 +751,8 @@ class Platform::Session_component : public Genode::Rpc_object return; try { - addr_t const function = device->device_config().bus_number() * 32 * 8 + - device->device_config().device_number() * 8 + - device->device_config().function_number(); addr_t const base_ecam = Dataspace_client(_pciconf.cap()).phys_addr(); - addr_t const base_offset = 0x1000UL * function; + addr_t const base_offset = 0x1000UL * device->device_config().bdf().bdf; if (base_ecam + base_offset != device->config_space()) throw 1; @@ -778,7 +763,8 @@ class Platform::Session_component : public Genode::Rpc_object _device_pd.attach_dma_mem(rmrr_cap); } - _device_pd.assign_pci(_pciconf.cap(), base_offset, device->device_config().bdf()); + _device_pd.assign_pci(_pciconf.cap(), base_offset, + device->device_config().bdf().bdf); } catch (...) { Genode::error("assignment to device pd or of RMRR region failed"); @@ -857,8 +843,12 @@ class Platform::Root : public Genode::Root_component Session_component::add_config_space(bdf_start, func_count, base, _heap); - Device_config const bdf_first(bdf_start); - Device_config const bdf_last(bdf_start + func_count - 1); + Pci::Bdf const bdf_s(bdf_start); + Pci::Bdf const bdf_l(bdf_start + func_count - 1); + + Device_config const bdf_first(bdf_s); + Device_config const bdf_last(bdf_l); + addr_t const memory_size = 0x1000UL * func_count; /* Simplification: Only consider first config space and @@ -937,8 +927,8 @@ class Platform::Root : public Genode::Root_component path.attribute("dev") .value(dev); path.attribute("func").value(func); - Device_config bridge(bus, dev, func, - &config_access); + Pci::Bdf const bdf(bus, dev, func); + Device_config bridge(bdf, &config_access); if (bridge.pci_bridge()) /* PCI bridge spec 3.2.5.3, 3.2.5.4 */ bus = bridge.read(config_access, 0x19, @@ -1062,7 +1052,7 @@ class Platform::Root : public Genode::Root_component } if (Platform::Bridge::root_bridge_bdf < Platform::Bridge::INVALID_ROOT_BRIDGE) { - Device_config config(Platform::Bridge::root_bridge_bdf); + Device_config config(Pci::Bdf(Platform::Bridge::root_bridge_bdf)); Genode::log("Root bridge: ", config); } else Genode::warning("Root bridge: unknown"); @@ -1087,9 +1077,9 @@ class Platform::Root : public Genode::Root_component &config_access)) return; - bus = config.bus_number(); - device = config.device_number(); - function = config.function_number(); + bus = config.bdf().bus(); + device = config.bdf().device(); + function = config.bdf().function(); using Genode::String; using Genode::Hex; diff --git a/repos/os/src/drivers/platform/spec/x86/session.cc b/repos/os/src/drivers/platform/spec/x86/session.cc index ed2559d34f..6005f03aa0 100644 --- a/repos/os/src/drivers/platform/spec/x86/session.cc +++ b/repos/os/src/drivers/platform/spec/x86/session.cc @@ -45,7 +45,7 @@ void Platform::Pci_buses::scan_bus(Config_access &config_access, for (int fun = 0; fun < Device_config::MAX_FUNCTIONS; ++fun) { /* read config space */ - Device_config config(bus, dev, fun, &config_access); + Device_config config(Pci::Bdf(bus, dev, fun), &config_access); /* * Switch off PCI bus master DMA for some classes of devices,