From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 19 Nov 2012 13:16:58 -0700 Subject: [U-Boot] [PATCH v4 5/7] gpt: Support for GPT (GUID Partition Table) restoration In-Reply-To: <1352458087-20462-2-git-send-email-p.wilczek@samsung.com> References: <1352452938-2375-1-git-send-email-p.wilczek@samsung.com> <1352458087-20462-1-git-send-email-p.wilczek@samsung.com> <1352458087-20462-2-git-send-email-p.wilczek@samsung.com> Message-ID: <50AA93BA.20403@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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?