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::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::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 0D0AD22436937 for ; Fri, 23 Feb 2018 07:11:45 -0800 (PST) Received: by mail-io0-x241.google.com with SMTP id t22so10140140iob.3 for ; Fri, 23 Feb 2018 07:17:47 -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=Oo3udfD/9uEuAmOpBTRu0nHwoscNLOYXNWjUgoW/vAw=; b=f2K15FzzsDXET8ZCESAfhJvyXi444/7Oe+WkWuDwYHVOjWHg8Lv8OK9Tr8sQkQ+br9 VYWuLvnIYMFy+2lh9CYBZiOoxMX+Phi0d11LttUwFB7nDMFm3eBF0/vmWG9HX7lGLHfX 9/M5HDi8UIm+3G0+Q6lzTFx89Ml0XPP3QKS7I= 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=Oo3udfD/9uEuAmOpBTRu0nHwoscNLOYXNWjUgoW/vAw=; b=JidB3Ypb5g7Uawm8xz+RCWJznbTtth/+cpJcMHc5CPN+Bt1brx6qRXu2cPNre55a6J lVGAmFQbFxnyG15P51iox0xe2caCD1qGGZE5TnoxlVVmciCui0JJfF+z4xGv6K/VsaUI B/502DYAgyxUXF7UyWNAoPQtZnavTDaXXyLp3BXsaxI7/bz9YXvECpgunY8Pn6odRno+ 3qMKm+yVWiwsftuC3lpRBEdDZcEW1o3I+VPKxSKkMumg7wwZKNRPSDgR5xK7+N0xuQVu VzQaIDOhoP5lTBAG4SG9sU/NYrE3lh1apFHTQhjYQThbJRaPfzsUZ3n3fyKVVY9eabmX 8SJA== X-Gm-Message-State: APf1xPDPaZbBqGJTOO2eZ6X83IZeawbkXS1uCV9tEvgrp40K5Gj3KUMe kj3BEFw0fC4/xZ0HD97rHLSKdMuhR5G18s8jkmNGNw== X-Google-Smtp-Source: AG47ELvz2BqfVHiD1sIWH7Yw/TuCOpdMypaYCRJlUtYUxOstATiFEvxF7B8CLGVX/S7pOkMb4eGqtesY3sYHpRSCr2M= X-Received: by 10.107.56.69 with SMTP id f66mr2111678ioa.170.1519399066633; Fri, 23 Feb 2018 07:17:46 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.138.209 with HTTP; Fri, 23 Feb 2018 07:17:46 -0800 (PST) In-Reply-To: References: <1519376008-110662-1-git-send-email-heyi.guo@linaro.org> <1519376008-110662-2-git-send-email-heyi.guo@linaro.org> From: Ard Biesheuvel Date: Fri, 23 Feb 2018 15:17:46 +0000 Message-ID: To: Laszlo Ersek Cc: Heyi Guo , "edk2-devel@lists.01.org" , Ruiyu Ni , Eric Dong , Michael D Kinney , Star Zeng Subject: Re: [RFC v3 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: Fri, 23 Feb 2018 15:11:46 -0000 Content-Type: text/plain; charset="UTF-8" On 23 February 2018 at 15:05, Laszlo Ersek wrote: > Thanks for writing up the details! > > Comments: > > On 02/23/18 09:53, 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. > > OK, this looks like a sensible choice to me. It means that the "apply" > term used in the UEFI spec means "add", which (I think) is the "natural" > interpretation. > >> 2. PciRootBridgeIo->Configuration should return CPU view address, as >> well as PciIo->GetBarAttributes. > > OK. > >> >> 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. > > I agree that PciHostBridgeLib instances that do not set a nonzero > Translation need not care about the difference. > > (1) Still, can you confirm this is a "natural" choice? Were the > DmaAbove4G, MemAbove4G and PMemAbove4G fields originally introduced with > the device (PCI) view in mind? > Yes. Although DMA and MMIO space are different in this regard (and this series does not cover the former), the purpose of having 32-bit addressable BAR space and/or 32-bit addressable memory for DMA is to ensure that the addresses can be generated/decoded by the PCI device. So it is appropriate for the distinction between 'below' and 'above' 4 GB to be made based on the PCI address. > ... I also meant to raise a concern about the InitializePciHostBridge() > function where we call AddIoSpace() and AddMemoryMappedIoSpace() -- > which end up manipulating GCD -- with data from > PCI_ROOT_BRIDGE_APERTURE. I can see you modify those call sites in the > patch, so that's good. (I do have more comments on the actual > implementation.) > >> >> 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. >> >> 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 >> --- >> .../Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 74 +++++++++++--- >> .../Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 2 + >> .../Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 112 ++++++++++++++++++--- >> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 8 ++ >> 4 files changed, 167 insertions(+), 29 deletions(-) >> >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> index 1494848..e8979eb 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c >> @@ -32,6 +32,29 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol; >> EFI_EVENT mIoMmuEvent; >> VOID *mIoMmuRegistration; >> >> +STATIC >> +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: > > (2) How about we return zero for TypeBus, explicitly, and then > ASSERT(FALSE) and return zero for "default"? > +1 >> + return 0; >> + } >> +} >> + >> /** >> Ensure the compatibility of an IO space descriptor with the IO aperture. >> >> @@ -411,8 +434,12 @@ 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. > > (3) In general, the comment style requires comments like this: > > // > // Add empty comment lines both before and after. > // > Quite the opposite, in fact. Search for 'horror vacui' in the coding style document. That does not mean I agree with that document, and I am perfectly fine with both styles. >> 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); > > This looks fine. > >> @@ -422,7 +449,7 @@ InitializePciHostBridge ( >> EfiGcdIoTypeIo, >> 0, >> RootBridges[Index].Io.Limit - RootBridges[Index].Io.Base + 1, >> - &RootBridges[Index].Io.Base, >> + &RootBridges[Index].Io.Base - RootBridges[Index].Io.Translation, >> gImageHandle, >> NULL >> ); > > (4) But this is wrong (operating under the assumption that > PCI_ROOT_BRIDGE_APERTURE providing device addresses). Namely, 5th > parameter of gDS->AllocateIoSpace() is an in/out parameter: > > [MdeModulePkg/Core/Dxe/Gcd/Gcd.c] > > EFI_STATUS > EFIAPI > CoreAllocateIoSpace ( > IN EFI_GCD_ALLOCATE_TYPE GcdAllocateType, > IN EFI_GCD_IO_TYPE GcdIoType, > IN UINTN Alignment, > IN UINT64 Length, > IN OUT EFI_PHYSICAL_ADDRESS *BaseAddress, > IN EFI_HANDLE ImageHandle, > IN EFI_HANDLE DeviceHandle OPTIONAL > ) > > The patch, as written, takes the address of the Base field, and then > decreases that address by Translation * sizeof Base. The resultant > pointer is passed to the function as 5th argument. > > I don't think that's what you meant :) > > Instead, you have to translate the device address to host address first > (using a temporary variable), and pass that to the function. (Updating > the Base field back to the resultant / output value shouldn't be > necessary, because GcdAllocateType==EfiGcdAllocateAddress, so the > allocation is required to succeed at the exact address requested.) > >> @@ -443,14 +470,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 >> ); >> @@ -463,7 +494,7 @@ InitializePciHostBridge ( >> EfiGcdMemoryTypeMemoryMappedIo, >> 0, >> MemApertures[MemApertureIndex]->Limit - MemApertures[MemApertureIndex]->Base + 1, >> - &MemApertures[MemApertureIndex]->Base, >> + &MemApertures[MemApertureIndex]->Base - MemApertures[MemApertureIndex]->Translation, >> gImageHandle, >> NULL >> ); > > (5) Same comment as (4) applies, to the 5th argument. > >> @@ -824,12 +855,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 +874,8 @@ NotifyPhase ( >> 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 +889,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 +899,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 +913,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; > > (6) For all of these, I believe that we have to think about the corner > case when the Translation value is not a multiple of (Alignment + 1). > > "Alignment" comes from the PCI BAR in question, whereas Base comes from > the platform PciHostBridgeLib. I think these are fairly independent (you > can plug a 3rd party, discrete PCI card into a board). I find it > plausible that the card has such a huge MMIO BAR (and alignment) that > the platform's Translation offset is not a multiple thereof. > > So, which "view" has to be aligned here? The PCI (device) view, or the > host (CPU) view? > > ... Hmmm wait, the AllocateResource() function aligns BaseAddress > internally anyway. So, first, I don't understand why the pre-patch code > uses ALIGN_VALUE at the call site as well; second, due to the alignment > internal to AllocateResource(), we couldn't align *just* the PCI view > even if we wanted to. > > So I guess this code is correct (due to the alignment internal to > AllocateResource()), but could we perhaps simplify it by passing > > RootBridge->Xxx.Base - RootBridge->Xxx.Translation > > i.e., by dropping the outer alignment? > > (To be clear, dropping the "outer" alignment, if it's a valid change, > should be done as a separate patch, before the translation is > introduced. I hope Ray can comment on this.) > >> >> @@ -1152,6 +1188,7 @@ StartBusEnumeration ( >> Descriptor->AddrSpaceGranularity = 0; >> Descriptor->AddrRangeMin = RootBridge->Bus.Base; >> Descriptor->AddrRangeMax = 0; >> + // Ignore translation offset for bus >> Descriptor->AddrTranslationOffset = 0; >> Descriptor->AddrLen = RootBridge->Bus.Limit - RootBridge->Bus.Base + 1; >> > > (7) Ah, in this case, adding TypeBus under (2) is not necessary, just > ASSERT(FALSE) under the currently proposed "default" label. > > >> @@ -1421,7 +1458,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); >> 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/PciHostResource.h b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h >> index 8612c0c..662c2dd 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 >> UINT64 Base; >> UINT64 Length; >> UINT64 Alignment; >> diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> index dc06c16..04ed411 100644 >> --- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> +++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c >> @@ -86,12 +86,23 @@ 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 >> + )); > > (8) I suggest capitalizing "Translation" in the debug output. > > (9) The indentation is not idiomatic, it should be > > DEBUG (( > ... > ... > )); > >> >> // >> // Make sure Mem and MemAbove4G apertures are valid >> @@ -206,7 +217,10 @@ 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, so translation needs to be subtracted. >> + RootBridge->ResAllocNode[Index].Base = Aperture->Base - >> + Aperture->Translation; >> RootBridge->ResAllocNode[Index].Length = Aperture->Limit - Aperture->Base + 1; >> RootBridge->ResAllocNode[Index].Status = ResAllocated; >> } else { >> @@ -403,6 +417,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 +694,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); >> + 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 +752,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); >> } >> >> /** >> @@ -746,6 +804,8 @@ RootBridgeIoIoRead ( >> ) >> { >> EFI_STATUS Status; >> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >> + >> Status = RootBridgeIoCheckParameter ( >> This, IoOperation, Width, >> Address, Count, Buffer >> @@ -753,7 +813,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); >> } >> >> /** >> @@ -788,6 +853,8 @@ RootBridgeIoIoWrite ( >> ) >> { >> EFI_STATUS Status; >> + PCI_ROOT_BRIDGE_INSTANCE *RootBridge; >> + >> Status = RootBridgeIoCheckParameter ( >> This, IoOperation, Width, >> Address, Count, Buffer >> @@ -795,7 +862,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); >> } >> >> /** >> @@ -1615,25 +1687,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; >> 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; >> diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> index d42e9ec..21ee8cd 100644 >> --- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> +++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h >> @@ -20,8 +20,16 @@ >> // (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 >> 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, Translation is probably >> + // negative for we may translate an above-4G host address into a below-4G >> + // device address for legacy PCIe device compatibility. >> + UINT64 Translation; >> } PCI_ROOT_BRIDGE_APERTURE; >> >> typedef struct { >> > > (10) I suggest replacing the "negative" language with "the subtraction > is to be performed with UINT64 wrap-around semantics". > +1 > > I've made some comments, but I admit my understanding is quite limited; > sorry about that. > > Thanks > Laszlo