[PATCH] bootstd: cros: store partition type in an efi_guid_t

[PATCH] bootstd: cros: store partition type in an efi_guid_t

[Vincent Stehlé]

The scan_part() function uses a struct uuid to store the little-endian
partition type GUID, but this structure should be used only to contain a
big-endian UUID. Use an efi_guid_t instead.

Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---
 boot/bootmeth_cros.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c
index f015f2e1c75..1f83c14aeab 100644
--- a/boot/bootmeth_cros.c
+++ b/boot/bootmeth_cros.c
@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum,
 {
 	struct blk_desc *desc = dev_get_uclass_plat(blk);
 	struct vb2_keyblock *hdr;
-	struct uuid type;
+	efi_guid_t type;
 	ulong num_blks;
 	int ret;
 
@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum,
 
 	/* Check for kernel partition type */
 	log_debug("part %x: type=%s\n", partnum, info->type_guid);
-	if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID))
+	if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID))
 		return log_msg_ret("typ", -EINVAL);
 
 	if (memcmp(&cros_kern_type, &type, sizeof(type)))
-- 
2.43.0

Re: [PATCH] bootstd: cros: store partition type in an efi_guid_t

[Simon Glass]

On Thu, 27 Jun 2024 at 18:06, Vincent Stehlé <vincent.stehle@arm.com> wrote:
>
> The scan_part() function uses a struct uuid to store the little-endian
> partition type GUID, but this structure should be used only to contain a
> big-endian UUID. Use an efi_guid_t instead.
>
> Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Tom Rini <trini@konsulko.com>
> ---
>  boot/bootmeth_cros.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

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

Re: [PATCH] bootstd: cros: store partition type in an efi_guid_t

[Heinrich Schuchardt]

Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
>The scan_part() function uses a struct uuid to store the little-endian
>partition type GUID, but this structure should be used only to contain a
>big-endian UUID. Use an efi_guid_t instead.
>
>Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
>Cc: Simon Glass <sjg@chromium.org>
>Cc: Tom Rini <trini@konsulko.com>
>---
> boot/bootmeth_cros.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c
>index f015f2e1c75..1f83c14aeab 100644
>--- a/boot/bootmeth_cros.c
>+++ b/boot/bootmeth_cros.c
>@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum,
> {
> 	struct blk_desc *desc = dev_get_uclass_plat(blk);
> 	struct vb2_keyblock *hdr;
>-	struct uuid type;
>+	efi_guid_t type;

Does Chrome OS only support GPT partitioning?

> 	ulong num_blks;
> 	int ret;
> 
>@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum,
> 
> 	/* Check for kernel partition type */
> 	log_debug("part %x: type=%s\n", partnum, info->type_guid);
>-	if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID))
>+	if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID))
> 		return log_msg_ret("typ", -EINVAL);

struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID?

> 
> 	if (memcmp(&cros_kern_type, &type, sizeof(type)))

You could use the guidcmp() macro here.

Best regards

Heinrich

Re: [PATCH] bootstd: cros: store partition type in an efi_guid_t

[Simon Glass]

Hi,

On Thu, 27 Jun 2024 at 20:28, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >The scan_part() function uses a struct uuid to store the little-endian
> >partition type GUID, but this structure should be used only to contain a
> >big-endian UUID. Use an efi_guid_t instead.
> >
> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> >Cc: Simon Glass <sjg@chromium.org>
> >Cc: Tom Rini <trini@konsulko.com>
> >---
> > boot/bootmeth_cros.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c
> >index f015f2e1c75..1f83c14aeab 100644
> >--- a/boot/bootmeth_cros.c
> >+++ b/boot/bootmeth_cros.c
> >@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum,
> > {
> >       struct blk_desc *desc = dev_get_uclass_plat(blk);
> >       struct vb2_keyblock *hdr;
> >-      struct uuid type;
> >+      efi_guid_t type;
>
> Does Chrome OS only support GPT partitioning?

Indeed.

>
> >       ulong num_blks;
> >       int ret;
> >
> >@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum,
> >
> >       /* Check for kernel partition type */
> >       log_debug("part %x: type=%s\n", partnum, info->type_guid);
> >-      if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID))
> >+      if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID))
> >               return log_msg_ret("typ", -EINVAL);
>
> struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID?

