From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:400e:c01::233; helo=mail-pl0-x233.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pl0-x233.google.com (mail-pl0-x233.google.com [IPv6:2607:f8b0:400e:c01::233]) (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 EA39D20954B8E for ; Wed, 14 Mar 2018 19:51:26 -0700 (PDT) Received: by mail-pl0-x233.google.com with SMTP id c11-v6so2919771plo.0 for ; Wed, 14 Mar 2018 19:57:50 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:date:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=st8IUGLI51BNryIdf/BzuqEK1yq7eMGeBXF+Ho0W9xw=; b=KrjtqBTRzcTCE0/C8gl0xtp7pFhtkL0qkVf8lgXfti6TYDUOvxKUQnaxs4ju+pZY43 VHhKDSkg/Me3pclNQpecjuCwXqfwb4R0kroyUKOWi65N+cccmAabFgKigUC9SKrjg0h8 /nF2tG0mV2Bmcd7gUBKw/JhDp/iwohmDSkuwo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:date:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=st8IUGLI51BNryIdf/BzuqEK1yq7eMGeBXF+Ho0W9xw=; b=Zxxf6BoV8Vk8hSqfoROZXssYrOQjea/Wx9ZOy9xX0l5PphLib+/yXqbmb/Jb+l/4dR 4jpRTc/NyqHKVmmpooY7pERd5i+Rb20gvMYtQfcNuGkLNYFJZfBDlgNM8j6mZE1q7Co8 O76xtOuWmVUl+xOcu7caxQHUd3QXBZVWfGgAj537XY6udQe19OdV35sUn04xLcPfrRAw NLyukDuRfWpjh/WLCgNeZVZrmS7ktqCbG8K+aFfpvliH3KnPfvMT+8rRzFX+/FQoMyWu aDHZiP8zeHMtMOh5tDodBrbhxK4eghKTUchdjWKioeLDSp0w8DhIdvTRQq0fEoPFRlIm FI5Q== X-Gm-Message-State: AElRT7HJd0zPZGnRu0G3a10f60du/SC8SLvcHwxNllGULIUhVwHekRAI BEc6iq9wzNLUHg3OnIqJObSprQ== X-Google-Smtp-Source: AG47ELv2OsVIo8LLB7gd9F2wuq0G663byAmXzKl0+6eDJuufWbOe62EKrX6WLsDSfMO+5ULCxw9Wqw== X-Received: by 2002:a17:902:a705:: with SMTP id w5-v6mr6412156plq.299.1521082669762; Wed, 14 Mar 2018 19:57:49 -0700 (PDT) Received: from SZX1000114654 ([45.56.152.100]) by smtp.gmail.com with ESMTPSA id a6sm7008514pfi.22.2018.03.14.19.57.46 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 14 Mar 2018 19:57:48 -0700 (PDT) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 15 Mar 2018 10:57:44 +0800 To: "Ni, Ruiyu" Cc: Guo Heyi , Eric Dong , Ard Biesheuvel , edk2-devel@lists.01.org, Michael D Kinney , Laszlo Ersek , Star Zeng Message-ID: <20180315025744.GA108227@SZX1000114654> References: <1519887444-75510-1-git-send-email-heyi.guo@linaro.org> <1519887444-75510-5-git-send-email-heyi.guo@linaro.org> <20180306024440.GA90780@SZX1000114654> <71a21699-a1b3-155a-e743-ee5e6288ce20@Intel.com> <20180307060143.GB91936@SZX1000114654> <9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@Intel.com> MIME-Version: 1.0 In-Reply-To: <9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [PATCH v5 4/6] MdeModulePkg/PciHostBridgeDxe: Add support for address translation X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 15 Mar 2018 02:51:27 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Ray, Sorry I never tried Ecc tool before, for there is few documents about it. When I tried running python BaseTools/Source/Python/Ecc/Ecc.py -t -s xxxx, it showed me with error message: ImportError: No module named Common.LongFilePathOs Then I found there was an executable named "Ecc" in BaseTools/BinWrappers/PosixLike/, and change the place of "-t" and "-s" after it prompted: : error 1003: Invalid value of option Target [-s] does NOT exist However, it still failed with error: RuntimeError: ANTLR version mismatch: The recognizer has been generated with API V0, but this runtime does not support this. I could not find the reason for this error, maybe it was caused by incompatible version of python and antlr3 (mine is Ubuntu 16.04.3 LTS, python 2.7.12, python-antlr3 3.5.2-1). Finally I downloaded prebuilt Win32 BaseTools and ran Ecc.exe on Windows. It success and produced a "Report.csv" with only a titel line. I guess this means there is no error in the coding style, isn't it? Please let me know if there is anything wrong with what I did. Thanks and regards, Heyi On Wed, Mar 14, 2018 at 03:57:14PM +0800, Ni, Ruiyu wrote: > On 3/7/2018 2:01 PM, Guo Heyi wrote: > >Thanks. Please let me know if any further changes are needed. > > > >Regards, > > > >Heyi > > > >On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote: > >>On 3/6/2018 10:44 AM, Guo Heyi wrote: > >>>Hi Ray, > >>> > >>>Any comments for v5? > >> > >>Heyi, > >>Some backward compatibility concerns were received from internal production > >>teams. Current change will cause silent failure on old platforms because > >>TranslationOffset might be random if uninitialized. > >>I will solve the concern and then send out updates to you, hopefully by end > >>of next week. > >> > >>> > >>>Regards, > >>> > >>>Heyi > >>> > >>>On Thu, Mar 01, 2018 at 02:57:22PM +0800, Heyi Guo wrote: > >>>>PCI address translation is necessary for some non-x86 platforms. On > >>>>such platforms, address value (denoted as "device address" or "address > >>>>in PCI view") set to PCI BAR registers in configuration space might be > >>>>different from the address which is used by CPU to access the > >>>>registers in memory BAR or IO BAR spaces (denoted as "host address" or > >>>>"address in CPU view"). The difference between the two addresses is > >>>>called "Address Translation Offset" or simply "translation", and can > >>>>be represented by "Address Translation Offset" in ACPI QWORD Address > >>>>Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the > >>>>definitions of QWORD Address Space Descriptor, and we will follow UEFI > >>>>definition on UEFI protocols, such as PCI root bridge IO protocol and > >>>>PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset > >>>>to apply to the Starting address to convert it to a PCI address". This > >>>>means: > >>>> > >>>>1. Translation = device address - host address. > >>>> > >>>>2. PciRootBridgeIo->Configuration should return CPU view address, as > >>>>well as PciIo->GetBarAttributes. > >>>> > >>>>Summary of addresses used in protocol interfaces and internal > >>>>implementations: > >>>> > >>>>1. *Only* the following protocol interfaces assume Address is Device > >>>> Address: > >>>>(1). PciHostBridgeResourceAllocation.GetProposedResources() > >>>> Otherwise PCI bus driver cannot set correct address into PCI > >>>> BARs. > >>>>(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write() > >>>>(3). PciRootBridgeIo.CopyMem() > >>>>UEFI and PI spec have clear statements for all other protocol > >>>>interfaces about the address type. > >>>> > >>>>2. Library interfaces and internal implementation: > >>>>(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>> It is easy to check whether the address is below 4G or above 4G. > >>>>(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host > >>>> address, for they are allocated from GCD. > >>>>(3). Address passed to PciHostBridgeResourceConflict is host address, > >>>> for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode. > >>>> > >>>>RESTRICTION: to simplify the situation, we require the alignment of > >>>>Translation must be larger than any BAR alignment in the same root > >>>>bridge, so that resource allocation alignment can be applied to both > >>>>device address and host address. > >>>> > >>>>Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>Signed-off-by: Heyi Guo > >>>>Cc: Ruiyu Ni > >>>>Cc: Ard Biesheuvel > >>>>Cc: Star Zeng > >>>>Cc: Eric Dong > >>>>Cc: Laszlo Ersek > >>>>Cc: Michael D Kinney > >>>>--- > >>>> > >>>>Notes: > >>>> v5: > >>>> - Add check for the alignment of Translation against the BAR alignment > >>>> [Ray] > >>>> - Keep coding style of comments consistent with the context [Ray] > >>>> - Comment change for Base in PCI_RES_NODE [Ray] > >>>> - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address type > >>>> conversion (After that we can also simply the comments about the > >>>> calculation [Ray] > >>>> - Add check for bus translation offset in CreateRootBridge(), making > >>>> sure it is zero, and unify code logic for all types of resource > >>>> after that [Ray] > >>>> - Use GetTranslationByResourceType() to simplify the code in > >>>> RootBridgeIoConfiguration() (also fix a bug in previous patch > >>>> version of missing a break after case TypePMem64) [Ray] > >>>> - Commit message refinement [Ray] > >>>> v4: > >>>> - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType > >>>> [Laszlo] > >>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and > >>>> gDS->AllocateMemorySpace [Laszlo] > >>>> - Add comment for applying alignment to both device address and host > >>>> address, and add NOTE for the alignment requirement of Translation, > >>>> as well as in commit message [Laszlo][Ray] > >>>> - Improve indention for the code in CreateRootBridge [Laszlo] > >>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE > >>>> definition [Laszlo] > >>>> - Ignore translation of bus in CreateRootBridge > >>>> v4: > >>>> - Add ASSERT (FALSE) to default branch in GetTranslationByResourceType > >>>> [Laszlo] > >>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and > >>>> gDS->AllocateMemorySpace [Laszlo] > >>>> - Add comment for applying alignment to both device address and host > >>>> address, and add NOTE for the alignment requirement of Translation, > >>>> as well as in commit message [Laszlo][Ray] > >>>> - Improve indention for the code in CreateRootBridge [Laszlo] > >>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE > >>>> definition [Laszlo] > >>>> - Ignore translation of bus in CreateRootBridge > >>>> > >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 21 ++++ > >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 3 + > >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 129 +++++++++++++++++--- > >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 118 ++++++++++++++++-- > >>>> 4 files changed, 245 insertions(+), 26 deletions(-) > >>>> > >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >>>>index 9a8ca21f4819..c2791ea5c2a4 100644 > >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h > >>>>@@ -38,6 +38,13 @@ typedef struct { > >>>> #define PCI_HOST_BRIDGE_FROM_THIS(a) CR (a, PCI_HOST_BRIDGE_INSTANCE, ResAlloc, PCI_HOST_BRIDGE_SIGNATURE) > >>>> // > >>>>+// Macros to translate device address to host address and vice versa. According > >>>>+// to UEFI 2.7, device address = host address + translation offset. > >>>>+// > >>>>+#define TO_HOST_ADDRESS(DeviceAddress,TranslationOffset) ((DeviceAddress) - (TranslationOffset)) > >>>>+#define TO_DEVICE_ADDRESS(HostAddress,TranslationOffset) ((HostAddress) + (TranslationOffset)) > >>>>+ > >>>>+// > >>>> // Driver Entry Point > >>>> // > >>>> /** > >>>>@@ -247,6 +254,20 @@ ResourceConflict ( > >>>> IN PCI_HOST_BRIDGE_INSTANCE *HostBridge > >>>> ); > >>>>+/** > >>>>+ This routine gets translation offset from a root bridge instance by resource type. > >>>>+ > >>>>+ @param RootBridge The Root Bridge Instance for the resources. > >>>>+ @param ResourceType The Resource Type of the translation offset. > >>>>+ > >>>>+ @retval The Translation Offset of the specified resource. > >>>>+**/ > >>>>+UINT64 > >>>>+GetTranslationByResourceType ( > >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, > >>>>+ IN PCI_RESOURCE_TYPE ResourceType > >>>>+ ); > >>>>+ > >>>> extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome; > >>>> extern EFI_CPU_IO2_PROTOCOL *mCpuIo; > >>>> #endif > >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>>index 8612c0c3251b..a6c3739db368 100644 > >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>>@@ -38,6 +38,9 @@ typedef enum { > >>>> typedef struct { > >>>> PCI_RESOURCE_TYPE Type; > >>>>+ // > >>>>+ // Base is a host address > >>>>+ // > >>>> UINT64 Base; > >>>> UINT64 Length; > >>>> UINT64 Alignment; > >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>>index 1494848c3e8c..42ded2855c71 100644 > >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>>@@ -33,6 +33,39 @@ EFI_EVENT mIoMmuEvent; > >>>> VOID *mIoMmuRegistration; > >>>> /** > >>>>+ This routine gets translation offset from a root bridge instance by resource type. > >>>>+ > >>>>+ @param RootBridge The Root Bridge Instance for the resources. > >>>>+ @param ResourceType The Resource Type of the translation offset. > >>>>+ > >>>>+ @retval The Translation Offset of the specified resource. > >>>>+**/ > >>>>+UINT64 > >>>>+GetTranslationByResourceType ( > >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, > >>>>+ IN PCI_RESOURCE_TYPE ResourceType > >>>>+ ) > >>>>+{ > >>>>+ switch (ResourceType) { > >>>>+ case TypeIo: > >>>>+ return RootBridge->Io.Translation; > >>>>+ case TypeMem32: > >>>>+ return RootBridge->Mem.Translation; > >>>>+ case TypePMem32: > >>>>+ return RootBridge->PMem.Translation; > >>>>+ case TypeMem64: > >>>>+ return RootBridge->MemAbove4G.Translation; > >>>>+ case TypePMem64: > >>>>+ return RootBridge->PMemAbove4G.Translation; > >>>>+ case TypeBus: > >>>>+ return RootBridge->Bus.Translation; > >>>>+ default: > >>>>+ ASSERT (FALSE); > >>>>+ return 0; > >>>>+ } > >>>>+} > >>>>+ > >>>>+/** > >>>> Ensure the compatibility of an IO space descriptor with the IO aperture. > >>>> The IO space descriptor can come from the GCD IO space map, or it can > >>>>@@ -366,6 +399,7 @@ InitializePciHostBridge ( > >>>> UINTN MemApertureIndex; > >>>> BOOLEAN ResourceAssigned; > >>>> LIST_ENTRY *Link; > >>>>+ UINT64 HostAddress; > >>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); > >>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) { > >>>>@@ -411,8 +445,15 @@ InitializePciHostBridge ( > >>>> } > >>>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) { > >>>>+ // > >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>>+ // For GCD resource manipulation, we need to use host address. > >>>>+ // > >>>>+ HostAddress = TO_HOST_ADDRESS (RootBridges[Index].Io.Base, > >>>>+ RootBridges[Index].Io.Translation); > >>>>+ > >>>> Status = AddIoSpace ( > >>>>- RootBridges[Index].Io.Base, > >>>>+ HostAddress, > >>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1 > >>>> ); > >>>> ASSERT_EFI_ERROR (Status); > >>>>@@ -422,7 +463,7 @@ InitializePciHostBridge ( > >>>> EfiGcdIoTypeIo, > >>>> 0, > >>>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, > >>>>- &RootBridges[Index].Io.Base, > >>>>+ &HostAddress, > >>>> gImageHandle, > >>>> NULL > >>>> ); > >>>>@@ -443,14 +484,20 @@ InitializePciHostBridge ( > >>>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE (MemApertures); MemApertureIndex++) { > >>>> if (MemApertures[MemApertureIndex]->Base <= MemApertures[MemApertureIndex]->Limit) { > >>>>+ // > >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>>+ // For GCD resource manipulation, we need to use host address. > >>>>+ // > >>>>+ HostAddress = TO_HOST_ADDRESS (MemApertures[MemApertureIndex]->Base, > >>>>+ MemApertures[MemApertureIndex]->Translation); > >>>> Status = AddMemoryMappedIoSpace ( > >>>>- MemApertures[MemApertureIndex]->Base, > >>>>+ HostAddress, > >>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>>> EFI_MEMORY_UC > >>>> ); > >>>> ASSERT_EFI_ERROR (Status); > >>>> Status = gDS->SetMemorySpaceAttributes ( > >>>>- MemApertures[MemApertureIndex]->Base, > >>>>+ HostAddress, > >>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>>> EFI_MEMORY_UC > >>>> ); > >>>>@@ -463,7 +510,7 @@ InitializePciHostBridge ( > >>>> EfiGcdMemoryTypeMemoryMappedIo, > >>>> 0, > >>>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>>>- &MemApertures[MemApertureIndex]->Base, > >>>>+ &HostAddress, > >>>> gImageHandle, > >>>> NULL > >>>> ); > >>>>@@ -654,6 +701,11 @@ AllocateResource ( > >>>> if (BaseAddress < Limit) { > >>>> // > >>>> // Have to make sure Aligment is handled since we are doing direct address allocation > >>>>+ // Strictly speaking, alignment requirement should be applied to device > >>>>+ // address instead of host address which is used in GCD manipulation below, > >>>>+ // but as we restrict the alignment of Translation to be larger than any BAR > >>>>+ // alignment in the root bridge, we can simplify the situation and consider > >>>>+ // the same alignment requirement is also applied to host address. > >>>> // > >>>> BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1, BitsOfAlignment)); > >>>>@@ -721,6 +773,7 @@ NotifyPhase ( > >>>> PCI_RESOURCE_TYPE Index2; > >>>> BOOLEAN ResNodeHandled[TypeMax]; > >>>> UINT64 MaxAlignment; > >>>>+ UINT64 Translation; > >>>> HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This); > >>>>@@ -822,14 +875,43 @@ NotifyPhase ( > >>>> BitsOfAlignment = LowBitSet64 (Alignment + 1); > >>>> BaseAddress = MAX_UINT64; > >>>>+ // > >>>>+ // RESTRICTION: To simplify the situation, we require the alignment of > >>>>+ // Translation must be larger than any BAR alignment in the same root > >>>>+ // bridge, so that resource allocation alignment can be applied to > >>>>+ // both device address and host address. > >>>>+ // > >>>>+ Translation = GetTranslationByResourceType (RootBridge, Index); > >>>>+ if ((Translation & Alignment) != 0) { > >>>>+ DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned to %lx!\n", > >>>>+ __FUNCTION__, __LINE__, Translation, Alignment > >>>>+ )); > >>>>+ ASSERT (FALSE); > 1. Cool! But can you replace ASSERT(FALSE) with > ASSERT ((Translation & Alignment) == 0)? > >>>>+ // > >>>>+ // This may be caused by too large alignment or too small > >>>>+ // Translation; pick the 1st possibility and return out of resource, > >>>>+ // which can also go thru the same process for out of resource > >>>>+ // outside the loop. > >>>>+ // > >>>>+ ReturnStatus = EFI_OUT_OF_RESOURCES; > >>>>+ continue; > >>>>+ } > >>>>+ > >>>> switch (Index) { > >>>> case TypeIo: > >>>>+ // > >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>>+ // For AllocateResource is manipulating GCD resource, we need to use > >>>>+ // host address here. > >>>>+ // > >>>> BaseAddress = AllocateResource ( > >>>> FALSE, > >>>> RootBridge->ResAllocNode[Index].Length, > >>>> MIN (15, BitsOfAlignment), > >>>>- ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), > >>>>- RootBridge->Io.Limit > >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), > >>>>+ RootBridge->Io.Translation), > >>>>+ TO_HOST_ADDRESS (RootBridge->Io.Limit, > >>>>+ RootBridge->Io.Translation) > >>>> ); > >>>> break; > >>>>@@ -838,8 +920,10 @@ NotifyPhase ( > >>>> TRUE, > >>>> RootBridge->ResAllocNode[Index].Length, > >>>> MIN (63, BitsOfAlignment), > >>>>- ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), > >>>>- RootBridge->MemAbove4G.Limit > >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), > >>>>+ RootBridge->MemAbove4G.Translation), > >>>>+ TO_HOST_ADDRESS (RootBridge->MemAbove4G.Limit, > >>>>+ RootBridge->MemAbove4G.Translation) > >>>> ); > >>>> if (BaseAddress != MAX_UINT64) { > >>>> break; > >>>>@@ -853,8 +937,10 @@ NotifyPhase ( > >>>> TRUE, > >>>> RootBridge->ResAllocNode[Index].Length, > >>>> MIN (31, BitsOfAlignment), > >>>>- ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), > >>>>- RootBridge->Mem.Limit > >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), > >>>>+ RootBridge->Mem.Translation), > >>>>+ TO_HOST_ADDRESS (RootBridge->Mem.Limit, > >>>>+ RootBridge->Mem.Translation) > >>>> ); > >>>> break; > >>>>@@ -863,8 +949,10 @@ NotifyPhase ( > >>>> TRUE, > >>>> RootBridge->ResAllocNode[Index].Length, > >>>> MIN (63, BitsOfAlignment), > >>>>- ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), > >>>>- RootBridge->PMemAbove4G.Limit > >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), > >>>>+ RootBridge->PMemAbove4G.Translation), > >>>>+ TO_HOST_ADDRESS (RootBridge->PMemAbove4G.Limit, > >>>>+ RootBridge->PMemAbove4G.Translation) > >>>> ); > >>>> if (BaseAddress != MAX_UINT64) { > >>>> break; > >>>>@@ -877,8 +965,10 @@ NotifyPhase ( > >>>> TRUE, > >>>> RootBridge->ResAllocNode[Index].Length, > >>>> MIN (31, BitsOfAlignment), > >>>>- ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), > >>>>- RootBridge->PMem.Limit > >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), > >>>>+ RootBridge->PMem.Translation), > >>>>+ TO_HOST_ADDRESS (RootBridge->PMem.Limit, > >>>>+ RootBridge->PMem.Translation) > >>>> ); > >>>> break; > >>>>@@ -1421,7 +1511,14 @@ GetProposedResources ( > >>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; > >>>> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;; > >>>> Descriptor->GenFlag = 0; > >>>>- Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base; > >>>>+ // > >>>>+ // AddrRangeMin in Resource Descriptor here should be device address > >>>>+ // instead of host address, or else PCI bus driver cannot set correct > >>>>+ // address into PCI BAR registers. > >>>>+ // Base in ResAllocNode is a host address, so conversion is needed. > >>>>+ // > >>>>+ Descriptor->AddrRangeMin = TO_DEVICE_ADDRESS (RootBridge->ResAllocNode[Index].Base, > >>>>+ GetTranslationByResourceType (RootBridge, Index)); > >>>> Descriptor->AddrRangeMax = 0; > >>>> Descriptor->AddrTranslationOffset = (ResStatus == ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS; > >>>> Descriptor->AddrLen = RootBridge->ResAllocNode[Index].Length; > >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > >>>>index dc06c16dc038..5dd9d257d46d 100644 > >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > >>>>@@ -86,12 +86,35 @@ CreateRootBridge ( > >>>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"", > >>>> (Bridge->AllocationAttributes & EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L"" > >>>> )); > >>>>+ // > >>>>+ // Translation for bus is not supported. > >>>>+ // > >>>> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n", Bridge->Bus.Base, Bridge->Bus.Limit)); > >>>>- DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n", Bridge->Io.Base, Bridge->Io.Limit)); > >>>>- DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n", Bridge->Mem.Base, Bridge->Mem.Limit)); > >>>>- DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n", Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit)); > >>>>- DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base, Bridge->PMem.Limit)); > >>>>- DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n", Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit)); > >>>>+ ASSERT (Bridge->Bus.Translation == 0); > >>>>+ if (Bridge->Bus.Translation != 0) { > >>>>+ return NULL; > >>>>+ } > > 2. Can you please use the same debug message format for Bus range? I mean to > print the Translation for Bus as well. Then in the end of debug message, use > assertion and if-check. This way developer can know what the Bus.Translation > is from the debug log. > > >>>>+ > >>>>+ DEBUG (( > >>>>+ DEBUG_INFO, " Io: %lx - %lx Translation=%lx\n", > >>>>+ Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation > >>>>+ )); > >>>>+ DEBUG (( > >>>>+ DEBUG_INFO, " Mem: %lx - %lx Translation=%lx\n", > >>>>+ Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation > >>>>+ )); > >>>>+ DEBUG (( > >>>>+ DEBUG_INFO, " MemAbove4G: %lx - %lx Translation=%lx\n", > >>>>+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit, Bridge->MemAbove4G.Translation > >>>>+ )); > >>>>+ DEBUG (( > >>>>+ DEBUG_INFO, " PMem: %lx - %lx Translation=%lx\n", > >>>>+ Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation > >>>>+ )); > >>>>+ DEBUG (( > >>>>+ DEBUG_INFO, " PMemAbove4G: %lx - %lx Translation=%lx\n", > >>>>+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit, Bridge->PMemAbove4G.Translation > >>>>+ )); > >>>> // > >>>> // Make sure Mem and MemAbove4G apertures are valid > >>>>@@ -206,7 +229,12 @@ CreateRootBridge ( > >>>> } > >>>> RootBridge->ResAllocNode[Index].Type = Index; > >>>> if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) { > >>>>- RootBridge->ResAllocNode[Index].Base = Aperture->Base; > >>>>+ // > >>>>+ // Base in ResAllocNode is a host address, while Base in Aperture is a > >>>>+ // device address. > >>>>+ // > >>>>+ RootBridge->ResAllocNode[Index].Base = TO_HOST_ADDRESS (Aperture->Base, > >>>>+ Aperture->Translation); > >>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; > >>>> RootBridge->ResAllocNode[Index].Status = ResAllocated; > >>>> } else { > >>>>@@ -403,6 +431,28 @@ RootBridgeIoCheckParameter ( > >>>> return EFI_SUCCESS; > >>>> } > > 3. Please add the missing function header comments. > > >>>>+EFI_STATUS > >>>>+RootBridgeIoGetMemTranslationByAddress ( > >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge, > >>>>+ IN UINT64 Address, > >>>>+ IN OUT UINT64 *Translation > >>>>+ ) > >>>>+{ > >>>>+ if (Address >= RootBridge->Mem.Base && Address <= RootBridge->Mem.Limit) { > >>>>+ *Translation = RootBridge->Mem.Translation; > >>>>+ } else if (Address >= RootBridge->PMem.Base && Address <= RootBridge->PMem.Limit) { > >>>>+ *Translation = RootBridge->PMem.Translation; > >>>>+ } else if (Address >= RootBridge->MemAbove4G.Base && Address <= RootBridge->MemAbove4G.Limit) { > >>>>+ *Translation = RootBridge->MemAbove4G.Translation; > >>>>+ } else if (Address >= RootBridge->PMemAbove4G.Base && Address <= RootBridge->PMemAbove4G.Limit) { > >>>>+ *Translation = RootBridge->PMemAbove4G.Translation; > >>>>+ } else { > >>>>+ return EFI_INVALID_PARAMETER; > >>>>+ } > >>>>+ > >>>>+ return EFI_SUCCESS; > >>>>+} > >>>>+ > >>>> /** > >>>> Polls an address in memory mapped I/O space until an exit condition is met, > >>>> or a timeout occurs. > >>>>@@ -658,13 +708,25 @@ RootBridgeIoMemRead ( > >>>> ) > >>>> { > >>>> EFI_STATUS Status; > >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>>+ UINT64 Translation; > >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address, > >>>> Count, Buffer); > >>>> if (EFI_ERROR (Status)) { > >>>> return Status; > >>>> } > >>>>- return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); > >>>>+ > >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This); > >>>>+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation); > >>>>+ if (EFI_ERROR (Status)) { > >>>>+ return Status; > >>>>+ } > >>>>+ > >>>>+ // Address passed to CpuIo->Mem.Read needs to be a host address instead of > >>>>+ // device address. > >>>>+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, > >>>>+ TO_HOST_ADDRESS (Address, Translation), Count, Buffer); > >>>> } > >>>> /** > >>>>@@ -705,13 +767,25 @@ RootBridgeIoMemWrite ( > >>>> ) > >>>> { > >>>> EFI_STATUS Status; > >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>>+ UINT64 Translation; > >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width, Address, > >>>> Count, Buffer); > >>>> if (EFI_ERROR (Status)) { > >>>> return Status; > >>>> } > >>>>- return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); > >>>>+ > >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This); > >>>>+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address, &Translation); > >>>>+ if (EFI_ERROR (Status)) { > >>>>+ return Status; > >>>>+ } > >>>>+ > >>>>+ // Address passed to CpuIo->Mem.Write needs to be a host address instead of > >>>>+ // device address. > >>>>+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, > >>>>+ TO_HOST_ADDRESS (Address, Translation), Count, Buffer); > >>>> } > >>>> /** > >>>>@@ -746,6 +820,8 @@ RootBridgeIoIoRead ( > >>>> ) > >>>> { > >>>> EFI_STATUS Status; > >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>>+ > >>>> Status = RootBridgeIoCheckParameter ( > >>>> This, IoOperation, Width, > >>>> Address, Count, Buffer > >>>>@@ -753,7 +829,13 @@ RootBridgeIoIoRead ( > >>>> if (EFI_ERROR (Status)) { > >>>> return Status; > >>>> } > >>>>- return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); > >>>>+ > >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This); > >>>>+ > >>>>+ // Address passed to CpuIo->Io.Read needs to be a host address instead of > >>>>+ // device address. > >>>>+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, > >>>>+ TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer); > >>>> } > >>>> /** > >>>>@@ -788,6 +870,8 @@ RootBridgeIoIoWrite ( > >>>> ) > >>>> { > >>>> EFI_STATUS Status; > >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>>+ > >>>> Status = RootBridgeIoCheckParameter ( > >>>> This, IoOperation, Width, > >>>> Address, Count, Buffer > >>>>@@ -795,7 +879,13 @@ RootBridgeIoIoWrite ( > >>>> if (EFI_ERROR (Status)) { > >>>> return Status; > >>>> } > >>>>- return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer); > >>>>+ > >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This); > >>>>+ > >>>>+ // Address passed to CpuIo->Io.Write needs to be a host address instead of > >>>>+ // device address. > >>>>+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, > >>>>+ TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count, Buffer); > >>>> } > >>>> /** > >>>>@@ -1615,9 +1705,17 @@ RootBridgeIoConfiguration ( > >>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; > >>>> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3; > >>>>+ // According to UEFI 2.7, RootBridgeIo->Configuration should return address > >>>>+ // range in CPU view (host address), and ResAllocNode->Base is already a CPU > >>>>+ // view address (host address). > >>>> Descriptor->AddrRangeMin = ResAllocNode->Base; > >>>> Descriptor->AddrRangeMax = ResAllocNode->Base + ResAllocNode->Length - 1; > >>>> Descriptor->AddrLen = ResAllocNode->Length; > >>>>+ Descriptor->AddrTranslationOffset = GetTranslationByResourceType ( > >>>>+ RootBridge, > >>>>+ ResAllocNode->Type > >>>>+ ); > >>>>+ > >>>> switch (ResAllocNode->Type) { > >>>> case TypeIo: > >>>>-- > >>>>2.7.4 > >>>> > >> > >> > >>-- > >>Thanks, > >>Ray > > > Reviewed-by: Ruiyu Ni > Please modify code according to the above 3 comments and make sure > the changes pass ECC check: > BaseTools/Source/Python/Ecc/Ecc.py -t -s \ > MdeModulePkg/Pci/PciHostBridgeDxe > > >_______________________________________________ > >edk2-devel mailing list > >edk2-devel@lists.01.org > >https://lists.01.org/mailman/listinfo/edk2-devel > > > > > -- > Thanks, > Ray