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

["Zeng, Star" <star.zeng@intel.com>]

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