[coreboot-gerrit] Change in coreboot[master]: security/intel/cbnt: Add logging

[Angel Pons (Code Review)]

Attention is currently required from: Anjaneya "Reddy" Chagam, Jonathan Zhang, Johnny Lin, Arthur Heymans, Morgan Jang, Patrick Rudolph.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/54093 )

Change subject: security/intel/cbnt: Add logging
......................................................................

Patch Set 6:

(11 comments)

File src/security/intel/cbnt/Kconfig:

https://review.coreboot.org/c/coreboot/+/54093/comment/5d3ed8f1_f82c11fa 
PS6, Line 20: config INTEL_CBNT_LOGGING
Would it make sense to use the same TXT option? This way, we avoid having two separate options.

Also, this option is currently never build-tested.

File src/security/intel/cbnt/cbnt.h:

https://review.coreboot.org/c/coreboot/+/54093/comment/27cf518c_385a68bf 
PS6, Line 3: 0xfed30000
Hmmm, but this matches TXT_PUBLIC_SPACE

File src/security/intel/cbnt/logging.c:

https://review.coreboot.org/c/coreboot/+/54093/comment/d0b004a2_5bfc216d 
PS6, Line 14: nem
nit: nem_enabled

https://review.coreboot.org/c/coreboot/+/54093/comment/6c3571a1_7c1482b6 
PS6, Line 16: tpm_succes
missing one s: tpm_succes*s*

https://review.coreboot.org/c/coreboot/+/54093/comment/24be845f_93b42ffb 
PS6, Line 32:  
no space after `*`.

https://review.coreboot.org/c/coreboot/+/54093/comment/f29b13a8_5e4add4d 
PS6, Line 103: const
This `const` is meaningless. Also, no space after `*`

https://review.coreboot.org/c/coreboot/+/54093/comment/346c2604_27287285 
PS6, Line 106: 			  *btg = "Boot Guard Error", *reserved = "Reserved";
You don't need these static variables here. String literals are already static and constant.

https://review.coreboot.org/c/coreboot/+/54093/comment/ce8c18ab_612fcfa7 
PS6, Line 124: 						 | ((uint64_t)sacm_info_msr.hi << 32)};
Why not add `lo` and `hi` to `union sacm_info`?

const union sacm_info acm_info = {
    .lo = sacm_info_msr.lo,
    .hi = sacm_info_msr.hi,
 };

Oh wait, sacm_info is a MSR. Then, simply add a `msr_t` union field:

union sacm_info_msr {
    struct {
    	uint64_t nem : 1;
    	uint64_t tpm_type : 2;
    	uint64_t tpm_succes : 1;
    	uint64_t reserved1 : 1;
    	uint64_t measured_boot : 1;
    	uint64_t verified_boot : 1;
    	uint64_t revoked : 1;
    	uint64_t : 24;
    	uint64_t btg_cap : 1;
    	uint64_t : 1;
    	uint64_t txt_cap : 1;
    	uint64_t : 29;
    };
    msr_t msr;
    uint64_t raw;
 };

Then, just initialise it directly:

const union sacm_info acm_info = {
    .msr = rdmsr(MSR_BOOT_GUARD_SACM_INFO),
 };

https://review.coreboot.org/c/coreboot/+/54093/comment/b1cedf4e_36a0eefa 
PS6, Line 126: CBNT:\tSACM INFO:
I wouldn't print this on each and every line. Just indent the lines after the first instance.

Same for the other registers

https://review.coreboot.org/c/coreboot/+/54093/comment/8f024282_7adf0afe 
PS6, Line 139: read64((void *)CBNT_BOOTSTATUS)
nit: read64p(CBNT_BOOTSTATUS)

Same story for the other MMIO reads.

https://review.coreboot.org/c/coreboot/+/54093/comment/78a4dcf1_348156a9 
PS6, Line 157: if (txt_err_valid) {
             : 		if (!btsts.txt_dis_pol) {
Join these two ifs to save one indentation level?

if (txt_err_valid && !btsts.txt_dis_pol) {