Create Chrome OS recovery firmware procedure.

Create Chrome OS recovery firmware procedure

The command cros_rec is the main procedure the recovery firmware runs by default. It also appears in developer image for debug purpose.

BUG=chromium-os:1305
TEST=build successfully and boot like:

U-Boot 2010.09 (Mar 09 2011 - 17:16:01)

Board:   Tegra2 chromeos/tegra2/seaboard/recovery
DRAM:  1 GiB
Using default environment

In:    tegra-kbc
Out:   serial
Err:   serial
Hit any key to stop autoboot:  0 
EMMC 1 Probed Successfully
EMMC 1 Probed Successfully[mmc_legacy_init:231] EMMC 1 Probe Failed
USB:   Tegra ehci init hccr c5008100 and hcor c5008140 hc_length 64
Register 10011 NbrPorts 1
USB EHCI 1.00
scanning bus for devices... 1 USB Device(s) found
       scanning bus for storage devices... 0 Storage Device(s) found
[mmc_legacy_init:231] EMMC 1 Probe Failed
USB:   Tegra ehci init hccr c5008100 and hcor c5008140 hc_length 64
Register 10011 NbrPorts 1
USB EHCI 1.00
scanning bus for devices... 1 USB Device(s) found
       scanning bus for storage devices... 0 Storage Device(s) found
EMMC 1 Probed Successfully
EMMC 1 Probed Successfully
DEBUG: TPM: Startup
DEBUG: TPM: command 0x99 returned 0x0
DEBUG: TPM: Self test full
DEBUG: TPM: command 0x50 returned 0x0
ReadSingle Operation failed.
 mmc_bread:133 READ : failed 
ReadSingle Operation failed.
 mmc_bread:133 READ : failed 
DEBUG: GptNextKernelEntry looking at new prio partition 1
DEBUG: GptNextKernelEntry s1 t15 p15
DEBUG: GptNextKernelEntry looking at new prio partition 3
DEBUG: GptNextKernelEntry s0 t15 p0
DEBUG: GptNextKernelEntry looking at new prio partition 5
DEBUG: GptNextKernelEntry s0 t15 p0
DEBUG: GptNextKernelEntry likes that one
DEBUG: Found kernel entry at 4096 size 32768
DEBUG: Checking key block signature...
DEBUG: Kernel preamble is good.
DEBUG: Partition is good.
DEBUG: In recovery mode or dev-signed kernel
DEBUG: Updating GPT header 2
 Write failed. Block=8388606
 mmc_bwrite:171 WRITE : failed 
DEBUG: Updating GPT entries 2
 Write failed. Block=8388607
 mmc_bwrite:171 WRITE : failed 
DEBUG: Good_partition >= 0
Success; good kernel found on device
partition_number: 2
bootloader_address: 0x68c000bootloader_size: 0x1000partition_guid: 10 22 57 b6 1a 5f 41 e8 b9 fc 75 a2 7c a9 fa 30
## Booting kernel from Legacy Image at 0040c000 ...
   Image Name:   kernel
   Image Type:   ARM Linux Kernel Image (uncompressed)
   Data Size:    5807272 Bytes = 5.5 MiB
   Load Address: 10008000
   Entry Point:  10008000
   Verifying Checksum ... OK
   Loading Kernel Image ... OK
OK

Starting kernel ...

Change-Id: Iee8c703b934c7d7b7fb244355c8f4399bf3d057e

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=dc0cc33

[Tom Wai-Hong Tam, 2010-12-27 02:51:37]

clchiou: reviewer
robotboy: fyi

[Che-Liang Chiou, 2010-12-27 05:35:56]

The code looks generally okay.  Thanks for the effort.

As the offline discussion, please submit another CL for completed version of the
recovery firmware.  CLs for template code should mostly be used for the purpose
of  parallelizing development work.

The recovery firmware is a big part of the verify boot, and so you might want to
split it into multiple individually-tracked subtasks.  Would you open issues on
the public tracker for us?

You may also put test commands in cmd_cros.c for testing each submodule of the
recovery firmware.

Thanks again.

[Tom Wai-Hong Tam, 2010-12-27 09:47:33]

Thanks for your feedback. Please suspend you review and wait me complete all the
dummy code.

[robotboy, 2011-01-05 18:38:47]

Thanks Tom and Che-Lian, this looks like you're on the right track, or at least
on the same track as I am.  I'm just catching up on all of my code reviews from
over the holidays.

Thanks,
    Anton

[robotboy, 2011-03-08 21:07:26]

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c
File common/lcd.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c#newcode84
common/lcd.c:84: char *const argv[]);
This should be a separate CL so that rebasing is easier and so that this could
be upstreamed.

http://codereview.chromium.org/5989010/diff/14001/include/configs/chromeos/te...
File include/configs/chromeos/tegra2/seaboard/recovery.h (left):

http://codereview.chromium.org/5989010/diff/14001/include/configs/chromeos/te...
include/configs/chromeos/tegra2/seaboard/recovery.h:17: "run mmc0_boot"
Two things.  Why only change Seaboard?  And second, everyone currently rely's on
the recovery build to be able to boot their systems from SD or USB.  We need to
make sure that we don't break everything with this change.  It might be time to
switch everyone over to the developer config, or if you think it's time switch
to the full firmware image...

[Che-Liang Chiou, 2011-03-09 02:37:53]

