Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47988 )
Change subject: soc/intel/common/dmi: Add DMI driver support ......................................................................
Patch Set 4:
(6 comments)
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... File src/soc/intel/common/block/dmi/dmi.c:
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 8: /* 3 GPMR registers */ Comment can be dropped. Not much helpful.
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 8: nit: Generally, we use tabs to align all the macros.
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 10: * spaces around *
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 15: * spaces around *
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 56: base + size - 1 We might have to be careful about not overflowing here.
limit = base + (size - 1);
We should also check: if (limit < base) { printk(BIOS_ERR, "..."); return CB_ERR; }
https://review.coreboot.org/c/coreboot/+/47988/4/src/soc/intel/common/block/... PS4, Line 57: limit & ~DMI_PCR_GPMR_LIMIT_MASK Actually, this check needs to ensure that the value is 0xFFFF. Sorry, I had missed this in my comment on earlier patchset.
Example: base 0xf8000000 size 0x200000 then limit = 0xF9FFFFFF. So, when we AND with ~DMI_PCR_GPMR_LIMIT_MASK, it comes out to 0xFFFF. So, the check has to be:
if ((limit & ~DMI_PCR_GPMR_LIMIT_MASK) != 0xFFFF) { }