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:c06::243; helo=mail-io0-x243.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x243.google.com (mail-io0-x243.google.com [IPv6:2607:f8b0:4001:c06::243]) (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 435592034D8D6 for ; Tue, 27 Feb 2018 00:49:25 -0800 (PST) Received: by mail-io0-x243.google.com with SMTP id p78so20341721iod.13 for ; Tue, 27 Feb 2018 00:55:31 -0800 (PST) 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=zCjm+TpZISC4CwsduOQKjSCNDlgv3gEWsgbBFhN1DeY=; b=BlnGMTdXuUWMq4dOUI49qrL9IplbJyPi0mQlaWbPTdqUxs1jHYSqzCqSYOrkN5K9ZZ VjxWG/eInIcQsstuT5GKQMmXiF0gbzpmPVkIhyRwBBOnQl6I6267WOJ19LF+wGQrUNDq q7wm7dhJ09puPP/nO51FNrusvlctRaZX6D7BM= 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=zCjm+TpZISC4CwsduOQKjSCNDlgv3gEWsgbBFhN1DeY=; b=R9UiToWcyu5vfApHI7KzUUnjV9XAubIOSpDqrugUS0HSwD/T5610aFV7xTd9FO9kIr fBCXbJzzsRMR2KcrgWt/kNwCyJIagvTZjuqYXj4SKd+FqLSEbITp4esq3xZaDMEYRAia 85QYc3amTtH1o8o8ziE3/pzAbGq/6uTOtnmGFMkzB8Njbd98icgCfDQwBe+h6607Q3EL GHDcbkLw29ZsQds6yF6SXvweyKmgoyuT9HxwwPLFy1imETwFAMCChoMqmR4byLIyaTfd gX0V+u855mmrJgchm51etVEpiW8hIosxM89xI8r60B5Cu08HsfYf2c116/vi7fE05EIb ObOg== X-Gm-Message-State: APf1xPA04HfVP+I5BBY5siG6Q4VDlBsA0H2X0IIrO9jhXVmkZugR3gkV GFOtbREUM6Stnw2bqIxLPcAVRm2HZWTHiFcUGKPguA== X-Google-Smtp-Source: AG47ELvze1vls5+QU6ct20u15YHBOoSG05dS8bagU1XWiAfA1xy94W/5CyTQdZYy+91EZPmnf9Z2H8ilo0hJ9KX3jFk= X-Received: by 10.107.56.69 with SMTP id f66mr14978168ioa.170.1519721730794; Tue, 27 Feb 2018 00:55:30 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Tue, 27 Feb 2018 00:55:29 -0800 (PST) In-Reply-To: <563e76b9-ebbc-be09-d306-8dcb86f12112@Intel.com> 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> From: Ard Biesheuvel Date: Tue, 27 Feb 2018 08:55:29 +0000 Message-ID: To: "Ni, Ruiyu" Cc: Heyi Guo , "edk2-devel@lists.01.org" , Star Zeng , Eric Dong , Laszlo Ersek , Michael D Kinney 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 08:49:26 -0000 Content-Type: text/plain; charset="UTF-8" On 27 February 2018 at 08:48, 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] Please, can we stop bikheshedding about the comment style? This style is the exact opposite of what the coding style document mandates, and both flavours exist throughout the code. > 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:) > Why? >> +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. > Again, can we please focus on the code? This issue is complex and controversial enough as it is. >> 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