Patchwork [U-Boot,5/6] gpt: Support for new "gpt" command

login
register
mail settings
Submitter Łukasz Majewski
Date Aug. 24, 2012, 8:13 a.m.
Message ID <1345795995-24656-6-git-send-email-l.majewski@samsung.com>
Download mbox | patch
Permalink /patch/179787/
State Superseded
Delegated to: Tom Rini
Headers show

Comments

Łukasz Majewski - Aug. 24, 2012, 8:13 a.m.
New command - "gpt" is now supported. It shows and restores the GPT partition
table.
It looks into the "partitions" environment variable for partitions definition.
It can be enabled at target configuration file with CONFIG_CMD_GPT.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 common/Makefile  |    1 +
 common/cmd_gpt.c |  182 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 183 insertions(+), 0 deletions(-)
 create mode 100644 common/cmd_gpt.c
Stephen Warren - Sept. 5, 2012, 8:08 p.m.
On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> New command - "gpt" is now supported. It shows and restores the GPT partition
> table.
> It looks into the "partitions" environment variable for partitions definition.
> It can be enabled at target configuration file with CONFIG_CMD_GPT.

> diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c

> +int set_gpt_info(block_dev_desc_t *dev_desc)
> +{
> +	char *ps[GPT_PARTS_NUM], *name[GPT_PARTS_NUM];
> +	unsigned int size[GPT_PARTS_NUM];
> +	char *tok, *t, *p, *s, *ss;
> +	int i, ret;
> +
> +	s = getenv("partitions");
> +	if (s == NULL) {
> +		printf("%s: \"partitions\" env variable not defined!\n",
> +		       __func__);
> +		return -1;
> +	}

It'd be nice to be able to pass the partition definition on the
command-line instead of (or perhaps as an alternative to) reading an
environment variable.

Some documentation of the expected format of the partitions variable
would be useful. From the following patch, the format appears to be:

8M(csa-mmc),60M(u-boot),60M(kernel),...

That's not particularly extensible (think about allowing partition type
UUID to or attributes to be specified), and the brackets are a bit
painful. Can we use key/value for defining the values, and have a simple
separate separator between fields and partitions, e.g. something like:

size=8M,name=csa-mmc;size=60M,name=u-boot;size=60M,name=kernel;...

That would allow us to very easily allow new fields to be specified per
partition in the future, e.g.:

uuid=XXXXX,size=512M,name=boot,type=21686148-6449-6E6F-744E-656564454649,attrs=2;uuid=YYYYY,size=7000M,name=root,type=0FC63DAF-8483-4772-8E79-3D69D8477DE4,attrs=0;...

