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

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)

/**