scripts: create symlink for community base FW binary #10352
Conversation
lgirdwood
requested review from
dbaluta,
kv2019i,
lbetlej,
mmaka1 and
plbossart
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes the creation of symlinks for community base firmware binaries in the IPC4 deployment structure. The change ensures that deployable builds can be copied directly to targets from the staging directory by creating the missing sof-ipc4/platform/sof-basefw.ri symlink.
- Adds symlink creation for the "plain" firmware file in the
sof/<vendor>/sof-ipc4/<platform>directory structure - Ensures complete firmware directory structure for direct deployment to targets
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/xtensa-build-zephyr.py
Outdated
| # copying the entire sof/<vendor>/sof-ipc4 directory to | ||
| # the target, all platforms are there. | ||
| alias_vendor_dir = pathlib.Path(sof_output_dir, p_alias).parent | ||
| alias_ipc4_dir = pathlib.Path(alias_vendor_dir, p_alias) |
There was a problem hiding this comment.
The path construction appears incorrect. alias_ipc4_dir is being set to Path(alias_vendor_dir, p_alias), but alias_vendor_dir is already Path(sof_output_dir, p_alias).parent. This would result in a path like sof/<vendor>/sof-ipc4/<platform> instead of sof/<vendor>/sof-ipc4. The second argument should likely be 'sof-ipc4' rather than p_alias.
| alias_ipc4_dir = pathlib.Path(alias_vendor_dir, p_alias) | |
| alias_ipc4_dir = pathlib.Path(alias_vendor_dir, "sof-ipc4") |
scripts/xtensa-build-zephyr.py
Outdated
| alias_ipc4_dir = pathlib.Path(alias_vendor_dir, p_alias) | ||
| alias_install_key_dir = alias_ipc4_dir / "community" | ||
| os.makedirs(alias_ipc4_dir, exist_ok=True) | ||
| symlink_or_copy(alias_install_key_dir, alias_fwname, alias_ipc4_dir, alias_fwname) |
There was a problem hiding this comment.
The source path alias_install_key_dir ('community' subdirectory) is being used as the target directory for the symlink, but this directory is the same as install_key_dir from line 1158. The source should be install_key_dir to correctly reference the actual firmware file location created earlier in the function.
| symlink_or_copy(alias_install_key_dir, alias_fwname, alias_ipc4_dir, alias_fwname) | |
| symlink_or_copy(install_key_dir, alias_fwname, alias_ipc4_dir, alias_fwname) |
lgirdwood
force-pushed
the
topic/scripts-deploy
branch
from
c41c4a1 to
93eb9bb
Compare
kv2019i
left a comment
kv2019i
left a comment
There was a problem hiding this comment.
Sorry for late reply. I've started to take a look at this a few time, but failed to understand the problem. Let me just ask directly :)
scripts/xtensa-build-zephyr.py
Outdated
| # Also create the "plain" sof-<platform>.ri symlink in the | ||
| # sof/<vendor>/sof-ipc4/<platform> directory, so that when | ||
| # copying the entire sof/<vendor>/sof-ipc4 directory to | ||
| # the target, all platforms are there. |
There was a problem hiding this comment.
Sorry, can you explain the problem this is fixing? Our release scripts expect the community binary to be in "community" subfolder like done in current builds:
| | |-- sof-ipc4
| | | |-- adl
| | | | |-- community
| | | | | +-- sof-adl.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-adl.ri -> community/sof-adl.ri
| | | |-- adl-n
| | | | |-- community
| | | | | +-- sof-adl-n.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-adl-n.ri -> community/sof-adl-n.ri
| | | |-- rpl
| | | | |-- community
| | | | | +-- sof-rpl.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-rpl.ri -> community/sof-rpl.ri
| | | +-- tgl
| | | +-- community
| | | +-- sof-tgl.ri
Kernel looks for this location on devices that use community binary. So where's the problem?
There was a problem hiding this comment.
The current build script does not create the link today i.e. run ./scripts/xtensa-build-zephyr.py -p --deployable-build tgl
|-- sof
| |-- intel
| | |-- sof-ipc4
| | | |-- adl
| | | | +-- community
| | | | +-- sof-adl.ri -> ../../tgl/community/sof-tgl.ri
| | | |-- adl-n
| | | | +-- community
| | | | +-- sof-adl-n.ri -> ../../tgl/community/sof-tgl.ri
| | | |-- rpl
| | | | +-- community
| | | | +-- sof-rpl.ri -> ../../tgl/community/sof-tgl.ri
| | | +-- tgl
| | | +-- community
| | | +-- sof-tgl.ri sha256=226620d4824ae77532d6b5ede2ec8628bdbae14393809a9df017070122bce8db
| | +-- sof-ipc4-lib
| | +-- tgl
| | +-- community
So this cant be deployed via scp or rsync to target without some manual intervention.
With this patch we get using same build command producing output that can be copied as-is to target. i.e.
build-sof-staging
|-- sof
| |-- intel
| | |-- sof-ipc4
| | | |-- adl
| | | | |-- community
| | | | | +-- sof-adl.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-adl.ri -> community/sof-adl.ri
| | | |-- adl-n
| | | | |-- community
| | | | | +-- sof-adl-n.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-adl-n.ri -> community/sof-adl-n.ri
| | | |-- rpl
| | | | |-- community
| | | | | +-- sof-rpl.ri -> ../../tgl/community/sof-tgl.ri
| | | | +-- sof-rpl.ri -> community/sof-rpl.ri
| | | +-- tgl
| | | +-- community
| | | +-- sof-tgl.ri sha256=a3788782ec0bcf392f1408a5296cd7d388ecb773d0f65d68c63fe0cfe925ab6d
| | +-- sof-ipc4-lib
| | +-- tgl
| | +-- community
There was a problem hiding this comment.
Thanks. Not sure that is what we want. In kernel, the community key devices are identified and "community/" is added:
sof-pci-dev.c:
int sof_pci_probe(struct pci_dev *pci, const struct pci_device_id *pci_id)
....
» if (dmi_check_system(community_key_platforms) &&
» sof_dmi_use_community_key) {
» » path_override->fw_path_postfix = "community";
» » path_override->fw_lib_path_postfix = "community";
» }
One alternative for your own use is to set the 'fw_path' kernel parameter if your devel system is not listed (by its DMI id) as a community-key platform.
I worry a bit if this breaks the release process if we have multiple sof-ipc4/PLAT/sof-PLAT.ri builds created by xtense-build-zephyr.py. Probably not a big problem, the release packager needs to decide which signed key gets the preference.
There was a problem hiding this comment.
If the device needs community key then we should add it to the dmi check, otherwise the developer should use path override for the device, fwiw, I have this on selected machines:
options snd_sof fw_path=sof-peter/ipc4-firmware lib_path=sof-peter/ipc4-lib tplg_path=sof-peter/ipc4-tplg
and copy the firmware/lib and topologies to the given directory to not interfere with the distro packages.
Also: intel/sof-ipc4/tgl/sof-tgl.ri is production keyed firmware and not community key, it is wrong to 'trick' the system and use 'random' key signed firmware.
|
I'm confused, we don't use |
ujfalusi
left a comment
ujfalusi
left a comment
There was a problem hiding this comment.
I don't think it is correct to symlink the production key firmware to the community key one.
These must be different, their md5sum must be different, the key used to sign them must be different.
This is a developer script. When we want to build -> deploy -> test this is what we use. We dont want to use custom install steps or add kernel command line options. Production signing uses a different process so not impacted. |
lgirdwood
force-pushed
the
topic/scripts-deploy
branch
from
93eb9bb to
9c9df0f
Compare
|
v2: opt-in only with new command line switch to create the developer deployable build. |
|
@lrudyX I dont think Internal CI tests these developer scripts ? |
scripts/xtensa-build-zephyr.py
Outdated
| parser.add_argument("-z", "--zephyrsdk", required=False, action="store_true", | ||
| help="Force Build using Zephyr SDK for target") | ||
| parser.add_argument("--deployable-dev-build", required=False, action="store_true", | ||
| help="""Create a deployable development build linking binaries to default""") |
There was a problem hiding this comment.
Have you tried
./scripts/xtensa-build-zephyr.py --key-type-subdir=none mtl
which results:
build-sof-staging
|-- sof
| |-- intel
| | |-- sof-ipc4
| | | |-- arl
| | | | +-- sof-arl.ri -> ../mtl/sof-mtl.ri
| | | |-- arl-s
| | | | +-- sof-arl-s.ri -> ../mtl/sof-mtl.ri
| | | +-- mtl
| | | +-- sof-mtl.ri sha256=b3faca3086b1b8c42ebfa7751bb5bfc48fcb90197bcbe88f1983aa1513c09859
| | +-- sof-ipc4-lib
| | +-- mtl
| | |-- 08AEB4FF-7F68-4C71-86C3-C842B4262898.bin -> tester.llext
| | |-- B780A0A6-269F-466F-B477-23DFA05AF758.bin -> google_rtc_audio_processing.llext
| | |-- google_rtc_audio_processing.llext sha256=6ce64c11c877ce7dd683e0fad8c2f6297606301977c6bddfaedd565ac27e0d83
| | +-- tester.llext sha256=fa3ea870157c38f188f021176f5730d2c9a13b5bb4dab39b4f984b671bc62e2b
| +-- sof.tgz
I'm guessing that this PR is to address development machines which are using debug key (so the fw is built with -k <path to dbgkey>), it is not community key as those devices will load the fw from the community subdir.
The kernel on dev machines does not know that they are dev machines, so we use the production signed binary path.
I still think that linking the production file to community is a wrong practice, even for developers.
There was a problem hiding this comment.
Have you tried
./scripts/xtensa-build-zephyr.py --key-type-subdir=none mtl
gah - this is badly named, it has nothing to do with key types since this is abstracted by rimage/Cmake. I'm going to fix this as --deployable-build-link-to
There was a problem hiding this comment.
well, it has to do with the key type...
intel/sof-ipc4/<plat>/sof-<plat>.ri - production signed
intel/sof-ipc4/<plat>/community/sof-<plat>.ri - community signed
intel/sof-ipc4/<plat>/dbgkey/sof-<plat>.ri - debug key signed signed
From SOF one can build the community key signed version only, so the default is community.
Probably it would be better to have -key-type-subdir=production to act as the current none, but it is to specify the subdir, so none is actually does what it say, it places the firmware in place where the production signed should be.
Yes, awkward, but makes sense. Developer working on device with debug key should override the fw-path to the dbgkey subdir.
It might be better to have another override in kernel to override/specify a key-type, so one can set dbgkey and that would be ultimately taken into account on all platforms to look at the ../dbgkey subdir instead of hacking around in firmware deploy?
There was a problem hiding this comment.
@lgirdwood, something like this:
diff --git a/sound/soc/sof/core.c b/sound/soc/sof/core.c
index 5fa8c40de5d9..448c7f09a302 100644
--- a/sound/soc/sof/core.c
+++ b/sound/soc/sof/core.c
@@ -24,6 +24,10 @@ static char *override_fw_path;
module_param_named(fw_path, override_fw_path, charp, 0444);
MODULE_PARM_DESC(fw_path, "alternate path for SOF firmware.");
+static char *override_fw_postfix;
+module_param_named(fw_key_type, override_fw_postfix, charp, 0444);
+MODULE_PARM_DESC(fw_key_type, "alternate subdir for SOF firmware based on signing key (community, dbgkey, etc).");
+
static char *override_fw_filename;
module_param_named(fw_filename, override_fw_filename, charp, 0444);
MODULE_PARM_DESC(fw_filename, "alternate filename for SOF firmware.");
@@ -278,6 +282,10 @@ static int sof_select_ipc_and_paths(struct snd_sof_dev *sdev)
if (base_profile->fw_path)
dev_dbg(dev, "Module parameter used, changed fw path to %s\n",
base_profile->fw_path);
+ else if (override_fw_postfix)
+ dev_dbg(dev,
+ "Module parameter used, default fw path extended with: %s\n",
+ override_fw_postfix);
else if (base_profile->fw_path_postfix)
dev_dbg(dev, "Path postfix appended to default fw path: %s\n",
base_profile->fw_path_postfix);
@@ -285,6 +293,10 @@ static int sof_select_ipc_and_paths(struct snd_sof_dev *sdev)
if (base_profile->fw_lib_path)
dev_dbg(dev, "Module parameter used, changed fw_lib path to %s\n",
base_profile->fw_lib_path);
+ else if (override_fw_postfix)
+ dev_dbg(dev,
+ "Module parameter used, default fw_lib path extended with: %s\n",
+ override_fw_postfix);
else if (base_profile->fw_lib_path_postfix)
dev_dbg(dev, "Path postfix appended to default fw_lib path: %s\n",
base_profile->fw_lib_path_postfix);
@@ -631,10 +643,14 @@ sof_apply_profile_override(struct sof_loadable_file_profile *path_override,
path_override->ipc_type = override_ipc_type;
if (override_fw_path)
path_override->fw_path = override_fw_path;
+ else if (override_fw_postfix)
+ path_override->fw_path_postfix = override_fw_postfix;
if (override_fw_filename)
path_override->fw_name = override_fw_filename;
if (override_lib_path)
path_override->fw_lib_path = override_lib_path;
+ else if (override_fw_postfix)
+ path_override->fw_lib_path_postfix = override_fw_postfix;
if (override_tplg_path)
path_override->tplg_path = override_tplg_path;
if (override_tplg_filename) {There was a problem hiding this comment.
@ujfalusi - lets keep it simple. I'm going to fix the cmd line arg.
There was a problem hiding this comment.
but the point is that intel/sof-ipc4/<plat>/sof-<plat>.ri should not be a binary signed with "random" key, it should be always a binary, which is signed with production key.
To avoid really confusing setups and logs. One will never know the source of the binary from logs and cannot even guess what key needs to be used to deploy own firmware...
symlinking it to community hints that it is community key, but it might be a debug key, or it might be a real product key.
There was a problem hiding this comment.
ack on that, it has been very beneficial we see from kernel logs the full path to the fw file and if any kernel param is used (to override e.g. the key type with fw_path), this is visible in the logs.
Use a relative directory to script dir for default workspace if workspace is not set. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Determine workspace if environment variable not defined. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
…-subdir Add more context to this command line option around deployment usage. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Use a fallback SOF_WORKSPACE relative to script dir if environment not set. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
Use a fallback SOF_WORKSPACE relative to script dir if environment not set. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
…ot set. Use a fallback SOF_WORKSPACE relative to script dir if environment not set. Signed-off-by: Liam Girdwood <liam.r.girdwood@linux.intel.com>
lgirdwood
force-pushed
the
topic/scripts-deploy
branch
from
9c9df0f to
e1a5269
Compare
|
@lgirdwood, am I looking at this new version right and you are no longer creating the link: |
correct - I've just updated the help to point out that existing option can be used for developer deploy. |
4 participants
Currently the symlink for sof-ipc4/platform/sof-basefw.ri is not created. Fix this so that deployable builds can be copied directly to target from staging directory.