[coreboot-gerrit] Change in ...coreboot[master]: arch/x86/Kconfig: move MAX_REBOOT_CNT option

[Nico Huber (Code Review)]

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/29979 )

Change subject: arch/x86/Kconfig: move MAX_REBOOT_CNT option
......................................................................


Patch Set 2:

>> Everything below the currently defined defaults is wrong.
> 
> why are they wrong? I tested it with values below and it behaves
> like it should.

Tested on what board and tested how? With/without MRC cache might make
a difference for instance, plus there are about half a dozen of software
reset methods, which one was used?

Take modern Intel systems for instance. It's possible to reset the
CPU only, causing an INIT, the cores reset the x86 register state,
BSP restarts at the reset vector. Let's call it Reset#1. This is not
enough to rerun coreboot, though. Hence you'll usually find a 0x6 ->
0xcf9 reset (aka. system reset, still a warm reset) early in the
romstage: Reset#2. If no mrc.cache is present (bug or not) the raminit
will refuse to retrain on warm resets: 0xe -> 0xcf9 (full reset, goes
into S5 and back into S0 after a few seconds, hence a cold reset).
That'd be Reset#3.

So if MAX_REBOOT_CNT is < 3 and you hit the path described above, it
will run into fallback for no reason.

While that's the only path that comes to mind atm, there may be more
like that. If you can prove that a board doesn't have any valid path
with 3 resets, you can set MAX_REBOOT_CNT lower, and I won't call it
wrong ;)

>
>> In other words, this option was hidden on purpose. You can add another
>> option for instance and take the minimum of both or something like
>> that. But you mustn't lower it.
> 
> An other option would be to adapt the range in the Kconfig or fix
> the issue which cause this wrong behavior.

What wrong behaviour?

The option has a misleading name. Maybe that's what causes the
confusion. What it actually represents should be something like
MAX_EXPECTED_RESET_COUNT_ON_A_SINGLE_REBOOT.


-- 
To view, visit https://review.coreboot.org/c/coreboot/+/29979
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings

Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Change-Id: Ice26e2ef349f1172b564edc14f1012c33546a93c
Gerrit-Change-Number: 29979
Gerrit-PatchSet: 2
Gerrit-Owner: Marcello Sylvester Bauer <sylvblck at sylv.io>
Gerrit-Reviewer: Marcello Sylvester Bauer <sylvblck at sylv.io>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-CC: Nico Huber <nico.h at gmx.de>
Gerrit-Comment-Date: Sun, 02 Dec 2018 11:00:57 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.coreboot.org/pipermail/coreboot-gerrit/attachments/20181202/d29a2a32/attachment.html>