Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/35525 )
Change subject: soc/intel/skylake: lock TCO base address on pch finalize ......................................................................
Patch Set 2:
(4 comments)
https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/skylake: lock TCO base address on pch finalize
Just curious: What is the motivation behind this locking? Is this following any particular recommend […]
from security perspective there is absolutely no reason for leaving it unlocked. see https://bugzilla.tianocore.org/show_bug.cgi?id=1457
https://review.coreboot.org/c/coreboot/+/35525/2//COMMIT_MSG@7 PS2, Line 7: soc/intel/skylake
This is applying the logic for not just skylake but all Intel SoCs using this common code.
indeed, the prefix is wrong and will be corrected
https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/tco.c:
https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... PS2, Line 67: if defined(__SIMPLE_DEVICE__) I guess _SIMPLE_DEVICE_ is wrong here... will rework that
https://review.coreboot.org/c/coreboot/+/35525/2/src/soc/intel/common/block/... PS2, Line 67: #if defined(__SIMPLE_DEVICE__) : int devfn = PCH_DEVFN_SMBUS; : pci_devfn_t dev = PCI_DEV(0, PCI_SLOT(devfn), PCI_FUNC(devfn)); : #else : struct device *dev; : dev = PCH_DEV_SMBUS; : #endif : : /* TCO base address lockdown */ : reg32 = pci_read_config32(dev, TCOCTL); : pci_write_config32(dev, TCOCTL, reg32 | TCO_BASE_LOCK);
this should be guarded by SOC_INTEL_COMMON_BLOCK_TCO_ENABLE_THROUGH_SMBUS
no, it shouldn't. The base address should be locked in any case; maybe we should guard it by COREBOOT_LOCKDOWN