Hi Ray,
Sorry I never tried Ecc tool before, for there is few documents about it. When I
tried running python BaseTools/Source/Python/Ecc/Ecc.py -t -s xxxx,
it showed me with error message:
ImportError: No module named Common.LongFilePathOs
Then I found there was an executable named "Ecc" in
BaseTools/BinWrappers/PosixLike/, and change the place of "-t" and "-s" after it
prompted:
: error 1003: Invalid value of option
Target [-s] does NOT exist
However, it still failed with error:
RuntimeError: ANTLR version mismatch: The recognizer has been generated with API
V0, but this runtime does not support this.
I could not find the reason for this error, maybe it was caused by incompatible
version of python and antlr3 (mine is Ubuntu 16.04.3 LTS, python 2.7.12,
python-antlr3 3.5.2-1).
Finally I downloaded prebuilt Win32 BaseTools and ran Ecc.exe on Windows. It
success and produced a "Report.csv" with only a titel line. I guess this means
there is no error in the coding style, isn't it?
Please let me know if there is anything wrong with what I did.
Thanks and regards,
Heyi
On Wed, Mar 14, 2018 at 03:57:14PM +0800, Ni, Ruiyu wrote:
> On 3/7/2018 2:01 PM, Guo Heyi wrote:
> >Thanks. Please let me know if any further changes are needed.
> >
> >Regards,
> >
> >Heyi
> >
> >On Wed, Mar 07, 2018 at 12:30:59PM +0800, Ni, Ruiyu wrote:
> >>On 3/6/2018 10:44 AM, Guo Heyi wrote:
> >>>Hi Ray,
> >>>
> >>>Any comments for v5?
> >>
> >>Heyi,
> >>Some backward compatibility concerns were received from internal production
> >>teams. Current change will cause silent failure on old platforms because
> >>TranslationOffset might be random if uninitialized.
> >>I will solve the concern and then send out updates to you, hopefully by end
> >>of next week.
> >>
> >>>
> >>>Regards,
> >>>
> >>>Heyi
> >>>
> >>>On Thu, Mar 01, 2018 at 02:57:22PM +0800, Heyi Guo wrote:
> >>>>PCI address translation is necessary for some non-x86 platforms. On
> >>>>such platforms, address value (denoted as "device address" or "address
> >>>>in PCI view") set to PCI BAR registers in configuration space might be
> >>>>different from the address which is used by CPU to access the
> >>>>registers in memory BAR or IO BAR spaces (denoted as "host address" or
> >>>>"address in CPU view"). The difference between the two addresses is
> >>>>called "Address Translation Offset" or simply "translation", and can
> >>>>be represented by "Address Translation Offset" in ACPI QWORD Address
> >>>>Space Descriptor (Offset 0x1E). However UEFI and ACPI differs on the
> >>>>definitions of QWORD Address Space Descriptor, and we will follow UEFI
> >>>>definition on UEFI protocols, such as PCI root bridge IO protocol and
> >>>>PCI IO protocol. In UEFI 2.7, "Address Translation Offset" is "Offset
> >>>>to apply to the Starting address to convert it to a PCI address". This
> >>>>means:
> >>>>
> >>>>1. Translation = device address - host address.
> >>>>
> >>>>2. PciRootBridgeIo->Configuration should return CPU view address, as
> >>>>well as PciIo->GetBarAttributes.
> >>>>
> >>>>Summary of addresses used in protocol interfaces and internal
> >>>>implementations:
> >>>>
> >>>>1. *Only* the following protocol interfaces assume Address is Device
> >>>> Address:
> >>>>(1). PciHostBridgeResourceAllocation.GetProposedResources()
> >>>> Otherwise PCI bus driver cannot set correct address into PCI
> >>>> BARs.
> >>>>(2). PciRootBridgeIo.Mem.Read() and PciRootBridgeIo.Mem.Write()
> >>>>(3). PciRootBridgeIo.CopyMem()
> >>>>UEFI and PI spec have clear statements for all other protocol
> >>>>interfaces about the address type.
> >>>>
> >>>>2. Library interfaces and internal implementation:
> >>>>(1). Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>> It is easy to check whether the address is below 4G or above 4G.
> >>>>(2). Addresses in PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode are host
> >>>> address, for they are allocated from GCD.
> >>>>(3). Address passed to PciHostBridgeResourceConflict is host address,
> >>>> for it comes from PCI_ROOT_BRIDGE_INSTANCE.ResAllocNode.
> >>>>
> >>>>RESTRICTION: to simplify the situation, we require the alignment of
> >>>>Translation must be larger than any BAR alignment in the same root
> >>>>bridge, so that resource allocation alignment can be applied to both
> >>>>device address and host address.
> >>>>
> >>>>Contributed-under: TianoCore Contribution Agreement 1.1
> >>>>Signed-off-by: Heyi Guo <[email protected]>
> >>>>Cc: Ruiyu Ni <[email protected]>
> >>>>Cc: Ard Biesheuvel <[email protected]>
> >>>>Cc: Star Zeng <[email protected]>
> >>>>Cc: Eric Dong <[email protected]>
> >>>>Cc: Laszlo Ersek <[email protected]>
> >>>>Cc: Michael D Kinney <[email protected]>
> >>>>---
> >>>>
> >>>>Notes:
> >>>> v5:
> >>>> - Add check for the alignment of Translation against the BAR
> >>>> alignment
> >>>> [Ray]
> >>>> - Keep coding style of comments consistent with the context [Ray]
> >>>> - Comment change for Base in PCI_RES_NODE [Ray]
> >>>> - Add macros of TO_HOST_ADDRESS and TO_DEVICE_ADDRESS for address
> >>>> type
> >>>> conversion (After that we can also simply the comments about the
> >>>> calculation [Ray]
> >>>> - Add check for bus translation offset in CreateRootBridge(), making
> >>>> sure it is zero, and unify code logic for all types of resource
> >>>> after that [Ray]
> >>>> - Use GetTranslationByResourceType() to simplify the code in
> >>>> RootBridgeIoConfiguration() (also fix a bug in previous patch
> >>>> version of missing a break after case TypePMem64) [Ray]
> >>>> - Commit message refinement [Ray]
> >>>> v4:
> >>>> - Add ASSERT (FALSE) to default branch in
> >>>> GetTranslationByResourceType
> >>>> [Laszlo]
> >>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
> >>>> gDS->AllocateMemorySpace [Laszlo]
> >>>> - Add comment for applying alignment to both device address and host
> >>>> address, and add NOTE for the alignment requirement of Translation,
> >>>> as well as in commit message [Laszlo][Ray]
> >>>> - Improve indention for the code in CreateRootBridge [Laszlo]
> >>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
> >>>> definition [Laszlo]
> >>>> - Ignore translation of bus in CreateRootBridge
> >>>> v4:
> >>>> - Add ASSERT (FALSE) to default branch in
> >>>> GetTranslationByResourceType
> >>>> [Laszlo]
> >>>> - Fix bug when passing BaseAddress to gDS->AllocateIoSpace and
> >>>> gDS->AllocateMemorySpace [Laszlo]
> >>>> - Add comment for applying alignment to both device address and host
> >>>> address, and add NOTE for the alignment requirement of Translation,
> >>>> as well as in commit message [Laszlo][Ray]
> >>>> - Improve indention for the code in CreateRootBridge [Laszlo]
> >>>> - Improve comment for Translation in PCI_ROOT_BRIDGE_APERTURE
> >>>> definition [Laszlo]
> >>>> - Ignore translation of bus in CreateRootBridge
> >>>>
> >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h | 21 ++++
> >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h | 3 +
> >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c | 129
> >>>> +++++++++++++++++---
> >>>> MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c | 118
> >>>> ++++++++++++++++--
> >>>> 4 files changed, 245 insertions(+), 26 deletions(-)
> >>>>
> >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
> >>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
> >>>>index 9a8ca21f4819..c2791ea5c2a4 100644
> >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
> >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.h
> >>>>@@ -38,6 +38,13 @@ typedef struct {
> >>>> #define PCI_HOST_BRIDGE_FROM_THIS(a) CR (a, PCI_HOST_BRIDGE_INSTANCE,
> >>>> ResAlloc, PCI_HOST_BRIDGE_SIGNATURE)
> >>>> //
> >>>>+// Macros to translate device address to host address and vice versa.
> >>>>According
> >>>>+// to UEFI 2.7, device address = host address + translation offset.
> >>>>+//
> >>>>+#define TO_HOST_ADDRESS(DeviceAddress,TranslationOffset)
> >>>>((DeviceAddress) - (TranslationOffset))
> >>>>+#define TO_DEVICE_ADDRESS(HostAddress,TranslationOffset) ((HostAddress)
> >>>>+ (TranslationOffset))
> >>>>+
> >>>>+//
> >>>> // Driver Entry Point
> >>>> //
> >>>> /**
> >>>>@@ -247,6 +254,20 @@ ResourceConflict (
> >>>> IN PCI_HOST_BRIDGE_INSTANCE *HostBridge
> >>>> );
> >>>>+/**
> >>>>+ This routine gets translation offset from a root bridge instance by
> >>>>resource type.
> >>>>+
> >>>>+ @param RootBridge The Root Bridge Instance for the resources.
> >>>>+ @param ResourceType The Resource Type of the translation offset.
> >>>>+
> >>>>+ @retval The Translation Offset of the specified resource.
> >>>>+**/
> >>>>+UINT64
> >>>>+GetTranslationByResourceType (
> >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>>+ IN PCI_RESOURCE_TYPE ResourceType
> >>>>+ );
> >>>>+
> >>>> extern EFI_METRONOME_ARCH_PROTOCOL *mMetronome;
> >>>> extern EFI_CPU_IO2_PROTOCOL *mCpuIo;
> >>>> #endif
> >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>index 8612c0c3251b..a6c3739db368 100644
> >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostResource.h
> >>>>@@ -38,6 +38,9 @@ typedef enum {
> >>>> typedef struct {
> >>>> PCI_RESOURCE_TYPE Type;
> >>>>+ //
> >>>>+ // Base is a host address
> >>>>+ //
> >>>> UINT64 Base;
> >>>> UINT64 Length;
> >>>> UINT64 Alignment;
> >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>index 1494848c3e8c..42ded2855c71 100644
> >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridge.c
> >>>>@@ -33,6 +33,39 @@ EFI_EVENT mIoMmuEvent;
> >>>> VOID *mIoMmuRegistration;
> >>>> /**
> >>>>+ This routine gets translation offset from a root bridge instance by
> >>>>resource type.
> >>>>+
> >>>>+ @param RootBridge The Root Bridge Instance for the resources.
> >>>>+ @param ResourceType The Resource Type of the translation offset.
> >>>>+
> >>>>+ @retval The Translation Offset of the specified resource.
> >>>>+**/
> >>>>+UINT64
> >>>>+GetTranslationByResourceType (
> >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>>+ IN PCI_RESOURCE_TYPE ResourceType
> >>>>+ )
> >>>>+{
> >>>>+ switch (ResourceType) {
> >>>>+ case TypeIo:
> >>>>+ return RootBridge->Io.Translation;
> >>>>+ case TypeMem32:
> >>>>+ return RootBridge->Mem.Translation;
> >>>>+ case TypePMem32:
> >>>>+ return RootBridge->PMem.Translation;
> >>>>+ case TypeMem64:
> >>>>+ return RootBridge->MemAbove4G.Translation;
> >>>>+ case TypePMem64:
> >>>>+ return RootBridge->PMemAbove4G.Translation;
> >>>>+ case TypeBus:
> >>>>+ return RootBridge->Bus.Translation;
> >>>>+ default:
> >>>>+ ASSERT (FALSE);
> >>>>+ return 0;
> >>>>+ }
> >>>>+}
> >>>>+
> >>>>+/**
> >>>> Ensure the compatibility of an IO space descriptor with the IO
> >>>> aperture.
> >>>> The IO space descriptor can come from the GCD IO space map, or it can
> >>>>@@ -366,6 +399,7 @@ InitializePciHostBridge (
> >>>> UINTN MemApertureIndex;
> >>>> BOOLEAN ResourceAssigned;
> >>>> LIST_ENTRY *Link;
> >>>>+ UINT64 HostAddress;
> >>>> RootBridges = PciHostBridgeGetRootBridges (&RootBridgeCount);
> >>>> if ((RootBridges == NULL) || (RootBridgeCount == 0)) {
> >>>>@@ -411,8 +445,15 @@ InitializePciHostBridge (
> >>>> }
> >>>> if (RootBridges[Index].Io.Base <= RootBridges[Index].Io.Limit) {
> >>>>+ //
> >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>>+ // For GCD resource manipulation, we need to use host address.
> >>>>+ //
> >>>>+ HostAddress = TO_HOST_ADDRESS (RootBridges[Index].Io.Base,
> >>>>+ RootBridges[Index].Io.Translation);
> >>>>+
> >>>> Status = AddIoSpace (
> >>>>- RootBridges[Index].Io.Base,
> >>>>+ HostAddress,
> >>>> RootBridges[Index].Io.Limit -
> >>>> RootBridges[Index].Io.Base + 1
> >>>> );
> >>>> ASSERT_EFI_ERROR (Status);
> >>>>@@ -422,7 +463,7 @@ InitializePciHostBridge (
> >>>> EfiGcdIoTypeIo,
> >>>> 0,
> >>>> RootBridges[Index].Io.Limit -
> >>>> RootBridges[Index].Io.Base + 1,
> >>>>- &RootBridges[Index].Io.Base,
> >>>>+ &HostAddress,
> >>>> gImageHandle,
> >>>> NULL
> >>>> );
> >>>>@@ -443,14 +484,20 @@ InitializePciHostBridge (
> >>>> for (MemApertureIndex = 0; MemApertureIndex < ARRAY_SIZE
> >>>> (MemApertures); MemApertureIndex++) {
> >>>> if (MemApertures[MemApertureIndex]->Base <=
> >>>> MemApertures[MemApertureIndex]->Limit) {
> >>>>+ //
> >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device address.
> >>>>+ // For GCD resource manipulation, we need to use host address.
> >>>>+ //
> >>>>+ HostAddress = TO_HOST_ADDRESS
> >>>>(MemApertures[MemApertureIndex]->Base,
> >>>>+ MemApertures[MemApertureIndex]->Translation);
> >>>> Status = AddMemoryMappedIoSpace (
> >>>>- MemApertures[MemApertureIndex]->Base,
> >>>>+ HostAddress,
> >>>> MemApertures[MemApertureIndex]->Limit -
> >>>> MemApertures[MemApertureIndex]->Base + 1,
> >>>> EFI_MEMORY_UC
> >>>> );
> >>>> ASSERT_EFI_ERROR (Status);
> >>>> Status = gDS->SetMemorySpaceAttributes (
> >>>>- MemApertures[MemApertureIndex]->Base,
> >>>>+ HostAddress,
> >>>> MemApertures[MemApertureIndex]->Limit -
> >>>> MemApertures[MemApertureIndex]->Base + 1,
> >>>> EFI_MEMORY_UC
> >>>> );
> >>>>@@ -463,7 +510,7 @@ InitializePciHostBridge (
> >>>> EfiGcdMemoryTypeMemoryMappedIo,
> >>>> 0,
> >>>> MemApertures[MemApertureIndex]->Limit -
> >>>> MemApertures[MemApertureIndex]->Base + 1,
> >>>>- &MemApertures[MemApertureIndex]->Base,
> >>>>+ &HostAddress,
> >>>> gImageHandle,
> >>>> NULL
> >>>> );
> >>>>@@ -654,6 +701,11 @@ AllocateResource (
> >>>> if (BaseAddress < Limit) {
> >>>> //
> >>>> // Have to make sure Aligment is handled since we are doing direct
> >>>> address allocation
> >>>>+ // Strictly speaking, alignment requirement should be applied to
> >>>>device
> >>>>+ // address instead of host address which is used in GCD manipulation
> >>>>below,
> >>>>+ // but as we restrict the alignment of Translation to be larger than
> >>>>any BAR
> >>>>+ // alignment in the root bridge, we can simplify the situation and
> >>>>consider
> >>>>+ // the same alignment requirement is also applied to host address.
> >>>> //
> >>>> BaseAddress = ALIGN_VALUE (BaseAddress, LShiftU64 (1,
> >>>> BitsOfAlignment));
> >>>>@@ -721,6 +773,7 @@ NotifyPhase (
> >>>> PCI_RESOURCE_TYPE Index2;
> >>>> BOOLEAN ResNodeHandled[TypeMax];
> >>>> UINT64 MaxAlignment;
> >>>>+ UINT64 Translation;
> >>>> HostBridge = PCI_HOST_BRIDGE_FROM_THIS (This);
> >>>>@@ -822,14 +875,43 @@ NotifyPhase (
> >>>> BitsOfAlignment = LowBitSet64 (Alignment + 1);
> >>>> BaseAddress = MAX_UINT64;
> >>>>+ //
> >>>>+ // RESTRICTION: To simplify the situation, we require the
> >>>>alignment of
> >>>>+ // Translation must be larger than any BAR alignment in the
> >>>>same root
> >>>>+ // bridge, so that resource allocation alignment can be
> >>>>applied to
> >>>>+ // both device address and host address.
> >>>>+ //
> >>>>+ Translation = GetTranslationByResourceType (RootBridge, Index);
> >>>>+ if ((Translation & Alignment) != 0) {
> >>>>+ DEBUG ((DEBUG_ERROR, "[%a:%d] Translation %lx is not aligned
> >>>>to %lx!\n",
> >>>>+ __FUNCTION__, __LINE__, Translation, Alignment
> >>>>+ ));
> >>>>+ ASSERT (FALSE);
> 1. Cool! But can you replace ASSERT(FALSE) with
> ASSERT ((Translation & Alignment) == 0)?
> >>>>+ //
> >>>>+ // This may be caused by too large alignment or too small
> >>>>+ // Translation; pick the 1st possibility and return out of
> >>>>resource,
> >>>>+ // which can also go thru the same process for out of
> >>>>resource
> >>>>+ // outside the loop.
> >>>>+ //
> >>>>+ ReturnStatus = EFI_OUT_OF_RESOURCES;
> >>>>+ continue;
> >>>>+ }
> >>>>+
> >>>> switch (Index) {
> >>>> case TypeIo:
> >>>>+ //
> >>>>+ // Base and Limit in PCI_ROOT_BRIDGE_APERTURE are device
> >>>>address.
> >>>>+ // For AllocateResource is manipulating GCD resource, we
> >>>>need to use
> >>>>+ // host address here.
> >>>>+ //
> >>>> BaseAddress = AllocateResource (
> >>>> FALSE,
> >>>> RootBridge->ResAllocNode[Index].Length,
> >>>> MIN (15, BitsOfAlignment),
> >>>>- ALIGN_VALUE (RootBridge->Io.Base, Alignment
> >>>>+ 1),
> >>>>- RootBridge->Io.Limit
> >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE
> >>>>(RootBridge->Io.Base, Alignment + 1),
> >>>>+ RootBridge->Io.Translation),
> >>>>+ TO_HOST_ADDRESS (RootBridge->Io.Limit,
> >>>>+ RootBridge->Io.Translation)
> >>>> );
> >>>> break;
> >>>>@@ -838,8 +920,10 @@ NotifyPhase (
> >>>> TRUE,
> >>>> RootBridge->ResAllocNode[Index].Length,
> >>>> MIN (63, BitsOfAlignment),
> >>>>- ALIGN_VALUE (RootBridge->MemAbove4G.Base,
> >>>>Alignment + 1),
> >>>>- RootBridge->MemAbove4G.Limit
> >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE
> >>>>(RootBridge->MemAbove4G.Base, Alignment + 1),
> >>>>+ RootBridge->MemAbove4G.Translation),
> >>>>+ TO_HOST_ADDRESS
> >>>>(RootBridge->MemAbove4G.Limit,
> >>>>+ RootBridge->MemAbove4G.Translation)
> >>>> );
> >>>> if (BaseAddress != MAX_UINT64) {
> >>>> break;
> >>>>@@ -853,8 +937,10 @@ NotifyPhase (
> >>>> TRUE,
> >>>> RootBridge->ResAllocNode[Index].Length,
> >>>> MIN (31, BitsOfAlignment),
> >>>>- ALIGN_VALUE (RootBridge->Mem.Base, Alignment
> >>>>+ 1),
> >>>>- RootBridge->Mem.Limit
> >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE
> >>>>(RootBridge->Mem.Base, Alignment + 1),
> >>>>+ RootBridge->Mem.Translation),
> >>>>+ TO_HOST_ADDRESS (RootBridge->Mem.Limit,
> >>>>+ RootBridge->Mem.Translation)
> >>>> );
> >>>> break;
> >>>>@@ -863,8 +949,10 @@ NotifyPhase (
> >>>> TRUE,
> >>>> RootBridge->ResAllocNode[Index].Length,
> >>>> MIN (63, BitsOfAlignment),
> >>>>- ALIGN_VALUE (RootBridge->PMemAbove4G.Base,
> >>>>Alignment + 1),
> >>>>- RootBridge->PMemAbove4G.Limit
> >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE
> >>>>(RootBridge->PMemAbove4G.Base, Alignment + 1),
> >>>>+ RootBridge->PMemAbove4G.Translation),
> >>>>+ TO_HOST_ADDRESS
> >>>>(RootBridge->PMemAbove4G.Limit,
> >>>>+ RootBridge->PMemAbove4G.Translation)
> >>>> );
> >>>> if (BaseAddress != MAX_UINT64) {
> >>>> break;
> >>>>@@ -877,8 +965,10 @@ NotifyPhase (
> >>>> TRUE,
> >>>> RootBridge->ResAllocNode[Index].Length,
> >>>> MIN (31, BitsOfAlignment),
> >>>>- ALIGN_VALUE (RootBridge->PMem.Base,
> >>>>Alignment + 1),
> >>>>- RootBridge->PMem.Limit
> >>>>+ TO_HOST_ADDRESS (ALIGN_VALUE
> >>>>(RootBridge->PMem.Base, Alignment + 1),
> >>>>+ RootBridge->PMem.Translation),
> >>>>+ TO_HOST_ADDRESS (RootBridge->PMem.Limit,
> >>>>+ RootBridge->PMem.Translation)
> >>>> );
> >>>> break;
> >>>>@@ -1421,7 +1511,14 @@ GetProposedResources (
> >>>> Descriptor->Desc =
> >>>> ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>> Descriptor->Len = sizeof
> >>>> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;;
> >>>> Descriptor->GenFlag = 0;
> >>>>- Descriptor->AddrRangeMin =
> >>>>RootBridge->ResAllocNode[Index].Base;
> >>>>+ //
> >>>>+ // AddrRangeMin in Resource Descriptor here should be device
> >>>>address
> >>>>+ // instead of host address, or else PCI bus driver cannot set
> >>>>correct
> >>>>+ // address into PCI BAR registers.
> >>>>+ // Base in ResAllocNode is a host address, so conversion is
> >>>>needed.
> >>>>+ //
> >>>>+ Descriptor->AddrRangeMin = TO_DEVICE_ADDRESS
> >>>>(RootBridge->ResAllocNode[Index].Base,
> >>>>+ GetTranslationByResourceType (RootBridge, Index));
> >>>> Descriptor->AddrRangeMax = 0;
> >>>> Descriptor->AddrTranslationOffset = (ResStatus ==
> >>>> ResAllocated) ? EFI_RESOURCE_SATISFIED : PCI_RESOURCE_LESS;
> >>>> Descriptor->AddrLen =
> >>>> RootBridge->ResAllocNode[Index].Length;
> >>>>diff --git a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>index dc06c16dc038..5dd9d257d46d 100644
> >>>>--- a/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>+++ b/MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciRootBridgeIo.c
> >>>>@@ -86,12 +86,35 @@ CreateRootBridge (
> >>>> (Bridge->AllocationAttributes &
> >>>> EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM) != 0 ? L"CombineMemPMem " : L"",
> >>>> (Bridge->AllocationAttributes &
> >>>> EFI_PCI_HOST_BRIDGE_MEM64_DECODE) != 0 ? L"Mem64Decode" : L""
> >>>> ));
> >>>>+ //
> >>>>+ // Translation for bus is not supported.
> >>>>+ //
> >>>> DEBUG ((EFI_D_INFO, " Bus: %lx - %lx\n", Bridge->Bus.Base,
> >>>> Bridge->Bus.Limit));
> >>>>- DEBUG ((EFI_D_INFO, " Io: %lx - %lx\n", Bridge->Io.Base,
> >>>>Bridge->Io.Limit));
> >>>>- DEBUG ((EFI_D_INFO, " Mem: %lx - %lx\n", Bridge->Mem.Base,
> >>>>Bridge->Mem.Limit));
> >>>>- DEBUG ((EFI_D_INFO, " MemAbove4G: %lx - %lx\n",
> >>>>Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit));
> >>>>- DEBUG ((EFI_D_INFO, " PMem: %lx - %lx\n", Bridge->PMem.Base,
> >>>>Bridge->PMem.Limit));
> >>>>- DEBUG ((EFI_D_INFO, " PMemAbove4G: %lx - %lx\n",
> >>>>Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit));
> >>>>+ ASSERT (Bridge->Bus.Translation == 0);
> >>>>+ if (Bridge->Bus.Translation != 0) {
> >>>>+ return NULL;
> >>>>+ }
>
> 2. Can you please use the same debug message format for Bus range? I mean to
> print the Translation for Bus as well. Then in the end of debug message, use
> assertion and if-check. This way developer can know what the Bus.Translation
> is from the debug log.
>
> >>>>+
> >>>>+ DEBUG ((
> >>>>+ DEBUG_INFO, " Io: %lx - %lx Translation=%lx\n",
> >>>>+ Bridge->Io.Base, Bridge->Io.Limit, Bridge->Io.Translation
> >>>>+ ));
> >>>>+ DEBUG ((
> >>>>+ DEBUG_INFO, " Mem: %lx - %lx Translation=%lx\n",
> >>>>+ Bridge->Mem.Base, Bridge->Mem.Limit, Bridge->Mem.Translation
> >>>>+ ));
> >>>>+ DEBUG ((
> >>>>+ DEBUG_INFO, " MemAbove4G: %lx - %lx Translation=%lx\n",
> >>>>+ Bridge->MemAbove4G.Base, Bridge->MemAbove4G.Limit,
> >>>>Bridge->MemAbove4G.Translation
> >>>>+ ));
> >>>>+ DEBUG ((
> >>>>+ DEBUG_INFO, " PMem: %lx - %lx Translation=%lx\n",
> >>>>+ Bridge->PMem.Base, Bridge->PMem.Limit, Bridge->PMem.Translation
> >>>>+ ));
> >>>>+ DEBUG ((
> >>>>+ DEBUG_INFO, " PMemAbove4G: %lx - %lx Translation=%lx\n",
> >>>>+ Bridge->PMemAbove4G.Base, Bridge->PMemAbove4G.Limit,
> >>>>Bridge->PMemAbove4G.Translation
> >>>>+ ));
> >>>> //
> >>>> // Make sure Mem and MemAbove4G apertures are valid
> >>>>@@ -206,7 +229,12 @@ CreateRootBridge (
> >>>> }
> >>>> RootBridge->ResAllocNode[Index].Type = Index;
> >>>> if (Bridge->ResourceAssigned && (Aperture->Limit >=
> >>>> Aperture->Base)) {
> >>>>- RootBridge->ResAllocNode[Index].Base = Aperture->Base;
> >>>>+ //
> >>>>+ // Base in ResAllocNode is a host address, while Base in Aperture
> >>>>is a
> >>>>+ // device address.
> >>>>+ //
> >>>>+ RootBridge->ResAllocNode[Index].Base = TO_HOST_ADDRESS
> >>>>(Aperture->Base,
> >>>>+ Aperture->Translation);
> >>>> RootBridge->ResAllocNode[Index].Length = Aperture->Limit -
> >>>> Aperture->Base + 1;
> >>>> RootBridge->ResAllocNode[Index].Status = ResAllocated;
> >>>> } else {
> >>>>@@ -403,6 +431,28 @@ RootBridgeIoCheckParameter (
> >>>> return EFI_SUCCESS;
> >>>> }
>
> 3. Please add the missing function header comments.
>
> >>>>+EFI_STATUS
> >>>>+RootBridgeIoGetMemTranslationByAddress (
> >>>>+ IN PCI_ROOT_BRIDGE_INSTANCE *RootBridge,
> >>>>+ IN UINT64 Address,
> >>>>+ IN OUT UINT64 *Translation
> >>>>+ )
> >>>>+{
> >>>>+ if (Address >= RootBridge->Mem.Base && Address <=
> >>>>RootBridge->Mem.Limit) {
> >>>>+ *Translation = RootBridge->Mem.Translation;
> >>>>+ } else if (Address >= RootBridge->PMem.Base && Address <=
> >>>>RootBridge->PMem.Limit) {
> >>>>+ *Translation = RootBridge->PMem.Translation;
> >>>>+ } else if (Address >= RootBridge->MemAbove4G.Base && Address <=
> >>>>RootBridge->MemAbove4G.Limit) {
> >>>>+ *Translation = RootBridge->MemAbove4G.Translation;
> >>>>+ } else if (Address >= RootBridge->PMemAbove4G.Base && Address <=
> >>>>RootBridge->PMemAbove4G.Limit) {
> >>>>+ *Translation = RootBridge->PMemAbove4G.Translation;
> >>>>+ } else {
> >>>>+ return EFI_INVALID_PARAMETER;
> >>>>+ }
> >>>>+
> >>>>+ return EFI_SUCCESS;
> >>>>+}
> >>>>+
> >>>> /**
> >>>> Polls an address in memory mapped I/O space until an exit condition
> >>>> is met,
> >>>> or a timeout occurs.
> >>>>@@ -658,13 +708,25 @@ RootBridgeIoMemRead (
> >>>> )
> >>>> {
> >>>> EFI_STATUS Status;
> >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>+ UINT64 Translation;
> >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width,
> >>>> Address,
> >>>> Count, Buffer);
> >>>> if (EFI_ERROR (Status)) {
> >>>> return Status;
> >>>> }
> >>>>- return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>Address, Count, Buffer);
> >>>>+
> >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address,
> >>>>&Translation);
> >>>>+ if (EFI_ERROR (Status)) {
> >>>>+ return Status;
> >>>>+ }
> >>>>+
> >>>>+ // Address passed to CpuIo->Mem.Read needs to be a host address
> >>>>instead of
> >>>>+ // device address.
> >>>>+ return mCpuIo->Mem.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>+ TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
> >>>> }
> >>>> /**
> >>>>@@ -705,13 +767,25 @@ RootBridgeIoMemWrite (
> >>>> )
> >>>> {
> >>>> EFI_STATUS Status;
> >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>+ UINT64 Translation;
> >>>> Status = RootBridgeIoCheckParameter (This, MemOperation, Width,
> >>>> Address,
> >>>> Count, Buffer);
> >>>> if (EFI_ERROR (Status)) {
> >>>> return Status;
> >>>> }
> >>>>- return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>Address, Count, Buffer);
> >>>>+
> >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>+ Status = RootBridgeIoGetMemTranslationByAddress (RootBridge, Address,
> >>>>&Translation);
> >>>>+ if (EFI_ERROR (Status)) {
> >>>>+ return Status;
> >>>>+ }
> >>>>+
> >>>>+ // Address passed to CpuIo->Mem.Write needs to be a host address
> >>>>instead of
> >>>>+ // device address.
> >>>>+ return mCpuIo->Mem.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>+ TO_HOST_ADDRESS (Address, Translation), Count, Buffer);
> >>>> }
> >>>> /**
> >>>>@@ -746,6 +820,8 @@ RootBridgeIoIoRead (
> >>>> )
> >>>> {
> >>>> EFI_STATUS Status;
> >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>+
> >>>> Status = RootBridgeIoCheckParameter (
> >>>> This, IoOperation, Width,
> >>>> Address, Count, Buffer
> >>>>@@ -753,7 +829,13 @@ RootBridgeIoIoRead (
> >>>> if (EFI_ERROR (Status)) {
> >>>> return Status;
> >>>> }
> >>>>- return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>Address, Count, Buffer);
> >>>>+
> >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>+
> >>>>+ // Address passed to CpuIo->Io.Read needs to be a host address instead
> >>>>of
> >>>>+ // device address.
> >>>>+ return mCpuIo->Io.Read (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>+ TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count,
> >>>>Buffer);
> >>>> }
> >>>> /**
> >>>>@@ -788,6 +870,8 @@ RootBridgeIoIoWrite (
> >>>> )
> >>>> {
> >>>> EFI_STATUS Status;
> >>>>+ PCI_ROOT_BRIDGE_INSTANCE *RootBridge;
> >>>>+
> >>>> Status = RootBridgeIoCheckParameter (
> >>>> This, IoOperation, Width,
> >>>> Address, Count, Buffer
> >>>>@@ -795,7 +879,13 @@ RootBridgeIoIoWrite (
> >>>> if (EFI_ERROR (Status)) {
> >>>> return Status;
> >>>> }
> >>>>- return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>Address, Count, Buffer);
> >>>>+
> >>>>+ RootBridge = ROOT_BRIDGE_FROM_THIS (This);
> >>>>+
> >>>>+ // Address passed to CpuIo->Io.Write needs to be a host address
> >>>>instead of
> >>>>+ // device address.
> >>>>+ return mCpuIo->Io.Write (mCpuIo, (EFI_CPU_IO_PROTOCOL_WIDTH) Width,
> >>>>+ TO_HOST_ADDRESS (Address, RootBridge->Io.Translation), Count,
> >>>>Buffer);
> >>>> }
> >>>> /**
> >>>>@@ -1615,9 +1705,17 @@ RootBridgeIoConfiguration (
> >>>> Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR;
> >>>> Descriptor->Len = sizeof (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3;
> >>>>+ // According to UEFI 2.7, RootBridgeIo->Configuration should return
> >>>>address
> >>>>+ // range in CPU view (host address), and ResAllocNode->Base is
> >>>>already a CPU
> >>>>+ // view address (host address).
> >>>> Descriptor->AddrRangeMin = ResAllocNode->Base;
> >>>> Descriptor->AddrRangeMax = ResAllocNode->Base +
> >>>> ResAllocNode->Length - 1;
> >>>> Descriptor->AddrLen = ResAllocNode->Length;
> >>>>+ Descriptor->AddrTranslationOffset = GetTranslationByResourceType (
> >>>>+ RootBridge,
> >>>>+ ResAllocNode->Type
> >>>>+ );
> >>>>+
> >>>> switch (ResAllocNode->Type) {
> >>>> case TypeIo:
> >>>>--
> >>>>2.7.4
> >>>>
> >>
> >>
> >>--
> >>Thanks,
> >>Ray
>
>
> Reviewed-by: Ruiyu Ni <[email protected]>
> Please modify code according to the above 3 comments and make sure
> the changes pass ECC check:
> BaseTools/Source/Python/Ecc/Ecc.py -t -s \
> MdeModulePkg/Pci/PciHostBridgeDxe
>
> >_______________________________________________
> >edk2-devel mailing list
> >[email protected]
> >https://lists.01.org/mailman/listinfo/edk2-devel
> >
>
>
> --
> Thanks,
> Ray
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
