Re: [PATCH 07/10] EmulatorPkg/AutoScanPei: Report the correct CPU address size

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 8/29/2018 9:54 AM, Wu, Hao A wrote:
>> -----Original Message-----
>> From: Ni, Ruiyu
>> Sent: Monday, August 27, 2018 3:53 PM
>> To: edk2-devel@lists.01.org
>> Cc: Wu, Hao A; Andrew Fish
>> Subject: [PATCH 07/10] EmulatorPkg/AutoScanPei: Report the correct CPU
>> address size
>>
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1119
>>
>> Today's implementation unconditionally reports CPU address size
>> as 36 through CPU HOB. But when WinHost is running at 64bit,
>> the system memory might be allocated above 2^36.
>>
>> It causes system asserts when DxeCore code tries to find the
>> corresponding GCD entry for a given valid address.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Andrew Fish <afish@apple.com>
>> ---
>>   EmulatorPkg/AutoScanPei/AutoScanPei.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/EmulatorPkg/AutoScanPei/AutoScanPei.c
>> b/EmulatorPkg/AutoScanPei/AutoScanPei.c
>> index 78a40db3a2..bf9958a4a9 100644
>> --- a/EmulatorPkg/AutoScanPei/AutoScanPei.c
>> +++ b/EmulatorPkg/AutoScanPei/AutoScanPei.c
>> @@ -1,6 +1,6 @@
>>   /*++ @file
>>
>> -Copyright (c) 2006 - 2008, Intel Corporation. All rights reserved.<BR>
>> +Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>>   Portions copyright (c) 2011, Apple Inc. All rights reserved.
>>   This program and the accompanying materials
>>   are licensed and made available under the terms and conditions of the BSD
>> License
>> @@ -51,7 +51,8 @@ Returns:
>>     EFI_PHYSICAL_ADDRESS        MemoryBase;
>>     UINTN                       Index;
>>     EFI_RESOURCE_ATTRIBUTE_TYPE Attributes;
>> -
>> +  UINTN                       HighBitSet;
>> +  UINT8                       SizeOfMemorySpace;
>>
>>     DEBUG ((EFI_D_ERROR, "Emu Autoscan PEIM Loaded\n"));
>>
>> @@ -66,7 +67,8 @@ Returns:
>>                );
>>     ASSERT_EFI_ERROR (Status);
>>
>> -  Index = 0;
>> +  SizeOfMemorySpace = 0;
>> +  Index             = 0;
>>     do {
>>       Status = Thunk->MemoryAutoScan (Index, &MemoryBase, &MemorySize);
>>       if (!EFI_ERROR (Status)) {
>> @@ -96,6 +98,12 @@ Returns:
>>           MemoryBase,
>>           MemorySize
>>           );
>> +      HighBitSet        = HighBitSet64 (MemoryBase + MemorySize);
>> +      SizeOfMemorySpace = MAX (SizeOfMemorySpace, (UINT8)HighBitSet + 1);
>> +      DEBUG ((
>> +        DEBUG_INFO, "Emu Memory Discoverred[%d]: Base = %016lx / Size
>> = %016x\n",
>> +        Index, MemoryBase, MemorySize
>> +        ));
>>       }
>>       Index++;
>>     } while (!EFI_ERROR (Status));
>> @@ -103,7 +111,8 @@ Returns:
>>     //
>>     // Build the CPU hob with 36-bit addressing and 16-bits of IO space.
>>     //
>> -  BuildCpuHob (36, 16);
>> +  DEBUG ((DEBUG_INFO, "Emu SizeOfMemorySpace = %d\n",
>> SizeOfMemorySpace));
>> +  BuildCpuHob (SizeOfMemorySpace, 16);
> 
> Hi Ray,
> 
> Just a question, I saw the implementation in Nt32Pkg just hard-code the
> address size to 52 to support both 32-bit and 64-bit. Is there a concern
> for hard coding it to 52?

I didn't check the NT32 implementation. I will try to use 64 directly.
52 may become insufficient when 5-level paging is widely used in OS.

> 
> Best Regards,
> Hao Wu
> 
>>
>>     return Status;
>>   }
>> --
>> 2.16.1.windows.1
> 


-- 
Thanks,
Ray