Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 13:50, Ard Biesheuvel wrote:
> On 19 February 2016 at 13:48, Laszlo Ersek  wrote:
>> On 02/19/16 13:40, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
>>
 I think customizing this code for MDE_CPU_AARCH64, on the source code
 level, is not good.

 Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
 two consequences:

 - it says Universal, so it shouldn't be specific to architecture on the
 source code level,
 - it says IntelFrameworkModulePkg, so you have a PCD namespace that
 doesn't clutter MdePkg's / MdeModulePkg's.

 Also, you mentioned earlier that memory layout is not specific to ISA,
 but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.

 My vote is to introduce a new PCD that controls this allocation limit.
 After all, it can depend on a bunch of things:
 - Whether you have S3 or not
 - whether PEI can address all of the memory or just 4GB
 - whether your platform has RAM under 4GB
 - whether allocating under 4GB is detrimental to performance

 and so on. I think this is the textbook case for a new PCD.

>>>
>>> OK, fair enough. That still means a PCD with different default values
>>> for X64 and other archs, but that is apparently not something to care
>>> about.
>>
>> Sorry, I don't understand; perhaps I missed something earlier.
>>
>> Your patch changes AllocateMaxAddress into AllocateAnyPages,
>> conditionally, for the "AcpiLowMemoryBase" allocation. Why is it not
>> enough to enable this change (with the PCD) for AARCH64 only? Why must
>> X64 differ?
>>
> 
> The only downside is that AARCH64 platforms don't get the change 'for
> free' but have to set this new PCD. But thinking about it, that is not
> entirely unreasonable ...

Yes: they are the freaks with no RAM under 4GB! ;)

Laszlo

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Ard Biesheuvel 
On 19 February 2016 at 13:48, Laszlo Ersek  wrote:
> On 02/19/16 13:40, Ard Biesheuvel wrote:
>> On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
>
>>> I think customizing this code for MDE_CPU_AARCH64, on the source code
>>> level, is not good.
>>>
>>> Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
>>> two consequences:
>>>
>>> - it says Universal, so it shouldn't be specific to architecture on the
>>> source code level,
>>> - it says IntelFrameworkModulePkg, so you have a PCD namespace that
>>> doesn't clutter MdePkg's / MdeModulePkg's.
>>>
>>> Also, you mentioned earlier that memory layout is not specific to ISA,
>>> but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.
>>>
>>> My vote is to introduce a new PCD that controls this allocation limit.
>>> After all, it can depend on a bunch of things:
>>> - Whether you have S3 or not
>>> - whether PEI can address all of the memory or just 4GB
>>> - whether your platform has RAM under 4GB
>>> - whether allocating under 4GB is detrimental to performance
>>>
>>> and so on. I think this is the textbook case for a new PCD.
>>>
>>
>> OK, fair enough. That still means a PCD with different default values
>> for X64 and other archs, but that is apparently not something to care
>> about.
>
> Sorry, I don't understand; perhaps I missed something earlier.
>
> Your patch changes AllocateMaxAddress into AllocateAnyPages,
> conditionally, for the "AcpiLowMemoryBase" allocation. Why is it not
> enough to enable this change (with the PCD) for AARCH64 only? Why must
> X64 differ?
>

The only downside is that AARCH64 platforms don't get the change 'for
free' but have to set this new PCD. But thinking about it, that is not
entirely unreasonable ...
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Ard Biesheuvel 
On 19 February 2016 at 13:33, Laszlo Ersek  wrote:
> On 02/19/16 13:18, Ard Biesheuvel wrote:
>> On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
>>> On 02/19/16 12:52, Ard Biesheuvel wrote:
 On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
