Attention is currently required from: Sean Rhodes, Martin Roth, Patrick Rudolph, Matt DeVillier, Angel Pons, Arthur Heymans. Jakub Czapiga 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:
(15 comments)
File src/drivers/efi/Makefile.inc:
PS10: Missing SPDX-License-Identifier
File src/drivers/efi/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/87aeeac7_80acbb05 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? 123e4567-e89b-12d3-a456-426614174000
https://review.coreboot.org/c/coreboot/+/52564/comment/5a2283d1_4cc514cf 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.
https://review.coreboot.org/c/coreboot/+/52564/comment/9c9bb8a9_c6c88c8d PS10, Line 387: 4 nit: I think, that align value should be defined somewhere in the header file. This would eliminate some magic values here and in test file.
File tests/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52564/comment/d5725f18_50a389cc PS10, Line 85: tests/drivers tests/drivers No need to add tests/drivers directory two times
File tests/drivers/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/52564/comment/667e6cd0_3239fed8 PS10, Line 5: efi I think, that it should be named efivars-test to match it with test file name
File tests/drivers/efivars.c:
https://review.coreboot.org/c/coreboot/+/52564/comment/be2d20ea_1bc0249c PS10, Line 11: /* Firmware volume */ : 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, : 0x00, 0x00, 0x8d, 0x2b, 0xf1, 0xff, 0x96, 0x76, 0x8b, 0x4c, 0xa9, 0x85, 0x27, 0x47, : 0x07, 0x5b, 0x4f, 0x50, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x5f, 0x46, : 0x56, 0x48, 0x36, 0x0e, 0x00, 0x00, 0x48, 0x00, 0x00, 0xfa, 0x00, 0x00, 0x00, 0x02, : 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, : 0x00, 0x00, : /* Variable Info Header */ : 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43, 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, : 0x77, 0x92, 0xb8, 0xff, 0x00, 0x00, 0x5a, 0xfe, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, : /* First entry: coreboot, data: "is great" */ : 0xaa, 0x55, 0x3f, 0xff, 0x07, 0x00, 0x00, 0x00, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, : 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, : 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0x12, 0x00, 0x00, 0x00, 0x09, 0x00, : 0x00, 0x00, 0x1d, 0x4c, 0xae, 0xce, 0x5b, 0x33, 0x85, 0x46, 0xa4, 0xa0, 0xfc, 0x4a, : 0x94, 0xee, 0xa0, 0x85, 0x63, 0x00, 0x6f, 0x00, 0x72, 0x00, 0x65, 0x00, 0x62, 0x00, : 0x6f, 0x00, 0x6f, 0x00, 0x74, 0x00, 0x00, 0x00, 0x69, 0x73, 0x20, 0x67, 0x72, 0x65, : 0x61, 0x74, 0x00, nit: Can you make it more descriptive?
https://review.coreboot.org/c/coreboot/+/52564/comment/64b36e38_5b81639e PS10, Line 62: size = sizeof(buf); Value stored in `size` by `efi_fv_get_option()` not checked
https://review.coreboot.org/c/coreboot/+/52564/comment/6b0b7564_7ad7e776 PS10, Line 67: 8 What does this "8" represent? Can it be represented as const/define?
https://review.coreboot.org/c/coreboot/+/52564/comment/d360e25d_89fa1b39 PS10, Line 73: size = sizeof(buf); This probably should be checked (assert_int_equal()), as it it modified by efi_fv_get_option()
https://review.coreboot.org/c/coreboot/+/52564/comment/027b097e_5145cad3 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.
https://review.coreboot.org/c/coreboot/+/52564/comment/c87229b0_c71ba7b7 PS10, Line 111: "is awesome", 11 Same as above with value and size
https://review.coreboot.org/c/coreboot/+/52564/comment/abecc8b9_265f8875 PS10, Line 111: name Here you are using name from variable, and above it is a bare value. Please, unify it.
https://review.coreboot.org/c/coreboot/+/52564/comment/9d148cec_aeff591b PS10, Line 125: size = sizeof(buf); Same as above
https://review.coreboot.org/c/coreboot/+/52564/comment/bd963683_f7d91c65 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: 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) };
-
Jakub Czapiga (Code Review)