Hi Piotr, > Dear Stephen, > > > -----Original Message----- > > From: Stephen Warren [mailto:swar...@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 > > -- Best regards, Lukasz Majewski Samsung Poland R&D Center | Linux Platform Group _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot