Re: [PATCH 1/6] disk: part_efi: Size the partition entry array from the block size

On 7/31/26 14:40, Alexey Charkov wrote:
> gpt_fill_header() computes last_usable_lba and first_usable_lba from
> hardcoded block counts that silently assumes 512-byte sectors and the

Hello Alexey,

Thank you for fixing this issue.

Nits:

%s/assumes/assume/

> default number of partition entries.
 > > The partition entry array holds GPT_ENTRY_NUMBERS entries of 128 bytes
> each, so the number of blocks it needs depends on both the entry count

The size of the partition table entries is not fixed to 128 bytes. 
Instead it is 128 * 2 ** n with n >= 0. The size is stored in field 
SizeOfPartitionEntry of the partition table header.

You already consider this value in gpt_pte_blocks(). It is just the 
commit message that is misleading.

> and the block size. write_gpt_table() derives that count correctly and
> writes the backup array at last_usable_lba + 1, so whenever the two
> disagree the backup array no longer immediately precedes the backup GPT

It remains unclear which two values might "disagree".

Otherwise the patch looks good to me.

Best regards

Heinrich

> header. Furthermore, on a device with 4096-byte native sectors, such as
> UFS flash, and the default 128 entries, the array is 4 blocks rather than
> 32. Current code reserves 34 blocks at each end of the disk for any block
> size and wastes about 114 KiB at each end.
 > > Add a new helper gpt_pte_blocks() and use it in all four places which