> +U_BOOT_CMD(
> +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> +	"GUID Partition Table",
...
> +	"gpt restore - reset GPT partition to defaults\n"
> +	"gpt dev #num - set device number\n"
...
> +static void set_gpt_dev(int dev)
> +{
> +	gpt_dev = dev;
> +}
> +

Hmmm. I think it'd be better to specify the device each time the gpt
command was invoked. That would be simpler to use, more flexible, and
more consistent with how other commands such as "ext2load" operate.

In other words, I would get rid of "gpt dev" completely, and instead of
implementing "gpt restore", implement "gpt restore mmc 0".

I'm not sure "restore" is the correct name, given that the command can
write arbitrary new partition layouts, rather than just restoring some
specific hard-coded table from e.g. a backup image.

I'm not sure that "gpt" is even the best command name; it'd be nice if
this were generic and could be extended to work with e.g. FAT in the
future - something like:

part write usb 1 gpt uuid=XXXXX,size=512M,name=boot,...
part write mmc 0 fat size=512M,attrs=0x80;...

> +U_BOOT_CMD(
> +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> +	"GUID Partition Table",
> +	"show - show GPT\n"

s/show/gpt show/

> +static void gpt_show(void)
> +{
> +	struct mmc *mmc = find_mmc_device(gpt_dev);
> +
> +	print_part_efi(&mmc->block_dev);
> +}

Do we really need another way of showing partition tables; "mmc part",
"usb part", ... already exist. I think if we want another way, it'd be
better to add this functionality to my proposed "part" command, i.e.
"part show mmc 0".

> +static int gpt_default(void)
> +{
> +	struct mmc *mmc = find_mmc_device(gpt_dev);
> +
> +	if (mmc == NULL) {
> +		printf("%s: mmc dev %d NOT available\n", __func__, gpt_dev);
> +		return CMD_RET_FAILURE;
> +	}

Why only allow mmc devices; what about USB for example? Other commands
such as ext2load allow arbitrary device types to be used. Rob Herring
recently posted some patches to unify how commands such as ext2load,
ext2ls, fatload, fatls, ... all obtain a device/partition handle from
their command-line - this patch should probably build on top of those
patches.

> +	puts("Using default GPT UUID\n");
> +
> +	return set_gpt_info(&mmc->block_dev);
> +}
Łukasz Majewski - Sept. 6, 2012, 2:01 p.m.
Hi Stephen,

> On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> > New command - "gpt" is now supported. It shows and restores the GPT
> > partition table.
> > It looks into the "partitions" environment variable for partitions
> > definition. It can be enabled at target configuration file with
> > CONFIG_CMD_GPT.
> 
> > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
> 
> > +int set_gpt_info(block_dev_desc_t *dev_desc)
> > +{
> > +	char *ps[GPT_PARTS_NUM], *name[GPT_PARTS_NUM];
> > +	unsigned int size[GPT_PARTS_NUM];
> > +	char *tok, *t, *p, *s, *ss;
> > +	int i, ret;
> > +
> > +	s = getenv("partitions");
> > +	if (s == NULL) {
> > +		printf("%s: \"partitions\" env variable not
> > defined!\n",
> > +		       __func__);
> > +		return -1;
> > +	}
> 
> It'd be nice to be able to pass the partition definition on the
> command-line instead of (or perhaps as an alternative to) reading an
> environment variable.
> 
> Some documentation of the expected format of the partitions variable
> would be useful. From the following patch, the format appears to be:
> 
> 8M(csa-mmc),60M(u-boot),60M(kernel),...
> 
> That's not particularly extensible (think about allowing partition
> type UUID to or attributes to be specified), and the brackets are a
> bit painful. Can we use key/value for defining the values, and have a
> simple separate separator between fields and partitions, e.g.
> something like:
> 
> size=8M,name=csa-mmc;size=60M,name=u-boot;size=60M,name=kernel;...
This approach looks reasonable, but...

> 
> That would allow us to very easily allow new fields to be specified
> per partition in the future, e.g.:
> 
> uuid=XXXXX,size=512M,name=boot,type=21686148-6449-6E6F-744E-656564454649,attrs=2;uuid=YYYYY,size=7000M,name=root,type=0FC63DAF-8483-4772-8E79-3D69D8477DE4,attrs=0;...

it would be a pain to paste so long string to u-boot command line.
Suppose we have 8 partitions to define. Then the definition string grows
considerable.

For default restoration, it would be good to define the environment
variable (like "partitions") to perform quick default restoration.

However there is a problem with passing UUID - size, name and
attributes can be defined at ./include/config/{board}.h
(partitions=size=8M,name=csa-mmc;size=60M,name=u-boot;size=60M,name=kernel)
, but UUID shall be passed by user via command line or generated
internally at u-boot (as it is proposed in this patch).

When passing from user it would look like:

gpt default uuid1 uuid2 .... uuid8

But we shall also think about using guid_gen() function (reimplemented)
for handy and quick gpt restoration.


> 
> > +U_BOOT_CMD(
> > +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> > +	"GUID Partition Table",
> ...
> > +	"gpt restore - reset GPT partition to defaults\n"
> > +	"gpt dev #num - set device number\n"
> ...
> > +static void set_gpt_dev(int dev)
> > +{
> > +	gpt_dev = dev;
> > +}
> > +
> 
> Hmmm. I think it'd be better to specify the device each time the gpt
> command was invoked. That would be simpler to use, more flexible, and
> more consistent with how other commands such as "ext2load" operate.
> 
> In other words, I would get rid of "gpt dev" completely, and instead
> of implementing "gpt restore", implement "gpt restore mmc 0".

Agree - the same approach is in dfu.

> 
> I'm not sure "restore" is the correct name, given that the command can
> write arbitrary new partition layouts, rather than just restoring some
> specific hard-coded table from e.g. a backup image.
> 
> I'm not sure that "gpt" is even the best command name; it'd be nice if
> this were generic and could be extended to work with e.g. FAT in the
> future - something like:
> 
> part write usb 1 gpt uuid=XXXXX,size=512M,name=boot,...
> part write mmc 0 fat size=512M,attrs=0x80;...

I'm open for discussion. Since part command is not yet finished, I will
use the gpt command for version 2 of this patch series. When we agree
with rest of GPT implementation we can decide if gpt or part command
will be used. OK?
> 
> > +U_BOOT_CMD(
> > +	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
> > +	"GUID Partition Table",
> > +	"show - show GPT\n"
> 
> s/show/gpt show/
> 
> > +static void gpt_show(void)
> > +{
> > +	struct mmc *mmc = find_mmc_device(gpt_dev);
> > +
> > +	print_part_efi(&mmc->block_dev);
> > +}
> 
> Do we really need another way of showing partition tables; "mmc part",
> "usb part", ... already exist. I think if we want another way, it'd be
> better to add this functionality to my proposed "part" command, i.e.
> "part show mmc 0".

I agree that "gpt show" is not needed. Instead "mmc part" can be used.
> 
> > +static int gpt_default(void)
> > +{
> > +	struct mmc *mmc = find_mmc_device(gpt_dev);
> > +
> > +	if (mmc == NULL) {
> > +		printf("%s: mmc dev %d NOT available\n", __func__,
> > gpt_dev);
> > +		return CMD_RET_FAILURE;
> > +	}
> 
> Why only allow mmc devices; what about USB for example? Other commands
> such as ext2load allow arbitrary device types to be used. Rob Herring
> recently posted some patches to unify how commands such as ext2load,
> ext2ls, fatload, fatls, ... all obtain a device/partition handle from
> their command-line - this patch should probably build on top of those
> patches.

Yes, but Rob's patches are not yet accepted to mainline. I'd prefer to
not use patches which aren't accepted as a base for development. 

> 
> > +	puts("Using default GPT UUID\n");
> > +
> > +	return set_gpt_info(&mmc->block_dev);
> > +}
> 


I think, that most important change is to pass table with pointers to
partition entries to set_gpt_table. Second thing is to fix guid_gen()
function and change the format of "partitions" to be key=value pairs.

Patch

diff --git a/common/Makefile b/common/Makefile
index 49df751..438d36c 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -185,6 +185,7 @@  COBJS-$(CONFIG_MODEM_SUPPORT) += modem.o
 COBJS-$(CONFIG_UPDATE_TFTP) += update.o
 COBJS-$(CONFIG_USB_KEYBOARD) += usb_kbd.o
 COBJS-$(CONFIG_CMD_DFU) += cmd_dfu.o
+COBJS-$(CONFIG_CMD_GPT) += cmd_gpt.o
 endif
 
 ifdef CONFIG_SPL_BUILD
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c
new file mode 100644
index 0000000..06d7c5f
--- /dev/null
+++ b/common/cmd_gpt.c
@@ -0,0 +1,182 @@ 
+/*
+ * cmd_gpt.c -- GPT (GUID Partition Table) handling command
+ *
+ * Copyright (C) 2012 Samsung Electronics
+ * author: Lukasz Majewski <l.majewski@samsung.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#include <common.h>
+#include <malloc.h>
+#include <command.h>
+#include <mmc.h>
+
+static unsigned int gpt_parts;
+static int gpt_dev;
+
+static unsigned long memsize_to_blocks(const char *const ptr,
+				       const char **retptr)
+{
+	unsigned long ret = simple_strtoul(ptr, (char **)retptr, 0);
+	int shift = 0;
+
+	switch (**retptr) {
+	case 'G':
+	case 'g':
+		shift += 10;
+	case 'M':
+	case 'm':
+		shift += 10;
+	case 'K':
+	case 'k':
+		shift += 10;
+	(*retptr)++;
+	default:
+		shift -= 9;
+		break;
+	}
+
+	if (shift > 0)
+		ret <<= shift;
+	else
+		ret >>= shift;
+
+	return ret;
+}
+
+int set_gpt_info(block_dev_desc_t *dev_desc)
+{
+	char *ps[GPT_PARTS_NUM], *name[GPT_PARTS_NUM];
+	unsigned int size[GPT_PARTS_NUM];
+	char *tok, *t, *p, *s, *ss;
+	int i, ret;
+
+	s = getenv("partitions");
+	if (s == NULL) {
+		printf("%s: \"partitions\" env variable not defined!\n",
+		       __func__);
+		return -1;
+	}
+
+	ss = calloc(strlen(s) + 1, 1);
+	if (ss == NULL) {
+		printf("%s: calloc failed!\n", __func__);
+		return -1;
+	}
+	memcpy(ss, s, strlen(s) + 1);
+
+	for (i = 0, p = ss; ; i++) {
+		tok = strsep(&p, ",");
+		if (tok == NULL)
+			break;
+
+		t = strsep(&tok, "(");
+		ps[i] = calloc(strlen(t) + 1, 1);
+		if (ps[i] == NULL) {
+			printf("%s: calloc failed!\n", __func__);
+			ret = -1;
+			goto err;
+		}
+		strcpy(ps[i], t);
+
+		t = strsep(&tok, ")");
+		name[i] = calloc(strlen(t) + 1, 1);
+		if (name[i] == NULL) {
+			printf("%s: calloc failed!\n", __func__);
+			ret = -1;
+			goto err;
+		}
+		strcpy(name[i], t);
+	}
+
+	gpt_parts = i;
+	printf("found %d partitions\n", gpt_parts);
+
+	for (i = 0; i < gpt_parts; i++) {
+		p = ps[i];
+		size[i] = memsize_to_blocks(p, (const char **)&p);
+	}
+
+	puts("save the GPT Table...\n");
+	ret = set_gpt_table(dev_desc, gpt_parts, size, name);
+
+	i = gpt_parts;
+ err:
+	for (i--; i >= 0; i--) {
+		free(name[i]);
+		free(ps[i]);
+	}
+
+	free(ss);
+	return ret;
+}
+
+static void set_gpt_dev(int dev)
+{
+	gpt_dev = dev;
+}
+
+static void gpt_show(void)
+{
+	struct mmc *mmc = find_mmc_device(gpt_dev);
+
+	print_part_efi(&mmc->block_dev);
+}
+
+static int gpt_default(void)
+{
+	struct mmc *mmc = find_mmc_device(gpt_dev);
+
+	if (mmc == NULL) {
+		printf("%s: mmc dev %d NOT available\n", __func__, gpt_dev);
+		return CMD_RET_FAILURE;
+	}
+
+	puts("Using default GPT UUID\n");
+
+	return set_gpt_info(&mmc->block_dev);
+}
+
+static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	int ret = CMD_RET_SUCCESS;
+
+	switch (argc) {
+	case 2:
+		if (strncmp(argv[1], "show", 4) == 0)
+			gpt_show();
+		else if (strncmp(argv[1], "restore", 7) == 0)
+			if (gpt_default())
+				return CMD_RET_FAILURE;
+		break;
+	case 3:
+		if (strncmp(argv[1], "dev", 3) == 0)
+			set_gpt_dev(simple_strtoul(argv[2], NULL, 10));
+		break;
+	default:
+		ret = CMD_RET_USAGE;
+	}
+
+	return ret;
+}
+
+U_BOOT_CMD(
+	gpt,	CONFIG_SYS_MAXARGS,	1, do_gpt,
+	"GUID Partition Table",
+	"show - show GPT\n"
+	"gpt restore - reset GPT partition to defaults\n"
+	"gpt dev #num - set device number\n"
+);