diff mbox

[Linaro-uefi,23/28] D02: Disable memory test in BDS

Message ID 1479122995-50330-24-git-send-email-heyi.guo@linaro.org
State New
Headers show

Commit Message

gary guo Nov. 14, 2016, 11:29 a.m. UTC
Memory test may take long time when there is a lot of memory in system,
so we disable memory test in BDS to accelerate boot speed.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
 Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Leif Lindholm Nov. 15, 2016, 8:49 p.m. UTC | #1
On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
> Memory test may take long time when there is a lot of memory in system,
> so we disable memory test in BDS to accelerate boot speed.

I am still not a fan of this.
Do you have any feedback with regards to the comments I made on the
previous version?:
---
It would be very much preferable if you could make use of the provided
facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
to 1/16.

Have a look at GenericMemoryTestEntryPoint() and let me know what you
think.
---

Same comment applies to D03 patch.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>  Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc b/Platforms/Hisilicon/D02/Pv660D02.dsc
> index c11fa4e..d6fbcb9 100644
> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
> @@ -429,7 +429,7 @@
>    #
>    # Memory test
>    #
> -  MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> +  MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>  
>    MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf b/Platforms/Hisilicon/D02/Pv660D02.fdf
> index ec4d749..c941e4e 100644
> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>    #
>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>  
> -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> +  INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>    INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> -- 
> 1.9.1
>
gary guo Nov. 16, 2016, 12:53 a.m. UTC | #2
sorry, I missed the comments previous version, i will try it.

在 11/16/2016 4:49 AM, Leif Lindholm 写道:
> On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
>> Memory test may take long time when there is a lot of memory in system,
>> so we disable memory test in BDS to accelerate boot speed.
> I am still not a fan of this.
> Do you have any feedback with regards to the comments I made on the
> previous version?:
> ---
> It would be very much preferable if you could make use of the provided
> facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
> or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
> to 1/16.
>
> Have a look at GenericMemoryTestEntryPoint() and let me know what you
> think.
> ---
>
> Same comment applies to D03 patch.
>
> /
>      Leif
>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> ---
>>   Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>>   Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc b/Platforms/Hisilicon/D02/Pv660D02.dsc
>> index c11fa4e..d6fbcb9 100644
>> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
>> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
>> @@ -429,7 +429,7 @@
>>     #
>>     # Memory test
>>     #
>> -  MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>> +  MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>   
>>     MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>     MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf b/Platforms/Hisilicon/D02/Pv660D02.fdf
>> index ec4d749..c941e4e 100644
>> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
>> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
>> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>>     #
>>     INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>   
>> -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>> +  INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>     INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>     INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>     INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>> -- 
>> 1.9.1
>>
gary guo Nov. 19, 2016, 7:41 a.m. UTC | #3
Hi Leif,

We have the test result about the BootMode,
see detail below, please check and let me know your comment.

Thanks and Regards,
Heyi.

在 11/16/2016 8:53 AM, Heyi Guo 写道:
> sorry, I missed the comments previous version, i will try it.
>
> 在 11/16/2016 4:49 AM, Leif Lindholm 写道:
>> On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
>>> Memory test may take long time when there is a lot of memory in system,
>>> so we disable memory test in BDS to accelerate boot speed.
>> I am still not a fan of this.
>> Do you have any feedback with regards to the comments I made on the
>> previous version?:
>> ---
>> It would be very much preferable if you could make use of the provided
>> facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
>> or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
>> to 1/16.
>>
>> Have a look at GenericMemoryTestEntryPoint() and let me know what you
>> think.
>> ---
>>
         We have checked the code and found that the BootMode is already
          BOOT_WITH_FULL_CONFIGURATION at the previous version, but it 
is could not define the test level,
          actually, the test level is passed by the 
PlatformBdsPolicyBehavior at PlatformIntelBdsLib,
          and the level is 'QUICK' now, if we switch it to 'SPARSE' the 
test time
         will cut to 1/4, but the D05 have 16 DIMM slots(D03 8 slots) 
and totally support
         16GB*16(256GB)  memory, the test time is still too long, so 
could we
         set the level to 'IGNOR'?

