From 5ca30b0318cb44c28c7df1dc14a0bed7237b7f6d Mon Sep 17 00:00:00 2001 From: Alexander Boettcher Date: Tue, 22 May 2018 09:10:02 +0200 Subject: [PATCH] platform_drv: disable PCI DMA class specific for such classes where it should be safe and where we have seen issues. Disabling in general bus master DMA causes on some machines hard hangs, e.g. because the USB handover protocol was violated. Fixes #2835 --- repos/dde_ipxe/src/lib/dde_ipxe/dde.c | 8 +-- .../drivers/platform/spec/x86/pci_device.cc | 9 ++-- .../platform/spec/x86/pci_device_component.h | 16 ++---- .../platform/spec/x86/pci_device_config.h | 15 ++++++ .../src/drivers/platform/spec/x86/session.cc | 52 ++++++++++++++----- 5 files changed, 64 insertions(+), 36 deletions(-) diff --git a/repos/dde_ipxe/src/lib/dde_ipxe/dde.c b/repos/dde_ipxe/src/lib/dde_ipxe/dde.c index e73bfb9017..a6aada6922 100644 --- a/repos/dde_ipxe/src/lib/dde_ipxe/dde.c +++ b/repos/dde_ipxe/src/lib/dde_ipxe/dde.c @@ -242,13 +242,7 @@ void adjust_pci_device ( struct pci_device *pci ) { pci_read_config_word(pci, PCI_COMMAND, &pci_command); new_command = pci_command | PCI_COMMAND_MASTER | PCI_COMMAND_MEM | PCI_COMMAND_IO; - if (pci_command != new_command) { - LOG("PCI BIOS has not enabled device " FMT_BUSDEVFN "! " - "Updating PCI command %04x->%04x\n", PCI_BUS(pci->busdevfn), - PCI_SLOT(pci->busdevfn), PCI_FUNC (pci->busdevfn), - pci_command, new_command); - pci_write_config_word(pci, PCI_COMMAND, new_command); - } + pci_write_config_word(pci, PCI_COMMAND, new_command); unsigned char pci_latency; pci_read_config_byte ( pci, PCI_LATENCY_TIMER, &pci_latency); diff --git a/repos/os/src/drivers/platform/spec/x86/pci_device.cc b/repos/os/src/drivers/platform/spec/x86/pci_device.cc index 11a5e440d1..5386cd46ec 100644 --- a/repos/os/src/drivers/platform/spec/x86/pci_device.cc +++ b/repos/os/src/drivers/platform/spec/x86/pci_device.cc @@ -108,10 +108,10 @@ void Platform::Device_component::config_write(unsigned char address, "size=", Genode::Hex(size), " " "denied - it is used by the platform driver."); return; - case PCI_CMD_REG: /* COMMAND register - first byte */ + case Device_config::PCI_CMD_REG: /* COMMAND register - first byte */ if (size == Access_size::ACCESS_16BIT) break; - case PCI_CMD_REG + 1: /* COMMAND register - second byte */ + case Device_config::PCI_CMD_REG + 1: /* COMMAND register - second byte */ case 0xd: /* Latency timer */ if (size == Access_size::ACCESS_8BIT) break; @@ -125,13 +125,16 @@ void Platform::Device_component::config_write(unsigned char address, } /* assign device to device_pd */ - if (address == PCI_CMD_REG && value & PCI_CMD_DMA) { + if (address == Device_config::PCI_CMD_REG && + (value & Device_config::PCI_CMD_DMA)) { + try { _session.assign_device(this); } catch (Out_of_ram) { throw; } catch (Out_of_caps) { throw; } catch (...) { Genode::error("assignment to device failed"); } + _enabled_bus_master = true; } _device_config.write(_config_access, address, value, 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 8d9089e8fd..71c1e29fc0 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 @@ -48,8 +48,9 @@ class Platform::Device_component : public Genode::Rpc_object, Genode::addr_t _config_space; Config_access _config_access; Platform::Session_component &_session; - unsigned short _irq_line; Irq_session_component *_irq_session = nullptr; + unsigned short _irq_line; + bool _enabled_bus_master { false }; Genode::Allocator &_global_heap; @@ -72,8 +73,6 @@ class Platform::Device_component : public Genode::Rpc_object, Device::NUM_RESOURCES + 32 + 8 * sizeof(void *), IO_MEM_SIZE = sizeof(Io_mem) * Device::NUM_RESOURCES + 32 + 8 * sizeof(void *), - PCI_CMD_REG = 0x4, - PCI_CMD_DMA = 0x4, PCI_IRQ_LINE = 0x3c, PCI_IRQ_PIN = 0x3d, @@ -184,12 +183,7 @@ class Platform::Device_component : public Genode::Rpc_object, if (_device_config.pci_bridge()) return; - unsigned cmd = _device_config.read(_config_access, PCI_CMD_REG, - Platform::Device::ACCESS_16BIT); - if (cmd & PCI_CMD_DMA) - _device_config.write(_config_access, PCI_CMD_REG, - cmd ^ PCI_CMD_DMA, - Platform::Device::ACCESS_16BIT); + _device_config.disable_bus_master_dma(_config_access); } public: @@ -217,8 +211,6 @@ class Platform::Device_component : public Genode::Rpc_object, for (unsigned i = 0; i < Device::NUM_RESOURCES; i++) { _io_port_conn[i] = nullptr; } - - _disable_bus_master_dma(); } /** @@ -263,7 +255,7 @@ class Platform::Device_component : public Genode::Rpc_object, } } - if (_device_config.valid()) + if (_device_config.valid() && _enabled_bus_master) _disable_bus_master_dma(); } 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 71348cb3cb..8d42953f9d 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 @@ -65,6 +65,12 @@ namespace Platform { enum { MAX_BUSES = 256, MAX_DEVICES = 32, MAX_FUNCTIONS = 8 }; + enum { + PCI_CMD_REG = 0x4, + PCI_CMD_MASK = 0x7, /* IOPORT (1), MEM(2), DMA(4) */ + PCI_CMD_DMA = 0x4, + }; + /** * Constructor */ @@ -220,6 +226,15 @@ namespace Platform { bool reg_in_use(Config_access &pci_config, unsigned char address, Device::Access_size size) { return pci_config.reg_in_use(address, size); } + + void disable_bus_master_dma(Config_access &pci_config) + { + unsigned const cmd = read(pci_config, PCI_CMD_REG, + Platform::Device::ACCESS_16BIT); + if (cmd & PCI_CMD_DMA) + write(pci_config, PCI_CMD_REG, cmd ^ PCI_CMD_DMA, + Platform::Device::ACCESS_16BIT); + } }; class Config_space : private Genode::List::Element diff --git a/repos/os/src/drivers/platform/spec/x86/session.cc b/repos/os/src/drivers/platform/spec/x86/session.cc index d245efe76b..ed2559d34f 100644 --- a/repos/os/src/drivers/platform/spec/x86/session.cc +++ b/repos/os/src/drivers/platform/spec/x86/session.cc @@ -47,6 +47,33 @@ void Platform::Pci_buses::scan_bus(Config_access &config_access, /* read config space */ Device_config config(bus, dev, fun, &config_access); + /* + * Switch off PCI bus master DMA for some classes of devices, + * which caused trouble. + * Some devices are enabled by BIOS/UEFI or/and bootloaders and + * aren't switch off when handing over to the kernel and Genode. + * By disabling bus master DMA they should stop to issue DMA + * operations and IRQs. IRQs are problematic, if it is a shared + * IRQ - e.g. Ethernet and graphic card share a GSI IRQ. If the + * graphic card driver is started without a Ethernet driver, + * the graphic card may ack all IRQs. We may end up in a endless + * IRQ/ACK loop, since no Ethernet driver acknowledge/disable IRQ + * generation on the Ethernet device. + * + * Switching off PCI bus master DMA in general is a bad idea, + * since some device classes require a explicit handover protocol + * between BIOS/UEFI and device, e.g. USB. Violating such protocols + * lead to hard hangs on some machines. + */ + if (config.class_code() >> 8) { + uint16_t classcode = config.class_code() >> 16; + uint16_t subclass = (config.class_code() >> 8) & 0xff; + + if ((classcode == 0x2 && subclass == 0x00) /* ETHERNET */) { + config.disable_bus_master_dma(config_access); + } + } + if (!config.valid()) continue; @@ -68,25 +95,22 @@ void Platform::Pci_buses::scan_bus(Config_access &config_access, bridges()->insert(new (heap) Bridge(bus, dev, fun, sec_bus, sub_bus)); - enum { - PCI_CMD_REG = 0x4, - PCI_CMD_MASK = 0x7 /* IOPORT, MEM, DMA */ - }; + uint16_t cmd = config.read(config_access, + Device_config::PCI_CMD_REG, + Platform::Device::ACCESS_16BIT); - unsigned short cmd = config.read(config_access, PCI_CMD_REG, - Platform::Device::ACCESS_16BIT); + const bool enabled = (cmd & Device_config::PCI_CMD_MASK) + == Device_config::PCI_CMD_MASK; - if ((cmd & PCI_CMD_MASK) != PCI_CMD_MASK) { - config.write(config_access, PCI_CMD_REG, - cmd | PCI_CMD_MASK, + if (!enabled) { + config.write(config_access, Device_config::PCI_CMD_REG, + cmd | Device_config::PCI_CMD_MASK, Platform::Device::ACCESS_16BIT); } - Genode::log(config, " - bridge ", - Hex(sec_bus, Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), - ":00.0", - ((cmd & PCI_CMD_MASK) != PCI_CMD_MASK) ? " enabled" - : ""); + log(config, " - bridge ", + Hex(sec_bus, Hex::Prefix::OMIT_PREFIX, Hex::Pad::PAD), + ":00.0", !enabled ? " enabled" : ""); scan_bus(config_access, heap, sec_bus); }