From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::244; helo=mail-io0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x244.google.com (mail-io0-x244.google.com [IPv6:2607:f8b0:4001:c06::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A5581210D226F for ; Fri, 15 Jun 2018 07:16:42 -0700 (PDT) Received: by mail-io0-x244.google.com with SMTP id l25-v6so10801571ioh.12 for ; Fri, 15 Jun 2018 07:16:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=NIrLP+WFNsihFjtP8fSIznKnrtJqna8nn488205wW1s=; b=Q6faDiE/U8Et0UHOXBhj/Y/+W4uSxH9y9wVBKdEbDzku5wRhUA5l/6vSEwzUpEJ/xi 0yPDmR3qzZfjbXcnL/Yq7fAI6oHgU0yAvTmcvQAvijGHNhkQZDnrWQ1uabjnAqsxsuR+ 04w12Kod2blAcjepC98x4TUzbRgT/7xuzxN0I= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=NIrLP+WFNsihFjtP8fSIznKnrtJqna8nn488205wW1s=; b=YHfjx3RdS3z+8fxML44rAgr/O6upqOmk836aMDPa+sRj+E7igyT2U46P15jIKKNCsI qe9aaNVuudcpe5Wpam9GQN6EldWSxnaG03SuYZKQGk4RQqJ9t6IuSPQiHH3CQcFmVSAt vAZLak1ar5xOLWmNSRj2Vk+cmtaWUnKcfa+XJPdphOgKZ0N+k5gUMauYXNChA9alx2MU y59tmYYJXv9jxBLv+hNJJIYz3pPrGz4JHqu4UzN2rpQxNfReVWaxMZDuAaAS7wKCec86 fc22bM/Z7rKG2YWq5oowrUF/fpyaAes7i8XidYW1vPTe3+jasIstaVORde/NV/89a/es X9hA== X-Gm-Message-State: APt69E0ZBAc46iO3zwWXaDLCOgLsiCCsEUl7D1CD3Pzh0bx2Dw39y6jL fAWANOWKoTKRfZH3F090CUhRuWK7u3yXb7Gq2mVi1A== X-Google-Smtp-Source: ADUXVKLoD1eOEYOvn6br3qIvUq2FwBK+O4XRw3nwfGqYHZngfaIHqfmcLfMTcjDEltEKviwgCWYdBlqzDYg7BT9Qw1g= X-Received: by 2002:a6b:dd0b:: with SMTP id f11-v6mr1630094ioc.173.1529072201651; Fri, 15 Jun 2018 07:16:41 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Fri, 15 Jun 2018 07:16:40 -0700 (PDT) In-Reply-To: <20180615141348.13020-1-sami.mujawar@arm.com> References: <20180615141348.13020-1-sami.mujawar@arm.com> From: Ard Biesheuvel Date: Fri, 15 Jun 2018 16:16:40 +0200 Message-ID: To: Sami Mujawar Cc: "edk2-devel@lists.01.org" , "Zeng, Star" , Eric Dong , Ruiyu Ni , Leif Lindholm , Matteo Carlini , Stephanie Hughes-Fitt , Evan Lloyd , Thomas Panakamattam Abraham , nd 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: Fri, 15 Jun 2018 14:16:43 -0000 Content-Type: text/plain; charset="UTF-8" On 15 June 2018 at 16:13, Sami Mujawar 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 > --- > 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.
> + 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 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.
> + 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 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)' > >