| Message ID | 1352458087-20462-2-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
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?
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
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 > >
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.
> -----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.
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?
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 */