>> Same comment applies to D03 patch.
>>
>> /
>>      Leif
>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>> ---
>>>   Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>>>   Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc 
>>> b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>> index c11fa4e..d6fbcb9 100644
>>> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>> @@ -429,7 +429,7 @@
>>>     #
>>>     # Memory test
>>>     #
>>> - 
>>> MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>> + 
>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>     MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf 
>>> b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>> index ec4d749..c941e4e 100644
>>> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>>>     #
>>>     INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>>   -  INF 
>>> MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>> +  INF 
>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>     INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>     INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>     INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>> -- 
>>> 1.9.1
>>>
>
Leif Lindholm Nov. 28, 2016, 4:31 p.m. UTC | #4
(Hi Heyi, I am now back from my holiday.)

On Sat, Nov 19, 2016 at 03:41:57PM +0800, Heyi Guo wrote:
> Hi Leif,
> 
> We have the test result about the BootMode,
> see detail below, please check and let me know your comment.
> 
> Thanks and Regards,
> Heyi.
> 
> 在 11/16/2016 8:53 AM, Heyi Guo 写道:
> >sorry, I missed the comments previous version, i will try it.
> >
> >在 11/16/2016 4:49 AM, Leif Lindholm 写道:
> >>On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
> >>>Memory test may take long time when there is a lot of memory in system,
> >>>so we disable memory test in BDS to accelerate boot speed.
> >>I am still not a fan of this.
> >>Do you have any feedback with regards to the comments I made on the
> >>previous version?:
> >>---
> >>It would be very much preferable if you could make use of the provided
> >>facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
> >>or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
> >>to 1/16.
> >>
> >>Have a look at GenericMemoryTestEntryPoint() and let me know what you
> >>think.
> >>---
> >>
>         We have checked the code and found that the BootMode is already
>          BOOT_WITH_FULL_CONFIGURATION at the previous version, but it is
> could not define the test level,
>          actually, the test level is passed by the PlatformBdsPolicyBehavior
> at PlatformIntelBdsLib,
>          and the level is 'QUICK' now, if we switch it to 'SPARSE' the test
> time
>         will cut to 1/4, but the D05 have 16 DIMM slots(D03 8 slots) and
> totally support
>         16GB*16(256GB)  memory, the test time is still too long, so could we
>         set the level to 'IGNOR'?

For reference, what periods of time are we talking about here?

As far as I can see, this protocol is required only because D0* are
still using IntelBds? And looking at that code, it will happily skip
over doing the memory test (returning EFI_SUCCESS) if the protocol
cannot be found.

So if we are genuinely looking to remove the feature of verifying that
the RAM is basically functional - why are we not just dropping the 
GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?

Regards,

Leif

> >>Same comment applies to D03 patch.
> >>
> >>/
> >>     Leif
> >>
> >>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>---
> >>>  Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
> >>>  Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
> >>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>index c11fa4e..d6fbcb9 100644
> >>>--- a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>+++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>@@ -429,7 +429,7 @@
> >>>    #
> >>>    # Memory test
> >>>    #
> >>>- MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>+
> >>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>index ec4d749..c941e4e 100644
> >>>--- a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>+++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>@@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
> >>>    #
> >>>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> >>>  -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>+  INF
> >>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>    INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> >>>-- 
> >>>1.9.1
> >>>
> >
>
gary guo Nov. 29, 2016, 5:18 a.m. UTC | #5
Hi Leif,

Welcome back :)

Please help to review the RP1612 v4 version patchsets, thanks.

