[U-Boot,v2,5/6] rename GPT partitions to detect boot failure

From: Alison Chaiken <alison@she-devel.com>

This patch provides support in u-boot for renaming GPT
partitions.  The renaming is accomplished via a new 'gpt flip'
command which is enabled via a CONFIG_CMD_GPT_FLIP option.

The concept for the bootloader state machine is the following:

-- u-boot renames ‘primary’ partitions as ‘candidate’ and tries
   to boot them.
-- Linux, at boot, will rename ‘candidate’ partitions as
   ‘primary’.
-- If u-boot sees a ‘candidate’ partition after a boot attempt,
   it renames it failed’ and renames the ‘backup’ partition as
   ‘candidate’.

Logic:
-- Partitions can go to ‘failed’ only from ‘candidate’ and only
   via u-boot.  Partitions can go to ‘backup’ only from ‘primary’
   and vice-versa, only via Linux.  Partitions go to ‘candidate’
   from ‘primary’ or ‘backup’ only via u-boot.  Only system
   update software will rename 'failed' partitions.

Rewriting the partition table has the side-effect that all partitions
end up with "msftdata" flag set.  The reason is that partition type
PARTITION_BASIC_DATA_GUID is hard-coded in the gpt_fill_pte()
function.  This does not appear to cause any harm.

Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
---
 cmd/Kconfig    |   7 ++
 cmd/gpt.c      | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 doc/README.gpt |  13 ++++
 3 files changed, 215 insertions(+), 4 deletions(-)

alison@peloton-tech.com wrote:

