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

login
register
mail settings
Submitter Piotr Wilczek
Date Nov. 9, 2012, 10:48 a.m.
Message ID <1352458087-20462-2-git-send-email-p.wilczek@samsung.com>
Download mbox | patch
Permalink /patch/198016/
State Changes Requested
Delegated to: Tom Rini
Headers show

Comments

Piotr Wilczek - Nov. 9, 2012, 10:48 a.m.
From: Lukasz Majewski <l.majewski@samsung.com>

The restoration of GPT table (both primary and secondary) is now possible.
Simple GUID generation is supported.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Piotr Wilczek <p.wilczek@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes for v2:
- Move GPT Header and Page Table Entries generation code to cmd_gpt.c
- Provide clean API to use set_gpt_table function for GPT restoration on
  a block device

Changes for v3:
- Replace printf with puts

Changes for v4:
- Add public functions for fill gpt header and entries
- Add public function for fill and save gpt tables on the basis of
  simple partions list information
---
 disk/part_efi.c |  342 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/part.h  |    8 ++
 2 files changed, 350 insertions(+), 0 deletions(-)
Stephen Warren - Nov. 19, 2012, 8:16 p.m.
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?

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.

> 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.

> +/**
> + * set_gpt_table() - Restore the GUID Partition Table

"write" would probably be better than "set".

> + *
> + * @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.

> + * @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.

> +	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?

> +{
> +	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?

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.

> +		/* 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.

> +		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?

> +		/* partition attributes */
> +		memset(&gpt_e[i].attributes, 0,
> +		       sizeof(gpt_entry_attributes));

(Perhaps in the future) We should allow specifying attributes too.

> +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?

> +	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?

> +void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t *partitions[],
> +		const int parts_count)

Rename to gpt_write()?

> +	puts("save the GPT ...\n");

Again, why print anything here?

> +	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?

> +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.

> +#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?

> +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'?

> +	}
> +	memcpy(part_name, p, strlen(p));

Why not write directly to part_name?
Piotr Wilczek - Nov. 21, 2012, 1:18 p.m.
Dear Stephen,

> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Monday, November 19, 2012 9:17 PM
> To: Piotr Wilczek
> Cc: u-boot@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.
> 
> > +	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.
> 
> > +		/* 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?
> 
> > +	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?
> 
> > +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?
> 
> > +	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.
> 
> > +#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?
> 
> > +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'?
> 
> > +	}
> > +	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
Ɓukasz Majewski - Nov. 22, 2012, 12:16 p.m.
Hi Piotr,

> Dear Stephen,
> 
> > -----Original Message-----
> > From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> > Sent: Monday, November 19, 2012 9:17 PM
> > To: Piotr Wilczek
> > Cc: u-boot@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
> 
>
Stephen Warren - Nov. 24, 2012, 6:05 p.m.
On 11/21/2012 06:18 AM, Piotr Wilczek wrote:
> Dear Stephen,
> 
>> Stephen Warren wrote at Monday, November 19, 2012 9:17 PM:
>> 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.
...
>>> +	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.

But if the user provided a start address, shouldn't it always be honored
exactly, or any error generated; silently ignoring a start address when
there's an overlap seems wrong. Also, the overlap checking seems a
little simplistic; it should really sort the partition array and then
walk through checking for overlaps, rather than maintaining a single
"highest used offset", since there's no requirement for the physical
order of partitions to match the order in the partition table, hence why
I asked:

>> Why can't partitions be out of order? IIRC, the GPT spec allows it.


>> 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.

Perhaps. It can always be made non-static at that time.
Piotr Wilczek - Nov. 26, 2012, 1:08 p.m.
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@wwwdotorg.org]
> Sent: Saturday, November 24, 2012 7:06 PM
> To: Piotr Wilczek
> Cc: u-boot@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/21/2012 06:18 AM, Piotr Wilczek wrote:
> > Dear Stephen,
> >
> >> Stephen Warren wrote at Monday, November 19, 2012 9:17 PM:
> >> 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.
> ...
> >>> +	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.
> 
> But if the user provided a start address, shouldn't it always be
> honored exactly, or any error generated; silently ignoring a start
> address when there's an overlap seems wrong. Also, the overlap checking
> seems a little simplistic; it should really sort the partition array
> and then walk through checking for overlaps, rather than maintaining a
> single "highest used offset", since there's no requirement for the
> physical order of partitions to match the order in the partition table,
> hence why I asked:
> 
You are right. Ignoring start address when there's overlap is a bad idea and
I change the code.

The overlap check method is simple: the offset points to the first address
where the next partition can start. At first it is equal to the first usable
lba, then is increased by size of the previous partition (now I see that
there's a bug in the code I posted, it should be: offset +=
partitions[i]->size);
I think that this simple overlap check method should be enough at the
moment.

> >> Why can't partitions be out of order? IIRC, the GPT spec allows it.
> 
> 
> >> 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.
> 
> Perhaps. It can always be made non-static at that time.

Making it non-static I say that it was designed to be used by other code.
Stephen Warren - Nov. 26, 2012, 11:46 p.m.
On 11/22/2012 05:16 AM, Lukasz Majewski wrote:
> Hi Piotr,
> 
>> Dear Stephen,
>>
>>> Stephen Warren  wrote at Monday, November 19, 2012 9:17 PM:
>>> 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.

>>>> +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

Ah right, I was forgetting that there is no protective MBR at the end of
the disk, so 1 less sector is used there.

>>>> +	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;....

I guess that'd work. Note that means the calculation of # of partitions
based on counting the # of ; different. Perhaps a separate command-line
argument would be simpler?

Patch

diff --git a/disk/part_efi.c b/disk/part_efi.c
index bacb40a..e501ac1 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -37,6 +37,8 @@ 
 #include <part_efi.h>
 #include <linux/ctype.h>
 
+DECLARE_GLOBAL_DATA_PTR;
+
 #if defined(CONFIG_CMD_IDE) || \
     defined(CONFIG_CMD_SATA) || \
     defined(CONFIG_CMD_SCSI) || \
@@ -71,6 +73,16 @@  static gpt_entry *alloc_read_gpt_entries(block_dev_desc_t * dev_desc,
 
 static int is_pte_valid(gpt_entry * pte);
 
+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);
+
 static char *print_efiname(gpt_entry *pte)
 {
 	static char name[PARTNAME_SZ + 1];
@@ -225,6 +237,197 @@  int test_part_efi(block_dev_desc_t * dev_desc)
 	return 0;
 }
 
+/**
+ * set_gpt_table() - Restore the GUID Partition Table
+ *
+ * @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
+ *
+ * @return - zero on success, otherwise error
+ */
+int set_gpt_table(block_dev_desc_t *dev_desc,
+		  gpt_header *gpt_h, gpt_entry *gpt_e)
+{
+	const int pte_blk_num = (GPT_ENTRY_NUMBERS * sizeof(gpt_entry)) /
+		dev_desc->blksz;
+	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));
+	gpt_h->partition_entry_array_crc32 = cpu_to_le32(calc_crc32);
+
+	calc_crc32 = efi_crc32((const unsigned char *)gpt_h,
+			      le32_to_cpu(gpt_h->header_size));
+	gpt_h->header_crc32 = cpu_to_le32(calc_crc32);
+
+	/* Write the First GPT to the block right after the Legacy MBR */
+	if (dev_desc->block_write(dev_desc->dev, 1, 1, gpt_h) != 1)
+		goto err;
+
+	if (dev_desc->block_write(dev_desc->dev, 2, pte_blk_num, gpt_e)
+	    != pte_blk_num)
+		goto err;
+
+	/* recalculate the values for the Second GPT Header*/
+	val = le64_to_cpu(gpt_h->my_lba);
+	gpt_h->my_lba = gpt_h->alternate_lba;
+	gpt_h->alternate_lba = cpu_to_le64(val);
+	gpt_h->header_crc32 = 0;
+
+	calc_crc32 = efi_crc32((const unsigned char *)gpt_h,
+			      le32_to_cpu(gpt_h->header_size));
+	gpt_h->header_crc32 = cpu_to_le32(calc_crc32);
+
+	if (dev_desc->block_write(dev_desc->dev,
+				  le32_to_cpu(gpt_h->last_usable_lba + 1),
+				  pte_blk_num, gpt_e) != pte_blk_num)
+		goto err;
+
+	if (dev_desc->block_write(dev_desc->dev,
+				  le32_to_cpu(gpt_h->my_lba), 1, gpt_h) != 1)
+		goto err;
+
+	puts("GPT successfully written to block device!\n");
+	return 0;
+
+ err:
+	printf("** Can't write to device %d **\n",
+	       dev_desc->dev);
+	return -1;
+}
+
+/**
+ * gpt_fill_pte(): Fill the GPT partition table entry
+ *
+ * @param gpt_h - GPT header representation
+ * @param gpt_e - GPT partition table entries
+ * @param partitions - list of partitions
+ * @param parts - number of partitions
+ */
+void gpt_fill_pte(gpt_header *gpt_h, gpt_entry *gpt_e,
+		disk_partition_t *partitions[], int parts)
+{
+	u32 offset = (u32) le32_to_cpu(gpt_h->first_usable_lba);
+	u8 guid[sizeof(efi_guid_t)];
+	char *str_uuid;
+	ulong start;
+	int i;
+
+	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);
+
+		offset = le32_to_cpu(gpt_e[i].starting_lba)
+			+ partitions[i]->size;
+		if (offset >= gpt_h->last_usable_lba) {
+			printf("Partitions layout excedds disk size\n");
+			return;
+		}
+		/* 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;
+
+		/* partition type GUID*/
+		memcpy(gpt_e[i].partition_type_guid.b,
+			&PARTITION_BASIC_DATA_GUID, 16);
+
+		str_uuid = NULL;
+#ifdef CONFIG_PARTITION_UUIDS
+		str_uuid = partitions[i]->uuid;
+#endif
+		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));
+		}
+
+		/* partition attributes */
+		memset(&gpt_e[i].attributes, 0,
+		       sizeof(gpt_entry_attributes));
+
+		/* partition name */
+		set_part_name(gpt_e[i].partition_name,
+			(char *)partitions[i]->name);
+
+		debug("%s: name: %s offset[%d]: 0x%x size[%d]: 0x%lx\n",
+		      __func__, partitions[i]->name, i,
+		      offset, i, partitions[i]->size);
+	}
+}
+
+/**
+ * gpt_fill_header(): Fill the GPT header
+ *
+ * @param dev_desc - block device descriptor
+ * @param gpt_h - GPT header representation
+ */
+void gpt_fill_header(block_dev_desc_t *dev_desc, gpt_header *gpt_h)
+{
+	u8 guid[sizeof(gpt_h->disk_guid.b)];
+	char *s;
+
+	gpt_h->signature = cpu_to_le64(GPT_HEADER_SIGNATURE);
+	gpt_h->revision = cpu_to_le32(GPT_HEADER_REVISION_V1);
+	gpt_h->header_size = cpu_to_le32(sizeof(gpt_header));
+	gpt_h->my_lba = cpu_to_le64(1);
+	gpt_h->alternate_lba = cpu_to_le64(dev_desc->lba - 1);
+	gpt_h->first_usable_lba = cpu_to_le64(34);
+	gpt_h->last_usable_lba = cpu_to_le64(dev_desc->lba - 34);
+	gpt_h->partition_entry_lba = cpu_to_le64(2);
+	gpt_h->num_partition_entries = cpu_to_le32(GPT_ENTRY_NUMBERS);
+	gpt_h->sizeof_partition_entry = cpu_to_le32(sizeof(gpt_entry));
+	gpt_h->header_crc32 = 0;
+	gpt_h->partition_entry_array_crc32 = 0;
+
+	s = getenv("uuid_gpt_disk");
+	if (!convert_uuid(s, gpt_h->disk_guid.b)) {
+		guid_gen((u32 *)gd->start_addr_sp, guid);
+		memcpy(gpt_h->disk_guid.b, guid, sizeof(efi_guid_t));
+	}
+}
+
+/**
+ * gpt_fill(): Fill the GPT header
+ *
+ * @param dev_desc - block device descriptor
+ * @param partitions - list of partitions
+ * @param parts - number of partitions
+ */
+void gpt_fill(block_dev_desc_t *dev_desc, disk_partition_t *partitions[],
+		const int parts_count)
+{
+	gpt_header *gpt_h = calloc(sizeof(gpt_header), 1);
+	gpt_entry *gpt_e = calloc(sizeof(gpt_entry), GPT_ENTRY_NUMBERS);
+
+	/* Generate Primary GPT header (LBA1) */
+	gpt_fill_header(dev_desc, gpt_h);
+	gpt_fill_pte(gpt_h, gpt_e, partitions, parts_count);
+
+	puts("save the GPT ...\n");
+	set_gpt_table(dev_desc, gpt_h, gpt_e);
+
+	free(gpt_e);
+	free(gpt_h);
+}
+
 /*
  * Private functions
  */