> On 02/19/16 09:53, Ard Biesheuvel wrote:
>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>>> I can explain the reason on allocating <4G. It is because this data 
>>> will be used in PEI phase in S3 resume.
>>>
>>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
>>> 64bit. So we have to limit the allocation <4G.
>>>
>>
>> OK, got it.
>>
>>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>>> PcdDxeIplSwitchToLongMode?
>>>
>>
>> But that does not tell us if we are running a 32-bit PEI, right? It
>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>> 64-bit capable machine.
>
> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>
> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>
> (This is what we have in the three DSC files of OVMF.)
>
> So, in order to see in DXE if your PEI is 32-bit, you can do:
>
>   BOOLEAN PeiIs32Bit;
>
>   PeiIs32Bit = FALSE;
>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> //
> // this implies a 32->64 switch
> //
> PeiIs32Bit = TRUE;
>   } else {
> //
> // otherwise, PEI and DXE have the same bitness,
> // so derive it from DXE's bitness
> //
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
> PeiIs32Bit = TRUE;
> #endif
>   }
>
> Alternatively:
>
>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>sizeof (UINTN) == sizeof (UINT32);
>
> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
> nonexistent. :)
>

 Thanks for clarifying.

 So the only case where we have to take care to allocate below 4 GB is
 the 32->64 case, since in all other cases, the memory allocated in DXE
 will always be addressable in PEI.
>>>
>>> I don't think so. PEI is not obliged to be able to address all of the
>>> memory.
>>>
>>> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
>>> tables only for the first 4GB (for the SEC and PEI phases), and only the
>>> DXE core identify-maps the full memory.
>>>
>>
>> OK, so that would imply that we simply need to check for
>> defined(MDE_CPU_X64) here? Or does the same apply to IPF?
>>
>> Since this is a bit of a can of worms, I am perfectly happy to only
>> drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
>> easier ...
>
> Okay, so the new PCD idea has been raised several times. IIRC Jiewen
> mentioned that proliferation of PCDs is now frowned upon. On the other
> hand, in this thread, Jiewen mentioned the possibility of a new PCD:
>
> http://thread.gmane.org/gmane.comp.bios.edk2.devel/7817/focus=7871
>
> I think customizing this code for MDE_CPU_AARCH64, on the source code
> level, is not good.
>
> Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
> two consequences:
>
> - it says Universal, so it shouldn't be specific to architecture on the
> source code level,
> - it says IntelFrameworkModulePkg, so you have a PCD namespace that
> doesn't clutter MdePkg's / MdeModulePkg's.
>
> Also, you mentioned earlier that memory layout is not specific to ISA,
> but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.
>
> My vote is to introduce a new PCD that controls this allocation limit.
> After all, it can depend on a bunch of things:
> - Whether you have S3 or not
> - whether PEI can address all of the memory or just 4GB
> - whether your platform has RAM under 4GB
> - whether allocating under 4GB is detrimental to performance
>
> and so on. I think this is the textbook case for a new PCD.
>

OK, fair enough. That still means a PCD with different default values
for X64 and other archs, but that is apparently not something to care
about.
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 13:18, Ard Biesheuvel wrote:
> On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
>> On 02/19/16 12:52, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
 On 02/19/16 09:53, Ard Biesheuvel wrote:
> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>> I can explain the reason on allocating <4G. It is because this data will 
>> be used in PEI phase in S3 resume.
>>
>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
>> 64bit. So we have to limit the allocation <4G.
>>
>
> OK, got it.
>
>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>> PcdDxeIplSwitchToLongMode?
>>
>
> But that does not tell us if we are running a 32-bit PEI, right? It
> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
> 64-bit capable machine.

 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.

 Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.

 (This is what we have in the three DSC files of OVMF.)

 So, in order to see in DXE if your PEI is 32-bit, you can do:

   BOOLEAN PeiIs32Bit;

   PeiIs32Bit = FALSE;
   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
 //
 // this implies a 32->64 switch
 //
 PeiIs32Bit = TRUE;
   } else {
 //
 // otherwise, PEI and DXE have the same bitness,
 // so derive it from DXE's bitness
 //
 #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
 PeiIs32Bit = TRUE;
 #endif
   }

 Alternatively:

   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
sizeof (UINTN) == sizeof (UINT32);

 I'll admit I'm not really sure about EBC. The above mostly treats EBC as
 nonexistent. :)

