New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 797853 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

[uni-build] Coreboot always add EC from the same build target even if EC is set as different one

Project Member Reported by marcochen@chromium.org, Dec 28 2017

Issue description

The master configuration allows to set individual build target for coreboot, ec, depthcharge and libpayload. For example, we can set "coreboot = coral" and "ec = other" then we expect that ec will be built by target - other and it will be added into coreboot image too. 

In current code base, ebuild of coreboot will always operate things on the build target of itself even if EC is on the different target. Finally coreboot will embed EC with coral not other.
 
Cc: adurbin@chromium.org nsanders@chromium.org sjg@chromium.org pbe...@chromium.org
In coreboot-9999.ebuild, I tried to get build targets of EC and map to coreboot ones so expect that pairs of coreboot and ec can be identified. But actually the command like [1] just get merged build targets so what I get for coreboot and ec will be (coral) and (other, coral). As the result, I still can't map them together.

[1] cros_config_host_py get-firmware-build-targets ec
[2] https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/845252
As issue title mentioned, depthcharge are also with the same issue. In ebuild of chromeos-bootimage, the build target is also from coreboot as an additional folder name and be used to get elf file of depthcharge.

Comment 4 by sjg@chromium.org, Dec 29 2017

Just a comment in case this is requesting a unibuild change...

The build targets only specify what targets get built. There is no control (within the unibuild config) over what is actually selected to go in the image.

I am guessing that coreboot ebuild selects which EC it wants, and that it probably picks up the same one as its build target. But I am not sure.

Perhaps we could enhance the ebuild to tell coreboot which EC image to pick up?

What is ec 'other'?
re#4

> I am guessing that coreboot ebuild selects which EC it wants, and that it probably 
> picks up the same one as its build target.

Yes, please refer to the second paragraph in the issue description.

> Perhaps we could enhance the ebuild to tell coreboot which EC image to pick up?

If we really allow different build targets for each [coreboot|ec|depthcharge|libpayload] (not only EC) then this improvement would need to be planed and supported.

>> "ec = other"
> What is ec 'other'?

"other" just means another build target different than "coral".

Comment 6 by sjg@chromium.org, Jan 2 2018

So this bug is requesting an enhancement / change to the coreboot ebuild?
Labels: -Pri-3 Pri-2
Aaron, do you know how firmwares for various boards are built from the same firmware branch for other systems, like reef?

I think this bug is addressing the need to build different firmware targets per model for the same unibuild image in a firmware branch. I don't think this has come up before. 

Basically we need to be able to build per-model coreboots, but in the past, we've only ever needed to build one coreboot image per overlay. So this build process probably doesn't exist now.

I think possible solutions are: 
* Modify the coreboot build to be unibuild aware and produce multiple images, similar to how the ec build works now
* Make a new overlay per model for firmware builds and don't use unibuild in firmware branches / firmware builders.

Bumping priority since this presumably blocks any babymega fw development.
re#6

Yes, the coreboot ebuild should be enhanced to support uni-build in which build targets of coreboot and EC in a model are set to different ones.
re#7,

> * Modify the coreboot build to be unibuild aware and produce multiple images, similar 
> to how the ec build works now
1. Refer to [1], currently coreboot ebuild already looks into unibuild config and tries to make_coreboot multiple times for individual build targets of coreboot.

2. Take an example of unibuild config to describe the issue here:
   
    Example [2] shows that a project - babymako would like to leverage coral's coreboot/depthcharge/libpayload but needs different build target of EC. In this case, coreboot ebuild will call add_ec to embed EC image into coreboot but the EC image is came from build target - coral not babymako actually. Because coreboot ebuild always refer to build target from coreboot only to fetch other resources.

    In the end, if you set the same build target of these 4 firmware resources then it is ok. But if one of them is set to different build target then built coreboot is not correct.

3. Workaround now: I can only try to create all 4 firmware resources to the same and new build target even though I only need EC from different build target.

[1] https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/sys-boot/coreboot/coreboot-9999.ebuild#352
[2]
    shared_babymako: babymako {
        ...    		
        build-targets {
	    coreboot = "coral";
	    ec = "babymako";
	    depthcharge = "coral";
	    libpayload = "coral";
	};
    };
Owner: shapiroc@chromium.org
Owner: jclinton@chromium.org
The historical unibuild team doesn't currently have time to work on this (the unibuild project is essentially done modulo some cleanups that we have in flight this quarter) and the firmware folks are more familiar with the firmware build process. Can the firmware team implement this feature request? Unibuilds are the steady state going forward for CrOS so it seems like a worthwhile investment.

