From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=thomas.abraham@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id 549E6210C125F for ; Thu, 14 Jun 2018 05:26:51 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2391315BF for ; Thu, 14 Jun 2018 05:26:51 -0700 (PDT) Received: from mail-io0-f181.google.com (usa-sjc-mx-foss1.foss.arm.com [217.140.101.70]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0C1FD3F7BB for ; Thu, 14 Jun 2018 05:26:51 -0700 (PDT) Received: by mail-io0-f181.google.com with SMTP id t5-v6so6925339ioa.8 for ; Thu, 14 Jun 2018 05:26:51 -0700 (PDT) X-Gm-Message-State: APt69E1cSyzuJQcSXx4KPxE99q5qWcphlaRIqBxwbHtQHNGkWbLt1mJ+ QxApYS2Rh3qyGJ4/s2LHxz9mOIerrtnQ0sQ8ppg= X-Google-Smtp-Source: ADUXVKK4HPV8m1b0kTzmafUZiBbyO9JEVHm4bvXWr2Phak7rRF2DmAurk0PUBRMRefvg//uEZZEUXxNEmfqEraQeQ+Y= X-Received: by 2002:a5e:9407:: with SMTP id q7-v6mr2199173ioj.268.1528979210197; Thu, 14 Jun 2018 05:26:50 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a02:6c4e:0:0:0:0:0 with HTTP; Thu, 14 Jun 2018 05:26:49 -0700 (PDT) In-Reply-To: <20180614113820.11872-1-sami.mujawar@arm.com> References: <20180614113820.11872-1-sami.mujawar@arm.com> From: Thomas Abraham Date: Thu, 14 Jun 2018 17:56:49 +0530 X-Gmail-Original-Message-ID: Message-ID: To: Sami Mujawar Cc: edk2-devel@lists.01.org, ruiyu.ni@intel.com, nd@arm.com, Stephanie.Hughes-Fitt@arm.com, eric.dong@intel.com, Ard Biesheuvel , Leif Lindholm , star.zeng@intel.com Subject: Re: [PATCH v1] 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: Thu, 14 Jun 2018 12:26:52 -0000 Content-Type: text/plain; charset="UTF-8" On Thu, Jun 14, 2018 at 5:08 PM, Sami Mujawar wrote: > 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 > --- > The changes can be seen at https://github.com/samimujawar/edk2/tree/284_sata_controler_pci_mem_fix_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..21cc101d693f5adfd9d43f0c21a096eb59ba73b1 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, > + "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..cb82b55763a077f5994c4a00ea4893bfa2e07a79 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)' Tested on SGI-575 platform and this helps to avoid a exception when accessing SATA controller over PCI. Tested-by: Thomas Abraham