| Message ID | 1479122995-50330-24-git-send-email-heyi.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 >
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 >>
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 >>> >
(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 > >>> > > >
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 >>>>>
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 > >>>>> >
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 >>>>>>>
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 > >>>>>>> >
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 >>>>>>>>>
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 > >>>>>>>>> >
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 --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
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(-)