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:c0b::241; helo=mail-it0-x241.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x241.google.com (mail-it0-x241.google.com [IPv6:2607:f8b0:4001:c0b::241]) (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 AADC2209574EF for ; Tue, 27 Feb 2018 02:39:01 -0800 (PST) Received: by mail-it0-x241.google.com with SMTP id w19so10852253ite.0 for ; Tue, 27 Feb 2018 02:45:07 -0800 (PST) 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=xU/ihwxe/0gm7yyTP2MT3hVkOmlVkmXLq6ZdvLiRWHg=; b=HhbKBFo9g2gPIU7agsQnUFIoED2pE4ugvA2OfJAFyQihUUbe/j2eK1n5w1ekpupS53 xoNFcSkoXQlUpwyvn+t3vOE49ke2U3tmdyibFYFdcGxyfez87hfB+8j9Oup8RJxqkaX0 Pmmj2Sss7EECbUlRJpV/ISPTy7STjXkAPqXmk= 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=xU/ihwxe/0gm7yyTP2MT3hVkOmlVkmXLq6ZdvLiRWHg=; b=p3ga7VexzL9VbpW+n9dmC/CboXGm6ApzDgrm+cgomMuQ4YuvS8zazMwtX0oDVISlqF 8tUrFdHEVRX4ieq+V2ZqBp9iRhC+MCl1WRKTQVMS0s0jxcXETsQTpdFQK907clpdpHOY fkNFrSifMw63gA6Q0Nl75P4Ka+5PKXBD/FEiGqBTnzAp2yl8RK90fGhIiLWGtygGqJRx gTAWjKhYfGe9GWKBLi2Eny/l0BD4BFsreZ+YvL3X9kOeT7PI9dRP23IFtwzqJuDFlDGG gu9/ah0bzj9pF+0C2hZSJ51bft4nvb2+A6c/7VxgKG32ssQhkk4JjA7Zw1+Su+xIoLRE rLhg== X-Gm-Message-State: APf1xPAuQ0ElngLWAjJoA9NUAg7B9qdQj2nfNZf4NGInUKp5L8f+mqsQ Pg+DTcKiK48g9D7SuH9+ePmzgw== X-Google-Smtp-Source: AG47ELu5FbRGfk4b59pLeFN7KfTL9IlyWB2T6xBRpjFE1x4nlU6PtqwQ8+VsZjqVU7WbDIYKy11iuw== X-Received: by 10.36.152.195 with SMTP id n186mr16469568itd.147.1519728306345; Tue, 27 Feb 2018 02:45:06 -0800 (PST) Received: from SZX1000114654 ([104.237.91.49]) by smtp.gmail.com with ESMTPSA id k199sm7498932itb.35.2018.02.27.02.45.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 27 Feb 2018 02:45:05 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Tue, 27 Feb 2018 18:45:01 +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: <20180227104501.GD3918@SZX1000114654> References: <1519697389-3525-1-git-send-email-heyi.guo@linaro.org> <1519697389-3525-2-git-send-email-heyi.guo@linaro.org> <563e76b9-ebbc-be09-d306-8dcb86f12112@Intel.com> <20180227093334.GA3918@SZX1000114654> <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com> MIME-Version: 1.0 In-Reply-To: <19deadb3-4d26-914c-c9f8-c6c6716746fa@Intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) Subject: Re: [RFC v4 1/3] 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: Tue, 27 Feb 2018 10:39:02 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Feb 27, 2018 at 05:59:48PM +0800, Ni, Ruiyu wrote: > On 2/27/2018 5:33 PM, Guo Heyi wrote: > >Hi Ray, > > > >Thanks for your comments. It seems comments 10 and 12 are the major issue; let's > >discuss that first. > > > >1. In current implementation, namely PciBusDxe and PciHostBridgeDxe both in > >MdeModulePkg, Address parameter passed to RootBridgeIoMemRead comes from > >PciIoMemRead(). > >2. In PciIoMemRead(), Offset is only an offset within selected BAR; then > >PciIoDevice->PciBar[BarIndex].BaseAddress is added to Offset in > >PciIoVerifyBarAccess(). > Agree. I thought PciIoMemRead() gets the BAR base address from > GetBarAttributes(). > > >3. So whether the "Address" passed to RootBridgeIoMemRead() is host address or > >device address depends on the property of > >PciIoDevice->PciBar[BarIndex].BaseAddress. > RootBridgeIoMemRead() can choose to accept HOST address or Device address. > Either can be implemented. > I am fine to your proposal to use Device Address. > > >4. In PciBusDxe, we can see PciIoDevice->PciBar[BarIndex].BaseAddress simply > >comes from the value read from BAR register, which is absolutely a device > >address. > >5. What we do in patch 3/3 is also to convert the device address in > >PciBar[BarIndex].BaseAddress to host address before returning to the caller of > >PciIoGetBarAttributes(). > > > >Please let me know your comments. > > > >For other comments, I have no strong opinion and I can follow the conclusion of > >the community. > > So the issue (13) (not 12) is closed. > But what's your opinion to my comment (1): Explicitly add assertion and > if-check to make sure the alignment meets requirement. Ah, I thought it is obviously a good advice to place the restriction in the code rather than in documentation only, so I didn't reply it separately :) Thanks, Heyi > > Ard, > I provided the detailed comments because I felt the code is almost > finalized. > So the detailed comments can avoid creating more versions of patches. > > For the EMPTY comments and STATIC modifier, I created the first version of > this driver, so I'd like to make the coding style in this driver is > consistent. > But if you insist to use a different style, I am fine as long as the coding > style rule allows. > > Thanks, > Ray > > > > >Thanks and regards, > > > >Heyi > > > >On Tue, Feb 27, 2018 at 04:48:47PM +0800, Ni, Ruiyu wrote: > >>On 2/27/2018 10:09 AM, 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: > >>> > >>>1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address, for > >>>it is easy to check whether the address is below 4G or above 4G. > >>> > >>>2. Address returned by > >>>EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources > >>>is device address, or else PCI bus driver cannot set correct address > >>>into PCI BAR registers. > >>> > >>>3. Address returned by PciRootBridgeIo->Configuration is host address > >>>per UEFI 2.7 definition. > >>> > >>>4. Addresses used in GCD manipulation are host address. > >>> > >>>5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host > >>>address, for they are allocated from GCD. > >>> > >>>6. Address passed to PciHostBridgeResourceConflict is host address, > >>>for it comes from 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. > >>(1). I'd like to see this restriction be reflected in code. > >>Both assertion and if-check are needed to ensure debug firmware hang > >>and release firmware return fail status from PciHostBridge driver. > >> > >>> > >>>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: > >>> 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/PciHostResource.h | 2 + > >>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 14 +++ > >>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 85 +++++++++++--- > >>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 122 +++++++++++++++++--- > >>> 4 files changed, 194 insertions(+), 29 deletions(-) > >>> > >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>index 8612c0c3251b..662c2dd59529 100644 > >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h > >>>@@ -38,6 +38,8 @@ typedef enum { > >>> typedef struct { > >>> PCI_RESOURCE_TYPE Type; > >>>+ // Base is a host address instead of a device address when address translation > >>>+ // exists and host address != device address > >>(2). Coding style issue. The comments need to be in style like: > >>//[EMPTY] > >>// xxxx > >>//[EMPTY] > >>And how about just simply say Base is a host address. No matter address > >>translation exists or not, Base is a host address actually. > >> > >>> UINT64 Base; > >>> UINT64 Length; > >>> UINT64 Alignment; > >>>diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h > >>>index d42e9ecdd763..c842a4152e85 100644 > >>>--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h > >>>+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h > >>>@@ -20,8 +20,22 @@ > >>> // (Base > Limit) indicates an aperture is not available. > >>> // > >>> typedef struct { > >>>+ // Base and Limit are the device address instead of host address when > >>>+ // Translation is not zero > >>(3). Similar comments to (2). > >>> UINT64 Base; > >>> UINT64 Limit; > >>>+ // According to UEFI 2.7, Device Address = Host Address + Translation, > >>>+ // so Translation = Device Address - Host Address. > >>>+ // On platforms where Translation is not zero, the subtraction is probably to > >>>+ // be performed with UINT64 wrap-around semantics, for we may translate an > >>>+ // above-4G host address into a below-4G device address for legacy PCIe device > >>>+ // compatibility. > >>>+ // NOTE: The alignment of Translation is required to be larger than any BAR > >>>+ // alignment in the same root bridge, so that the same alignment can be > >>>+ // applied to both device address and host address, which simplifies the > >>>+ // situation and makes the current resource allocation code in generic PCI > >>>+ // host bridge driver still work. > >>(4). Still I'd like to see the alignment requirement be reflected in code > >>through assertion and if-check. > >> > >>>+ UINT64 Translation; > >>> } PCI_ROOT_BRIDGE_APERTURE; > >>> typedef struct { > >>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>index 1494848c3e8c..1e65faee9084 100644 > >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c > >>>@@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; > >>> EFI_EVENT mIoMmuEvent; > >>> VOID *mIoMmuRegistration; > >>>+STATIC > >>(5). Can you remove STATIC? personal preference:) > >>>+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; > >>>+ default: > >>>+ ASSERT (FALSE); > >>>+ return 0; > >>>+ } > >>>+} > >>>+ > >>> /** > >>> Ensure the compatibility of an IO space descriptor with the IO aperture. > >>>@@ -366,6 +390,7 @@ InitializePciHostBridge ( > >>> UINTN MemApertureIndex; > >>> BOOLEAN ResourceAssigned; > >>> LIST_ENTRY *Link; > >>>+ UINT64 TempHostAddress; > >>(6). Just use name HostAddress? > >>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount); > >>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) { > >>>@@ -411,18 +436,24 @@ InitializePciHostBridge ( > >>> } > >>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) { > >>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>+ // According to UEFI 2.7, device address = host address + Translation. > >>>+ // For GCD resource manipulation, we should use host address, so > >>>+ // Translation is subtracted from device address here. > >>(7). Please adjust the comment style to have two EMPTY line of comment above > >>and below. Same to all comments in the patch. > >>> Status = AddIoSpace ( > >>>- RootBridges[Index].Io.Base, > >>>+ RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation, > >>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1 > >>> ); > >>> ASSERT_EFI_ERROR (Status); > >>> if (ResourceAssigned) { > >>>+ TempHostAddress = RootBridges[Index].Io.Base - > >>>+ RootBridges[Index].Io.Translation; > >>> Status = gDS->AllocateIoSpace ( > >>> EfiGcdAllocateAddress, > >>> EfiGcdIoTypeIo, > >>> 0, > >>> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, > >>>- &RootBridges[Index].Io.Base, > >>>+ &TempHostAddress, > >>> gImageHandle, > >>> NULL > >>> ); > >>>@@ -443,14 +474,18 @@ 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. > >>>+ // According to UEFI 2.7, device address = host address + Translation. > >>>+ // For GCD resource manipulation, we should use host address, so > >>>+ // Translation is subtracted from device address here. > >>> Status = AddMemoryMappedIoSpace ( > >>>- MemApertures[MemApertureIndex]->Base, > >>>+ MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, > >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>> EFI_MEMORY_UC > >>> ); > >>> ASSERT_EFI_ERROR (Status); > >>> Status = gDS->SetMemorySpaceAttributes ( > >>>- MemApertures[MemApertureIndex]->Base, > >>>+ MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, > >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>> EFI_MEMORY_UC > >>> ); > >>>@@ -458,12 +493,14 @@ InitializePciHostBridge ( > >>> DEBUG ((DEBUG_WARN, "PciHostBridge driver failed to set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status)); > >>> } > >>> if (ResourceAssigned) { > >>>+ TempHostAddress = MemApertures[MemApertureIndex]->Base - > >>>+ MemApertures[MemApertureIndex]->Translation; > >>> Status = gDS->AllocateMemorySpace ( > >>> EfiGcdAllocateAddress, > >>> EfiGcdMemoryTypeMemoryMappedIo, > >>> 0, > >>> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, > >>>- &MemApertures[MemApertureIndex]->Base, > >>>+ &TempHostAddress, > >>> gImageHandle, > >>> NULL > >>> ); > >>>@@ -654,6 +691,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)); > >>>@@ -824,12 +866,17 @@ NotifyPhase ( > >>> switch (Index) { > >>> case TypeIo: > >>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address. > >>>+ // According to UEFI 2.7, device address = host address + Translation. > >>>+ // For AllocateResource is manipulating GCD resource, we should use > >>>+ // host address here, so Translation is subtracted from Base and > >>>+ // Limit. > >>> BaseAddress = AllocateResource ( > >>> FALSE, > >>> RootBridge->ResAllocNode[Index].Length, > >>> MIN (15, BitsOfAlignment), > >>>- ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1), > >>>- RootBridge->Io.Limit > >>>+ ALIGN_VALUE (RootBridge->Io.Base, Alignment + 1) - RootBridge->Io.Translation, > >>>+ RootBridge->Io.Limit - RootBridge->Io.Translation > >>> ); > >>> break; > >>>@@ -838,8 +885,8 @@ NotifyPhase ( > >>(8). Add assertion and if-check here to make sure alignment of Translation > >>is no less than the Alignment. > >>> TRUE, > >>> RootBridge->ResAllocNode[Index].Length, > >>> MIN (63, BitsOfAlignment), > >>>- ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1), > >>>- RootBridge->MemAbove4G.Limit > >>>+ ALIGN_VALUE (RootBridge->MemAbove4G.Base, Alignment + 1) - RootBridge->MemAbove4G.Translation, > >>>+ RootBridge->MemAbove4G.Limit - RootBridge->MemAbove4G.Translation > >>> ); > >>> if (BaseAddress != MAX_UINT64) { > >>> break; > >>>@@ -853,8 +900,8 @@ NotifyPhase ( > >>> TRUE, > >>> RootBridge->ResAllocNode[Index].Length, > >>> MIN (31, BitsOfAlignment), > >>>- ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1), > >>>- RootBridge->Mem.Limit > >>>+ ALIGN_VALUE (RootBridge->Mem.Base, Alignment + 1) - RootBridge->Mem.Translation, > >>>+ RootBridge->Mem.Limit - RootBridge->Mem.Translation > >>> ); > >>> break; > >>>@@ -863,8 +910,8 @@ NotifyPhase ( > >>> TRUE, > >>> RootBridge->ResAllocNode[Index].Length, > >>> MIN (63, BitsOfAlignment), > >>>- ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1), > >>>- RootBridge->PMemAbove4G.Limit > >>>+ ALIGN_VALUE (RootBridge->PMemAbove4G.Base, Alignment + 1) - RootBridge->PMemAbove4G.Translation, > >>>+ RootBridge->PMemAbove4G.Limit - RootBridge->PMemAbove4G.Translation > >>> ); > >>> if (BaseAddress != MAX_UINT64) { > >>> break; > >>>@@ -877,8 +924,8 @@ NotifyPhase ( > >>> TRUE, > >>> RootBridge->ResAllocNode[Index].Length, > >>> MIN (31, BitsOfAlignment), > >>>- ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1), > >>>- RootBridge->PMem.Limit > >>>+ ALIGN_VALUE (RootBridge->PMem.Base, Alignment + 1) - RootBridge->PMem.Translation, > >>>+ RootBridge->PMem.Limit - RootBridge->PMem.Translation > >>> ); > >>> break; > >>>@@ -1152,6 +1199,7 @@ StartBusEnumeration ( > >>> Descriptor->AddrSpaceGranularity = 0; > >>> Descriptor->AddrRangeMin = RootBridge->Bus.Base; > >>> Descriptor->AddrRangeMax = 0; > >>>+ // Ignore translation offset for bus > >>(9). Per PI spec on StartBusEnumeration, AddrRangeMax is ignored. So I don't > >>think we need add comments here. > >>> Descriptor->AddrTranslationOffset = 0; > >>> Descriptor->AddrLen = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1; > >>>@@ -1421,7 +1469,12 @@ 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 Translation is added. > >>>+ Descriptor->AddrRangeMin = RootBridge->ResAllocNode[Index].Base + > >>>+ GetTranslationByResourceType (RootBridge, Index); > >>(10). Do you think defining a PciHostBridgeDxe internal macro > >>TO_HOST_ADDRESS(DeviceAddress, Offset) and TO_DEVICE_ADDRESS(HostAddress, > >>Offset) is better? > >> > >>> 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..edaa0d48a441 100644 > >>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > >>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c > >>>@@ -86,12 +86,28 @@ 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"" > >>> )); > >>>+ // We don't see any scenario for bus translation, so translation for bus is just ignored. > >>> 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)); > >>>+ 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 +222,15 @@ CreateRootBridge ( > >>> } > >>> RootBridge->ResAllocNode[Index].Type = Index; > >>> if (Bridge->ResourceAssigned && (Aperture->Limit >= Aperture->Base)) { > >>>- RootBridge->ResAllocNode[Index].Base = Aperture->Base; > >>>+ // Ignore translation for bus > >>(11). How about we only make sure the TranslationOffset is 0 for TypeBus > >>when getting the aperture from PciHostBridgeLib. > >>So that later logic doesn't need to do special handling for the TypeBus. > >>This way the code can be simpler. > >>>+ if (Index == TypeBus) { > >>>+ RootBridge->ResAllocNode[Index].Base = Aperture->Base; > >>>+ } else { > >>>+ // Base in ResAllocNode is a host address, while Base in Aperture is a > >>>+ // device address, so translation needs to be subtracted. > >>>+ RootBridge->ResAllocNode[Index].Base = Aperture->Base - > >>>+ Aperture->Translation; > >>(12). Still. Do you think using TO_HOST_ADDRESS() is better? > >>>+ } > >>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; > >>> RootBridge->ResAllocNode[Index].Status = ResAllocated; > >>> } else { > >>>@@ -403,6 +427,28 @@ RootBridgeIoCheckParameter ( > >>> return EFI_SUCCESS; > >>> } > >>>+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 +704,24 @@ 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); > >>(13). Why do you think the Address is a Device Address? > >>PciIo.GetBarAttributes() returns a HOST Address and the Translation Offset. > >>I didn't see you change the PciIoMemRead() implementation. > >>So it means the same HOST Address is passed into the this function. > >> > >>>+ if (EFI_ERROR (Status)) { > >>>+ return Status; > >>>+ } > >>>+ > >>>+ // Address passed to CpuIo->Mem.Read should be a host address instead of > >>>+ // device address, so Translation should be subtracted. > >>>+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer); > >>> } > >>> /** > >>>@@ -705,13 +762,24 @@ 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 should be a host address instead of > >>>+ // device address, so Translation should be subtracted. > >>>+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation, Count, Buffer); > >>(14). Same as (13) > >>> } > >>> /** > >>>@@ -746,6 +814,8 @@ RootBridgeIoIoRead ( > >>> ) > >>> { > >>> EFI_STATUS Status; > >>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>+ > >>> Status = RootBridgeIoCheckParameter ( > >>> This, IoOperation, Width, > >>> Address, Count, Buffer > >>>@@ -753,7 +823,12 @@ 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 should be a host address instead of > >>>+ // device address, so Translation should be subtracted. > >>>+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer); > >>(15). Same as (13) > >>> } > >>> /** > >>>@@ -788,6 +863,8 @@ RootBridgeIoIoWrite ( > >>> ) > >>> { > >>> EFI_STATUS Status; > >>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge; > >>>+ > >>> Status = RootBridgeIoCheckParameter ( > >>> This, IoOperation, Width, > >>> Address, Count, Buffer > >>>@@ -795,7 +872,12 @@ 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 should be a host address instead of > >>>+ // device address, so Translation should be subtracted. > >>>+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - RootBridge->Io.Translation, Count, Buffer); > >>(16). Same as (13) > >>> } > >>> /** > >>>@@ -1615,25 +1697,39 @@ 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; > >>> switch (ResAllocNode->Type) { > >>> case TypeIo: > >>>- Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; > >>>+ Descriptor->AddrTranslationOffset = RootBridge->Io.Translation; > >>(17). Can GetTranslationByResourceType() be used to simplify the code? > >>> break; > >>> case TypePMem32: > >>>- Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > >>>+ Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > >>>+ Descriptor->AddrTranslationOffset = RootBridge->PMem.Translation; > >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > >>>+ Descriptor->AddrSpaceGranularity = 32; > >>>+ break; > >>>+ > >>> case TypeMem32: > >>>+ Descriptor->AddrTranslationOffset = RootBridge->Mem.Translation; > >>> Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > >>> Descriptor->AddrSpaceGranularity = 32; > >>> break; > >>> case TypePMem64: > >>>- Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > >>>+ Descriptor->SpecificFlag = EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; > >>>+ Descriptor->AddrTranslationOffset = RootBridge->PMemAbove4G.Translation; > >>>+ Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > >>>+ Descriptor->AddrSpaceGranularity = 64; > >>> case TypeMem64: > >>>+ Descriptor->AddrTranslationOffset = RootBridge->MemAbove4G.Translation; > >>> Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; > >>> Descriptor->AddrSpaceGranularity = 64; > >>> break; > >>> > >> > >>(18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py on the > >>PciHostBridgeDxe driver to make sure coding style matches the requirement. > >> > >>-- > >>Thanks, > >>Ray > >_______________________________________________ > >edk2-devel mailing list > >edk2-devel@lists.01.org > >https://lists.01.org/mailman/listinfo/edk2-devel > > > > > -- > Thanks, > Ray