>>>
>>> Thanks for clarifying.
>>>
>>> So the only case where we have to take care to allocate below 4 GB is
>>> the 32->64 case, since in all other cases, the memory allocated in DXE
>>> will always be addressable in PEI.
>>
>> I don't think so. PEI is not obliged to be able to address all of the
>> memory.
>>
>> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
>> tables only for the first 4GB (for the SEC and PEI phases), and only the
>> DXE core identify-maps the full memory.
>>
> 
> OK, so that would imply that we simply need to check for
> defined(MDE_CPU_X64) here? Or does the same apply to IPF?
> 
> Since this is a bit of a can of worms, I am perfectly happy to only
> drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
> easier ...

Okay, so the new PCD idea has been raised several times. IIRC Jiewen
mentioned that proliferation of PCDs is now frowned upon. On the other
hand, in this thread, Jiewen mentioned the possibility of a new PCD:

http://thread.gmane.org/gmane.comp.bios.edk2.devel/7817/focus=7871

I think customizing this code for MDE_CPU_AARCH64, on the source code
level, is not good.

Here we are under IntelFrameworkModulePkg/Universal/BdsDxe/, which has
two consequences:

- it says Universal, so it shouldn't be specific to architecture on the
source code level,
- it says IntelFrameworkModulePkg, so you have a PCD namespace that
doesn't clutter MdePkg's / MdeModulePkg's.

Also, you mentioned earlier that memory layout is not specific to ISA,
but platform, hence MDE_CPU_AARCH64 would not be a perfect match anyway.

My vote is to introduce a new PCD that controls this allocation limit.
After all, it can depend on a bunch of things:
- Whether you have S3 or not
- whether PEI can address all of the memory or just 4GB
- whether your platform has RAM under 4GB
- whether allocating under 4GB is detrimental to performance

and so on. I think this is the textbook case for a new PCD.

Thanks
Laszlo
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Ard Biesheuvel 
On 19 February 2016 at 13:14, Laszlo Ersek  wrote:
> On 02/19/16 12:52, Ard Biesheuvel wrote:
>> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
>>> On 02/19/16 09:53, Ard Biesheuvel wrote:
 On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
> I can explain the reason on allocating <4G. It is because this data will 
> be used in PEI phase in S3 resume.
>
> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 
> 64bit. So we have to limit the allocation <4G.
>

 OK, got it.

> Using ACPI version PCD looks strange here. Maybe another PCD, like 
> PcdDxeIplSwitchToLongMode?
>

 But that does not tell us if we are running a 32-bit PEI, right? It
 only tells us if a 32-bit PEI should load a 32-bit DXE core on a
 64-bit capable machine.
>>>
>>> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>>>
>>> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>>>
>>> (This is what we have in the three DSC files of OVMF.)
>>>
>>> So, in order to see in DXE if your PEI is 32-bit, you can do:
>>>
>>>   BOOLEAN PeiIs32Bit;
>>>
>>>   PeiIs32Bit = FALSE;
>>>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>>> //
>>> // this implies a 32->64 switch
>>> //
>>> PeiIs32Bit = TRUE;
>>>   } else {
>>> //
>>> // otherwise, PEI and DXE have the same bitness,
>>> // so derive it from DXE's bitness
>>> //
>>> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
>>> PeiIs32Bit = TRUE;
>>> #endif
>>>   }
>>>
>>> Alternatively:
>>>
>>>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>>>sizeof (UINTN) == sizeof (UINT32);
>>>
>>> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
>>> nonexistent. :)
>>>
>>
>> Thanks for clarifying.
>>
>> So the only case where we have to take care to allocate below 4 GB is
>> the 32->64 case, since in all other cases, the memory allocated in DXE
>> will always be addressable in PEI.
>
> I don't think so. PEI is not obliged to be able to address all of the
> memory.
>
> In OVMF for example, even if PEI is 64-bit, the reset vector builds page
> tables only for the first 4GB (for the SEC and PEI phases), and only the
> DXE core identify-maps the full memory.
>

OK, so that would imply that we simply need to check for
defined(MDE_CPU_X64) here? Or does the same apply to IPF?

