[edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space

Sami Mujawar posted 1 patch 5 years, 9 months ago
Failed in applying to current master (apply log)
There is a newer version of this series
MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 80 +++++++++++++++++++-
MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h |  7 ++
2 files changed, 86 insertions(+), 1 deletion(-)
[edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Sami Mujawar 5 years, 9 months ago
The SATA controller driver crashes while accessing the
PCI memory, 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v1

Notes:
    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..21cc101d693f5adfd9d43f0c21a096eb59ba73b1 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,
+    "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..cb82b55763a077f5994c4a00ea4893bfa2e07a79 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)'


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Thomas Abraham 5 years, 9 months ago
On Thu, Jun 14, 2018 at 5:08 PM, Sami Mujawar <sami.mujawar@arm.com> wrote:
> The SATA controller driver crashes while accessing the
> PCI memory, 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v1
>
> Notes:
>     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..21cc101d693f5adfd9d43f0c21a096eb59ba73b1 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,
> +    "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..cb82b55763a077f5994c4a00ea4893bfa2e07a79 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)'


Tested on SGI-575 platform and this helps to avoid a exception when
accessing SATA controller over PCI.

Tested-by: Thomas Abraham <thomas.abraham@arm.com>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Evan Lloyd 5 years, 9 months ago
Reviewed-by: Evan Lloyd <evan.lloyd@arm.com>

> -----Original Message-----
> From: Sami Mujawar <sami.mujawar@arm.com>
> Sent: 14 June 2018 12:38
> To: edk2-devel@lists.01.org
> Cc: star.zeng@intel.com; eric.dong@intel.com; ruiyu.ni@intel.com;
> ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Matteo Carlini
> <Matteo.Carlini@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-
> Fitt@arm.com>; Evan Lloyd <Evan.Lloyd@arm.com>; Thomas Abraham
> <thomas.abraham@arm.com>; nd <nd@arm.com>
> Subject: [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
> 
> The SATA controller driver crashes while accessing the PCI memory, 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 seen at
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
> ix_v1
> 
> Notes:
>     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..21cc101d693f5adfd9d43f0c2
> 1a096eb59ba73b1 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,
> +    "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..cb82b55763a077f5994c4a00e
> a4893bfa2e07a79 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)'
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Zeng, Star 5 years, 9 months ago
Generally, the patch is good to me.
Some comments below.

On 2018/6/14 19:38, Sami Mujawar wrote:
> The SATA controller driver crashes while accessing the
> PCI memory, as the PCI memory space is not enabled.

The code "accessing the PCI memory" you mentioned here is
the AhciReadReg in the following code block, right?

> 
> 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v1
> 
> Notes:
>      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..21cc101d693f5adfd9d43f0c21a096eb59ba73b1 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;
> +  }

Good to me.

> +
> +  DEBUG ((
> +    EFI_D_INFO,
> +    "PCI Attributes = 0x%llx\n",

How about using "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;
> +    }

It is the case for enabling memory space, but there may be case to need 
IO space enabling. I suggest to use the code block (same with other 
device drivers).

   Status = PciIo->Attributes (
                     PciIo,
                     EfiPciIoAttributeOperationSupported,
                     0,
                     &Supports
                     );
   if (!EFI_ERROR (Status)) {
     Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
     Status = PciIo->Attributes (
                       PciIo,
                       EfiPciIoAttributeOperationEnable,
                       Supports,
                       NULL
                       );
   }

> +    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

Prefer to use

//
// 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
Prefer to use

//
// 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..cb82b55763a077f5994c4a00ea4893bfa2e07a79 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

Prefer to use

//
// Track the state so that the PCI attributes that were modified
// can be restored to the original value later
//

to align with others.

> +  BOOLEAN                           PciAttributesChanged;
> +  /// Copy of the original PCI Attributes

Prefer to use

//
// Copy of the original PCI Attributes
//

to align with others.

> +  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)
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Sami Mujawar 5 years, 9 months ago
Hi Zeng,

Please find my response marked [SAMI] below.

Regards,

Sami Mujawar

-----Original Message-----
From: Zeng, Star <star.zeng@intel.com> 
Sent: 15 June 2018 10:42 AM
To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
Cc: ruiyu.ni@intel.com; nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; star.zeng@intel.com
Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space

Generally, the patch is good to me.
Some comments below.

On 2018/6/14 19:38, Sami Mujawar wrote:
> The SATA controller driver crashes while accessing the PCI memory, as 
> the PCI memory space is not enabled.

The code "accessing the PCI memory" you mentioned here is the AhciReadReg in the following code block, right?
[SAMI] Yes.
> 
> 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 seen at 
> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fi
> x_v1
> 
> Notes:
>      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..21cc101d693f5adfd9d43f0c21a0
> 96eb59ba73b1 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;
> +  }

