Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46275 )
Change subject: cpu/intel/common: rework AES-NI locking ......................................................................
Patch Set 4:
(4 comments)
https://review.coreboot.org/c/coreboot/+/46275/3/src/cpu/intel/common/common... File src/cpu/intel/common/common_init.c:
https://review.coreboot.org/c/coreboot/+/46275/3/src/cpu/intel/common/common... PS3, Line 279: msr_unset_and_set(MSR_FEATURE_CONFIG, 0, AESNI_LOCK);
I'm not 100% sure about this. Sometimes registers that are already locked […]
It says another write is not allowed, which could mean it won't have any effect, or it could generate a #GP. The check is definitely required.
https://review.coreboot.org/c/coreboot/+/46275/4/src/cpu/intel/common/common... File src/cpu/intel/common/common_init.c:
https://review.coreboot.org/c/coreboot/+/46275/4/src/cpu/intel/common/common... PS4, Line 269: /* : * Lock AES-NI feature (MSR_FEATURE_CONFIG) to prevent unintended changes : * to the enablement state as suggested in Intel document 325384-070US. : */ This is redundant, it's copied verbatim from the header
https://review.coreboot.org/c/coreboot/+/46275/4/src/cpu/intel/common/common... PS4, Line 279: msr_unset_and_set This is where it may still be helpful to have another function like msr_set_bits, that can just call msr_unset_and_set and pass 0 as the 2nd argument.
https://review.coreboot.org/c/coreboot/+/46275/4/src/include/cpu/intel/msr.h File src/include/cpu/intel/msr.h:
https://review.coreboot.org/c/coreboot/+/46275/4/src/include/cpu/intel/msr.h... PS4, Line 9: (1 << 0) How about `BIT(0)` ? (would require #include <types.h>)
-
Tim Wawrzynczak (Code Review)