在 11/29/2016 12:31 AM, Leif Lindholm 写道:
> (Hi Heyi, I am now back from my holiday.)
>
> On Sat, Nov 19, 2016 at 03:41:57PM +0800, Heyi Guo wrote:
>> Hi Leif,
>>
>> We have the test result about the BootMode,
>> see detail below, please check and let me know your comment.
>>
>> Thanks and Regards,
>> Heyi.
>>
>> 在 11/16/2016 8:53 AM, Heyi Guo 写道:
>>> sorry, I missed the comments previous version, i will try it.
>>>
>>> 在 11/16/2016 4:49 AM, Leif Lindholm 写道:
>>>> On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
>>>>> Memory test may take long time when there is a lot of memory in system,
>>>>> so we disable memory test in BDS to accelerate boot speed.
>>>> I am still not a fan of this.
>>>> Do you have any feedback with regards to the comments I made on the
>>>> previous version?:
>>>> ---
>>>> It would be very much preferable if you could make use of the provided
>>>> facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
>>>> or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
>>>> to 1/16.
>>>>
>>>> Have a look at GenericMemoryTestEntryPoint() and let me know what you
>>>> think.
>>>> ---
>>>>
>>          We have checked the code and found that the BootMode is already
>>           BOOT_WITH_FULL_CONFIGURATION at the previous version, but it is
>> could not define the test level,
>>           actually, the test level is passed by the PlatformBdsPolicyBehavior
>> at PlatformIntelBdsLib,
>>           and the level is 'QUICK' now, if we switch it to 'SPARSE' the test
>> time
>>          will cut to 1/4, but the D05 have 16 DIMM slots(D03 8 slots) and
>> totally support
>>          16GB*16(256GB)  memory, the test time is still too long, so could we
>>          set the level to 'IGNOR'?
> For reference, what periods of time are we talking about here?
>
> As far as I can see, this protocol is required only because D0* are
> still using IntelBds? And looking at that code, it will happily skip
> over doing the memory test (returning EFI_SUCCESS) if the protocol
> cannot be found.
>
> So if we are genuinely looking to remove the feature of verifying that
> the RAM is basically functional - why are we not just dropping the
> GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
>
> Regards,
>
> Leif
    It's cost about 2 minutes on 256G memory platform,
    yes, this protocol is required because we still using IntelBds, and 
we also think that using NullMemoryTestDxe
    is  better.

    Thanks,
    Heyi

>>>> Same comment applies to D03 patch.
>>>>
>>>> /
>>>>      Leif
>>>>
>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>>> ---
>>>>>   Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>>>>>   Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>> b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>> index c11fa4e..d6fbcb9 100644
>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>> @@ -429,7 +429,7 @@
>>>>>     #
>>>>>     # Memory test
>>>>>     #
>>>>> - MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>> +
>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>     MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>> b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>> index ec4d749..c941e4e 100644
>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>>>>>     #
>>>>>     INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>>>>   -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>> +  INF
>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>>     INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>     INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>>     INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>>>> -- 
>>>>> 1.9.1
>>>>>
Leif Lindholm Nov. 29, 2016, 11:42 a.m. UTC | #6
On Tue, Nov 29, 2016 at 01:18:26PM +0800, Heyi Guo wrote:
> Hi Leif,
> 
> Welcome back :)
> 
> Please help to review the RP1612 v4 version patchsets, thanks.

Yes, working from home today to do just that.

> >
> >So if we are genuinely looking to remove the feature of verifying that
> >the RAM is basically functional - why are we not just dropping the
> >GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
> >
> >Regards,
> >
> >Leif
>    It's cost about 2 minutes on 256G memory platform,

Even the SPARSE one? Sure, that is substantial. But it also sounds too
much. We should definitely look into that post release.

>    yes, this protocol is required because we still using IntelBds,
> and we
> also think that using NullMemoryTestDxe
>    is  better.

Why?

I don't have a D02 here to test on, but it certainly builds fine
without it. And from inspection, the runtime check for it does not
trigger an ASSERT or even an error condition.

So what benefit does including NullMemoryTestDxe give you?

Regards,

Leif

> 
>    Thanks,
>    Heyi
> 
> >>>>Same comment applies to D03 patch.
> >>>>
> >>>>/
> >>>>     Leif
> >>>>
> >>>>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>>>---
> >>>>>  Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
> >>>>>  Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
> >>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>
> >>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>index c11fa4e..d6fbcb9 100644
> >>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>@@ -429,7 +429,7 @@
> >>>>>    #
> >>>>>    # Memory test
> >>>>>    #
> >>>>>- MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>+
> >>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>index ec4d749..c941e4e 100644
> >>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>@@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
> >>>>>    #
> >>>>>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> >>>>>  -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>+  INF
> >>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>    INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> >>>>>-- 
> >>>>>1.9.1
> >>>>>
>
gary guo Dec. 2, 2016, 2:09 a.m. UTC | #7
Hi Leif,