Since this is a bit of a can of worms, I am perfectly happy to only
drop the limit for MDE_CPU_AARCH64 instead, and make everybody's lives
easier ...
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 12:52, Ard Biesheuvel wrote:
> On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
>> On 02/19/16 09:53, Ard Biesheuvel wrote:
>>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
 I can explain the reason on allocating <4G. It is because this data will 
 be used in PEI phase in S3 resume.

 On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
 So we have to limit the allocation <4G.

>>>
>>> OK, got it.
>>>
 Using ACPI version PCD looks strange here. Maybe another PCD, like 
 PcdDxeIplSwitchToLongMode?

>>>
>>> But that does not tell us if we are running a 32-bit PEI, right? It
>>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>>> 64-bit capable machine.
>>
>> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>>
>> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>>
>> (This is what we have in the three DSC files of OVMF.)
>>
>> So, in order to see in DXE if your PEI is 32-bit, you can do:
>>
>>   BOOLEAN PeiIs32Bit;
>>
>>   PeiIs32Bit = FALSE;
>>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
>> //
>> // this implies a 32->64 switch
>> //
>> PeiIs32Bit = TRUE;
>>   } else {
>> //
>> // otherwise, PEI and DXE have the same bitness,
>> // so derive it from DXE's bitness
>> //
>> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
>> PeiIs32Bit = TRUE;
>> #endif
>>   }
>>
>> Alternatively:
>>
>>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>>sizeof (UINTN) == sizeof (UINT32);
>>
>> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
>> nonexistent. :)
>>
> 
> Thanks for clarifying.
> 
> So the only case where we have to take care to allocate below 4 GB is
> the 32->64 case, since in all other cases, the memory allocated in DXE
> will always be addressable in PEI.

I don't think so. PEI is not obliged to be able to address all of the
memory.

In OVMF for example, even if PEI is 64-bit, the reset vector builds page
tables only for the first 4GB (for the SEC and PEI phases), and only the
DXE core identify-maps the full memory.

Thanks
Laszlo

> 
> So I will take Jiewen's suggestion, and only limit the memory
> allocation if PcdDxeIplSwitchToLongMode is TRUE. But I will still need
> the MDE_CPU_X64 ifdef, since PcdDxeIplSwitchToLongMode is only defined
> for IA32 and X64, and since it defaults to TRUE, defining it for other
> archs would mean we have to change the default value as well.
> 
> Alternatively, I could propose this:
> 
> [PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
>   
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b
> 
> [PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF]
>  
> gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b
> 
> and only test the PCD, but I don't think that is an improvement.
> 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Ard Biesheuvel 
On 19 February 2016 at 10:56, Laszlo Ersek  wrote:
> On 02/19/16 09:53, Ard Biesheuvel wrote:
>> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>>> I can explain the reason on allocating <4G. It is because this data will be 
>>> used in PEI phase in S3 resume.
>>>
>>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
>>> So we have to limit the allocation <4G.
>>>
>>
>> OK, got it.
>>
>>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>>> PcdDxeIplSwitchToLongMode?
>>>
>>
>> But that does not tell us if we are running a 32-bit PEI, right? It
>> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
>> 64-bit capable machine.
>
> 32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.
>
> Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.
>
> (This is what we have in the three DSC files of OVMF.)
>
> So, in order to see in DXE if your PEI is 32-bit, you can do:
>
>   BOOLEAN PeiIs32Bit;
>
>   PeiIs32Bit = FALSE;
>   if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
> //
> // this implies a 32->64 switch
> //
> PeiIs32Bit = TRUE;
>   } else {
> //
> // otherwise, PEI and DXE have the same bitness,
> // so derive it from DXE's bitness
> //
> #if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
> PeiIs32Bit = TRUE;
> #endif
>   }
>
> Alternatively:
>
>   PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
>sizeof (UINTN) == sizeof (UINT32);
>
> I'll admit I'm not really sure about EBC. The above mostly treats EBC as
> nonexistent. :)
>

Thanks for clarifying.

So the only case where we have to take care to allocate below 4 GB is
the 32->64 case, since in all other cases, the memory allocated in DXE
will always be addressable in PEI.