Good to me.

> +
> +  DEBUG ((
> +    EFI_D_INFO,
> +    "PCI Attributes = 0x%llx\n",

How about using "Original PCI Attributes = 0x%llx\n"?
[SAMI] I will submit a patch with this fixed.

> +    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;
> +    }

It is the case for enabling memory space, but there may be case to need IO space enabling. I suggest to use the code block (same with other device drivers).

   Status = PciIo->Attributes (
                     PciIo,
                     EfiPciIoAttributeOperationSupported,
                     0,
                     &Supports
                     );
   if (!EFI_ERROR (Status)) {
     Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
     Status = PciIo->Attributes (
                       PciIo,
                       EfiPciIoAttributeOperationEnable,
                       Supports,
                       NULL
                       );
   }

[SAMI] 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.

> +    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

Prefer to use

//
// Restore original PCI attributes
//

[SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

> +        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
Prefer to use

//
// Restore original PCI attributes
//

[SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

> +      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..cb82b55763a077f5994c4a00ea48
> 93bfa2e07a79 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

Prefer to use

//
// Track the state so that the PCI attributes that were modified // can be restored to the original value later //

to align with others.
[SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3. 
             Also '///' is used for documenting structure members. See EDK2 Coding Standard Specification, revision 2.20, section 6.9.
             I will submit an updated patch with '.' at the end to indicate where the comment block ends.

> +  BOOLEAN                           PciAttributesChanged;
> +  /// Copy of the original PCI Attributes

Prefer to use

//
// Copy of the original PCI Attributes
//

to align with others.
[SAMI] This would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

> +  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)
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Zeng, Star 5 years, 9 months ago
Hi Sami,

My feedback are inline.

On 2018/6/15 21:51, Sami Mujawar wrote:
> Hi Zeng,
> 
> Please find my response marked [SAMI] below.
> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: 15 June 2018 10:42 AM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
> Cc: ruiyu.ni@intel.com; nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; star.zeng@intel.com
> Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
> 
> Generally, the patch is good to me.
> Some comments below.
> 
> On 2018/6/14 19:38, Sami Mujawar wrote:
>> The SATA controller driver crashes while accessing the PCI memory, as
>> the PCI memory space is not enabled.
> 
> The code "accessing the PCI memory" you mentioned here is the AhciReadReg in the following code block, right?
> [SAMI] Yes.
>>
>> 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 seen at
>> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fi
>> x_v1
>>
>> Notes:
>>       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..21cc101d693f5adfd9d43f0c21a0
>> 96eb59ba73b1 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;
>> +  }
> 
> Good to me.
> 
>> +
>> +  DEBUG ((
>> +    EFI_D_INFO,
>> +    "PCI Attributes = 0x%llx\n",
> 
> How about using "Original PCI Attributes = 0x%llx\n"?
> [SAMI] I will submit a patch with this fixed.
> 
>> +    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;
>> +    }
> 
> It is the case for enabling memory space, but there may be case to need IO space enabling. I suggest to use the code block (same with other device drivers).
> 
>     Status = PciIo->Attributes (
>                       PciIo,
>                       EfiPciIoAttributeOperationSupported,
>                       0,
>                       &Supports
>                       );
>     if (!EFI_ERROR (Status)) {
>       Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
>       Status = PciIo->Attributes (
>                         PciIo,
>                         EfiPciIoAttributeOperationEnable,
>                         Supports,
>                         NULL
>                         );
>     }
> 
> [SAMI] 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.

Yes, I did know AHCI has ABAR with memory space. But this SATA 
Controller driver also supports IDE mode that uses IO space.
So I suggested to handle them both. The attributes has been reflected in 
Supports, the code can naturally enable the supported attributes.

> 
>> +    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
> 
> Prefer to use
> 
> //
> // Restore original PCI attributes
> //
> 
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Before we have clear direction about this to align code and CCS spec. I 
prefer to align with the surrounding code to use below style.
//
// xxxxx
//

> 
>> +        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
> Prefer to use
> 
> //
> // Restore original PCI attributes
> //
> 
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Same with above.

> 
>> +      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..cb82b55763a077f5994c4a00ea48
>> 93bfa2e07a79 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
> 
> Prefer to use
> 
> //
> // Track the state so that the PCI attributes that were modified // can be restored to the original value later //
> 
> to align with others.
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.
>               Also '///' is used for documenting structure members. See EDK2 Coding Standard Specification, revision 2.20, section 6.9.
>               I will submit an updated patch with '.' at the end to indicate where the comment block ends.

I was not saying using '///' is wrong here. As I know, '///' are mainly 
used in *public* header files. For this case, I prefer to align with the 
surrounding code to use below style.
//
// xxxxx
//

> 
>> +  BOOLEAN                           PciAttributesChanged;
>> +  /// Copy of the original PCI Attributes
> 
> Prefer to use
> 
> //
> // Copy of the original PCI Attributes
> //
> 
> to align with others.
> [SAMI] This would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Same with above.


Thanks,
Star

> 
>> +  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)
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space
Posted by Sami Mujawar 5 years, 9 months ago
Hi Star,

Thank you for your feedback. I will submit a new patch with the issues addressed.
Kindly ignore the v2 patch.

Regards,

Sami Mujawar

-----Original Message-----
From: Zeng, Star <star.zeng@intel.com> 
Sent: 19 June 2018 03:53 AM
To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
Cc: ruiyu.ni@intel.com; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; eric.dong@intel.com; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; nd <nd@arm.com>; star.zeng@intel.com
Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller PCI mem space

Hi Sami,

My feedback are inline.

On 2018/6/15 21:51, Sami Mujawar wrote:
> Hi Zeng,
> 
> Please find my response marked [SAMI] below.
> 
> Regards,
> 
> Sami Mujawar
> 
> -----Original Message-----
> From: Zeng, Star <star.zeng@intel.com>
> Sent: 15 June 2018 10:42 AM
> To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org
> Cc: ruiyu.ni@intel.com; nd <nd@arm.com>; Stephanie Hughes-Fitt 
> <Stephanie.Hughes-Fitt@arm.com>; eric.dong@intel.com; 
> ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; 
> star.zeng@intel.com
> Subject: Re: [edk2] [PATCH v1] MdeModulePkg: Enable SATA Controller 
> PCI mem space
> 
> Generally, the patch is good to me.
> Some comments below.
> 
> On 2018/6/14 19:38, Sami Mujawar wrote:
>> The SATA controller driver crashes while accessing the PCI memory, as 
>> the PCI memory space is not enabled.
> 
> The code "accessing the PCI memory" you mentioned here is the AhciReadReg in the following code block, right?
> [SAMI] Yes.
>>
>> 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 seen at
>> https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_f
>> i
>> x_v1
>>
>> Notes:
>>       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..21cc101d693f5adfd9d43f0c21a
>> 0
>> 96eb59ba73b1 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;
>> +  }
> 
> Good to me.
> 
>> +
>> +  DEBUG ((
>> +    EFI_D_INFO,
>> +    "PCI Attributes = 0x%llx\n",
> 
> How about using "Original PCI Attributes = 0x%llx\n"?
> [SAMI] I will submit a patch with this fixed.
> 
>> +    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;
>> +    }
> 
> It is the case for enabling memory space, but there may be case to need IO space enabling. I suggest to use the code block (same with other device drivers).
> 
>     Status = PciIo->Attributes (
>                       PciIo,
>                       EfiPciIoAttributeOperationSupported,
>                       0,
>                       &Supports
>                       );
>     if (!EFI_ERROR (Status)) {
>       Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
>       Status = PciIo->Attributes (
>                         PciIo,
>                         EfiPciIoAttributeOperationEnable,
>                         Supports,
>                         NULL
>                         );
>     }
> 
> [SAMI] 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.

Yes, I did know AHCI has ABAR with memory space. But this SATA Controller driver also supports IDE mode that uses IO space.
So I suggested to handle them both. The attributes has been reflected in Supports, the code can naturally enable the supported attributes.

> 
>> +    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
> 
> Prefer to use
> 
> //
> // Restore original PCI attributes
> //
> 
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Before we have clear direction about this to align code and CCS spec. I prefer to align with the surrounding code to use below style.
//
// xxxxx
//

> 
>> +        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
> Prefer to use
> 
> //
> // Restore original PCI attributes
> //
> 
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Same with above.

> 
>> +      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..cb82b55763a077f5994c4a00ea4
>> 8
>> 93bfa2e07a79 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
> 
> Prefer to use
> 
> //
> // Track the state so that the PCI attributes that were modified // 
> can be restored to the original value later //
> 
> to align with others.
> [SAMI] Doing this would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.
>               Also '///' is used for documenting structure members. See EDK2 Coding Standard Specification, revision 2.20, section 6.9.
>               I will submit an updated patch with '.' at the end to indicate where the comment block ends.

I was not saying using '///' is wrong here. As I know, '///' are mainly used in *public* header files. For this case, I prefer to align with the surrounding code to use below style.
//
// xxxxx
//

> 
>> +  BOOLEAN                           PciAttributesChanged;
>> +  /// Copy of the original PCI Attributes
> 
> Prefer to use
> 
> //
> // Copy of the original PCI Attributes //
> 
> to align with others.
> [SAMI] This would violate the edk2 coding standard. See EDK2 Coding Standard Specification, revision 2.20, section 6.2.3.

Same with above.


Thanks,
Star

> 
>> +  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)
>>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel