Attention is currently required from: Sean Rhodes, Martin Roth, Patrick Rudolph, Jakub Czapiga, Matt DeVillier, Arthur Heymans. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/52564 )
Change subject: drivers/efi: Add EFI variable store option support ......................................................................
Patch Set 10:
(16 comments)
File src/drivers/efi/Kconfig:
https://review.coreboot.org/c/coreboot/+/52564/comment/e297b055_0fd7a171 PS10, Line 1: DRIVERS_EFI_VARIABLE_STORE
Should it select the right UDK to get the UEFI headers?
If it only needs the headers, why not include them directly? I'd avoid binding to a specific edk2 version if possible.
https://review.coreboot.org/c/coreboot/+/52564/comment/d798a79c_b4826078 PS10, Line 4: Adds a driver that is able to read and write an EFI formated
'formated' may be misspelled - perhaps 'formatted'?
Please fix.
File src/drivers/efi/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/f883c618_858dbcb7 PS10, Line 75: WA ?
https://review.coreboot.org/c/coreboot/+/52564/comment/1f84da77_92a2bbc5 PS10, Line 83: while (1) I'd prefer a for-loop to avoid going past the end of `msg`
https://review.coreboot.org/c/coreboot/+/52564/comment/6db235dc_f21391ff PS10, Line 86: (r = (c - msg[i])) Is it necessary to do this assignment in the condition?
https://review.coreboot.org/c/coreboot/+/52564/comment/ae722f5f_e17a9dd5 PS10, Line 90: ; not needed
https://review.coreboot.org/c/coreboot/+/52564/comment/101f768c_742b45a5 PS10, Line 110: ; Not needed
https://review.coreboot.org/c/coreboot/+/52564/comment/41ee1f5a_8f53a2b7 PS10, Line 119: ; Not needed
https://review.coreboot.org/c/coreboot/+/52564/comment/983b11a5_9e30288a PS10, Line 208: zd zu
https://review.coreboot.org/c/coreboot/+/52564/comment/11929953_f52d7fa4 PS10, Line 271: void * Why is this a void pointer?
https://review.coreboot.org/c/coreboot/+/52564/comment/9931f163_12abb90b PS10, Line 443: args.guid = guid; : args.name = name; : args.size = size; : args.data = dest; Why not initialise the struct directly, instead of assigning each member individually?
https://review.coreboot.org/c/coreboot/+/52564/comment/3b0b533c_9129f88e PS10, Line 532: (hdr.StartId != UINT16_MAX || : hdr.State != UINT8_MAX || : hdr.DataSize != UINT32_MAX || : hdr.NameSize != UINT32_MAX || : hdr.Attributes != UINT32_MAX) Why not use `memcmp()` instead?
File tests/drivers/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/b782cc30_5d13ca8f PS10, Line 10: uint8_t #include <types.h>
https://review.coreboot.org/c/coreboot/+/52564/comment/32085437_cc73d047 PS10, Line 116: + nit: spaces around `+`
https://review.coreboot.org/c/coreboot/+/52564/comment/f4972734_b89bf296 PS10, Line 119: 89 What does this magic 89 number mean?
https://review.coreboot.org/c/coreboot/+/52564/comment/b3345253_730605ca PS10, Line 119: + nit: spaces around `+`