Yes I agree, it would be nice to fix that.

>
> >
> >       if (memcmp(&cros_kern_type, &type, sizeof(type)))
>
> You could use the guidcmp() macro here.

Regards,
Simon

Re: [PATCH] bootstd: cros: store partition type in an efi_guid_t

[Vincent Stehlé]

On Thu, Jun 27, 2024 at 09:28:04PM +0200, Heinrich Schuchardt wrote:
> 

Hi Heinrich,

Thanks for your review.
My comments below.

Best regards,
Vincent.

> 
> Am 27. Juni 2024 19:06:29 MESZ schrieb "Vincent Stehlé" <vincent.stehle@arm.com>:
> >The scan_part() function uses a struct uuid to store the little-endian
> >partition type GUID, but this structure should be used only to contain a
> >big-endian UUID. Use an efi_guid_t instead.
> >
> >Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
> >Cc: Simon Glass <sjg@chromium.org>
> >Cc: Tom Rini <trini@konsulko.com>
> >---
> > boot/bootmeth_cros.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> >diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c
> >index f015f2e1c75..1f83c14aeab 100644
> >--- a/boot/bootmeth_cros.c
> >+++ b/boot/bootmeth_cros.c
> >@@ -148,7 +148,7 @@ static int scan_part(struct udevice *blk, int partnum,
> > {
> > 	struct blk_desc *desc = dev_get_uclass_plat(blk);
> > 	struct vb2_keyblock *hdr;
> >-	struct uuid type;
> >+	efi_guid_t type;
> 
> Does Chrome OS only support GPT partitioning?
> 
> > 	ulong num_blks;
> > 	int ret;
> > 
> >@@ -161,7 +161,7 @@ static int scan_part(struct udevice *blk, int partnum,
> > 
> > 	/* Check for kernel partition type */
> > 	log_debug("part %x: type=%s\n", partnum, info->type_guid);
> >-	if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID))
> >+	if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID))
> > 		return log_msg_ret("typ", -EINVAL);
> 
> struct disk_partition containing a string which is only needed in the CLI instead of the 16 byte GUID was a bad idea to start with. Shouldn't we replace it or add least add a GUID field instead of first converting to string and than back to GUID?

I had a quick look and it seems that converting all those UUIDs from strings to
binary would indeed impact many places; let's separate this longer-term effort
from this change if you agree.

> 
> > 
> > 	if (memcmp(&cros_kern_type, &type, sizeof(type)))
> 
> You could use the guidcmp() macro here.

Thanks for the tip; I will send a v2 series.

> 
> Best regards
> 
> Heinrich
> 

[PATCH v2 0/2] Respin bootstd cros patch into a series of two

[Vincent Stehlé]

Hi,

This is a respin of this patch [1] after discussion [2]. Thanks to
Simon and Heinrich for their reviews.

To use the guidcmp() function, as suggested by Heinrich, we need to
make it available to bootmeth_cros.c and I think that the cleanest way
to do that is (arguably) to move the guid helper functions to efi.h
near the efi_guid_t definition; this is why the original patch has now
become a series of two patches.

The alternative would be to include efi_loader.h from bootmeth_cros.c
but I think this does not sound "right". If this is in fact the
preferred approach just let me know and I will respin.

There is no difference in the sandbox binaries before/after this
series on Arm and on PC, and all the tests I have run on the sandbox
are unchanged.

Best regards,
Vincent.

[1] https://patchwork.ozlabs.org/project/uboot/patch/20240627170629.2696427-1-vincent.stehle@arm.com/
[2] https://lists.denx.de/pipermail/u-boot/2024-June/557588.html

Vincent Stehlé (2):
  efi: move guid helper functions to efi.h
  bootstd: cros: store partition type in an efi_guid_t

 boot/bootmeth_cros.c |  6 +++---
 include/efi.h        | 10 ++++++++++
 include/efi_loader.h | 10 ----------
 3 files changed, 13 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] efi: move guid helper functions to efi.h