在 11/29/2016 7:42 PM, Leif Lindholm 写道:
> On Tue, Nov 29, 2016 at 01:18:26PM +0800, Heyi Guo wrote:
>> Hi Leif,
>>
>> Welcome back :)
>>
>> Please help to review the RP1612 v4 version patchsets, thanks.
> Yes, working from home today to do just that.
>
>>> So if we are genuinely looking to remove the feature of verifying that
>>> the RAM is basically functional - why are we not just dropping the
>>> GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
>>>
>>> Regards,
>>>
>>> Leif
>>     It's cost about 2 minutes on 256G memory platform,
> Even the SPARSE one? Sure, that is substantial. But it also sounds too
> much. We should definitely look into that post release.
   We tested it at D05 using SPARSE mode, it  also takes a long time.
>
>>     yes, this protocol is required because we still using IntelBds,
>> and we
>> also think that using NullMemoryTestDxe
>>     is  better.
> Why?
It is just not to do the memory testing when switching to 
NullMemoryTestDxe, only speed up the boot
time and no other benefits, but we do not keep to maintain D02 now, so 
just switch it on D03 and D05.

thanks,
Heyi
> I don't have a D02 here to test on, but it certainly builds fine
> without it. And from inspection, the runtime check for it does not
> trigger an ASSERT or even an error condition.
>
> So what benefit does including NullMemoryTestDxe give you?
>
> Regards,
>
> Leif
>
>>     Thanks,
>>     Heyi
>>
>>>>>> Same comment applies to D03 patch.
>>>>>>
>>>>>> /
>>>>>>      Leif
>>>>>>
>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>>>>> ---
>>>>>>>   Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>>>>>>>   Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>>>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>> b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>> index c11fa4e..d6fbcb9 100644
>>>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>> @@ -429,7 +429,7 @@
>>>>>>>     #
>>>>>>>     # Memory test
>>>>>>>     #
>>>>>>> - MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>>>> +
>>>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>>>> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>>>     MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>> b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>> index ec4d749..c941e4e 100644
>>>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>>>>>>>     #
>>>>>>>     INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>>>>>>   -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>>>> +  INF
>>>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>>>>     INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>>>     INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>>>>     INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>>>>>> -- 
>>>>>>> 1.9.1
>>>>>>>
Leif Lindholm Dec. 2, 2016, 10:12 a.m. UTC | #8
Hi Heyi,

On Fri, Dec 02, 2016 at 10:09:43AM +0800, Heyi Guo wrote:
> >>>So if we are genuinely looking to remove the feature of verifying that
> >>>the RAM is basically functional - why are we not just dropping the
> >>>GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
> >>>
> >>>Regards,
> >>>
> >>>Leif
> >>    It's cost about 2 minutes on 256G memory platform,
> >Even the SPARSE one? Sure, that is substantial. But it also sounds too
> >much. We should definitely look into that post release.
>   We tested it at D05 using SPARSE mode, it  also takes a long time.
> >
> >>    yes, this protocol is required because we still using IntelBds,
> >>and we
> >>also think that using NullMemoryTestDxe
> >>    is  better.
> >Why?
> It is just not to do the memory testing when switching to NullMemoryTestDxe,
> only speed up the boot
> time and no other benefits,

Yes, but why do you think that using NullMemoryTestDxe is better than
not including any memory test?

Regards,

Leif

> but we do not keep to maintain D02 now, so just
> switch it on D03 and D05.
> 
> thanks,
> Heyi
> >I don't have a D02 here to test on, but it certainly builds fine
> >without it. And from inspection, the runtime check for it does not
> >trigger an ASSERT or even an error condition.
> >
> >So what benefit does including NullMemoryTestDxe give you?
> >
> >Regards,
> >
> >Leif
> >
> >>    Thanks,
> >>    Heyi
> >>
> >>>>>>Same comment applies to D03 patch.
> >>>>>>
> >>>>>>/
> >>>>>>     Leif
> >>>>>>
> >>>>>>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>>>>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>>>>>---
> >>>>>>>  Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
> >>>>>>>  Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
> >>>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>
> >>>>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>index c11fa4e..d6fbcb9 100644
> >>>>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>@@ -429,7 +429,7 @@
> >>>>>>>    #
> >>>>>>>    # Memory test
> >>>>>>>    #
> >>>>>>>- MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>>>+
> >>>>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>>>MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>>>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>index ec4d749..c941e4e 100644
> >>>>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>@@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
> >>>>>>>    #
> >>>>>>>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> >>>>>>>  -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>>>+  INF
> >>>>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>>>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>>>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>>>    INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> >>>>>>>-- 
> >>>>>>>1.9.1
> >>>>>>>
>
gary guo Dec. 5, 2016, 6:25 a.m. UTC | #9
Hi Leif,


