Attention is currently required from: Sean Rhodes, Martin Roth, Jakub Czapiga, Matt DeVillier, Angel Pons, Arthur Heymans, Patrick Rudolph. Patrick Rudolph 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 11:
(25 comments)
File src/drivers/efi/Kconfig:
https://review.coreboot.org/c/coreboot/+/52564/comment/6923294e_98485a2c PS10, Line 4: Adds a driver that is able to read and write an EFI formated
'formated' may be misspelled - perhaps 'formatted'? […]
Done
File src/drivers/efi/Makefile.inc:
PS10:
Missing SPDX-License-Identifier
Done
File src/drivers/efi/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/3da9bf50_87dcef67 PS10, Line 25: static void print_guid(int log_level, const EFI_GUID *g) : { : printk(log_level, "GUID: "); : : for (int i = 0; i < sizeof(*g); i++) : printk(log_level, "%02x", ((uint8_t *)g)[i]); : }
nit: Should not GUID be printed to match general format? […]
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/0e619b5f_d391989e PS10, Line 75: WA
rdev_strcmp_wchar_ascii I guess? I've never seen `WA` in this context before.
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/d111200b_8ef69c60 PS10, Line 90: ;
not needed
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/3dd2cc49_53ac19db PS10, Line 110: ;
Not needed
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/b83ac055_55f0b7d8 PS10, Line 119: ;
Not needed
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/87a9e7f6_d5e7d571 PS10, Line 208: zd
zu
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/1d2d0ae0_5f0f6ed1 PS10, Line 361: else { : if (rdev_readat(rdev, &hdr, 0, sizeof(hdr)) != sizeof(hdr)) : return CB_EFI_ACCESS_ERROR; : }
These can be joined to form one `else if` block.
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/009cd77e_09905e1c PS10, Line 387: 4
nit: I think, that align value should be defined somewhere in the header file. […]
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/2d52f6f2_ed44ed82 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?
Fields are not continous.
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52564/comment/d443dee7_f988bfc7 PS10, Line 85: tests/drivers tests/drivers
No need to add tests/drivers directory two times
Done
File tests/drivers/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52564/comment/886af505_6d627daa PS10, Line 5: efi
I think, that it should be named efivars-test to match it with test file name
Done
File tests/drivers/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/dbe588aa_8bcd59f5 PS10, Line 10: uint8_t
#include <types. […]
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/14a040a8_afd8bf37 PS10, Line 62: size = sizeof(buf);
Value stored in `size` by `efi_fv_get_option()` not checked
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/27a006a7_5f32dd11 PS10, Line 67: 8
What does this "8" represent? Can it be represented as const/define?
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/918714cd_78938d54 PS10, Line 73: size = sizeof(buf);
This probably should be checked (assert_int_equal()), as it it modified by efi_fv_get_option()
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/a7d29e42_d1194d0a PS10, Line 91: "is great", : 9
I think, it would be better to store value in array and use ARRAY_SIZE() instead of static value.
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/198064f2_afcb4903 PS10, Line 111: name
Here you are using name from variable, and above it is a bare value. Please, unify it.
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/6f6262fe_872a9960 PS10, Line 111: "is awesome", 11
Same as above with value and size
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/f38c631b_1fd6ceac PS10, Line 116: +
nit: spaces around `+`
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/48157881_e07d552b PS10, Line 119: +
nit: spaces around `+`
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/d4da4308_ddb56984 PS10, Line 119: 89
What does this magic 89 number mean?
Length of the newly written option.
https://review.coreboot.org/c/coreboot/+/52564/comment/2b97e9d4_ae26e16f PS10, Line 125: size = sizeof(buf);
Same as above
Done
https://review.coreboot.org/c/coreboot/+/52564/comment/f58f9d98_6842bb4c PS10, Line 135: const struct CMUnitTest tests[] = {cmocka_unit_test(efi_test_header), : cmocka_unit_test(efi_test_noop_existing_write), : cmocka_unit_test(efi_test_new_write)};
nit: […]
Done