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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 15 June 2018 at 16:13, Sami Mujawar <sami.mujawar@arm.com> wrote:
> 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]
>

Please stop obsessing about the coding standard. The maintainer was
quite clear what he wanted, and in the past, I have also indicated
that for the ARM related packages, I prefer readability and
consistency over adherence to a dubious standard.


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