[U-Boot] [PATCH 1/2] disk: efi: unify code for finding a valid gpt

[U-Boot] [PATCH 1/2] disk: efi: unify code for finding a valid gpt

[Urja Rannikko]

There were 3 copies of the same sequence, make it into a function.

Signed-off-by: Urja Rannikko <urjaman@gmail.com>
---
 disk/part_efi.c | 73 +++++++++++++++++++++++--------------------------
 1 file changed, 34 insertions(+), 39 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 239455b816..5f935da4c2 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -51,6 +51,8 @@ static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 static gpt_entry *alloc_read_gpt_entries(struct blk_desc *dev_desc,
 					 gpt_header *pgpt_head);
 static int is_pte_valid(gpt_entry * pte);
+static int find_valid_gpt(struct blk_desc *dev_desc, gpt_header *gpt_head,
+			  gpt_entry **pgpt_pte);
 
 static char *print_efiname(gpt_entry *pte)
 {
@@ -192,19 +194,8 @@ int get_disk_guid(struct blk_desc * dev_desc, char *guid)
 	unsigned char *guid_bin;
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 gpt_head, &gpt_pte) != 1) {
-		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
-		if (is_gpt_valid(dev_desc, dev_desc->lba - 1,
-				 gpt_head, &gpt_pte) != 1) {
-			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
-			       __func__);
-			return -EINVAL;
-		} else {
-			printf("%s: ***        Using Backup GPT ***\n",
-			       __func__);
-		}
-	}
+	if (find_valid_gpt(dev_desc, gpt_head, &gpt_pte) != 1)
+		return -EINVAL;
 
 	guid_bin = gpt_head->disk_guid.b;
 	uuid_bin_to_str(guid_bin, guid, UUID_STR_FORMAT_GUID);
@@ -221,19 +212,8 @@ void part_print_efi(struct blk_desc *dev_desc)
 	unsigned char *uuid_bin;
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 gpt_head, &gpt_pte) != 1) {
-		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
-		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
-				 gpt_head, &gpt_pte) != 1) {
-			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
-			       __func__);
-			return;
-		} else {
-			printf("%s: ***        Using Backup GPT ***\n",
-			       __func__);
-		}
-	}
+	if (find_valid_gpt(dev_desc, gpt_head, &gpt_pte) != 1)
+		return;
 
 	debug("%s: gpt-entry at %p\n", __func__, gpt_pte);
 
@@ -282,19 +262,8 @@ int part_get_info_efi(struct blk_desc *dev_desc, int part,
 	}
 
 	/* This function validates AND fills in the GPT header and PTE */
-	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			gpt_head, &gpt_pte) != 1) {
-		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
-		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
-				 gpt_head, &gpt_pte) != 1) {
-			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
-			       __func__);
-			return -1;
-		} else {
-			printf("%s: ***        Using Backup GPT ***\n",
-			       __func__);
-		}
-	}
+	if (find_valid_gpt(dev_desc, gpt_head, &gpt_pte) != 1)
+		return -1;
 
 	if (part > le32_to_cpu(gpt_head->num_partition_entries) ||
 	    !is_pte_valid(&gpt_pte[part - 1])) {
@@ -981,6 +950,32 @@ static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 	return 1;
 }
 