So I will take Jiewen's suggestion, and only limit the memory
allocation if PcdDxeIplSwitchToLongMode is TRUE. But I will still need
the MDE_CPU_X64 ifdef, since PcdDxeIplSwitchToLongMode is only defined
for IA32 and X64, and since it defaults to TRUE, defining it for other
archs would mean we have to change the default value as well.

Alternatively, I could propose this:

[PcdsFeatureFlag.IA32, PcdsFeatureFlag.X64]
  
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|TRUE|BOOLEAN|0x0001003b

[PcdsFeatureFlag.ARM, PcdsFeatureFlag.AARCH64, PcdsFeatureFlag.IPF]
 
gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode|FALSE|BOOLEAN|0x0001003b

and only test the PCD, but I don't think that is an improvement.

-- 
Ard.





>>
>>> -Original Message-
>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>>> Sent: Friday, February 19, 2016 4:03 AM
>>> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
>>> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; 
>>> Fan, Jeff; Yao, Jiewen
>>> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
>>> Biesheuvel
>>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate 
>>> below 4 GB on ACPI 1.0
>>>
>>> It is not entirely clear from the code why, but the reserved region for 
>>> storing performance data is allocated below 4 GB, and the variable to hold 
>>> the address of the allocation is called 'AcpiLowMemoryBase'
>>> (which is the only mention of ACPI in the entire file).
>>>
>>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as 
>>> well, since systems that can deal with ACPI table in high memory are also 
>>> just as likely to be able to deal with performance data in high memory. 
>>> This prevents memory fragmentation on systems that don't care about the 4 
>>> GB limit.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel 
>>> ---
>>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
>>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
>>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> index 6afb8a09df9c..38172780fb49 100644
>>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> @@ -215,6 +215,7 @@ [Pcd]
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution 
>>>## CONSUMES
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution   
>>>## CONSUMES
>>>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable   
>>>## CONSUMES
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions   
>>>## CONSUMES
>>>
>>>  [Depex]
>>>TRUE
>>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
>>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>>> index ae7ad2153c51..bf5bd6524a90 100644
>>> ---

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Laszlo Ersek 
On 02/19/16 09:53, Ard Biesheuvel wrote:
> On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
>> I can explain the reason on allocating <4G. It is because this data will be 
>> used in PEI phase in S3 resume.
>>
>> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. 
>> So we have to limit the allocation <4G.
>>
> 
> OK, got it.
> 
>> Using ACPI version PCD looks strange here. Maybe another PCD, like 
>> PcdDxeIplSwitchToLongMode?
>>
> 
> But that does not tell us if we are running a 32-bit PEI, right? It
> only tells us if a 32-bit PEI should load a 32-bit DXE core on a
> 64-bit capable machine.

32->32 and 64->64 builds have PcdDxeIplSwitchToLongMode set to FALSE.

Only 32->64 builds have PcdDxeIplSwitchToLongMode set to TRUE.

(This is what we have in the three DSC files of OVMF.)

So, in order to see in DXE if your PEI is 32-bit, you can do:

  BOOLEAN PeiIs32Bit;

  PeiIs32Bit = FALSE;
  if (FeaturePcdGet (PcdDxeIplSwitchToLongMode)) {
//
// this implies a 32->64 switch
//
PeiIs32Bit = TRUE;
  } else {
//
// otherwise, PEI and DXE have the same bitness,
// so derive it from DXE's bitness
//
#if defined (MDE_CPU_IA32) || defined (MDE_CPU_ARM)
PeiIs32Bit = TRUE;
#endif
  }

Alternatively:

  PeiIs32Bit = FeaturePcdGet (PcdDxeIplSwitchToLongMode) ||
   sizeof (UINTN) == sizeof (UINT32);

I'll admit I'm not really sure about EBC. The above mostly treats EBC as
nonexistent. :)

Thanks
Laszlo

