From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.151; helo=mga17.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E6C5320337342 for ; Mon, 18 Jun 2018 19:26:16 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga007.fm.intel.com ([10.253.24.52]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 18 Jun 2018 19:26:16 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,241,1526367600"; d="scan'208";a="48103248" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga007.fm.intel.com with ESMTP; 18 Jun 2018 19:26:13 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Jun 2018 19:26:13 -0700 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Mon, 18 Jun 2018 19:26:12 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.223]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.82]) with mapi id 14.03.0319.002; Tue, 19 Jun 2018 10:26:10 +0800 From: "Zeng, Star" To: Sami Mujawar , "edk2-devel@lists.01.org" CC: "Dong, Eric" , "Ni, Ruiyu" , "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" , "Zeng, Star" Thread-Topic: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space Thread-Index: AQHUBLMb5UcT4Fv0TUmfHpcJbBPn+KRm3yrg Date: Tue, 19 Jun 2018 02:26:10 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103BB590B8@shsmsx102.ccr.corp.intel.com> References: <20180615141348.13020-1-sami.mujawar@arm.com> In-Reply-To: <20180615141348.13020-1-sami.mujawar@arm.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 19 Jun 2018 02:26:17 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Sami, I will have feedback in the V1 patch thread based on your response there. Thanks, Star -----Original Message----- From: Sami Mujawar [mailto:sami.mujawar@arm.com]=20 Sent: Friday, June 15, 2018 10:14 PM To: edk2-devel@lists.01.org Cc: Zeng, Star ; Dong, Eric ; Ni,= Ruiyu ; ard.biesheuvel@linaro.org; leif.lindholm@linar= o.org; Matteo.Carlini@arm.com; Stephanie.Hughes-Fitt@arm.com; evan.lloyd@ar= m.com; thomas.abraham@arm.com; nd@arm.com Subject: [PATCH v2] MdeModulePkg: Enable SATA Controller PCI mem space The SATA controller driver crashes while accessing the PCI memory [AHCI Bas= e Registers (ABAR)], as the PCI memory space is not enabled. Enable the PCI memory space access to prevent the SATA Controller driver fr= om crashing. Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Sami Mujawar --- The changes can be viewed at https://github.com/samimujawar/edk2/tree/284_s= ata_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= ] =20 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/MdeM= odulePkg/Bus/Pci/SataControllerDxe/SataController.c index a6d55c15571728eb3fd572003f383ba7c86635ae..faf1b62e81000449e43e5298bb8= ff885e48e4318 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 Contro= llers. =20 Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+ Copyright (c) 2018, ARM Ltd. All rights reserved.
This program and the accompanying materials are licensed and made available under the terms and conditions of the BS= D License which accompanies this distribution. The full text of the license may b= e found at @@ -364,6 +365,7 @@ SataControllerStart ( EFI_SATA_CONTROLLER_PRIVATE_DATA *Private; UINT32 Data32; UINTN TotalCount; + UINT64 PciAttributes; =20 DEBUG ((EFI_D_INFO, "SataControllerStart start\n")); =20 @@ -406,6 +408,61 @@ SataControllerStart ( Private->IdeInit.CalculateMode =3D IdeInitCalculateMode; Private->IdeInit.SetTiming =3D IdeInitSetTiming; Private->IdeInit.EnumAll =3D SATA_ENUMER_ALL; + Private->PciAttributesChanged =3D FALSE; + + // Save original PCI attributes + Status =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationGet, + 0, + &Private->OriginalPciAttributes + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG (( + EFI_D_INFO, + "Original PCI Attributes =3D 0x%llx\n", + Private->OriginalPciAttributes + )); + + if ((Private->OriginalPciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) =3D= =3D 0) { + Status =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationSupported, + 0, + &PciAttributes + ); + if (EFI_ERROR (Status)) { + goto Done; + } + + DEBUG ((EFI_D_INFO, "Supported PCI Attributes =3D 0x%llx\n",=20 + PciAttributes)); + + if ((PciAttributes & EFI_PCI_IO_ATTRIBUTE_MEMORY) =3D=3D 0) { + DEBUG (( + EFI_D_ERROR, + "Error: EFI_PCI_IO_ATTRIBUTE_MEMORY not supported\n" + )); + Status =3D EFI_UNSUPPORTED; + goto Done; + } + + PciAttributes =3D Private->OriginalPciAttributes |=20 + EFI_PCI_IO_ATTRIBUTE_MEMORY; + + DEBUG ((EFI_D_INFO, "Enable PCI Attributes =3D 0x%llx\n", PciAttribute= s)); + Status =3D PciIo->Attributes ( + PciIo, + EfiPciIoAttributeOperationEnable, + PciAttributes, + NULL + ); + if (EFI_ERROR (Status)) { + goto Done; + } + Private->PciAttributesChanged =3D TRUE; } =20 Status =3D 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; + } =20 if (IS_PCI_IDE (&PciData)) { Private->IdeInit.ChannelCount =3D IDE_MAX_CHANNEL; @@ -481,6 +541,15 @= @ Done: if (Private->IdentifyValid !=3D 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 !=3D NULL) { FreePool (Private->IdentifyValid); } + if (Private->PciAttributesChanged) { + // Restore original PCI attributes + Private->PciIo->Attributes ( + Private->PciIo, + EfiPciIoAttributeOperationSet, + Private->OriginalPciAttributes, + NULL + ); + } FreePool (Private); } =20 diff --git a/MdeModulePkg/Bus/Pci/SataControllerDxe/SataController.h b/MdeM= odulePkg/Bus/Pci/SataControllerDxe/SataController.h index f7db3b832a14c0c8314518cfdf4198c7a4e8ef25..f927c1f02713771f92e3b760128= 48206321391f0 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. =20 Copyright (c) 2011 - 2016, Intel Corporation. All rights reserved.
+ Copyright (c) 2017, ARM Ltd. All rights reserved.
This program and the accompanying materials are licensed and made available under the terms and conditions of the BS= D License which accompanies this distribution. The full text of the license may b= e 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 =20 + /// can be restored to the original value later. + BOOLEAN PciAttributesChanged; + /// Copy of the original PCI Attributes + UINT64 OriginalPciAttributes; } EFI_SATA_CONTROLLER_PRIVATE_DATA; =20 #define SATA_CONTROLLER_PRIVATE_DATA_FROM_THIS(a) CR(a, EFI_SATA_CONTROLLE= R_PRIVATE_DATA, IdeInit, SATA_CONTROLLER_SIGNATURE) -- 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)'