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:c05::241; helo=mail-pg0-x241.google.com; envelope-from=heyi.guo@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-pg0-x241.google.com (mail-pg0-x241.google.com [IPv6:2607:f8b0:400e:c05::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 D76722034D8E3 for ; Wed, 28 Feb 2018 19:50:47 -0800 (PST) Received: by mail-pg0-x241.google.com with SMTP id r26so1820344pgv.13 for ; Wed, 28 Feb 2018 19:56:55 -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=WCc/2xOvbDtNV4wlRC/54nH+mAkqHg14oLolllW8r4w=; b=BHH/ayXf6Qv7XUj5a0mIjsr4kjpq0KnoXsBV+WABbHK1r+/zjw7MsfUHwu8Z7Z4X/C PIGkSkDBwDQ68UaeY99ZQlmWBiQqwy3V4Tymdt/eOI43PE4Kq4MCf9NFrskazrD7VpZQ l1j+xyoVjczqWUEAvvzh3UPIIhZxBR/cj/6H4= 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=WCc/2xOvbDtNV4wlRC/54nH+mAkqHg14oLolllW8r4w=; b=jCYr62EUApn2o+5Qibyg5GG1AmUQ+ead0jzICI5DX6zuiZ4269TtK6c/84ZWislQFW djEKPtag5BWmbWVO+mLR/K8v8jjqMUSxARuKlaOCZ9O4tSU3G+MnnJalkWM9pa69ux7A ZGwuRM+D3/bg3H08xLHjyv8gTmIg46lh/8K2rCfTmj0g73xtCywjhoy9xMe55sA7TpkQ PzpUOIYl+k08eTGMzTEYVmtP/lFoI+uTn4sAKBZ8b04J7xyJMUaBfQNFI4u9B4qb2je5 msMiRaunCIqBt2Pi2catfmhJumk1DHubYqU0Byw8j6AOwgULpo2qAbEjpqeTt6CZIEKA 0XoQ== X-Gm-Message-State: APf1xPAhcFfeF96EVC3SQt7YPMBcEOsGuFcJNDwfSW9OLjshaFkOgvEH dJfKfiXE1+FHcC6QlJ1vK8YwWw== X-Google-Smtp-Source: AG47ELv0RiC7H444Pzopnd9lGOe+ZwSOT9R0EvxE0nJlFWg3rq3Z5nJIW1H3e9rQXp+2kxn+5mVSeg== X-Received: by 10.98.75.18 with SMTP id y18mr518074pfa.124.1519876615248; Wed, 28 Feb 2018 19:56:55 -0800 (PST) Received: from SZX1000114654 ([45.56.152.115]) by smtp.gmail.com with ESMTPSA id b88sm6124623pfd.108.2018.02.28.19.56.51 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 28 Feb 2018 19:56:54 -0800 (PST) From: Guo Heyi X-Google-Original-From: Guo Heyi Date: Thu, 1 Mar 2018 11:56:49 +0800 To: "Ni, Ruiyu" Cc: Heyi Guo , edk2-devel@lists.01.org, Ard Biesheuvel , Star Zeng , Eric Dong , Laszlo Ersek , Michael D Kinney Message-ID: <20180301035649.GB39361@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> MIME-Version: 1.0 In-Reply-To: <563e76b9-ebbc-be09-d306-8dcb86f12112@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: Thu, 01 Mar 2018 03:50:48 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Ray, I've one question about (9); please see it below: 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. The comment is for the below code, not for AddrRangeMax, so I'm not quite understand the comments. Descriptor->AddrTranslationOffset = 0; Heyi > > 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