Add configurable EFI partition alignment to support UFS devices

[FantasyGmm]

Description

Default EFI partition uses 512B alignment, which fails on Qualcomm UFS (4K-aligned) devices. This change introduces a variable to optionally enforce 4K alignment while maintaining backward compatibility.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also note any relevant details for your test configuration.

• No variables set, correctly generated 512 alignment
• Set variables to correctly generate 4096 alignment

Checklist:

Please delete options that are not relevant.

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• Any dependent changes have been merged and published in downstream modules

[coderabbitai]

Walkthrough

The changes modify the prepare_partitions function by introducing a new variable, SECTOR_SIZE, which defaults to 512 if not specified. This variable is utilized in two key areas: first, when invoking the sfdisk command, the script checks the version of sfdisk to determine if the --sector-size option is supported. If the version is 24100 or higher, the command includes the --sector-size option with the value of SECTOR_SIZE; otherwise, it executes without this option. Additionally, the command that sets up the loop device checks the sfdisk version to decide on including the -b option with SECTOR_SIZE. Minor formatting changes were also made, including the addition of blank lines for improved readability.

Tip

⚡💬 Agentic Chat (Pro Plan, General Availability)

• We're introducing multi-step agentic chat in review comments and issue comments, within and outside of PR's. This feature enhances review and issue discussions with the CodeRabbit agentic chat by enabling advanced interactions, including the ability to create pull requests directly from comments and add commits to existing pull requests.

---

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bc4f5e4 and 7226b07.

📒 Files selected for processing (1)

• lib/functions/image/partitioning.sh (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/functions/image/partitioning.sh

---

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai plan to trigger planning for file edits and PR creation.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

lib/functions/image/partitioning.sh (2)

95-95: Configurable EFI Partition Alignment Variable

The addition of the line:

UEFI_PART_ALIGN=${UEFI_PART_ALIGN:-512}

ensures that the EFI partition alignment is configurable while retaining backward compatibility. This default allows the system to use the standard 512-byte alignment unless overridden (for example, to 4096 bytes for Qualcomm UFS devices). Please ensure that this new configuration option is clearly documented so that users know when and how to adjust it for UFS devices.

---

347-347: Integration of Custom Alignment in Filesystem Creation

The updated command:

run_host_command_logged mkfs.fat -F32 -S "${UEFI_PART_ALIGN}" -n "${UEFI_FS_LABEL^^}" ${LOOP}p${uefipart} 2>&1

now passes the UEFI_PART_ALIGN variable via the -S option to set the FAT sector size during formatting. This change appears to correctly support dynamic alignment configuration. Please verify that the value provided (e.g., 4096 when required) meets the expectations of the mkfs.fat utility and that any potential edge cases (such as non-numeric or out-of-range values) are handled or documented.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8e6fd84 and 3efa18e.

📒 Files selected for processing (1)

• lib/functions/image/partitioning.sh (4 hunks)

[HeyMeco]

Is this confirmed working? This would probably also fix #7942

[FantasyGmm]

Is this confirmed working? This would probably also fix #7942

its works on rb5

[FantasyGmm]

@igorpecovnik Hi, Which document should I add it to?

[EvilOlaf]

Interesting question. This seems a board or family level configuration variable but I don't think a documentation for that exists already?

[HeyMeco]

My proposal for this is to add a simple extension UFS.sh that has declare -g UEFI_PART_ALIGN=4096 so we can integrate UFS and EMMC/SD images side by side as build targets on the Website

[FantasyGmm]

Interesting question. This seems a board or family level configuration variable but I don't think a documentation for that exists already?

This might be a common variable—at least for UFS devices, it's needed for 4K alignment.

[FantasyGmm]

My proposal for this is to add a simple extension UFS.sh that has declare -g UEFI_PART_ALIGN=4096 so we can integrate UFS and EMMC/SD images side by side as build targets on the Website

This only applies to UFS devices booting with EFI. If Qualcomm uses ABL for boot, this is useless. I'm not sure this extension is needed

[HeyMeco]

My proposal for this is to add a simple extension UFS.sh that has declare -g UEFI_PART_ALIGN=4096 so we can integrate UFS and EMMC/SD images side by side as build targets on the Website

This only applies to UFS devices booting with EFI. If Qualcomm uses ABL for boot, this is useless. I'm not sure this extension is needed

Mediatek Genio can boot via EMMC/SD (512 alignment) or integrated UFS, that's why I was saying that.

[HeyMeco]

@FantasyGmm how did you make the rootfs 4k aligned? Just using this PR branch + setting UEFI_PART_ALIGN=4096 still gives me: find_valid_gpt: *** ERROR: Invalid GPT *** in U-Boot

[FantasyGmm]

Rootfs is supposed to be 4K aligned by default, I'm not getting any errors about rootfs

[HeyMeco]

Okay maybe this just means that for Mediatek Genio Family at least alignment isn't enough but the sector size needs to be 4096. (Btw which tool is that in the screenshot?)

[FantasyGmm]

its DiskGenius, How are you flashing the image to lun0, on top of Qualcomm I need to use winhex to separate the efi from the rootfs, then I can flush to the partition under fastboot or edl
It's a bit strange, my detached efi partition became 4096 aligned, but inside the full image it's 512 aligned

[HeyMeco]

How are you flashing the image

On Mediatek Genio we can just flash directly to UFS via fastboot: fastboot flash mmc0 ArmbianAligned.img. U-Boot is a separate partition to call in fastboot that is untouched from mmc0 (UFS)

[HeyMeco]

Probably means we still need the newest sfdisk from util-linux =>2.41 to make actual 4096 sector size images instead of this workaround that is not quite ready without it.

[FantasyGmm]

But if I don't add this, the efi partition I detached using winhex will report an error and uboot won't be able to boot, but after adding this parameter, the detached efi partition, I can boot into kernel

[HeyMeco]

detached using winhex

That is very hacky to be merged imo. We should focus on having fully working image files out of the builder and not do additional modifications

[FantasyGmm]

It looks like you might need to change the GPT partition table, I only changed the parameters for the efi partition, which will be 4096 aligned when making the filesystem, but inside the partition table it's still 512 aligned, which causes you to not be able to boot because you're flashing in a full image, whereas I'm separating and flushing it in.

[FantasyGmm]

detached using winhex

That is very hacky to be merged imo. We should focus on having fully working image files out of the builder and not do additional modifications

I can't write the entire image because abl checks for the rootfs partition name and if it doesn't exist it will refuse to boot to uboot. If it's possible to boot into uboot inside xbl then I can bypass that as well, I'll try to go XBL boot next week and try to flash a whole image into lun0

[FantasyGmm]

It looks like we need to upgrade utils-linux to 24.1

Compiled version 2.41 of util-linux and built it without docker, and it's generating a 4096 gpt partition table

The image can be successfully generated, but I'm not sure if flashing it directly will work

After booting through XBL's Uboot, I can just dd the full image to lun0 (enable ums in uboot) and then I can just boot it

[HeyMeco]

Thanks for testing it out.
@igorpecovnik would we consider merging a PR for adding https://packages.debian.org/source/testing/util-linux out of Debian Testing to the build deps? I'm still thinking an UFS extension would make the most sense but I'm not sure how if at all I could patch the partitioning.sh from an extension. Maybe you can give some insight

[FantasyGmm]

Can the image compiled from the new commit be boot on nio?

[HeyMeco]

Can the image compiled from the new commit be boot on nio?

Yes this works on Nio-12L. While we cannot merge this as is since it would fail on sfdisk versions before 2.41 a simple version. Here is a patch for build/main that integrates a version check.

diff --git a/lib/functions/image/partitioning.sh b/lib/functions/image/partitioning.sh
index 1d9aa34c0..af7cca6c9 100644
--- a/lib/functions/image/partitioning.sh
+++ b/lib/functions/image/partitioning.sh
@@ -89,6 +89,7 @@ function prepare_partitions() {
 
 	# default BOOTSIZE to use if not specified
 	DEFAULT_BOOTSIZE=256 # MiB
+	SECTOR_SIZE=${SECTOR_SIZE:-512}
 	# size of UEFI partition. 0 for no UEFI. Don't mix UEFISIZE>0 and BOOTSIZE>0
 	UEFISIZE=${UEFISIZE:-0}
 	BIOSSIZE=${BIOSSIZE:-0}
@@ -242,7 +243,16 @@ function prepare_partitions() {
 		)
 		# Output the partitioning options from above to the debug log first and then pipe it into the 'sfdisk' command
 		display_alert "Partitioning with the following options" "$partition_script_output" "debug"
-		echo "${partition_script_output}" | run_host_command_logged sfdisk "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+
+		# Check sfdisk version to determine if --sector-size is supported
+		sfdisk_version=$(sfdisk --version | awk '/util-linux/ {print $NF}')
+		sfdisk_version_num=$(echo "$sfdisk_version" | awk -F. '{printf "%d%02d%02d\n", $1, $2, $3}')
+		if [ "$sfdisk_version_num" -ge "24100" ]; then
+			echo "${partition_script_output}" | run_host_command_logged sfdisk --sector-size "$SECTOR_SIZE" "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+		else
+			echo "${partition_script_output}" | run_host_command_logged sfdisk "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+		fi
+
 	fi
 	
 	call_extension_method "post_create_partitions" <<- 'POST_CREATE_PARTITIONS'
@@ -256,7 +266,11 @@ function prepare_partitions() {
 
 	declare -g LOOP
 	#--partscan is using to force the kernel for scaning partition table in preventing of partprobe errors
-	LOOP=$(losetup --show --partscan --find "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	if [ "$sfdisk_version_num" -ge "24100" ]; then
+		LOOP=$(losetup --show --partscan --find -b "$SECTOR_SIZE" "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	else
+		LOOP=$(losetup --show --partscan --find "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	fi
 	display_alert "Allocated loop device" "LOOP=${LOOP}"
 
 	# loop device was grabbed here, unlock

[FantasyGmm]

Can the image compiled from the new commit be boot on nio?

Yes this works on Nio-12L. While we cannot merge this as is since it would fail on sfdisk versions before 2.41 a simple version. Here is a patch for build/main that integrates a version check.

diff --git a/lib/functions/image/partitioning.sh b/lib/functions/image/partitioning.sh
index 1d9aa34c0..af7cca6c9 100644
--- a/lib/functions/image/partitioning.sh
+++ b/lib/functions/image/partitioning.sh
@@ -89,6 +89,7 @@ function prepare_partitions() {
 
 	# default BOOTSIZE to use if not specified
 	DEFAULT_BOOTSIZE=256 # MiB
+	SECTOR_SIZE=${SECTOR_SIZE:-512}
 	# size of UEFI partition. 0 for no UEFI. Don't mix UEFISIZE>0 and BOOTSIZE>0
 	UEFISIZE=${UEFISIZE:-0}
 	BIOSSIZE=${BIOSSIZE:-0}
@@ -242,7 +243,16 @@ function prepare_partitions() {
 		)
 		# Output the partitioning options from above to the debug log first and then pipe it into the 'sfdisk' command
 		display_alert "Partitioning with the following options" "$partition_script_output" "debug"
-		echo "${partition_script_output}" | run_host_command_logged sfdisk "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+
+		# Check sfdisk version to determine if --sector-size is supported
+		sfdisk_version=$(sfdisk --version | awk '/util-linux/ {print $NF}')
+		sfdisk_version_num=$(echo "$sfdisk_version" | awk -F. '{printf "%d%02d%02d\n", $1, $2, $3}')
+		if [ "$sfdisk_version_num" -ge "24100" ]; then
+			echo "${partition_script_output}" | run_host_command_logged sfdisk --sector-size "$SECTOR_SIZE" "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+		else
+			echo "${partition_script_output}" | run_host_command_logged sfdisk "${SDCARD}".raw || exit_with_error "Partitioning failed!"
+		fi
+
 	fi
 	
 	call_extension_method "post_create_partitions" <<- 'POST_CREATE_PARTITIONS'
@@ -256,7 +266,11 @@ function prepare_partitions() {
 
 	declare -g LOOP
 	#--partscan is using to force the kernel for scaning partition table in preventing of partprobe errors
-	LOOP=$(losetup --show --partscan --find "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	if [ "$sfdisk_version_num" -ge "24100" ]; then
+		LOOP=$(losetup --show --partscan --find -b "$SECTOR_SIZE" "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	else
+		LOOP=$(losetup --show --partscan --find "${SDCARD}".raw) || exit_with_error "Unable to find free loop device"
+	fi
 	display_alert "Allocated loop device" "LOOP=${LOOP}"
 
 	# loop device was grabbed here, unlock

Thanks for the code, I'll rebase it later.
@amazingfate he'll probably make a deb package later so that the image can be built directly inside docker

[HeyMeco]

This way it keeps functionality on util-linux 2.41< and allows for sector size setting on >2.41 hosts (Like Debian Trixie)

[igorpecovnik]

We need sfdisk that is not a part of Ubuntu Noble to make this work?

[HeyMeco]

We need sfdisk that is not a part of Ubuntu Noble to make this work?

https://answers.launchpad.net/ubuntu/+source/util-linux/+question/821212
I will probably have to backport it to ubuntu per that reply I got but I haven’t gotten around to it yet.
Edit: It wasn't a quick cherry pick fix so it would be better if we just upgrade the host dependency to 2.41 ourselves @igorpecovnik and yes that is not part of ubuntu noble or any other newer version https://launchpad.net/ubuntu/+source/util-linux

[igorpecovnik]

I propose copy/pasting info here: https://docs.armbian.com/Developer-Guide_Build-Switches/#advanced

[HeyMeco]

I propose copy/pasting info here: https://docs.armbian.com/Developer-Guide_Build-Switches/#advanced

done