systemd: skip blkid probing for Qualcomm raw partitions - #2865
Wenwen Fu (wenwfu) wants to merge 1 commit into
Conversation
Dmitry Baryshkov (lumag)
left a comment
There was a problem hiding this comment.
You list too many partitions which never existed. Drop them.
| ENV{PARTNAME}=="catecontentfv|quantumfv|questdatafv|toolsfv", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="qupfw|qupfw_[ab]|storsec|storsec_[ab]|shrm|shrm_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="cpucp|cpucp_[ab]|featenabler|featenabler_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="modem|modem_[ab]|bluetooth|bluetooth_[ab]|dsp|dsp_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
Drop, these should not be used.
There was a problem hiding this comment.
Thanks. I will drop the modem/bluetooth/dsp entries and remove the superfluous cdt_[ab] match.
| ENV{PARTNAME}=="modem|modem_[ab]|bluetooth|bluetooth_[ab]|dsp|dsp_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="dtb|dtb_[ab]|dtbo|dtbo_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="vbmeta|vbmeta_[ab]|vbmeta_system|vbmeta_system_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="cdt|cdt_[ab]|ddr|ddr_[ab]|multiimgqti|multiimgqti_[ab]|multiimgoem|multiimgoem_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
There definitely are no cdt_a or ddr_b partitions. Drop the superfluous partitions from the list.
There was a problem hiding this comment.
For ddr_b, I observed a ddr_b partition on rb3gen2-core-kit and iq-9075-evk. Do you still prefer dropping ddr_b from this whitelist?
root@iq-9075-evk:/# ls /dev/disk/by-partlabel/
ALIGN_TO_128K_1 TZAPPS cdt ddr_a ddr_b diag_log dtb_a dtb_b efi emac gearvm_a gearvm_b gvm_log keymaster_a keymaster_b multiimgqti_a multiimgqti_b persist pvm_log rootfs softsku splash spunvm xbl_a xbl_b xbl_config_a xbl_config_b xbl_logs
root@rb3gen2-core-kit:/# ls /dev/disk/by-partlabel/
ALIGN_TO_128K_1 aop_b catecontentfv cpucp_b devcfg_a dip efi hyp_a imagefv_b logdump modemst2 qmcs questdatafv qweslicstore_a rootfs shrm_b tz_a uefi_b vm-data xbl_config_a
SYSFW_VERSION apdp_a cdt ddr_a devcfg_b dtb_a fsc hyp_b limits logfs multiimgoem_a quantumfv qupfw_a qweslicstore_b secdata splash tz_b uefisecapp_a xbl_a xbl_ramdump_a
aop_a apdp_b cpucp_a ddr_b devinfo dtb_b fsg imagefv_a limits-cdsp modemst1 multiimgoem_b quantumsdk qupfw_b recoveryinfo shrm_a toolsfv uefi_a uefisecapp_b xbl_b xbl_ramdump_b
There was a problem hiding this comment.
Hmm. okay.
476ad31 to
73cfa60
Compare
|
Updated per review comments:
Please review again when you get a chance. |
| ACTION=="remove", GOTO="qcom_raw_noblkid_end" | ||
| SUBSYSTEM!="block", GOTO="qcom_raw_noblkid_end" | ||
| ENV{DEVTYPE}!="partition", GOTO="qcom_raw_noblkid_end" | ||
| ENV{PARTNAME}=="", GOTO="qcom_raw_noblkid_end" |
There was a problem hiding this comment.
I'd say, that for paritions with an empty name we should create all symlinks.
There was a problem hiding this comment.
Fixed.
Empty PARTNAME now falls through to a shared qcom_raw_links label instead of exiting early.、
For partitions without a name, this still creates the non-name-dependent partition metadata links:
disk/by-partuuid/...、disk/by-path/.../by-partnum/...、disk/by-path/.../by-partuuid/...、
The by-partlabel links remain guarded by PARTNAME=="?*", so we do not create invalid empty-label symlinks.
| ENV{DEVTYPE}!="partition", GOTO="qcom_raw_noblkid_end" | ||
| ENV{PARTNAME}=="", GOTO="qcom_raw_noblkid_end" | ||
|
|
||
| ENV{PARTNAME}=="xbl|xbl_[ab]|xbl_config|xbl_config_[ab]|xbl_ramdump|xbl_ramdump_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
I really asked to review the list. Which platform has non-slot xbl_config? Likewise I know only one platform with non-slotted xbl, but it's easier to ignore it.
Please go through your list and then apply common sense
There was a problem hiding this comment.
Fixed. I went through the list again and trimmed it against the generated partition XML.、
I checked the generated qcom-partition-conf rawprogram*.xml from the deployed partition artifacts and removed entries that were not backed by those partition labels.
Notably, I dropped the non-slotted xbl, xbl_config, and xbl_ramdump entries, and also removed other unsupported bare names such as abl, aop, aop_config, uefi, imagefv, qupfw, shrm, cpucp, featenabler,
dtbo, vbmeta, and gearvm.
I kept bare names that do exist in the generated partition tables, such as apdp, tz, hyp, devcfg, keymaster, cmnlib, cmnlib64, ddr, multiimgqti, multiimgoem, and qweslicstore.
Test Results 79 files 409 suites 9h 22m 11s ⏱️ For more details on these failures, see this check. Results for commit 3f5434b. ♻️ This comment has been updated with latest results. |
73cfa60 to
64dc0a3
Compare
| ENV{PARTNAME}=="fsc|fsg|modemst[12]|qweslicstore|qweslicstore_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="devinfo|dip|limits|limits-cdsp|qmcs|quantumsdk|recoveryinfo|secdata|SYSFW_VERSION", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="diag_log|gvm_log|pvm_log|logdump|xbl_logs|ALIGN_TO_128K_*", GOTO="qcom_raw_noblkid" | ||
| GOTO="qcom_raw_noblkid_end" |
There was a problem hiding this comment.
This is a long list, we will have to keep track of the updates based on the changes available via https://github.com/qualcomm-linux/qcom-ptool.
There was a problem hiding this comment.
Can you add a comment saying how you got to these partitions here? Just so we know how to keep this file in sync with all latest targets.
There was a problem hiding this comment.
Currently the list was derived from labels observed on the tested devices and from generated rawprogram*.xml artifacts. I agree that is not enough as a maintenance rule.
Updated. I added a maintenance comment above the whitelist pointing to qcom-ptool platforms and explaining how to refresh the list for new targets or layout changes.
64dc0a3 to
a4903bd
Compare
| ENV{PARTNAME}=="abl_[ab]|aop_[ab]|aop_config_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="apdp|apdp_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="tz|tz_[ab]|TZAPPS", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="hyp|hyp_[ab]|devcfg|devcfg_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
Ignore non-slotted tz/hyp/devcfg/keymaster/cmnlib/cmnlib64, they are used only for the very old platforms (which we probably should fix).
There was a problem hiding this comment.
Agreed. In qcom-ptool these bare (non-slotted) names appear only on apq8016-sbc and apq8096-db820c — the legacy platforms. I'll change these to slot-only:
| ENV{PARTNAME}=="tz|tz_[ab]|TZAPPS", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="hyp|hyp_[ab]|devcfg|devcfg_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="keymaster|keymaster_[ab]|cmnlib|cmnlib_[ab]|cmnlib64|cmnlib64_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="uefi_[ab]|uefisecapp|uefisecapp_[ab]|imagefv_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
I don't see bare uefisecapp
There was a problem hiding this comment.
Correct. Bare uefisecapp exists only on iq-x7181-evk; every other target uses uefisecapp_[ab]. I'll drop the bare form and keep uefi_[ab]|uefisecapp_[ab]|imagefv_[ab].
| ENV{PARTNAME}=="catecontentfv|quantumfv|questdatafv|toolsfv", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="qupfw_[ab]|storsec|storsec_[ab]|shrm_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="cpucp_[ab]|featenabler_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="dtb|dtb_[ab]|dtbo_[ab]", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
I don't see dtb / dtb_a / dtb_b partitions at all
There was a problem hiding this comment.
dtb_[ab] is actually defined on 16 platforms in qcom-ptool, including the ones I tested (rb3gen2, iq-9075-evk), I also confirmed them live in /dev/disk/by-partlabel/ on both boards. Bare dtb only exists on iq-x7181-evk, and dtbo only on qrb5165-rb5.
There was a problem hiding this comment.
Ack. Let's kill dtbo_[ab] here and a raw dtb.
| ENV{PARTNAME}=="cdt|ddr|ddr_a|ddr_b|multiimgqti|multiimgqti_[ab]|multiimgoem|multiimgoem_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="gearvm_[ab]|spunvm|softsku|splash|emac", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="fsc|fsg|modemst[12]|qweslicstore|qweslicstore_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="devinfo|dip|limits|limits-cdsp|qmcs|quantumsdk|recoveryinfo|secdata|SYSFW_VERSION", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
limits-cdsp doesn't exist. SYSFW_VERSION doesn't exist
There was a problem hiding this comment.
Both exist in qcom-ptool: limits-cdsp on qcm6490-idp, qcs6490-rb3gen2, qcs6490-thundercomm-rubikpi3, qrb5165-rb5;
| ENV{PARTNAME}=="gearvm_[ab]|spunvm|softsku|splash|emac", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="fsc|fsg|modemst[12]|qweslicstore|qweslicstore_[ab]", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="devinfo|dip|limits|limits-cdsp|qmcs|quantumsdk|recoveryinfo|secdata|SYSFW_VERSION", GOTO="qcom_raw_noblkid" | ||
| ENV{PARTNAME}=="diag_log|gvm_log|pvm_log|logdump|xbl_logs|ALIGN_TO_128K_*", GOTO="qcom_raw_noblkid" |
There was a problem hiding this comment.
I don't see diag_log, gvm_log, pvm_log.
There was a problem hiding this comment.
All three are defined on iq-8275-evk, iq-9075-evk, qcs8275-monza, qcs8300-ride-sx, qcs9100-ride-sx
|
Ricardo Salveti (@ricardosalveti) Dmitry Baryshkov (@lumag) — on long-term maintenance (keeping this in sync with qcom-ptool): Two points I'd want your call on:
|
|
Sure, let's have a script. How do sort out the one-offs (like db410c / db820c / iq-x7181)? |
a4903bd to
3f5434b
Compare
Added a generator. It excludes the legacy db410c/db820c layouts, while iq-x7181 remains scanned. Labels found on fewer than two platforms are treated as one-offs and require explicit per-pattern approval. |
|
If you have written a generator, what about using it to generate the rules? Extend the qcom-ptool recipe, add separate udev-rules package and generate the rules while building. |
Yes, that makes sense. I’ll move the generator into the qcom-partition-conf recipe, generate the rules from the recipe’s qcom-ptool source during the build, and install them through a separate udev-rules package. |
|
Could this generator be part of qcom-ptool upstream instead? The same benefit we get here could also later be applied for other distros, since this is just a set of generic qcom udev rules. |
Thanks for the suggestion. I rechecked both qcom-ptool and how it is consumed by meta-qcom. qcom-ptool currently provides build-time partitioning utilities and platform descriptions. Its outputs are GPT binaries, partition XML, contents XML, and QDL flashing files; it does not currently provide runtime udev or systemd policy. This rule serves a different purpose. It is a systemd-udev runtime optimization that sets UDEV_DISABLE_PERSISTENT_STORAGE_BLKID_FLAG for a reviewed subset of raw partitions and preserves the partition symlinks that would otherwise depend on the blkid import. Whether a partition is safe to exclude from probing cannot be inferred solely from its presence in partitions.conf, so the generator also contains a deliberately reviewed policy. meta-qcom owns the mapping from an OE MACHINE to the relevant qcom-ptool layouts through QCOM_PARTITION_FILES_SUBDIR and QCOM_PARTITION_FILES_SUBDIR_SPINOR. The new recipe uses that mapping and the same pinned qcom-ptool revision to generate a machine-specific runtime package. Other distributions would still need their own layout selection, policy review, packaging, and rule installation even if the generator were moved to qcom-ptool. Therefore, I would prefer to keep the generator and systemd-specific template in meta-qcom for this implementation. A generalized upstream interface can be reconsidered later if another distribution wants to consume the same runtime policy. |
3f5434b to
5241fc5
Compare
|
Dmitry Baryshkov (@lumag) Ricardo Salveti (@ricardosalveti) -- This PR has been reworked from a hand-maintained partition allowlist to a generated, per-machine approach, addressing the maintainability concern Ricardo Salveti (@ricardosalveti) raised earlier and the accuracy issues Dmitry Baryshkov (@lumag) found in the previous partition-name list (several entries didn't exist on the platforms I'd tested, others existed but weren't in the reviewed list). Could you take another look when you have a chance? What changed:
This means adding a new machine or a new qcom-ptool partition just requires updating the approved-patterns policy (if needed) and regenerating — no more per-machine list edits or arguing over whether partition X exists on platform Y. |
Dmitry Baryshkov (lumag)
left a comment
There was a problem hiding this comment.
I think I'd second the question of moving it to qcom-ptool. Loïc Minier (@lool), would you want something similar in Debian images?
| # SPDX-License-Identifier: MIT | ||
|
|
||
| # Partition-name patterns reviewed as safe to exclude from blkid probing. | ||
| # Keep one udev glob per line. A new qcom-ptool label is not implicitly safe. |
There was a problem hiding this comment.
This is not better than the previous iteration. You still hardcode the full list.
There was a problem hiding this comment.
You're right. The per-machine filtering avoids generating rules for nonexistent partitions, but it does not eliminate the hardcoded policy list.
Would moving the policy, generator, rule template, and tests to qcom-ptool address your concern, or should the raw/no-blkid classification be explicit metadata in each partition definition? I want to avoid simply relocating the same hardcoded list to another repository.
|
I would have preferred to use the data directly rather than resorting to heuristics and generators. As you note, it's hard to exclude entire block devices because LUNs mix HLOS and NHLOS partitions. The ideal next level would have been partition GUIDs, but currently these are not reported by the kernel. Could we send a patch to allow this? That sounds like a more useful general mechanism. Lastly, I hate the very long-list of weird partition names. We know it will keep growing. Should simply flip this around? That is, just run blkid on a few partitions that we expect, and nothing else. It would mean:
The list of partitions we want to mount from HLOS is quite small: efi, rootfs, persist, userdata (UNOQ). |
This could cause bad side effect on users customizing the partition table as well, which is a use case we also want to support, that is why an exclude list seems better in the end. |
5241fc5 to
bffc4ce
Compare
|
The implementation has been reworked based on the previous review. The generic generator, reviewed policy, and udev template have been moved to qcom-ptool: This PR now only contains the Yocto machine integration and packaging. It capability-checks gen_udev_rules so existing builds remain functional until the qcom-ptool dependency is available. Please take another look. This PR must not be merged until qcom-ptool #156 is merged and SRCREV is updated to its final upstream commit. |
| do_install() { | ||
| if [ ! -f ${QCOM_RAW_PARTITIONS_RULES} ]; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
please drop this check, bitbake internal takes care of this
There was a problem hiding this comment.
I kept the file check intentionally, but removed the early exit.
gen_udev_rules may legitimately produce no output when:
- the machine has no selected qcom-ptool layout;
- no approved raw partitions exist in the selected layout;
- the transitional qcom-ptool revision lacks
gen_udev_rules.
ALLOW_EMPTY allows BitBake to create an empty package, but it does not prevent
install from failing when its source file does not exist.
With the conditional install, do_install:append still runs normally. Dropping
the check would require qcom-ptool to always emit an empty rules file and would
remove the current optional/compatibility behaviour. Please let me know if that
is the preferred direction.
| set -- "$@" --input "${S}/platforms/$layout/partitions.conf" | ||
| done | ||
| if [ "$#" -eq 0 ]; then | ||
| exit 0 |
There was a problem hiding this comment.
please don't use exit in bitbake task, this brakes any do_compile:append that may exist
| if ! ${STAGING_BINDIR_NATIVE}/qcom-ptool \ | ||
| gen_udev_rules --help >/dev/null 2>&1; then | ||
| bbwarn "qcom-ptool ${SRCREV} has no gen_udev_rules support; skipping raw partition rules" | ||
| exit 0 |
|
Generator feels overkill to me; why not a single list with all names? We have to support that list for generic images that work on all targets anyway Anyone on the kernel changes to send part GUID as part of uevent? |
Earlier revisions had a list of all partitions. It was a nightmare to review and/or change later. |
bffc4ce to
4b9afcf
Compare
|
This pull request has been marked as stale due to 30 days of inactivity. To prevent automatic closure in 5 days, remove the stale label or add a comment. You can reopen a closed pull request at any time. |
|
The related qcom-ptool PR has been updated based on the review feedback: The meta-qcom change is currently waiting for the qcom-ptool code-owner review. Please take another look when you have time, as this PR has been marked stale and is scheduled for automatic closure. |
Qualcomm platforms can expose many bootloader, firmware, metadata, modem NV, and dump partitions during coldplug. These partitions are accessed as raw GPT partitions rather than mounted filesystems, but systemd's persistent-storage rules still run the blkid builtin for each of them. Add a machine-specific udev rules recipe that reads the qcom-ptool layouts selected by each machine. Emit only explicitly approved raw partition patterns present in those layouts, leaving other partitions under normal filesystem probing. Set UDEV_DISABLE_PERSISTENT_STORAGE_BLKID_FLAG before the stock rules run and preserve partition metadata links using kernel-provided properties. Recommend the generated rules package whenever udev is installed on a Qualcomm machine. Sysinit milestone improved by about 0.38s on rb3gen2-core-kit (QLI-2.0 tag wrynose-260627.1): Default: sysinit.target reached at 5.74s With optimization: sysinit.target reached at 5.37s Assisted-by: Codex:GPT-5 Signed-off-by: Wenwen Fu <wenwfu@qti.qualcomm.com>
b30f473 to
31db906
Compare
Add a Qualcomm-specific udev rule to skip
blkidprobing for known raw GPT partitions.Qualcomm platforms can expose many bootloader, firmware, metadata, modem NV, and dump partitions during coldplug. These partitions are accessed as raw block partitions rather than mounted filesystems, but systemd's default
60-persistent-storage.rulesstill runs theblkidbuiltin for each one.This rule matches known raw partition names by GPT
PARTNAME, preserves/dev/disk/by-partlabeland/dev/disk/by-partuuidlinks from kernel-provided partition metadata, and setsUDEV_DISABLE_PERSISTENT_STORAGE_BLKID_FLAG=1before60-persistent-storage.rulesruns.The rule intentionally avoids matching by kernel block device names such as
sda,sdb, etc. Disk enumeration is board- and boot-order-dependent, and some platforms may place mountable partitions on the same storage device as raw boot or firmware partitions.The rule also does not set
SYSTEMD_READY=0, so block devices remain visible to systemd. Normal filesystem partitions such asrootfs,efi,persist,userdata,logfs, andvm-datacontinue to use the stock persistent-storage handling.Sysinit milestone improved by about 0.38s on
rb3gen2-core-kit(
QLI-2.0tagwrynose-260627.1):