On 12/02/2016 06:12 PM, Leif Lindholm wrote:
> Hi Heyi,
>
> On Fri, Dec 02, 2016 at 10:09:43AM +0800, Heyi Guo wrote:
>>>>> So if we are genuinely looking to remove the feature of verifying that
>>>>> the RAM is basically functional - why are we not just dropping the
>>>>> GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
>>>>>
>>>>> Regards,
>>>>>
>>>>> Leif
>>>>     It's cost about 2 minutes on 256G memory platform,
>>> Even the SPARSE one? Sure, that is substantial. But it also sounds too
>>> much. We should definitely look into that post release.
>>    We tested it at D05 using SPARSE mode, it  also takes a long time.
>>>>     yes, this protocol is required because we still using IntelBds,
>>>> and we
>>>> also think that using NullMemoryTestDxe
>>>>     is  better.
>>> Why?
>> It is just not to do the memory testing when switching to NullMemoryTestDxe,
>> only speed up the boot
>> time and no other benefits,
> Yes, but why do you think that using NullMemoryTestDxe is better than
> not including any memory test?
>
> Regards,
>
> Leif
Sorry, I was not very clear about your doubts before; I think I know 
your concern now.
Well, the NullMemoryTestDxe and GenericMemoryTestDxe both have one 
function, which is switching the untested memory of type 
EfiGcdMemoryTypeReserved to EfiGcdMemoryTypeSystemMemory.

The above 4GB memory is not reported as system memory in UEFI memory map 
before BDS on hisilicon platforms for the reason we have talked about 
before. After running memory test protocol, whether it is from 
NullMemoryTestDxe or GenericMemoryTestDxe, memory space above 4GB will 
be switched to EfiGcdMemoryTypeSystemMemory and reported in UEFI memory map.

Thanks,
Heyi

>> but we do not keep to maintain D02 now, so just
>> switch it on D03 and D05.
>>
>> thanks,
>> Heyi
>>> I don't have a D02 here to test on, but it certainly builds fine
>>> without it. And from inspection, the runtime check for it does not
>>> trigger an ASSERT or even an error condition.
>>>
>>> So what benefit does including NullMemoryTestDxe give you?
>>>
>>> Regards,
>>>
>>> Leif
>>>
>>>>     Thanks,
>>>>     Heyi
>>>>
>>>>>>>> Same comment applies to D03 patch.
>>>>>>>>
>>>>>>>> /
>>>>>>>>      Leif
>>>>>>>>
>>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>>>>>>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>>>>>>>>> ---
>>>>>>>>>   Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
>>>>>>>>>   Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
>>>>>>>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>>>> b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>>>> index c11fa4e..d6fbcb9 100644
>>>>>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
>>>>>>>>> @@ -429,7 +429,7 @@
>>>>>>>>>     #
>>>>>>>>>     # Memory test
>>>>>>>>>     #
>>>>>>>>> - MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>>>>>> +
>>>>>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>>>>>> MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>>>>>     MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>>>>>> diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>>>> b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>>>> index ec4d749..c941e4e 100644
>>>>>>>>> --- a/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>>>> +++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
>>>>>>>>> @@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
>>>>>>>>>     #
>>>>>>>>>     INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
>>>>>>>>>   -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
>>>>>>>>> +  INF
>>>>>>>>> MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
>>>>>>>>>     INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
>>>>>>>>>     INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
>>>>>>>>>     INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
>>>>>>>>> -- 
>>>>>>>>> 1.9.1
>>>>>>>>>
Leif Lindholm Dec. 5, 2016, 8:52 a.m. UTC | #10
On Mon, Dec 05, 2016 at 02:25:36PM +0800, Heyi Guo wrote:
> >>>>    It's cost about 2 minutes on 256G memory platform,
> >>>Even the SPARSE one? Sure, that is substantial. But it also sounds too
> >>>much. We should definitely look into that post release.
> >>   We tested it at D05 using SPARSE mode, it  also takes a long time.
> >>>>    yes, this protocol is required because we still using IntelBds,
> >>>>and we
> >>>>also think that using NullMemoryTestDxe
> >>>>    is  better.
> >>>Why?
> >>It is just not to do the memory testing when switching to NullMemoryTestDxe,
> >>only speed up the boot
> >>time and no other benefits,
> >Yes, but why do you think that using NullMemoryTestDxe is better than
> >not including any memory test?
> >
> >Regards,
> >
> >Leif
> Sorry, I was not very clear about your doubts before; I think I know your
> concern now.
> Well, the NullMemoryTestDxe and GenericMemoryTestDxe both have one function,
> which is switching the untested memory of type EfiGcdMemoryTypeReserved to
> EfiGcdMemoryTypeSystemMemory.
> 
> The above 4GB memory is not reported as system memory in UEFI memory map
> before BDS on hisilicon platforms for the reason we have talked about
> before. After running memory test protocol, whether it is from
> NullMemoryTestDxe or GenericMemoryTestDxe, memory space above 4GB will be
> switched to EfiGcdMemoryTypeSystemMemory and reported in UEFI memory map.

