Add a wrapper function of LoadKernel()

Add a wrapper function of LoadKernel()

BUG=chromium-os:1034
TEST=MAKEALL successfully

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

[sjg, 2011-03-06 21:34:09]

Please see comments

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c#new...
lib/chromeos/load_util.c:16: int load_kernel(LoadKernelParams *params, uint8_t
*kernel_sign_key_blob,
It's not great having load_kernel() and LoadKernel(). Can you think of another
name? Is this basically just packing params into the struct?

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c#new...
lib/chromeos/load_util.c:17: void *kernel_buffer, uint64_t kernel_buffer_size,
Why void* rather than char*? If an address perhaps just an u_int32_t address
would do?

[Che-Liang Chiou, 2011-03-07 05:06:51]

Hi Simon,

Thanks for comments. I should close this issue.

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c#new...
lib/chromeos/load_util.c:16: int load_kernel(LoadKernelParams *params, uint8_t
*kernel_sign_key_blob,
On 2011/03/06 21:34:09, sjg wrote:
> It's not great having load_kernel() and LoadKernel(). Can you think of another
> name? Is this basically just packing params into the struct?

I think you are right. I was thinking that this function can be reused by
rewritable firmware and recovery firmware. But maybe we should wait until the
calling pattern to LoadKernel() is clearer. After that we factor out common part
and share codes.

http://codereview.chromium.org/6628009/diff/2001/lib/chromeos/load_util.c#new...
lib/chromeos/load_util.c:17: void *kernel_buffer, uint64_t kernel_buffer_size,
On 2011/03/06 21:34:09, sjg wrote:
> Why void* rather than char*? If an address perhaps just an u_int32_t address
> would do?

I am not sure if I am making the right choice here: I use void* because
vboot_reference uses void* for kernel_buffer field of LoadKernelParams.

If you think we should use char* regardless of what type vboot_reference, I am
okay with that.

[Che-Liang Chiou, 2011-03-09 06:49:33]

Re-open this CL because the CL for recovery firmware also needs a wrapper of
LoadKernel(). I think it's time to share code between both sides. See
http://codereview.chromium.org/5989010/

[robotboy, 2011-03-11 18:56:18]

http://codereview.chromium.org/6628009/diff/18001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/18001/lib/chromeos/load_util.c#ne...
lib/chromeos/load_util.c:105: release_firmware_storage(&file);
Should the malloced gbb_data be freed here if the status is still
LOAD_KERNEL_NOT_FOUND?  Or is the called expected to do that even on failure?

[Che-Liang Chiou, 2011-03-14 05:51:40]

Thanks for the comments. PTAL.

http://codereview.chromium.org/6628009/diff/18001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/18001/lib/chromeos/load_util.c#ne...
lib/chromeos/load_util.c:105: release_firmware_storage(&file);
On 2011/03/11 18:56:18, robotboy wrote:
> Should the malloced gbb_data be freed here if the status is still
> LOAD_KERNEL_NOT_FOUND?  Or is the called expected to do that even on failure?

Good point. Change the logic here to 'not fee gbb only when success'.

[Tom Wai-Hong Tam, 2011-03-14 09:37:27]

http://codereview.chromium.org/6628009/diff/22001/include/chromeos/load_util.h
File include/chromeos/load_util.h (right):

http://codereview.chromium.org/6628009/diff/22001/include/chromeos/load_util....
include/chromeos/load_util.h:25: int load_kernel_wrapper(LoadKernelParams
*params, uint64_t boot_flags);
It seems confused that we mix loading GBB with loading kernel. Better to
separate these two things into 2 functions: one is load_gbb(void *gbb_data), and
other is load_kernel_wrapper(LoadKernelParams *params, void *gbb_data, uint64_t
boot_flags). Such that it is clear to see when to free the gbb_data.

http://codereview.chromium.org/6628009/diff/22001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/22001/lib/chromeos/load_util.c#ne...
lib/chromeos/load_util.c:42: if (boot_flags | BOOT_FLAG_RECOVERY) {
The condition should be "boot_flags & BOOT_FLAG_RECOVERY".

[Che-Liang Chiou, 2011-03-14 10:39:34]

Done. PTAL.

http://codereview.chromium.org/6628009/diff/22001/include/chromeos/load_util.h
File include/chromeos/load_util.h (right):

http://codereview.chromium.org/6628009/diff/22001/include/chromeos/load_util....
include/chromeos/load_util.h:25: int load_kernel_wrapper(LoadKernelParams
*params, uint64_t boot_flags);
On 2011/03/14 09:37:27, Tom Wai-Hong Tam wrote:
> It seems confused that we mix loading GBB with loading kernel. Better to
> separate these two things into 2 functions: one is load_gbb(void *gbb_data),
and
> other is load_kernel_wrapper(LoadKernelParams *params, void *gbb_data,
uint64_t
> boot_flags). Such that it is clear to see when to free the gbb_data.

Good point! Will do.

http://codereview.chromium.org/6628009/diff/22001/lib/chromeos/load_util.c
File lib/chromeos/load_util.c (right):

http://codereview.chromium.org/6628009/diff/22001/lib/chromeos/load_util.c#ne...
lib/chromeos/load_util.c:42: if (boot_flags | BOOT_FLAG_RECOVERY) {
On 2011/03/14 09:37:27, Tom Wai-Hong Tam wrote:
> The condition should be "boot_flags & BOOT_FLAG_RECOVERY".

You are right. My typo.

[Tom Wai-Hong Tam, 2011-03-15 03:54:00]

LGTM

[Peter Huewe (IFX), 2011-03-15 10:41:05]

Does not compile for me :/

load_util.c: In function 'load_kernel_wrapper':
load_util.c:59: error: 'LoadKernelParams' has no member named 'gbb_data'
load_util.c:60: error: 'LoadKernelParams' has no member named 'gbb_size'
load_util.c:65: error: 'LoadKernelParams' has no member named 'shared_data_blob'
load_util.c:66: error: 'LoadKernelParams' has no member named 'shared_data_size'
load_util.c:68: error: 'LoadKernelParams' has no member named 'shared_data_blob'
load_util.c:70: error: 'LoadKernelParams' has no member named 'shared_data_size'
load_util.c:70: error: 'VB_SHARED_DATA_REC_SIZE' undeclared (first use in this
function)
load_util.c:70: error: (Each undeclared identifier is reported only once
load_util.c:70: error: for each function it appears in.)


Compiled using: cros_workon_make --board=tegra2_seaboard chromeos-u-boot-next

[Peter Huewe (IFX), 2011-03-15 13:27:41]

Updating the whole chromeos environment helped - so please ignore my previous
message.