[coreboot] SPI controller and Lock bits

Fri Oct 5 08:39:22 CEST 2018

Am 01.10.2018 23:38 schrieb Youness Alaoui:
> On Sat, Sep 29, 2018 at 12:21 PM Nico Huber <nico.h at gmx.de> wrote:
>> 
>> 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
>> set.
>> 
>> > - Remove the chipset_lockdown devicetree config and change the code to
>> > always assume it's LOCKDOWN_COREBOOT (this applies to all platforms,
>> > right?)
>> 
>> Yes, unless somebody objects. Please make sure to add the right
>> reviewers to this patch. I expect some resistance.
>> 
> Ok, this is already done, but I want to finish/test my work before I
> send for review and I might not have time until the end of the week.
> 
>> > - remove the call to fast_spi_pr_dlock() and fix the comment above
>> > 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.
>> 
> Ok, thanks, will do.
> Note: I've also changed the fast_spi_pr_dlock() function to take 5 u8
> arguments to allow the caller to lock each prr individually instead of
> being an all or nothing.
> 
>> > - 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
>> path.
>> 
> Yeah, so far, the KConfigs I added are :
> FAST_SPI_LOCK_REGISTERS
> FAST_SPI_LOCK_REGISTERS_ON_RESUME
> FAST_SPI_LOCK_PRR34_REGISTERS
> FAST_SPI_LOCK_PRR34_REGISTERS_ON_RESUME
> FAST_SPI_PRR3_REGISTER (hex value)
> FAST_SPI_PRR4_REGISTER (hex value)
> 
> This way, a user can lock/unlock the register (since it affects more
> than just the PRRs) and they can override/set the PRR3 and PRR4 values
> and decide whether to lock them or not or only on resume, etc.. I
> think those should be enough for most use cases and it's enough for
> what I need.
> 
>> >> AIUI, FLOCKDN always locks the PRRs.
>> > Actually from what I can see, the FLOCKDN will lock a few registers,
>> > 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
>> > ranges).
>> 
>> 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?
> 
> On resume, even if coreboot sets them, the PRRs need to be filled,
> which is why I also added the hex value of the PRR in the KConfig.
> I also want to keep FLOCKDN unlocked, so Heads can set the write
> protect bits in the flash then prevent writing to the status register
> via hwseq before locking it with FLOCKDN.
> 
>> 
>> >> 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-
>> tection).
> Only way I see is through swseq, but you say it doesn't work because
> of the ME. I'd have to test if the disabled ME allows swseq to work.
> 
>> 
>> > Actually, if status register remains the same through a
>> > 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.
>> 
> Yes, see bit 11 of HSFSC register (WRSDIS). That one already has a
> Kconfig to decide if we want it locked or not.
> 
>> >
>> >> 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.
>> 
>> Nico
> Thanks!

is there already a gerrit change you are working on this? Do plan to 
support / test
a X230 flash chip's lock bits?
* MX25L6406E/MX25L6408E
* EN25QH64
* MX25L3206E

thanks,

                                           martin