[PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space

* [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space
@ 2018-06-15 14:13 Sami Mujawar
  2018-06-15 14:16 ` Ard Biesheuvel
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Sami Mujawar @ 2018-06-15 14:13 UTC (permalink / raw)
  To: edk2-devel
  Cc: star.zeng, eric.dong, ruiyu.ni, ard.biesheuvel, leif.lindholm,
	Matteo.Carlini, Stephanie.Hughes-Fitt, evan.lloyd, thomas.abraham,
	nd

The SATA controller driver crashes while accessing the
PCI memory [AHCI Base Registers (ABAR)], as the PCI memory
space is not enabled.

Enable the PCI memory space access to prevent the SATA
Controller driver from crashing.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Sami Mujawar <sami.mujawar@arm.com>
---
The changes can be viewed at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v2

Notes:
    v2:
    - Improved log message and code documentation based on feedback   [SAMI]
    - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE        [ZENG]
    - This SATA Controller driver only uses the PCI BAR5 register
      space which is the AHCI Base Address (ABAR). According to the
      'Serial ATA Advanced Host Controller Interface (AHCI) 1.3.1'
      specification, section 2.1.11, 'This register allocates space
      for the HBA memory registers'.
      The section 2.1.10, allows provision for Optional BARs which
      may support either memory or I/O spaces. However, in the context
      of the current SATA controller driver, which only ever access
      the ABAR, enabling I/O memory space is not required.            [SAMI]
    - Prefer to use // surrounding comments                           [ZENG]
    - Doing this would violate the edk2 coding standard. See EDK2
      Coding Standard Specification, revision 2.20, section 6.2.3.    [SAMI]
    
    v1:
    - Fix SATA Controller driver crash                                [SAMI]

 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 +++++++++++++++++++-
 MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
 2 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8ff885e48e4318 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
@@ -2,6 +2,7 @@
   This driver module produces IDE_CONTROLLER_INIT protocol for Sata Controllers.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2018, ARM Ltd. All rights reserved.<BR>
   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
@@ -364,6 +365,7 @@ SataControllerStart (
   EFI_SATA_CONTROLLER_PRIVATE_DATA  *Private;
   UINT32                            Data32;
   UINTN                             TotalCount;
+  UINT64                            PciAttributes;
 
   DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
 
@@ -406,6 +408,61 @@ SataControllerStart (
   Private->IdeInit.CalculateMode  = IdeInitCalculateMode;
   Private->IdeInit.SetTiming      = IdeInitSetTiming;
   Private->IdeInit.EnumAll        = SATA_ENUMER_ALL;
+  Private->PciAttributesChanged   = FALSE;
+
+  // Save original PCI attributes
+  Status = PciIo->Attributes (
+                    PciIo,
+                    EfiPciIoAttributeOperationGet,
+                    0,
+                    &Private->OriginalPciAttributes
+                    );
+  if (EFI_ERROR (Status)) {
+      goto Done;
+  }
+
+  DEBUG ((
+    EFI_D_INFO,
+    "Original PCI Attributes = 0x%llx\n",
+    Private->OriginalPciAttributes
+    ));
+
+  if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationSupported,
+                      0,
+                      &PciAttributes
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+
+    DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", PciAttributes));
+
+    if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) == 0) {
+      DEBUG ((
+        EFI_D_ERROR,
+        "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n"
+        ));
+      Status = EFI_UNSUPPORTED;
+      goto Done;
+    }
+
+    PciAttributes = Private->OriginalPciAttributes | EFI_PCI_IO_ATTRIBUTE_MEMORY;
+
+    DEBUG ((EFI_D_INFO, "Enable PCI Attributes = 0x%llx\n", PciAttributes));
+    Status = PciIo->Attributes (
+                      PciIo,
+                      EfiPciIoAttributeOperationEnable,
+                      PciAttributes,
+                      NULL
+                      );
+    if (EFI_ERROR (Status)) {
+      goto Done;
+    }
+    Private->PciAttributesChanged = TRUE;
+  }
 
   Status = PciIo->Pci.Read (
                         PciIo,
@@ -414,7 +471,10 @@ SataControllerStart (
                         sizeof (PciData.Hdr.ClassCode),
                         PciData.Hdr.ClassCode
                         );
-  ASSERT_EFI_ERROR (Status);
+  if (EFI_ERROR (Status)) {
+    ASSERT (FALSE);
+    goto Done;
+  }
 
   if (IS_PCI_IDE (&PciData)) {
     Private->IdeInit.ChannelCount = IDE_MAX_CHANNEL;
@@ -481,6 +541,15 @@ Done:
       if (Private->IdentifyValid != NULL) {
         FreePool (Private->IdentifyValid);
       }
+      if (Private->PciAttributesChanged) {
+        // Restore original PCI attributes
+        PciIo->Attributes (
+                 PciIo,
+                 EfiPciIoAttributeOperationSet,
+                 Private->OriginalPciAttributes,
+                 NULL
+                 );
+      }
       FreePool (Private);
     }
   }
@@ -556,6 +625,15 @@ SataControllerStop (
     if (Private->IdentifyValid != NULL) {
       FreePool (Private->IdentifyValid);
     }
+    if (Private->PciAttributesChanged) {
+      // Restore original PCI attributes
+      Private->PciIo->Attributes (
+                        Private->PciIo,
+                        EfiPciIoAttributeOperationSet,
+                        Private->OriginalPciAttributes,
+                        NULL
+                        );
+    }
     FreePool (Private);
   }
 
diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
index f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..f927c1f02713771f92e3b76012848206321391f0 100644
--- a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
+++ b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h
@@ -2,6 +2,7 @@
   Header file for Sata Controller driver.
 
   Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2017, ARM Ltd. All rights reserved.<BR>
   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
@@ -104,6 +105,12 @@ typedef struct _EFI_SATA_CONTROLLER_PRIVATE_DATA {
   //
   EFI_IDENTIFY_DATA                 *IdentifyData;
   BOOLEAN                           *IdentifyValid;
+
+  /// Track the state so that the PCI attributes that were modified
+  /// can be restored to the original value later.
+  BOOLEAN                           PciAttributesChanged;
+  /// Copy of the original PCI Attributes
+  UINT64                            OriginalPciAttributes;
 } EFI_SATA_CONTROLLER_PRIVATE_DATA;
 
 #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, EFI_SATA_CONTROLLER_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE)
-- 
'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'




^ permalink raw reply related	[flat|nested] 11+ messages in thread