[M] Change in coreboot[main]: util/ifdtool: Add new cmdline to enable GPR0 protection

[Reka Norman (Code Review)]

Attention is currently required from: Subrata Banik.

Reka Norman has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/80235?usp=email )

The change is no longer submittable: All-Comments-Resolved is unsatisfied now.

Change subject: util/ifdtool: Add new cmdline to enable GPR0 protection
......................................................................

Patch Set 3:

(15 comments)

Patchset:

PS3: 
Looks good overall, just some mostly minor comments.

File util/ifdtool/ifdtool.c:

https://review.coreboot.org/c/coreboot/+/80235/comment/5a82ee44_328e7417 :
PS3, Line 47: CSE_REGION_DATA_PARTITION_OFFSET
This applies to BPDT v1.7 (TGL onwards) only, referring to util/cbfstool/bpdt_formats/{bpdt_1_6.c,bpdt_1_7.c}. Do we want to support v1.6 too? Or just say it's unsupported on platforms older than TGL for now?

https://review.coreboot.org/c/coreboot/+/80235/comment/49f5a868_57fe9268 :
PS3, Line 1556: uint32_t gprd
nit: make `union gprd_info` public and use it here instead of manual bit shifting?

https://review.coreboot.org/c/coreboot/+/80235/comment/25f5f5b1_ebac8a00 :
PS3, Line 1610: fpsba->pchstrp[gpr0_offset] & 0x80000000
nit: use `union gprd_info` here too

https://review.coreboot.org/c/coreboot/+/80235/comment/69df3902_1e596507 :
PS3, Line 1624: FIT
FITC?

https://review.coreboot.org/c/coreboot/+/80235/comment/870c4122_b5a6d1bd :
PS3, Line 1624: FITC
FPT?

https://review.coreboot.org/c/coreboot/+/80235/comment/2157f78b_e3a4734b :
PS3, Line 1639: 	for (size_t index = 1; index <= fpt->count; index++) {
              : 		struct cse_fpt_sub_part *fpt_sub_part = (struct cse_fpt_sub_part *)(fpt + index);
This is kind of a hack which only works because `struct cse_fpt` and `struct cse_fpt_sub_part` happen to both be 32 bytes.

Can you increment fpt by sizeof(struct cse_fpt) first, then start the index at 0?

https://review.coreboot.org/c/coreboot/+/80235/comment/0ebbb4f2_1b91dfe4 :
PS3, Line 1641: &fpt_sub_part->signature[0]
nit: just `fpt_sub_part->signature` should work?

https://review.coreboot.org/c/coreboot/+/80235/comment/8c7ecd9c_e7cca50f :
PS3, Line 1648: }
Fail if FITC is not found? Right now it will silently continue and fill in an incorrect range.

https://review.coreboot.org/c/coreboot/+/80235/comment/27626216_9e206508 :
PS3, Line 1663: region.size <= 0xfff
Why 0xfff (not 0)?

https://review.coreboot.org/c/coreboot/+/80235/comment/26fc0139_c95ab583 :
PS3, Line 1712: fpsba->pchstrp[gpr0_offset] & 0x80000000
nit: use `union gprd_info` here too

https://review.coreboot.org/c/coreboot/+/80235/comment/69538403_444f6a39 :
PS3, Line 1724: union gprd_info
nit: just `union gprd`? This is more consistent with other places where we just use the register name.

https://review.coreboot.org/c/coreboot/+/80235/comment/3d220066_553ae76d :
PS3, Line 1726: start:16
I think bit 16 is actually read protect enable, referring to `union spi_bios_gpr0` in src/soc/intel/common/block/spi/spi.c.

https://review.coreboot.org/c/coreboot/+/80235/comment/689ea4ed_b7eeef40 :
PS3, Line 1733: 	/*
              : 	 * Start Address: bit 0-15 of the GPRD represents the protected region start address,
              : 	 * where bit 0-11 of the start address are assumed to be zero.
              : 	 */
nit: maybe move these comments to the struct definition, so we don't have to duplicate them here and in print_gpr0_range().

https://review.coreboot.org/c/coreboot/+/80235/comment/9a8344c6_f7322e78 :
PS3, Line 1737: 0xffff
0x7fff, based on my comment about read protect enable.