Looking at this briefly, it looks like supporting this in a legacy build would have been a hack to the Coreboot build so it's not like unibuild introduced a new bug.

Cc: teravest@chromium.org jettrink@chromium.org
Cc: shapiroc@chromium.org jclinton@chromium.org
 Issue 819363  has been merged into this issue.
Labels: -Pri-2 Pri-1
Owner: teravest@chromium.org
Status: Started (was: Untriaged)
Cc: furquan@chromium.org
Cc: yueherngl@chromium.org
Some patches are out now; the plan is to introduce a new command to cros_config_host_py that can be used to get the relevant tuples of firmware components for a given master config.

Following up on that I have a change to chromeos-bootimage to select coreboot/depthcharge properly, and I'll make a similar change to coreboot to pick the proper EC.
Project Member

Comment 19 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/084abd1ff74922e2724443987eefc05f64f161da

commit 084abd1ff74922e2724443987eefc05f64f161da
Author: Justin TerAvest <teravest@chromium.org>
Date: Thu Mar 08 06:08:08 2018

chromeos-config: Add command for firmware combinations

chromeos-bootimage needs to look up the relevant coreboot/depthcharge
pairs when assembling a bootimage. coreboot needs to do something
similar for building coreboot/EC pairs.  The commands that exist in
cros_config_host today aren't sufficient for this, so this commit
introduces a new command that returns all of the sets of build targets
for a given config.

TEST=run_tests.sh
TEST=python -m cros_config_host.cros_config_host get-firmware-build-combinations coreboot,ec,depthcharge for CL:*583896
BUG= chromium:797853 

Change-Id: I8410a479e15a2aeaeb42d0b487d31a4fac057717
Reviewed-on: https://chromium-review.googlesource.com/953124
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>

[modify] https://crrev.com/084abd1ff74922e2724443987eefc05f64f161da/chromeos-config/cros_config_host/cros_config_host.py
[modify] https://crrev.com/084abd1ff74922e2724443987eefc05f64f161da/chromeos-config/cros_config_host/libcros_config_host_unittest.py
[modify] https://crrev.com/084abd1ff74922e2724443987eefc05f64f161da/chromeos-config/cros_config_host/libcros_config_host.py

Project Member

Comment 20 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/fd6977dd3bfc6b46e1ea048ec83e20a3f7385664

commit fd6977dd3bfc6b46e1ea048ec83e20a3f7385664
Author: Justin TerAvest <teravest@chromium.org>
Date: Tue Mar 13 07:57:38 2018

coreboot: Remove unneeded fsp config logic

Historically, samus used a separate config file when USE="fsp" was true.
However, that file was removed in 2016, and there are no other files
that match the pattern. I came across this when reading through
"current" board logic for fixing  crbug.com/797853 

BUG= chromium:797853 
TEST=None

Change-Id: I2d67e6fb4b00d2f585bf6e0fdccaa3731483d3ab
Reviewed-on: https://chromium-review.googlesource.com/957321
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Reviewed-by: Aaron Durbin <adurbin@chromium.org>
Reviewed-by: Furquan Shaikh <furquan@chromium.org>
Reviewed-by: Duncan Laurie <dlaurie@google.com>
Reviewed-by: Patrick Georgi <pgeorgi@chromium.org>

[modify] https://crrev.com/fd6977dd3bfc6b46e1ea048ec83e20a3f7385664/sys-boot/coreboot/coreboot-9999.ebuild

Project Member

Comment 21 by bugdroid1@chromium.org, Mar 13 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/6be966e040c477749c46e5235e6378e461178685

commit 6be966e040c477749c46e5235e6378e461178685
Author: Justin TerAvest <teravest@chromium.org>
Date: Tue Mar 13 07:57:30 2018

coreboot: Allow different ec build target

Previously, EC images built into coreboot always shared the name with
the build target. However, the configuration provided in
chromeos-config-bsp allows the two fields to have different values. This
change allows the correct EC to be picked up with the two differ.

BUG= chromium:797853 ,b:72807332
TEST=Changed ec to "other", saw attempt to fetch from new path

Change-Id: Ic71fe671996963cdcc080d7661c89c8007e5369c
Reviewed-on: https://chromium-review.googlesource.com/957584
Commit-Ready: Justin TerAvest <teravest@chromium.org>
Tested-by: Justin TerAvest <teravest@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Reviewed-by: C Shapiro <shapiroc@google.com>

[modify] https://crrev.com/6be966e040c477749c46e5235e6378e461178685/sys-boot/coreboot/coreboot-9999.ebuild

Status: Fixed (was: Started)

Sign in to add a comment