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.

Sign up for GitHub

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

Jump to bottom

board: cavs15: Add a option to control signing ways #31902

Merged
merged 2 commits into from
Feb 11, 2021
Merged

board: cavs15: Add a option to control signing ways #31902

merged 2 commits into from
Feb 11, 2021

Conversation

KangJianX
Copy link
Collaborator

@KangJianX KangJianX commented Feb 3, 2021 •
edited
Loading

Fixed #31819

Zephyr testcases(not SOF case) not use kernel DSP driver to load image
on ADSP board, thus do not need signing with xman. So add a 'no-manifest'
to specify signing without xman in image. If use DSP driver load image,
we should not specify.
And fixed a syntax error in script when download firmware. When copy the data to buffer, the old script can not assign sequence of same size, so specify size.

Signed-off-by: Jian Kang [email protected]

@KangJianX KangJianX requested review from nashif, lyakh and tedd-an February 3, 2021 05:36
@KangJianX KangJianX requested a review from LeiW000 February 3, 2021 06:01
lyakh
lyakh reviewed Feb 3, 2021
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

looks good! Just a bit of clarification / clean up maybe

scripts/west_commands/sign.py Outdated
@@ -112,6 +112,8 @@ def do_add_parser(self, parser_adder):
help='''path to the tool itself, if needed''')
group.add_argument('-D', '--tool-data', default=None,
help='''path to tool data/configuration directory, if needed''')
group.add_argument('-X', '--no-xman', action='store_true',
help='''signing image without xman, if needed''')
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe just '''do not add extended manifest'''

Copy link
Collaborator Author

@KangJianX KangJianX Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, improved the description.

boards/xtensa/intel_adsp_cavs15/tools/lib/stream_desc.py
@@ -38,7 +38,7 @@ def copy(self, data, size):
raise ValueError("Not enough buffer. allocated: %d requested: %d"
% (self.alloc_size, size))
logging.debug("Copying Data to DMA buffer")
self.buf[:] = data[:]
self.buf[:size] = data[:size]
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this related or needed? Was the original version wrong?

Copy link
Collaborator Author

@KangJianX KangJianX Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The old version only can assign sequence of same size , buffer and data not same size, so need this change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

it changed now because you removed the manifest from the image?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This change is not due to remove manifest. Even if add manifest, the buffer and data size still not same, it is fix for load firmware process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, so it isn't really related to the main topic of this commit...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, should I use a new commit for this change?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that would be easier to understand, yes. Thanks!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done, do you think it's ok?

@KangJianX KangJianX force-pushed the adsp_west_script branch 2 times, most recently from fafecb2 to f53699f Compare February 3, 2021 09:09
lyakh
lyakh approved these changes Feb 3, 2021
View reviewed changes
Copy link
Collaborator

@lyakh lyakh left a comment

Choose a reason for hiding this comment

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

Thanks for addressing comments!

@zephyrbot zephyrbot added area: West West utility platform: Intel ADSP Intel Audio platforms labels Feb 3, 2021
mbolivar-nordic
mbolivar-nordic previously requested changes Feb 3, 2021
View reviewed changes
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

--no-xman should not be added to the common arguments, because it doesn't apply to imgtool. Please use tool_args.

@KangJianX
Copy link
Collaborator Author

KangJianX commented Feb 4, 2021 •
edited
Loading

@mbolivar-nordic

--no-xman should not be added to the common arguments, because it doesn't apply to imgtool. Please use tool_args.

source code:

 sign_base = ([tool_path] + args.tool_args +
                     ['-o', out_bin] +  conf_path_cmd + ['-i', '3', '-e'] +
                     [bootloader, kernel])

From source code, I saw the args.tool_args are used by imgtool already. My purpose is I do not want to add extended manifest, so added a new option --no-xman . Should I need use tool_args too? Or you has a better solution?

andyross
andyross approved these changes Feb 4, 2021
View reviewed changes
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.

This seems to be at best an optimization of just a few bytes in image transferred over DMA. The manifest doesn't go into SRAM and shouldn't change runtime behavior at all. Not that it's a bad idea, but why do we care? Is something breaking? +1 regardless.

That said: Marti's criticism is correct: this is a tool-specific thing with rimage and doesn't belong in the global signing arguments.

@KangJianX
Copy link
Collaborator Author

@mbolivar-nordic address your comment, could you help to approve or add any comments?

@LeiW000
Copy link
Collaborator

LeiW000 commented Feb 7, 2021

I would like to see it fxied in 2.5 release. @mbolivar-nordic , please take look at the code change again.

@KangJianX KangJianX mentioned this pull request Feb 9, 2021
Closed
@KangJianX
Copy link
Collaborator Author

@mbolivar-nordic Hi, could you help to review again?

@KangJianX @nashif
board: cavs15: fixed size error that load firmware by script
8f5b445 
When load firmware by script, the buffer size and data size are not
same, so specify size when copy data to buffer.

Signed-off-by: Jian Kang <[email protected]>
@nashif nashif requested a review from lyakh February 11, 2021 14:16
@KangJianX @nashif
board: cavs15: Add a option to control signing ways
6bef024 
Zephyr testcases(not SOF case) not use kernel DSP driver to load image
on ADSP board, thus do not need signing with xman. So add a input
'--no-manifest' to specify signing without xman in image. If use DSP
driver load image, we should not specify this.

Signed-off-by: Jian Kang <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
@nashif nashif merged commit 8c9b06a into zephyrproject-rtos:master Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lyakh lyakh lyakh approved these changes

@andyross andyross andyross approved these changes

@nashif nashif nashif approved these changes

@tedd-an tedd-an Awaiting requested review from tedd-an

@dcpleung dcpleung Awaiting requested review from dcpleung

@LeiW000 LeiW000 Awaiting requested review from LeiW000

@carlescufi carlescufi Awaiting requested review from carlescufi

@mbolivar-nordic mbolivar-nordic Awaiting requested review from mbolivar-nordic

Assignees

@nashif nashif

Labels
area: Boards area: West West utility platform: Intel ADSP Intel Audio platforms
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

intel_adsp_cavs15: signing not correct thus download firmware failed
7 participants
@KangJianX @LeiW000 @nashif @andyross @lyakh @mbolivar-nordic @zephyrbot