From b0bc24aff5205e71aebbb7a15872974bb131d0f3 Mon Sep 17 00:00:00 2001 From: Alex Williamson Date: Thu, 19 Jun 2014 01:41:15 +0000 Subject: [PATCH] The following call tree exposes a bug in the lifetime (ie. too late creation) of PciIoDevice->DevicePath. The bug can be triggered when PciBusDxe is built into OVMF, with qemu/KVM device assignment of a PCI-express device on the default 440FX machine type. OVMF correctly discovers that the device is PCIe and begins probing extended configuration space for the device. The root bridge has no way to access extended config space and correctly errors, sending us into the error reporting chain seen below. It's possible that this error path could also be reproduced on physical hardware when a PCI-to-PCIe bridge is present. GatherDeviceInfo() | GatherPpbInfo() | GatherP2CInfo() [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] CreatePciIoDevice() [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] AllocateZeroPool() LocateCapabilityRegBlock() [MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c] PciIoDevice->IsPciExp = TRUE LocatePciExpressCapabilityRegBlock() [MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c] PciIoConfigRead() via funcptr [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c] RootBridgeIoPciRead() via funcptr [PcAtChipsetPkg/PciHostBridgeDxe/PciRootBridgeIo.c] FAILS REPORT_STATUS_CODE_WITH_DEVICE_PATH() [MdePkg/Include/Library/ReportStatusCodeLib.h] ReportStatusCodeWithDevicePath() [MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c] ASSERT (DevicePath != NULL) <--+ CreatePciDevicePath() | [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] sets PciIoDevice->DevicePath ----------+ In English: - CreatePciIoDevice() allocates a zeroed out PCI_IO_DEVICE structure. - PciIoConfigRead() tries to access the (extended) config space, and fails. - PciIoConfigRead() wants to report a status code (read error) for the device path. - Unfortuantely, PciIoDevice->DevicePath is still NULL at that point. - The ASSERT() in ReportStatusCodeWithDevicePath() fires. Fix it by moving CreatePciDevicePath() into CreatePciIoDevice(), allowing PciIoDevice->DevicePath to be initialized before we begin probing the device capabilities: GatherDeviceInfo() | GatherPpbInfo() | GatherP2CInfo() [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] CreatePciIoDevice() [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] AllocateZeroPool() CreatePciDevicePath() [MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c] sets PciIoDevice->DevicePath -----------+ LocateCapabilityRegBlock() | [MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c] PciIoDevice->IsPciExp = TRUE | LocatePciExpressCapabilityRegBlock() | [MdeModulePkg/Bus/Pci/PciBusDxe/PciCommand.c] PciIoConfigRead() via funcptr | [MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c] RootBridgeIoPciRead() via funcptr | [PcAtChipsetPkg/PciHostBridgeDxe/PciRootBridgeIo.c] FAILS | REPORT_STATUS_CODE_WITH_DEVICE_PATH() | [MdePkg/Include/Library/ReportStatusCodeLib.h] ReportStatusCodeWithDevicePath() | [MdeModulePkg/Library/DxeReportStatusCodeLib/ReportStatusCodeLib.c] ASSERT (DevicePath != NULL) <-----+ Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Alex Williamson Reviewed-by: Laszlo Ersek git-svn-id: https://svn.code.sf.net/p/edk2/code/trunk/edk2@15567 6f19259b-4bc3-4df7-8a09-765794883524 --- .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 34 +++++-------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c index 8c31831ad9..26c9a33854 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c @@ -1,7 +1,7 @@ /** @file PCI emumeration support functions implementation for PCI Bus module. -Copyright (c) 2006 - 2012, Intel Corporation. All rights reserved.
+Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.
This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -392,14 +392,6 @@ GatherDeviceInfo ( return NULL; } - // - // Create a device path for this PCI device and store it into its private data - // - CreatePciDevicePath ( - Bridge->DevicePath, - PciIoDevice - ); - // // If it is a full enumeration, disconnect the device in advance // @@ -475,14 +467,6 @@ GatherPpbInfo ( return NULL; } - // - // Create a device path for this PCI device and store it into its private data - // - CreatePciDevicePath ( - Bridge->DevicePath, - PciIoDevice - ); - if (gFullEnumeration) { PCI_DISABLE_COMMAND_REGISTER (PciIoDevice, EFI_PCI_COMMAND_BITS_OWNED); @@ -634,14 +618,6 @@ GatherP2CInfo ( return NULL; } - // - // Create a device path for this PCI device and store it into its private data - // - CreatePciDevicePath ( - Bridge->DevicePath, - PciIoDevice - ); - if (gFullEnumeration) { PCI_DISABLE_COMMAND_REGISTER (PciIoDevice, EFI_PCI_COMMAND_BITS_OWNED); @@ -2028,6 +2004,14 @@ CreatePciIoDevice ( InitializePciLoadFile2 (PciIoDevice); PciIo = &PciIoDevice->PciIo; + // + // Create a device path for this PCI device and store it into its private data + // + CreatePciDevicePath ( + Bridge->DevicePath, + PciIoDevice + ); + // // Detect if PCI Express Device //