OK, that makes more sense.
Can you add that to the commit messages before resending?

Regards,

Leif

> 
> Thanks,
> Heyi
> 
> >>but we do not keep to maintain D02 now, so just
> >>switch it on D03 and D05.
> >>
> >>thanks,
> >>Heyi
> >>>I don't have a D02 here to test on, but it certainly builds fine
> >>>without it. And from inspection, the runtime check for it does not
> >>>trigger an ASSERT or even an error condition.
> >>>
> >>>So what benefit does including NullMemoryTestDxe give you?
> >>>
> >>>Regards,
> >>>
> >>>Leif
> >>>
> >>>>    Thanks,
> >>>>    Heyi
> >>>>
> >>>>>>>>Same comment applies to D03 patch.
> >>>>>>>>
> >>>>>>>>/
> >>>>>>>>     Leif
> >>>>>>>>
> >>>>>>>>>Contributed-under: TianoCore Contribution Agreement 1.0
> >>>>>>>>>Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> >>>>>>>>>---
> >>>>>>>>>  Platforms/Hisilicon/D02/Pv660D02.dsc | 2 +-
> >>>>>>>>>  Platforms/Hisilicon/D02/Pv660D02.fdf | 2 +-
> >>>>>>>>>  2 files changed, 2 insertions(+), 2 deletions(-)
> >>>>>>>>>
> >>>>>>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>>>b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>>>index c11fa4e..d6fbcb9 100644
> >>>>>>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
> >>>>>>>>>@@ -429,7 +429,7 @@
> >>>>>>>>>    #
> >>>>>>>>>    # Memory test
> >>>>>>>>>    #
> >>>>>>>>>- MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>>>>>+
> >>>>>>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>>>>>MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>>>>>    MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>>>>>diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>>>b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>>>index ec4d749..c941e4e 100644
> >>>>>>>>>--- a/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>>>+++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
> >>>>>>>>>@@ -278,7 +278,7 @@ READ_LOCK_STATUS   = TRUE
> >>>>>>>>>    #
> >>>>>>>>>    INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
> >>>>>>>>>  -  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
> >>>>>>>>>+  INF
> >>>>>>>>>MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
> >>>>>>>>>    INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
> >>>>>>>>>    INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
> >>>>>>>>>    INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf
> >>>>>>>>>-- 
> >>>>>>>>>1.9.1
> >>>>>>>>>
>
Jeremy Linton Dec. 5, 2016, 5:29 p.m. UTC | #11
Hi,

