Williamson, Thanks for fixing this issue. The patch was committed @ r15567.
Thanks, Ray -----Original Message----- From: Alex Williamson [mailto:[email protected]] Sent: Thursday, June 19, 2014 12:59 AM To: [email protected] Subject: [edk2] [PATCH] MdeModulePkg: PciBusDxe: fix lifetime of PciIoDevice->DevicePath 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 <[email protected]> Reviewed-by: Laszlo Ersek <[email protected]> --- .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 32 +++++--------------- 1 file changed, 8 insertions(+), 24 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c index 8c31831..f5060fc 100644 --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c @@ -393,14 +393,6 @@ GatherDeviceInfo ( } // - // 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 // if (gFullEnumeration) { @@ -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); @@ -2029,6 +2005,14 @@ CreatePciIoDevice ( 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 // PciIoDevice->PciExpressCapabilityOffset = 0; ------------------------------------------------------------------------------ HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions Find What Matters Most in Your Big Data with HPCC Systems Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. Leverages Graph Analysis for Fast Processing & Easy Data Exploration http://p.sf.net/sfu/hpccsystems _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