+/**
+ * find_valid_gpt() - finds a valid GPT header and PTEs
+ *
+ * gpt is a GPT header ptr, filled on return.
+ * ptes is a PTEs ptr, filled on return.
+ *
+ * Description: returns 1 if found a valid gpt,  0 on error.
+ * If valid, returns pointers to PTEs.
+ */
+static int find_valid_gpt(struct blk_desc *dev_desc, gpt_header *gpt_head,
+			  gpt_entry **pgpt_pte)
+{
+	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
+			 gpt_head, pgpt_pte) != 1) {
+		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
+		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
+				 gpt_head, pgpt_pte) != 1) {
+			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
+			       __func__);
+			return 0;
+		}
+		printf("%s: ***        Using Backup GPT ***\n",  __func__);
+	}
+	return 1;
+}
+
 /**
  * alloc_read_gpt_entries(): reads partition entries from disk
  * @dev_desc
-- 
2.21.0

[U-Boot] [PATCH 2/2] disk: efi: ignore 'IGNOREME' GPT header found on cros eMMCs

[Urja Rannikko]

Some ChromeOS devices (atleast veyron speedy) have the first 8MiB of
the eMMC write protected and equipped with a dummy 'IGNOREME' GPT
header - instead of spewing error messages about it, just silently
try the backup GPT.

Note: this does not touch the gpt cmd writing/verifying functions,
those will still complain.

Signed-off-by: Urja Rannikko <urjaman@gmail.com>
---
 disk/part_efi.c    | 28 +++++++++++++++++++++-------
 include/part_efi.h |  2 ++
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 5f935da4c2..83ba7db665 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -893,7 +893,7 @@ static int is_pmbr_valid(legacy_mbr * mbr)
  * gpt is a GPT header ptr, filled on return.
  * ptes is a PTEs ptr, filled on return.
  *
- * Description: returns 1 if valid,  0 on error.
+ * Description: returns 1 if valid,  0 on error, 2 if ignored header
  * If valid, returns pointers to PTEs.
  */
 static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
@@ -919,6 +919,12 @@ static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 		return 0;
 	}
 
+	/* Invalid but nothing to yell about. */
+	if (le64_to_cpu(pgpt_head->signature) == GPT_HEADER_CHROMEOS_IGNORE) {
+		debug("ChromeOS 'IGNOREME' GPT header found and ignored\n");
+		return 2;
+	}
+
 	if (validate_gpt_header(pgpt_head, (lbaint_t)lba, dev_desc->lba))
 		return 0;
 
@@ -962,16 +968,24 @@ static int is_gpt_valid(struct blk_desc *dev_desc, u64 lba,
 static int find_valid_gpt(struct blk_desc *dev_desc, gpt_header *gpt_head,
 			  gpt_entry **pgpt_pte)
 {
-	if (is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA,
-			 gpt_head, pgpt_pte) != 1) {
-		printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
-		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1),
-				 gpt_head, pgpt_pte) != 1) {
+	int r;
+
+	r = is_gpt_valid(dev_desc, GPT_PRIMARY_PARTITION_TABLE_LBA, gpt_head,
+			 pgpt_pte);
+
+	if (r != 1) {
+		if (r != 2)
+			printf("%s: *** ERROR: Invalid GPT ***\n", __func__);
+
+		if (is_gpt_valid(dev_desc, (dev_desc->lba - 1), gpt_head,
+				 pgpt_pte) != 1) {
 			printf("%s: *** ERROR: Invalid Backup GPT ***\n",
 			       __func__);
 			return 0;
 		}
-		printf("%s: ***        Using Backup GPT ***\n",  __func__);
+		if (r != 2)
+			printf("%s: ***        Using Backup GPT ***\n",
+			       __func__);
 	}
 	return 1;
 }
diff --git a/include/part_efi.h b/include/part_efi.h
index 7170b61c95..eb5797af74 100644
--- a/include/part_efi.h
+++ b/include/part_efi.h
@@ -25,6 +25,8 @@
 #define EFI_PMBR_OSTYPE_EFI_GPT 0xEE
 
 #define GPT_HEADER_SIGNATURE_UBOOT 0x5452415020494645ULL
+#define GPT_HEADER_CHROMEOS_IGNORE 0x454d45524f4e4749ULL // 'IGNOREME'
+
 #define GPT_HEADER_REVISION_V1 0x00010000
 #define GPT_PRIMARY_PARTITION_TABLE_LBA 1ULL
 #define GPT_ENTRY_NUMBERS		CONFIG_EFI_PARTITION_ENTRIES_NUMBERS
-- 
2.21.0

[U-Boot] [PATCH 1/2] disk: efi: unify code for finding a valid gpt

[Simon Glass]

On Wed, 3 Apr 2019 at 06:25, Urja Rannikko <urjaman@gmail.com> wrote:
>
> There were 3 copies of the same sequence, make it into a function.
>
> Signed-off-by: Urja Rannikko <urjaman@gmail.com>
> ---
>  disk/part_efi.c | 73 +++++++++++++++++++++++--------------------------
>  1 file changed, 34 insertions(+), 39 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>

