From: Guo Heyi <heyi.guo@linaro.org>
To: "Ni, Ruiyu" <ruiyu.ni@Intel.com>
Cc: Guo Heyi <heyi.guo@linaro.org>, Eric Dong <eric.dong@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
edk2-devel@lists.01.org,
Michael D Kinney <michael.d.kinney@intel.com>,
Laszlo Ersek <lersek@redhat.com>, Star Zeng <star.zeng@intel.com>
Subject: Re: [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation
Date: Wed, 28 Feb 2018 15:53:48 +0800 [thread overview]
Message-ID: <20180228075348.GB27903@SZX1000114654> (raw)
In-Reply-To: <17035d2f-3a5d-b3be-149d-d7179f173677@Intel.com>
On Wed, Feb 28, 2018 at 03:25:03PM +0800, Ni, Ruiyu wrote:
> On 2/28/2018 10:29 AM, Ni, Ruiyu wrote:
> >On 2/27/2018 7:44 PM, Guo Heyi wrote:
> >>Hi all,
> >>
> >>I believe we have come to conclusion for major parts, so I'm going to
> >>send out
> >>formal patches instead of RFC from now on, as well as applying changes to
> >>the existing implementations of PciHostBridgeLib in the tree.
> >>
> >>Thanks for your comments and advice.
> >
> >Heyi,
> >I saw you had a very good summary about where the Device Address is used,
> >where the Host Address is used.
> >I think it would be better if you could split them into two categories:
> >Protocol interfaces and internal implementations.
> >
> >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.
> >
> >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.
> >
> >Thanks,
> >Ray
> >
> >>
> >>Heyi
> >>
> >>On Tue, Feb 27, 2018 at 06:45:01PM +0800, Guo Heyi wrote:
> >>>On Tue, Feb 27, 2018 at 05:59:48PM +0800, Ni, Ruiyu wrote:
> >>>>On 2/27/2018 5:33 PM, Guo Heyi wrote:
> >>>>>Hi Ray,
> >>>>>
> >>>>>Thanks for your comments. It seems comments 10 and 12 are the
> >>>>>major issue; let's
> >>>>>discuss that first.
> >>>>>
> >>>>>1. In current implementation, namely PciBusDxe and
> >>>>>PciHostBridgeDxe both in
> >>>>>MdeModulePkg, Address parameter passed to RootBridgeIoMemRead
> >>>>>comes from
> >>>>>PciIoMemRead().
> >>>>>2. In PciIoMemRead(), Offset is only an offset within selected
> >>>>>BAR; then
> >>>>>PciIoDevice->PciBar[BarIndex].BaseAddress is added to Offset in
> >>>>>PciIoVerifyBarAccess().
> >>>>Agree. I thought PciIoMemRead() gets the BAR base address from
> >>>>GetBarAttributes().
> >>>>
> >>>>>3. So whether the "Address" passed to RootBridgeIoMemRead() is
> >>>>>host address or
> >>>>>device address depends on the property of
> >>>>>PciIoDevice->PciBar[BarIndex].BaseAddress.
> >>>>RootBridgeIoMemRead() can choose to accept HOST address or Device
> >>>>address.
> >>>>Either can be implemented.
> >>>>I am fine to your proposal to use Device Address.
> >>>>
> >>>>>4. In PciBusDxe, we can see
> >>>>>PciIoDevice->PciBar[BarIndex].BaseAddress simply
> >>>>>comes from the value read from BAR register, which is absolutely a
> >>>>>device
> >>>>>address.
> >>>>>5. What we do in patch 3/3 is also to convert the device address in
> >>>>>PciBar[BarIndex].BaseAddress to host address before returning to
> >>>>>the caller of
> >>>>>PciIoGetBarAttributes().
> >>>>>
> >>>>>Please let me know your comments.
> >>>>>
> >>>>>For other comments, I have no strong opinion and I can follow the
> >>>>>conclusion of
> >>>>>the community.
> >>>>
> >>>>So the issue (13) (not 12) is closed.
> >>>>But what's your opinion to my comment (1): Explicitly add assertion and
> >>>>if-check to make sure the alignment meets requirement.
> >>>
> >>>Ah, I thought it is obviously a good advice to place the restriction
> >>>in the code
> >>>rather than in documentation only, so I didn't reply it separately :)
> >>>
> >>>Thanks,
> >>>
> >>>Heyi
> >>>
> >>>>
> >>>>Ard,
> >>>>I provided the detailed comments because I felt the code is almost
> >>>>finalized.
> >>>>So the detailed comments can avoid creating more versions of patches.
> >>>>
> >>>>For the EMPTY comments and STATIC modifier, I created the first
> >>>>version of
> >>>>this driver, so I'd like to make the coding style in this driver is
> >>>>consistent.
> >>>>But if you insist to use a different style, I am fine as long as the
> >>>>coding
> >>>>style rule allows.
> >>>>
> >>>>Thanks,
> >>>>Ray
> >>>>
> >>>>>
> >>>>>Thanks and regards,
> >>>>>
> >>>>>Heyi
> >>>>>
> >>>>>On Tue, Feb 27, 2018 at 04:48:47PM +0800, Ni, Ruiyu wrote:
> >>>>>>On 2/27/2018 10:09 AM, Heyi Guo wrote:
> >>>>>>>PCI address translation is necessary for some non-x86 platforms. On
> >>>>>>>such platforms, address value (denoted as "device address" or
> >>>>>>>"address
> >>>>>>>in PCI view") set to PCI BAR registers in configuration space
> >>>>>>>might be
> >>>>>>>different from the address which is used by CPU to access the
> >>>>>>>registers in memory BAR or IO BAR spaces (denoted as "host
> >>>>>>>address" or
> >>>>>>>"address in CPU view"). The difference between the two addresses is
> >>>>>>>called "Address Translation Offset" or simply "translation", and can
> >>>>>>>be represented by "Address Translation Offset" in ACPI QWORD Address
> >>>>>>>Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> >>>>>>>definitions of QWORD Address Space Descriptor, and we will
> >>>>>>>follow UEFI
> >>>>>>>definition on UEFI protocols, such as PCI root bridge IO
> >>>>>>>protocol and
> >>>>>>>PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is
> >>>>>>>"Offset
> >>>>>>>to apply to the Starting address to convert it to a PCI
> >>>>>>>address". This
> >>>>>>>means:
> >>>>>>>
> >>>>>>>1. Translation = device address - host address.
> >>>>>>>
> >>>>>>>2. PciRootBridgeIo->Configuration should return CPU view address, as
> >>>>>>>well as PciIo->GetBarAttributes.
> >>>>>>>
> >>>>>>>Summary of addresses used:
> >>>>>>>
> >>>>>>>1. Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
> >>>>>>>address, for
> >>>>>>>it is easy to check whether the address is below 4G or above 4G.
> >>>>>>>
> >>>>>>>2. Address returned by
> >>>>>>>EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL::GetProposedResources
> >>>>>>>
> >>>>>>>is device address, or else PCI bus driver cannot set correct address
> >>>>>>>into PCI BAR registers.
> >>>>>>>
> >>>>>>>3. Address returned by PciRootBridgeIo->Configuration is host
> >>>>>>>address
> >>>>>>>per UEFI 2.7 definition.
> >>>>>>>
> >>>>>>>4. Addresses used in GCD manipulation are host address.
> >>>>>>>
> >>>>>>>5. Addresses in ResAllocNode of PCI_ROOT_BRIDGE_INSTANCE are host
> >>>>>>>address, for they are allocated from GCD.
> >>>>>>>
> >>>>>>>6. Address passed to PciHostBridgeResourceConflict is host address,
> >>>>>>>for it comes from ResAllocNode.
> >>>>>>>
> >>>>>>>RESTRICTION: to simplify the situation, we require the alignment of
> >>>>>>>Translation must be larger than any BAR alignment in the same root
> >>>>>>>bridge, so that resource allocation alignment can be applied to both
> >>>>>>>device address and host address.
> >>>>>>(1). I'd like to see this restriction be reflected in code.
> >>>>>>Both assertion and if-check are needed to ensure debug firmware hang
> >>>>>>and release firmware return fail status from PciHostBridge driver.
> >>>>>>
> >>>>>>>
> >>>>>>>Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>>>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>>>>>Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >>>>>>>Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>>>>>Cc: Star Zeng <star.zeng@intel.com>
> >>>>>>>Cc: Eric Dong <eric.dong@intel.com>
> >>>>>>>Cc: Laszlo Ersek <lersek@redhat.com>
> >>>>>>>Cc: Michael D Kinney <michael.d.kinney@intel.com>
> >>>>>>>---
> >>>>>>>
> >>>>>>>Notes:
> >>>>>>> v4:
> >>>>>>> - Add ASSERT (FALSE) to default branch in
> >>>>>>>GetTranslationByResourceType
> >>>>>>> [Laszlo]
> >>>>>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
> >>>>>>> gDS->AllocateMemorySpace [Laszlo]
> >>>>>>> - Add comment for applying alignment to both device
> >>>>>>>address and host
> >>>>>>> address, and add NOTE for the alignment requirement of
> >>>>>>>Translation,
> >>>>>>> as well as in commit message [Laszlo][Ray]
> >>>>>>> - Improve indention for the code in CreateRootBridge [Laszlo]
> >>>>>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
> >>>>>>> definition [Laszlo]
> >>>>>>> - Ignore translation of bus in CreateRootBridge
> >>>>>>>
> >>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 2 +
> >>>>>>> MdeModulePkg/Include/Library/PciHostBridgeLib.h | 14 +++
> >>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c |
> >>>>>>>85 +++++++++++---
> >>>>>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c |
> >>>>>>>122 +++++++++++++++++---
> >>>>>>> 4 files changed, 194 insertions(+), 29 deletions(-)
> >>>>>>>
> >>>>>>>diff --git
> >>>>>>>a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>>>>index 8612c0c3251b..662c2dd59529 100644
> >>>>>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>>>>@@ -38,6 +38,8 @@ typedef enum {
> >>>>>>> typedef struct {
> >>>>>>> PCI_RESOURCE_TYPE Type;
> >>>>>>>+ // Base is a host address instead of a device address when
> >>>>>>>address translation
> >>>>>>>+ // exists and host address != device address
> >>>>>>(2). Coding style issue. The comments need to be in style like:
> >>>>>>//[EMPTY]
> >>>>>>// xxxx
> >>>>>>//[EMPTY]
> >>>>>>And how about just simply say Base is a host address. No matter
> >>>>>>address
> >>>>>>translation exists or not, Base is a host address actually.
> >>>>>>
> >>>>>>> UINT64 Base;
> >>>>>>> UINT64 Length;
> >>>>>>> UINT64 Alignment;
> >>>>>>>diff --git a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>>>>>b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>>>>>index d42e9ecdd763..c842a4152e85 100644
> >>>>>>>--- a/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>>>>>+++ b/MdeModulePkg/Include/Library/PciHostBridgeLib.h
> >>>>>>>@@ -20,8 +20,22 @@
> >>>>>>> // (Base > Limit) indicates an aperture is not available.
> >>>>>>> //
> >>>>>>> typedef struct {
> >>>>>>>+ // Base and Limit are the device address instead of host
> >>>>>>>address when
> >>>>>>>+ // Translation is not zero
> >>>>>>(3). Similar comments to (2).
> >>>>>>> UINT64 Base;
> >>>>>>> UINT64 Limit;
> >>>>>>>+ // According to UEFI 2.7, Device Address = Host Address +
> >>>>>>>Translation,
> >>>>>>>+ // so Translation = Device Address - Host Address.
> >>>>>>>+ // On platforms where Translation is not zero, the
> >>>>>>>subtraction is probably to
> >>>>>>>+ // be performed with UINT64 wrap-around semantics, for we
> >>>>>>>may translate an
> >>>>>>>+ // above-4G host address into a below-4G device address for
> >>>>>>>legacy PCIe device
> >>>>>>>+ // compatibility.
> >>>>>>>+ // NOTE: The alignment of Translation is required to be
> >>>>>>>larger than any BAR
> >>>>>>>+ // alignment in the same root bridge, so that the same
> >>>>>>>alignment can be
> >>>>>>>+ // applied to both device address and host address, which
> >>>>>>>simplifies the
> >>>>>>>+ // situation and makes the current resource allocation code
> >>>>>>>in generic PCI
> >>>>>>>+ // host bridge driver still work.
> >>>>>>(4). Still I'd like to see the alignment requirement be
> >>>>>>reflected in code
> >>>>>>through assertion and if-check.
> >>>>>>
> >>>>>>>+ UINT64 Translation;
> >>>>>>> } PCI_ROOT_BRIDGE_APERTURE;
> >>>>>>> typedef struct {
> >>>>>>>diff --git
> >>>>>>>a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>index 1494848c3e8c..1e65faee9084 100644
> >>>>>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>>>>@@ -32,6 +32,30 @@ EDKII_IOMMU_PROTOCOL *mIoMmuProtocol;
> >>>>>>> EFI_EVENT mIoMmuEvent;
> >>>>>>> VOID *mIoMmuRegistration;
> >>>>>>>+STATIC
> >>>>>>(5). Can you remove STATIC? personal preference:)
> >>>>>>>+UINT64
> >>>>>>>+GetTranslationByResourceType (
> >>>>>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>>>>>+ IN PCI_RESOURCE_TYPE ResourceType
> >>>>>>>+ )
> >>>>>>>+{
> >>>>>>>+ switch (ResourceType) {
> >>>>>>>+ case TypeIo:
> >>>>>>>+ return RootBridge->Io.Translation;
> >>>>>>>+ case TypeMem32:
> >>>>>>>+ return RootBridge->Mem.Translation;
> >>>>>>>+ case TypePMem32:
> >>>>>>>+ return RootBridge->PMem.Translation;
> >>>>>>>+ case TypeMem64:
> >>>>>>>+ return RootBridge->MemAbove4G.Translation;
> >>>>>>>+ case TypePMem64:
> >>>>>>>+ return RootBridge->PMemAbove4G.Translation;
> >>>>>>>+ default:
> >>>>>>>+ ASSERT (FALSE);
> >>>>>>>+ return 0;
> >>>>>>>+ }
> >>>>>>>+}
> >>>>>>>+
> >>>>>>> /**
> >>>>>>> Ensure the compatibility of an IO space descriptor with
> >>>>>>>the IO aperture.
> >>>>>>>@@ -366,6 +390,7 @@ InitializePciHostBridge (
> >>>>>>> UINTN MemApertureIndex;
> >>>>>>> BOOLEAN ResourceAssigned;
> >>>>>>> LIST_ENTRY *Link;
> >>>>>>>+ UINT64 TempHostAddress;
> >>>>>>(6). Just use name HostAddress?
> >>>>>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> >>>>>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> >>>>>>>@@ -411,18 +436,24 @@ InitializePciHostBridge (
> >>>>>>> }
> >>>>>>> if (RootBridges[Index].Io.Base <=
> >>>>>>>RootBridges[Index].Io.Limit) {
> >>>>>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are
> >>>>>>>device address.
> >>>>>>>+ // According to UEFI 2.7, device address = host address
> >>>>>>>+ Translation.
> >>>>>>>+ // For GCD resource manipulation, we should use host
> >>>>>>>address, so
> >>>>>>>+ // Translation is subtracted from device address here.
> >>>>>>(7). Please adjust the comment style to have two EMPTY line of
> >>>>>>comment above
> >>>>>>and below. Same to all comments in the patch.
> >>>>>>> Status = AddIoSpace (
> >>>>>>>- RootBridges[Index].Io.Base,
> >>>>>>>+ RootBridges[Index].Io.Base -
> >>>>>>>RootBridges[Index].Io.Translation,
> >>>>>>> RootBridges[Index].Io.Limit -
> >>>>>>>RootBridges[Index].Io.Base + 1
> >>>>>>> );
> >>>>>>> ASSERT_EFI_ERROR (Status);
> >>>>>>> if (ResourceAssigned) {
> >>>>>>>+ TempHostAddress = RootBridges[Index].Io.Base -
> >>>>>>>+ RootBridges[Index].Io.Translation;
> >>>>>>> Status = gDS->AllocateIoSpace (
> >>>>>>> EfiGcdAllocateAddress,
> >>>>>>> EfiGcdIoTypeIo,
> >>>>>>> 0,
> >>>>>>> RootBridges[Index].Io.Limit -
> >>>>>>>RootBridges[Index].Io.Base + 1,
> >>>>>>>- &RootBridges[Index].Io.Base,
> >>>>>>>+ &TempHostAddress,
> >>>>>>> gImageHandle,
> >>>>>>> NULL
> >>>>>>> );
> >>>>>>>@@ -443,14 +474,18 @@ InitializePciHostBridge (
> >>>>>>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE
> >>>>>>>(MemApertures); MemApertureIndex++) {
> >>>>>>> if (MemApertures[MemApertureIndex]->Base <=
> >>>>>>>MemApertures[MemApertureIndex]->Limit) {
> >>>>>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are
> >>>>>>>device address.
> >>>>>>>+ // According to UEFI 2.7, device address = host
> >>>>>>>address + Translation.
> >>>>>>>+ // For GCD resource manipulation, we should use host
> >>>>>>>address, so
> >>>>>>>+ // Translation is subtracted from device address here.
> >>>>>>> Status = AddMemoryMappedIoSpace (
> >>>>>>>- MemApertures[MemApertureIndex]->Base,
> >>>>>>>+ MemApertures[MemApertureIndex]->Base -
> >>>>>>>MemApertures[MemApertureIndex]->Translation,
> >>>>>>> MemApertures[MemApertureIndex]->Limit -
> >>>>>>>MemApertures[MemApertureIndex]->Base + 1,
> >>>>>>> EFI_MEMORY_UC
> >>>>>>> );
> >>>>>>> ASSERT_EFI_ERROR (Status);
> >>>>>>> Status = gDS->SetMemorySpaceAttributes (
> >>>>>>>- MemApertures[MemApertureIndex]->Base,
> >>>>>>>+ MemApertures[MemApertureIndex]->Base
> >>>>>>>- MemApertures[MemApertureIndex]->Translation,
> >>>>>>>
> >>>>>>>MemApertures[MemApertureIndex]->Limit -
> >>>>>>>MemApertures[MemApertureIndex]->Base + 1,
> >>>>>>> EFI_MEMORY_UC
> >>>>>>> );
> >>>>>>>@@ -458,12 +493,14 @@ InitializePciHostBridge (
> >>>>>>> DEBUG ((DEBUG_WARN, "PciHostBridge driver failed
> >>>>>>>to set EFI_MEMORY_UC to MMIO aperture - %r.\n", Status));
> >>>>>>> }
> >>>>>>> if (ResourceAssigned) {
> >>>>>>>+ TempHostAddress = MemApertures[MemApertureIndex]->Base -
> >>>>>>>+ MemApertures[MemApertureIndex]->Translation;
> >>>>>>> Status = gDS->AllocateMemorySpace (
> >>>>>>> EfiGcdAllocateAddress,
> >>>>>>> EfiGcdMemoryTypeMemoryMappedIo,
> >>>>>>> 0,
> >>>>>>>
> >>>>>>>MemApertures[MemApertureIndex]->Limit -
> >>>>>>>MemApertures[MemApertureIndex]->Base + 1,
> >>>>>>>- &MemApertures[MemApertureIndex]->Base,
> >>>>>>>+ &TempHostAddress,
> >>>>>>> gImageHandle,
> >>>>>>> NULL
> >>>>>>> );
> >>>>>>>@@ -654,6 +691,11 @@ AllocateResource (
> >>>>>>> if (BaseAddress < Limit) {
> >>>>>>> //
> >>>>>>> // Have to make sure Aligment is handled since we are
> >>>>>>>doing direct address allocation
> >>>>>>>+ // Strictly speaking, alignment requirement should be
> >>>>>>>applied to device
> >>>>>>>+ // address instead of host address which is used in GCD
> >>>>>>>manipulation below,
> >>>>>>>+ // but as we restrict the alignment of Translation to be
> >>>>>>>larger than any BAR
> >>>>>>>+ // alignment in the root bridge, we can simplify the
> >>>>>>>situation and consider
> >>>>>>>+ // the same alignment requirement is also applied to host
> >>>>>>>address.
> >>>>>>> //
> >>>>>>> BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1,
> >>>>>>>BitsOfAlignment));
> >>>>>>>@@ -824,12 +866,17 @@ NotifyPhase (
> >>>>>>> switch (Index) {
> >>>>>>> case TypeIo:
> >>>>>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are
> >>>>>>>device address.
> >>>>>>>+ // According to UEFI 2.7, device address = host
> >>>>>>>address + Translation.
> >>>>>>>+ // For AllocateResource is manipulating GCD
> >>>>>>>resource, we should use
> >>>>>>>+ // host address here, so Translation is
> >>>>>>>subtracted from Base and
> >>>>>>>+ // Limit.
> >>>>>>> BaseAddress = AllocateResource (
> >>>>>>> FALSE,
> >>>>>>>RootBridge->ResAllocNode[Index].Length,
> >>>>>>> MIN (15, BitsOfAlignment),
> >>>>>>>- ALIGN_VALUE (RootBridge->Io.Base,
> >>>>>>>Alignment + 1),
> >>>>>>>- RootBridge->Io.Limit
> >>>>>>>+ ALIGN_VALUE (RootBridge->Io.Base,
> >>>>>>>Alignment + 1) - RootBridge->Io.Translation,
> >>>>>>>+ RootBridge->Io.Limit -
> >>>>>>>RootBridge->Io.Translation
> >>>>>>> );
> >>>>>>> break;
> >>>>>>>@@ -838,8 +885,8 @@ NotifyPhase (
> >>>>>>(8). Add assertion and if-check here to make sure alignment of
> >>>>>>Translation
> >>>>>>is no less than the Alignment.
> >>>>>>> TRUE,
> >>>>>>>RootBridge->ResAllocNode[Index].Length,
> >>>>>>> MIN (63, BitsOfAlignment),
> >>>>>>>- ALIGN_VALUE
> >>>>>>>(RootBridge->MemAbove4G.Base, Alignment + 1),
> >>>>>>>- RootBridge->MemAbove4G.Limit
> >>>>>>>+ ALIGN_VALUE
> >>>>>>>(RootBridge->MemAbove4G.Base, Alignment + 1) -
> >>>>>>>RootBridge->MemAbove4G.Translation,
> >>>>>>>+ RootBridge->MemAbove4G.Limit -
> >>>>>>>RootBridge->MemAbove4G.Translation
> >>>>>>> );
> >>>>>>> if (BaseAddress != MAX_UINT64) {
> >>>>>>> break;
> >>>>>>>@@ -853,8 +900,8 @@ NotifyPhase (
> >>>>>>> TRUE,
> >>>>>>>RootBridge->ResAllocNode[Index].Length,
> >>>>>>> MIN (31, BitsOfAlignment),
> >>>>>>>- ALIGN_VALUE
> >>>>>>>(RootBridge->Mem.Base, Alignment + 1),
> >>>>>>>- RootBridge->Mem.Limit
> >>>>>>>+ ALIGN_VALUE
> >>>>>>>(RootBridge->Mem.Base, Alignment + 1) -
> >>>>>>>RootBridge->Mem.Translation,
> >>>>>>>+ RootBridge->Mem.Limit -
> >>>>>>>RootBridge->Mem.Translation
> >>>>>>> );
> >>>>>>> break;
> >>>>>>>@@ -863,8 +910,8 @@ NotifyPhase (
> >>>>>>> TRUE,
> >>>>>>>RootBridge->ResAllocNode[Index].Length,
> >>>>>>> MIN (63, BitsOfAlignment),
> >>>>>>>- ALIGN_VALUE
> >>>>>>>(RootBridge->PMemAbove4G.Base, Alignment + 1),
> >>>>>>>- RootBridge->PMemAbove4G.Limit
> >>>>>>>+ ALIGN_VALUE
> >>>>>>>(RootBridge->PMemAbove4G.Base, Alignment + 1) -
> >>>>>>>RootBridge->PMemAbove4G.Translation,
> >>>>>>>+ RootBridge->PMemAbove4G.Limit -
> >>>>>>>RootBridge->PMemAbove4G.Translation
> >>>>>>> );
> >>>>>>> if (BaseAddress != MAX_UINT64) {
> >>>>>>> break;
> >>>>>>>@@ -877,8 +924,8 @@ NotifyPhase (
> >>>>>>> TRUE,
> >>>>>>>RootBridge->ResAllocNode[Index].Length,
> >>>>>>> MIN (31, BitsOfAlignment),
> >>>>>>>- ALIGN_VALUE
> >>>>>>>(RootBridge->PMem.Base, Alignment + 1),
> >>>>>>>- RootBridge->PMem.Limit
> >>>>>>>+ ALIGN_VALUE
> >>>>>>>(RootBridge->PMem.Base, Alignment + 1) -
> >>>>>>>RootBridge->PMem.Translation,
> >>>>>>>+ RootBridge->PMem.Limit -
> >>>>>>>RootBridge->PMem.Translation
> >>>>>>> );
> >>>>>>> break;
> >>>>>>>@@ -1152,6 +1199,7 @@ StartBusEnumeration (
> >>>>>>> Descriptor->AddrSpaceGranularity = 0;
> >>>>>>> Descriptor->AddrRangeMin = RootBridge->Bus.Base;
> >>>>>>> Descriptor->AddrRangeMax = 0;
> >>>>>>>+ // Ignore translation offset for bus
> >>>>>>(9). Per PI spec on StartBusEnumeration, AddrRangeMax is
> >>>>>>ignored. So I don't
> >>>>>>think we need add comments here.
> >>>>>>> Descriptor->AddrTranslationOffset = 0;
> >>>>>>> Descriptor->AddrLen =
> >>>>>>>RootBridge->Bus.Limit - RootBridge->Bus.Base + 1;
> >>>>>>>@@ -1421,7 +1469,12 @@ GetProposedResources (
> >>>>>>> Descriptor->Desc =
> >>>>>>>ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>>>>> Descriptor->Len = sizeof
> >>>>>>>(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
> >>>>>>> Descriptor->GenFlag = 0;
> >>>>>>>- Descriptor->AddrRangeMin =
> >>>>>>>RootBridge->ResAllocNode[Index].Base;
> >>>>>>>+ // AddrRangeMin in Resource Descriptor here should
> >>>>>>>be device address
> >>>>>>>+ // instead of host address, or else PCI bus driver
> >>>>>>>cannot set correct
> >>>>>>>+ // address into PCI BAR registers.
> >>>>>>>+ // Base in ResAllocNode is a host address, so
> >>>>>>>Translation is added.
> >>>>>>>+ Descriptor->AddrRangeMin =
> >>>>>>>RootBridge->ResAllocNode[Index].Base +
> >>>>>>>+ GetTranslationByResourceType (RootBridge, Index);
> >>>>>>(10). Do you think defining a PciHostBridgeDxe internal macro
> >>>>>>TO_HOST_ADDRESS(DeviceAddress, Offset) and
> >>>>>>TO_DEVICE_ADDRESS(HostAddress,
> >>>>>>Offset) is better?
> >>>>>>
> >>>>>>> Descriptor->AddrRangeMax = 0;
> >>>>>>> Descriptor->AddrTranslationOffset = (ResStatus ==
> >>>>>>>ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
> >>>>>>> Descriptor->AddrLen =
> >>>>>>>RootBridge->ResAllocNode[Index].Length;
> >>>>>>>diff --git
> >>>>>>>a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>>>>index dc06c16dc038..edaa0d48a441 100644
> >>>>>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>>>>@@ -86,12 +86,28 @@ CreateRootBridge (
> >>>>>>> (Bridge->AllocationAttributes &
> >>>>>>>EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem
> >>>>>>>" : L"",
> >>>>>>> (Bridge->AllocationAttributes &
> >>>>>>>EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> >>>>>>> ));
> >>>>>>>+ // We don't see any scenario for bus translation, so
> >>>>>>>translation for bus is just ignored.
> >>>>>>> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n",
> >>>>>>>Bridge->Bus.Base, Bridge->Bus.Limit));
> >>>>>>>- DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n",
> >>>>>>>Bridge->Io.Base, Bridge->Io.Limit));
> >>>>>>>- DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n",
> >>>>>>>Bridge->Mem.Base, Bridge->Mem.Limit));
> >>>>>>>- DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n",
> >>>>>>>Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
> >>>>>>>- DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n",
> >>>>>>>Bridge->PMem.Base, Bridge->PMem.Limit));
> >>>>>>>- DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n",
> >>>>>>>Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
> >>>>>>>+ DEBUG ((
> >>>>>>>+ DEBUG_INFO, " Io: %lx - %lx Translation=%lx\n",
> >>>>>>>+ Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
> >>>>>>>+ ));
> >>>>>>>+ DEBUG ((
> >>>>>>>+ DEBUG_INFO, " Mem: %lx - %lx Translation=%lx\n",
> >>>>>>>+ Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
> >>>>>>>+ ));
> >>>>>>>+ DEBUG ((
> >>>>>>>+ DEBUG_INFO, " MemAbove4G: %lx - %lx Translation=%lx\n",
> >>>>>>>+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit,
> >>>>>>>Bridge->MemAbove4G.Translation
> >>>>>>>+ ));
> >>>>>>>+ DEBUG ((
> >>>>>>>+ DEBUG_INFO, " PMem: %lx - %lx Translation=%lx\n",
> >>>>>>>+ Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
> >>>>>>>+ ));
> >>>>>>>+ DEBUG ((
> >>>>>>>+ DEBUG_INFO, " PMemAbove4G: %lx - %lx Translation=%lx\n",
> >>>>>>>+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit,
> >>>>>>>Bridge->PMemAbove4G.Translation
> >>>>>>>+ ));
> >>>>>>> //
> >>>>>>> // Make sure Mem and MemAbove4G apertures are valid
> >>>>>>>@@ -206,7 +222,15 @@ CreateRootBridge (
> >>>>>>> }
> >>>>>>> RootBridge->ResAllocNode[Index].Type = Index;
> >>>>>>> if (Bridge->ResourceAssigned && (Aperture->Limit >=
> >>>>>>>Aperture->Base)) {
> >>>>>>>- RootBridge->ResAllocNode[Index].Base = Aperture->Base;
> >>>>>>>+ // Ignore translation for bus
> >>>>>>(11). How about we only make sure the TranslationOffset is 0 for
> >>>>>>TypeBus
> >>>>>>when getting the aperture from PciHostBridgeLib.
> >>>>>>So that later logic doesn't need to do special handling for the
> >>>>>>TypeBus.
> >>>>>>This way the code can be simpler.
> >>>>>>>+ if (Index == TypeBus) {
> >>>>>>>+ RootBridge->ResAllocNode[Index].Base = Aperture->Base;
> >>>>>>>+ } else {
> >>>>>>>+ // Base in ResAllocNode is a host address, while Base
> >>>>>>>in Aperture is a
> >>>>>>>+ // device address, so translation needs to be subtracted.
> >>>>>>>+ RootBridge->ResAllocNode[Index].Base = Aperture->Base -
> >>>>>>>+ Aperture->Translation;
> >>>>>>(12). Still. Do you think using TO_HOST_ADDRESS() is better?
> >>>>>>>+ }
> >>>>>>> RootBridge->ResAllocNode[Index].Length =
> >>>>>>>Aperture->Limit - Aperture->Base + 1;
> >>>>>>> RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >>>>>>> } else {
> >>>>>>>@@ -403,6 +427,28 @@ RootBridgeIoCheckParameter (
> >>>>>>> return EFI_SUCCESS;
> >>>>>>> }
> >>>>>>>+EFI_STATUS
> >>>>>>>+RootBridgeIoGetMemTranslationByAddress (
> >>>>>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>>>>>+ IN UINT64 Address,
> >>>>>>>+ IN OUT UINT64 *Translation
> >>>>>>>+ )
> >>>>>>>+{
> >>>>>>>+ if (Address >= RootBridge->Mem.Base && Address <=
> >>>>>>>RootBridge->Mem.Limit) {
> >>>>>>>+ *Translation = RootBridge->Mem.Translation;
> >>>>>>>+ } else if (Address >= RootBridge->PMem.Base && Address <=
> >>>>>>>RootBridge->PMem.Limit) {
> >>>>>>>+ *Translation = RootBridge->PMem.Translation;
> >>>>>>>+ } else if (Address >= RootBridge->MemAbove4G.Base &&
> >>>>>>>Address <= RootBridge->MemAbove4G.Limit) {
> >>>>>>>+ *Translation = RootBridge->MemAbove4G.Translation;
> >>>>>>>+ } else if (Address >= RootBridge->PMemAbove4G.Base &&
> >>>>>>>Address <= RootBridge->PMemAbove4G.Limit) {
> >>>>>>>+ *Translation = RootBridge->PMemAbove4G.Translation;
> >>>>>>>+ } else {
> >>>>>>>+ return EFI_INVALID_PARAMETER;
> >>>>>>>+ }
> >>>>>>>+
> >>>>>>>+ return EFI_SUCCESS;
> >>>>>>>+}
> >>>>>>>+
> >>>>>>> /**
> >>>>>>> Polls an address in memory mapped I/O space until an exit
> >>>>>>>condition is met,
> >>>>>>> or a timeout occurs.
> >>>>>>>@@ -658,13 +704,24 @@ RootBridgeIoMemRead (
> >>>>>>> )
> >>>>>>> {
> >>>>>>> EFI_STATUS Status;
> >>>>>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>>>+ UINT64 Translation;
> >>>>>>> Status = RootBridgeIoCheckParameter (This, MemOperation,
> >>>>>>>Width, Address,
> >>>>>>> Count, Buffer);
> >>>>>>> if (EFI_ERROR (Status)) {
> >>>>>>> return Status;
> >>>>>>> }
> >>>>>>>- return mCpuIo->Mem.Read (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>>>>>+
> >>>>>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>>>>+ Status = RootBridgeIoGetMemTranslationByAddress
> >>>>>>>(RootBridge, Address, &Translation);
> >>>>>>(13). Why do you think the Address is a Device Address?
> >>>>>>PciIo.GetBarAttributes() returns a HOST Address and the
> >>>>>>Translation Offset.
> >>>>>>I didn't see you change the PciIoMemRead() implementation.
> >>>>>>So it means the same HOST Address is passed into the this function.
> >>>>>>
> >>>>>>>+ if (EFI_ERROR (Status)) {
> >>>>>>>+ return Status;
> >>>>>>>+ }
> >>>>>>>+
> >>>>>>>+ // Address passed to CpuIo->Mem.Read should be a host
> >>>>>>>address instead of
> >>>>>>>+ // device address, so Translation should be subtracted.
> >>>>>>>+ return mCpuIo->Mem.Read (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation,
> >>>>>>>Count, Buffer);
> >>>>>>> }
> >>>>>>> /**
> >>>>>>>@@ -705,13 +762,24 @@ RootBridgeIoMemWrite (
> >>>>>>> )
> >>>>>>> {
> >>>>>>> EFI_STATUS Status;
> >>>>>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>>>+ UINT64 Translation;
> >>>>>>> Status = RootBridgeIoCheckParameter (This, MemOperation,
> >>>>>>>Width, Address,
> >>>>>>> Count, Buffer);
> >>>>>>> if (EFI_ERROR (Status)) {
> >>>>>>> return Status;
> >>>>>>> }
> >>>>>>>- return mCpuIo->Mem.Write (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>>>>>+
> >>>>>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>>>>+ Status = RootBridgeIoGetMemTranslationByAddress
> >>>>>>>(RootBridge, Address, &Translation);
> >>>>>>>+ if (EFI_ERROR (Status)) {
> >>>>>>>+ return Status;
> >>>>>>>+ }
> >>>>>>>+
> >>>>>>>+ // Address passed to CpuIo->Mem.Write should be a host
> >>>>>>>address instead of
> >>>>>>>+ // device address, so Translation should be subtracted.
> >>>>>>>+ return mCpuIo->Mem.Write (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address - Translation,
> >>>>>>>Count, Buffer);
> >>>>>>(14). Same as (13)
> >>>>>>> }
> >>>>>>> /**
> >>>>>>>@@ -746,6 +814,8 @@ RootBridgeIoIoRead (
> >>>>>>> )
> >>>>>>> {
> >>>>>>> EFI_STATUS Status;
> >>>>>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>>>+
> >>>>>>> Status = RootBridgeIoCheckParameter (
> >>>>>>> This, IoOperation, Width,
> >>>>>>> Address, Count, Buffer
> >>>>>>>@@ -753,7 +823,12 @@ RootBridgeIoIoRead (
> >>>>>>> if (EFI_ERROR (Status)) {
> >>>>>>> return Status;
> >>>>>>> }
> >>>>>>>- return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
> >>>>>>>Width, Address, Count, Buffer);
> >>>>>>>+
> >>>>>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>>>>+
> >>>>>>>+ // Address passed to CpuIo->Io.Read should be a host
> >>>>>>>address instead of
> >>>>>>>+ // device address, so Translation should be subtracted.
> >>>>>>>+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH)
> >>>>>>>Width, Address - RootBridge->Io.Translation, Count, Buffer);
> >>>>>>(15). Same as (13)
> >>>>>>> }
> >>>>>>> /**
> >>>>>>>@@ -788,6 +863,8 @@ RootBridgeIoIoWrite (
> >>>>>>> )
> >>>>>>> {
> >>>>>>> EFI_STATUS Status;
> >>>>>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>>>>+
> >>>>>>> Status = RootBridgeIoCheckParameter (
> >>>>>>> This, IoOperation, Width,
> >>>>>>> Address, Count, Buffer
> >>>>>>>@@ -795,7 +872,12 @@ RootBridgeIoIoWrite (
> >>>>>>> if (EFI_ERROR (Status)) {
> >>>>>>> return Status;
> >>>>>>> }
> >>>>>>>- return mCpuIo->Io.Write (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address, Count, Buffer);
> >>>>>>>+
> >>>>>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>>>>+
> >>>>>>>+ // Address passed to CpuIo->Io.Write should be a host
> >>>>>>>address instead of
> >>>>>>>+ // device address, so Translation should be subtracted.
> >>>>>>>+ return mCpuIo->Io.Write (mCpuIo,
> >>>>>>>(EFI_CPU_IO_PROTOCOL_WIDTH) Width, Address -
> >>>>>>>RootBridge->Io.Translation, Count, Buffer);
> >>>>>>(16). Same as (13)
> >>>>>>> }
> >>>>>>> /**
> >>>>>>>@@ -1615,25 +1697,39 @@ RootBridgeIoConfiguration (
> >>>>>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>>>>> Descriptor->Len = sizeof
> >>>>>>>(EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> >>>>>>>+ // According to UEFI 2.7, RootBridgeIo->Configuration
> >>>>>>>should return address
> >>>>>>>+ // range in CPU view (host address), and
> >>>>>>>ResAllocNode->Base is already a CPU
> >>>>>>>+ // view address (host address).
> >>>>>>> Descriptor->AddrRangeMin = ResAllocNode->Base;
> >>>>>>> Descriptor->AddrRangeMax = ResAllocNode->Base +
> >>>>>>>ResAllocNode->Length - 1;
> >>>>>>> Descriptor->AddrLen = ResAllocNode->Length;
> >>>>>>> switch (ResAllocNode->Type) {
> >>>>>>> case TypeIo:
> >>>>>>>- Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>>>>>+ Descriptor->ResType =
> >>>>>>>ACPI_ADDRESS_SPACE_TYPE_IO;
> >>>>>>>+ Descriptor->AddrTranslationOffset =
> >>>>>>>RootBridge->Io.Translation;
> >>>>>>(17). Can GetTranslationByResourceType() be used to simplify the
> >>>>>>code?
> >>>>>>> break;
> >>>>>>> case TypePMem32:
> >>>>>>>- Descriptor->SpecificFlag =
> >>>>>>>EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>>>>>+ Descriptor->SpecificFlag =
> >>>>>>>EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>>>>>+ Descriptor->AddrTranslationOffset =
> >>>>>>>RootBridge->PMem.Translation;
> >>>>>>>+ Descriptor->ResType =
> >>>>>>>ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>>>>>+ Descriptor->AddrSpaceGranularity = 32;
> >>>>>>>+ break;
> >>>>>>>+
> >>>>>>> case TypeMem32:
> >>>>>>>+ Descriptor->AddrTranslationOffset =
> >>>>>>>RootBridge->Mem.Translation;
> >>>>>>> Descriptor->ResType =
> >>>>>>>ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>>>>> Descriptor->AddrSpaceGranularity = 32;
> >>>>>>> break;
> >>>>>>> case TypePMem64:
> >>>>>>>- Descriptor->SpecificFlag =
> >>>>>>>EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>>>>>+ Descriptor->SpecificFlag =
> >>>>>>>EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE;
> >>>>>>>+ Descriptor->AddrTranslationOffset =
> >>>>>>>RootBridge->PMemAbove4G.Translation;
> >>>>>>>+ Descriptor->ResType =
> >>>>>>>ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>>>>>+ Descriptor->AddrSpaceGranularity = 64;
> >>>>>>> case TypeMem64:
> >>>>>>>+ Descriptor->AddrTranslationOffset =
> >>>>>>>RootBridge->MemAbove4G.Translation;
> >>>>>>> Descriptor->ResType =
> >>>>>>>ACPI_ADDRESS_SPACE_TYPE_MEM;
> >>>>>>> Descriptor->AddrSpaceGranularity = 64;
> >>>>>>> break;
> >>>>>>>
> >>>>>>
> >>>>>>(18). Please remember to run BaseTools/Source/Python/Ecc/Ecc.py
> >>>>>>on the
> >>>>>>PciHostBridgeDxe driver to make sure coding style matches the
> >>>>>>requirement.
> >>>>>>
> >>>>>>--
> >>>>>>Thanks,
> >>>>>>Ray
> >>>>>_______________________________________________
> >>>>>edk2-devel mailing list
> >>>>>edk2-devel@lists.01.org
> >>>>>https://lists.01.org/mailman/listinfo/edk2-devel
> >>>>>
> >>>>
> >>>>
> >>>>--
> >>>>Thanks,
> >>>>Ray
> >
> >
> Heyi,
> My understanding is this whole change is backward compatible.
> Which means an old version of PciHostBridgeLib + new PciHostBridgeLib.h +
> new PciHostBridgeDxe driver won't cause build failure.
> right?
Not really; as Laszlo indicated in one mail, some implementations of
PciHostBridgeLib uses temporary PCI_ROOT_BRIDGE_APERTURE variables and only
initialize Base and Limit fields of the variables, like the code in
https://github.com/tianocore/edk2/blob/master/CorebootPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c#L315
and
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c#L216
and
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Library/PciHostBridgeLib/XenSupport.c#L189
I'm also preparing the patches for these PciHostBridgeLib instances in edk2
tree, which are expected to be committed after PciHostBridgeLib.h change and
before PciHostBridge driver change.
Thanks,
Heyi
>
> --
> Thanks,
> Ray
next prev parent reply other threads:[~2018-02-28 7:47 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-02-27 2:09 [RFC v4 0/3] Add translation support to generic PciHostBridge Heyi Guo
2018-02-27 2:09 ` [RFC v4 1/3] MdeModulePkg/PciHostBridgeDxe: Add support for address translation Heyi Guo
2018-02-27 8:48 ` Ni, Ruiyu
2018-02-27 8:55 ` Ard Biesheuvel
2018-02-27 9:33 ` Guo Heyi
2018-02-27 9:59 ` Ni, Ruiyu
2018-02-27 10:14 ` Ard Biesheuvel
2018-02-27 10:45 ` Guo Heyi
2018-02-27 11:44 ` Guo Heyi
2018-02-28 2:29 ` Ni, Ruiyu
2018-02-28 7:25 ` Ni, Ruiyu
2018-02-28 7:53 ` Guo Heyi [this message]
2018-02-28 9:39 ` Laszlo Ersek
2018-02-28 23:31 ` Guo Heyi
2018-03-01 3:56 ` Guo Heyi
2018-03-01 4:44 ` Ni, Ruiyu
2018-02-27 2:09 ` [RFC v4 2/3] MdeModulePkg/PciBus: convert host address to device address Heyi Guo
2018-02-27 2:09 ` [RFC v4 3/3] MdeModulePkg/PciBus: return CPU address for GetBarAttributes Heyi Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180228075348.GB27903@SZX1000114654 \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox