From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Wed, 05 Sep 2012 14:19:49 -0600
Subject: [U-Boot] [PATCH 4/6] gpt: Support for GPT (GUID Partition
	Table) restoration
In-Reply-To: <1345795995-24656-5-git-send-email-l.majewski@samsung.com>
References: <1345795995-24656-1-git-send-email-l.majewski@samsung.com>
	<1345795995-24656-5-git-send-email-l.majewski@samsung.com>
Message-ID: <5047B3E5.70609@wwwdotorg.org>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> The restoration of GPT table (both primary and secondary) is now possible.
> Simple GUID generation is supported.

> +/**
> + * guid_gen(): Generate UUID
> + *
> + * @param dev_desc - block device descriptor
> + *
> + * @return - generated UUID table
> + *
> + * NOTE: The entrophy of this function is small
> + */
> +static u8 *guid_gen(block_dev_desc_t * dev_desc)
> +{
> +	int k = 0;
> +	static int i = 1;
> +	static u8 __aligned(CONFIG_SYS_CACHELINE_SIZE) guid[16];

Hmmm. Wouldn't it be better to take a pointer to the GUID as a parameter
rather than returning a pointer to the same static over and over again.
That way, the caller won't cause problems if they call this function 4
times, and cache the pointer each time.

> +	static u8 __aligned(CONFIG_SYS_CACHELINE_SIZE) ent_pool[512];
> +	u32 *ptr = (u32 *) guid;
> +
> +	/* Entrophy initialization - read random content of one SD sector */
> +	if (i == 1) {
> +		debug("Init entropy:%x\n", (u32)(dev_desc->lba >> 14));
> +
> +		if (dev_desc->block_read(dev_desc->dev, (dev_desc->lba >> 14),
> +					 1, (u32 *) ent_pool) != 1) {

I imagine you might get more entropy out of just reading sector 0, or 1
(or both) than some random sector in the middle of the disk, which quite
possibly won't ever have been written to.

Is there any particular reason why ">> 14" rather than any other shift?

> +			printf("** Can't read from device %d **\n",
> +			       dev_desc->dev);
> +		}
> +	}
> +
> +	for (k = 0; k < 4; k++) {
> +		*(ptr + k) = efi_crc32((const void *) ent_pool,
> +				       sizeof(ent_pool));
> +		ent_pool[511 - k] = *(ptr + k);
> +	}
> +
> +	ent_pool[0] = ((u8) i) & 0xff;

That doesn't quite implement a fully compliant UUID. According to:

http://en.wikipedia.org/wiki/Universally_unique_identifier

the "variant" (UUID) and "version" (4; random) should be encoded into a
few specific bits of the final UUID value.

> +	debug("GUID: ");
> +	for (k = 0; k < sizeof(guid); k++)
> +		debug(" %x ", guid[k]);

I think inventing (stealing from Linux) a proper UUID formatting
function would be useful; that way it could be shared by
print_part_efi() if that function was ever enhanced to print the
partition UUID and partition type UUID.

> +	debug("     i:%d,\n", i);
> +
> +	i++;
> +	return guid;
> +}