[U-Boot] [PATCH 2/2] disk: efi: ignore 'IGNOREME' GPT header found on cros eMMCs

[Simon Glass]

Hi Urja,

On Wed, 3 Apr 2019 at 06:25, Urja Rannikko <urjaman@gmail.com> wrote:
>
> Some ChromeOS devices (atleast veyron speedy) have the first 8MiB of
> the eMMC write protected and equipped with a dummy 'IGNOREME' GPT
> header - instead of spewing error messages about it, just silently
> try the backup GPT.
>
> Note: this does not touch the gpt cmd writing/verifying functions,
> those will still complain.
>
> Signed-off-by: Urja Rannikko <urjaman@gmail.com>
> ---
>  disk/part_efi.c    | 28 +++++++++++++++++++++-------
>  include/part_efi.h |  2 ++
>  2 files changed, 23 insertions(+), 7 deletions(-)

Could we add a Kconfig to control this? Other devices shouldn't have this code.

Regards,
SImon

[U-Boot] [PATCH 2/2] disk: efi: ignore 'IGNOREME' GPT header found on cros eMMCs

[Urja Rannikko]

Hi Simon,

On Wed, Apr 24, 2019 at 3:54 AM Simon Glass <sjg@chromium.org> wrote:
>
> Hi Urja,
<snip>
>
> Could we add a Kconfig to control this? Other devices shouldn't have this code.
I suppose i can do that... but I will need to know all the devices
that do this -
I'd assume atleast jerry, minnie and speedy but maybe also rk3399
chromeos devices?

I'm thinking basically an ifdef around the check in is_gpt_valid and
let gcc optimize the rest of the code (test for 2) out, and then
select the Kconfig (ummm maybe CONFIG_IGNORE_CROS_IGNOREME_GPT or do
you have better ideas?) for the appropriate boards.

-- 
Urja Rannikko

[U-Boot] [PATCH 1/2] disk: efi: unify code for finding a valid gpt

[Urja Rannikko]

Hi,

On Wed, Apr 24, 2019 at 3:55 AM Simon Glass <sjg@chromium.org> wrote:
>
> On Wed, 3 Apr 2019 at 06:25, Urja Rannikko <urjaman@gmail.com> wrote:
> >
> > There were 3 copies of the same sequence, make it into a function.
> >
> > Signed-off-by: Urja Rannikko <urjaman@gmail.com>
> > ---
> >  disk/part_efi.c | 73 +++++++++++++++++++++++--------------------------
> >  1 file changed, 34 insertions(+), 39 deletions(-)
> >
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
Thank you,
I'd just like to point out that you replied to the v1 of this series,
though v2 is identical in content (just rebased).

The actual patch that should apply is this one:
http://patchwork.ozlabs.org/patch/1084276/


-- 
Urja Rannikko

[U-Boot] [PATCH 2/2] disk: efi: ignore 'IGNOREME' GPT header found on cros eMMCs

[Simon Glass]

Hi Urja,

On Fri, 26 Apr 2019 at 04:26, Urja Rannikko <urjaman@gmail.com> wrote:
>
> Hi Simon,
>
> On Wed, Apr 24, 2019 at 3:54 AM Simon Glass <sjg@chromium.org> wrote:
> >
> > Hi Urja,
> <snip>
> >
> > Could we add a Kconfig to control this? Other devices shouldn't have this code.
> I suppose i can do that... but I will need to know all the devices
> that do this -
> I'd assume atleast jerry, minnie and speedy but maybe also rk3399
> chromeos devices?
>
> I'm thinking basically an ifdef around the check in is_gpt_valid and
> let gcc optimize the rest of the code (test for 2) out, and then

How about a new 'SUPPORT_CHROMEOS' Kconfig which implies your new
CONFIG below? You could enable it on configs/*chrome*

See if you can use

   if (IS_ENABLED(CONFIG_xx))

instead of #ifdef.

> select the Kconfig (ummm maybe CONFIG_IGNORE_CROS_IGNOREME_GPT or do

Maybe just CONFIG_CROS_GPT.

> you have better ideas?) for the appropriate boards.
>
> --
> Urja Rannikko

Regards,
Simon