On 9/27/18 10:29 PM, Youness Alaoui wrote:
Thanks everyone for the responses.
So far my understanding on the task at hand is :
- Add a CONFIG to decide if we set FLOCKDN or not (and one to decide
if we lock it on the resume path?)
Maybe no config at all, see discussion of PRR34_LOCKDN below. But if,
then only one config, IMO: On the resume path FLOCKDN should always be
- Remove the chipset_lockdown devicetree config and
change the code to
always assume it's LOCKDOWN_COREBOOT (this applies to all platforms,
Yes, unless somebody objects. Please make sure to add the right
reviewers to this patch. I expect some resistance.
- remove the call to fast_spi_pr_dlock() and fix the
that function to be more accurate
Please also check the tree for code that sets PRRs. IIRC, it is used
somewhere to protect the MRC cache. I would set the discrete lock
directly after writing the region.
- Add .config CONFIG options or maybe optional
devicetree configs for
how to setup the registers so coreboot can prepare them (due to the
resume path) ?
Hmm, I would add Kconfig options for this as well. I you want to match
the configuration of your payload, it definitely doesn't belong into the
devicetree. Note that "prepare" also implies to lock them on the resume
always locks the PRRs.
Actually from what I can see, the FLOCKDN will lock a few
among which it will lock the "discrete PR locks" register, and it will
lock the PR 0, 1 and 2, but there's also a PRR34_LOCKDN bit to
separately lock the PRR 3 and 4. So technically, I could leave FLOCKDN
set, and in the payload, just set protected range in PRR3 or PRR4,
then set the PRR34_LOCKDN. Unfortunately, I can't do that because the
PRR3 and 4 were already locked with the discrete lock register (if I
remove the call to fast_spi_pr_dlock(), it should be fine for my needs
though I think, since you said swseq would also fail for protected
Ah, that PRR34_LOCKDN was only introduced with SKL. Didn't know it or
at least didn't remember it. In this case it seems best to always set
FLOCKDN in coreboot. The payload can then still protect additional
regions with PRR3/4. On the resume path, coreboot would set both FLOCKDN
and PRR34_LOCKDN, right?
Also note that
swseq is not an option anymore since Skylake (IIRC, the
ME protects the flash-cycle go bit in its default configuration). With
only hwseq, you can only access the flash chip's first status register
which makes many security features unusable. So we have to rely on the
protected regions :-/ I hope Intel is going to fix that for future plat-
forms (or with an ME update if that could add more hwseq commands).
Well, I saw the opmenu/optypes/etc.. registers still being set in
skylake (their offsets were not in the datasheet though), so I figured
swseq is still there, just undocumented. It might work on systems
where the ME is disabled. I'd have to test. I was thinking of using
swseq and add the write-register-2 opcode and use it to set SPR1 and
lock the entire flash until it gets power cycled. I don't know if that
would work during suspend, but if it does, it might help with the
resume path at least (but would prevent updating flash after a reboot
and would force user to shut down and boot). I guess I have some
trial-error to do.
The tricky part is to get access to the flash chip's second status
register. I couldn't figure any way to do that with SKL (this cost
me some weeks of work once because FSP also locked empty flash pro-
Actually, if status register remains the same through
reboot/suspend/resume, and swseq doesn't work on skylake at all, I
could just set the protect bits and prevent writing to status register
on resume path.
I don't understand, is there a lock to prevent status register writes
with hwseq? Also I though, you can't disable these status register bits
w/o a reset. Whether the flash chip is reset, depends on your board
design and power sequencing ofc.
One way would be to let coreboot decide, e.g.
prepare the configuration
and don't lock it, and let the payload lock. The payload could validate
this configuration before locking (and issue a warning if coreboot
didn't set the expected bits).
Thanks, that's a good idea, I forgot about the suspend/resume case.
This implies I need coreboot to know about what config the payload
will want to set. I'm not sure if that should go into a CONFIG or a
devicetree config. Since it might be user-configured, it makes more
sense for it to go into .config, right ?
Yes, Kconfig seems to be the right place.