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::244; helo=mail-it0-x244.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x244.google.com (mail-it0-x244.google.com [IPv6:2607:f8b0:4001:c0b::244]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id D2FB921E082B5 for ; Wed, 14 Mar 2018 04:19:37 -0700 (PDT) Received: by mail-it0-x244.google.com with SMTP id l187-v6so4264502ith.4 for ; Wed, 14 Mar 2018 04:26:00 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Q7N6/BWrv66qJRdefGIT+kDSX3dJpr3xBa0M/WlBCtI=; b=C/zUYI+PWHCZzIZm2797saS2VuMfotqnjHPBdRabgwOQQ7fKpbrbkXYJiuz2u/6X8S gafAv+sUvOeXHhMgJTpcmlZwW5jwgK3BHsv+x1ECxF0ABdUXnh3e5YMZ9kD5SokUPNSH DK7KoaviNUcbhvpqFBwgmLraAW25PnEdg3puc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Q7N6/BWrv66qJRdefGIT+kDSX3dJpr3xBa0M/WlBCtI=; b=HPdEdUfuQdMzi2iukAG9v57WNI+g3Afja9exNqvM2v69PW8bXcOk2FOHXvIiKRVSrD QN+6QNU4eqqYuf6CRgcxBCw2fKhluTsdagqAwgygqebTBTXt4mOFRnsAFE3fabhqim9/ guruzWpCXGvEyDpjrgoVXcBO3JL0yTC4zmHGYF91tQqp3D49GagOm2lv8qViMudZCzB4 bl01tCh+dY6SFyLI+GIMRI1rcW+aAY8LlxF0Ho5GsgO7K/2ZKtn0BoHz6te6fkwH7pD3 8PZ1XH8rKfQxUsy6HiOYo0B5p5HaFX+Y15tdEK6WBkzQTJunLpz3vn2nhTzaOTmNmTQC Wq0Q== X-Gm-Message-State: AElRT7F8VkY8d6zJIAUJrMIzH92N5PsZo2OAsCgvGXHPn5K8mJ2jjSeg T6fUc6GZqEF4M1ujIqcbnYcNQ4zC0YL3CKaTDsexZg== X-Google-Smtp-Source: AG47ELucmak/bsr/KvKc1aWkwA67/geJAXbY88WfqFt58anIBoI+uO7dUm04Ng00ULn6EKe/G6pH0bDHLG+O4pMZrEk= X-Received: by 10.36.90.5 with SMTP id v5mr1521630ita.138.1521026759935; Wed, 14 Mar 2018 04:25:59 -0700 (PDT) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Wed, 14 Mar 2018 04:25:59 -0700 (PDT) In-Reply-To: <9f3e6241-e7fd-1178-ddc2-5b1cdd1b1153@Intel.com> 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> From: Ard Biesheuvel Date: Wed, 14 Mar 2018 11:25:59 +0000 Message-ID: To: "Ni, Ruiyu" Cc: Guo Heyi , Eric Dong , "edk2-devel@lists.01.org" , Michael D Kinney , Laszlo Ersek , Star Zeng 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: Wed, 14 Mar 2018 11:19:38 -0000 Content-Type: text/plain; charset="UTF-8" On 14 March 2018 at 07:57, 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 > Thanks Ray Does that apply to the whole series?