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

[Evan Lloyd <Evan.Lloyd@arm.com>]

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