| Message ID | 1345795995-24656-5-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
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. > diff --git a/include/part.h b/include/part.h > +int set_gpt_table(block_dev_desc_t *dev_desc, > + int parts, unsigned int *blocks, char *name[]); That interface seems very limiting; what if you want to select specific type UUIDs, set the partition attributes, leave gaps between the partitions, have the physical order of partitions (their block numbers) be different to the partition table ordering, etc. I wonder if it wouldn't be better to take an array of structures here, and have each entry define all the properties of the partition. That would allow fields to be added to the structure to describe more information as we add more functionality. I didn't review the implementation.
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; > +}
Hi Stephen, > 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. > > > diff --git a/include/part.h b/include/part.h > > > +int set_gpt_table(block_dev_desc_t *dev_desc, > > + int parts, unsigned int *blocks, char *name[]); > > That interface seems very limiting; what if you want to select > specific type UUIDs, set the partition attributes, leave gaps between > the partitions, have the physical order of partitions (their block > numbers) be different to the partition table ordering, etc. > > I wonder if it wouldn't be better to take an array of structures here, > and have each entry define all the properties of the partition. That > would allow fields to be added to the structure to describe more > information as we add more functionality. I agree. I'd propose following approach: - set_gpt_table will receive a table with pointers to partition entries (up to 128 entries in this table). In this way one can easily define partition properties (which can be different for different commands/boards). - I think, that Legacy MBR, and Primary/Secondary GPT shall be generated in the set_gpt_table function. However I expect a problem with one Primary/Secondary GPT field - the Disk GUID. We can read it from environment, generate it or hardcode it at ./include/boards/{target}.h What is your opinion? > > I didn't review the implementation.
On 09/06/2012 05:19 AM, Lukasz Majewski wrote: > Hi Stephen, > >> 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. >> >>> diff --git a/include/part.h b/include/part.h >> >>> +int set_gpt_table(block_dev_desc_t *dev_desc, >>> + int parts, unsigned int *blocks, char *name[]); >> >> That interface seems very limiting; what if you want to select >> specific type UUIDs, set the partition attributes, leave gaps between >> the partitions, have the physical order of partitions (their block >> numbers) be different to the partition table ordering, etc. >> >> I wonder if it wouldn't be better to take an array of structures here, >> and have each entry define all the properties of the partition. That >> would allow fields to be added to the structure to describe more >> information as we add more functionality. > > I agree. > > I'd propose following approach: > > - set_gpt_table will receive a table with pointers to partition entries > (up to 128 entries in this table). In this way one can easily define > partition properties (which can be different for different > commands/boards). > > - I think, that Legacy MBR, and Primary/Secondary GPT shall be > generated in the set_gpt_table function. However I expect a problem > with one Primary/Secondary GPT field > - the Disk GUID. We can read it from environment, generate it or > hardcode it at ./include/boards/{target}.h > > > What is your opinion? Perhaps rather than /just/ taking a pointer to a table of partitions, take a pointer to a struct, which can hold both global properties and a pointer to the list of partitions?
On Thu, Aug 23, 2012 at 10:13:13PM -0000, Lukasz Majewski wrote: > 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: Kyungmin Park <kyungmin.park@samsung.com> While the changes are fine, tt01 and eb_cpux9k2 use CONFIG_PART_EFI and do not set CONFIG_SYS_CACHELINE_SIZE and so fail to build after this patch. tt01 is easily fixable (it relies on a non-exported define elsewhere to 32) but the eb_cpu9k2 please contact the listed board maintainer to get the define added.
Hi Jens and Helmut, > On Thu, Aug 23, 2012 at 10:13:13PM -0000, Lukasz Majewski wrote: > > > 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: Kyungmin Park <kyungmin.park@samsung.com> > > While the changes are fine, tt01 and eb_cpux9k2 use CONFIG_PART_EFI > and do not set CONFIG_SYS_CACHELINE_SIZE and so fail to build after > this patch. tt01 is easily fixable (it relies on a non-exported > define elsewhere to 32) but the eb_cpu9k2 please contact the listed > board maintainer to get the define added. > Would it be possible to add the CONFIG_SYS_CACHELINE_SIZE definition to ./include/configs/{tty01|eb_cpux9k2} boards definition? It would help improving cache alignment and GPT development. Thanks in advance
On 10/04/2012 09:18 AM, Lukasz Majewski wrote: > Hi Jens and Helmut, > >> On Thu, Aug 23, 2012 at 10:13:13PM -0000, Lukasz Majewski wrote: >> >>> 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: Kyungmin Park<kyungmin.park@samsung.com> >> While the changes are fine, tt01 and eb_cpux9k2 use CONFIG_PART_EFI >> and do not set CONFIG_SYS_CACHELINE_SIZE and so fail to build after >> this patch. tt01 is easily fixable (it relies on a non-exported >> define elsewhere to 32) but the eb_cpu9k2 please contact the listed >> board maintainer to get the define added. >> > Would it be possible to add the CONFIG_SYS_CACHELINE_SIZE > definition to ./include/configs/{tty01|eb_cpux9k2} boards definition? > > It would help improving cache alignment and GPT development. > > Thanks in advance Hi Lukasz, feel free to do the appropriate changes in the TT-01 platform code (explicitly setting CACHELINE_SIZE), I'm currently too busy to do any rebasing and testing on the board, we'll have to give it a time slice in the near future (to have a few platform things changed) anyway. Helmut -- Scanned by MailScanner.
diff --git a/disk/part_efi.c b/disk/part_efi.c index 86e7f33..c1e8d54 100644 --- a/disk/part_efi.c +++ b/disk/part_efi.c @@ -403,4 +403,229 @@ static int is_pte_valid(gpt_entry * pte) return 1; } } + +/** + * 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]; + 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) { + 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; + + debug("GUID: "); + for (k = 0; k < sizeof(guid); k++) + debug(" %x ", guid[k]); + + debug(" i:%d,\n", i); + + i++; + return guid; +} + +/** + * 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; +} + +/** + * fill_pte(): Fill the GPT partition table entry + * + * @param dev_desc - block device descriptor + * @param gpt_h - GPT header representation + * @param gpt_e - GPT partition table entries + * @param parts - number of partitions + * @param size - size of each partition + * @param name - name of each partition + */ +static void fill_pte(block_dev_desc_t *dev_desc, gpt_header *gpt_h, + gpt_entry *gpt_e, int parts, unsigned int *size, + char *name[]) +{ + u32 offset = (u32) gpt_h->first_usable_lba; + char p[PARTNAME_SZ]; + int i, k, j; + char *s; + + for (i = 0; i < parts; i++) { + memcpy(gpt_e[i].partition_type_guid.b, + &PARTITION_BASIC_DATA_GUID, 16); + memcpy(gpt_e[i].unique_partition_guid.b, + guid_gen(dev_desc), + sizeof(gpt_e[i].unique_partition_guid.b)); + + s = name[i]; + + memset(p, 0x00, sizeof(p)); + for (k = 0, j = 0; k < strlen(s); k++, j += 2) { + p[j] = *(s + k); + p[j + 1] = '.'; + } + + memcpy(gpt_e[i].partition_name, + p, strlen(p)); + + gpt_e[i].starting_lba = cpu_to_le32(offset); + + /* allocate remaining memory in last partition */ + if (i != parts - 1) { + gpt_e[i].ending_lba = + cpu_to_le64(offset + size[i] - 1); + } else { + gpt_e[i].ending_lba = gpt_h->last_usable_lba; + } + + memset(&gpt_e[i].attributes, 0, + sizeof(gpt_entry_attributes)); + + offset += size[i]; + debug("%s: name: %s offset[%d]: 0x%x size[%d]: 0x%x\n", + __func__, name[i], i, offset, i, size[i]); + } +} + +/** + * 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, + int parts, unsigned int *size, char *name[]) +{ + const int pte_blk_num = (GPT_ENTRY_NUMBERS * sizeof(gpt_entry)) / + dev_desc->blksz; + gpt_entry gpt_e[GPT_ENTRY_NUMBERS]; + gpt_header gpt_h; + 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; + + memset((u32 *) &gpt_h, 0x00, sizeof(gpt_h)); + + /* Generate Primary GPT header (LBA1) */ + 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; + memcpy(gpt_h.disk_guid.b, guid_gen(dev_desc), + sizeof(gpt_h.disk_guid.b)); + + memset((u32 *) gpt_e, 0x00, sizeof(gpt_e)); + + fill_pte(dev_desc, &gpt_h, gpt_e, parts, size, name); + + /* 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); + + /* Write the Second GPT that is located at the end of the disk */ + 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; + + printf("GPT successfully written to block device!\n"); + return 0; + + err: + printf("** Can't write to device %d **\n", + dev_desc->dev); + return -1; +} #endif diff --git a/include/part.h b/include/part.h index e1478f4..e12c10f 100644 --- a/include/part.h +++ b/include/part.h @@ -161,6 +161,8 @@ int test_part_amiga (block_dev_desc_t *dev_desc); 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, + int parts, unsigned int *blocks, char *name[]); #endif #endif /* _PART_H */