Ali,
I don't understand what "thunking the address" means. The old code was
typecasting the address which could truncate it. In my experience, when people
say thunk, they refer to a processor environment switch, eg from 64b to 32b.
Other uses should probably be avoided or at least clearly defined. Please
provide a clearer description for this change in patch V2.
The prior code behavior was to make the limit 4GB if the PCIE base address is
over 4GB. I suspect that this was intended.
Your change would make the limit just under the PCIE base address. The
difference is to make the limit above 4GB if the PCIE base is above 4GB.
I am not an expert on this, but the data structure seems to be trying to define
a range below 4GB and there are different ranges for above 4GB. And that
matches my understanding of the way devices request and we allocate PCI
resources. If this range is intended to be allowed to span above 4GB, we
probably should comment the data structure definitions to better explain that.
I further note that the original design seems hacky and confusing. It is
essentially "if the board didn't specify the end, just hack it to PCIE Base or
4GB, whichever is lower." I say hack because there is nothing to guarantee
that none of that space is in use and comments aren't clear that the truncation
is functionally important.
1. I don't think that matches the MinPlatform intent to have simple code that
is easy to understand.
2. I don't think it is good to have two mechanisms for the same thing. Let's
just always tell people they must set the base and limit.
3. I don't see a reason to allow people to explicitly tie the PCIE resource
window to the PCIE base address.
4. I don't think it is good for the range limit to be 4GB or above, as there
are almost always other items near 4GB. APIC and flash, etc.
Please change the code to something like:
if (PcdGet32(PcdPciReservedMemLimit) <= PcdGet32 (PcdPciReservedMemBase)) {
DEBUG ((DEBUG_ERROR, "PciHostBridge: PCIE below 4GB memory range invalid
limit\n"));
ASSERT_EFI_ERROR (EFI_INVALID_PARAMETER);
*Count = 0;
return RootBridge;
}
mRootBridgeTemplate.Mem.Base = PcdGet32 (PcdPciReservedMemBase);
mRootBridgeTemplate.Mem.Limit = PcdGet32 (PcdPciReservedMemLimit);
(note this code is not tested and is intended to be pseudo-code, also note that
some error checking on the other ranges would be good too.)
And update the PCD definitions to remove the comments indicating the limit is
optional and PcdPciExpressBaseAddress will be used if 0.
Thanks,
Isaac
-----Original Message-----
From: S, Ashraf Ali <ashraf.al...@intel.com>
Sent: Sunday, September 18, 2022 10:53 PM
To: devel@edk2.groups.io
Cc: S, Ashraf Ali <ashraf.al...@intel.com>; Ni, Ray <ray...@intel.com>;
Chaganty, Rangasai V <rangasai.v.chaga...@intel.com>; Oram, Isaac W
<isaac.w.o...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; Desimone,
Nathaniel L <nathaniel.l.desim...@intel.com>; Gao, Liming
<gaolim...@byosoft.com.cn>; Dong, Eric <eric.d...@intel.com>
Subject: [PATCH] FIX MinPlatformPkg PCIE Base Addess avoid thunking operation.
thunking the PCIE base address will cause the distruption in the execution flow
when the PCIE base address is 64bit bit.
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4068
Signed-off-by: Ashraf Ali S <ashraf.al...@intel.com>
Cc: Ray Ni <ray...@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com>
Cc: Isaac Oram <isaac.w.o...@intel.com>
Cc: Chasel Chiu <chasel.c...@intel.com>
Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
Cc: Isaac Oram <isaac.w.o...@intel.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Eric Dong <eric.d...@intel.com>
---
.../Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git
a/Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.c
b/Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.c
index 0e3fee28b5..e38975eee5 100644
---
a/Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/PciHostBridgeLibSimple.c
+++ b/Platform/Intel/MinPlatformPkg/Pci/Library/PciHostBridgeLibSimple/P
+++ ciHostBridgeLibSimple.c
@@ -90,7 +90,7 @@ PciHostBridgeGetRootBridges (
if (PcdGet32(PcdPciReservedMemLimit) != 0) {
mRootBridgeTemplate.Mem.Limit = PcdGet32 (PcdPciReservedMemLimit);
} else {
- mRootBridgeTemplate.Mem.Limit = (UINT32) PcdGet64
(PcdPciExpressBaseAddress) - 1;
+ mRootBridgeTemplate.Mem.Limit = PcdGet64 (PcdPciExpressBaseAddress)
+ - 1;
}
mRootBridgeTemplate.MemAbove4G.Base = PcdGet64
(PcdPciReservedMemAbove4GBBase);
--
2.30.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93974): https://edk2.groups.io/g/devel/message/93974
Mute This Topic: https://groups.io/mt/93775914/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
