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

Issue 739771 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

ec: Build should analyze shared_mem_acquire() calls

Project Member Reported by rspangler@chromium.org, Jul 6 2017

Issue description

The size of shared memory is determined by the amount of free RAM left after static allocations.  The size of allocations is typically fixed/known at compile time.  We should be able to determine at compile time if any of the calls to shared_mem_acquire() will always fail.  If so, we should fail the build.

Incentive: I just broke my cr50 system by increasing a task stack size large enough that the firmware update TPM commands can't allocate a page-size buffer in shmem.  Of course, I didn't notice until I tried to update from the broken firmware and discovered I couldn't.

 
Labels: OS-Chrome
Cc: vpalatin@chromium.org
Owner: drinkcat@chromium.org
Status: Started (was: Available)
I have a very basic idea how to do this, at least for non-cr50 boards (cr50 uses malloc, so it'd be more difficult to do this analysis correctly).

One possible implementation here: https://chromium-review.googlesource.com/c/chromiumos/platform/ec/+/1002713/1, comments welcome.
This bug reminds me https://b.corp.google.com/issues/35582029, and I forget to rebase the change from glados to master: 

https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/408216
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/3a9b89116fd0b3e358706f9c5822d4ee944978b6

commit 3a9b89116fd0b3e358706f9c5822d4ee944978b6
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Wed Apr 11 02:13:28 2018

nautilus: Shrink accelerometer FIFO to 512 entries

It appears that the shared memory buffer on Nautilus starts at
200c7720 D __shared_mem_buf

That's 29.78 kb into the RAM. Software sync needs 1kb, so we should
be fine, expect that the last 2kb of RAM are supposed to be reserved
for the "booter" (NPCX_BTRAM_SIZE).

We shrink the accelerometer FIFO to 512 entries, freeing up 4kb of
RAM, and increase the UART TX buffer to 4kb, to make use of 3kb of
that freed up space:
200c7320 D __shared_mem_buf

That's 28.78 kb into the RAM.

BRANCH=poppy
BUG= chromium:739771 
TEST=make BOARD=nautilus -j, check that shared_mem_buf offset is
     < 29 kb.

Change-Id: I361a439b847d31d3415f2ee66229bd32f8816e2d
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1002712
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Philip Chen <philipchen@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@google.com>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/3a9b89116fd0b3e358706f9c5822d4ee944978b6/board/nautilus/board.h

Project Member

Comment 5 by bugdroid1@chromium.org, Apr 11 2018

Labels: merge-merged-firmware-poppy-10431.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/c9bc9f1d2b7b623b78ebd4babfd2cce7a5c0b79f

commit c9bc9f1d2b7b623b78ebd4babfd2cce7a5c0b79f
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Wed Apr 11 03:32:19 2018

nautilus: Shrink accelerometer FIFO to 512 entries

It appears that the shared memory buffer on Nautilus starts at
200c7720 D __shared_mem_buf

That's 29.78 kb into the RAM. Software sync needs 1kb, so we should
be fine, expect that the last 2kb of RAM are supposed to be reserved
for the "booter" (NPCX_BTRAM_SIZE).

We shrink the accelerometer FIFO to 512 entries, freeing up 4kb of
RAM, and increase the UART TX buffer to 4kb, to make use of 3kb of
that freed up space:
200c7320 D __shared_mem_buf

That's 28.78 kb into the RAM.

BRANCH=poppy
BUG= chromium:739771 
TEST=make BOARD=nautilus -j, check that shared_mem_buf offset is
     < 29 kb.

Change-Id: I361a439b847d31d3415f2ee66229bd32f8816e2d
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1002712
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Reviewed-by: Philip Chen <philipchen@chromium.org>
Reviewed-by: Gwendal Grignou <gwendal@google.com>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>
(cherry picked from commit 3a9b89116fd0b3e358706f9c5822d4ee944978b6)
Reviewed-on: https://chromium-review.googlesource.com/1006315
Commit-Queue: Philip Chen <philipchen@chromium.org>
Tested-by: Philip Chen <philipchen@chromium.org>

[modify] https://crrev.com/c9bc9f1d2b7b623b78ebd4babfd2cce7a5c0b79f/board/nautilus/board.h

Project Member

Comment 6 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d

commit 6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d
Author: Nicolas Boichat <drinkcat@chromium.org>
Date: Tue May 08 03:45:54 2018

shared_mem: Assert that shared memory size is large enough

We add a configuration option to set the minimum shared memory
size (CONFIG_SHAREDMEM_MINIMUM_SIZE), so that the link will fail
if there is not enough IRAM left.

Also, we add 2 macros around shared_mem_acquire, that check, at
build time, that the shared memory size is sufficient for the
allocation:
 - SHARED_MEM_ACQUIRE_CHECK should be used instead of
   shared_mem_acquire, when size is known in advance.
 - SHARED_MEM_CHECK_SIZE should be used when only a maximum size
   is known.

This does not account for "jump tags" that boards often add on
jump from RO to RW. Luckily, RW usually does not do verification,
and does not need as much shared memory.

BRANCH=none
BUG= chromium:739771 
TEST=make buildall -j, no error

Change-Id: Ic4c72938affe65fe8f8bc17ee5111c1798fc536f
Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1002713
Reviewed-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Vincent Palatin <vpalatin@chromium.org>

[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/vboot_hash.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/core/cortex-m/ec.lds.S
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/include/shared_mem.h
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/core/cortex-m0/ec.lds.S
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/keyboard_8042.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/vboot/common.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/host_command.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/include/config.h
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/nvmem_vars.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/common/rwsig.c
[modify] https://crrev.com/6e8cbe40eea777aeaeffe0fa0781a0a5d1763a4d/core/nds32/ec.lds.S

Status: Fixed (was: Started)

Sign in to add a comment