Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

intel: adsp: fix firmware image in IMR overwriting #76196

Merged
merged 1 commit into from
Jul 27, 2024

Conversation

lyakh
Copy link
Collaborator

@lyakh lyakh commented Jul 23, 2024

The IMR is used by the firmware to hold its own copy for hot-booting and for an "L3 heap," used for slow large allocations like loadable libraries. The beginning of the L3 heap is currently hard-coded and now the firmware has grown too large to fit into the dedicated area so that it gets overwritten by heap allocations. This is a critical bug that needs an urgent solution, for which we increase the offset, but a real fix must calculate the L3 heap offset automatically.

BugLink: thesofproject/sof#9308

The IMR is used by the firmware to hold its own copy for hot-booting
and for an "L3 heap," used for slow large allocations like loadable
libraries. The beginning of the L3 heap is currently hard-coded and
now the firmware has grown too large to fit into the dedicated area
so that it gets overwritten by heap allocations. This is a critical
bug that needs an urgent solution, for which we increase the offset,
but a real fix must calculate the L3 heap offset automatically.

BugLink: thesofproject/sof#9308
Signed-off-by: Guennadi Liakhovetski <[email protected]>
@zephyrbot zephyrbot added the platform: Intel ADSP Intel Audio platforms label Jul 23, 2024
@lyakh lyakh requested review from lgirdwood and marc-hb and removed request for lgirdwood and marc-hb July 23, 2024 08:14
@tmleman
Copy link
Collaborator

tmleman commented Jul 23, 2024

During debugging, I found another thing. At this line in boot.c, IMRSTACK points to IMR_BOOT_LDR_MANIFEST_BASE (0xa1042000), shouldn't it rather be IMR_BOOT_LDR_STACK_BASE?

@tmleman tmleman added the bug The issue is a bug, or the PR is fixing a bug label Jul 23, 2024
@lyakh
Copy link
Collaborator Author

lyakh commented Jul 23, 2024

During debugging, I found another thing. At this line in boot.c, IMRSTACK points to IMR_BOOT_LDR_MANIFEST_BASE (0xa1042000), shouldn't it rather be IMR_BOOT_LDR_STACK_BASE?

@tmleman yes, I stumbled over that too, but I don't know what the correct fix there should be. I think IMRSTACK should point to the top of the stack, so it might coincide with the beginning of some area, but there it doesn't seem apparent to me that the area below the manifest is reserved for the stack? I traced back that stack == manifest idea to some 2.5 year old commit 692770a . But back then we probably didn't re-use the firmware IMR image for resuming, so now we have to revisit that design... And at least to document it well. But I'd really like to know what is in that area of IMR below the manifest? If there's really nothing then maybe we have to free it or use it in some meaningful way.

@lyakh
Copy link
Collaborator Author

lyakh commented Jul 23, 2024

A link to an explanation of the IMR memory layout and how parts of it get corrupted thesofproject/sof#9308 (comment)

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix is sound. Agreed that the the tradition of manually-typed layout offsets in SOF (well, "SOF-derived code") has been stretched past the breaking point and needs a rework.

@marc-hb marc-hb added the priority: high High impact/importance bug label Jul 23, 2024
@marc-hb
Copy link
Collaborator

marc-hb commented Jul 23, 2024

I just filed new issue #76247 to discuss and track a longer term solution.

@nashif nashif merged commit 293fa11 into zephyrproject-rtos:main Jul 27, 2024
29 checks passed
marc-hb added a commit to marc-hb/sof that referenced this pull request Aug 5, 2024
This reverts commit 8847de0.

This should now work thanks to the "better" IMR addresses in
zephyrproject-rtos/zephyr#76196

Note the longer term issue is still open:
zephyrproject-rtos/zephyr#76247

Signed-off-by: Marc Herbert <[email protected]>
kv2019i pushed a commit to thesofproject/sof that referenced this pull request Aug 6, 2024
This reverts commit 8847de0.

This should now work thanks to the "better" IMR addresses in
zephyrproject-rtos/zephyr#76196

Note the longer term issue is still open:
zephyrproject-rtos/zephyr#76247

Signed-off-by: Marc Herbert <[email protected]>
@lyakh lyakh deleted the imr branch August 12, 2024 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug platform: Intel ADSP Intel Audio platforms priority: high High impact/importance bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants