[U-Boot] [PATCH v4 5/7] gpt: Support for GPT (GUID Partition Table) restoration

[Lukasz Majewski]

Hi Piotr,

> Dear Stephen,
> 
> > -----Original Message-----
> > From: Stephen Warren [mailto:swarren at wwwdotorg.org]
> > Sent: Monday, November 19, 2012 9:17 PM
> > To: Piotr Wilczek
> > Cc: u-boot at lists.denx.de; Minkyu Kang; Kyungmin Park; Chang Hyun
> > Park; Lukasz Majewski; Stephen Warren; Tom Rini; Rob Herring
> > Subject: Re: [PATCH v4 5/7] gpt: Support for GPT (GUID Partition
> > Table) restoration
> > 
> > On 11/09/2012 03:48 AM, Piotr Wilczek wrote:
> > > The restoration of GPT table (both primary and secondary) is now
> > possible.
> > > Simple GUID generation is supported.
> > 
> > > diff --git a/include/part.h b/include/part.h
> > 
> > > +int set_gpt_table(block_dev_desc_t *dev_desc,
> > > +		  gpt_header *gpt_h, gpt_entry *gpt_e); void
> > > +gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
> > > +		disk_partition_t *partitions[], int parts); void
> > > +gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h);
> > 
> > > +void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t
> > *partitions[],
> > > +		const int parts_count);
> > 
> > Why const?
> > 
> The const is not necessary.
> 
> > Some documentation for these functions (particularly high-level
> > information such as which order you'd expect to call them and
> > why/what for) would be useful in the header rather than the .c
> > file; the header is where people would go to find out what
> > functions they can call, so making them also go look in the .c file
> > would be a bit annoying.
> > 
> That documentation should be moved to header files.
> 
> > > diff --git a/disk/part_efi.c b/disk/part_efi.c
> > 
> > > +static int set_protective_mbr(block_dev_desc_t *dev_desc);
> > > +
> > > +static void guid_dump(u8 *guid, int i);
> > > +
> > > +static void guid_gen(const u32 *pool, u8 *guid);
> > > +
> > > +static int convert_uuid(char *uuid, u8 *dst);
> > > +
> > > +static void set_part_name(efi_char16_t *part_name, char *name);
> > 
> > There probably should be a CONFIG_ option required to enable this
> > new feature, so people who don't want it don't suffer code bloat.
> > 
> > Do you really need the blank lines between prototypes?
> > 
> > I suspect you can re-order the functions so that none of the
> > prototypes are needed anyway.
> > 
> I try to reorder the functions and eliminate prototypes.
> 
> > > +/**
> > > + * set_gpt_table() - Restore the GUID Partition Table
> > 
> > "write" would probably be better than "set".
> > 
> Yes, "write" is better.
> 
> > > + *
> > > + * @param dev_desc - block device descriptor
> > > + * @param parts - number of partitions
> > > + * @param size - pointer to array with each partition size
> > > + * @param name - pointer to array with each partition name
> > 
> > Those last 3 lines don't match the prototype.
> > 
> I should fix it.
> 
> > > + * @return - zero on success, otherwise error  */ int
> > > +set_gpt_table(block_dev_desc_t *dev_desc,
> > > +		  gpt_header *gpt_h, gpt_entry *gpt_e)
> > 
> > Presumably the code assumes that gpt_e always has 128(?) entries.
> > Instead of taking a pointer, should the function take an array:
> > gpt_entry gpt_e[GPT_ENTRY_NUMBERS] to enforce this?
> > 
> > > +{
> > > +	const int pte_blk_num = (GPT_ENTRY_NUMBERS *
> > > sizeof(gpt_entry)) /
> > > +		dev_desc->blksz;
> > 
> > Related, this hard-codes the number of ptes as GPT_ENTRY_NUMBERS,
> > whereas ...
> > 
> > > +	u32 calc_crc32;
> > > +	u64 val;
> > > +
> > > +	debug("max lba: %x\n", (u32) dev_desc->lba);
> > > +	/* Setup the Protective MBR */
> > > +	if (set_protective_mbr(dev_desc) < 0)
> > > +		goto err;
> > > +
> > > +	/* Generate CRC for the Primary GPT Header */
> > > +	calc_crc32 = efi_crc32((const unsigned char *)gpt_e,
> > > +
> > > le32_to_cpu(gpt_h->num_partition_entries) *
> > > +
> > > le32_to_cpu(gpt_h->sizeof_partition_entry));
> > 
> > ... here, gpt_h->num_partition_entries is used instead. Should both
> > places use the same size (entry count) definition?
> > 
> > > +	if (dev_desc->block_write(dev_desc->dev, 2, pte_blk_num,
> > > gpt_e)
> > > +	    != pte_blk_num)
> > > +		goto err;
> > 
> > Here, we assume GPT_ENTRY_NUMBERS entries in gpt_e. If the array
> > isn't that big, the block_write() above will read past the end of
> > it.

The code allocates (and thereafter reads) the GPT_ENTRY_NUMBERS (which
is 128) * sizeof(struct pte). It will be changed to allocate only as
much space as needed. 

> > 
> > > +	puts("GPT successfully written to block device!\n");
> > 
> > Isn't that something that a command should be doing, not a utility
> > function? I'd rather have this function not print anything, except
> > perhaps on errors, just like typical Unix command-line applications.
> > 
> > > +void gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
> > > +		disk_partition_t *partitions[], int parts)
> > 
> > Why is partitions an array of pointers rather than an array of
> > partitions?
> > 
> The partitions is a variable size array stack allocated and each
> element points to heap allocated disk_partition. I think of how to
> reorganize this part of the code.
> 
> > > +{
> > > +	u32 offset = (u32) le32_to_cpu(gpt_h->first_usable_lba);
> > 
> > I don't think there should be a space after the cast. The same
> > comment probably applies elsewhere.
> > 
> > > +	for (i = 0; i < parts; i++) {
> > > +		/* partition starting lba */
> > > +		start = partitions[i]->start;
> > > +		if (start && (offset <= start))
> > > +			gpt_e[i].starting_lba =
> > > cpu_to_le32(start);
> > > +		else
> > > +			gpt_e[i].starting_lba =
> > > cpu_to_le32(offset);
> > 
> > That seems a little odd. The else branch seems fine when !start, but
> > what about when (start && (offset > start))? Shouldn't that be an
> > error, rather than just ignoring the requested start value?
> > 
> The idea is that if the user provided start address and the
> partitions does not overlap, the partition is located at the start
> address. Otherwise partitions are located next to each other.
> 
> > Why can't partitions be out of order? IIRC, the GPT spec allows it.
> > 
> > > +		/* partition ending lba */
> > > +		if (i != parts - 1)
> > > +			gpt_e[i].ending_lba =
> > > +				cpu_to_le64(offset - 1);
> > > +		else
> > > +			gpt_e[i].ending_lba =
> > > gpt_h->last_usable_lba;
> > 
> > Why extend the last partition to fill the disk; why not simply
> > always use the requested size? If a "fill to max size" option is
> > implemented, the user might not want it to apply to the last
> > partition, but perhaps some arbitrary partition in the middle of
> > the list.

Yes, you are right. User shall have the option to specify the size for
the last partition. I think, that it is welcome to specify a special
"mark" (e.g. size=- or size=0) to express that we want partition's size
up to the end of the device.

> > 
> > > +		/* partition type GUID*/
> > > +		memcpy(gpt_e[i].partition_type_guid.b,
> > > +			&PARTITION_BASIC_DATA_GUID, 16);
> > 
> > Shouldn't that be a user-specifiable option too? I suppose we could
> > add that later.
> > 
> > > +		str_uuid = NULL;
> > > +#ifdef CONFIG_PARTITION_UUIDS
> > > +		str_uuid = partitions[i]->uuid;
> > > +#endif
> > 
> > I think it should be required to enable that feature if creating
> > GPTs; writing a GPT without a value UUID seems wrong.
> > 
> Yes, it should be checked if CONFIG_PARTITION_UUIDS is defined at all.
> 
> > > +		if (convert_uuid(str_uuid,
> > gpt_e[i].unique_partition_guid.b)) {
> > > +			guid_gen((((u32 *) gd->start_addr_sp) -
> > > i - 1),
> > > +				guid);
> > > +			memcpy(gpt_e[i].unique_partition_guid.b,
> > > guid,
> > > +			       sizeof(efi_guid_t));
> > > +		}
> > 
> > Shouldn't there be a difference between no specified UUID, and an
> > error converting a specified UUID?
> > 
> Yes, it's a good idea.
> 
> > > +		/* partition attributes */
> > > +		memset(&gpt_e[i].attributes, 0,
> > > +		       sizeof(gpt_entry_attributes));
> > 
> > (Perhaps in the future) We should allow specifying attributes too.
> > 
> Ok
> 
> > > +void gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header
> > > *gpt_h)
> > 
> > > +	gpt_h->first_usable_lba = cpu_to_le64(34);
> > > +	gpt_h->last_usable_lba = cpu_to_le64(dev_desc->lba - 34);
> > 
> > Is lba the number of LBAs or the last LBA number? include/part.h
> > says its the number of LBAs, in which case I think that should be
> > dev_desc-
> > >lba - 1 - 34?

LBA is the number of blocks. So the addressable range is 0 to LBA-1.
The last_usable_lba is calculated with following equation (it is
similar with parted tool):

lba - 2 - ptes_size = lba -2 -32 = lba - 34

ptes_size = number_of_entries(128) * sizeof(pte_entry) (128) /
sector_size (512) = 32

> > 
> > > +	s = getenv("uuid_gpt_disk");
> > 
> > I'd rather not depend on environment variables; can't this be a
> > command-line parameter to the gpt command?

I think, that the uuid_gpt_disk shall be added as another key=value to
the partitions.

e.g. gpt
uuid_gpt_disk=6aa01a70-349d-11e2-ae74-001fd09285c0;name=boot,size=100MiB,uuid=xx;name=kernel,
size=200MiB,uuid=yy;....

> > 
> > > +void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t
> > *partitions[],
> > > +		const int parts_count)
> > 
> > Rename to gpt_write()?
> > 
> Ok.
> 
> > > +	puts("save the GPT ...\n");
> > 
> > Again, why print anything here?
Will be changed to debug().