@@ -453,4 +656,143 @@  static int is_pte_valid(gpt_entry * pte)
 		return 1;
 	}
 }
+
+/**
+ * set_protective_mbr(): Set the EFI protective MBR
+ * @param dev_desc - block device descriptor
+ *
+ * @return - zero on success, otherwise error
+ */
+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));
+	/* Append signature */
+	p_mbr.signature = MSDOS_MBR_SIGNATURE;
+	p_mbr.partition_record[0].sys_ind = EFI_PMBR_OSTYPE_EFI_GPT;
+	p_mbr.partition_record[0].start_sect = 1;
+	p_mbr.partition_record[0].nr_sects = (u32) dev_desc->lba;
+
+	/* Write MBR sector to the MMC device */
+	if (dev_desc->block_write(dev_desc->dev, 0, 1, &p_mbr) != 1) {
+		printf("** Can't write to device %d **\n",
+			dev_desc->dev);
+		return -1;
+	}
+
+	return 0;
+}
+
+#ifdef DEBUG
+/**
+ * guid_dump(): Dump guid content
+ *
+ * @param guid - pinter to guid
+ * @param i - number of bytes to dump
+ */
+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
+
+/**
+ * guid_gen(): Generate UUID
+ *
+ * @param pool - pointer to pseudo random data (e.g. SP)
+ * @param guid - pointer to guid table
+ *
+ * @return - generated UUID table
+ *
+ * NOTE: The entrophy of this function is small
+ */
+static void guid_gen(const u32 *pool, u8 *guid)
+{
+	int k = 0;
+	u32 *ptr = (u32 *) guid;
+
+	debug("%s: pool: 0x%p guid: 0x%p\n", __func__, pool, guid);
+	for (k = 0; k < 4; k++)
+		*(ptr + k) = crc32((u32) pool, (const void *) (pool - k), 512);
+
+	guid_dump(guid, sizeof(efi_guid_t));
+}
+
+/**
+ * convert_uuid(); Convert UUID stored as string to bytes
+ *
+ * @param uuid - UUID represented as string
+ * @param dst - GUID buffer
+ *
+ * @return return 0 on successful conversion
+ */
+static int convert_uuid(char *uuid, u8 *dst)
+{
+	efi_guid_t guid;
+	u16 b, c, d;
+	u64 e;
+	u32 a;
+	u8 *p;
+	u8 i;
+
+	const u8 uuid_str_len = 36;
+
+	/* The UUID is written in text: */
+	/* 1        9    14   19   24 */
+	/* xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx */
+
+	debug("%s: uuid: %s\n", __func__, uuid);
+
+	if (strlen(uuid) != uuid_str_len)
+		return -1;
+
+	for (i = 0; i < uuid_str_len; i++) {
+		if ((i == 8) || (i == 13) || (i == 18) || (i == 23)) {
+			if (uuid[i] != '-')
+				return -1;
+		} else {
+			if (!isxdigit(uuid[i]))
+				return -1;
+		}
+	}
+
+	a = (u32)simple_strtoul(uuid, NULL, 16);
+	b = (u16)simple_strtoul(uuid + 9, NULL, 16);
+	c = (u16)simple_strtoul(uuid + 14, NULL, 16);
+	d = (u16)simple_strtoul(uuid + 19, NULL, 16);
+	e = (u64)simple_strtoull(uuid + 24, NULL, 16);
+
+	p = (u8 *) &e;
+	guid = EFI_GUID(a, b, c, d >> 8, d & 0xFF,
+			*(p + 5), *(p + 4), *(p + 3),
+			*(p + 2), *(p + 1) , *p);
+
+	guid_dump(guid.b, sizeof(efi_guid_t));
+	memcpy(dst, guid.b, sizeof(efi_guid_t));
+
+	return 0;
+}
+
+static void set_part_name(efi_char16_t *part_name, char *name)
+{
+	int k, j;
+	char p[PARTNAME_SZ];
+
+	memset(p, 0x00, sizeof(p));
+	for (k = 0, j = 0; k < strlen(name); k++, j += 2) {
+		p[j] = *(name + k);
+		p[j + 1] = '.';
+	}
+	memcpy(part_name, p, strlen(p));
+}
+
 #endif
diff --git a/include/part.h b/include/part.h
index 27ea283..40e175a 100644
--- a/include/part.h
+++ b/include/part.h
@@ -176,10 +176,18 @@  int   test_part_amiga (block_dev_desc_t *dev_desc);
 #endif
 
 #ifdef CONFIG_EFI_PARTITION
+#include <part_efi.h>
 /* disk/part_efi.c */
 int get_partition_info_efi (block_dev_desc_t * dev_desc, int part, disk_partition_t *info);
 void print_part_efi (block_dev_desc_t *dev_desc);
 int   test_part_efi (block_dev_desc_t *dev_desc);
+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);
 #endif
 
 #endif /* _PART_H */