> currently calculate the number of blocks each in its own way, so the
> layout written by gpt_fill_header() and the extent written by
> write_gpt_table() cannot drift apart again. With this, first_usable_lba
> on 4096-byte sectors is 6, last_usable_lba is lba - 6, and the backup
> array occupies lba - 5 .. lba - 2, immediately preceding the backup
> header, as the UEFI specification describes.
> 
> Note that this changes the 512-byte layout too for boards that do not use
> the default entry count. CONFIG_EFI_PARTITION_ENTRIES_NUMBERS is
> "default 56 if ARCH_SUNXI" and is set to 64 by a number of Rockchip
> defconfigs:
> 
>    entries  array blocks  first_usable_lba  last_usable_lba
>         56            14        34 -> 16    lba - 34 -> lba - 16
>         64            16        34 -> 18    lba - 34 -> lba - 18
>        128            32        34 -> 34    lba - 34 -> lba - 34
> 
> Existing partition tables stay readable either way, since is_gpt_valid()
> locates the entry array from the on-disk partition_entry_lba. Only newly
> written tables change. Partitions given without an explicit start= will
> now be placed lower on those boards; on sunxi first_usable_lba lands on
> the 8 KiB SPL offset, so such boards should keep specifying start=
> explicitly.
> 
> While here, report both LBAs when the requested layout does not fit, since
> the existing "Partitions layout exceeds disk size" debug message gives no
> clue as to by how much, making it less helpful in debugging.
> 
> Co-developed-by: Anton Burticica <[email protected]>
> Signed-off-by: Anton Burticica <[email protected]>
> Signed-off-by: Alexey Charkov <[email protected]>
> ---
>   disk/part_efi.c | 47 +++++++++++++++++++++++++++++++++++------------
>   1 file changed, 35 insertions(+), 12 deletions(-)
> 
> diff --git a/disk/part_efi.c b/disk/part_efi.c
> index d8b17ec2e91a..af311867a285 100644
> --- a/disk/part_efi.c
> +++ b/disk/part_efi.c
> @@ -388,10 +388,23 @@ static int set_protective_mbr(struct blk_desc *desc)
>   	return 0;
>   }
>   
> +/**
> + * gpt_pte_blocks() - number of blocks taken up by a partition entry array
> + *
> + * @desc:	block device descriptor
> + * @gpt_h:	GPT header describing the array
> + * Return: number of blocks the array occupies on @desc
> + */
> +static u32 gpt_pte_blocks(struct blk_desc *desc, const gpt_header *gpt_h)
> +{
> +	return DIV_ROUND_UP(le32_to_cpu(gpt_h->num_partition_entries) *
> +			    le32_to_cpu(gpt_h->sizeof_partition_entry),
> +			    desc->blksz);
> +}
> +
>   int write_gpt_table(struct blk_desc *desc, gpt_header *gpt_h, gpt_entry *gpt_e)
>   {
> -	const int pte_blk_cnt = BLOCK_CNT((gpt_h->num_partition_entries
> -					   * sizeof(gpt_entry)), desc);
> +	const int pte_blk_cnt = gpt_pte_blocks(desc, gpt_h);
>   	u32 calc_crc32;
>   
>   	debug("max lba: %x\n", (u32)desc->lba);
> @@ -453,9 +466,7 @@ int gpt_fill_pte(struct blk_desc *desc,
>   	size_t hdr_end = hdr_start + 1;
>   
>   	size_t pte_start = gpt_h->partition_entry_lba;
> -	size_t pte_end = pte_start +
> -		gpt_h->num_partition_entries * gpt_h->sizeof_partition_entry /
> -		desc->blksz;
> +	size_t pte_end = pte_start + gpt_pte_blocks(desc, gpt_h);
>   
>   	for (i = 0; i < parts; i++) {
>   		/* partition starting lba */
> @@ -482,7 +493,9 @@ int gpt_fill_pte(struct blk_desc *desc,
>   		gpt_e[i].starting_lba = cpu_to_le64(start);
>   
>   		if (offset > (last_usable_lba + 1)) {
> -			log_debug("Partitions layout exceeds disk size\n");
> +			log_debug("Partitions layout exceeds disk size: "
> +				  LBAFU " > " LBAFU "\n",
> +				  offset, last_usable_lba + 1);
>   			return -E2BIG;
>   		}
>   		/* partition ending lba */
> @@ -604,18 +617,29 @@ static uint32_t partition_entries_offset(struct blk_desc *desc)
>   int gpt_fill_header(struct blk_desc *desc, gpt_header *gpt_h, char *str_guid,
>   		    int parts_count)
>   {
> +	u32 pte_sectors;
> +
>   	gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE_UBOOT);
>   	gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1);
>   	gpt_h->header_size = cpu_to_le32(sizeof(gpt_header));
> +	gpt_h->num_partition_entries = cpu_to_le32(GPT_ENTRY_NUMBERS);
> +	gpt_h->sizeof_partition_entry = cpu_to_le32(sizeof(gpt_entry));
> +
> +	/*
> +	 * Number of blocks occupied by the partition entry array. For the
> +	 * default of 128 entries that is 32 blocks on 512-byte sectors, but
> +	 * only 4 blocks on 4096-byte sectors.
> +	 */
> +	pte_sectors = gpt_pte_blocks(desc, gpt_h);
> +
>   	gpt_h->my_lba = cpu_to_le64(1);
>   	gpt_h->alternate_lba = cpu_to_le64(desc->lba - 1);
> -	gpt_h->last_usable_lba = cpu_to_le64(desc->lba - 34);
> +	/* Reserve space for backup GPT header (1) + backup partition entries */
> +	gpt_h->last_usable_lba = cpu_to_le64(desc->lba - pte_sectors - 2);
>   	gpt_h->partition_entry_lba =
>   		cpu_to_le64(partition_entries_offset(desc));
>   	gpt_h->first_usable_lba =
> -		cpu_to_le64(le64_to_cpu(gpt_h->partition_entry_lba) + 32);
> -	gpt_h->num_partition_entries = cpu_to_le32(GPT_ENTRY_NUMBERS);
> -	gpt_h->sizeof_partition_entry = cpu_to_le32(sizeof(gpt_entry));
> +		cpu_to_le64(le64_to_cpu(gpt_h->partition_entry_lba) + pte_sectors);
>   	gpt_h->header_crc32 = 0;
>   	gpt_h->partition_entry_array_crc32 = 0;
>   
> @@ -747,8 +771,7 @@ static void restore_primary_gpt_header(gpt_header *gpt_h, struct blk_desc *desc)
>   static int write_one_gpt_table(struct blk_desc *desc, gpt_header *gpt_h,
>   			       gpt_entry *gpt_e)
>   {
> -	const int pte_blk_cnt = BLOCK_CNT((gpt_h->num_partition_entries
> -					   * sizeof(gpt_entry)), desc);
> +	const int pte_blk_cnt = gpt_pte_blocks(desc, gpt_h);
>   	lbaint_t start;
>   	int ret = 0;
>   
>