> > 
> > > +	set_gpt_table(dev_desc, gpt_h, gpt_e);
> > 
> > Oh, so is set_gpt_table() an internal-only function? If so,
> > shouldn't it be static and not in the header file?
> > 
> It could be used by some other code in future.
> 
> > > +static int set_protective_mbr(block_dev_desc_t *dev_desc) {
> > > +	legacy_mbr p_mbr;
> > > +
> > > +	/* Setup the Protective MBR */
> > > +	memset((u32 *) &p_mbr, 0x00, sizeof(p_mbr));
> > 
> > Why use a stack variable and memset() here, but use calloc() in
> > gpt_fill() above? That seems inconsistent.

Good point. Consistency will be provided in the next version.

> > 
> > > +#ifdef DEBUG
> > > +static void guid_dump(u8 *guid, int i) {
> > > +	int k;
> > > +
> > > +	debug("GUID: ");
> > > +	for (k = 0; k < i; k++, guid++)
> > > +		debug(" %x ", *guid);
> > > +	debug("\n");
> > > +}
> > > +#else
> > > +static void guid_dump(u8 *guid, int i) {} #endif
> > 
> > Wouldn't using the existing uuid_string be better?

Since the guid_gen will be removed in a next version (the correct UUID
will be provided from command line/env), the guid_dump will be removed
as well.

> > 
> > > +static int convert_uuid(char *uuid, u8 *dst)
> > 
> > It's not too clear from the function name what kind of conversion is
> > being applied. string_uuid() would be more consistent with the
> > existing uuid_string(), or perhaps even better string_to_uuid().
> > 
> > > +static void set_part_name(efi_char16_t *part_name, char *name)
> > 
> > > +	for (k = 0, j = 0; k < strlen(name); k++, j += 2) {
> > > +		p[j] = *(name + k);
> > > +		p[j + 1] = '.';
> > 
> > Not '\0'?

Yes, this is a mistake. Will be fixed.

> > 
> > > +	}
> > > +	memcpy(part_name, p, strlen(p));
> > 
> > Why not write directly to part_name?
> Right, I fix it.
> 
> Best regards,
> Piotr Wilczek
> --
> Samsung Poland R&D Center | Linux Platform Group
> 
> 



-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group