I scan through the CL and did a partial review.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c
File common/cmd_cros_rec.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newcode2
common/cmd_cros_rec.c:2: * Copyright 2010, Google Inc.
2011. I would suggest we use the "Chromium OS Authors" copyright header.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:63: int lcd_clear();
Where is this function defined? Would you add a comment for this?

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:65: int g_is_dev = 0;
Why use a global variable, instead of polling the gpio each time? Is that
because you want to ignore later changed gpio status?

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:112: GoogleBinaryBlockHeader *gbbh = (void *)(TEXT_BASE +
CONFIG_OFFSET_GBB);
This address might not be valid. Please use the interface defined in
firmware_storage.h

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:206: (uint8_t *)(TEXT_BASE + CONFIG_OFFSET_GBB), scr);
This address of GBB might not be valid. Please use interface defined in
firmware_storage.h and load GBB into memory.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:212: ScreenIndex scr;
What's scr used for? I don't see any use of it except on line 220.

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c
File common/lcd.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c#newcode403
common/lcd.c:403: int lcd_clear()
Maybe you should put a declaration of lcd_clear() in lcd.h so that you can
include lcd.h in cmd_cros_rec.c?

[Che-Liang Chiou, 2011-03-09 06:55:26]

Hi Tom,

I finished code review. Thanks for the efforts.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c
File common/cmd_cros_rec.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:78: return mmc_legacy_init(1) == 0;
Why pass 1? Is this value cross-platform? If not, please add a FIXME comment
here.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:84: usb_stop();
Do we have to restart usb here? Should add a FIXME here (there should be a
better way to poll usb status).

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:88: return usb_stor_scan(1) == 0;
Same as above. (Why 1?)

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:106: int load_kernel(uint8_t *load_addr, size_t load_size)
Normal firmware also needs to call LoadKernel(). I had a CL for a wrapper of
LoadKernel(), but was not temporarily held off. We should share this part of
code. Please take a look of that CL.

http://codereview.chromium.org/6628009/

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:220: scr = SCREEN_RECOVERY_MODE;
Never use this variable hereafter.

[Tom Wai-Hong Tam, 2011-03-09 10:15:05]

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c
File common/cmd_cros_rec.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newcode2
common/cmd_cros_rec.c:2: * Copyright 2010, Google Inc.
On 2011/03/09 02:37:53, Che-Liang Chiou wrote:
> 2011. I would suggest we use the "Chromium OS Authors" copyright header.

Done.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:63: int lcd_clear();
Fixed it. Now it is included in lcd.h.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:65: int g_is_dev = 0;
Checking it once can keep the logic simple and security. An example use case is
like: developer mode on, boot up => show the developer image => user press space
bar => enter recovery mode => USER SWITCH DEVELOPER OFF => insert a SD card. We
still consider it is developer mode on because the user saw the developer screen
before and thought it is on. It seems confused when we consider it is developer
mode off.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:78: return mmc_legacy_init(1) == 0;
There are 2 MMC devices in the system. 0 is the main disk and 1 is the SD card.
I changed it to more readable.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:84: usb_stop();
OK. I added a TODO.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:88: return usb_stor_scan(1) == 0;
Now 1 is the mode that to probe storage device.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:106: int load_kernel(uint8_t *load_addr, size_t load_size)
Will change it when your CL is committed.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:112: GoogleBinaryBlockHeader *gbbh = (void *)(TEXT_BASE +
CONFIG_OFFSET_GBB);
On 2011/03/09 02:37:53, Che-Liang Chiou wrote:
> This address might not be valid. Please use the interface defined in
> firmware_storage.h

Done.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:206: (uint8_t *)(TEXT_BASE + CONFIG_OFFSET_GBB), scr);
On 2011/03/09 02:37:53, Che-Liang Chiou wrote:
> This address of GBB might not be valid. Please use interface defined in
> firmware_storage.h and load GBB into memory.

Done.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:212: ScreenIndex scr;
My mistake. Fixed.

http://codereview.chromium.org/5989010/diff/14001/common/cmd_cros_rec.c#newco...
common/cmd_cros_rec.c:220: scr = SCREEN_RECOVERY_MODE;
Fixed.

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c
File common/lcd.c (right):

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c#newcode84
common/lcd.c:84: char *const argv[]);
On 2011/03/08 21:07:26, robotboy wrote:
> This should be a separate CL so that rebasing is easier and so that this could
> be upstreamed.

Done.

http://codereview.chromium.org/5989010/diff/14001/common/lcd.c#newcode403
common/lcd.c:403: int lcd_clear()
On 2011/03/09 02:37:53, Che-Liang Chiou wrote:
> Maybe you should put a declaration of lcd_clear() in lcd.h so that you can
> include lcd.h in cmd_cros_rec.c?

Done.

http://codereview.chromium.org/5989010/diff/14001/include/configs/chromeos/te...
File include/configs/chromeos/tegra2/seaboard/recovery.h (left):

http://codereview.chromium.org/5989010/diff/14001/include/configs/chromeos/te...
include/configs/chromeos/tegra2/seaboard/recovery.h:17: "run mmc0_boot"
Reverted it as before. The current recovery firmware is not so stable to use it
as default. Will change it when recovery mode is stable.

[robotboy, 2011-03-10 00:17:10]

I have not done a review of the core code (cms_cros_rec.c).  But all of the
interface of that code to the build and configurations LGTM.

Thanks,
    Anton

[Che-Liang Chiou, 2011-03-10 07:13:02]

Hi Tom,

Please update with latest vboot_reference changes. You may see it here:
http://codereview.chromium.org/6626045/
http://codereview.chromium.org/6658009/

On 2011/03/10 00:17:10, robotboy wrote:
> I have not done a review of the core code (cms_cros_rec.c).  But all of the
> interface of that code to the build and configurations LGTM.
> 
> Thanks,
>     Anton

[Tom Wai-Hong Tam, 2011-03-11 08:02:16]

Thanks. I updated to use the new vboot struct.

[Che-Liang Chiou, 2011-03-14 06:32:08]

LGTM, thanks.