Re: [PATCH] FIX MinPlatformPkg PCIE Base Addess avoid thunking operation.

["Isaac Oram" <isaac.w.oram@intel.com>]

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.ali.s@intel.com> 
Sent: Sunday, September 18, 2022 10:53 PM
To: devel@edk2.groups.io
Cc: S, Ashraf Ali <ashraf.ali.s@intel.com>; Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@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.ali.s@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@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