> From: Alison Chaiken <alison@she-devel.com>
> 
> This patch provides support in u-boot for renaming GPT
> partitions.  The renaming is accomplished via a new 'gpt flip'
> command which is enabled via a CONFIG_CMD_GPT_FLIP option.
> 
> The concept for the bootloader state machine is the following:
> 
> -- u-boot renames ‘primary’ partitions as ‘candidate’ and tries
>    to boot them.
> -- Linux, at boot, will rename ‘candidate’ partitions as
>    ‘primary’.
> -- If u-boot sees a ‘candidate’ partition after a boot attempt,
>    it renames it failed’ and renames the ‘backup’ partition as
>    ‘candidate’.
> 
> Logic:
> -- Partitions can go to ‘failed’ only from ‘candidate’ and only
>    via u-boot.  Partitions can go to ‘backup’ only from ‘primary’
>    and vice-versa, only via Linux.  Partitions go to ‘candidate’
>    from ‘primary’ or ‘backup’ only via u-boot.  Only system
>    update software will rename 'failed' partitions.
> 
> Rewriting the partition table has the side-effect that all partitions
> end up with "msftdata" flag set.  The reason is that partition type
> PARTITION_BASIC_DATA_GUID is hard-coded in the gpt_fill_pte()
> function.  This does not appear to cause any harm.
> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/Kconfig    |   7 ++
>  cmd/gpt.c      | 199 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  doc/README.gpt |  13 ++++
>  3 files changed, 215 insertions(+), 4 deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5ee52f6..a8f7716 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -575,6 +575,13 @@ config CMD_GPT
>  	  Enable the 'gpt' command to ready and write GPT style partition
>  	  tables.
>  
> +config CMD_GPT_FLIP
> +	bool "GPT flip-partitions command"
> +	depends on CMD_GPT
> +	help
> +	  Enables the 'gpt' command to write modified GPT partition
> +	  tables via the 'gpt flip' command.
> +
>  config CMD_ARMFLASH
>  	#depends on FLASH_CFI_DRIVER
>  	bool "armflash"
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index c61d2b1..6a0b70f 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -20,6 +20,7 @@
>  #include <div64.h>
>  #include <memalign.h>
>  #include <linux/compat.h>
> +#include <linux/sizes.h>
>  
>  static LIST_HEAD(disk_partitions);
>  
> @@ -190,16 +191,33 @@ static struct disk_part *allocate_disk_part(disk_partition_t *info, int partnum)
>  	return newpart;
>  }
>  
> +static void prettyprint_part_size(char *sizestr, unsigned long partsize,
> +				  unsigned long blksize)
> +{
> +	unsigned long long partbytes;
> +	unsigned long partmegabytes;
> +
> +	partbytes = partsize * blksize;
> +	partmegabytes = lldiv(partbytes, SZ_1M);
> +	snprintf(sizestr, 16, "%luMiB", partmegabytes);
> +}
> +
>  static void print_gpt_info(void)
>  {
>  	struct list_head *pos;
>  	struct disk_part *curr;
> +	char partstartstr[16];
> +	char partsizestr[16];
>  
>  	list_for_each(pos, &disk_partitions) {
>  		curr = list_entry(pos, struct disk_part, list);
> +		prettyprint_part_size(partstartstr, (unsigned long)curr->gpt_part_info.start,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +		prettyprint_part_size(partsizestr, (unsigned long)curr->gpt_part_info.size,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +
>  		printf("Partition %d:\n", curr->partnum);
> -		printf("1st block %x, size %x\n", (unsigned)curr->gpt_part_info.start,
> -		       (unsigned)curr->gpt_part_info.size);
> +		printf("Start %s, size %s\n", partstartstr, partsizestr);
>  		printf("Block size %lu, name %s\n", curr->gpt_part_info.blksz,
>  		       curr->gpt_part_info.name);
>  		printf("Type %s, bootable %d\n", curr->gpt_part_info.type,
> @@ -211,6 +229,85 @@ static void print_gpt_info(void)
>  	}
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int calc_parts_list_len(int numparts)
> +{
> +	/*
> +	 * prefatory string:
> +	 * doc/README.GPT, suggests that
> +	 * int partlistlen = UUID_STR_LEN + 1 + strlen("partitions=uuid_disk=");
> +	 * is correct, but extract_val() expects "uuid_disk" first.
> +	 */
> +	int partlistlen = UUID_STR_LEN + 1 + strlen("uuid_disk=");
> +	/* for the comma */
> +	partlistlen++;
> +
> +	/* per-partition additions; numparts starts at 1, so this should be correct */
> +	partlistlen += numparts * (strlen("name=,") + PART_NAME_LEN + 1);
> +	/* 17 because partstr in create_gpt_partitions_list() is 16 chars */
> +	partlistlen += numparts * (strlen("start=MiB,") + 17);
> +	partlistlen += numparts * (strlen("size=MiB,") + 17);
> +	partlistlen += numparts * (strlen("uuid=;") + UUID_STR_LEN + 1);
> +	/* for the terminating null */
> +	partlistlen++;
> +	debug("Length of partitions_list is %d for %d partitions\n", partlistlen,
> +	       numparts);
> +	return partlistlen;
> +}
> +
> +/*
> + * create the string that upstream 'gpt write' command will accept as an
> + * argument
> + *
> + * From doc/README.gpt, Format of partitions layout:
> + *    "partitions=uuid_disk=...;name=u-boot,size=60MiB,uuid=...;
> + *	name=kernel,size=60MiB,uuid=...;"
> + * The fields 'name' and 'size' are mandatory for every partition.
> + * The field 'start' is optional. The fields 'uuid' and 'uuid_disk'
> + * are optional if CONFIG_RANDOM_UUID is enabled.
> + */
> +static int create_gpt_partitions_list(int numparts, const char *guid, char *partitions_list)
> +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +	char partstr[PART_NAME_LEN + 1];
> +
> +	if (!partitions_list)
> +		return -1;
> +
> +	/*
> +	 * README.gpt specifies starting with "partitions=" like so:
> +	 *      strcpy(partitions_list, "partitions=uuid_disk=");
> +	 * but that breaks extract_val, which doesn't skip over 'partitions='.
> +	 */
> +	strcpy(partitions_list, "uuid_disk=");
> +	strncat(partitions_list, guid, UUID_STR_LEN + 1);
> +	strcat(partitions_list, ";");
> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		strcat(partitions_list, "name=");
> +		strncat(partitions_list, (const char *)curr->gpt_part_info.name, PART_NAME_LEN + 1);
> +		strcat(partitions_list, ",start=");
> +		prettyprint_part_size(partstr, (unsigned long)curr->gpt_part_info.start,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +		/* one extra byte for NULL */
> +		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
> +		strcat(partitions_list, ",size=");
> +		/* lbaint_t is unsigned long, per include/ide.h */
> +		prettyprint_part_size(partstr, (unsigned long)curr->gpt_part_info.size,
> +				      (unsigned long) curr->gpt_part_info.blksz);
> +		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
> +
> +		strcat(partitions_list, ",uuid=");
> +		strncat(partitions_list, (const char *)curr->gpt_part_info.uuid,
> +			UUID_STR_LEN + 1);
> +		strcat(partitions_list, ";");
> +	}
> +	return 0;
> +}
> +#endif
> +
>  /*
>   * read partition info into disk_partitions list where
>   * it can be printed or modified
> @@ -222,8 +319,11 @@ static int get_gpt_info(struct blk_desc *dev_desc)
>  	disk_partition_t info;
>  	struct disk_part *new_disk_part;
>  
> -	if (disk_partitions.next == NULL)
> -		INIT_LIST_HEAD(&disk_partitions);
> +	/*
> +	 * Always re-read partition info from device, in case
> +	 * it has changed
> +	 */
> +	INIT_LIST_HEAD(&disk_partitions);
>  
>  	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>  		ret = part_get_info(dev_desc, p, &info);
> @@ -294,6 +394,8 @@ static int set_gpt_info(struct blk_desc *dev_desc,
>  		return -1;
>  
>  	str = strdup(str_part);
> +	if (str == NULL)
> +		return -ENOMEM;
>  
>  	/* extract disk guid */
>  	s = str;
> @@ -523,6 +625,86 @@ static int do_disk_guid(struct blk_desc *dev_desc, char * const namestr)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int do_flip_gpt_parts(struct blk_desc *dev_desc)
> +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +	disk_partition_t *new_partitions = NULL;
> +	char disk_guid[UUID_STR_LEN + 1];
> +	char *partitions_list, *str_disk_guid;
> +	u8 part_count = 0;
> +	int partlistlen, ret, numparts = 0;
> +
> +	ret = get_disk_guid(dev_desc, disk_guid);
> +	if (ret < 0)
> +		return ret;
>
This function should return either CMD_RET_FAILURE or CMD_RET_SUCCESS,
since the return value is passed on as the exit code of the calling CMD
handler function!

> +
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
see above.

> +	printf("Current partition table with %d partitions is:\n", numparts);
> +	print_gpt_info();
> +
> +	partlistlen = calc_parts_list_len(numparts);
> +	partitions_list = (char *)malloc(partlistlen);
> +	memset(partitions_list, '\0', partlistlen);
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *) disk_guid,
> +					 partitions_list);
> +	if (ret < 0)
> +		return ret;
see above.

> +	debug("OLD partitions_list is %s with %d chars\n", partitions_list, strlen(partitions_list));
> +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list, &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
see above.

> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		if (!strcmp((char *)curr->gpt_part_info.name, "backup_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name, "candidate_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "primary_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name, "backup_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "backup_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name, "candidate_rootfs");
> +		if (!strcmp((char *)curr->gpt_part_info.name, "primary_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name, "backup_rootfs");
> +	}
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *) disk_guid, partitions_list);
> +	if (ret < 0)
> +		return ret;
see above.

> +	debug("NEW partitions_list is %s with %d chars\n", partitions_list, strlen(partitions_list));
> +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list, &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
see above.

> +	debug("Writing new partition table\n");
> +	ret = gpt_restore(dev_desc, disk_guid, new_partitions, numparts);
> +	if (ret < 0) {
> +		printf("Writing new partition table failed\n");
> +		return ret;
see above.

> +	}
> +
> +	debug("Reading back new partition table\n");
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
see above.

> +	printf("new partition table with %d partitions is:\n", numparts);
> +	print_gpt_info();
> +
> +	del_gpt_info();
> +	free(partitions_list);
> +	free(str_disk_guid);
> +	free(new_partitions);
> +	return ret;
see above.

> +}
> +#endif
> +
>  /**
>   * do_gpt(): Perform GPT operations
>   *
> @@ -567,6 +749,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>  		return do_disk_guid(blk_dev_desc, varname);
>  	} else if (strcmp(argv[1], "read") == 0) {
>  		return do_get_gpt_info(blk_dev_desc);
> +#ifdef CONFIG_CMD_GPT_FLIP
> +	} else if (strcmp(argv[1], "flip") == 0) {
> +		return do_flip_gpt_parts(blk_dev_desc);
>
Either make do_flip_gpt_part() return CMD_RET_... as indicated above, or
check its return value here and convert it to CMD_RET_SUCCESS or
CMD_RET_FAILURE.

Also see my comment to your patch 4/6 concerning this code location.


Lothar Waßmann

Hi Alison,

> From: Alison Chaiken <alison@she-devel.com>
> 
> This patch provides support in u-boot for renaming GPT
> partitions.  The renaming is accomplished via a new 'gpt flip'
> command which is enabled via a CONFIG_CMD_GPT_FLIP option.
> 
> The concept for the bootloader state machine is the following:
> 
> -- u-boot renames ‘primary’ partitions as ‘candidate’ and tries
>    to boot them.
> -- Linux, at boot, will rename ‘candidate’ partitions as
>    ‘primary’.
> -- If u-boot sees a ‘candidate’ partition after a boot attempt,
>    it renames it failed’ and renames the ‘backup’ partition as
>    ‘candidate’.
> 
> Logic:
> -- Partitions can go to ‘failed’ only from ‘candidate’ and only
>    via u-boot.  Partitions can go to ‘backup’ only from ‘primary’
>    and vice-versa, only via Linux.  Partitions go to ‘candidate’
>    from ‘primary’ or ‘backup’ only via u-boot.  Only system
>    update software will rename 'failed' partitions.
> 
> Rewriting the partition table has the side-effect that all partitions
> end up with "msftdata" flag set.  The reason is that partition type
> PARTITION_BASIC_DATA_GUID is hard-coded in the gpt_fill_pte()
> function.  This does not appear to cause any harm.
> 
> Signed-off-by: Alison Chaiken <alison@peloton-tech.com>
> ---
>  cmd/Kconfig    |   7 ++
>  cmd/gpt.c      | 199
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> doc/README.gpt |  13 ++++ 3 files changed, 215 insertions(+), 4
> deletions(-)
> 
> diff --git a/cmd/Kconfig b/cmd/Kconfig
> index 5ee52f6..a8f7716 100644
> --- a/cmd/Kconfig
> +++ b/cmd/Kconfig
> @@ -575,6 +575,13 @@ config CMD_GPT
>  	  Enable the 'gpt' command to ready and write GPT style
> partition tables.
>  
> +config CMD_GPT_FLIP
> +	bool "GPT flip-partitions command"
> +	depends on CMD_GPT
> +	help
> +	  Enables the 'gpt' command to write modified GPT partition
> +	  tables via the 'gpt flip' command.
> +
>  config CMD_ARMFLASH
>  	#depends on FLASH_CFI_DRIVER
>  	bool "armflash"
> diff --git a/cmd/gpt.c b/cmd/gpt.c
> index c61d2b1..6a0b70f 100644
> --- a/cmd/gpt.c
> +++ b/cmd/gpt.c
> @@ -20,6 +20,7 @@
>  #include <div64.h>
>  #include <memalign.h>
>  #include <linux/compat.h>
> +#include <linux/sizes.h>
>  
>  static LIST_HEAD(disk_partitions);
>  
> @@ -190,16 +191,33 @@ static struct disk_part
> *allocate_disk_part(disk_partition_t *info, int partnum) return
> newpart; }
>  
> +static void prettyprint_part_size(char *sizestr, unsigned long
> partsize,
> +				  unsigned long blksize)
> +{
> +	unsigned long long partbytes;
> +	unsigned long partmegabytes;
> +
> +	partbytes = partsize * blksize;
> +	partmegabytes = lldiv(partbytes, SZ_1M);
> +	snprintf(sizestr, 16, "%luMiB", partmegabytes);
> +}
> +
>  static void print_gpt_info(void)
>  {
>  	struct list_head *pos;
>  	struct disk_part *curr;
> +	char partstartstr[16];
> +	char partsizestr[16];
>  
>  	list_for_each(pos, &disk_partitions) {
>  		curr = list_entry(pos, struct disk_part, list);
> +		prettyprint_part_size(partstartstr, (unsigned
> long)curr->gpt_part_info.start,
> +				      (unsigned long)
> curr->gpt_part_info.blksz);
> +		prettyprint_part_size(partsizestr, (unsigned
> long)curr->gpt_part_info.size,
> +				      (unsigned long)
> curr->gpt_part_info.blksz); +
>  		printf("Partition %d:\n", curr->partnum);
> -		printf("1st block %x, size %x\n",
> (unsigned)curr->gpt_part_info.start,
> -		       (unsigned)curr->gpt_part_info.size);
> +		printf("Start %s, size %s\n", partstartstr,
> partsizestr); printf("Block size %lu, name %s\n",
> curr->gpt_part_info.blksz, curr->gpt_part_info.name);
>  		printf("Type %s, bootable %d\n",
> curr->gpt_part_info.type, @@ -211,6 +229,85 @@ static void
> print_gpt_info(void) }
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int calc_parts_list_len(int numparts)
> +{
> +	/*
> +	 * prefatory string:
> +	 * doc/README.GPT, suggests that
> +	 * int partlistlen = UUID_STR_LEN + 1 +
> strlen("partitions=uuid_disk=");
> +	 * is correct, but extract_val() expects "uuid_disk" first.
> +	 */
> +	int partlistlen = UUID_STR_LEN + 1 + strlen("uuid_disk=");
> +	/* for the comma */
> +	partlistlen++;
> +
> +	/* per-partition additions; numparts starts at 1, so this
> should be correct */
> +	partlistlen += numparts * (strlen("name=,") + PART_NAME_LEN
> + 1);
> +	/* 17 because partstr in create_gpt_partitions_list() is 16
> chars */
> +	partlistlen += numparts * (strlen("start=MiB,") + 17);
> +	partlistlen += numparts * (strlen("size=MiB,") + 17);
> +	partlistlen += numparts * (strlen("uuid=;") + UUID_STR_LEN +
> 1);
> +	/* for the terminating null */
> +	partlistlen++;
> +	debug("Length of partitions_list is %d for %d partitions\n",
> partlistlen,
> +	       numparts);
> +	return partlistlen;
> +}
> +
> +/*
> + * create the string that upstream 'gpt write' command will accept
> as an
> + * argument
> + *
> + * From doc/README.gpt, Format of partitions layout:
> + *    "partitions=uuid_disk=...;name=u-boot,size=60MiB,uuid=...;
> + *	name=kernel,size=60MiB,uuid=...;"
> + * The fields 'name' and 'size' are mandatory for every partition.
> + * The field 'start' is optional. The fields 'uuid' and 'uuid_disk'
> + * are optional if CONFIG_RANDOM_UUID is enabled.
> + */
> +static int create_gpt_partitions_list(int numparts, const char
> *guid, char *partitions_list) +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +	char partstr[PART_NAME_LEN + 1];
> +
> +	if (!partitions_list)
> +		return -1;
> +
> +	/*
> +	 * README.gpt specifies starting with "partitions=" like so:
> +	 *      strcpy(partitions_list, "partitions=uuid_disk=");
> +	 * but that breaks extract_val, which doesn't skip over
> 'partitions='.
> +	 */
> +	strcpy(partitions_list, "uuid_disk=");
> +	strncat(partitions_list, guid, UUID_STR_LEN + 1);
> +	strcat(partitions_list, ";");
> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		strcat(partitions_list, "name=");
> +		strncat(partitions_list, (const char
> *)curr->gpt_part_info.name, PART_NAME_LEN + 1);
> +		strcat(partitions_list, ",start=");
> +		prettyprint_part_size(partstr, (unsigned
> long)curr->gpt_part_info.start,
> +				      (unsigned long)
> curr->gpt_part_info.blksz);
> +		/* one extra byte for NULL */
> +		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
> +		strcat(partitions_list, ",size=");
> +		/* lbaint_t is unsigned long, per include/ide.h */
> +		prettyprint_part_size(partstr, (unsigned
> long)curr->gpt_part_info.size,
> +				      (unsigned long)
> curr->gpt_part_info.blksz);
> +		strncat(partitions_list, partstr, PART_NAME_LEN + 1);
> +
> +		strcat(partitions_list, ",uuid=");
> +		strncat(partitions_list, (const char
> *)curr->gpt_part_info.uuid,
> +			UUID_STR_LEN + 1);
> +		strcat(partitions_list, ";");
> +	}
> +	return 0;
> +}
> +#endif
> +
>  /*
>   * read partition info into disk_partitions list where
>   * it can be printed or modified
> @@ -222,8 +319,11 @@ static int get_gpt_info(struct blk_desc
> *dev_desc) disk_partition_t info;
>  	struct disk_part *new_disk_part;
>  
> -	if (disk_partitions.next == NULL)
> -		INIT_LIST_HEAD(&disk_partitions);
> +	/*
> +	 * Always re-read partition info from device, in case
> +	 * it has changed
> +	 */
> +	INIT_LIST_HEAD(&disk_partitions);
>  
>  	for (p = 1; p <= MAX_SEARCH_PARTITIONS; p++) {
>  		ret = part_get_info(dev_desc, p, &info);
> @@ -294,6 +394,8 @@ static int set_gpt_info(struct blk_desc *dev_desc,
>  		return -1;
>  
>  	str = strdup(str_part);
> +	if (str == NULL)
> +		return -ENOMEM;
>  
>  	/* extract disk guid */
>  	s = str;
> @@ -523,6 +625,86 @@ static int do_disk_guid(struct blk_desc
> *dev_desc, char * const namestr) return 0;
>  }
>  
> +#ifdef CONFIG_CMD_GPT_FLIP
> +static int do_flip_gpt_parts(struct blk_desc *dev_desc)
> +{
> +	struct list_head *pos;
> +	struct disk_part *curr;
> +	disk_partition_t *new_partitions = NULL;
> +	char disk_guid[UUID_STR_LEN + 1];
> +	char *partitions_list, *str_disk_guid;
> +	u8 part_count = 0;
> +	int partlistlen, ret, numparts = 0;
> +
> +	ret = get_disk_guid(dev_desc, disk_guid);
> +	if (ret < 0)
> +		return ret;
> +
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
> +	printf("Current partition table with %d partitions is:\n",
> numparts);
> +	print_gpt_info();
> +
> +	partlistlen = calc_parts_list_len(numparts);
> +	partitions_list = (char *)malloc(partlistlen);
> +	memset(partitions_list, '\0', partlistlen);
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *)
> disk_guid,
> +					 partitions_list);
> +	if (ret < 0)
> +		return ret;
> +	debug("OLD partitions_list is %s with %d chars\n",
> partitions_list, strlen(partitions_list)); +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list,
> &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
> +
> +	list_for_each(pos, &disk_partitions) {
> +		curr = list_entry(pos, struct disk_part, list);
> +		if (!strcmp((char *)curr->gpt_part_info.name,
> "backup_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name,
> "candidate_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name,
> "primary_kernel"))
> +			strcpy((char *)curr->gpt_part_info.name,
> "backup_kernel");
> +		if (!strcmp((char *)curr->gpt_part_info.name,
> "backup_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name,
> "candidate_rootfs");
> +		if (!strcmp((char *)curr->gpt_part_info.name,
> "primary_rootfs"))
> +			strcpy((char *)curr->gpt_part_info.name,
> "backup_rootfs");
> +	}
> +
> +	ret = create_gpt_partitions_list(numparts, (const char *)
> disk_guid, partitions_list);
> +	if (ret < 0)
> +		return ret;
> +	debug("NEW partitions_list is %s with %d chars\n",
> partitions_list, strlen(partitions_list)); +
> +	ret = set_gpt_info(dev_desc, (const char *)partitions_list,
> &str_disk_guid,
> +			   &new_partitions, &part_count);
> +	if (ret < 0)
> +		return ret;
> +
> +	debug("Writing new partition table\n");
> +	ret = gpt_restore(dev_desc, disk_guid, new_partitions,
> numparts);
> +	if (ret < 0) {
> +		printf("Writing new partition table failed\n");
> +		return ret;
> +	}
> +
> +	debug("Reading back new partition table\n");
> +	numparts = get_gpt_info(dev_desc);
> +	if (numparts <  0)
> +		return numparts;
> +	printf("new partition table with %d partitions is:\n",
> numparts);
> +	print_gpt_info();
> +
> +	del_gpt_info();
> +	free(partitions_list);
> +	free(str_disk_guid);
> +	free(new_partitions);
> +	return ret;
> +}
> +#endif
> +
>  /**
>   * do_gpt(): Perform GPT operations
>   *
> @@ -567,6 +749,10 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag,
> int argc, char * const argv[]) return do_disk_guid(blk_dev_desc,
> varname); } else if (strcmp(argv[1], "read") == 0) {
>  		return do_get_gpt_info(blk_dev_desc);
> +#ifdef CONFIG_CMD_GPT_FLIP
> +	} else if (strcmp(argv[1], "flip") == 0) {
> +		return do_flip_gpt_parts(blk_dev_desc);
> +#endif
>  	} else {
>  		return CMD_RET_USAGE;
>  	}
> @@ -598,4 +784,9 @@ U_BOOT_CMD(gpt, CONFIG_SYS_MAXARGS, 1, do_gpt,
>  	" Example usage:\n"
>  	" gpt guid mmc 0\n"
>  	" gpt guid mmc 0 varname\n"
> +#ifdef CONFIG_CMD_GPT_FLIP
> +	"gpt partition-flip command\n"
> +	"gpt flip <interface> <dev>\n"
> +	"    - exchange device's 'primary' and 'backup' partition
> names\n" +#endif
>  );
> diff --git a/doc/README.gpt b/doc/README.gpt
> index c0779a4..e29b188 100644
> --- a/doc/README.gpt
> +++ b/doc/README.gpt
> @@ -210,6 +210,19 @@ Following line can be used to assess if GPT
> verification has succeed: U-BOOT> gpt verify mmc 0 $partitions
>  U-BOOT> if test $? = 0; then echo "GPT OK"; else echo "GPT ERR"; fi
>  
> +Renaming GPT partitions from U-Boot:
> +====================================
> +
> +GPT partition names are a mechanism via which userspace and U-Boot
> can +communicate about software updates and boot failure.  The 'gpt
> guid', +'gpt read' and 'gpt flip' commands facilitate programmatic
> renaming of +partitions from bootscripts by generating and modifying
> the partitions +layout string.  The code in gpt_flip() illustrates
> the case of +swapping 'primary' and 'backup' partitions via:
> +
> +U-BOOT> gpt flip mmc 0

Maybe it would be better to have 

gpt flip mmc 0 <optional parameter 'name'>

(By default we have "primary" and "backup")

In that way we could set other names to GPT partitions without the
need to modify the code.





And another request -> Could you consider adding tests for those new
gpt commands to the 'sandbox' (sandbox_defconfig) ?

Then you can 'mount' some gpt test image ('host' command) and use it
with:
gpt <command> host X .....


Despite above comments - you did a great job :-)

Reviewed-by: Lukasz Majewski <lukma@denx.de>

> +
> +Choose different partition names by modifying these strings in gpt.c.
>  
>  Partition type GUID:
>  ====================




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

On 2017-05-31 01:12, Lukasz Majewski wrote:

[ SNIP ]

> Maybe it would be better to have
> 
> gpt flip mmc 0 <optional parameter 'name'>
> 
> (By default we have "primary" and "backup")
> 
> In that way we could set other names to GPT partitions without the
> need to modify the code.

Another possibility is to support

     gpt flip mmc 0 <name 1> <name 2>

where two names are required, with defaults 'primary' and 'backup'.   
Perhaps I should rework patch 5 that way?  I considered this form for 
the original submission, but wasn't sure what other projects might find 
useful.

> And another request -> Could you consider adding tests for those new
> gpt commands to the 'sandbox' (sandbox_defconfig) ?

I will look into adding another patch to do that.   I'm sure such tests 
are a good idea for our project.

> Despite above comments - you did a great job :-)

Thanks.   I'm sorry to have omitted you, Lukasz, from the original 
submission, but I consulted an old version of MAINTAINERS from my 
original 2015.07.

-- Alison

---
Alison Chaiken                      alison@she-devel.com, 650-279-5600
http://{ she-devel.com, exerciseforthereader.org }
"We are giving up our privacy, one convenience at a time." -- Evangelos 
Simoudis

Hi Alison,

> On 2017-05-31 01:12, Lukasz Majewski wrote:
> 
> [ SNIP ]
> 
> > Maybe it would be better to have
> > 
> > gpt flip mmc 0 <optional parameter 'name'>
> > 
> > (By default we have "primary" and "backup")
> > 
> > In that way we could set other names to GPT partitions without the
> > need to modify the code.
> 
> Another possibility is to support
> 
>      gpt flip mmc 0 <name 1> <name 2>
> 
> where two names are required, with defaults 'primary' and 'backup'.   
> Perhaps I should rework patch 5 that way? 

Please correct my understanding if I'm wrong.

You parse GPT partitions to a list and there are only two names
possible:

"primary" and "backup".

When we call gpt filp we rename those names ("primary" -> "backup" and
the other way).

Ahhh..... so your patch would allow to rename "primary" to <name1> and
"backup" to <name2> for all names.

My idea was rather to have a gpt call to set name of a GPT partition....

> I considered this form for 
> the original submission, but wasn't sure what other projects might
> find useful.

I can image that somebody would like to change on the fly single GPT
partition name (when having/care about 2 partitions for his/her project
specific data).

But, I don't know how much effort it would take to implement it (and if
others would see it beneficial).

> 
> > And another request -> Could you consider adding tests for those new
> > gpt commands to the 'sandbox' (sandbox_defconfig) ?
> 
> I will look into adding another patch to do that.   I'm sure such
> tests are a good idea for our project.

It is relatively easy to use sandbox to test some high level stuff
(like file systems, gpt, etc). One don't need HW for it.

> 
> > Despite above comments - you did a great job :-)
> 
> Thanks.   I'm sorry to have omitted you, Lukasz, from the original 
> submission, but I consulted an old version of MAINTAINERS from my 
> original 2015.07.

It's Ok, don't worry. Things like that do happen....

> 
> -- Alison
> 
> ---
> Alison Chaiken                      alison@she-devel.com, 650-279-5600
> http://{ she-devel.com, exerciseforthereader.org }
> "We are giving up our privacy, one convenience at a time." --
> Evangelos Simoudis




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

Lukasz Majewski wrote:

> Maybe it would be better to have
> 
> gpt flip mmc 0 <optional parameter 'name'>
> 
> (By default we have "primary" and "backup")
> 
> In that way we could set other names to GPT partitions without the
> need to modify the code.

I answered: 

>> Another possibility is to support
>> 
>> gpt flip mmc 0 <name 1> <name 2>
>> 
>> where two names are required, with defaults 'primary' and 'backup'.   
>> Perhaps I should rework patch 5 that way?

Lukasz responded: 

> Please correct my understanding if I'm wrong.
> 
> You parse GPT partitions to a list and there are only two names
> possible:
> 
> "primary" and "backup".
> 
> When we call gpt filp we rename those names ("primary" -> "backup" and
> the other way).
> 
> Ahhh..... so your patch would allow to rename "primary" to <name1> and
> "backup" to <name2> for all names.
> 
> My idea was rather to have a gpt call to set name of a GPT partition....

> I can image that somebody would like to change on the fly single GPT
> partition name (when having/care about 2 partitions for his/her project
> specific data).
> 
> But, I don't know how much effort it would take to implement it (and if
> others would see it beneficial).

I would propose then two options: 

    gpt rename mmc 0 <name 1> <name 2> 

which renames all the 1's to 2's, and 

    gpt flip mmc 0 <name 1> <name 2> 

which swaps the two name strings for all partitions where they're found.
 These two operations together with 'gpt write' then cover all the
common use cases I can imagine. 

I'm open to any other suggestions, of course.   I just posted what I
already had to get started. 

-- Alison 

---
Alison Chaiken                      alison@she-devel.com, 650-279-5600 
http://{ she-devel.com, exerciseforthereader.org } 
"We are giving up our privacy, one convenience at a time." -- Evangelos
Simoudis

Hi Alison,

> Lukasz Majewski wrote:
> 
> > Maybe it would be better to have
> > 
> > gpt flip mmc 0 <optional parameter 'name'>
> > 
> > (By default we have "primary" and "backup")
> > 
> > In that way we could set other names to GPT partitions without the
> > need to modify the code.
> 
> I answered: 
> 
> >> Another possibility is to support
> >> 
> >> gpt flip mmc 0 <name 1> <name 2>
> >> 
> >> where two names are required, with defaults 'primary' and
> >> 'backup'. Perhaps I should rework patch 5 that way?
> 
> Lukasz responded: 
> 
> > Please correct my understanding if I'm wrong.
> > 
> > You parse GPT partitions to a list and there are only two names
> > possible:
> > 
> > "primary" and "backup".
> > 
> > When we call gpt filp we rename those names ("primary" -> "backup"
> > and the other way).
> > 
> > Ahhh..... so your patch would allow to rename "primary" to <name1>
> > and "backup" to <name2> for all names.
> > 
> > My idea was rather to have a gpt call to set name of a GPT
> > partition....
> 
> > I can image that somebody would like to change on the fly single GPT
> > partition name (when having/care about 2 partitions for his/her
> > project specific data).
> > 
> > But, I don't know how much effort it would take to implement it
> > (and if others would see it beneficial).
> 
> I would propose then two options: 
> 
>     gpt rename mmc 0 <name 1> <name 2> 
> 
> which renames all the 1's to 2's, and 
> 
>     gpt flip mmc 0 <name 1> <name 2> 
> 
> which swaps the two name strings for all partitions where they're
> found. These two operations together with 'gpt write' then cover all
> the common use cases I can imagine. 

I think that this is enough. Let's wait for other's opinions (if
any).

> 
> I'm open to any other suggestions, of course.   I just posted what I
> already had to get started. 
> 
> -- Alison 
> 
> ---
> Alison Chaiken                      alison@she-devel.com,
> 650-279-5600 http://{ she-devel.com, exerciseforthereader.org } 
> "We are giving up our privacy, one convenience at a time." --
> Evangelos Simoudis



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de

From: Alison Chaiken <alison@peloton-tech.com>

One way for userspace and the bootloader to exchange information about
dynamic image selection is via the storage device partition table, as
described at

https://source.android.com/devices/tech/ota/ab_updates

The scheme described there relies on setting partitions' "boot" flag.
When no partition on a device is bootable since the kernel and U-Boot
are stored elsewhere, the name field in the GPT partition table offers
another logical place to store information.  These patches allow users
to easily modify GPT partition names via bootscripts that can select
different images based on a boot-failure counter, or when userspace
installs a software update.

These patches have been tested in the u-boot sandbox and (re)tested on
a TI DRA7xx-based SOM with U-Boot 2015.07.

Significant changes since v3:
-- Testing in sandbox showed error in NULL termination of UUID string
   in cmd/gpt.c.  That problem is fixed in
   0001-GPT-read-partition-table-from-device-into-a-data-str.patch.
-- Patch 3 "GPT: fix error in partitions string doc" now only treats
   the topic in the commit title, as the part dealing with the
   proposed 'flip' feature is moved to that patch.
-- Two new commits come from testing in the sandbox (of which I was
   previously unaware).   One fixes an error in README.sandbox, and
   one adds a few comments about sandbox testing to README.gpt.

Significant changes since v2:
-- Got rid of the need to allocate memory for the GUID string in
   do_gpt();
-- Fixed the problems with string NULL termination in
   allocate_disk_part();
-- Removed duplicate definition of MAX_SEARCH_PARTITIONS from
   disk/part.c and increased the value in include/part.h to 64;
-- Improved the commit message for "rename GPT partitions to detect
   boot failure" to better describe the version of the patch I
   submitted;
-- Fixed numerous small problems with function return values.

Significant changes since v1:
-- Put the gpt_flip() function and auxiliary ones inside a new
   CONFIG_CMD_GPT_FLIP option that depends on CMD_GPT.
-- Replace intentional overwriting of name and type string arrays with
   memset() instead.
-- Move part.h changes earlier in the patchset.
-- Add a few lines to README.gpt about the new gpt subcommands.

Added a few simple patches to do the following:
-- Replace remaining occurrences of '37' with UUID_STR_LEN+1;
-- Introduce new macros to get rid of the similar '32';
-- fix a smaller error in doc/README.gpt.

There are also some fixups of whitespace and formatting errors (plus
usual inevitable addition of new ones).

To do in future:
-- Add support for preserving the type flag for partitions. The u-boot
   version on which this patchset is based did not have this feature,
   and it's easy to add, but I need to figure how to test it first.

-- Add tests for the new gpt commands to the sandbox.

Alison Chaiken (5):
  GPT: read partition table from device into a data structure
  rename GPT partitions to detect boot failure
  GPT: fix error in partitions string doc
  sandbox: README: fix partition command invocation
  cmd gpt: test in sandbox

 board/sandbox/README.sandbox |   2 +-
 cmd/Kconfig                  |   7 +
 cmd/gpt.c                    | 298 +++++++++++++++++++++++++++++++++++++++++++
 doc/README.gpt               |  31 ++++-
 include/part.h               |   7 +
 5 files changed, 340 insertions(+), 5 deletions(-)

On 2017-05-31 01:12, Lukasz Majewski wrote:

[ . . . ]

> And another request -> Could you consider adding tests for those new
> gpt commands to the 'sandbox' (sandbox_defconfig) ?
> 
> Then you can 'mount' some gpt test image ('host' command) and use it
> with:
> gpt <command> host X .....

The GPT functionality really cannot be tested without a block device.   
We could build the disk.raw described in board/sandbox/README.sandbox 
when (CONFIG_UNIT_TEST && CONFIG_CMD_GPT_RENAME) in imitation of the 
file helloworld.efi.   There would be no reason for the size to be as 
large as 1200M, of course.   256KB appears to be the smallest usable 
size for 4 partitions (although trying to partition a 16 KB raw object 
with gdisk produces entertaining results).

To create this object for testing, we could use a subdirectory like 
lib/efi_loader, but maybe doing so in test/ makes more sense?  If the 
build system did create the object, then we could modify 
board/sandbox/README.sandbox to so indicate.   Then the test should go 
in test/py/tests presumably.

Thanks for feedback,
Alison

---
Alison Chaiken                      alison@she-devel.com
http://{ she-devel.com, exerciseforthereader.org }
"We are giving up our privacy, one convenience at a time." -- Evangelos 
Simoudis

On 08/28/2017 01:02 AM, Chaiken, Alison wrote:
> On 2017-05-31 01:12, Lukasz Majewski wrote:
>
> [ . . . ]
>
>> And another request -> Could you consider adding tests for those new
>> gpt commands to the 'sandbox' (sandbox_defconfig) ?
>>
>> Then you can 'mount' some gpt test image ('host' command) and use it
>> with:
>> gpt <command> host X .....
>
> The GPT functionality really cannot be tested without a block device.
> We could build the disk.raw described in board/sandbox/README.sandbox
> when (CONFIG_UNIT_TEST && CONFIG_CMD_GPT_RENAME) in imitation of the
> file helloworld.efi.   There would be no reason for the size to be as
> large as 1200M, of course.   256KB appears to be the smallest usable
> size for 4 partitions (although trying to partition a 16 KB raw object
> with gdisk produces entertaining results).
>
> To create this object for testing, we could use a subdirectory like
> lib/efi_loader, but maybe doing so in test/ makes more sense?  If the
> build system did create the object, then we could modify
> board/sandbox/README.sandbox to so indicate.   Then the test should go
> in test/py/tests presumably.

If you have encountered some issues with adding this test to sandbox 
(and those are difficult to solve), then it would be also correct to add 
this test to test/py and run it on a real HW.

Please also keep in mind to provide some kind of verbose in-code 
description or proper documentation entry.

>
> Thanks for feedback,
> Alison
>
> ---
> Alison Chaiken                      alison@she-devel.com
> http://{ she-devel.com, exerciseforthereader.org }
> "We are giving up our privacy, one convenience at a time." -- Evangelos
> Simoudis
>

On Mon, Aug 28, 2017 at 09:54:38AM +0200, Łukasz Majewski wrote:
> On 08/28/2017 01:02 AM, Chaiken, Alison wrote:
> >On 2017-05-31 01:12, Lukasz Majewski wrote:
> >
> >[ . . . ]
> >
> >>And another request -> Could you consider adding tests for those new
> >>gpt commands to the 'sandbox' (sandbox_defconfig) ?
> >>
> >>Then you can 'mount' some gpt test image ('host' command) and use it
> >>with:
> >>gpt <command> host X .....
> >
> >The GPT functionality really cannot be tested without a block device.
> >We could build the disk.raw described in board/sandbox/README.sandbox
> >when (CONFIG_UNIT_TEST && CONFIG_CMD_GPT_RENAME) in imitation of the
> >file helloworld.efi.   There would be no reason for the size to be as
> >large as 1200M, of course.   256KB appears to be the smallest usable
> >size for 4 partitions (although trying to partition a 16 KB raw object
> >with gdisk produces entertaining results).
> >
> >To create this object for testing, we could use a subdirectory like
> >lib/efi_loader, but maybe doing so in test/ makes more sense?  If the
> >build system did create the object, then we could modify
> >board/sandbox/README.sandbox to so indicate.   Then the test should go
> >in test/py/tests presumably.
> 
> If you have encountered some issues with adding this test to sandbox
> (and those are difficult to solve), then it would be also correct to
> add this test to test/py and run it on a real HW.

Right.  Everyone can run test.py on sandbox _and_ QEMU, and there are
then some people with labs setup that also run test.py on real hardware.
Between QEMU and real hardware we should be able to add and run tests
for this.  Thanks!