From ca54cdef63809b776eb85585c7c2ec45df43e3ef Mon Sep 17 00:00:00 2001 From: Michael Kubacki Date: Mon, 14 Aug 2023 16:57:34 -0400 Subject: [PATCH] MdeModulePkg/NvmExpressDxe: Improve robustness [Rebase & FF] (#516) ## Description Resolves #515 The main change the first commit: ``` MdeModulePkg/NvmExpressDxe: Improve NVMe controller init robustness It has been observed that some faulty NVMe devices may return invalid values. The code in NvmExpressDxe recognizes the controller is not responding correctly and issues an ASSERT() often in DEBUG builds or a reset in RELEASE builds. The following changes are made to NvmeControllerInit() to prevent a faulty NVMe device from halting the overall boot: 1. Check the Vendor ID and Device ID to verify they are read properly and if not, return EFI_DEVICE_ERROR 2. Replace the ASSERT() when Memory Page Size Minimum (Cap.Mpsmin) with an error message and return EFI_DEVICE_ERROR ``` The additional changes are added due to improvements found in nearby code. 2: ``` MdeModulePkg/NvmExpressDxe: Correct function parameter modifer Updates the `Cap` parameter for `ReadNvmeControllerCapabilities()` to be `OUT` since the buffer pointed to is written within the function. ``` 3: ``` MdePkg/Nvme.h: Add missing NVMe capability descriptions Adds missing structure member documentation. ``` This will also be sent to the edk2 mailing list, however, it is being checked in here first due to some devices that may be able to benefit from the change and the upcoming 202308 freeze. - [ ] Impacts functionality? - **Functionality** - Does the change ultimately impact how firmware functions? - Examples: Add a new library, publish a new PPI, update an algorithm, ... - [ ] Impacts security? - **Security** - Does the change have a direct security impact on an application, flow, or firmware? - Examples: Crypto algorithm change, buffer overflow fix, parameter validation improvement, ... - [ ] Breaking change? - **Breaking change** - Will anyone consuming this change experience a break in build or boot behavior? - Examples: Add a new library class, move a module to a different repo, call a function in a new library class in a pre-existing module, ... - [ ] Includes tests? - **Tests** - Does the change include any explicit test code? - Examples: Unit tests, integration tests, robot tests, ... - [ ] Includes documentation? - **Documentation** - Does the change contain explicit documentation additions outside direct code modifications (and comments)? - Examples: Update readme file, add feature readme file, link to documentation on an a separate Web page, ... ## How This Was Tested - Verified NVMe boot on a physical system - Verified NVMe driver binding on QEMU Q35 with an emulated NVMe device ## Integration Instructions N/A --------- Signed-off-by: Michael Kubacki --- .../Bus/Pci/NvmExpressDxe/NvmExpressHci.c | 43 +++++++++++++++++-- MdePkg/Include/IndustryStandard/Nvme.h | 14 +++--- 2 files changed, 47 insertions(+), 10 deletions(-) diff --git a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c index 5eb9d922fd..4e9831c72b 100644 --- a/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c +++ b/MdeModulePkg/Bus/Pci/NvmExpressDxe/NvmExpressHci.c @@ -30,8 +30,10 @@ UINTN mNvmeControllerNumber = 0; **/ EFI_STATUS ReadNvmeControllerCapabilities ( - IN NVME_CONTROLLER_PRIVATE_DATA *Private, - IN NVME_CAP *Cap + // MU_CHANGE [BEGIN] - Correct Cap parameter modifier + IN NVME_CONTROLLER_PRIVATE_DATA *Private, + OUT NVME_CAP *Cap + // MU_CHANGE [END] - Correct Cap parameter modifier ) { EFI_PCI_IO_PROTOCOL *PciIo; @@ -743,13 +745,36 @@ NvmeControllerInit ( NVME_AQA Aqa; NVME_ASQ Asq; NVME_ACQ Acq; + UINT16 VidDid[2]; // MU_CHANGE - Improve NVMe controller init robustness UINT8 Sn[21]; UINT8 Mn[41]; + // MU_CHANGE [BEGIN] - Improve NVMe controller init robustness + PciIo = Private->PciIo; + + // + // Verify the controller is still accessible + // + Status = PciIo->Pci.Read ( + PciIo, + EfiPciIoWidthUint16, + PCI_VENDOR_ID_OFFSET, + ARRAY_SIZE (VidDid), + VidDid + ); + if (EFI_ERROR (Status)) { + ASSERT_EFI_ERROR (Status); + return EFI_DEVICE_ERROR; + } + + if ((VidDid[0] == 0xFFFF) || (VidDid[1] == 0xFFFF)) { + return EFI_DEVICE_ERROR; + } + // // Enable this controller. // - PciIo = Private->PciIo; + // MU_CHANGE [END] - Improve NVMe controller init robustness Status = PciIo->Attributes ( PciIo, EfiPciIoAttributeOperationSupported, @@ -788,7 +813,17 @@ NvmeControllerInit ( // // Currently the driver only supports 4k page size. // - ASSERT ((Private->Cap.Mpsmin + 12) <= EFI_PAGE_SHIFT); + + // MU_CHANGE [BEGIN] - Improve NVMe controller init robustness + + // Currently, this means Cap.Mpsmin must be zero for an EFI_PAGE_SHIFT size of 12. + // ASSERT ((Private->Cap.Mpsmin + 12) <= EFI_PAGE_SHIFT); + if ((Private->Cap.Mpsmin + 12) > EFI_PAGE_SHIFT) { + DEBUG ((DEBUG_ERROR, "NvmeControllerInit: Mpsmin is larger than expected (0x%02x).\n", Private->Cap.Mpsmin)); + return EFI_DEVICE_ERROR; + } + + // MU_CHANGE [END] - Improve NVMe controller init robustness Private->Cid[0] = 0; Private->Cid[1] = 0; diff --git a/MdePkg/Include/IndustryStandard/Nvme.h b/MdePkg/Include/IndustryStandard/Nvme.h index d1a7b683a0..c494194813 100644 --- a/MdePkg/Include/IndustryStandard/Nvme.h +++ b/MdePkg/Include/IndustryStandard/Nvme.h @@ -52,23 +52,25 @@ // // 3.1.1 Offset 00h: CAP - Controller Capabilities // +// MU_CHANGE [BEGIN] - Add missing capability descriptions typedef struct { UINT16 Mqes; // Maximum Queue Entries Supported UINT8 Cqr : 1; // Contiguous Queues Required UINT8 Ams : 2; // Arbitration Mechanism Supported UINT8 Rsvd1 : 5; - UINT8 To; // Timeout - UINT16 Dstrd : 4; + UINT8 To; // Timeout + UINT16 Dstrd : 4; // Doorbell Stride UINT16 Nssrs : 1; // NVM Subsystem Reset Supported NSSRS UINT16 Css : 8; // Command Sets Supported - Bit 37 UINT16 Bps : 1; // Boot Partition Support - Bit 45 in NVMe1.4 UINT16 Rsvd3 : 2; - UINT8 Mpsmin : 4; - UINT8 Mpsmax : 4; - UINT8 Pmrs : 1; - UINT8 Cmbs : 1; + UINT8 Mpsmin : 4; // Memory Page Size Minimum + UINT8 Mpsmax : 4; // Memory Page Size Maximum + UINT8 Pmrs : 1; // Persistent Memory Region Supported + UINT8 Cmbs : 1; // Controller Memory Buffer Supported UINT8 Rsvd4 : 6; } NVME_CAP; +// MU_CHANGE [END] - Add missing capability descriptions // // 3.1.2 Offset 08h: VS - Version