> 
> 
>> -Original Message-
>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
>> Sent: Friday, February 19, 2016 4:03 AM
>> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
>> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; Fan, 
>> Jeff; Yao, Jiewen
>> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
>> Biesheuvel
>> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 
>> 4 GB on ACPI 1.0
>>
>> It is not entirely clear from the code why, but the reserved region for 
>> storing performance data is allocated below 4 GB, and the variable to hold 
>> the address of the allocation is called 'AcpiLowMemoryBase'
>> (which is the only mention of ACPI in the entire file).
>>
>> Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
>> since systems that can deal with ACPI table in high memory are also just as 
>> likely to be able to deal with performance data in high memory. This 
>> prevents memory fragmentation on systems that don't care about the 4 GB 
>> limit.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel 
>> ---
>>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
>> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> index 6afb8a09df9c..38172780fb49 100644
>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> @@ -215,6 +215,7 @@ [Pcd]
>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution  
>>   ## CONSUMES
>>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution
>>   ## CONSUMES
>>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable
>>   ## CONSUMES
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions
>>   ## CONSUMES
>>
>>  [Depex]
>>TRUE
>> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
>> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> index ae7ad2153c51..bf5bd6524a90 100644
>> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
>> @@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
>> EXPRESS OR IMPLIED.
>>  #include "Hotkey.h"
>>  #include "HwErrRecSupport.h"
>>
>> +#include 
>> +
>>  ///
>>  /// BDS arch protocol instance initial value.
>>  ///
>> @@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
>>EFI_STATUSStatus;
>>EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
>>EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
>> +  EFI_ALLOCATE_TYPE AcpiTableAllocType;
>>
>>AcpiLowMemoryBase = 0x0ULL;
>>
>> +  if ((PcdGet32 (PcdAcpiExposedTableVersions) & 
>> EFI_ACPI_TABLE_VERSION_1_0B) != 0) {
>> +AcpiTableAllocType = AllocateMaxAddress;  } else {
>> +AcpiTableAllocType = AllocateAnyPages;  }
>> +
>>//
>>// Allocate a block of memory that will contain performance data to OS.
>>//
>>Status = gBS->AllocatePages (
>> -  AllocateMaxAddress,
>> +  AcpiTableAllocType,
>>EfiReservedMemoryType,
>>

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Ard Biesheuvel 
On 19 February 2016 at 09:45, Yao, Jiewen  wrote:
> I can explain the reason on allocating <4G. It is because this data will be 
> used in PEI phase in S3 resume.
>
> On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. So 
> we have to limit the allocation <4G.
>

OK, got it.

> Using ACPI version PCD looks strange here. Maybe another PCD, like 
> PcdDxeIplSwitchToLongMode?
>

But that does not tell us if we are running a 32-bit PEI, right? It
only tells us if a 32-bit PEI should load a 32-bit DXE core on a
64-bit capable machine.


> -Original Message-
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Friday, February 19, 2016 4:03 AM
> To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; 
> leif.lindh...@linaro.org; graeme.greg...@linaro.org; ler...@redhat.com; Fan, 
> Jeff; Yao, Jiewen
> Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
> Biesheuvel
> Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 
> 4 GB on ACPI 1.0
>
> It is not entirely clear from the code why, but the reserved region for 
> storing performance data is allocated below 4 GB, and the variable to hold 
> the address of the allocation is called 'AcpiLowMemoryBase'
> (which is the only mention of ACPI in the entire file).
>
> Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
> since systems that can deal with ACPI table in high memory are also just as 
> likely to be able to deal with performance data in high memory. This prevents 
> memory fragmentation on systems that don't care about the 4 GB limit.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel 
> ---
>  IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
> IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> index 6afb8a09df9c..38172780fb49 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> @@ -215,6 +215,7 @@ [Pcd]
>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution   
>  ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution 
>  ## CONSUMES
>gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable 
>  ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions 
>  ## CONSUMES
>
>  [Depex]
>TRUE
> diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
> b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> index ae7ad2153c51..bf5bd6524a90 100644
> --- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> +++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
> @@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
> EXPRESS OR IMPLIED.
>  #include "Hotkey.h"
>  #include "HwErrRecSupport.h"
>
> +#include 
> +
>  ///
>  /// BDS arch protocol instance initial value.
>  ///
> @@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
>EFI_STATUSStatus;
>EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
>EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
> +  EFI_ALLOCATE_TYPE AcpiTableAllocType;
>
>AcpiLowMemoryBase = 0x0ULL;
>
> +  if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) 
> != 0) {
> +AcpiTableAllocType = AllocateMaxAddress;  } else {
> +AcpiTableAllocType = AllocateAnyPages;  }
> +
>//
>// Allocate a block of memory that will contain performance data to OS.
>//
>Status = gBS->AllocatePages (
> -  AllocateMaxAddress,
> +  AcpiTableAllocType,
>EfiReservedMemoryType,
>EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
>
> --
> 2.5.0
>
___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 GB on ACPI 1.0

2016-02-19 Thread Yao, Jiewen 
I can explain the reason on allocating <4G. It is because this data will be 
used in PEI phase in S3 resume.

On most X86 platform, PEI phase is 32bit, and DXE phase is 32bit or 64bit. So 
we have to limit the allocation <4G.

Using ACPI version PCD looks strange here. Maybe another PCD, like 
PcdDxeIplSwitchToLongMode?

Thank you
Yao Jiewen

-Original Message-
From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] 
Sent: Friday, February 19, 2016 4:03 AM
To: edk2-devel@lists.01.org; Tian, Feng; Zeng, Star; leif.lindh...@linaro.org; 
graeme.greg...@linaro.org; ler...@redhat.com; Fan, Jeff; Yao, Jiewen
Cc: mark.rutl...@arm.com; Gao, Liming; samer.el-haj-mahm...@hpe.com; Ard 
Biesheuvel
Subject: [PATCH v2 2/3] IntelFrameworkModulePkg: BdsDxe: only allocate below 4 
GB on ACPI 1.0

