MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c | 75 +++++++++++++++++++- MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 11 +++ 2 files changed, 85 insertions(+), 1 deletion(-)
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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v3
Notes:
v3:
- Integrated suggested changes. [SAMI]
v2:
- Improved log message and code documentation based on feedback [SAMI]
- Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE [STAR]
- 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 [STAR]
- 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 | 75 +++++++++++++++++++-
MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 11 +++
2 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c
index a6d55c15571728eb3fd572003f383ba7c86635ae..87c201dabdcf14fa228c0b3577fbbead2ec9b6bd 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 Supports;
DEBUG ((EFI_D_INFO, "SataControllerStart start\n"));
@@ -406,6 +408,52 @@ 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
+ ));
+
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationSupported,
+ 0,
+ &Supports
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", Supports));
+
+ Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE;
+ Status = PciIo->Attributes (
+ PciIo,
+ EfiPciIoAttributeOperationEnable,
+ Supports,
+ NULL
+ );
+ if (EFI_ERROR (Status)) {
+ goto Done;
+ }
+
+ DEBUG ((EFI_D_INFO, "Enabled PCI Attributes = 0x%llx\n", Supports));
+ Private->PciAttributesChanged = TRUE;
Status = PciIo->Pci.Read (
PciIo,
@@ -414,7 +462,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 +532,17 @@ 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 +618,17 @@ 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..26c44034f6cdf0d9a3e1abee14fe316f6158d854 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) 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
@@ -104,6 +105,16 @@ 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
With + // + // Track the state so that the PCI attributes that were modified + // can be restored to the original value later. Updated to + // + // Track the state so that the PCI attributes that were modified + // can be restored to the original value later. + // Reviewed-by: Star Zeng <star.zeng@intel.com> If you agree, you do not need resend new patch. I will help update it simply and push the patch. Thanks, Star -----Original Message----- From: Sami Mujawar [mailto:sami.mujawar@arm.com] Sent: Tuesday, June 19, 2018 7:58 PM To: edk2-devel@lists.01.org Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; evan.lloyd@arm.com; thomas.abraham@arm.com; nd@arm.com Subject: [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v3 Notes: v3: - Integrated suggested changes. [SAMI] v2: - Improved log message and code documentation based on feedback [SAMI] - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE [STAR] - 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 [STAR] - 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 | 75 +++++++++++++++++++- MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 11 +++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c index a6d55c15571728eb3fd572003f383ba7c86635ae..87c201dabdcf14fa228c0b3577fbbead2ec9b6bd 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 Supports; DEBUG ((EFI_D_INFO, "SataControllerStart start\n")); @@ -406,6 +408,52 @@ 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 + )); + + Status = PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSupported, + 0, + &Supports + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", Supports)); + + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; + Status = PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationEnable, + Supports, + NULL + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Enabled PCI Attributes = 0x%llx\n", Supports)); + Private->PciAttributesChanged = TRUE; Status = PciIo->Pci.Read ( PciIo, @@ -414,7 +462,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 +532,17 @@ 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 +618,17 @@ 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..26c44034f6cdf0d9a3e1abee14fe316f6158d854 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) 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 @@ -104,6 +105,16 @@ 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
Hi Star, I agree with this change. Regards, Sami Mujawar -----Original Message----- From: Zeng, Star <star.zeng@intel.com> Sent: 20 June 2018 05:42 AM To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <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>; Zeng, Star <star.zeng@intel.com> Subject: RE: [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space With + // + // Track the state so that the PCI attributes that were modified // + can be restored to the original value later. Updated to + // + // Track the state so that the PCI attributes that were modified // + can be restored to the original value later. + // Reviewed-by: Star Zeng <star.zeng@intel.com> If you agree, you do not need resend new patch. I will help update it simply and push the patch. Thanks, Star -----Original Message----- From: Sami Mujawar [mailto:sami.mujawar@arm.com] Sent: Tuesday, June 19, 2018 7:58 PM To: edk2-devel@lists.01.org Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; evan.lloyd@arm.com; thomas.abraham@arm.com; nd@arm.com Subject: [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v3 Notes: v3: - Integrated suggested changes. [SAMI] v2: - Improved log message and code documentation based on feedback [SAMI] - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE [STAR] - 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 [STAR] - 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 | 75 +++++++++++++++++++- MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 11 +++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c index a6d55c15571728eb3fd572003f383ba7c86635ae..87c201dabdcf14fa228c0b3577fbbead2ec9b6bd 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 Supports; DEBUG ((EFI_D_INFO, "SataControllerStart start\n")); @@ -406,6 +408,52 @@ 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 + )); + + Status = PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSupported, + 0, + &Supports + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", + Supports)); + + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; Status = + PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationEnable, + Supports, + NULL + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Enabled PCI Attributes = 0x%llx\n", Supports)); + Private->PciAttributesChanged = TRUE; Status = PciIo->Pci.Read ( PciIo, @@ -414,7 +462,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 +532,17 @@ 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 +618,17 @@ 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..26c44034f6cdf0d9a3e1abee14fe316f6158d854 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) 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 @@ -104,6 +105,16 @@ 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
Pushed at 24fee0528c32b240720547afdd737ca928b34e60. Thanks, Star -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Sami Mujawar Sent: Wednesday, June 20, 2018 4:23 PM To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; nd <nd@arm.com>; Stephanie Hughes-Fitt <Stephanie.Hughes-Fitt@arm.com>; Dong, Eric <eric.dong@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org Subject: Re: [edk2] [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space Hi Star, I agree with this change. Regards, Sami Mujawar -----Original Message----- From: Zeng, Star <star.zeng@intel.com> Sent: 20 June 2018 05:42 AM To: Sami Mujawar <Sami.Mujawar@arm.com>; edk2-devel@lists.01.org Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <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>; Zeng, Star <star.zeng@intel.com> Subject: RE: [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space With + // + // Track the state so that the PCI attributes that were modified // + can be restored to the original value later. Updated to + // + // Track the state so that the PCI attributes that were modified // + can be restored to the original value later. + // Reviewed-by: Star Zeng <star.zeng@intel.com> If you agree, you do not need resend new patch. I will help update it simply and push the patch. Thanks, Star -----Original Message----- From: Sami Mujawar [mailto:sami.mujawar@arm.com] Sent: Tuesday, June 19, 2018 7:58 PM To: edk2-devel@lists.01.org Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; ard.biesheuvel@linaro.org; leif.lindholm@linaro.org; Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; evan.lloyd@arm.com; thomas.abraham@arm.com; nd@arm.com Subject: [PATCH v3] MdeModulePkg: Enable SATA Controller PCI mem space 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 seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_v3 Notes: v3: - Integrated suggested changes. [SAMI] v2: - Improved log message and code documentation based on feedback [SAMI] - Enable IO space, suggestion to use EFI_PCI_DEVICE_ENABLE [STAR] - 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 [STAR] - 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 | 75 +++++++++++++++++++- MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h | 11 +++ 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c b/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.c index a6d55c15571728eb3fd572003f383ba7c86635ae..87c201dabdcf14fa228c0b3577fbbead2ec9b6bd 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 Supports; DEBUG ((EFI_D_INFO, "SataControllerStart start\n")); @@ -406,6 +408,52 @@ 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 + )); + + Status = PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSupported, + 0, + &Supports + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Supported PCI Attributes = 0x%llx\n", + Supports)); + + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; Status = + PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationEnable, + Supports, + NULL + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Enabled PCI Attributes = 0x%llx\n", Supports)); + Private->PciAttributesChanged = TRUE; Status = PciIo->Pci.Read ( PciIo, @@ -414,7 +462,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 +532,17 @@ 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 +618,17 @@ 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..26c44034f6cdf0d9a3e1abee14fe316f6158d854 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) 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 @@ -104,6 +105,16 @@ 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.