On 11/28/2016 11:18 PM, Heyi Guo wrote:
> Hi Leif,
>
> Welcome back :)
>
> Please help to review the RP1612 v4 version patchsets, thanks.
>
> 在 11/29/2016 12:31 AM, Leif Lindholm 写道:
>> (Hi Heyi, I am now back from my holiday.)
>>
>> On Sat, Nov 19, 2016 at 03:41:57PM +0800, Heyi Guo wrote:
>>> Hi Leif,
>>>
>>> We have the test result about the BootMode,
>>> see detail below, please check and let me know your comment.
>>>
>>> Thanks and Regards,
>>> Heyi.
>>>
>>> 在 11/16/2016 8:53 AM, Heyi Guo 写道:
>>>> sorry, I missed the comments previous version, i will try it.
>>>>
>>>> 在 11/16/2016 4:49 AM, Leif Lindholm 写道:
>>>>> On Mon, Nov 14, 2016 at 07:29:50PM +0800, Heyi Guo wrote:
>>>>>> Memory test may take long time when there is a lot of memory in
>>>>>> system,
>>>>>> so we disable memory test in BDS to accelerate boot speed.
>>>>> I am still not a fan of this.
>>>>> Do you have any feedback with regards to the comments I made on the
>>>>> previous version?:
>>>>> ---
>>>>> It would be very much preferable if you could make use of the provided
>>>>> facilities and set your BootMode to BOOT_WITH_DEFAULT_SETTINGS
>>>>> or BOOT_WITH_FULL_CONFIGURATION, which would cut the memory test time
>>>>> to 1/16.
>>>>>
>>>>> Have a look at GenericMemoryTestEntryPoint() and let me know what you
>>>>> think.
>>>>> ---
>>>>>
>>>          We have checked the code and found that the BootMode is already
>>>           BOOT_WITH_FULL_CONFIGURATION at the previous version, but
>>> it is
>>> could not define the test level,
>>>           actually, the test level is passed by the
>>> PlatformBdsPolicyBehavior
>>> at PlatformIntelBdsLib,
>>>           and the level is 'QUICK' now, if we switch it to 'SPARSE'
>>> the test
>>> time
>>>          will cut to 1/4, but the D05 have 16 DIMM slots(D03 8 slots)
>>> and
>>> totally support
>>>          16GB*16(256GB)  memory, the test time is still too long, so
>>> could we
>>>          set the level to 'IGNOR'?
>> For reference, what periods of time are we talking about here?
>>
>> As far as I can see, this protocol is required only because D0* are
>> still using IntelBds? And looking at that code, it will happily skip
>> over doing the memory test (returning EFI_SUCCESS) if the protocol
>> cannot be found.
>>
>> So if we are genuinely looking to remove the feature of verifying that
>> the RAM is basically functional - why are we not just dropping the
>> GenericMemoryTestDxe instead of replacing it with NullMemoryTestDxe?
>>
>> Regards,
>>
>> Leif
>    It's cost about 2 minutes on 256G memory platform,
>    yes, this protocol is required because we still using IntelBds, and
> we also think that using NullMemoryTestDxe
>    is  better.

It seems to me, that these are the kinds of decisions best left up to 
the end user. IMHO, doing some basic hardware sanity checking at power 
on is part of what makes a machine "enterprise". Giving users more 
concerned with boot speed the option to disable (and particularly 
interactively skip) the check is a much better choice than trying to 
make the decision for all the users.

Consider the case of scheduled downtime to install RAM. In that case, 
spending an additional few minutes to sanity check the RAM is a much 
better option than discovering half an hour after boot there is 
something wrong when a particular page starts taking uncorrectable 
errors on first use.

BTW: This isn't all or nothing either, there are a lot of choices about 
how aggressive the POST should be, based on whether the machine detects 
a hardware change, is being cold powered on, warm rebooted, etc.

Thanks,
diff mbox

Patch

diff --git a/Platforms/Hisilicon/D02/Pv660D02.dsc b/Platforms/Hisilicon/D02/Pv660D02.dsc
index c11fa4e..d6fbcb9 100644
--- a/Platforms/Hisilicon/D02/Pv660D02.dsc
+++ b/Platforms/Hisilicon/D02/Pv660D02.dsc
@@ -429,7 +429,7 @@ 
   #
   # Memory test
   #
-  MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
+  MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
 
   MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
diff --git a/Platforms/Hisilicon/D02/Pv660D02.fdf b/Platforms/Hisilicon/D02/Pv660D02.fdf
index ec4d749..c941e4e 100644
--- a/Platforms/Hisilicon/D02/Pv660D02.fdf
+++ b/Platforms/Hisilicon/D02/Pv660D02.fdf
@@ -278,7 +278,7 @@  READ_LOCK_STATUS   = TRUE
   #
   INF MdeModulePkg/Universal/DevicePathDxe/DevicePathDxe.inf
 
-  INF MdeModulePkg/Universal/MemoryTest/GenericMemoryTestDxe/GenericMemoryTestDxe.inf
+  INF MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf
   INF MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe.inf
   INF MdeModulePkg/Universal/SetupBrowserDxe/SetupBrowserDxe.inf
   INF IntelFrameworkModulePkg/Universal/BdsDxe/BdsDxe.inf