[Vincent Stehlé]

Move the guidcmp() and guidcpy() functions to efi.h, near the definition of
the efi_guid_t type those functions deal with.

Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
---
 include/efi.h        | 10 ++++++++++
 include/efi_loader.h | 10 ----------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/efi.h b/include/efi.h
index c3c4b93f860..d5af2139946 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -78,6 +78,16 @@ typedef struct {
 	u8 b[16];
 } efi_guid_t __attribute__((aligned(4)));
 
+static inline int guidcmp(const void *g1, const void *g2)
+{
+	return memcmp(g1, g2, sizeof(efi_guid_t));
+}
+
+static inline void *guidcpy(void *dst, const void *src)
+{
+	return memcpy(dst, src, sizeof(efi_guid_t));
+}
+
 #define EFI_BITS_PER_LONG	(sizeof(long) * 8)
 
 /* Bit mask for EFI status code with error */
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 6c993e1a694..ca8fc0820f6 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -21,16 +21,6 @@
 struct blk_desc;
 struct jmp_buf_data;
 
-static inline int guidcmp(const void *g1, const void *g2)
-{
-	return memcmp(g1, g2, sizeof(efi_guid_t));
-}
-
-static inline void *guidcpy(void *dst, const void *src)
-{
-	return memcpy(dst, src, sizeof(efi_guid_t));
-}
-
 #if CONFIG_IS_ENABLED(EFI_LOADER)
 
 /**
-- 
2.43.0

[PATCH v2 2/2] bootstd: cros: store partition type in an efi_guid_t

[Vincent Stehlé]

The scan_part() function uses a struct uuid to store the little-endian
partition type GUID, but this structure should be used only to contain a
big-endian UUID. Use an efi_guid_t instead and use guidcmp() for the
comparison.

Suggested-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Signed-off-by: Vincent Stehlé <vincent.stehle@arm.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Tom Rini <trini@konsulko.com>
---


Changes for v2:
- Use guidcmp() for the comparison, as suggested by Heinrich.


 boot/bootmeth_cros.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/boot/bootmeth_cros.c b/boot/bootmeth_cros.c
index 645b8bed102..676f550ca25 100644
--- a/boot/bootmeth_cros.c
+++ b/boot/bootmeth_cros.c
@@ -147,7 +147,7 @@ static int scan_part(struct udevice *blk, int partnum,
 {
 	struct blk_desc *desc = dev_get_uclass_plat(blk);
 	struct vb2_keyblock *hdr;
-	struct uuid type;
+	efi_guid_t type;
 	ulong num_blks;
 	int ret;
 
@@ -160,10 +160,10 @@ static int scan_part(struct udevice *blk, int partnum,
 
 	/* Check for kernel partition type */
 	log_debug("part %x: type=%s\n", partnum, info->type_guid);
-	if (uuid_str_to_bin(info->type_guid, (u8 *)&type, UUID_STR_FORMAT_GUID))
+	if (uuid_str_to_bin(info->type_guid, type.b, UUID_STR_FORMAT_GUID))
 		return log_msg_ret("typ", -EINVAL);
 
-	if (memcmp(&cros_kern_type, &type, sizeof(type)))
+	if (guidcmp(&cros_kern_type, &type))
 		return log_msg_ret("typ", -ENOEXEC);
 
 	/* Make a buffer for the header information */
-- 
2.43.0

Re: [PATCH v2 0/2] Respin bootstd cros patch into a series of two

[Tom Rini]

On Wed, 03 Jul 2024 13:37:54 +0200, Vincent Stehlé wrote:

> This is a respin of this patch [1] after discussion [2]. Thanks to
> Simon and Heinrich for their reviews.
> 
> To use the guidcmp() function, as suggested by Heinrich, we need to
> make it available to bootmeth_cros.c and I think that the cleanest way
> to do that is (arguably) to move the guid helper functions to efi.h
> near the efi_guid_t definition; this is why the original patch has now
> become a series of two patches.
> 
> [...]

Applied to u-boot/master, thanks!

-- 
Tom