Hi Steve,
> Hi Lukasz,
>
> On 14-12-09 01:05 AM, Lukasz Majewski wrote:
> > Hi Steve,
> >
> >> Implement a feature to allow fastboot to write the downloaded image
> >> to the space reserved for the Protective MBR and the Primary GUID
> >> Partition Table.
> >>
> >> Signed-off-by: Steve Rae <s...@broadcom.com>
> >> ---
> >>
> >> Changes in v2:
> >> add validation of the GPT before writing to flash
> >> (suggested by: Lukasz Majewski <l.majew...@samsung.com>)
> >>
> >> README | 7 +++++++
> >> common/fb_mmc.c | 26 +++++++++++++++++++++++---
> >> disk/part_efi.c | 37 +++++++++++++++++++++++++++++++++++++
> >> include/part.h | 10 ++++++++++
> >> 4 files changed, 77 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/README b/README
> >> index 4ca04d0..5f50cdd 100644
> >> --- a/README
> >> +++ b/README
> >> @@ -1773,6 +1773,13 @@ The following options need to be configured:
> >> regarding the non-volatile storage device. Define
> >> this to the eMMC device that fastboot should use to store the
> >> image.
> >>
> >> + CONFIG_FASTBOOT_GPT_NAME
> >> + The fastboot "flash" command supports writing the
> >> downloaded
> >> + image to the Protective MBR and the Primary GUID
> >> Partition
> >> + Table. This occurs when the specified "partition
> >> name" on the
> >> + "fastboot flash" command line matches this value.
> >> + Default is GPT_ENTRY_NAME (currently "gpt") if
> >> undefined. +
> >> - Journaling Flash filesystem support:
> >> CONFIG_JFFS2_NAND, CONFIG_JFFS2_NAND_OFF,
> >> CONFIG_JFFS2_NAND_SIZE, CONFIG_JFFS2_NAND_DEV
> >> diff --git a/common/fb_mmc.c b/common/fb_mmc.c
> >> index fb06d8a..a646a7b 100644
> >> --- a/common/fb_mmc.c
> >> +++ b/common/fb_mmc.c
> >> @@ -4,12 +4,17 @@
> >> * SPDX-License-Identifier: GPL-2.0+
> >> */
> >>
> >> +#include <config.h>
> >> #include <common.h>
> >> #include <fb_mmc.h>
> >> #include <part.h>
> >> #include <aboot.h>
> >> #include <sparse_format.h>
> >>
> >> +#ifndef CONFIG_FASTBOOT_GPT_NAME
> >> +#define CONFIG_FASTBOOT_GPT_NAME GPT_ENTRY_NAME
> >> +#endif
> >> +
> >> /* The 64 defined bytes plus the '\0' */
> >> #define RESPONSE_LEN (64 + 1)
> >>
> >> @@ -62,9 +67,9 @@ static void write_raw_image(block_dev_desc_t
> >> *dev_desc, disk_partition_t *info, void fb_mmc_flash_write(const
> >> char *cmd, void *download_buffer, unsigned int download_bytes, char
> >> *response) {
> >> - int ret;
> >> block_dev_desc_t *dev_desc;
> >> disk_partition_t info;
> >> + lbaint_t blksz;
> >>
> >> /* initialize the response buffer */
> >> response_str = response;
> >> @@ -76,8 +81,23 @@ void fb_mmc_flash_write(const char *cmd, void
> >> *download_buffer, return;
> >> }
> >>
> >> - ret = get_partition_info_efi_by_name(dev_desc, cmd,
> >> &info);
> >> - if (ret) {
> >> + if (strcmp(cmd, CONFIG_FASTBOOT_GPT_NAME) == 0) {
> >> + printf("%s: updating GUID Partition Table
> >> (including MBR)\n",
> >> + __func__);
> >> + /* start at Protective MBR */
> >> + info.start = (GPT_PRIMARY_PARTITION_TABLE_LBA -
> >> 1);
> >> + blksz = dev_desc->blksz;
> >> + info.blksz = blksz;
> >> + /* assume that the Partition Entry Array starts in
> >> LBA 2 */
> >> + info.size = (2 + (GPT_ENTRY_NUMBERS *
> >> GPT_ENTRY_SIZE) / blksz); +
> >> + if (is_valid_gpt_primary(download_buffer,
> >> dev_desc->blksz)) {
> >> + printf("%s: invalid GPT - refusing to
> >> write to flash\n",
> >> + __func__);
> >> + fastboot_fail("invalid GPT partition");
> >> + return;
> >> + }
> >
> > IMHO some extra code to update secondary (backup) GPT is needed
> > here.
> >
> > A good example may be at part_efi.c - print_part_efi() function [1].
> >
>
> OK -- added "prepare and write Backup GPT" in v3
>
> >> + } else if (get_partition_info_efi_by_name(dev_desc, cmd,
> >> &info)) { error("cannot find partition: '%s'\n", cmd);
> >> fastboot_fail("cannot find partition");
> >> return;
> >> diff --git a/disk/part_efi.c b/disk/part_efi.c
> >> index efed58f..86cb160 100644
> >> --- a/disk/part_efi.c
> >> +++ b/disk/part_efi.c
> >> @@ -455,6 +455,43 @@ err:
> >> free(gpt_h);
> >> return ret;
> >> }
> >> +
> >> +int is_valid_gpt_primary(void *buf, unsigned long blksz)
> >
> > The name is a bit misleading - I rather though about something like
> > is_valid_gpt_buf() and write it in a similar way to is_valid_gpt()
> > which would allow using it for both primary and secondary GPTs
> > (like in [1]).
>
> OK - I changed the name, however, it still can only handle a buffer
> which contains:
> - 512 bytes of MBR, followed by
> - 512 bytes of GPT Header, followed by
> the GPT Entries at the specified offset (typically 2 * 512 bytes)
> Thus, it cannot be used for the Backup GPT anyway....
I think that it should be possible to reuse the code:
1. There is "is_pmbr_valid()" function @ part_efi.c - so you don't need
to check pmbr validity in the is_valid_gpt_buf function.
2. If 1. is done, then one function can be used to check CRC's for
primary and secondary GPT headers and entries (as is done in the
print_part_efi()).
>
> >
> >> +{
> >> + int rc = 0;
> >> + gpt_header *gpt_h;
> >> + gpt_entry *gpt_e;
> >> + gpt_header gpt_hdr;
> >> + uint32_t calc_crc32;
> >> +
> >> + /* GPT Header starts at "LBA[1]" in the buffer */
> >> + gpt_h = buf + blksz;
> >> + if (gpt_h->signature !=
> >> cpu_to_le64(GPT_HEADER_SIGNATURE)) {
> >> + printf("%s: 'signature' is invalid\n", __func__);
> >> + rc += 1;
> >> + }
> >> +
> >> + memcpy(&gpt_hdr, gpt_h, sizeof(gpt_header));
> >> + gpt_hdr.header_crc32 = 0;
> >> + calc_crc32 = efi_crc32((const unsigned char *)&gpt_hdr,
> >> + le32_to_cpu(gpt_h->header_size));
> >> + if (gpt_h->header_crc32 != cpu_to_le32(calc_crc32)) {
> >> + printf("%s: 'header_crc32' is invalid\n",
> >> __func__);
> >> + rc += 1;
> >> + }
> >> +
> >> + /* GPT Entries start at "LBA[2]" in the buffer */
> >> + gpt_e = buf + (2 * blksz);
> >> + 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));
> >> + if (gpt_h->partition_entry_array_crc32 !=
> >> cpu_to_le32(calc_crc32)) {
> >> + printf("%s: 'partition_entry_array_crc32' is
> >> invalid\n",
> >> + __func__);
> >> + rc += 1;
> >> + }
> >> + return rc;
> >> +}
> >> #endif
> >>
> >> /*
> >> diff --git a/include/part.h b/include/part.h
> >> index a496a4a..9cfa4c9 100644
> >> --- a/include/part.h
> >> +++ b/include/part.h
> >> @@ -244,6 +244,16 @@ int gpt_fill_header(block_dev_desc_t
> >> *dev_desc, gpt_header *gpt_h, */
> >> int gpt_restore(block_dev_desc_t *dev_desc, char *str_disk_guid,
> >> disk_partition_t *partitions, const int
> >> parts_count); +
> >> +/**
> >> + * is_valid_gpt_primary() - Ensure that the Primary GPT
> >> information is valid
> >> + *
> >> + * @param buf - buffer which contains the Primary GPT info
> >> + * @param blksz - block size (in bytes)
> >> + *
> >> + * @return - '0' on success, otherwise error
> >> + */
> >> +int is_valid_gpt_primary(void *buf, unsigned long blksz);
> >> #endif
> >>
> >> #endif /* _PART_H */
> >
> >
> >
--
Best regards,
Lukasz Majewski
Samsung R&D Institute Poland (SRPOL) | Linux Platform Group
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