It is not entirely clear from the code why, but the reserved region for storing 
performance data is allocated below 4 GB, and the variable to hold the address 
of the allocation is called 'AcpiLowMemoryBase'
(which is the only mention of ACPI in the entire file).

Let's make this allocation dependent on PcdAcpiExposedTableVersions as well, 
since systems that can deal with ACPI table in high memory are also just as 
likely to be able to deal with performance data in high memory. This prevents 
memory fragmentation on systems that don't care about the 4 GB limit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel 
---
 IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf |  1 +  
IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c | 11 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
index 6afb8a09df9c..38172780fb49 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
@@ -215,6 +215,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoHorizontalResolution
## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdSetupVideoVerticalResolution  
## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable  
## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiExposedTableVersions  
## CONSUMES
 
 [Depex]
   TRUE
diff --git a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c 
b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
index ae7ad2153c51..bf5bd6524a90 100644
--- a/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
+++ b/IntelFrameworkModulePkg/Universal/BdsDxe/BdsEntry.c
@@ -22,6 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER 
EXPRESS OR IMPLIED.
 #include "Hotkey.h"
 #include "HwErrRecSupport.h"
 
+#include 
+
 ///
 /// BDS arch protocol instance initial value.
 ///
@@ -474,14 +476,21 @@ BdsAllocateMemoryForPerformanceData (
   EFI_STATUSStatus;
   EFI_PHYSICAL_ADDRESS  AcpiLowMemoryBase;
   EDKII_VARIABLE_LOCK_PROTOCOL  *VariableLock;
+  EFI_ALLOCATE_TYPE AcpiTableAllocType;
 
   AcpiLowMemoryBase = 0x0ULL;
 
+  if ((PcdGet32 (PcdAcpiExposedTableVersions) & EFI_ACPI_TABLE_VERSION_1_0B) 
!= 0) {
+AcpiTableAllocType = AllocateMaxAddress;  } else {
+AcpiTableAllocType = AllocateAnyPages;  }
+
   //
   // Allocate a block of memory that will contain performance data to OS.
   //
   Status = gBS->AllocatePages (
-  AllocateMaxAddress,
+  AcpiTableAllocType,
   EfiReservedMemoryType,
   EFI_SIZE_TO_PAGES (PERF_DATA_MAX_LENGTH),
   
--
2.5.0

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel